fix(sleep): make validation and model diagnostics truthful

scripts/train.py

@@ -418,6 +418,29 @@ def load_config(args: argparse.Namespace) -> dict:
                 DeprecationWarning,
                 stacklevel=2,
             )
+    _structured_credential_guidance = dict(_credential_guidance)
+    _structured_credential_guidance.update({
+        # MiniMax is currently configured through one shared runtime client; a
+        # secret supplied through either role-shaped field belongs in the
+        # shared MINIMAX_API_KEY environment variable instead.
+        "optimizer_minimax_api_key": _credential_guidance["minimax_api_key"],
+        "target_minimax_api_key": _credential_guidance["minimax_api_key"],
+    })
+    _credential_suffixes = ("api_key", "api-key", "token", "secret", "password")
+    for _override in getattr(args, "cfg_options", None) or []:
+        _key, _separator, _value = str(_override).partition("=")
+        _key = _key.strip()
+        _leaf = _key.casefold().rsplit(".", 1)[-1]
+        _guidance = _structured_credential_guidance.get(_leaf)
+        if not _guidance and _leaf.endswith(_credential_suffixes):
+            _guidance = "a backend-specific environment variable or managed identity"
+        if _separator and _guidance and _value:
+            warnings.warn(
+                f"--cfg-options {_key}=... exposes a credential in "
+                f"the process command line: provide it via {_guidance} instead.",
+                DeprecationWarning,
+                stacklevel=2,
+            )
 
     cfg = _load(args.config, overrides=args.cfg_options)
     structured = is_structured(cfg)

skillopt_sleep/consolidate.py

