From 4c2961c511c523c621b8847493f032ab65bf116a Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 28 Jun 2026 15:10:21 -0700 Subject: [PATCH] fix(curator): never archive cron-referenced skills + floor use=0 pruning (#54443) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The curator's inactivity prune archived any non-pinned agent-created skill whose activity was older than archive_after_days (90d). A skill loaded only by a cron job had its usage bumped solely when the job fired, so paused jobs, infrequent (quarterly/annual) schedules, and far-future one-shots aged their skills out from under them — the next run then failed to load the now-archived skill. - cron/jobs.py: add referenced_skill_names() returning skills used by ANY job (incl. paused/disabled). - curator.apply_automatic_transitions(): skip cron-referenced skills like pinned; add a use=0 grace floor so a never-used skill is not marked stale/archived until it is at least stale_after_days old. - LLM review pass: candidate list marks cron=yes; prompt forbids pruning cron-referenced skills and never-used skills under 30 days. Tested E2E against a real cron job + real usage records and with 4 new unit tests. --- agent/curator.py | 51 ++++++++++++++++++++- cron/jobs.py | 31 ++++++++++++- tests/agent/test_curator.py | 88 +++++++++++++++++++++++++++++++++++++ 3 files changed, 168 insertions(+), 2 deletions(-) diff --git a/agent/curator.py b/agent/curator.py index 6843205c684..c13a36ecbbd 100644 --- a/agent/curator.py +++ b/agent/curator.py @@ -273,6 +273,21 @@ def should_run_now(now: Optional[datetime] = None) -> bool: # Automatic state transitions (pure function, no LLM) # --------------------------------------------------------------------------- +def _cron_referenced_skills() -> Set[str]: + """Skill names referenced by any cron job (incl. paused/disabled). + + Best-effort: a cron-module import error or corrupt jobs store must never + break the curator, so any failure yields an empty set (no protection, + but no crash). + """ + try: + from cron.jobs import referenced_skill_names as _refs + return _refs() + except Exception as e: + logger.debug("Curator could not read cron skill references: %s", e, exc_info=True) + return set() + + def apply_automatic_transitions(now: Optional[datetime] = None) -> Dict[str, int]: """Walk every curator-managed skill and move active/stale/archived based on the latest real activity timestamp. Pinned skills are never touched. @@ -292,6 +307,8 @@ def apply_automatic_transitions(now: Optional[datetime] = None) -> Dict[str, int stale_cutoff = now - timedelta(days=get_stale_after_days()) archive_cutoff = now - timedelta(days=get_archive_after_days()) + cron_referenced = _cron_referenced_skills() + counts = {"marked_stale": 0, "archived": 0, "reactivated": 0, "checked": 0, "seeded": 0} for row in _u.agent_created_report(): @@ -300,6 +317,15 @@ def apply_automatic_transitions(now: Optional[datetime] = None) -> Dict[str, int if row.get("pinned"): continue + # A skill referenced by any cron job (incl. paused/disabled) is in + # use by definition — resuming or the next fire must find it. The + # scheduler only bumps usage when a job actually fires, so jobs that + # fire less often than archive_after_days, paused jobs, and far-future + # one-shots would otherwise have their skills aged out from under + # them. Treat referenced skills like pinned: never auto-transition. + if name in cron_referenced: + continue + # First sight of a curation-eligible skill with no persisted record # (e.g. a newly-eligible built-in): anchor its clock to now and defer. if not row.get("_persisted", True): @@ -316,6 +342,18 @@ def apply_automatic_transitions(now: Optional[datetime] = None) -> Dict[str, int current = row.get("state", _u.STATE_ACTIVE) + # Never-used skills (use_count == 0) get a grace floor: don't archive + # one until it is at least stale_after_days old. A use=0 skill is + # absence of evidence, not evidence of staleness — a skill created + # recently may simply not have had its trigger come up yet. + never_used = int(row.get("use_count", 0) or 0) == 0 + if never_used and anchor > stale_cutoff: + # Younger than the stale window — leave it alone entirely. + if current == _u.STATE_STALE: + _u.set_state(name, _u.STATE_ACTIVE) + counts["reactivated"] += 1 + continue + if anchor <= archive_cutoff and current != _u.STATE_ARCHIVED: ok, _msg = _u.archive_skill(name) if ok: @@ -390,10 +428,19 @@ CURATOR_REVIEW_PROMPT = ( "back load-bearing UX (slash-command entry points referenced in docs and " "tips) and are filtered out of the candidate list below — never resurrect " "one as an archive or absorb target.\n" + "3c. DO NOT archive or prune any skill marked `cron=yes` in the candidate " + "list. A cron job depends on it and will fail to load it on its next " + "run. You MAY still consolidate it into an umbrella — but only because " + "the curator rewrites cron job skill references to follow consolidations; " + "never simply prune it.\n" "4. DO NOT use usage counters as a reason to skip consolidation. The " "counters are new and often mostly zero. Judge overlap on CONTENT, " "not on use_count. 'use=0' is not evidence a skill is valuable; it's " - "absence of evidence either way.\n" + "absence of evidence either way. Corollary: 'use=0' is ALSO not a " + "reason to PRUNE a skill. Never archive a never-used skill (use=0) " + "unless it is at least 30 days old (check last_activity / created date) " + "AND its content is genuinely obsolete or fully absorbed elsewhere — a " + "recently-created skill simply may not have had its trigger come up yet.\n" "5. DO NOT reject consolidation on the grounds that 'each skill has " "a distinct trigger'. Pairwise distinctness is the wrong bar. The " "right bar is: 'would a human maintainer write this as N separate " @@ -1413,12 +1460,14 @@ def _render_candidate_list() -> str: rows = skill_usage.agent_created_report() if not rows: return "No agent-created skills to review." + cron_referenced = _cron_referenced_skills() lines = [f"Agent-created skills ({len(rows)}):\n"] for r in rows: lines.append( f"- {r['name']} " f"state={r['state']} " f"pinned={'yes' if r.get('pinned') else 'no'} " + f"cron={'yes' if r['name'] in cron_referenced else 'no'} " f"activity={r.get('activity_count', 0)} " f"use={r.get('use_count', 0)} " f"view={r.get('view_count', 0)} " diff --git a/cron/jobs.py b/cron/jobs.py index e9ab8939fed..dd69ef55ef0 100644 --- a/cron/jobs.py +++ b/cron/jobs.py @@ -32,7 +32,7 @@ except ImportError: # pragma: no cover - non-Windows from datetime import datetime, timedelta from pathlib import Path from hermes_constants import get_hermes_home -from typing import Optional, Dict, List, Any, Tuple, Union +from typing import Optional, Dict, List, Any, Set, Tuple, Union logger = logging.getLogger(__name__) @@ -1634,6 +1634,35 @@ def save_job_output(job_id: str, output: str): # Skill reference rewriting (curator integration) # ============================================================================= +def referenced_skill_names() -> Set[str]: + """Return the set of skill names referenced by ANY cron job. + + Includes paused and disabled jobs deliberately: a paused job never + fires, so its skills never get a ``bump_use`` from the scheduler, yet + resuming it must still find its skills present. The curator uses this + set to protect referenced skills from inactivity archival — a skill a + live job depends on is "in use" regardless of when it was last loaded. + + Best-effort: a corrupt/unreadable jobs store returns an empty set + rather than raising, so a cron issue can never break the curator. + """ + try: + jobs = load_jobs() + except Exception: + logger.debug("referenced_skill_names: failed to load cron jobs", exc_info=True) + return set() + + names: Set[str] = set() + for job in jobs: + if not isinstance(job, dict): + continue + for name in _normalize_skill_list(job.get("skill"), job.get("skills")): + cleaned = str(name).strip().lstrip("/") + if cleaned: + names.add(cleaned) + return names + + def rewrite_skill_refs( consolidated: Optional[Dict[str, str]] = None, pruned: Optional[List[str]] = None, diff --git a/tests/agent/test_curator.py b/tests/agent/test_curator.py index 151faf138f4..7e67246dbb3 100644 --- a/tests/agent/test_curator.py +++ b/tests/agent/test_curator.py @@ -263,6 +263,94 @@ def test_new_skill_without_last_used_not_immediately_archived(curator_env): assert (skills_dir / "fresh").exists() +def _backdate(u, name: str, days: int, *, use_count: int = 1): + """Write an agent-created usage record whose activity is *days* old.""" + ts = (datetime.now(timezone.utc) - timedelta(days=days)).isoformat() + data = u.load_usage() + data[name] = u._empty_record() + data[name]["created_by"] = "agent" + data[name]["created_at"] = ts + data[name]["last_used_at"] = ts if use_count else None + data[name]["last_activity_at"] = ts if use_count else None + data[name]["use_count"] = use_count + u.save_usage(data) + + +def test_cron_referenced_skill_is_not_archived(curator_env, monkeypatch): + """A skill referenced by a cron job must survive inactivity archival even + when its activity is well past archive_after_days. The scheduler only + bumps usage when a job fires, so paused / infrequent / far-future jobs + would otherwise have their skills aged out from under them.""" + c = curator_env["curator"] + u = curator_env["usage"] + skills_dir = curator_env["home"] / "skills" + _write_skill(skills_dir, "cron-dep") + _write_skill(skills_dir, "orphan") + _backdate(u, "cron-dep", 200) + _backdate(u, "orphan", 200) + + # Pretend a (paused/infrequent) cron job references "cron-dep" only. + monkeypatch.setattr(c, "_cron_referenced_skills", lambda: {"cron-dep"}) + + counts = c.apply_automatic_transitions() + + assert u.get_record("cron-dep")["state"] == "active" # protected + assert (skills_dir / "cron-dep").exists() + assert u.get_record("orphan")["state"] == "archived" # control + assert counts["archived"] == 1 + + +def test_unused_skill_not_archived_before_stale_floor(curator_env): + """A never-used skill (use_count == 0) younger than stale_after_days must + not be marked stale or archived — absence of use is not evidence of + staleness when the skill simply hasn't had its trigger come up yet.""" + c = curator_env["curator"] + u = curator_env["usage"] + skills_dir = curator_env["home"] / "skills" + _write_skill(skills_dir, "young-unused") + _backdate(u, "young-unused", 10, use_count=0) # < 30d stale floor + + counts = c.apply_automatic_transitions() + + assert u.get_record("young-unused")["state"] == "active" + assert counts["archived"] == 0 + assert counts["marked_stale"] == 0 + + +def test_unused_skill_archived_past_archive_window(curator_env): + """The use=0 floor only protects YOUNG skills — a never-used skill older + than archive_after_days still archives (no perpetual reprieve).""" + c = curator_env["curator"] + u = curator_env["usage"] + skills_dir = curator_env["home"] / "skills" + _write_skill(skills_dir, "old-unused") + _backdate(u, "old-unused", 200, use_count=0) + + counts = c.apply_automatic_transitions() + + assert u.get_record("old-unused")["state"] == "archived" + assert counts["archived"] == 1 + + +def test_candidate_list_marks_cron_referenced_skills(curator_env, monkeypatch): + """The LLM review candidate list flags cron-referenced skills so the + review pass knows not to prune them.""" + c = curator_env["curator"] + u = curator_env["usage"] + skills_dir = curator_env["home"] / "skills" + _write_skill(skills_dir, "cron-dep") + _write_skill(skills_dir, "plain") + _backdate(u, "cron-dep", 1) + _backdate(u, "plain", 1) + monkeypatch.setattr(c, "_cron_referenced_skills", lambda: {"cron-dep"}) + + listing = c._render_candidate_list() + cron_line = next(l for l in listing.splitlines() if l.startswith("- cron-dep")) + plain_line = next(l for l in listing.splitlines() if l.startswith("- plain")) + assert "cron=yes" in cron_line + assert "cron=no" in plain_line + + def test_manual_skill_is_not_auto_archived(curator_env): """Manual skills can have usage records, but without the agent-created marker they must stay out of curator transitions."""