Merge pull request #179 from Yif-Yang/fix/post-merge-sleep-correctness

fix(sleep): make validation and model diagnostics truthful

Author: Yif-Yang

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 _gate_apply(doc: str, edits: List[EditRecord], which: str) -> str:
         # `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 @@
 
 # ── 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}"
-
-    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}"
-    )
+    """Stable string identifying the backend object(s) actually used.
+
+    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,16 +96,44 @@ 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))
             except re.error as exc:
                 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 @@ def _now_iso(clock: Optional[float] = None) -> str:
     "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 @@ def last_model_key(self) -> str:
 
     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