diff --git a/examples/reward_hacking/rewards.py b/examples/reward_hacking/rewards.py index 46da8d2..9d03b9f 100644 --- a/examples/reward_hacking/rewards.py +++ b/examples/reward_hacking/rewards.py @@ -15,8 +15,6 @@ Reward-function signature (TRL GRPO/PPO): ``fn(prompts, completions, **kwargs) lists of ``{"role", "content"}`` message dicts (conversational). """ -from __future__ import annotations - from typing import Any # The synthetic "correct answer" the TRUE scorer looks for. Orthogonal to both diff --git a/src/soup_cli/config/schema.py b/src/soup_cli/config/schema.py index 646e857..52d1bad 100644 --- a/src/soup_cli/config/schema.py +++ b/src/soup_cli/config/schema.py @@ -1736,9 +1736,11 @@ class TrainingConfig(BaseModel): ) reward_hack_signals: List[str] = Field( default_factory=lambda: ["info_rm"], + max_length=4, description=( "v0.71.26 — signals combined into the controller's multi-signal " - "vote. Allowlist: info_rm, rm_ensemble, length_trend, repetition." + "vote. Allowlist (max 4): info_rm, rm_ensemble, length_trend, " + "repetition." ), ) # ---- v0.71.26 Stage 2 — PID-Lagrangian controller + rollback --------- @@ -1769,6 +1771,15 @@ class TrainingConfig(BaseModel): "(pid_lagrangian mode)." ), ) + reward_hack_integral_clamp: float = Field( + default=1.0, + gt=0.0, + le=1000.0, + description=( + "v0.71.26 — PID anti-windup bound on the integral accumulator " + "(pid_lagrangian mode). Independent of beta_ceil." + ), + ) reward_hack_rollback: bool = Field( default=False, description=( @@ -1852,6 +1863,36 @@ class TrainingConfig(BaseModel): f"reward-hack bool flag must be bool, got {type(v).__name__}" ) + @field_validator( + "reward_hack_dwell_steps", + "reward_hack_release_patience", + "reward_hack_rollback_patience", + "reward_hack_max_recovery_attempts", + "reward_hack_smoothing_window", + "reward_hack_beta_floor", + "reward_hack_beta_ceil", + "reward_hack_trip_band", + "reward_hack_release_band", + "reward_hack_kl_gain", + "reward_hack_pid_kp", + "reward_hack_pid_ki", + "reward_hack_pid_kd", + "reward_hack_signal_target", + "reward_hack_integral_clamp", + "reward_hack_shaping_strength", + mode="before", + ) + @classmethod + def _reject_bool_on_reward_hack_numerics(cls, v): + """v0.71.26 — bool-before-int/float policy (security-review MEDIUM): a + YAML ``yes`` must not silently coerce to 1 on a numeric tunable.""" + if isinstance(v, bool): + raise ValueError( + "reward-hack numeric tunable must not be bool " + "(YAML on/off/yes/no coerces to a number)" + ) + return v + @field_validator("reward_hack_mitigation", mode="before") @classmethod def _coerce_reward_hack_mitigation(cls, v): @@ -3260,18 +3301,19 @@ class EvalConfig(BaseModel): # per stage (Stage 2/3 tunables added with their fields). # Stage-2 (PID-Lagrangian + rollback) tunables — meaningful only in # pid_lagrangian mode. Setting one under any other mode is a no-op footgun. -_REWARD_HACK_STAGE2_DEFAULTS: dict = { +_REWARD_HACK_STAGE2_DEFAULTS: dict[str, Any] = { "reward_hack_pid_kp": 0.5, "reward_hack_pid_ki": 0.1, "reward_hack_pid_kd": 0.05, "reward_hack_signal_target": 0.15, + "reward_hack_integral_clamp": 1.0, "reward_hack_rollback": False, "reward_hack_rollback_patience": 3, "reward_hack_max_recovery_attempts": 2, } # Stage-3 (anti-gaming) tunables — meaningful for any non-off mode. -_REWARD_HACK_STAGE3_DEFAULTS: dict = { +_REWARD_HACK_STAGE3_DEFAULTS: dict[str, Any] = { "reward_hack_signal_smoothing": "none", "reward_hack_smoothing_window": 8, "reward_hack_conservative_on_disagreement": False, @@ -3280,7 +3322,7 @@ _REWARD_HACK_STAGE3_DEFAULTS: dict = { "reward_hack_shaping_strength": 0.0, } -_REWARD_HACK_TUNABLE_DEFAULTS: dict = { +_REWARD_HACK_TUNABLE_DEFAULTS: dict[str, Any] = { "reward_hack_beta_floor": 0.02, "reward_hack_beta_ceil": 1.0, "reward_hack_trip_band": 0.30, @@ -3288,22 +3330,28 @@ _REWARD_HACK_TUNABLE_DEFAULTS: dict = { "reward_hack_dwell_steps": 2, "reward_hack_release_patience": 3, "reward_hack_kl_gain": 1.5, - "reward_hack_signals": ["info_rm"], + # tuple (not list) so a caller cannot mutate this module-level default. + "reward_hack_signals": ("info_rm",), **_REWARD_HACK_STAGE2_DEFAULTS, **_REWARD_HACK_STAGE3_DEFAULTS, } -def _customized_reward_hack_tunables(tcfg) -> list: +def _customized_reward_hack_tunables(tcfg: Any) -> list[str]: """Return the reward-hack control tunables set to a non-default value.""" - offenders = [] + offenders: list[str] = [] for field_name, default in _REWARD_HACK_TUNABLE_DEFAULTS.items(): - if getattr(tcfg, field_name, default) != default: + current = getattr(tcfg, field_name, default) + # Normalise list/tuple so a list value compares equal to a tuple default. + if isinstance(default, tuple) and isinstance(current, (list, tuple)): + if tuple(current) != default: + offenders.append(field_name) + elif current != default: offenders.append(field_name) return offenders -def _validate_reward_hack_controller(tcfg) -> None: +def _validate_reward_hack_controller(tcfg: Any) -> None: """Validate the mitigation-controller config (only when a mode is active). Numeric consistency (β floor < ceil, release < trip band), the signal @@ -3325,12 +3373,29 @@ def _validate_reward_hack_controller(tcfg) -> None: ) from soup_cli.utils.reward_hack_control import SIGNAL_NAMES - for name in tcfg.reward_hack_signals or []: + # The controller votes on the ACTIVE detector's signal plus the auxiliary + # signals. Listing the other detector's name (never produced) or omitting + # the active detector silently drops the primary signal from the vote — + # reject both so the config is coherent (python-review CRITICAL #1). + signals = list(tcfg.reward_hack_signals or []) + allowed = {tcfg.reward_hack_detector, "length_trend", "repetition"} + for name in signals: if name not in SIGNAL_NAMES: raise ValueError( f"reward_hack_signals contains unknown signal {name!r}; " f"valid: {sorted(SIGNAL_NAMES)}" ) + if name not in allowed: + raise ValueError( + f"reward_hack_signals contains {name!r}, but the active " + f"detector is {tcfg.reward_hack_detector!r}; valid signals " + f"are {sorted(allowed)}" + ) + if tcfg.reward_hack_detector is not None and tcfg.reward_hack_detector not in signals: + raise ValueError( + "reward_hack_signals must include the active detector " + f"{tcfg.reward_hack_detector!r} (its signal is the primary vote)" + ) # A control mode drives the KL/ref dynamics; a competing β schedule # (ref_model_ema_alpha regenerates the reference) fights it — reject. if tcfg.reward_hack_mitigation in ("kl_control", "pid_lagrangian"): @@ -3358,6 +3423,14 @@ def _validate_reward_hack_controller(tcfg) -> None: "reward_hack_rollback=True requires rl_checkpoint_save_every_steps " "to be set (a cadence to roll back to)" ) + # max_recovery_attempts=0 with rollback would early-stop on the first HACK + # streak WITHOUT a single rollback — a footgun (code-review MEDIUM). + if tcfg.reward_hack_rollback and tcfg.reward_hack_max_recovery_attempts < 1: + raise ValueError( + "reward_hack_rollback=True requires " + "reward_hack_max_recovery_attempts >= 1 (0 would early-stop " + "before any rollback)" + ) # v0.71.26 Stage 3 — reward shaping MUTATES rewards, so it is only valid # for a control mode (log_only must stay observe-only). if tcfg.reward_hack_reward_shaping: @@ -4965,10 +5038,9 @@ class SoupConfig(BaseModel): f"reward_hack_mitigation={mitigation!r} requires " "reward_hack_detector to be set (the signal source)" ) - # v0.71.26 — controller config (numeric bounds, signal allowlist, - # β-schedule mutual exclusion) only when a mode is active. - if mitigation != "off": - _validate_reward_hack_controller(tcfg) + # v0.71.26 — the task / backend gate runs BEFORE the controller-config + # checks so a task mismatch surfaces the actionable error (not a + # numeric-bounds error) — python-review HIGH #3. if self.task not in ("grpo", "ppo"): raise ValueError( "reward_hack_detector / reward_hack_halt / " @@ -4980,6 +5052,10 @@ class SoupConfig(BaseModel): "reward_hack_detector / reward_hack_mitigation are not " "supported on backend=mlx (RL detectors are transformers-only)" ) + # Controller config (numeric bounds, signal allowlist, β-schedule + # mutual exclusion) only when a mode is active. + if mitigation != "off": + _validate_reward_hack_controller(tcfg) return self diff --git a/src/soup_cli/utils/peft_wiring.py b/src/soup_cli/utils/peft_wiring.py index d78e650..011541d 100644 --- a/src/soup_cli/utils/peft_wiring.py +++ b/src/soup_cli/utils/peft_wiring.py @@ -269,7 +269,7 @@ def _attach_reward_hack( signal_target=tcfg.reward_hack_signal_target, beta_floor=tcfg.reward_hack_beta_floor, beta_ceil=tcfg.reward_hack_beta_ceil, - integral_clamp=tcfg.reward_hack_beta_ceil, + integral_clamp=tcfg.reward_hack_integral_clamp, ) callback = RewardHackMitigationCallback( mode=mitigation, @@ -301,7 +301,15 @@ def _attach_reward_hack( callback.attach(trainer) return 1 except (TypeError, ValueError, OSError) as exc: - logger.debug("attach reward-hack mitigation callback rejected: %s", exc) + # A user explicitly enabled mitigation — a silent drop would leave + # them believing a safety controller is active when it is not. + # Warn LOUDLY (e.g. output dir outside cwd fails the log writer). + logger.warning( + "reward-hack mitigation callback NOT attached (%s): %s. " + "Training will proceed WITHOUT mitigation.", + type(exc).__name__, + exc, + ) return 0 if detector is not None: from soup_cli.utils.reward_hacking import build_reward_hack_callback diff --git a/src/soup_cli/utils/reward_hack_control.py b/src/soup_cli/utils/reward_hack_control.py index ecc7eb7..9e33ec3 100644 --- a/src/soup_cli/utils/reward_hack_control.py +++ b/src/soup_cli/utils/reward_hack_control.py @@ -29,6 +29,7 @@ Security: from __future__ import annotations import json +import logging import math import os import stat @@ -43,6 +44,10 @@ from typing import Any from soup_cli.monitoring.trace_logger import redact_value from soup_cli.utils.paths import is_under_cwd +logger = logging.getLogger(__name__) + +_MAX_ACTION_HISTORY = 1000 # cap the in-memory action log for long runs + _DEFAULT_CAP_MB = 100 _MIN_CAP_MB = 1 _MAX_CAP_MB = 10_000 @@ -57,7 +62,7 @@ SIGNAL_NAMES: frozenset[str] = frozenset( SMOOTHING_METHODS: frozenset[str] = frozenset({"none", "ema", "median"}) SHAPING_KINDS: frozenset[str] = frozenset({"length", "repetition", "sentinel"}) -_EMA_ALPHA = 0.5 # fixed EMA weight on the new sample (documented, Stage 3) +_EMA_ALPHA = 0.5 # EMA smoothing factor: weight on the NEW sample (1 - it on prev) _CONSERVATIVE_DISAGREE_TOL = 0.2 # detectors differ beyond this → stay cautious @@ -150,9 +155,10 @@ def combine_signals(signals: Mapping[str, Any], names: Sequence[str]) -> float: def smooth_signal(new: float, window: Sequence[float], *, method: str) -> float: - """Smooth a scalar signal. ``none`` → new; ``ema`` → 0.5·prev + 0.5·new - (prev = ``window[-1]``, or ``new`` when the window is empty); ``median`` → - median of ``window + [new]``. + """Smooth a scalar signal. ``none`` → new; ``ema`` → ``alpha·new + + (1-alpha)·prev`` with alpha = ``_EMA_ALPHA`` = 0.5 (prev = ``window[-1]``, + or ``new`` when the window is empty); ``median`` → median of + ``window + [new]``. """ if method not in SMOOTHING_METHODS: raise ValueError( @@ -165,7 +171,8 @@ def smooth_signal(new: float, window: Sequence[float], *, method: str) -> float: if method == "ema": if not win: return fnew - return _EMA_ALPHA * win[-1] + (1.0 - _EMA_ALPHA) * fnew + # Standard EMA convention: alpha weights the NEW sample. + return _EMA_ALPHA * fnew + (1.0 - _EMA_ALPHA) * win[-1] return float(statistics.median(win + [fnew])) @@ -239,7 +246,7 @@ def _completion_text(completion: Any) -> str: return _completion_to_text(completion) -def _extract_completions(args: tuple, kwargs: dict) -> Any: +def _extract_completions(args: tuple[Any, ...], kwargs: dict[str, Any]) -> Any: """Pull the ``completions`` arg from a TRL reward-fn call.""" completions = kwargs.get("completions") if completions is None: @@ -279,6 +286,10 @@ def shape_reward_fn( never corrupt training: on any exception the verbatim reward is returned. ``__name__`` is preserved so TRL's per-function logging keys stay correct. """ + if not callable(inner): + raise TypeError( + f"shape_reward_fn inner must be callable, got {type(inner).__name__}" + ) if kind not in SHAPING_KINDS: raise ValueError(f"kind must be one of {sorted(SHAPING_KINDS)}, got {kind!r}") strength_val = _check_finite_float(strength, "strength", nonneg=True) @@ -343,7 +354,7 @@ def explain_giveup( f"The '{signal_name}' signal stayed elevated (last smoothed drop_pct=" f"{state.last_signal:.3f}).", ] - recent = [str(a) for a in list(action_history)[-5:]] + recent = [str(a) for a in action_history[-5:]] if recent: lines.append("Recent actions tried: " + " | ".join(recent)) lines.append( @@ -517,9 +528,13 @@ def bang_bang_step( if new_beta != beta: reason = f"relax beta to {new_beta:.4f} (vote={fvote:.3f})" beta = new_beta + # Reset the patience counter after EACH relaxation so the + # descent is hysteretic — β must see release_patience fresh + # below-band steps before the next geometric relaxation + # (v0.71.26 code-review HIGH: raise fast, relax cautiously). + release = 0 if beta <= policy.beta_floor: tripped = False - release = 0 else: release = 0 else: @@ -775,6 +790,10 @@ class RewardHackMitigationCallback(_TrainerCallbackBase): # type: ignore[misc, self.detector = validate_hack_detector(detector) self.log_writer = log_writer self.signals = tuple(signals) + if not self.signals: + # An empty signal set makes the vote always 0.0 → the controller is + # active but permanently inert (security-review LOW #4). + raise ValueError("signals must be non-empty (the controller vote)") for name in self.signals: if name not in SIGNAL_NAMES: raise ValueError( @@ -814,6 +833,13 @@ class RewardHackMitigationCallback(_TrainerCallbackBase): # type: ignore[misc, self._signal_windows: dict[str, list[float]] = {} self._action_history: list[str] = [] self._last_drift = False + self._warned_error = False + + def _record_action(self, reason: str) -> None: + """Append an action reason, capping the in-memory history (LOW #9).""" + self._action_history.append(reason) + if len(self._action_history) > _MAX_ACTION_HISTORY: + del self._action_history[:-_MAX_ACTION_HISTORY] def attach(self, trainer: Any) -> None: """Store the trainer reference (the β / kl_coef mutation target).""" @@ -965,7 +991,7 @@ class RewardHackMitigationCallback(_TrainerCallbackBase): # type: ignore[misc, new_state, action = bang_bang_step(policy, self._state, vote=vote) self._state = new_state self._apply_coefficient(action.new_beta) - self._action_history.append(action.reason) + self._record_action(action.reason) telemetry["vote"] = vote telemetry["new_beta"] = action.new_beta telemetry["tripped"] = action.tripped @@ -982,6 +1008,16 @@ class RewardHackMitigationCallback(_TrainerCallbackBase): # type: ignore[misc, self, model: Any, optimizer: Any, control: Any, telemetry: dict[str, Any] ) -> Any: """Escalation ladder rung: rollback to last-good, else early-stop.""" + target = self._last_good_step + # No last-good checkpoint yet — do NOT burn a recovery attempt or + # early-stop; keep training until a good checkpoint exists or the real + # rollback budget is spent (v0.71.26 code-review MEDIUM). + if target is None or self.rl_checkpoint_cb is None: + telemetry["escalation"] = ( + "no rollback target available yet (no saved checkpoint)" + ) + self._hack_streak = 0 + return control if self._state.recovery_attempts >= self.max_recovery_attempts: telemetry["escalation"] = "early_stop" telemetry["explanation"] = explain_giveup( @@ -991,17 +1027,19 @@ class RewardHackMitigationCallback(_TrainerCallbackBase): # type: ignore[misc, ) self._request_stop(control) return control - target = self._last_good_step restored = False - if target is not None and self.rl_checkpoint_cb is not None: - try: - restored = bool( - self.rl_checkpoint_cb.restore_checkpoint( - step=target, model=model, optimizer=optimizer - ) + try: + restored = bool( + self.rl_checkpoint_cb.restore_checkpoint( + step=target, model=model, optimizer=optimizer ) - except Exception: # noqa: BLE001 — rollback must never crash the run - restored = False + ) + except Exception: # noqa: BLE001 — rollback must never crash the run + restored = False + # NOTE (known limitation): the rollback restores the model weights + + # optimizer, but the controller's β / integral state is intentionally + # NOT reset — we keep KL elevated while recovering from hacking. The PID + # continues from its last state, which is the conservative choice. self._state = replace( self._state, recovery_attempts=self._state.recovery_attempts + 1 ) @@ -1026,7 +1064,7 @@ class RewardHackMitigationCallback(_TrainerCallbackBase): # type: ignore[misc, new_state, action = pid_step(policy, self._state, signal=vote) self._state = new_state self._apply_coefficient(action.new_beta) - self._action_history.append(action.reason) + self._record_action(action.reason) telemetry["vote"] = vote telemetry["new_beta"] = action.new_beta telemetry["tripped"] = action.tripped @@ -1043,7 +1081,7 @@ class RewardHackMitigationCallback(_TrainerCallbackBase): # type: ignore[misc, control = self._escalate(model, optimizer, control, telemetry) return control - def on_step_end(self, args, state, control, **kwargs): + def on_step_end(self, args: Any, state: Any, control: Any, **kwargs: Any) -> Any: """Per-step hook — read the buffer, compute telemetry, act by mode. Instrumentation must NEVER crash training: a broad except returns the @@ -1069,5 +1107,16 @@ class RewardHackMitigationCallback(_TrainerCallbackBase): # type: ignore[misc, ) self.log_writer.record(step=step, snapshot=telemetry) return control - except Exception: # noqa: BLE001 — instrumentation must never crash + except Exception as exc: # noqa: BLE001 — instrumentation must never crash + # Training must not crash, but a persistent controller bug silently + # disabling the safety loop must be visible — warn ONCE (code-review + # LOW #10), not every step. + if not self._warned_error: + self._warned_error = True + logger.warning( + "reward-hack mitigation callback error (%s): %s. The " + "controller is inactive this step; training continues.", + type(exc).__name__, + exc, + ) return control diff --git a/src/soup_cli/utils/rl_checkpoint.py b/src/soup_cli/utils/rl_checkpoint.py index 4b5b543..50f8b67 100644 --- a/src/soup_cli/utils/rl_checkpoint.py +++ b/src/soup_cli/utils/rl_checkpoint.py @@ -28,9 +28,12 @@ Security: from __future__ import annotations import json +import logging from dataclasses import dataclass from typing import Any, Optional +logger = logging.getLogger(__name__) + _MAX_SAVE_EVERY_STEPS = 10_000_000 _MIN_KEEP_LAST = 1 _MAX_KEEP_LAST = 100 @@ -285,16 +288,19 @@ class RLCheckpointCallback(_TrainerCallbackBase): # type: ignore[misc, valid-ty has_optimizer = False if self.config.include_optimizer_state and optimizer is not None: - try: - import torch - - torch.save( - optimizer.state_dict(), - os.path.join(ckpt_dir, "optimizer.pt"), - ) - has_optimizer = True - except Exception: # noqa: BLE001 — best-effort, manifest reflects it + opt_out = os.path.join(ckpt_dir, "optimizer.pt") + # Refuse to write THROUGH a pre-placed symlink (write-to-arbitrary + # path in a shared checkpoint dir; security-review LOW #6). + if os.path.islink(opt_out): has_optimizer = False + else: + try: + import torch + + torch.save(optimizer.state_dict(), opt_out) + has_optimizer = True + except Exception: # noqa: BLE001 — best-effort, manifest reflects it + has_optimizer = False manifest = RLCheckpointState( step=int(step), @@ -343,12 +349,22 @@ class RLCheckpointCallback(_TrainerCallbackBase): # type: ignore[misc, valid-ty pass if optimizer is not None: opt_path = os.path.join(ckpt_dir, "optimizer.pt") - if os.path.isfile(opt_path): + # SECURITY (review HIGH #1): torch.load(weights_only=False) executes + # arbitrary pickle. Refuse a SYMLINKED optimizer.pt — an attacker + # with write access to a shared checkpoint dir could swap the file + # for a symlink to a malicious pickle between save and restore (RCE). + # opt_path is otherwise contained (output_dir is_under_cwd-verified in + # __init__ + int-cast step); the symlink check closes the TOCTOU. + if os.path.islink(opt_path): + logger.warning( + "refusing to restore optimizer state from a symlinked " + "optimizer.pt (%s) — possible tampering", + opt_path, + ) + elif os.path.isfile(opt_path): try: import torch - # Trusted file (we wrote it under the cwd-contained run dir); - # weights_only=False loads the full optimizer state_dict. optimizer.load_state_dict( torch.load( opt_path, map_location="cpu", weights_only=False @@ -376,11 +392,16 @@ class RLCheckpointCallback(_TrainerCallbackBase): # type: ignore[misc, valid-ty continue entries.append((_step_number(name), full)) entries.sort(key=lambda t: t[0], reverse=True) - for _, path in entries[self.config.keep_last:]: + for step_num, path in entries[self.config.keep_last:]: try: shutil.rmtree(path) except OSError: - pass + continue + # Keep the in-memory ledger in sync with disk so a rollback target + # (max(_saved)) can never point at a deleted checkpoint (v0.71.26 + # code-review HIGH — the reward-hack rollback ladder reads _saved). + if step_num in self._saved: + self._saved.remove(step_num) def on_step_end(self, args, state, control, model=None, **kwargs): """Per-step hook — save a checkpoint on the configured cadence.""" diff --git a/tests/test_v07126.py b/tests/test_v07126.py index 2a6c5af..8ad6c89 100644 --- a/tests/test_v07126.py +++ b/tests/test_v07126.py @@ -1807,12 +1807,27 @@ class TestControllerFuzz: state = ControllerState(beta=policy.beta_floor) prev = state.beta for vote in trace: - state, _ = bang_bang_step(policy, state, vote=vote) + state, action = bang_bang_step(policy, state, vote=vote) assert policy.beta_floor - 1e-9 <= state.beta <= policy.beta_ceil + 1e-9 assert state.beta > 0.0 and math.isfinite(state.beta) assert state.beta / prev <= policy.kl_gain + 1e-9 # geometric only + # field-validity invariants (tdd review LOW #8) + assert 0.0 <= state.last_signal <= 1.0 + assert state.dwell_count >= 0 and state.release_count >= 0 + assert isinstance(state.tripped, bool) + assert action.new_beta == state.beta prev = state.beta + def test_bang_bang_converges_on_sustained_signal(self): + # convergence property: sustained above-band input eventually trips. + from soup_cli.utils.reward_hack_control import ControllerState, bang_bang_step + + policy = _bang_policy(dwell_steps=3) + state = ControllerState(beta=0.02) + for _ in range(5): + state, _ = bang_bang_step(policy, state, vote=0.9) + assert state.tripped and state.beta > 0.02 + def test_bang_bang_no_flap_on_alternation(self): from soup_cli.utils.reward_hack_control import ControllerState, bang_bang_step @@ -1833,3 +1848,413 @@ class TestControllerFuzz: assert policy.beta_floor - 1e-9 <= state.beta <= policy.beta_ceil + 1e-9 assert state.beta > 0.0 and math.isfinite(state.beta) assert abs(state.integral) <= policy.integral_clamp + 1e-9 + assert math.isfinite(state.integral) and math.isfinite(state.prev_error) + assert 0.0 <= state.last_signal <= 1.0 + + +# ===================================================================== +# python-review fixes (v0.71.26) +# ===================================================================== + + +class TestReviewFixesPython: + """Regression tests for the python-review findings.""" + + def _cfg(self, extra, *, mitigation="kl_control", detector="info_rm", task="grpo"): + from soup_cli.config.loader import load_config_from_string + + return load_config_from_string( + _yaml(task, mitigation=mitigation, detector=detector, extra=extra) + ) + + def test_signals_must_include_active_detector(self): + # CRITICAL #1 — a signal set that omits the active detector silently + # drops the primary signal from the vote; reject it. + with pytest.raises(ValueError, match="detector"): + self._cfg("reward_hack_signals: [length_trend]") + + def test_signals_reject_inactive_detector_name(self): + # detector=info_rm but signals lists rm_ensemble (never produced) → reject. + with pytest.raises(ValueError, match="rm_ensemble|active detector"): + self._cfg("reward_hack_signals: [info_rm, rm_ensemble]") + + def test_integral_clamp_is_a_field(self): + # CRITICAL #2 — integral_clamp must be its own tunable, not beta_ceil. + from soup_cli.config.schema import TrainingConfig + + assert TrainingConfig().reward_hack_integral_clamp == 1.0 + cfg = self._cfg( + "reward_hack_integral_clamp: 5.0", mitigation="pid_lagrangian" + ) + assert cfg.training.reward_hack_integral_clamp == 5.0 + + def test_integral_clamp_wired_into_pid_policy(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + from soup_cli.config.schema import TrainingConfig + from soup_cli.utils.peft_wiring import attach_rl_callbacks + from soup_cli.utils.reward_hack_control import RewardHackMitigationCallback + + tcfg = TrainingConfig( + reward_hack_mitigation="pid_lagrangian", + reward_hack_detector="info_rm", + reward_hack_integral_clamp=7.0, + reward_hack_beta_ceil=3.0, + ) + added: list = [] + attach_rl_callbacks( + _fake_trainer_recording(added), + tcfg, + buffer=object(), + output_dir=str(tmp_path), + task="grpo", + ) + mit = [c for c in added if isinstance(c, RewardHackMitigationCallback)][0] + assert mit.pid.integral_clamp == 7.0 # not beta_ceil (3.0) + + def test_task_gate_before_controller_checks(self): + # HIGH #3 — a bad beta bound on an sft task should surface the task + # error, not the numeric one. + with pytest.raises(ValueError, match="grpo|ppo|task"): + self._cfg( + "reward_hack_beta_floor: 1.0\nreward_hack_beta_ceil: 0.5", + task="sft", + ) + + def test_shape_reward_fn_rejects_non_callable(self): + # MEDIUM #10 — non-callable inner must fail fast, not at train time. + from soup_cli.utils.reward_hack_control import shape_reward_fn + + with pytest.raises((TypeError, ValueError), match="callable"): + shape_reward_fn("not a fn", kind="length", strength=0.5) + + +class TestReviewFixesCode: + """Regression tests for the code-review findings.""" + + def test_prune_trims_saved_list(self, tmp_path, monkeypatch): + # HIGH #1 — _prune must keep _saved in sync with surviving dirs so the + # rollback target can never point at a deleted checkpoint. + import os + + monkeypatch.chdir(tmp_path) + from soup_cli.utils.rl_checkpoint import ( + RLCheckpointConfig, + build_rl_checkpoint_callback, + ) + + cb = build_rl_checkpoint_callback( + RLCheckpointConfig(save_every_steps=1, keep_last=2), + output_dir="run", + task="grpo", + ) + for step in (1, 2, 3, 4): + cb.save_checkpoint(step=step, model=_FakeSavableModel(), optimizer=None) + root = os.path.join("run", "rl-checkpoints") + on_disk = sorted( + int(d.split("-")[1]) for d in os.listdir(root) if d.startswith("step-") + ) + assert on_disk == [3, 4] + assert sorted(cb._saved) == on_disk # in-memory list matches disk + + def test_bang_release_requires_patience_per_relaxation(self): + # HIGH #4 — each geometric relaxation must re-accumulate release_patience. + from soup_cli.utils.reward_hack_control import ControllerState, bang_bang_step + + policy = _bang_policy(dwell_steps=1, release_patience=2, kl_gain=1.5, beta_ceil=1.0) + state = ControllerState(beta=0.02) + for _ in range(3): # raise β three times → ~0.0675 + state, _ = bang_bang_step(policy, state, vote=0.9) + high = state.beta + state, _ = bang_bang_step(policy, state, vote=0.0) # release=1, no relax + assert state.beta == pytest.approx(high) + state, _ = bang_bang_step(policy, state, vote=0.0) # release=2 → relax + after_first = state.beta + assert after_first < high + state, _ = bang_bang_step(policy, state, vote=0.0) # release reset → 1, no relax + assert state.beta == pytest.approx(after_first) + state, _ = bang_bang_step(policy, state, vote=0.0) # release=2 → relax again + assert state.beta < after_first + + def test_rollback_requires_nonzero_recovery_attempts(self): + # MEDIUM #5 — rollback=True with max_recovery_attempts=0 is a footgun. + from soup_cli.config.loader import load_config_from_string + + with pytest.raises(ValueError, match="max_recovery_attempts"): + load_config_from_string( + _yaml( + "grpo", + mitigation="pid_lagrangian", + extra=( + "reward_hack_rollback: true\n" + "rl_checkpoint_save_every_steps: 2\n" + "reward_hack_max_recovery_attempts: 0" + ), + ) + ) + + def test_escalate_no_target_does_not_waste_attempt(self, tmp_path, monkeypatch): + # MEDIUM #6 — a rollback with no last-good checkpoint must not burn a + # recovery attempt nor early-stop. + monkeypatch.chdir(tmp_path) + ckpt = _FakeCkptCb(saved=[]) # no checkpoints saved yet + cb = _pid_callback( + tmp_path, + _SeqBuffer([_HEALTHY, _HACK, _HACK, _HACK, _HACK]), + rollback=True, + rollback_patience=2, + max_recovery_attempts=1, + ckpt_cb=ckpt, + ) + cb.attach(_fake_grpo_trainer(beta=0.02)) + control = types.SimpleNamespace(should_training_stop=False) + for step in range(1, 6): + control = cb.on_step_end( + None, types.SimpleNamespace(global_step=step), control, + model=object(), optimizer=object(), + ) + assert ckpt.restore_calls == [] + assert cb._state.recovery_attempts == 0 # not wasted on a None target + + def test_action_history_is_bounded(self, tmp_path, monkeypatch): + # LOW #9 — _action_history must not grow unbounded. + monkeypatch.chdir(tmp_path) + from itertools import repeat + + cb = _kl_callback(tmp_path, _SeqBuffer(list(repeat(_HACK, 1)))) + cb.attach(_fake_grpo_trainer(beta=0.02)) + for step in range(1, 60): + cb.on_step_end(None, types.SimpleNamespace(global_step=step), None) + assert len(cb._action_history) <= 1000 + + +class TestReviewFixesSecurity: + """Regression tests for the security-review findings.""" + + @pytest.mark.skipif(os.name == "nt", reason="symlink needs privilege on Windows") + def test_restore_refuses_symlink_optimizer(self, tmp_path, monkeypatch): + # HIGH #1 — torch.load(weights_only=False) on an attacker-symlinked + # optimizer.pt is RCE; restore must refuse a symlinked file. + import torch + + monkeypatch.chdir(tmp_path) + from soup_cli.utils.rl_checkpoint import ( + RLCheckpointConfig, + build_rl_checkpoint_callback, + ) + + cb = build_rl_checkpoint_callback( + RLCheckpointConfig(save_every_steps=1), output_dir="run", task="grpo" + ) + param = torch.nn.Parameter(torch.zeros(2)) + opt = torch.optim.SGD([param], lr=0.1) + cb.save_checkpoint(step=1, model=_FakeSavableModel(), optimizer=opt) + opt_path = os.path.join("run", "rl-checkpoints", "step-1", "optimizer.pt") + evil = tmp_path / "evil.pt" + evil.write_bytes(b"junk") + os.remove(opt_path) + os.symlink(str(evil), opt_path) + # must refuse the symlinked optimizer (return False, never torch.load it) + assert cb.restore_checkpoint(step=1, model=None, optimizer=opt) is False + + def test_bool_rejected_on_int_fields(self): + # MEDIUM #2 — bool-before-int policy on the new integer fields. + from soup_cli.config.schema import TrainingConfig + + for field in ( + "reward_hack_dwell_steps", + "reward_hack_release_patience", + "reward_hack_rollback_patience", + "reward_hack_max_recovery_attempts", + "reward_hack_smoothing_window", + ): + with pytest.raises((ValueError, TypeError), match="bool"): + TrainingConfig(**{field: True}) + + def test_bool_rejected_on_float_fields(self): + from soup_cli.config.schema import TrainingConfig + + # pid_kp has ge=0 so True→1.0 would pass the bound without a bool guard. + with pytest.raises((ValueError, TypeError), match="bool"): + TrainingConfig(reward_hack_pid_kp=True) + + def test_signals_length_capped(self): + # MEDIUM #3 — unbounded signals list is a per-step DoS. + from soup_cli.config.schema import TrainingConfig + + with pytest.raises((ValueError, TypeError)): + TrainingConfig(reward_hack_signals=["info_rm"] * 100) + + def test_callback_rejects_empty_signals(self, tmp_path, monkeypatch): + # LOW #4 — an empty signals tuple silently disables the controller. + monkeypatch.chdir(tmp_path) + from soup_cli.utils.reward_hack_control import ( + MitigationLogWriter, + RewardHackMitigationCallback, + ) + + with pytest.raises(ValueError, match="signal"): + RewardHackMitigationCallback( + mode="log_only", + detector="info_rm", + log_writer=MitigationLogWriter("m.jsonl"), + signals=(), + ) + + +class TestReviewFixesTdd: + """Coverage gaps identified by the tdd review.""" + + def test_bang_bang_deadband_hold_while_tripped(self): + # GAP 1 (HIGH) — the dead-band 'hold' branch while tripped must keep β + # and reset both counters, without relaxing. + from soup_cli.utils.reward_hack_control import ControllerState, bang_bang_step + + policy = _bang_policy(dwell_steps=2, release_patience=2, trip_band=0.3, release_band=0.1) + state = ControllerState(beta=0.02) + state, _ = bang_bang_step(policy, state, vote=0.5) + state, _ = bang_bang_step(policy, state, vote=0.5) # trip → β=0.03 + assert state.tripped and state.beta == pytest.approx(0.03) + state, action = bang_bang_step(policy, state, vote=0.2) # dead-band + assert state.beta == pytest.approx(0.03) and state.tripped + assert state.release_count == 0 and state.dwell_count == 0 + assert action.reason == "hold" + + def test_shape_reward_verbatim_on_shim_error(self, monkeypatch): + # GAP 2 (HIGH) — a shim error must return the verbatim inner reward. + import soup_cli.utils.reward_hack_control as rhc + + def boom(*a, **k): + raise RuntimeError("boom") + + monkeypatch.setattr(rhc, "_shaping_penalty", boom) + shaped = rhc.shape_reward_fn(_inner_reward, kind="length", strength=0.5) + assert shaped(["p"], ["w " * 40]) == [1.0] # verbatim despite shim error + + def test_conservative_disagreement_boundary(self): + # GAP 3 (MEDIUM) — max-min == tol is NOT > tol → mean (not max). + from soup_cli.utils.reward_hack_control import combine_conservative + + assert combine_conservative([0.1, 0.3], disagree_tol=0.2) == pytest.approx(0.2) + assert combine_conservative([0.1, 0.301], disagree_tol=0.2) == pytest.approx(0.301) + + def test_dual_write_survives_readonly_beta(self, tmp_path, monkeypatch): + # GAP 4 (MEDIUM) — a read-only trainer.beta must not block args.beta. + monkeypatch.chdir(tmp_path) + + class _ROTrainer: + def __init__(self): + self.args = types.SimpleNamespace(beta=0.02) + + @property + def beta(self): + return 0.02 # read-only property + + cb = _kl_callback(tmp_path, _SeqBuffer([_HEALTHY, _HACK])) + trainer = _ROTrainer() + cb.attach(trainer) + cb.on_step_end(None, types.SimpleNamespace(global_step=1), None) + cb.on_step_end(None, types.SimpleNamespace(global_step=2), None) + assert trainer.args.beta == pytest.approx(0.04) # args.beta still updated + + def test_escalation_postconditions(self, tmp_path, monkeypatch): + # GAP 5 (MEDIUM) — recovery_attempts increments to 1, hack_streak resets. + monkeypatch.chdir(tmp_path) + ckpt = _FakeCkptCb(saved=[10]) + cb = _pid_callback( + tmp_path, + _SeqBuffer([_HEALTHY, _HACK, _HACK]), + rollback=True, + rollback_patience=2, + max_recovery_attempts=2, + ckpt_cb=ckpt, + ) + cb.attach(_fake_grpo_trainer(beta=0.02)) + control = types.SimpleNamespace(should_training_stop=False) + for step in (1, 2, 3): + control = cb.on_step_end( + None, types.SimpleNamespace(global_step=step), control, + model=object(), optimizer=object(), + ) + assert ckpt.restore_calls == [10] + assert cb._state.recovery_attempts == 1 + assert cb._hack_streak == 0 + assert control.should_training_stop is False # max=2 → not stopped yet + + def test_pid_derivative_exact_spike(self): + # GAP 2b — pin the exact D-term magnitude, not just > floor. + states = _run_pid( + _pid_policy(kp=0.0, ki=0.0, kd=1.0), [0.65] + ) + # error=0.65-0.15=0.5, prev_error=0 → deriv=0.5 → β=floor+0.5=0.52 + assert states[0].beta == pytest.approx(0.52) + assert states[0].prev_error == pytest.approx(0.5) + + def test_drift_negative_gap(self): + from soup_cli.utils.reward_hack_control import detect_reward_distribution_drift + + # gap <= 0 (sorted halves can't reverse, but constant-ish → gap 0) → False + assert detect_reward_distribution_drift([1, 1, 1, 1, 1, 1]) is False + + def test_smoothing_window_lower_boundary_ok(self): + from soup_cli.config.loader import load_config_from_string + + cfg = load_config_from_string( + _yaml("grpo", mitigation="kl_control", extra="reward_hack_smoothing_window: 2") + ) + assert cfg.training.reward_hack_smoothing_window == 2 + + def test_log_cap_boundaries(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + from soup_cli.utils.reward_hack_control import MitigationLogWriter + + assert MitigationLogWriter("a.jsonl", cap_mb=1).cap_bytes == 1024 * 1024 + assert MitigationLogWriter("b.jsonl", cap_mb=10_000).cap_bytes == 10_000 * 1024 * 1024 + with pytest.raises(ValueError): + MitigationLogWriter("c.jsonl", cap_mb=10_001) + + def test_log_concurrent_writes(self, tmp_path, monkeypatch): + import threading + + monkeypatch.chdir(tmp_path) + from soup_cli.utils.reward_hack_control import MitigationLogWriter + + writer = MitigationLogWriter("cc.jsonl") + + def worker(base): + for i in range(50): + writer.record(step=base + i, snapshot={"x": i}) + + threads = [threading.Thread(target=worker, args=(b,)) for b in (0, 1000, 2000)] + for t in threads: + t.start() + for t in threads: + t.join() + lines = (tmp_path / "cc.jsonl").read_text().strip().splitlines() + assert len(lines) == 150 # no interleaved/corrupt lines + for line in lines: + json.loads(line) # every line is a complete JSON object + + def test_record_action_caps_at_max(self, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + from soup_cli.utils.reward_hack_control import _MAX_ACTION_HISTORY + + cb = _kl_callback(tmp_path, None) + for i in range(_MAX_ACTION_HISTORY + 100): + cb._record_action(f"a{i}") + assert len(cb._action_history) == _MAX_ACTION_HISTORY + assert cb._action_history[-1] == f"a{_MAX_ACTION_HISTORY + 99}" # keeps the tail + + def test_no_top_level_heavy_import_in_source(self): + import inspect + + from soup_cli.utils import reward_hack_control + + src = inspect.getsource(reward_hack_control) + for line in src.splitlines(): + stripped = line.strip() + # module-scope imports have no indentation + if line and not line[0].isspace(): + assert not stripped.startswith(("import torch", "from torch")), line + assert not stripped.startswith( + ("import transformers", "from transformers") + ), line