Skip to content

Commit bdd2120

Browse files
teknium1nickdlkk
authored andcommitted
fix(curator): authoritative absorbed_into on delete + restore cron skill links on rollback (NousResearch#18671) (NousResearch#18731)
* fix(curator): authoritative absorbed_into declarations on skill delete Closes NousResearch#18671. The classification pipeline that feeds cron-ref rewriting used to infer consolidation vs pruning from two brittle signals: the curator model's post-hoc YAML summary block, and a substring heuristic scanning other tool calls for the removed skill's name. Both miss in real consolidations — the model forgets the YAML under reasoning pressure, and the heuristic misses when the umbrella's patch content describes the absorbed behavior abstractly instead of naming the old slug. When both miss, the skill falls through to 'no-evidence fallback' pruned, and NousResearch#18253's cron rewriter drops the cron ref entirely instead of mapping it to the umbrella. Same observable symptom as pre-NousResearch#18253: 'Skill(s) not found and skipped' at the next cron run. The fix makes the model declare intent at the moment of deletion. skill_manage(action='delete') now accepts absorbed_into: - absorbed_into='<umbrella>' -> consolidated, target must exist on disk - absorbed_into='' -> explicit prune, no forwarding target - missing -> legacy path, falls through to heuristic/YAML The curator reconciler reads these declarations off llm_meta.tool_calls BEFORE either the YAML block or the substring heuristic. Declaration wins. Fallback logic stays intact for backward compat with any caller (human or older curator conversation) that doesn't populate the arg. Changes - tools/skill_manager_tool.py: add absorbed_into param to skill_manage + _delete_skill. Validate target exists when non-empty. Reject absorbed_into=<self>. Wire through dispatcher + registry + schema. - agent/curator.py: new _extract_absorbed_into_declarations() walks tool calls for skill_manage(delete) with the arg. _reconcile_classification accepts absorbed_declarations= and treats them as authoritative. Curator prompt updated to require the arg on every delete. - Tests: 7 new skill_manager tests covering the tool contract (valid target, empty string, nonexistent target, self-reference, whitespace, backward compat, dispatcher plumbing). 11 new curator tests covering the extractor + authoritative reconciler path + mixed-legacy-and- declared runs. Validation - 307/307 targeted tests pass (curator + cron + skill_manager suites). - E2E NousResearch#18671 repro: 3 narrow skills, 1 umbrella, cron job referencing all 3. Model emits NO YAML block. Heuristic misses (patch prose doesn't name old slugs). Delete calls carry absorbed_into. Result: both PR skills correctly classified 'consolidated' + cron rewritten ['pr-review-format', 'pr-review-checklist', 'stale-junk'] -> ['hermes-agent-dev']; stale-junk pruned via absorbed_into=''. - E2E backward-compat: delete without absorbed_into, model emits YAML -> routed via existing 'model' source, cron still rewritten correctly. * feat(curator): capture + restore cron skill links across snapshot/rollback Before this, rolling back a curator run restored the skills tree but cron jobs still pointed at the umbrella skills the curator had rewritten them to. The user would see their old narrow skills back on disk but their cron jobs still configured with the merged umbrella — not actually 'back to how it was'. Snapshot side: snapshot_skills() now captures ~/.hermes/cron/jobs.json alongside the skills tarball, as cron-jobs.json. The manifest gets a new 'cron_jobs' block with {backed_up, jobs_count} so rollback (and the CLI confirm dialog) can surface what's in the snapshot. If jobs.json is missing/unreadable/malformed, snapshot proceeds without cron data — the skills backup is the core guarantee; cron is additive. Rollback side: after the skills extract succeeds, the new _restore_cron_skill_links() reconciles the backed-up jobs into the live jobs.json SURGICALLY. Only 'skills' and 'skill' fields are restored, and only on jobs matched by id. Everything else about a cron job — schedule, last_run_at, next_run_at, enabled, prompt, workdir, hooks — is live state the user or scheduler has modified since the snapshot; overwriting it would regress unrelated activity. Reconciliation rules: - Job in backup AND live, skills differ → skills restored. - Job in backup AND live, skills match → no-op. - Job in backup, NOT in live → skipped (user deleted it after snapshot; their choice is later than the snapshot). - Job in live, NOT in backup → untouched (user created it after snapshot). - Snapshot missing cron-jobs.json at all → rollback still succeeds, reports 'not captured' (older pre-feature snapshots keep working). Writes go through cron.jobs.save_jobs under the same _jobs_file_lock the scheduler uses, so rollback doesn't race tick(). Also: - hermes_cli/curator.py: rollback confirm dialog now shows 'cron jobs: N (will be restored for skill-link fields only)' when the snapshot has cron data, or 'not in snapshot (<reason>)' otherwise. - rollback()'s message string includes a 'cron links: ...' clause summarizing the reconciliation outcome. Tests - 9 new cases: snapshot-with-cron, snapshot-without-cron, malformed-json captured-as-raw, full rollback-restores-skills-and-cron, rollback touches only skill fields, rollback skips user-deleted jobs, rollback leaves user-created jobs untouched, rollback still works with pre-feature snapshot that has no cron-jobs.json, standalone unit test on _restore_cron_skill_links exercising the full report shape. Validation - 484/484 targeted tests pass (curator + cron + skill_manager suites). - E2E: real snapshot_skills, real cron rewrite, real rollback. Before: ['pr-review-format', 'pr-review-checklist', 'pr-triage-salvage']. After curator: ['hermes-agent-dev']. After rollback: ['pr-review-format', 'pr-review-checklist', 'pr-triage-salvage']. Non-skill fields (id, name, prompt) preserved across the round trip.
1 parent 1c452fa commit bdd2120

7 files changed

Lines changed: 1049 additions & 11 deletions

File tree

agent/curator.py

Lines changed: 108 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -387,6 +387,11 @@ def apply_automatic_transitions(now: Optional[datetime] = None) -> Dict[str, int
387387
" - skill_manage action=write_file — add a references/, templates/, "
388388
"or scripts/ file under an existing skill (the skill must already "
389389
"exist)\n"
390+
" - skill_manage action=delete — archive a skill. MUST pass "
391+
"`absorbed_into=<umbrella>` when you've merged its content into another "
392+
"skill, or `absorbed_into=\"\"` when you're truly pruning with no "
393+
"forwarding target. This drives cron-job skill-reference migration — "
394+
"guessing from your YAML summary after the fact is fragile.\n"
390395
" - terminal — mv a sibling into the archive "
391396
"OR move its content into a support subfile\n\n"
392397
"'keep' is a legitimate decision ONLY when the skill is already a "
@@ -637,15 +642,76 @@ def _parse_structured_summary(
637642
return out
638643

639644

645+
def _extract_absorbed_into_declarations(
646+
tool_calls: List[Dict[str, Any]],
647+
) -> Dict[str, Dict[str, Any]]:
648+
"""Walk this run's tool calls and extract model-declared absorption targets.
649+
650+
The curator prompt requires every ``skill_manage(action='delete')`` call
651+
to pass ``absorbed_into=<umbrella>`` when consolidating, or
652+
``absorbed_into=""`` when truly pruning. This is the single authoritative
653+
signal for classification — the model's own declaration at the moment of
654+
deletion, which beats both post-hoc YAML summary parsing and substring
655+
heuristics on other tool calls.
656+
657+
Returns ``{skill_name: {"into": "<umbrella>" | "", "declared": True}}``.
658+
Entries with ``into == ""`` are explicit prunings.
659+
Skills without a ``skill_manage(delete)`` call, or with one that omitted
660+
``absorbed_into``, are not in the returned dict — caller falls back to
661+
the existing heuristic/YAML logic for those (backward compat with older
662+
curator runs and any callers that don't populate the arg).
663+
"""
664+
out: Dict[str, Dict[str, Any]] = {}
665+
for tc in tool_calls or []:
666+
if not isinstance(tc, dict):
667+
continue
668+
if tc.get("name") != "skill_manage":
669+
continue
670+
raw = tc.get("arguments") or ""
671+
args: Dict[str, Any] = {}
672+
if isinstance(raw, dict):
673+
args = raw
674+
elif isinstance(raw, str):
675+
try:
676+
args = json.loads(raw)
677+
except Exception:
678+
continue
679+
if not isinstance(args, dict):
680+
continue
681+
if args.get("action") != "delete":
682+
continue
683+
name = args.get("name")
684+
if not isinstance(name, str) or not name.strip():
685+
continue
686+
# absorbed_into must be present (even empty string is meaningful);
687+
# missing key means the model didn't declare intent.
688+
if "absorbed_into" not in args:
689+
continue
690+
target = args.get("absorbed_into")
691+
if target is None:
692+
continue
693+
if not isinstance(target, str):
694+
continue
695+
out[name.strip()] = {"into": target.strip(), "declared": True}
696+
return out
697+
698+
640699
def _reconcile_classification(
641700
removed: List[str],
642701
heuristic: Dict[str, List[Dict[str, Any]]],
643702
model_block: Dict[str, List[Dict[str, str]]],
644703
destinations: Set[str],
704+
absorbed_declarations: Optional[Dict[str, Dict[str, Any]]] = None,
645705
) -> Dict[str, List[Dict[str, Any]]]:
646706
"""Merge heuristic (tool-call evidence) with the model's structured block.
647707
648-
Rules:
708+
Rules (evaluated in order; first match wins):
709+
- **Model-declared `absorbed_into` at delete time is authoritative.** Any
710+
entry in ``absorbed_declarations`` beats every other signal. This is
711+
the model telling us directly, at the moment of deletion, what it did.
712+
``into != ""`` and target exists → consolidated. ``into == ""`` →
713+
pruned. ``into != ""`` but target doesn't exist → hallucination; fall
714+
through to the usual signals.
649715
- Model-declared consolidation wins when its ``into`` target exists
650716
in ``destinations`` (survived or newly-created). This gives the
651717
model authority over intent + rationale.
@@ -666,13 +732,45 @@ def _reconcile_classification(
666732
model_cons = {e["from"]: e for e in model_block.get("consolidations", [])}
667733
model_pruned = {e["name"]: e for e in model_block.get("prunings", [])}
668734

735+
declared = absorbed_declarations or {}
736+
669737
consolidated: List[Dict[str, Any]] = []
670738
pruned: List[Dict[str, Any]] = []
671739

672740
for name in removed:
673741
mc = model_cons.get(name)
674742
mp = model_pruned.get(name)
675743
hc = heur_cons.get(name)
744+
dec = declared.get(name)
745+
746+
# Authoritative: model declared `absorbed_into` at the delete call.
747+
if dec is not None:
748+
into_claim = dec.get("into", "")
749+
if into_claim and into_claim in destinations:
750+
entry: Dict[str, Any] = {
751+
"name": name,
752+
"into": into_claim,
753+
"source": "absorbed_into (model-declared at delete)",
754+
"reason": (mc.get("reason") or "") if mc else "",
755+
}
756+
if hc and hc.get("evidence"):
757+
entry["evidence"] = hc["evidence"]
758+
consolidated.append(entry)
759+
continue
760+
if into_claim == "":
761+
# Explicit prune declaration
762+
pruned.append({
763+
"name": name,
764+
"source": "absorbed_into=\"\" (model-declared prune)",
765+
"reason": (mp.get("reason") or "") if mp else "",
766+
})
767+
continue
768+
# into_claim is non-empty but target doesn't exist: the model
769+
# named a nonexistent umbrella at delete time. The tool already
770+
# rejects this at the skill_manage layer, so we shouldn't see it
771+
# in practice — but if it slips through (e.g. the umbrella was
772+
# deleted LATER in the same run), fall through to the usual
773+
# signals rather than trusting a broken reference.
676774

677775
# Model says consolidated — trust it if the destination is real.
678776
if mc and mc.get("into") in destinations:
@@ -808,11 +906,20 @@ def _write_run_report(
808906
)
809907
model_block = _parse_structured_summary(llm_meta.get("final", "") or "")
810908
destinations = set(after_names) | set(added or [])
909+
# Authoritative signal: extract per-delete `absorbed_into` declarations
910+
# from this run's tool calls. These beat both the YAML summary block and
911+
# the substring heuristic — the model is telling us directly, at the
912+
# moment of deletion, whether each archived skill was consolidated
913+
# (into=<umbrella>) or pruned (into="").
914+
absorbed_declarations = _extract_absorbed_into_declarations(
915+
llm_meta.get("tool_calls", []) or []
916+
)
811917
classification = _reconcile_classification(
812918
removed=removed,
813919
heuristic=heuristic,
814920
model_block=model_block,
815921
destinations=destinations,
922+
absorbed_declarations=absorbed_declarations,
816923
)
817924
consolidated = classification["consolidated"]
818925
pruned = classification["pruned"]

0 commit comments

Comments
 (0)