diff --git a/CHANGELOG.md b/CHANGELOG.md index 47d0e99a..35205bcc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -100,6 +100,15 @@ All notable changes to SkillOpt are documented here. This project adheres to `--cfg-options`, or a structured or legacy flat config file) is accepted and warns rather than being dropped in silence (reported by @xs229 in #213; implemented by @wilyan09007 in #219). +- A misspelled SkillOpt-Sleep `backend` or `transcript_source` now stops the + run with an error that lists the supported names. Previously it silently + fell back to the mock backend or to Claude transcripts, so a scheduled run + with a typo appeared to succeed; it now fails until the config is fixed + (thanks @Hrushikesh-ramilla, #350, #354). +- `transcript_source` ignores case and surrounding whitespace, and the + normalized name also keys the harvest checkpoint and the tasks document, so + `"Codex"` no longer reads a different checkpoint than `"codex"`. A non-string + value is rejected with a clear error. ### Fixed - Apply Codex executable and sandbox aliases consistently across inherited @@ -143,8 +152,8 @@ All notable changes to SkillOpt are documented here. This project adheres to ### Acknowledgements 🙏 Thank you to the contributors behind this unreleased work: -@AKhozya, @Alphaxalchemy, @Phoenix0531-sudo, @SparshGarg999, -@Tanmay9223, @chirag127, @codeL1985, @dimitarvdenev, +@AKhozya, @Alphaxalchemy, @Hrushikesh-ramilla, @Phoenix0531-sudo, +@SparshGarg999, @Tanmay9223, @chirag127, @codeL1985, @dimitarvdenev, @bogdanbaciu21, @ichoosetoaccept, @jcforever1, @nankingjing, @wilyan09007, @xs229, and @zixuanguo786-ctrl. diff --git a/skillopt_sleep/__main__.py b/skillopt_sleep/__main__.py index 696b15ec..fb0be7ea 100644 --- a/skillopt_sleep/__main__.py +++ b/skillopt_sleep/__main__.py @@ -36,7 +36,7 @@ from skillopt_sleep.config import ConfigError, load_config from skillopt_sleep.cycle import _one_line_display_text, run_sleep_cycle from skillopt_sleep.harvest import format_harvest_note -from skillopt_sleep.harvest_sources import harvest_for_config +from skillopt_sleep.harvest_sources import harvest_for_config, normalize_transcript_source from skillopt_sleep.mine import mine from skillopt_sleep.staging import ( StagingError, @@ -406,6 +406,7 @@ def _handoff_mine_and_pin(cfg, args, backend, snapshot: str, dry: bool): from skillopt_sleep.types import SessionDigest project = cfg.get("invoked_project") or os.getcwd() + source = normalize_transcript_source(cfg.get("transcript_source")) state = SleepState.load(cfg.state_path) started = _now_iso() @@ -425,7 +426,7 @@ def _handoff_mine_and_pin(cfg, args, backend, snapshot: str, dry: bool): file=sys.stderr) digests = None if digests is None: - since = state.last_harvest_for(project, cfg.get("transcript_source", "")) + since = state.last_harvest_for(project, source) lookback_hours = cfg.get("lookback_hours", 72) if since is None and lookback_hours and lookback_hours > 0: since = _now_iso(time.time() - lookback_hours * 3600) @@ -477,13 +478,13 @@ def _handoff_mine_and_pin(cfg, args, backend, snapshot: str, dry: bool): if not dry: # Advance the harvest window like run_sleep_cycle's no-tasks # branch, or every later run re-scans the same stale window. - state.set_last_harvest(project, started, cfg.get("transcript_source", "")) + state.set_last_harvest(project, started, source) state.save() return 0, None payload = make_tasks_payload( tasks, project=project, - transcript_source=cfg.get("transcript_source", ""), + transcript_source=source, n_sessions=len(digests), target_skill_path=target_skill_path, ) @@ -809,7 +810,7 @@ def cmd_harvest(args) -> int: payload = make_tasks_payload( tasks, project=cfg.get("invoked_project") or os.getcwd(), - transcript_source=cfg.get("transcript_source", ""), + transcript_source=normalize_transcript_source(cfg.get("transcript_source")), n_sessions=len(digests), target_skill_path=target_skill_path, ) diff --git a/skillopt_sleep/cycle.py b/skillopt_sleep/cycle.py index 42a66362..d19f9893 100644 --- a/skillopt_sleep/cycle.py +++ b/skillopt_sleep/cycle.py @@ -29,7 +29,7 @@ from skillopt_sleep.evidence import EvidenceLog from skillopt_sleep.handoff_backend import PendingCalls from skillopt_sleep.harvest import format_harvest_note -from skillopt_sleep.harvest_sources import harvest_for_config +from skillopt_sleep.harvest_sources import harvest_for_config, normalize_transcript_source from skillopt_sleep.memory import ensure_skill_scaffold from skillopt_sleep.mine import group_tasks_by_skill_hint, mine from skillopt_sleep.replay import aggregate_scores, replay_batch @@ -676,6 +676,8 @@ def run_sleep_cycle( it can inspect the backend's pending calls after the run. """ cfg = cfg or load_config() + # Checked before the night writes anything (state, staging dir, evidence). + harvest_source = normalize_transcript_source(cfg.get("transcript_source")) budget = _night_token_budget(cfg) state = SleepState.load(cfg.state_path) project = _project_paths(cfg) @@ -791,7 +793,7 @@ def budget_exhausted() -> bool: # ── 1+2. harvest + mine (unless seed_tasks injected) ───────────────── # The harvest cursor is per (project, source): each source reads its own # store, so one shared cursor would make a source skip its sessions (#312). - harvest_source = cfg.get("transcript_source", "") + # harvest_source is the canonical name harvest_for_config reads. digests: List[SessionDigest] = [] if seed_tasks is not None: tasks = seed_tasks @@ -815,7 +817,7 @@ def budget_exhausted() -> bool: candidate_limit = max(max_tasks, max_tasks * 3) _progress( cfg, - f"harvest start: source={cfg.get('transcript_source')} max_sessions={max_sessions}", + f"harvest start: source={harvest_source} max_sessions={max_sessions}", ) digests = harvest_for_config( cfg, diff --git a/skillopt_sleep/harvest_sources.py b/skillopt_sleep/harvest_sources.py index c1925531..d1954ca6 100644 --- a/skillopt_sleep/harvest_sources.py +++ b/skillopt_sleep/harvest_sources.py @@ -24,6 +24,31 @@ ) +def normalize_transcript_source(value: object) -> str: + """Canonical name of a configured ``transcript_source``. + + Unset or empty means ``claude``; otherwise case and surrounding + whitespace are ignored. The harvester, its per-source cursor and the + tasks document must all use this one value: a cursor keyed by the raw + spelling (``::Codex``) is not the one the harvested source + owns, and skips its older sessions (#312). + """ + supported = ", ".join(SUPPORTED_SOURCES) + if value is None: + return "claude" + if not isinstance(value, str): + raise ValueError( + f"transcript_source must be a string, not {type(value).__name__} " + f"{value!r}. Supported sources: {supported}" + ) + source = (value or "claude").strip().lower() + if source not in SUPPORTED_SOURCES: + raise ValueError( + f"Unknown transcript source '{value}'. Supported sources: {supported}" + ) + return source + + def harvest_for_config( cfg, *, @@ -37,17 +62,10 @@ def harvest_for_config( `skillopt_sleep.harvest.harvest`); the other sources have no repeat filter to report yet and leave it untouched. """ - raw_source = cfg.get("transcript_source", "claude") - source = (raw_source or "claude").strip().lower() + source = normalize_transcript_source(cfg.get("transcript_source", "claude")) scope = cfg.get("projects", "invoked") invoked_project = cfg.get("invoked_project", "") - if source not in SUPPORTED_SOURCES: - supported = ", ".join(SUPPORTED_SOURCES) - raise ValueError( - f"Unknown transcript source '{raw_source}'. Supported sources: {supported}" - ) - if source == "codex": return harvest_codex( cfg.codex_archived_sessions_dir, diff --git a/tests/test_harvest_checkpoint_source.py b/tests/test_harvest_checkpoint_source.py index 6df852f3..6f6ad282 100644 --- a/tests/test_harvest_checkpoint_source.py +++ b/tests/test_harvest_checkpoint_source.py @@ -10,8 +10,14 @@ """ from __future__ import annotations +import contextlib +import io import json import os +from types import SimpleNamespace +from unittest import mock + +import pytest from skillopt_sleep.config import load_config from skillopt_sleep.state import SleepState, harvest_cursor_key @@ -143,3 +149,149 @@ def test_a_source_still_skips_what_it_already_harvested(tmp_path): # its own cursor advanced: the same session is not harvested twice assert run_sleep_cycle(claude, dry_run=True).report.n_sessions == 0 + + +# ── source spelling (follow-up to #354) ───────────────────────────────────── +# harvest_for_config accepts "Codex" or " codex " as Codex, so the cursor has +# to be the Codex one too. A "::Codex" cursor may hold a time written +# while that spelling still silently harvested Claude; reading it would skip +# older Codex sessions for good, the #312 failure again. + +STALE = "2026-01-09T03:00:00" # written under the raw spelling +CODEX_CURSOR = "2026-01-05T03:00:00" + + +def _codex_cfg(tmp_path, source): + proj = str(tmp_path / "proj") + os.makedirs(proj, exist_ok=True) + os.makedirs(tmp_path / ".claude", exist_ok=True) + return proj, load_config( + invoked_project=proj, projects="all", backend="mock", + claude_home=str(tmp_path / ".claude"), + codex_home=str(tmp_path / ".codex"), + transcript_source=source, lookback_hours=24, + ) + + +def _seed_cursors(cfg, proj, spelling): + state = SleepState.load(cfg.state_path) + state.set_last_harvest(proj, CODEX_CURSOR, "codex") + state.set_last_harvest(proj, STALE, spelling) + state.save() + + +@pytest.mark.parametrize("spelling", ["Codex", " codex ", "CODEX"]) +def test_source_spelling_uses_the_canonical_cursor_and_harvester(tmp_path, spelling): + from skillopt_sleep.cycle import run_sleep_cycle + + proj, cfg = _codex_cfg(tmp_path, spelling) + _seed_cursors(cfg, proj, spelling) + + with mock.patch("skillopt_sleep.harvest_sources.harvest_codex", + return_value=[]) as codex, \ + mock.patch("skillopt_sleep.harvest_sources.harvest") as claude: + run_sleep_cycle(cfg, dry_run=False) + + claude.assert_not_called() + assert codex.call_args.kwargs["since_iso"] == CODEX_CURSOR + cursors = SleepState.load(cfg.state_path).data["last_harvest"] + # the Codex cursor advanced; the stale one is neither read nor rewritten + assert set(cursors) == {harvest_cursor_key(proj, "codex"), + harvest_cursor_key(proj, spelling)} + assert cursors[harvest_cursor_key(proj, "codex")] != CODEX_CURSOR + assert cursors[harvest_cursor_key(proj, spelling)] == STALE + + +def test_handoff_pinning_uses_the_canonical_cursor(tmp_path): + from skillopt_sleep.__main__ import _handoff_mine_and_pin + from skillopt_sleep.handoff_backend import HandoffBackend + + proj, cfg = _codex_cfg(tmp_path, "Codex") + _seed_cursors(cfg, proj, "Codex") + backend = HandoffBackend(handoff_dir=str(tmp_path / "handoff")) + + with mock.patch("skillopt_sleep.harvest_sources.harvest_codex", + return_value=[]) as codex, \ + contextlib.redirect_stdout(io.StringIO()): + rc, tasks = _handoff_mine_and_pin( + cfg, SimpleNamespace(json=False), backend, + str(tmp_path / "tasks.json"), False, + ) + + assert (rc, tasks) == (0, None) + assert codex.call_args.kwargs["since_iso"] == CODEX_CURSOR + cursors = SleepState.load(cfg.state_path).data["last_harvest"] + assert cursors[harvest_cursor_key(proj, "codex")] != CODEX_CURSOR + assert cursors[harvest_cursor_key(proj, "Codex")] == STALE + + +def test_harvest_command_records_the_canonical_source(tmp_path): + from skillopt_sleep.__main__ import cmd_harvest + + _, cfg = _codex_cfg(tmp_path, " Codex ") + out = io.StringIO() + with mock.patch("skillopt_sleep.__main__._cfg_from_args", return_value=cfg), \ + mock.patch("skillopt_sleep.harvest_sources.harvest_codex", + return_value=[]), \ + contextlib.redirect_stdout(out): + assert cmd_harvest(SimpleNamespace(json=True, output="")) == 0 + + assert json.loads(out.getvalue())["transcript_source"] == "codex" + + +@pytest.mark.parametrize("value", [1, ["codex"], {"name": "codex"}, True]) +def test_non_string_source_is_a_clear_value_error(tmp_path, value): + from skillopt_sleep.harvest_sources import ( + harvest_for_config, + normalize_transcript_source, + ) + + for call in (lambda: normalize_transcript_source(value), + lambda: harvest_for_config(_codex_cfg(tmp_path, value)[1])): + with pytest.raises(ValueError, match="transcript_source must be a string"): + call() + + +@pytest.mark.parametrize("value", [["codex"], "Codexx"]) +def test_invalid_source_fails_before_the_night_writes_anything(tmp_path, value): + from skillopt_sleep.cycle import run_sleep_cycle + + proj, cfg = _codex_cfg(tmp_path, value) + with pytest.raises(ValueError): + run_sleep_cycle(cfg, dry_run=False) + + # no staging dir for a night that never started, and no state change + assert os.listdir(proj) == [] + assert not os.path.exists(cfg.state_path) + + +def test_canonical_sources_keep_their_cursor_keys(): + from skillopt_sleep.harvest_sources import ( + SUPPORTED_SOURCES, + normalize_transcript_source, + ) + + for source in SUPPORTED_SOURCES: + assert normalize_transcript_source(source) == source + assert harvest_cursor_key(PROJECT, normalize_transcript_source(source)) \ + == f"{PROJECT}::{source}" + assert normalize_transcript_source("") == "claude" + assert normalize_transcript_source(None) == "claude" + + +def test_default_claude_source_still_reads_the_legacy_cursor(tmp_path): + from skillopt_sleep.cycle import run_sleep_cycle + + proj, cfg = _codex_cfg(tmp_path, "claude") + state = SleepState.load(cfg.state_path) + state.set_last_harvest(proj, STALE) # pre-#322 bare project key + state.save() + + with mock.patch("skillopt_sleep.harvest_sources.harvest", + return_value=[]) as claude: + run_sleep_cycle(cfg, dry_run=False) + + assert claude.call_args.kwargs["since_iso"] == STALE + cursors = SleepState.load(cfg.state_path).data["last_harvest"] + assert set(cursors) == {proj, f"{proj}::claude"} + assert cursors[proj] == STALE