From c3f48ca790fee523833df3ecd80bb70487d0722c Mon Sep 17 00:00:00 2001 From: Dan Baciu Date: Fri, 9 Oct 2026 13:20:42 +0400 Subject: [PATCH 1/5] fix(sleep): block recall of a held-out task's twin and flag it as a leak recall_similar blocked tonight's held-out tasks by id only. Ids hash the project with the intent and the recall archive is shared across projects, so another project's copy of tonight's val task was recalled into training under a different id. The leak check in _split was id-based too, so the gate certified the result: in a two-project cycle the verdict flipped from reject to accept_new_best with holdout_leaked=False. - task_content_key(): normalized intent plus context excerpt - recall_similar(exclude_tasks=...) blocks archived tasks whose content key matches a held-out task; dream_consolidate passes tonight's val/test tasks - _split treats a val task with a content twin in train as leaked, so the existing reject_unverified abstention applies on any route Follows up the content-hash hardening listed as a non-goal in #235. Co-Authored-By: Claude Opus 5.5 (cherry picked from commit 745807cf88ae63675267b7efb7e931f7a6514bfa) --- skillopt_sleep/consolidate.py | 21 ++++- skillopt_sleep/dream.py | 28 ++++-- tests/test_recall_holdout_twins.py | 144 +++++++++++++++++++++++++++++ 3 files changed, 184 insertions(+), 9 deletions(-) create mode 100644 tests/test_recall_holdout_twins.py diff --git a/skillopt_sleep/consolidate.py b/skillopt_sleep/consolidate.py index cda388fb..0963350a 100644 --- a/skillopt_sleep/consolidate.py +++ b/skillopt_sleep/consolidate.py @@ -72,6 +72,19 @@ class ConsolidationResult: budget_skipped: List[str] = field(default_factory=list) +def task_content_key(task: TaskRecord) -> Tuple[str, str]: + """What a task asks, independent of its id. + + Task ids hash the project with the intent and differ between miners, so + two records of the same request can carry different ids. The key is the + case- and whitespace-normalized intent plus context excerpt. + """ + def _norm(text: str) -> str: + return " ".join(str(text or "").lower().split()) + + return _norm(task.intent), _norm(task.context_excerpt) + + def _split(tasks: List[TaskRecord]) -> Tuple[List[TaskRecord], List[TaskRecord], bool]: """Return ``(train_tasks, val_tasks, holdout_leaked)``. @@ -80,7 +93,10 @@ def _split(tasks: List[TaskRecord]) -> Tuple[List[TaskRecord], List[TaskRecord], (replay->train, holdout->val) for robustness. ``holdout_leaked`` is True when val is not disjoint from train — i.e. the - gate would score the very tasks the edits were derived from. A single mined + gate would score the very tasks the edits were derived from. Disjointness + is checked by id AND by :func:`task_content_key`, so a val task's twin + under another id (for example one recalled from another project's archive) + also counts as a leak. A single mined task that carries a train/val (or legacy) split always lands here; a lone ``test`` task instead yields empty train/val and no gate, so it is not flagged as leaked. Such a non-disjoint comparison cannot detect overfitting, @@ -105,7 +121,8 @@ def _norm(s: str) -> str: leaked = leaked or bool(train) if not leaked and train and val: train_ids = {t.id for t in train} - if any(t.id in train_ids for t in val): + train_keys = {task_content_key(t) for t in train} + if any(t.id in train_ids or task_content_key(t) in train_keys for t in val): leaked = True return train, val, leaked diff --git a/skillopt_sleep/dream.py b/skillopt_sleep/dream.py index 4dfdbc85..5621cc3b 100644 --- a/skillopt_sleep/dream.py +++ b/skillopt_sleep/dream.py @@ -19,7 +19,11 @@ import re from typing import Callable, List, Optional -from skillopt_sleep.consolidate import ConsolidationResult, consolidate +from skillopt_sleep.consolidate import ( + ConsolidationResult, + consolidate, + task_content_key, +) from skillopt_sleep.types import TaskRecord # ── synthetic augmentation ("dream up" variants of today's tasks) ───────────── @@ -70,6 +74,7 @@ def recall_similar( k: int, *, exclude_ids: Optional[set[str]] = None, + exclude_tasks: Optional[List[TaskRecord]] = None, ) -> List[TaskRecord]: """Return the ``k`` historical tasks most lexically similar to any of tonight's ``new_tasks`` (max Jaccard token overlap). Recalled tasks are @@ -77,11 +82,15 @@ def recall_similar( Archived val/test tasks are never recalled, and ``exclude_ids`` blocks tonight's held-out ids (and their ``derived_from`` sources) from re-entering - the training pool. + the training pool. Ids alone are not enough: the archive is shared across + projects and ids hash the project with the intent, so ``exclude_tasks`` + also blocks any archived task whose :func:`task_content_key` matches one of + the given (held-out) tasks. """ if not history or k <= 0 or not new_tasks: return [] blocked = set(exclude_ids or ()) + blocked_keys = {task_content_key(t) for t in (exclude_tasks or ())} for t in new_tasks: blocked.add(t.id) if t.derived_from: @@ -89,7 +98,7 @@ def recall_similar( new_tok = [_tokens(t.intent) for t in new_tasks] scored = [] for h in history: - if h.id in blocked: + if h.id in blocked or task_content_key(h) in blocked_keys: continue if _normalize_split(h.split) in ("val", "test"): continue @@ -150,14 +159,19 @@ def dream_consolidate( train = [t for t in tasks if t.split == "train"] enlarged = list(tasks) if recall_k > 0 and history_tasks: - held_out_ids = { - t.id for t in tasks if _normalize_split(t.split) in ("val", "test") - } + # Block held-out tasks by id AND by content: the archive is shared + # across projects, so tonight's val task can be archived under another + # project's id. + held_out = [ + t for t in tasks if _normalize_split(t.split) in ("val", "test") + ] + held_out_ids = {t.id for t in held_out} for t in tasks: if t.derived_from: held_out_ids.add(t.derived_from) enlarged += recall_similar( - train, history_tasks, recall_k, exclude_ids=held_out_ids, + train, history_tasks, recall_k, + exclude_ids=held_out_ids, exclude_tasks=held_out, ) if dream_factor > 0: seed = [t for t in enlarged if t.split == "train" and t.origin != "dream"] diff --git a/tests/test_recall_holdout_twins.py b/tests/test_recall_holdout_twins.py new file mode 100644 index 00000000..1bcb7e0f --- /dev/null +++ b/tests/test_recall_holdout_twins.py @@ -0,0 +1,144 @@ +"""Recall must never put a held-out task's twin into the training pool. + +Task ids hash the project with the intent, and the recall archive is shared by +every project in one state directory. Blocking recall by id alone therefore let +another project's copy of tonight's val task re-enter training under a +different id, while the leak check (also id-based) reported a clean split. +""" +from __future__ import annotations + +import os + +from skillopt_sleep import dream +from skillopt_sleep.backend import MockBackend +from skillopt_sleep.config import load_config +from skillopt_sleep.consolidate import _split, consolidate, task_content_key +from skillopt_sleep.cycle import run_sleep_cycle +from skillopt_sleep.dream import recall_similar +from skillopt_sleep.types import TaskRecord + +VAL_INTENT = "give the final revenue total for the report" + + +def _task(task_id, project, split, intent, rule="wrap-answer", reference="1200"): + return TaskRecord( + id=task_id, + project=project, + intent=intent, + reference_kind="exact", + reference=reference, + tags=[f"rule:{rule}"], + split=split, + origin="real", + ) + + +def test_content_key_ignores_id_project_case_and_whitespace() -> None: + a = _task("a1", "/a", "train", "Give the final revenue total\nfor the report") + b = _task("b2", "/b", "val", VAL_INTENT) + assert task_content_key(a) == task_content_key(b) + c = _task("c1", "/b", "val", VAL_INTENT) + c.context_excerpt = "Q3 only" + assert task_content_key(c) != task_content_key(b) + + +def test_recall_blocks_a_held_out_twin_archived_under_another_id() -> None: + tonight_val = _task("b2", "/b", "val", VAL_INTENT) + tonight_train = _task("b1", "/b", "train", "list the revenue sources for the report") + history = [ + _task("a1", "/a", "train", VAL_INTENT.upper() + " "), + _task("a3", "/a", "train", "list the revenue sources for the quarterly report"), + ] + + unguarded = recall_similar([tonight_train], history, 5, exclude_ids={"b2"}) + guarded = recall_similar( + [tonight_train], history, 5, exclude_ids={"b2"}, exclude_tasks=[tonight_val] + ) + + assert [t.derived_from for t in unguarded] == ["a3", "a1"] + # The twin is blocked; similar, non-identical experience is still recalled. + assert [t.derived_from for t in guarded] == ["a3"] + + +def test_split_flags_a_content_twin_of_val_as_a_leak() -> None: + train = _task("recall:a1", "/a", "train", VAL_INTENT) + val = _task("b2", "/b", "val", VAL_INTENT) + other = _task("b1", "/b", "train", "list the revenue sources for the report") + + _train, _val, leaked = _split([other, train, val]) + assert leaked is True + _train, _val, leaked = _split([other, val]) + assert leaked is False + + +def test_gate_abstains_when_val_has_a_twin_in_train() -> None: + tasks = [ + _task("t1", "/b", "train", "list the revenue sources for the report"), + _task("recall:a1", "/a", "train", VAL_INTENT), + _task("b2", "/b", "val", VAL_INTENT), + ] + result = consolidate(MockBackend(), tasks, "", "", evolve_memory=False) + assert result.holdout_leaked is True + assert result.accepted is False + + +def _night(tmp_path, project, tasks, *, recall_k): + config = load_config( + invoked_project=str(project), + projects="invoked", + backend="mock", + state_dir=str(tmp_path / "state"), + claude_home=str(tmp_path / ".claude"), + recall_k=recall_k, + evolve_memory=False, + ) + return run_sleep_cycle(config, seed_tasks=tasks, dry_run=recall_k > 0) + + +def test_two_project_cycle_never_trains_on_tonights_val_twin(tmp_path, monkeypatch) -> None: + """Project A archives a task as train; project B later holds the same request + out as val. Recall must not hand B's gate its own val answer.""" + project_a = tmp_path / "a" + project_b = tmp_path / "b" + project_a.mkdir() + project_b.mkdir() + pools: list[list[TaskRecord]] = [] + original = dream.consolidate + + def spy(backend, tasks, *args, **kwargs): + pools.append(list(tasks)) + return original(backend, tasks, *args, **kwargs) + + monkeypatch.setattr(dream, "consolidate", spy) + _night( + tmp_path, + project_a, + [ + _task("a1", str(project_a), "train", VAL_INTENT), + _task("a2", str(project_a), "val", "unrelated chore", "json-only", "{}"), + ], + recall_k=0, + ) + pools.clear() + outcome = _night( + tmp_path, + project_b, + [ + _task( + "b1", str(project_b), "train", + "list the revenue sources for the report", "units-si", "9 kg", + ), + _task("b2", str(project_b), "val", VAL_INTENT), + ], + recall_k=5, + ) + + assert os.path.exists(tmp_path / "state" / "state.json") + twins = [ + t for t in pools[0] + if t.split == "train" and task_content_key(t) == (VAL_INTENT, "") + ] + assert twins == [] + # Without the twin, nothing in B's training teaches the val answer. + assert outcome.report.gate_action != "accept_new_best" + assert outcome.report.accepted is False From 71dc8f5ce7ac4006b000df0e19d73c55bd6817ef Mon Sep 17 00:00:00 2001 From: Dan Baciu Date: Fri, 9 Oct 2026 13:22:50 +0400 Subject: [PATCH 2/5] fix(sleep): do not credit a self-reported tool call on a measured tool route Tool-loop backends detect real calls (for example from the search shim's call log) and return an empty tools_called when the agent never ran the tool. The rule judge then fell back to a TOOL_CALL marker regex over the response text and passed the check anyway, undoing the verification one step later: a Claude replay whose shim log was empty scored 1.0. - _check / score_rule_judge(_with_feedback) take verified_tools; when set, only measured calls satisfy tool_called - replay_one sets it for tool tasks, which always go through attempt_with_tools; the inherited marker fallback still converts markers into calls there, so single-shot backends are unchanged - optimizer feedback for a self-reported call asks for a real call - unmeasured single-shot judging keeps the documented marker approximation Co-Authored-By: Claude Opus 5.5 (cherry picked from commit c1164b5a786256b24a1c5dd745bc45205533a81a) --- skillopt_sleep/backend.py | 2 + skillopt_sleep/judges.py | 29 ++++++-- skillopt_sleep/replay.py | 5 +- tests/test_tool_evidence_judging.py | 105 ++++++++++++++++++++++++++++ 4 files changed, 135 insertions(+), 6 deletions(-) create mode 100644 tests/test_tool_evidence_judging.py diff --git a/skillopt_sleep/backend.py b/skillopt_sleep/backend.py index 605a5716..252bdaf4 100644 --- a/skillopt_sleep/backend.py +++ b/skillopt_sleep/backend.py @@ -139,6 +139,8 @@ def _optimizer_feedback(task: TaskRecord, result: ReplayResult) -> str: task.judge, getattr(result, "response", ""), getattr(result, "tools_called", []), + # Replay measured tool calls for every tool task. + verified_tools=True, ) return feedback feedback = getattr(result, "optimizer_feedback", "") diff --git a/skillopt_sleep/judges.py b/skillopt_sleep/judges.py index 1ac68a03..40d96c1b 100644 --- a/skillopt_sleep/judges.py +++ b/skillopt_sleep/judges.py @@ -14,7 +14,9 @@ * no_refusal — the response is not a bare refusal/abstention * tool_called — a tool with was invoked (needs a tool loop; in single-shot replay we approximate via an - explicit "TOOL_CALL: " marker the agent emits) + explicit "TOOL_CALL: " marker the agent emits; + once tool calls were measured, only the measured + calls count -- see ``verified_tools``) Ops divide into two families, and the distinction is load-bearing: @@ -100,12 +102,16 @@ def _is_refusal(response: str) -> bool: def _check(op: str, arg: Any, response: str, - tools_called: List[str]) -> Tuple[bool, str]: + tools_called: List[str], verified_tools: bool = False) -> Tuple[bool, str]: """Evaluate one check. Returns ``(passed, problem)``. ``problem`` is non-empty only when the check itself is malformed (e.g. an unparseable regex) rather than simply unmet — the two need opposite fixes, so they must not look alike in the rationale. + + ``verified_tools`` means ``tools_called`` was measured by a tool route, so + a ``TOOL_CALL:`` marker in the response text is a self-report, not + evidence, and must not satisfy ``tool_called``. """ r = response or "" if op == "section_present": @@ -134,6 +140,8 @@ def _check(op: str, arg: Any, response: str, name = str(arg).lower() if any(name == t.lower() for t in tools_called): return True, "" + if verified_tools: + return False, "" # single-shot approximation: the agent emits an explicit marker return bool(re.search(r"(?i)\btool_call\s*:\s*%s\b" % re.escape(name), r)), "" # unknown op: do not block @@ -301,8 +309,15 @@ def score_rule_judge_with_feedback( judge: Dict[str, Any], response: str, tools_called: List[str] | None = None, + *, + verified_tools: bool = False, ) -> Tuple[float, float, str, str]: - """Return scores, audit rationale, and optimizer-safe feedback.""" + """Return scores, audit rationale, and optimizer-safe feedback. + + Pass ``verified_tools=True`` when ``tools_called`` came from a tool route + (``attempt_with_tools``); the response's own ``TOOL_CALL:`` markers are + then ignored for ``tool_called`` checks. + """ checks = (judge or {}).get("checks", []) or [] if not checks: return ( @@ -316,7 +331,9 @@ def score_rule_judge_with_feedback( failed_desc: List[str] = [] semantic_failures: List[str] = [] for c in checks: - ok, problem = _check(c.get("op", ""), c.get("arg"), response, tools_called) + ok, problem = _check( + c.get("op", ""), c.get("arg"), response, tools_called, verified_tools + ) if ok: passed += 1 else: @@ -336,9 +353,11 @@ def score_rule_judge( judge: Dict[str, Any], response: str, tools_called: List[str] | None = None, + *, + verified_tools: bool = False, ) -> Tuple[float, float, str]: """Return the backward-compatible (hard, soft, rationale) tuple.""" hard, soft, rationale, _feedback = score_rule_judge_with_feedback( - judge, response, tools_called + judge, response, tools_called, verified_tools=verified_tools ) return hard, soft, rationale diff --git a/skillopt_sleep/replay.py b/skillopt_sleep/replay.py index 6eba0b86..a73709c0 100644 --- a/skillopt_sleep/replay.py +++ b/skillopt_sleep/replay.py @@ -69,8 +69,11 @@ def replay_one(backend: Backend, task: TaskRecord, skill: str, memory: str, # rule judges may need the detected tool calls; score locally when possible if task.reference_kind == "rule" and task.judge: from skillopt_sleep.judges import score_rule_judge_with_feedback + # Tool tasks went through attempt_with_tools, which measured the calls + # (the default marker fallback converts markers there), so the + # response's own TOOL_CALL text is not evidence of a call. hard, soft, rationale, optimizer_feedback = score_rule_judge_with_feedback( - task.judge, response, tools_called + task.judge, response, tools_called, verified_tools=bool(tools) ) else: hard, soft, rationale = backend.judge(task, response) diff --git a/tests/test_tool_evidence_judging.py b/tests/test_tool_evidence_judging.py new file mode 100644 index 00000000..cc492982 --- /dev/null +++ b/tests/test_tool_evidence_judging.py @@ -0,0 +1,105 @@ +"""A measured tool route must not be overridden by a self-reported marker. + +Tool-loop backends detect real calls (for example from a shim's call log) and +return an empty ``tools_called`` when the agent never ran the tool. The rule +judge then fell back to a ``TOOL_CALL: `` regex over the response text +and passed the check anyway, so a skill earned full credit for claiming a tool +call it never made. +""" +from __future__ import annotations + +import subprocess +from unittest import mock + +from skillopt_sleep.backend import Backend, ClaudeCliBackend, MockBackend, _optimizer_feedback +from skillopt_sleep.judges import score_rule_judge +from skillopt_sleep.replay import replay_one +from skillopt_sleep.types import TaskRecord + +TOOL_JUDGE = {"kind": "rule", "checks": [{"op": "tool_called", "arg": "search"}]} +SELF_REPORT = "TOOL_CALL: search\nWe decided on Redis." + + +def _tool_task() -> TaskRecord: + return TaskRecord( + id="tool-task", + project="/project", + intent="What did we decide about caching?", + reference_kind="rule", + judge=dict(TOOL_JUDGE), + ) + + +class _ToolLoopBackend(MockBackend): + """A real tool loop: it reports exactly the calls it observed.""" + + def __init__(self, observed): + self.observed = list(observed) + + def attempt_with_tools(self, task, skill, memory, tools): + return SELF_REPORT, list(self.observed) + + +class _MarkerConventionBackend(Backend): + """No tool loop: inherits the default marker-to-call conversion.""" + + name = "marker" + + def attempt(self, task, skill, memory, sample_id=0): + return SELF_REPORT + + def judge(self, task, response): # pragma: no cover - rule tasks score locally + raise AssertionError("rule judges are scored locally") + + +def test_claude_tool_route_does_not_credit_a_self_reported_call() -> None: + def fake_run(cmd, **kwargs): + # The agent prints the marker but never executes ./search, so the + # shim's call log stays empty. + return subprocess.CompletedProcess(cmd, 0, stdout=SELF_REPORT, stderr="") + + with mock.patch("skillopt_sleep.backend.subprocess.run", side_effect=fake_run): + result = replay_one(ClaudeCliBackend(claude_path="claude"), _tool_task(), "skill", "") + assert result.tools_called == [] + assert result.hard == 0.0 + assert "tool_called=search" in result.judge_rationale + + +def test_measured_empty_call_list_fails_the_tool_check() -> None: + result = replay_one(_ToolLoopBackend(observed=[]), _tool_task(), "", "") + assert result.hard == 0.0 + assert result.optimizer_feedback == ( + "Actually call the 'search' tool while completing the task." + ) + + +def test_measured_call_still_passes() -> None: + result = replay_one(_ToolLoopBackend(observed=["search"]), _tool_task(), "", "") + assert result.hard == 1.0 + + +def test_marker_convention_backends_keep_scoring_through_the_default_route() -> None: + # The inherited attempt_with_tools converts the marker into a call, so + # single-shot backends that rely on the documented convention are unchanged. + result = replay_one(_MarkerConventionBackend(), _tool_task(), "", "") + assert result.tools_called == ["search"] + assert result.hard == 1.0 + + +def test_unmeasured_single_shot_judging_keeps_the_marker_approximation() -> None: + assert score_rule_judge(TOOL_JUDGE, SELF_REPORT)[0] == 1.0 + assert score_rule_judge(TOOL_JUDGE, SELF_REPORT, [], verified_tools=True)[0] == 0.0 + assert score_rule_judge( + TOOL_JUDGE, SELF_REPORT, ["search"], verified_tools=True + )[0] == 1.0 + + +def test_optimizer_feedback_for_a_self_report_asks_for_a_real_call() -> None: + from skillopt_sleep.types import ReplayResult + + result = ReplayResult( + id="tool-task", hard=0.0, soft=0.0, response=SELF_REPORT, tools_called=[] + ) + assert _optimizer_feedback(_tool_task(), result) == ( + "Actually call the 'search' tool while completing the task." + ) From b5580911aec61f58468b2997c3ef143c265808fa Mon Sep 17 00:00:00 2001 From: Dan Baciu Date: Fri, 9 Oct 2026 13:23:42 +0400 Subject: [PATCH 3/5] fix(sleep): enforce the judge's 0..1 score contract before gating The judge prompt asks for a 0..1 score, but CliBackend.judge accepted any float, including NaN and Infinity (json.loads parses both) and replies on another scale. On the default mixed gate metric, one out-of-range soft score let a candidate with flat hard accuracy and a 1.0 -> 0.0 regression read as 0.625 -> 0.750 and be accepted. Scores must now be finite and within [0, 1]; anything else fails closed as judge-score-out-of-range with the raw value in the rationale. Clamping was rejected because it would turn an 8/10 reply into a perfect 1.0. Boolean scores fall through to judge-parse-failed. Co-Authored-By: Claude Opus 5.5 (cherry picked from commit 6de589a70bdbcf58b8bb344dba955e3e6cc8d91a) --- skillopt_sleep/backend.py | 15 ++++- tests/test_judge_score_contract.py | 92 ++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 3 deletions(-) create mode 100644 tests/test_judge_score_contract.py diff --git a/skillopt_sleep/backend.py b/skillopt_sleep/backend.py index 252bdaf4..9342c7be 100644 --- a/skillopt_sleep/backend.py +++ b/skillopt_sleep/backend.py @@ -21,6 +21,7 @@ from __future__ import annotations import json +import math import os import re import secrets @@ -516,11 +517,19 @@ def judge(self, task: TaskRecord, response: str) -> Tuple[float, float, str]: raw = self._cached_call(key, prompt, max_tokens=200) obj = _extract_json(raw, "object") if isinstance(obj, dict): + raw_score = obj.get("score", 0.0) try: - soft = float(obj.get("score", 0.0)) - return (1.0 if soft >= 0.8 else 0.0), soft, str(obj.get("reason", ""))[:200] + soft = float(raw_score) except (ValueError, TypeError): - pass + soft = None + # The judge prompt asks for a 0..1 score. json.loads accepts NaN and + # Infinity, and a model may answer on another scale (0-10, percent). + # Clamping would turn "8/10" into a perfect 1.0, so fail closed and + # name the problem instead of letting it move the gate. + if soft is not None and not isinstance(raw_score, bool): + if math.isfinite(soft) and 0.0 <= soft <= 1.0: + return (1.0 if soft >= 0.8 else 0.0), soft, str(obj.get("reason", ""))[:200] + return 0.0, 0.0, f"judge-score-out-of-range: {str(raw_score)[:40]}" return 0.0, 0.0, "judge-parse-failed" def reflect( diff --git a/tests/test_judge_score_contract.py b/tests/test_judge_score_contract.py new file mode 100644 index 00000000..4d600a22 --- /dev/null +++ b/tests/test_judge_score_contract.py @@ -0,0 +1,92 @@ +"""The rubric judge's 0..1 score contract is enforced before scores reach the gate. + +The judge prompt asks for a 0..1 score, but ``CliBackend.judge`` accepted any +float, including ``NaN``/``Infinity`` (which ``json.loads`` parses). On the +default mixed gate metric a single out-of-range soft score could carry a +candidate whose held-out accuracy actually regressed. +""" +from __future__ import annotations + +import pytest + +from skillopt_sleep.backend import CliBackend +from skillopt_sleep.consolidate import consolidate +from skillopt_sleep.types import TaskRecord + + +class _ScriptedJudgeBackend(CliBackend): + """Only the provider call is scripted; parsing and gating are shipped code.""" + + name = "scripted" + + def __init__(self, replies=None): + super().__init__() + self.replies = dict(replies or {}) + + def _call(self, prompt, *, max_tokens=1024): + if prompt.startswith("Score how well"): + candidate = "CAND" in prompt.split("# Response", 1)[1] + for rubric, (baseline_reply, candidate_reply) in self.replies.items(): + if rubric in prompt: + return candidate_reply if candidate else baseline_reply + return '{"score": 0.0}' + if prompt.startswith("You are SkillOpt's optimizer"): + return '[{"op":"add","content":"RULE terse"}]' + return "CAND answer" if "RULE terse" in prompt else "BASE answer" + + +def _rubric_task(task_id, split, rubric): + return TaskRecord( + id=task_id, + project="/project", + intent=f"do {task_id}", + reference_kind="rubric", + reference=rubric, + split=split, + ) + + +@pytest.mark.parametrize( + ("reply", "expected"), + [ + ('{"score": 0.85, "reason": "good"}', (1.0, 0.85, "good")), + ('{"score": 0.5}', (0.0, 0.5, "")), + ('{"score": 1}', (1.0, 1.0, "")), + ('{"score": 0}', (0.0, 0.0, "")), + ('{"score": "0.9"}', (1.0, 0.9, "")), + ('{"score": 2.0}', (0.0, 0.0, "judge-score-out-of-range: 2.0")), + ('{"score": 8}', (0.0, 0.0, "judge-score-out-of-range: 8")), + ('{"score": -0.1}', (0.0, 0.0, "judge-score-out-of-range: -0.1")), + ('{"score": Infinity}', (0.0, 0.0, "judge-score-out-of-range: inf")), + ('{"score": NaN}', (0.0, 0.0, "judge-score-out-of-range: nan")), + ('{"score": true}', (0.0, 0.0, "judge-parse-failed")), + ('{"score": "high"}', (0.0, 0.0, "judge-parse-failed")), + ("no json here", (0.0, 0.0, "judge-parse-failed")), + ], +) +def test_judge_accepts_only_finite_scores_in_the_unit_interval(reply, expected) -> None: + backend = _ScriptedJudgeBackend({"RUBRIC": (reply, reply)}) + task = _rubric_task("t", "val", "RUBRIC") + assert backend.judge(task, "BASE answer") == expected + + +def test_out_of_range_judge_score_cannot_carry_a_regressed_candidate() -> None: + """v1 regresses 1.0 -> 0.0; v2's judge answers 2.0 for the candidate. + + Before the contract was enforced, the mixed metric read 0.625 -> 0.750 and + the gate accepted a candidate with flat hard accuracy and one regression. + """ + backend = _ScriptedJudgeBackend({ + "RUBRIC-V1": ('{"score": 1.0}', '{"score": 0.0}'), + "RUBRIC-V2": ('{"score": 0.5}', '{"score": 2.0}'), + }) + tasks = [ + _rubric_task("t1", "train", "RUBRIC-T"), + _rubric_task("v1", "val", "RUBRIC-V1"), + _rubric_task("v2", "val", "RUBRIC-V2"), + ] + result = consolidate(backend, tasks, "skill", "", gate_metric="mixed", evolve_memory=False) + assert result.accepted is False + assert result.gate_action.startswith("reject") + skill_trial = next(t for t in result.gate_trials if t["target"] == "skill") + assert all(d["candidate_score"] <= 1.0 for d in skill_trial["task_deltas"]) From 7003f95d09009f433dcee3c8de1903fd60be5b25 Mon Sep 17 00:00:00 2001 From: Dan Baciu Date: Fri, 9 Oct 2026 13:25:53 +0400 Subject: [PATCH 4/5] fix(sleep): keep learned items intact across apply_edits round-trips The learned block is read back one "- " bullet per item, but set_learned wrote items verbatim. A multi-line item lost every continuation line the next time any edit was applied; a "- X" add slipped past the duplicate check for an existing "X"; and lstrip('- ') removed leading dashes such as "--force". Losing adopted text changes what the gate scores: a helpful edit was rejected (0.5 vs 0.5) because applying it silently deleted an adopted rule held on a continuation line. - _learned_item(): one item is one line (whitespace collapsed) with exactly one leading bullet marker removed; used for add, replace and set_learned - current_learned_lines joins continuation lines of blocks written by earlier versions onto their item instead of dropping them Co-Authored-By: Claude Opus 5.5 (cherry picked from commit e3c5c566c6a82c20ddcd9fb8dde88c9e80a6f98f) --- skillopt_sleep/memory.py | 37 +++++++-- tests/test_learned_block_roundtrip.py | 114 ++++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 6 deletions(-) create mode 100644 tests/test_learned_block_roundtrip.py diff --git a/skillopt_sleep/memory.py b/skillopt_sleep/memory.py index 86d91a05..4fa9ccde 100644 --- a/skillopt_sleep/memory.py +++ b/skillopt_sleep/memory.py @@ -44,10 +44,25 @@ def _strip_learned(doc: str) -> str: return doc.rstrip() +def _learned_item(text: str) -> str: + """Normalize one learned item to the single bullet line it is stored as. + + The block is read back one ``- `` line per item, so an item must not span + lines: internal whitespace (including newlines) collapses to single + spaces. Exactly one leading bullet marker is dropped, so ``- X`` and ``X`` + are the same item while text such as ``--force`` keeps its dashes. + """ + item = " ".join(str(text or "").split()) + if item.startswith("- "): + item = item[2:].lstrip() + return item + + def set_learned(doc: str, learned_lines: List[str]) -> str: """Replace the protected learned region with the given bullet lines.""" base = _strip_learned(doc) - body = "\n".join(f"- {ln.strip().lstrip('- ').strip()}" for ln in learned_lines if ln.strip()) + items = (_learned_item(ln) for ln in learned_lines) + body = "\n".join(f"- {item}" for item in items if item) block = ( f"\n\n{LEARNED_START}\n" f"## Learned preferences & procedures\n\n{_BANNER}\n\n{body}\n" @@ -57,12 +72,21 @@ def set_learned(doc: str, learned_lines: List[str]) -> str: def current_learned_lines(doc: str) -> List[str]: + """Return the learned items, one per ``- `` bullet. + + Blocks written before items were normalized to one line can contain an + item whose continuation lines do not start with ``- ``. Those lines are + joined onto the preceding item instead of being dropped, so adopted text + survives the next edit. + """ inner = extract_learned(doc) lines: List[str] = [] for ln in inner.splitlines(): ln = ln.strip() if ln.startswith("- "): - lines.append(ln[2:].strip()) + lines.append(_learned_item(ln)) + elif ln and lines: + lines[-1] = _learned_item(f"{lines[-1]} {ln}") return lines @@ -110,11 +134,12 @@ def apply_edits_detailed( for e in edits: op = (e.op or "add").lower() if op == "add": - if _norm(e.content) in norm_set or not e.content.strip(): + item = _learned_item(e.content) + if not item or _norm(item) in norm_set: unmatched.append(e) continue - lines.append(e.content.strip()) - norm_set.add(_norm(e.content)) + lines.append(item) + norm_set.add(_norm(item)) applied.append(e) elif op == "delete": anchor = _norm(e.anchor or e.content) @@ -130,7 +155,7 @@ def apply_edits_detailed( unmatched.append(e) elif op == "replace": anchor = _norm(e.anchor) - replacement = e.content.strip() + replacement = _learned_item(e.content) new_lines = [] changed = False for line in lines: diff --git a/tests/test_learned_block_roundtrip.py b/tests/test_learned_block_roundtrip.py new file mode 100644 index 00000000..5fc4b2af --- /dev/null +++ b/tests/test_learned_block_roundtrip.py @@ -0,0 +1,114 @@ +"""Learned items survive the read -> apply -> write round-trip intact. + +The learned block is read back one ``- `` bullet per item, but items were +written verbatim. A multi-line item therefore lost every continuation line the +next time any edit was applied, a ``- X`` add slipped past the duplicate check +for an existing ``X``, and ``lstrip('- ')`` ate leading dashes such as +``--force``. Losing adopted text changes what the gate scores: a good edit was +rejected because applying it silently deleted an adopted rule. +""" +from __future__ import annotations + +from skillopt_sleep.backend import MockBackend +from skillopt_sleep.consolidate import consolidate +from skillopt_sleep.memory import ( + LEARNED_END, + LEARNED_START, + apply_edits_detailed, + current_learned_lines, + extract_learned, + set_learned, +) +from skillopt_sleep.types import EditRecord, TaskRecord + + +def _legacy_doc(*items: str) -> str: + """A learned block exactly as earlier versions wrote it: items verbatim.""" + body = "\n".join(f"- {item}" for item in items) + return ( + f"# Skill\n\n{LEARNED_START}\n## Learned preferences & procedures\n\n" + f"_banner_\n\n{body}\n{LEARNED_END}\n" + ) + + +def _add(content: str) -> EditRecord: + return EditRecord(target="skill", op="add", content=content) + + +def test_legacy_multi_line_item_survives_the_next_edit() -> None: + doc = _legacy_doc( + "Always run tests before committing", + "When editing SQL:\n1. run the migration dry-run\n2. check row counts", + ) + new_doc, applied, unmatched = apply_edits_detailed(doc, [_add("Prefer small commits")]) + assert len(applied) == 1 and unmatched == [] + assert current_learned_lines(new_doc) == [ + "Always run tests before committing", + "When editing SQL: 1. run the migration dry-run 2. check row counts", + "Prefer small commits", + ] + assert "check row counts" in extract_learned(new_doc) + + +def test_multi_line_add_is_stored_as_one_item_and_round_trips() -> None: + doc, applied, _ = apply_edits_detailed( + "# Skill\n", [_add("When editing SQL:\n1. dry-run first\n2. check row counts")] + ) + assert len(applied) == 1 + item = "When editing SQL: 1. dry-run first 2. check row counts" + assert current_learned_lines(doc) == [item] + again, applied, unmatched = apply_edits_detailed(doc, [_add(item)]) + assert applied == [] and len(unmatched) == 1 + assert again == doc + + +def test_bulleted_add_is_a_duplicate_of_the_existing_item() -> None: + doc = set_learned("# Skill\n", ["Always run tests before committing"]) + _new, applied, unmatched = apply_edits_detailed( + doc, [_add("- Always run tests before committing")] + ) + assert applied == [] and len(unmatched) == 1 + + +def test_leading_dashes_in_rule_text_are_preserved() -> None: + doc, _applied, _ = apply_edits_detailed( + "# Skill\n", [_add("--force-with-lease is required instead of --force")] + ) + assert current_learned_lines(doc) == [ + "--force-with-lease is required instead of --force" + ] + replaced, applied, _ = apply_edits_detailed( + doc, + [EditRecord(target="skill", op="replace", anchor="force-with-lease", + content="- --dry-run before every push")], + ) + assert len(applied) == 1 + assert current_learned_lines(replaced) == ["--dry-run before every push"] + + +def test_gate_accepts_a_good_edit_on_a_skill_with_a_legacy_multi_line_item() -> None: + """Applying tonight's helpful edit must not silently drop adopted text. + + Before the fix, the adopted units rule lived on a continuation line, so the + candidate lost it and scored 0.5 against a 0.5 baseline: rejected. + """ + units = MockBackend.RULE_TEXT["units-si"] + + def task(task_id, rule, split, reference): + return TaskRecord( + id=task_id, project="/project", intent=f"task {task_id} please", + reference_kind="exact", reference=reference, + tags=[f"rule:{rule}"], split=split, + ) + + tasks = [ + task("tr1", "wrap-answer", "train", "42"), + task("va1", "units-si", "val", "9.8 m/s^2"), + task("va2", "wrap-answer", "val", "7"), + ] + skill = _legacy_doc("Numeric answers:\n" + units) + result = consolidate(MockBackend(), tasks, skill, "", evolve_memory=False, gate_metric="hard") + assert result.accepted is True + assert result.baseline_score == 0.5 + assert result.candidate_score == 1.0 + assert units in result.new_skill From 8d6254a4e255f6d16aa26689a6a497044ed685d0 Mon Sep 17 00:00:00 2001 From: Yifan Yang Date: Tue, 10 Nov 2026 16:03:23 +0000 Subject: [PATCH 5/5] test(sleep): isolate the Claude tool-route regression and note #310 in CHANGELOG Since #336, a Claude replay without ANTHROPIC_API_KEY probes the CLI for --safe-mode and otherwise refuses. Set a test key so the self-reported marker test exercises the measured tool route instead of the isolation guard. Add the CHANGELOG entry for the four #310 fixes. --- CHANGELOG.md | 6 ++++++ tests/test_tool_evidence_judging.py | 5 ++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 04cb3d90..94db227e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -127,6 +127,12 @@ All notable changes to SkillOpt are documented here. This project adheres to value is rejected with a clear error. ### Fixed +- Close four ways invalid evidence could reach the SkillOpt-Sleep gate: recall + no longer brings a content twin of tonight's held-out task into training, + a measured tool route no longer credits a self-reported `TOOL_CALL:` marker, + judge scores outside 0..1 (including NaN/Infinity) fail closed instead of + moving the gate, and learned items survive `apply_edits` round-trips + (thanks @bogdanbaciu21, #310). - Apply Codex executable and sandbox aliases consistently across inherited YAML, CLI overrides, training, and eval-only entry points. Codex CLI calls now use explicit sandbox and approval settings instead of the retired diff --git a/tests/test_tool_evidence_judging.py b/tests/test_tool_evidence_judging.py index cc492982..f3bcc831 100644 --- a/tests/test_tool_evidence_judging.py +++ b/tests/test_tool_evidence_judging.py @@ -58,7 +58,10 @@ def fake_run(cmd, **kwargs): # shim's call log stays empty. return subprocess.CompletedProcess(cmd, 0, stdout=SELF_REPORT, stderr="") - with mock.patch("skillopt_sleep.backend.subprocess.run", side_effect=fake_run): + # Since #336 a Claude replay must be isolated; an API key selects --bare + # without probing the CLI, so the measured tool route is what is exercised. + with mock.patch.dict("os.environ", {"ANTHROPIC_API_KEY": "test-key"}), \ + mock.patch("skillopt_sleep.backend.subprocess.run", side_effect=fake_run): result = replay_one(ClaudeCliBackend(claude_path="claude"), _tool_task(), "skill", "") assert result.tools_called == [] assert result.hard == 0.0