fix(curator): protect cron skills referenced by absolute path
4c2961c51added referenced_skill_names() so the curator never archives a skill a cron job depends on — paused jobs and infrequent schedules would otherwise age their skills out and the next run fails to load them.62972060cthen taught the scheduler that jobs may store ABSOLUTE skill paths, normalizing them through normalize_skill_lookup_name before skill_view. The protection set kept returning the raw string, so it now holds a full path while the curator matches it against bare skill names. Those jobs silently lost their protection: the skill is archived, and the next fire logs a warning and runs the job without its instructions. Canonicalize each reference the same way the scheduler resolves it, with a deferred import and a verbatim fallback so a resolver failure can never drop a name (referenced_skill_names has exactly one caller, the curator's protection lookup, so nothing else sees the change).
This commit is contained in:
parent
ac662c3f71
commit
d135f64b51
32
cron/jobs.py
32
cron/jobs.py
|
|
@ -3078,6 +3078,33 @@ def save_job_output(job_id: str, output: str):
|
|||
# Skill reference rewriting (curator integration)
|
||||
# =============================================================================
|
||||
|
||||
def _canonical_skill_ref(raw: Any) -> str:
|
||||
"""Reduce one job skill reference to the bare name the curator matches on.
|
||||
|
||||
A job may store an absolute path under ``HERMES_HOME/skills`` or an
|
||||
external skills dir; the scheduler resolves those through
|
||||
``normalize_skill_lookup_name`` before handing them to ``skill_view``.
|
||||
The curator compares this set against bare skill names, so it has to
|
||||
resolve them the same way — otherwise a path-referencing job's skill
|
||||
looks unreferenced and gets archived out from under it.
|
||||
|
||||
Best-effort: if the resolver is unavailable or rejects the value, fall
|
||||
back to the plain cleanup so a broken import can never lose a name.
|
||||
"""
|
||||
value = str(raw or "").strip()
|
||||
if not value:
|
||||
return ""
|
||||
try:
|
||||
from agent.skill_utils import normalize_skill_lookup_name
|
||||
value = normalize_skill_lookup_name(value) or value
|
||||
except Exception:
|
||||
logger.debug(
|
||||
"referenced_skill_names: could not normalize skill ref %r", raw,
|
||||
exc_info=True,
|
||||
)
|
||||
return value.strip().lstrip("/")
|
||||
|
||||
|
||||
def referenced_skill_names() -> Set[str]:
|
||||
"""Return the set of skill names referenced by ANY cron job.
|
||||
|
||||
|
|
@ -3087,6 +3114,9 @@ def referenced_skill_names() -> Set[str]:
|
|||
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.
|
||||
|
||||
Names are canonicalized the way the scheduler resolves them at load
|
||||
time, so a job that stores an absolute skill path is protected too.
|
||||
|
||||
Best-effort: a corrupt/unreadable jobs store returns an empty set
|
||||
rather than raising, so a cron issue can never break the curator.
|
||||
"""
|
||||
|
|
@ -3101,7 +3131,7 @@ def referenced_skill_names() -> Set[str]:
|
|||
if not isinstance(job, dict):
|
||||
continue
|
||||
for name in _normalize_skill_list(job.get("skill"), job.get("skills")):
|
||||
cleaned = str(name).strip().lstrip("/")
|
||||
cleaned = _canonical_skill_ref(name)
|
||||
if cleaned:
|
||||
names.add(cleaned)
|
||||
return names
|
||||
|
|
|
|||
|
|
@ -191,6 +191,111 @@ def test_candidate_list_marks_cron_referenced_skills(curator_env, monkeypatch):
|
|||
assert "cron=no" in plain_line
|
||||
|
||||
|
||||
def _write_cron_job(home: Path, skill_ref: str, monkeypatch):
|
||||
"""Write a real jobs.json referencing *skill_ref* and reload ``cron.jobs``.
|
||||
|
||||
Deliberately does NOT stub ``_cron_referenced_skills`` — these cases
|
||||
exercise the real protection lookup end to end. ``SKILLS_DIR`` is pinned
|
||||
because the path resolver reads it at call time to decide which roots are
|
||||
trusted (the same attribute the repo's other skill tests patch).
|
||||
"""
|
||||
import importlib
|
||||
import json
|
||||
|
||||
import tools.skills_tool as skills_tool
|
||||
monkeypatch.setattr(skills_tool, "SKILLS_DIR", home / "skills")
|
||||
|
||||
cron_dir = home / "cron"
|
||||
cron_dir.mkdir(parents=True, exist_ok=True)
|
||||
(cron_dir / "jobs.json").write_text(
|
||||
json.dumps([{
|
||||
"id": "job1",
|
||||
"name": "quarterly digest",
|
||||
"enabled": True,
|
||||
"prompt": "write the digest",
|
||||
"skills": [skill_ref],
|
||||
"schedule": {"kind": "cron", "expr": "0 9 1 */3 *"},
|
||||
}]),
|
||||
encoding="utf-8",
|
||||
)
|
||||
import cron.jobs as cron_jobs
|
||||
importlib.reload(cron_jobs)
|
||||
return cron_jobs
|
||||
|
||||
|
||||
def test_cron_referenced_skill_by_name_survives_inactivity(curator_env, monkeypatch):
|
||||
"""Control: the plain-name reference form has always been protected."""
|
||||
c = curator_env["curator"]
|
||||
u = curator_env["usage"]
|
||||
skills_dir = curator_env["home"] / "skills"
|
||||
_write_skill(skills_dir, "quarterly-report")
|
||||
_backdate(u, "quarterly-report", 200)
|
||||
_write_cron_job(curator_env["home"], "quarterly-report", monkeypatch)
|
||||
|
||||
counts = c.apply_automatic_transitions()
|
||||
|
||||
assert counts["archived"] == 0
|
||||
assert u.load_usage()["quarterly-report"]["state"] == u.STATE_ACTIVE
|
||||
|
||||
|
||||
def test_cron_referenced_skill_by_absolute_path_survives_inactivity(curator_env, monkeypatch):
|
||||
"""A job may store an absolute skill path — the scheduler resolves it
|
||||
before ``skill_view``. The protection set has to resolve it the same way,
|
||||
or the skill is archived out from under a live job and the next run
|
||||
silently proceeds without its instructions.
|
||||
"""
|
||||
c = curator_env["curator"]
|
||||
u = curator_env["usage"]
|
||||
skills_dir = curator_env["home"] / "skills"
|
||||
_write_skill(skills_dir, "quarterly-report")
|
||||
_backdate(u, "quarterly-report", 200)
|
||||
_write_cron_job(curator_env["home"], str(skills_dir / "quarterly-report"), monkeypatch)
|
||||
|
||||
counts = c.apply_automatic_transitions()
|
||||
|
||||
assert counts["archived"] == 0
|
||||
assert u.load_usage()["quarterly-report"]["state"] == u.STATE_ACTIVE
|
||||
|
||||
|
||||
def test_referenced_names_canonicalize_absolute_paths(curator_env, monkeypatch):
|
||||
skills_dir = curator_env["home"] / "skills"
|
||||
_write_skill(skills_dir, "quarterly-report")
|
||||
cron_jobs = _write_cron_job(
|
||||
curator_env["home"], str(skills_dir / "quarterly-report"), monkeypatch
|
||||
)
|
||||
|
||||
assert cron_jobs.referenced_skill_names() == {"quarterly-report"}
|
||||
|
||||
|
||||
def test_unresolvable_reference_is_kept_verbatim(curator_env, tmp_path, monkeypatch):
|
||||
"""A path outside the skills roots can't be canonicalized; keep it rather
|
||||
than dropping the name (the resolver passes such values through)."""
|
||||
outside = tmp_path / "elsewhere" / "some-skill"
|
||||
cron_jobs = _write_cron_job(curator_env["home"], str(outside), monkeypatch)
|
||||
|
||||
assert cron_jobs.referenced_skill_names() == {
|
||||
str(outside).strip().lstrip("/")
|
||||
}
|
||||
|
||||
|
||||
def test_unreferenced_skill_is_still_archived(curator_env, monkeypatch):
|
||||
"""Guard against over-protecting: a skill no job mentions still ages out."""
|
||||
c = curator_env["curator"]
|
||||
u = curator_env["usage"]
|
||||
skills_dir = curator_env["home"] / "skills"
|
||||
_write_skill(skills_dir, "quarterly-report")
|
||||
_write_skill(skills_dir, "orphan")
|
||||
_backdate(u, "quarterly-report", 200)
|
||||
_backdate(u, "orphan", 200)
|
||||
_write_cron_job(curator_env["home"], str(skills_dir / "quarterly-report"), monkeypatch)
|
||||
|
||||
c.apply_automatic_transitions()
|
||||
|
||||
usage = u.load_usage()
|
||||
assert usage["quarterly-report"]["state"] == u.STATE_ACTIVE
|
||||
assert usage["orphan"]["state"] == u.STATE_ARCHIVED
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue