diff --git a/skillopt_sleep/backend.py b/skillopt_sleep/backend.py index 3311f59dd..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 @@ -138,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", "") @@ -448,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/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/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/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/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/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_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]] 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"]) 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 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 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." + )