fix(curator): authoritative absorbed_into on delete + restore cron skill links on rollback (#18671) (#18731)
* fix(curator): authoritative absorbed_into declarations on skill delete Closes #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 #18253's cron rewriter drops the cron ref entirely instead of mapping it to the umbrella. Same observable symptom as pre-#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 #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.
This commit is contained in:
Teknium
@@ -560,8 +560,18 @@ def _patch_skill(
|
||||
}
|
||||
|
||||
|
||||
def _delete_skill(name: str) -> Dict[str, Any]:
|
||||
"""Delete a skill."""
|
||||
def _delete_skill(name: str, absorbed_into: Optional[str] = None) -> Dict[str, Any]:
|
||||
"""Delete a skill.
|
||||
|
||||
``absorbed_into`` declares intent:
|
||||
- ``None`` / missing → caller didn't declare (legacy / non-curator path);
|
||||
accepted for backward compat but logs a warning because the curator
|
||||
classification pipeline can't tell consolidation from pruning without it.
|
||||
- ``""`` (empty) → explicit "truly pruned, no forwarding target".
|
||||
- ``"<skill-name>"`` → content was absorbed into that umbrella; the
|
||||
target must exist on disk. Validated here so the model can't claim an
|
||||
umbrella that doesn't exist.
|
||||
"""
|
||||
existing = _find_skill(name)
|
||||
if not existing:
|
||||
return {"success": False, "error": f"Skill '{name}' not found."}
|
||||
@@ -570,6 +580,24 @@ def _delete_skill(name: str) -> Dict[str, Any]:
|
||||
if pinned_err:
|
||||
return {"success": False, "error": pinned_err}
|
||||
|
||||
# Validate absorbed_into target when declared non-empty
|
||||
if absorbed_into is not None and isinstance(absorbed_into, str) and absorbed_into.strip():
|
||||
target_name = absorbed_into.strip()
|
||||
if target_name == name:
|
||||
return {
|
||||
"success": False,
|
||||
"error": f"absorbed_into='{target_name}' cannot equal the skill being deleted.",
|
||||
}
|
||||
target = _find_skill(target_name)
|
||||
if not target:
|
||||
return {
|
||||
"success": False,
|
||||
"error": (
|
||||
f"absorbed_into='{target_name}' does not exist. "
|
||||
f"Create or patch the umbrella skill first, then retry the delete."
|
||||
),
|
||||
}
|
||||
|
||||
skill_dir = existing["path"]
|
||||
skills_root = _containing_skills_root(skill_dir)
|
||||
shutil.rmtree(skill_dir)
|
||||
@@ -579,9 +607,13 @@ def _delete_skill(name: str) -> Dict[str, Any]:
|
||||
if parent != skills_root and parent.exists() and not any(parent.iterdir()):
|
||||
parent.rmdir()
|
||||
|
||||
message = f"Skill '{name}' deleted."
|
||||
if absorbed_into is not None and isinstance(absorbed_into, str) and absorbed_into.strip():
|
||||
message += f" Content absorbed into '{absorbed_into.strip()}'."
|
||||
|
||||
return {
|
||||
"success": True,
|
||||
"message": f"Skill '{name}' deleted.",
|
||||
"message": message,
|
||||
}
|
||||
|
||||
|
||||
@@ -702,6 +734,7 @@ def skill_manage(
|
||||
old_string: str = None,
|
||||
new_string: str = None,
|
||||
replace_all: bool = False,
|
||||
absorbed_into: str = None,
|
||||
) -> str:
|
||||
"""
|
||||
Manage user-created skills. Dispatches to the appropriate action handler.
|
||||
@@ -726,7 +759,7 @@ def skill_manage(
|
||||
result = _patch_skill(name, old_string, new_string, file_path, replace_all)
|
||||
|
||||
elif action == "delete":
|
||||
result = _delete_skill(name)
|
||||
result = _delete_skill(name, absorbed_into=absorbed_into)
|
||||
|
||||
elif action == "write_file":
|
||||
if not file_path:
|
||||
@@ -778,6 +811,13 @@ SKILL_MANAGE_SCHEMA = {
|
||||
"patch (old_string/new_string — preferred for fixes), "
|
||||
"edit (full SKILL.md rewrite — major overhauls only), "
|
||||
"delete, write_file, remove_file.\n\n"
|
||||
"On delete, pass `absorbed_into=<umbrella>` when you're merging this "
|
||||
"skill's content into another one, or `absorbed_into=\"\"` when you're "
|
||||
"pruning it with no forwarding target. This lets the curator tell "
|
||||
"consolidation from pruning without guessing, so downstream consumers "
|
||||
"(cron jobs that reference the old skill name, etc.) get updated "
|
||||
"correctly. The target you name in `absorbed_into` must already "
|
||||
"exist — create/patch the umbrella first, then delete.\n\n"
|
||||
"Create when: complex task succeeded (5+ calls), errors overcome, "
|
||||
"user-corrected approach worked, non-trivial workflow discovered, "
|
||||
"or user asks you to remember a procedure.\n"
|
||||
@@ -855,6 +895,20 @@ SKILL_MANAGE_SCHEMA = {
|
||||
"type": "string",
|
||||
"description": "Content for the file. Required for 'write_file'."
|
||||
},
|
||||
"absorbed_into": {
|
||||
"type": "string",
|
||||
"description": (
|
||||
"For 'delete' only — declares intent so the curator can "
|
||||
"tell consolidation from pruning without guessing. "
|
||||
"Pass the umbrella skill name when this skill's content "
|
||||
"was merged into another (the target must already exist). "
|
||||
"Pass an empty string when the skill is truly stale and "
|
||||
"being pruned with no forwarding target. Omitting the arg "
|
||||
"on delete is supported for backward compatibility but "
|
||||
"downstream tooling (e.g. cron-job skill reference "
|
||||
"rewriting) will have to guess at intent."
|
||||
)
|
||||
},
|
||||
},
|
||||
"required": ["action", "name"],
|
||||
},
|
||||
@@ -877,6 +931,7 @@ registry.register(
|
||||
file_content=args.get("file_content"),
|
||||
old_string=args.get("old_string"),
|
||||
new_string=args.get("new_string"),
|
||||
replace_all=args.get("replace_all", False)),
|
||||
replace_all=args.get("replace_all", False),
|
||||
absorbed_into=args.get("absorbed_into")),
|
||||
emoji="📝",
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user