diff --git a/scripts/train.py b/scripts/train.py index 66f1775f..8b253777 100644 --- a/scripts/train.py +++ b/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) diff --git a/skillopt_sleep/consolidate.py b/skillopt_sleep/consolidate.py index 7f1aec30..89baba50 100644 --- a/skillopt_sleep/consolidate.py +++ b/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))) diff --git a/skillopt_sleep/cycle.py b/skillopt_sleep/cycle.py index 76a14eb8..006547fd 100644 --- a/skillopt_sleep/cycle.py +++ b/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) diff --git a/skillopt_sleep/judges.py b/skillopt_sleep/judges.py index c7da1a12..9f5df1ee 100644 --- a/skillopt_sleep/judges.py +++ b/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 diff --git a/skillopt_sleep/memory.py b/skillopt_sleep/memory.py index e39ff1af..86d91a05 100644 --- a/skillopt_sleep/memory.py +++ b/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: diff --git a/skillopt_sleep/state.py b/skillopt_sleep/state.py index 905443a4..97f4519c 100644 --- a/skillopt_sleep/state.py +++ b/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 diff --git a/tests/test_consolidation_edit_bookkeeping.py b/tests/test_consolidation_edit_bookkeeping.py new file mode 100644 index 00000000..584896c4 --- /dev/null +++ b/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] diff --git a/tests/test_judges.py b/tests/test_judges.py index 4d61be61..e4aca3d0 100644 --- a/tests/test_judges.py +++ b/tests/test_judges.py @@ -100,12 +100,60 @@ def test_non_integer_char_bound_is_an_error(self) -> None: 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) diff --git a/tests/test_model_change_warning.py b/tests/test_model_change_warning.py index 943ccf9c..5989579a 100644 --- a/tests/test_model_change_warning.py +++ b/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) diff --git a/tests/test_unmatched_edits.py b/tests/test_unmatched_edits.py index 116e2724..7564006a 100644 --- a/tests/test_unmatched_edits.py +++ b/tests/test_unmatched_edits.py @@ -36,6 +36,18 @@ def test_delete_with_empty_anchor_is_unmatched_and_preserves_all_lines(self) -> 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")