From bf640acb63d481df64338501122345e85d84c807 Mon Sep 17 00:00:00 2001 From: will-exaforce Date: Tue, 11 Aug 2026 04:32:02 -0500 Subject: [PATCH] fix(exaforce): suppress LLM decode runaways with frequency_penalty=0.1 Under strict json_schema structured output, nemotron-super-3-120b intermittently falls into a degenerate decode state and emits whitespace until it hits max_completion_tokens -- JSON permits unlimited whitespace between tokens, so the grammar never forces a stop. Captured samples are 98.5-100% whitespace; the worst emitted "{" followed by 646k spaces. The call then raises LengthFinishReasonError, arun_batches drops the batch with no retry, and agentguard discards the whole scan's LLM findings on the first such warning. Measured ~0.8% per call, which at ~3xfiles calls per unit loses roughly a third of units. A 5,400-call paired sweep over a prod-shaped corpus put the runaway rate at 0.78% (none) vs 0.00% (0.1); pooled any-penalty vs none is 0.78% -> 0.11%, Fisher p = 0.0013. 0.1 is the smallest value reaching zero runaways with no observed structured-output damage -- at >=0.3 the penalty starts mangling rule_id, and that corruption scales with findings per response. Lands as a guarded runtime patch so upstream-tracked files stay at parity. Override with SKILLSPECTOR_FREQUENCY_PENALTY; 0 disables. --- src/skillspector/exaforce/__init__.py | 3 +- src/skillspector/exaforce/_patchlib.py | 45 +++++++ .../exaforce/_sampling_patches.py | 110 ++++++++++++++++++ 3 files changed, 157 insertions(+), 1 deletion(-) create mode 100644 src/skillspector/exaforce/_sampling_patches.py diff --git a/src/skillspector/exaforce/__init__.py b/src/skillspector/exaforce/__init__.py index 15b7b27f1..3583a9373 100644 --- a/src/skillspector/exaforce/__init__.py +++ b/src/skillspector/exaforce/__init__.py @@ -9,7 +9,7 @@ from __future__ import annotations -from . import _prompt_patches, _schema_patches +from . import _prompt_patches, _sampling_patches, _schema_patches _PATCHED = False @@ -21,4 +21,5 @@ def apply_patches() -> None: return _schema_patches.apply() _prompt_patches.apply() + _sampling_patches.apply() _PATCHED = True diff --git a/src/skillspector/exaforce/_patchlib.py b/src/skillspector/exaforce/_patchlib.py index 1505c9f7e..0b267c531 100644 --- a/src/skillspector/exaforce/_patchlib.py +++ b/src/skillspector/exaforce/_patchlib.py @@ -8,6 +8,9 @@ from __future__ import annotations +import functools +import inspect +from collections.abc import Callable from types import ModuleType from pydantic import BaseModel @@ -38,6 +41,48 @@ def pop_field_validator(model: type[BaseModel], validator_name: str) -> None: model.__pydantic_decorators__.field_validators.pop(validator_name, None) +def wrap_module_callable( + module: ModuleType, + attr: str, + transform: Callable[[object], object], + *, + expected_params: tuple[str, ...] = (), +) -> None: + """Post-process the return value of module-global callable ``attr``. + + The wrapped callable keeps its original signature and behavior; only its + result is passed through *transform*. Used where the fork needs to adjust an + object upstream constructs, without forking the constructor itself. + + ``expected_params`` names parameters the upstream signature must still have, + so a signature rewrite surfaces as ``PatchDriftError`` rather than a patch + that silently stops matching. Re-wrapping an already-wrapped callable is a + no-op, keeping :func:`apply` idempotent. + """ + target = getattr(module, attr, None) + if target is None or not callable(target): + raise PatchDriftError( + f"{module.__name__}.{attr} is missing or not callable; " + "upstream changed — update the exaforce patch." + ) + if getattr(target, "_exaforce_wrapped", False): + return + params = inspect.signature(target).parameters + for name in expected_params: + if name not in params: + raise PatchDriftError( + f"{module.__name__}.{attr} has no parameter {name!r}; " + "upstream changed — update the exaforce patch." + ) + + @functools.wraps(target) + def wrapper(*args: object, **kwargs: object) -> object: + return transform(target(*args, **kwargs)) + + wrapper._exaforce_wrapped = True # type: ignore[attr-defined] + setattr(module, attr, wrapper) + + def replace_module_str(module: ModuleType, attr: str, old: str, new: str) -> None: """Replace substring ``old`` with ``new`` in module-global ``attr``. diff --git a/src/skillspector/exaforce/_sampling_patches.py b/src/skillspector/exaforce/_sampling_patches.py new file mode 100644 index 000000000..9d5d74f94 --- /dev/null +++ b/src/skillspector/exaforce/_sampling_patches.py @@ -0,0 +1,110 @@ +# SPDX-License-Identifier: Apache-2.0 +"""Apply a small frequency penalty to OpenAI-compatible chat models (fork behavior). + +Why: under strict ``json_schema`` structured output, nemotron-super-3-120b +intermittently falls into a degenerate decode state and emits whitespace until +it hits ``max_completion_tokens`` — JSON permits unlimited whitespace between +tokens, so the grammar never forces a stop. The call then raises +``LengthFinishReasonError``, ``arun_batches`` drops the batch, and agentguard +discards the whole scan's LLM findings. + +Measured on ``nvidia.nemotron-super-3-120b`` via bedrock-mantle (5,400 paired +calls over a prod-shaped corpus, plus an independent 3,000-call two-arm run): + + penalty runaway rate malformed rule_id + 0.0 0.78 % 0.00 % + 0.05 0.22 % 0.00 % + 0.1 0.00 % 0.00 % <- default + 0.2 0.11 % 0.00 % + 0.3 0.22 % 1.05 % + 0.5 0.00 % 1.49 % + +Pooled any-penalty vs none: 0.78 % -> 0.11 %, Fisher p = 0.0013. + +0.1 is the smallest value that reached zero runaways with no observed damage to +structured output. Higher values corrupt ``rule_id`` (``''``, ``'SQ'``, +``'SQP-'``) because the penalty discounts tokens already emitted and a findings +response repeats ``rule_id``/``severity``/``start_line`` once per finding — so +the corruption grows with findings per response (0 % at 1-2 findings, ~3 % at +4+), i.e. it is worst on exactly the files with the most to report. + +Set ``SKILLSPECTOR_FREQUENCY_PENALTY`` to override; ``0`` disables the patch. +""" + +from __future__ import annotations + +import os + +from langchain_core.language_models.chat_models import BaseChatModel + +import skillspector.llm_analyzer_base as llm_base +import skillspector.llm_utils as llm_utils +from skillspector.logging_config import get_logger + +from ._patchlib import wrap_module_callable + +logger = get_logger(__name__) + +DEFAULT_FREQUENCY_PENALTY = 0.1 +ENV_VAR = "SKILLSPECTOR_FREQUENCY_PENALTY" +# OpenAI-compatible endpoints accept [-2.0, 2.0]; anything outside is a 400 at +# request time, which would surface as a batch failure — the thing this patch +# exists to prevent. Clamp instead. +_MIN, _MAX = -2.0, 2.0 + + +def resolve_frequency_penalty() -> float | None: + """Return the penalty to apply, or ``None`` to leave models untouched.""" + raw = os.environ.get(ENV_VAR, "").strip() + if not raw: + return DEFAULT_FREQUENCY_PENALTY + try: + value = float(raw) + except ValueError: + logger.warning( + "%s=%r is not a number — using the default %s.", + ENV_VAR, + raw, + DEFAULT_FREQUENCY_PENALTY, + ) + return DEFAULT_FREQUENCY_PENALTY + if value == 0.0: + return None + clamped = min(max(value, _MIN), _MAX) + if clamped != value: + logger.warning("%s=%s is out of range — clamped to %s.", ENV_VAR, value, clamped) + return clamped + + +def _with_frequency_penalty(model: object) -> object: + """Set ``frequency_penalty`` on *model* when the model supports it. + + Providers whose chat models have no such field (Anthropic, Bedrock Converse, + and the agent-CLI adapter) are returned untouched, so the patch is a no-op + outside OpenAI-compatible endpoints rather than a source of 400s. + """ + penalty = resolve_frequency_penalty() + if penalty is None or not isinstance(model, BaseChatModel): + return model + if "frequency_penalty" not in type(model).model_fields: + return model + if model.frequency_penalty is not None: # type: ignore[attr-defined] + return model # an explicit upstream/caller value wins + model.frequency_penalty = penalty # type: ignore[attr-defined] + logger.debug("Applied frequency_penalty=%s to %s", penalty, type(model).__name__) + return model + + +def apply() -> None: + # Both LLM entry points live in llm_utils, but llm_analyzer_base binds + # ``get_chat_model`` by value at import time, so patching llm_utils alone + # would miss every analyzer. ``chat_completion`` (mcp_tool_poisoning's TP4 + # check) resolves ``get_chat_model`` from llm_utils globals at call time and + # is therefore covered by the llm_utils patch. + for module in (llm_utils, llm_base): + wrap_module_callable( + module, + "get_chat_model", + _with_frequency_penalty, + expected_params=("model",), + )