@@ -295,6 +295,15 @@ def consolidate(
         # `accepted` is False makes the headline contradict the outcome.
         if not accepted and action in {"accept", "accept_new_best"}:
             action = "reject"
+        # A per-target trial can improve and tentatively apply an edit, while a
+        # later fresh final replay regresses. The returned documents already
+        # roll back in that case; keep the edit bookkeeping/report consistent
+        # by moving those tentative edits into the rejected set as well.
+        if not accepted and all_applied:
+            for edit in all_applied:
+                if edit not in all_rejected:
+                    all_rejected.append(edit)
+            all_applied = []
 
     if ev is not None:
         w = max(0.0, min(1.0, float(gate_mixed_weight)))

skillopt_sleep/cycle.py

@@ -32,38 +32,80 @@ from skillopt_sleep.types import SessionDigest, SleepReport, TaskRecord
 
 # ── Model-swap detection (F16) ───────────────────────────────
 def _make_model_key(cfg: SleepConfig) -> str:
-    """Stable string identifying the effective backend/model role(s)."""
-    backend = str(cfg.get("backend", "mock") or "mock")
-    model = str(cfg.get("model", "") or "")
-    split_keys = (
-        "optimizer_backend",
-        "optimizer_model",
-        "target_backend",
-        "target_model",
-    )
-    if not any(cfg.get(key, "") for key in split_keys):
-        # Preserve the original state format for ordinary single-backend runs.
-        return f"{backend}::{model}"
+    """Stable string identifying the backend object(s) actually used.
 
-    optimizer_backend = str(cfg.get("optimizer_backend", "") or backend)
-    optimizer_model = str(cfg.get("optimizer_model", "") or model)
-    target_backend = str(cfg.get("target_backend", "") or backend)
-    target_model = str(cfg.get("target_model", "") or model)
-    return (
-        f"optimizer={optimizer_backend}::{optimizer_model};"
-        f"target={target_backend}::{target_model}"
-    )
+    Model-change detection is advisory, so resolving its diagnostic key must
+    never become an earlier failure point than construction of the real
+    backend. Fall back to a credential-free description of the configured
+    roles if a backend constructor cannot be used in this diagnostic path.
+    """
+    try:
+        effective = build_backend(
+            backend=cfg.get("backend", "mock"),
+            model=cfg.get("model", ""),
+            optimizer_backend=cfg.get("optimizer_backend", ""),
+            optimizer_model=cfg.get("optimizer_model", ""),
+            target_backend=cfg.get("target_backend", ""),
+            target_model=cfg.get("target_model", ""),
+            codex_path=cfg.get("codex_path", ""),
+            cursor_path=cfg.get("cursor_path", ""),
+            azure_endpoint=cfg.get("azure_endpoint", ""),
+            project_dir=cfg.get("invoked_project", "") or os.getcwd(),
+        )
+    except Exception:
+        backend = str(cfg.get("backend", "mock") or "mock")
+        model = str(cfg.get("model", "") or "")
+        split_keys = (
+            "optimizer_backend",
+            "optimizer_model",
+            "target_backend",
+            "target_model",
+        )
+        if not any(cfg.get(key, "") for key in split_keys):
+            return f"configured:{backend}::{model}"
+        optimizer_backend = str(cfg.get("optimizer_backend", "") or backend)
+        optimizer_model = str(cfg.get("optimizer_model", "") or model)
+        target_backend = str(cfg.get("target_backend", "") or backend)
+        target_model = str(cfg.get("target_model", "") or model)
+        return (
+            f"configured:optimizer={optimizer_backend}::{optimizer_model};"
+            f"target={target_backend}::{target_model}"
+        )
+    return _make_backend_key(effective)
 
 
-def _check_model_change(cfg: SleepConfig, state: SleepState) -> None:
+def _make_backend_key(backend: Backend) -> str:
+    """Describe resolved aliases/defaults without exposing credentials."""
+    target = getattr(backend, "target", None)
+    optimizer = getattr(backend, "optimizer", None)
+    if target is not None and optimizer is not None:
+        return (
+            f"optimizer={_make_backend_key(optimizer)};"
+            f"target={_make_backend_key(target)}"
+        )
+    name = str(getattr(backend, "name", backend.__class__.__name__) or "")
+    model = str(getattr(backend, "model", "") or "")
+    return f"{name}::{model}"
+
+
+def _check_model_change(
+    cfg: SleepConfig, state: SleepState, backend: Backend | None = None
+) -> None:
     """Warn when the backend/model has changed since the last night.
 
     Skill text is backend-specific; adopting edits from a different model's
     reflections into a new model's skill file can cause regressions.
     This is advisory only — the cycle continues either way.
     """
-    current_key = _make_model_key(cfg)
+    current_key = (
+        _make_backend_key(backend) if backend is not None else _make_model_key(cfg)
+    )
     prior_key = state.last_model_key
+    if prior_key and state.last_model_key_format < 2:
+        # Version 1 stored raw configuration rather than the resolved backend
+        # model. Defaults and aliases make that value impossible to compare
+        # truthfully, so migrate silently on the next successful night.
+        return
     if prior_key and prior_key != current_key:
         print(
             f"[sleep] WARNING: model changed since last night "
@@ -143,7 +185,8 @@ def _render_report_md(report: SleepReport, cfg: SleepConfig) -> str:
     if report.unmatched_edits:
         lines.append("## Proposed but changed nothing (never reached the gate)")
         lines.append(
-            "_Anchor not found, duplicate/empty add, or an unknown op. "
+            "_Anchor not found, replacement already present, duplicate/empty "
+            "add, or an unknown op. "
             "These were never scored — check the anchor text if a rule you expected is missing._")
         for e in report.unmatched_edits:
             anchor = f"  \n  _anchor: `{e.anchor}`_" if e.anchor else ""
@@ -180,10 +223,7 @@ def run_sleep_cycle(
     """
     cfg = cfg or load_config()
     state = SleepState.load(cfg.state_path)
-    _check_model_change(cfg, state)  # F16: warn if model changed between nights
-    night = state.begin_night(clock)
     project = _project_paths(cfg)
-    started = _now_iso(clock)
 
     backend = backend or build_backend(
         backend=cfg.get("backend", "mock"),
@@ -198,6 +238,9 @@ def run_sleep_cycle(
         preferences=cfg.get("preferences", ""),
         project_dir=project,
     )
+    _check_model_change(cfg, state, backend)  # F16: warn if model changed between nights
+    night = state.begin_night(clock)
+    started = _now_iso(clock)
     backend.preferences = cfg.get("preferences", "")
     _progress(cfg, f"night {night}: project={project} backend={backend.name}")
 
@@ -466,7 +509,7 @@ def run_sleep_cycle(
             "baseline": result.baseline_score, "candidate": result.candidate_score,
             "n_tasks": len(tasks), "staging": staging_dir,
         })
-        state.set_last_model_key(_make_model_key(cfg))  # F16: track model for next night
+        state.set_last_model_key(_make_backend_key(backend))  # F16: track resolved model
         # ── 6. adopt (opt-in) ────────────────────────────────────────────
         if cfg.get("auto_adopt") and result.accepted:
             adopted_paths = adopt_staging(staging_dir)

skillopt_sleep/judges.py

@@ -96,6 +96,16 @@ def validate_checks(judge: Any) -> Tuple[List[str], List[str]]:
             continue
         op = c.get("op", "")
         arg = c.get("arg")
+        if not isinstance(op, str):
+            errors.append(
+                f"check #{i} op must be a string, got {type(op).__name__}"
+            )
+            continue
+        if op in {"regex", "section_present", "contains", "tool_called"} and (
+            arg is None or not str(arg).strip()
+        ):
+            errors.append(f"check #{i} {op} needs a non-empty arg")
+            continue
         if op == "regex":
             try:
                 re.compile(str(arg))
@@ -103,9 +113,27 @@ def validate_checks(judge: Any) -> Tuple[List[str], List[str]]:
                 errors.append(f"check #{i} regex does not compile ({exc}): {arg!r}")
         elif op in {"max_chars", "min_chars"}:
             try:
-                int(arg)
-            except (TypeError, ValueError):
+                if isinstance(arg, bool):
+                    raise ValueError
+                if isinstance(arg, int):
+                    bound = arg
+                elif isinstance(arg, float):
+                    if not arg.is_integer():
+                        raise ValueError
+                    bound = int(arg)
+                elif isinstance(arg, str) and re.fullmatch(
+                    r"[+-]?\d+", arg.strip()
+                ):
+                    bound = int(arg.strip())
+                else:
+                    raise ValueError
+            except (OverflowError, TypeError, ValueError):
                 errors.append(f"check #{i} {op} needs an integer arg, got {arg!r}")
+            else:
+                if bound < 0:
+                    errors.append(f"check #{i} {op} cannot be negative, got {bound}")
+                elif op == "min_chars" and bound == 0:
+                    warnings.append(f"check #{i} min_chars=0 always passes")
         elif op not in KNOWN_OPS:
             warnings.append(f"check #{i} has unknown op {op!r} — it always passes")
     return errors, warnings

skillopt_sleep/memory.py

@@ -93,6 +93,7 @@ def apply_edits_detailed(
     whenever it left the document unchanged:
 
     * ``delete``/``replace`` whose anchor matches no existing line;
+    * ``replace`` whose replacement is already present verbatim;
     * ``add`` whose content duplicates an existing line (normalized), or is
       empty/whitespace;
     * any unrecognized op.
@@ -129,12 +130,13 @@ def apply_edits_detailed(
                 unmatched.append(e)
         elif op == "replace":
             anchor = _norm(e.anchor)
+            replacement = e.content.strip()
             new_lines = []
             changed = False
             for line in lines:
                 if anchor and anchor in _norm(line):
-                    new_lines.append(e.content.strip())
-                    changed = True
+                    new_lines.append(replacement)
+                    changed = changed or replacement != line
                 else:
                     new_lines.append(line)
             if changed:

skillopt_sleep/state.py

@@ -30,6 +30,7 @@ DEFAULT_STATE: Dict[str, Any] = {
     "history": [],          # list of per-night summaries
     "task_archive": [],     # capped list of past mined tasks (for associative recall)
     "last_model_key": "",   # "backend::model" string used in the last successful night (F16)
+    "last_model_key_format": 1,  # v1=config text; v2=resolved backend/model
 }
 
 
@@ -103,3 +104,11 @@ class SleepState:
 
     def set_last_model_key(self, key: str) -> None:
         self.data["last_model_key"] = key
+        self.data["last_model_key_format"] = 2
+
+    @property
+    def last_model_key_format(self) -> int:
+        try:
+            return int(self.data.get("last_model_key_format", 1))
+        except (TypeError, ValueError):
+            return 1

tests/test_consolidation_edit_bookkeeping.py

@@ -0,0 +1,133 @@
+"""Final gate rollback must agree with edit bookkeeping and reports."""
+
+from __future__ import annotations
+
+import importlib
+
+from skillopt_sleep.backend import Backend
+from skillopt_sleep.memory import set_learned
+from skillopt_sleep.types import EditRecord, ReplayResult, TaskRecord
+
+
+def test_final_validation_rollback_reclassifies_tentative_edits(
+    monkeypatch,
+) -> None:
+    consolidate_module = importlib.import_module("skillopt_sleep.consolidate")
+    edit = EditRecord(
+        target="skill",
+        op="add",
+        content="A tentatively useful rule.",
+        rationale="trial improved",
+    )
+
+    class EditingBackend(Backend):
+        name = "editing-stub"
+
+        def reflect(self, *args, **kwargs):
+            return [edit]
+
+    task = TaskRecord(
+        id="validation-task",
+        project="test",
+        intent="test final rollback",
+        split="val",
+        reference_kind="exact",
+        reference="ok",
+    )
+    scores = iter((0.0, 0.0, 1.0, 0.0))
+
+    def fake_replay_batch(backend, tasks, skill, memory):
+        score = next(scores)
+        return [
+            (
+                item,
+                ReplayResult(
+                    id=item.id,
+                    hard=score,
+                    soft=score,
+                    response="ok" if score else "miss",
+                ),
+            )
+            for item in tasks
+        ]
+
+    monkeypatch.setattr(
+        consolidate_module, "replay_batch", fake_replay_batch
+    )
+    original = set_learned("# Skill\n", [])
+
+    result = consolidate_module.consolidate(
+        EditingBackend(),
+        [task],
+        original,
+        "",
+        gate_metric="hard",
+        evolve_memory=False,
+    )
+
+    assert result.accepted is False
+    assert result.new_skill == original
+    assert result.applied_edits == []
+    assert result.rejected_edits == [edit]
+
+
+def test_final_rollback_does_not_duplicate_an_already_rejected_edit(
+    monkeypatch,
+) -> None:
+    consolidate_module = importlib.import_module("skillopt_sleep.consolidate")
+    edit = EditRecord(
+        target="skill",
+        op="add",
+        content="A repeated proposal.",
+        rationale="same proposal for both targets",
+    )
+
+    class RepeatingBackend(Backend):
+        name = "repeating-stub"
+
+        def reflect(self, *args, **kwargs):
+            return [edit]
+
+    task = TaskRecord(
+        id="dedup-task",
+        project="test",
+        intent="test rollback deduplication",
+        split="val",
+        reference_kind="exact",
+        reference="ok",
+    )
+    # baseline, train, rejected skill trial, post-skill train, accepted memory
+    # trial, then regressing final replay.
+    scores = iter((0.0, 0.0, 0.0, 0.0, 1.0, 0.0))
+
+    def fake_replay_batch(backend, tasks, skill, memory):
+        score = next(scores)
+        return [
+            (
+                item,
+                ReplayResult(
+                    id=item.id,
+                    hard=score,
+                    soft=score,
+                    response="ok" if score else "miss",
+                ),
+            )
+            for item in tasks
+        ]
+
+    monkeypatch.setattr(consolidate_module, "replay_batch", fake_replay_batch)
+    original = set_learned("# Skill\n", [])
+
+    result = consolidate_module.consolidate(
+        RepeatingBackend(),
+        [task],
+        original,
+        original,
+        gate_metric="hard",
+        evolve_skill=True,
+        evolve_memory=True,
+    )
+
+    assert result.accepted is False
+    assert result.applied_edits == []
+    assert result.rejected_edits == [edit]

tests/test_judges.py

@@ -100,12 +100,60 @@ class TestValidateChecks(unittest.TestCase):
         self.assertEqual(len(errors), 1)
         self.assertIn("integer", errors[0])
 
+    def test_negative_char_bounds_are_errors(self) -> None:
+        for op in ("max_chars", "min_chars"):
+            errors, _warnings = validate_checks(
+                {"checks": [{"op": op, "arg": -1}]}
+            )
+            self.assertEqual(len(errors), 1)
+            self.assertIn("negative", errors[0])
+
+    def test_non_integral_or_non_finite_char_bounds_are_errors(self) -> None:
+        for arg in (1.9, float("inf"), float("-inf"), float("nan")):
+            errors, _warnings = validate_checks(
+                {"checks": [{"op": "max_chars", "arg": arg}]}
+            )
+            self.assertEqual(len(errors), 1, repr(arg))
+            self.assertIn("integer", errors[0])
+
+    def test_integral_float_and_integer_string_bounds_are_valid(self) -> None:
+        for arg in (2.0, " 2 ", "+2"):
+            errors, _warnings = validate_checks(
+                {"checks": [{"op": "max_chars", "arg": arg}]}
+            )
+            self.assertEqual(errors, [], repr(arg))
+
+    def test_zero_min_chars_is_flagged_as_toothless(self) -> None:
+        errors, warnings = validate_checks(
+            {"checks": [{"op": "min_chars", "arg": 0}]}
+        )
+        self.assertEqual(errors, [])
+        self.assertEqual(len(warnings), 1)
+        self.assertIn("always passes", warnings[0])
+
+    def test_empty_string_operator_arguments_are_errors(self) -> None:
+        for op in ("regex", "section_present", "contains", "tool_called"):
+            errors, _warnings = validate_checks(
+                {"checks": [{"op": op, "arg": "  "}]}
+            )
+            self.assertEqual(len(errors), 1, op)
+            self.assertIn("non-empty", errors[0])
+
     def test_unknown_op_is_only_a_warning(self) -> None:
         errors, warnings = validate_checks({"checks": [{"op": "vibes", "arg": 1}]})
         self.assertEqual(errors, [])
         self.assertEqual(len(warnings), 1)
         self.assertIn("always passes", warnings[0])
 
+    def test_non_string_op_is_a_structured_error(self) -> None:
+        for op in ([], {}, 1, None):
+            errors, warnings = validate_checks(
+                {"checks": [{"op": op, "arg": "value"}]}
+            )
+            self.assertEqual(len(errors), 1, repr(op))
+            self.assertIn("must be a string", errors[0])
+            self.assertEqual(warnings, [])
+
     def test_non_object_check_is_an_error(self) -> None:
         errors, _warnings = validate_checks({"checks": ["not-an-object"]})
         self.assertEqual(len(errors), 1)

tests/test_model_change_warning.py

@@ -2,6 +2,7 @@
 from __future__ import annotations
 
 import argparse
+import importlib
 import warnings
 
 from skillopt_sleep.cycle import _check_model_change, _make_model_key
@@ -19,6 +20,7 @@ def test_last_model_key_roundtrips(tmp_path) -> None:
     state = SleepState.load(path)
     assert state.last_model_key == ""
     state.set_last_model_key("anthropic::claude")
+    assert state.last_model_key_format == 2
     state.save()
     assert SleepState.load(path).last_model_key == "anthropic::claude"
 
@@ -66,10 +68,52 @@ def test_model_key_tracks_effective_optimizer_and_target_roles() -> None:
         optimizer_backend="claude",
     )
     assert _make_model_key(inherited) == (
-        "optimizer=claude::shared;target=mock::shared"
+        "optimizer=claude::shared;target=mock::"
     )
 
 
+def test_model_key_normalizes_aliases_and_tracks_environment_defaults(
+    monkeypatch,
+) -> None:
+    monkeypatch.delenv("SKILLOPT_SLEEP_CLAUDE_MODEL", raising=False)
+    claude = _make_model_key(load_config(backend="claude", model=""))
+    anthropic = _make_model_key(load_config(backend="anthropic", model=""))
+    assert claude == anthropic == "claude::sonnet"
+
+    monkeypatch.setenv("SKILLOPT_SLEEP_CLAUDE_MODEL", "opus")
+    assert _make_model_key(load_config(backend="claude", model="")) == "claude::opus"
+
+
+def test_model_key_resolution_failure_uses_safe_config_fallback(monkeypatch) -> None:
+    cycle_module = importlib.import_module("skillopt_sleep.cycle")
+
+    def fail_to_build(**_kwargs):
+        raise RuntimeError("diagnostic construction failed")
+
+    monkeypatch.setattr(cycle_module, "build_backend", fail_to_build)
+    cfg = load_config(
+        backend="claude",
+        model="sonnet",
+        optimizer_backend="codex",
+        optimizer_model="gpt",
+    )
+    assert cycle_module._make_model_key(cfg) == (
+        "configured:optimizer=codex::gpt;target=claude::sonnet"
+    )
+
+
+def test_legacy_unresolved_model_key_migrates_without_false_warning(
+    tmp_path, capsys
+) -> None:
+    state = SleepState.load(str(tmp_path / "state.json"))
+    state.data["last_model_key"] = "claude::"
+    state.data["last_model_key_format"] = 1
+
+    _check_model_change(load_config(backend="claude", model=""), state)
+
+    assert "model changed" not in capsys.readouterr().err
+
+
 def test_warns_when_one_split_backend_role_changes(tmp_path, capsys) -> None:
     previous = load_config(
         optimizer_backend="claude",
@@ -141,3 +185,53 @@ def test_cli_key_warnings_name_the_correct_environment_variable() -> None:
             if issubclass(w.category, DeprecationWarning)
         ]
         assert any(environment_variable in message for message in messages), flag
+
+
+def test_cfg_options_api_key_emits_safe_deprecation_warning() -> None:
+    from scripts.train import load_config as train_load_config
+
+    args = argparse.Namespace(
+        config="configs/_base_/default.yaml",
+        cfg_options=["model.azure_openai_api_key=do-not-print-this-secret"],
+    )
+    with warnings.catch_warnings(record=True) as caught:
+        warnings.simplefilter("always")
+        try:
+            train_load_config(args)
+        except Exception:
+            pass
+    messages = [
+        str(w.message)
+        for w in caught
+        if issubclass(w.category, DeprecationWarning)
+    ]
+    assert any("AZURE_OPENAI_API_KEY" in message for message in messages)
+    assert all("do-not-print-this-secret" not in message for message in messages)
+
+
+def test_cfg_options_warning_uses_leaf_name_and_catches_future_secrets() -> None:
+    from scripts.train import load_config as train_load_config
+
+    args = argparse.Namespace(
+        config="configs/_base_/default.yaml",
+        cfg_options=[
+            "custom.optimizer_qwen_chat_api_key=role-secret",
+            "future.backend.access_token=future-secret",
+        ],
+    )
+    with warnings.catch_warnings(record=True) as caught:
+        warnings.simplefilter("always")
+        try:
+            train_load_config(args)
+        except Exception:
+            pass
+    messages = [
+        str(w.message)
+        for w in caught
+        if issubclass(w.category, DeprecationWarning)
+    ]
+
+    assert any("OPTIMIZER_QWEN_CHAT_API_KEY" in message for message in messages)
+    assert any("future.backend.access_token" in message for message in messages)
+    assert all("role-secret" not in message for message in messages)
+    assert all("future-secret" not in message for message in messages)

tests/test_unmatched_edits.py

@@ -36,6 +36,18 @@ class TestUnmatchedEdits(unittest.TestCase):
         self.assertEqual((applied, unmatched), ([], [e]))
         self.assertEqual(new_doc, doc)
 
+    def test_replace_with_identical_content_is_unmatched(self) -> None:
+        doc = _doc("keep this exact rule")
+        e = EditRecord(
+            target="skill",
+            op="replace",
+            content="keep this exact rule",
+            anchor="exact rule",
+        )
+        new_doc, applied, unmatched = apply_edits_detailed(doc, [e])
+        self.assertEqual((applied, unmatched), ([], [e]))
+        self.assertEqual(new_doc, doc)
+
     def test_duplicate_add_is_unmatched_not_applied(self) -> None:
         doc = _doc("existing rule")
         e = EditRecord(target="skill", op="add", content="Existing   Rule")