From 745807cf88ae63675267b7efb7e931f7a6514bfa 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 --- 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 5b80b4a98..533c62a57 100644 --- a/skillopt_sleep/consolidate.py +++ b/skillopt_sleep/consolidate.py @@ -61,6 +61,19 @@ class ConsolidationResult: gate_trials: List[dict] = 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)``. @@ -69,7 +82,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, @@ -94,7 +110,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 9906e07d5..8a7a89abd 100644 --- a/skillopt_sleep/dream.py +++ b/skillopt_sleep/dream.py @@ -19,7 +19,11 @@ import re from typing import 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) ───────────── @@ -69,6 +73,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 @@ -76,11 +81,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: @@ -88,7 +97,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 @@ -146,14 +155,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 000000000..1bcb7e0f9 --- /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 c1164b5a786256b24a1c5dd745bc45205533a81a 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 --- 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 3311f59dd..95ea14130 100644 --- a/skillopt_sleep/backend.py +++ b/skillopt_sleep/backend.py @@ -138,6 +138,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 1ac68a033..40d96c1b5 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 c60b0abb7..f12d260cd 100644 --- a/skillopt_sleep/replay.py +++ b/skillopt_sleep/replay.py @@ -50,8 +50,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 000000000..cc492982e --- /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 6de589a70bdbcf58b8bb344dba955e3e6cc8d91a 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 --- 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 95ea14130..9ae7eba5e 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 @@ -450,11 +451,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 000000000..4d600a227 --- /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 e3c5c566c6a82c20ddcd9fb8dde88c9e80a6f98f 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 --- 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 86d91a058..4fa9ccde5 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 000000000..5fc4b2aff --- /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 964d9c050935d57b73999901961d6657f25a5788 Mon Sep 17 00:00:00 2001 From: Dan Baciu Date: Fri, 9 Oct 2026 13:28:53 +0400 Subject: [PATCH 5/5] fix(sleep): derive headless-replay markers from the live prompt registry _is_headless_replay recognised the engine's own sessions by static prompt markers written for earlier prompt wording. None of them match the current attempt, judge, reflect or miner templates, and the duration fallback only covers prompts under 200 characters, so with projects="all" every engine session was harvested and could be mined back as a user task. - markers now include the opening line of every registry template, default and active override, so rewording a prompt cannot reopen the gap - a shared phrase of the Claude, Codex, Copilot and OpenCode tool-attempt prompts is matched case-insensitively - the existing static markers stay for transcripts from older versions; the Codex harvester (#286) is left to its own fix Co-Authored-By: Claude Opus 5.5 --- skillopt_sleep/harvest.py | 33 ++++- tests/test_harvest_engine_prompt_markers.py | 133 ++++++++++++++++++++ 2 files changed, 165 insertions(+), 1 deletion(-) create mode 100644 tests/test_harvest_engine_prompt_markers.py diff --git a/skillopt_sleep/harvest.py b/skillopt_sleep/harvest.py index 7f3bee8f7..4508d6c85 100644 --- a/skillopt_sleep/harvest.py +++ b/skillopt_sleep/harvest.py @@ -177,6 +177,35 @@ def _is_meta_prompt(text: str) -> bool: "## Skill\n", ) +# Shared by the tool-enabled attempt prompts of the Claude, Codex, Copilot and +# OpenCode backends (matched case-insensitively). +_TOOL_ATTEMPT_MARKER = "treat a 'learned preferences' block as" + + +def _engine_prompt_markers() -> tuple: + """Opening lines of the engine's own prompt templates. + + The static markers above were written for earlier prompt wording and no + longer match the current registry, so replay sessions slipped through. + Deriving markers from the registry (defaults and any active override) + keeps this filter in step with the prompts the engine actually sends. + """ + from skillopt_sleep import prompts + + markers = [] + for name, meta in prompts.DEFAULTS.items(): + texts = [meta["text"]] + try: + texts.append(prompts.get_prompt(name)) + except Exception: + pass # an unreadable override file must not break harvesting + for text in texts: + head = str(text).split("__", 1)[0].strip().splitlines() + # Short openings are too generic to identify an engine prompt. + if head and len(head[0].strip()) >= 24: + markers.append(head[0].strip()) + return tuple(dict.fromkeys(markers)) + # Sessions written by OTHER tools' sub-agents (memory observers, critic # sub-agents, plugin self-invocations). These are multi-turn, so the @@ -217,9 +246,11 @@ def _is_headless_replay(digest: "SessionDigest") -> bool: if digest.n_user_turns == 0: return True prompt = digest.user_prompts[0] if digest.user_prompts else "" - for marker in _REPLAY_PROMPT_MARKERS: + for marker in _REPLAY_PROMPT_MARKERS + _engine_prompt_markers(): if marker in prompt: return True + if _TOOL_ATTEMPT_MARKER in prompt.lower(): + return True # Sub-3-second single-turn sessions with short prompts are almost # certainly programmatic (engine grader/judge calls). We require the # prompt to also be short (<200 chars) to avoid false-positives on diff --git a/tests/test_harvest_engine_prompt_markers.py b/tests/test_harvest_engine_prompt_markers.py new file mode 100644 index 000000000..98c52dfed --- /dev/null +++ b/tests/test_harvest_engine_prompt_markers.py @@ -0,0 +1,133 @@ +"""The engine's own replay sessions are never harvested as user tasks. + +``_is_headless_replay`` recognised engine sessions by static prompt markers +that no longer matched the current prompt registry, and the duration fallback +only applies to prompts under 200 characters. With ``projects: "all"``, the +engine's attempt, judge, reflect and miner prompts were mined back as user +tasks. Markers are now derived from the registry itself, so rewording a prompt +cannot silently reopen the gap. +""" +from __future__ import annotations + +import json +import subprocess +from unittest import mock + +import pytest + +from skillopt_sleep import prompts +from skillopt_sleep.backend import ClaudeCliBackend, CodexCliBackend, CopilotCliBackend +from skillopt_sleep.harvest import _is_headless_replay, harvest +from skillopt_sleep.types import SessionDigest, TaskRecord + +RENDER_ARGS = { + "attempt": {"__SKILL__": "Use concise answers.", "__MEMORY__": "(none)", + "__INTENT__": "Summarize the PR", "__CONTEXT__": ""}, + "judge": {"__RUBRIC__": "Mentions the risk", "__RESPONSE__": "It is fine."}, + "reflect": {"__EDIT_BUDGET__": "3", "__TARGET__": "skill", "__CUR_DOC__": "x", + "__GUARD__": "", "__CRITERIA__": "", "__PREFS__": "", + "__FAILURES__": "- wanted: a\n got: b"}, + "miner": {"__PROJECT__": "/repo", "__PROMPTS__": " - do x", "__FINAL__": "ok", + "__FEEDBACK__": "(none)"}, +} + + +def _digest(prompt: str) -> SessionDigest: + return SessionDigest( + session_id="s", + project="/tmp/replay", + started_at="2026-10-01T00:00:00", + ended_at="2026-10-01T00:00:09", # too slow for the duration fallback + user_prompts=[prompt], + assistant_finals=["reply"], + n_user_turns=1, + n_assistant_turns=1, + ) + + +@pytest.fixture(autouse=True) +def _no_user_prompt_overrides(tmp_path, monkeypatch): + monkeypatch.setenv("SKILLOPT_SLEEP_PROMPTS_PATH", str(tmp_path / "prompts.json")) + + +def test_every_registry_prompt_has_render_args() -> None: + assert set(RENDER_ARGS) == set(prompts.DEFAULTS) + + +@pytest.mark.parametrize("name", sorted(RENDER_ARGS)) +def test_rendered_engine_prompts_are_filtered(name) -> None: + prompt = prompts.render(name, RENDER_ARGS[name]) + # The session took nine seconds, so only a prompt marker can catch it. + assert _is_headless_replay(_digest(prompt)) is True + + +def test_overridden_engine_prompt_is_filtered(tmp_path) -> None: + override = "Grade this answer for the nightly SkillOpt run.\n__RESPONSE__\n" + "x" * 300 + (tmp_path / "prompts.json").write_text(json.dumps({"judge": override}), encoding="utf-8") + prompt = prompts.render("judge", {"__RUBRIC__": "r", "__RESPONSE__": "resp"}) + assert prompt.startswith("Grade this answer") + assert _is_headless_replay(_digest(prompt)) is True + + +def _captured_tool_attempt_prompt(backend) -> str: + seen: list[str] = [] + + def fake_run(cmd, **kwargs): + texts = [kwargs.get("input") or ""] + [str(part) for part in cmd] + seen.extend(text for text in texts if "# Task" in text or "Summarize the PR" in text) + return subprocess.CompletedProcess(cmd, 0, stdout="answer", stderr="") + + task = TaskRecord( + id="t", project="/repo", intent="Summarize the PR", reference_kind="rule", + judge={"kind": "rule", "checks": [{"op": "tool_called", "arg": "search"}]}, + ) + with mock.patch("skillopt_sleep.backend.subprocess.run", side_effect=fake_run): + try: + backend.attempt_with_tools(task, "Use concise answers.", "(none)", ["search"]) + except Exception: + pass + assert seen, f"{type(backend).__name__} sent no capturable prompt" + return max(seen, key=len) + + +@pytest.mark.parametrize( + "backend", + [ClaudeCliBackend(claude_path="claude"), CodexCliBackend(), CopilotCliBackend()], + ids=["claude", "codex", "copilot"], +) +def test_tool_attempt_prompts_are_filtered(backend) -> None: + prompt = _captured_tool_attempt_prompt(backend) + assert _is_headless_replay(_digest(prompt)) is True + + +@pytest.mark.parametrize( + "prompt", + [ + "Complete the following task: fix the flaky login test and explain the root cause", + "Please score how well my essay answers the prompt, and suggest edits " + "x" * 200, + "You are mining data from logs today, find the failing deploy " + "y" * 200, + "Treat this as urgent: the release branch is broken " + "z" * 200, + ], +) +def test_real_user_prompts_are_kept(prompt) -> None: + assert _is_headless_replay(_digest(prompt)) is False + + +def test_harvest_with_all_projects_skips_engine_sessions_and_keeps_user_work(tmp_path) -> None: + sessions = {name: prompts.render(name, args) for name, args in RENDER_ARGS.items()} + sessions["user"] = "Refactor the billing module so invoices round to cents " + "q" * 200 + for name, prompt in sessions.items(): + folder = tmp_path / f"-tmp-{name}" + folder.mkdir() + records = [ + {"type": "user", "cwd": f"/tmp/{name}", "timestamp": "2026-10-01T00:00:00Z", + "message": {"role": "user", "content": prompt}}, + {"type": "assistant", "timestamp": "2026-10-01T00:00:09Z", + "message": {"role": "assistant", "content": [{"type": "text", "text": "ok"}]}}, + ] + (folder / f"{name}.jsonl").write_text( + "\n".join(json.dumps(record) for record in records), encoding="utf-8" + ) + + harvested = harvest(str(tmp_path), scope="all") + assert [digest.user_prompts[0][:20] for digest in harvested] == [sessions["user"][:20]]