test_skill_manager_tool.py
1 """Tests for tools/skill_manager_tool.py — skill creation, editing, and deletion.""" 2 3 import json 4 from contextlib import contextmanager 5 from pathlib import Path 6 from unittest.mock import patch 7 8 import pytest 9 10 from tools.skill_manager_tool import ( 11 _validate_name, 12 _validate_category, 13 _validate_frontmatter, 14 _validate_file_path, 15 _find_skill, 16 _resolve_skill_dir, 17 _create_skill, 18 _edit_skill, 19 _patch_skill, 20 _delete_skill, 21 _write_file, 22 _remove_file, 23 skill_manage, 24 VALID_NAME_RE, 25 ALLOWED_SUBDIRS, 26 MAX_NAME_LENGTH, 27 ) 28 29 30 @contextmanager 31 def _skill_dir(tmp_path): 32 """Patch both SKILLS_DIR and get_all_skills_dirs so _find_skill searches 33 only the temp directory — not the real ~/.hermes/skills/.""" 34 with patch("tools.skill_manager_tool.SKILLS_DIR", tmp_path), \ 35 patch("agent.skill_utils.get_all_skills_dirs", return_value=[tmp_path]): 36 yield 37 38 39 VALID_SKILL_CONTENT = """\ 40 --- 41 name: test-skill 42 description: A test skill for unit testing. 43 --- 44 45 # Test Skill 46 47 Step 1: Do the thing. 48 """ 49 50 VALID_SKILL_CONTENT_2 = """\ 51 --- 52 name: test-skill 53 description: Updated description. 54 --- 55 56 # Test Skill v2 57 58 Step 1: Do the new thing. 59 """ 60 61 62 # --------------------------------------------------------------------------- 63 # _validate_name 64 # --------------------------------------------------------------------------- 65 66 67 class TestValidateName: 68 def test_valid_names(self): 69 assert _validate_name("my-skill") is None 70 assert _validate_name("skill123") is None 71 assert _validate_name("my_skill.v2") is None 72 assert _validate_name("a") is None 73 74 def test_empty_name(self): 75 assert _validate_name("") == "Skill name is required." 76 77 def test_too_long(self): 78 err = _validate_name("a" * (MAX_NAME_LENGTH + 1)) 79 assert err == f"Skill name exceeds {MAX_NAME_LENGTH} characters." 80 81 def test_uppercase_rejected(self): 82 err = _validate_name("MySkill") 83 assert "Invalid skill name 'MySkill'" in err 84 85 def test_starts_with_hyphen_rejected(self): 86 err = _validate_name("-invalid") 87 assert "Invalid skill name '-invalid'" in err 88 89 def test_special_chars_rejected(self): 90 err = _validate_name("skill/name") 91 assert "Invalid skill name 'skill/name'" in err 92 err = _validate_name("skill name") 93 assert "Invalid skill name 'skill name'" in err 94 err = _validate_name("skill@name") 95 assert "Invalid skill name 'skill@name'" in err 96 97 98 class TestValidateCategory: 99 def test_valid_categories(self): 100 assert _validate_category(None) is None 101 assert _validate_category("") is None 102 assert _validate_category("devops") is None 103 assert _validate_category("mlops-v2") is None 104 105 def test_path_traversal_rejected(self): 106 err = _validate_category("../escape") 107 assert "Invalid category '../escape'" in err 108 109 def test_absolute_path_rejected(self): 110 err = _validate_category("/tmp/escape") 111 assert "Invalid category '/tmp/escape'" in err 112 113 114 # --------------------------------------------------------------------------- 115 # _validate_frontmatter 116 # --------------------------------------------------------------------------- 117 118 119 class TestValidateFrontmatter: 120 def test_valid_content(self): 121 assert _validate_frontmatter(VALID_SKILL_CONTENT) is None 122 123 def test_empty_content(self): 124 assert _validate_frontmatter("") == "Content cannot be empty." 125 assert _validate_frontmatter(" ") == "Content cannot be empty." 126 127 def test_no_frontmatter(self): 128 err = _validate_frontmatter("# Just a heading\nSome content.\n") 129 assert err == "SKILL.md must start with YAML frontmatter (---). See existing skills for format." 130 131 def test_unclosed_frontmatter(self): 132 content = "---\nname: test\ndescription: desc\nBody content.\n" 133 assert _validate_frontmatter(content) == "SKILL.md frontmatter is not closed. Ensure you have a closing '---' line." 134 135 def test_missing_name_field(self): 136 content = "---\ndescription: desc\n---\n\nBody.\n" 137 assert _validate_frontmatter(content) == "Frontmatter must include 'name' field." 138 139 def test_missing_description_field(self): 140 content = "---\nname: test\n---\n\nBody.\n" 141 assert _validate_frontmatter(content) == "Frontmatter must include 'description' field." 142 143 def test_no_body_after_frontmatter(self): 144 content = "---\nname: test\ndescription: desc\n---\n" 145 assert _validate_frontmatter(content) == "SKILL.md must have content after the frontmatter (instructions, procedures, etc.)." 146 147 def test_invalid_yaml(self): 148 content = "---\n: invalid: yaml: {{{\n---\n\nBody.\n" 149 assert "YAML frontmatter parse error" in _validate_frontmatter(content) 150 151 152 # --------------------------------------------------------------------------- 153 # _validate_file_path — path traversal prevention 154 # --------------------------------------------------------------------------- 155 156 157 class TestValidateFilePath: 158 def test_valid_paths(self): 159 assert _validate_file_path("references/api.md") is None 160 assert _validate_file_path("templates/config.yaml") is None 161 assert _validate_file_path("scripts/train.py") is None 162 assert _validate_file_path("assets/image.png") is None 163 164 def test_empty_path(self): 165 assert _validate_file_path("") == "file_path is required." 166 167 def test_path_traversal_blocked(self): 168 err = _validate_file_path("references/../../../etc/passwd") 169 assert err == "Path traversal ('..') is not allowed." 170 171 def test_disallowed_subdirectory(self): 172 err = _validate_file_path("secret/hidden.txt") 173 assert "File must be under one of:" in err 174 assert "'secret/hidden.txt'" in err 175 176 def test_directory_only_rejected(self): 177 err = _validate_file_path("references") 178 assert "Provide a file path, not just a directory" in err 179 assert "'references/myfile.md'" in err 180 181 def test_root_level_file_rejected(self): 182 err = _validate_file_path("malicious.py") 183 assert "File must be under one of:" in err 184 assert "'malicious.py'" in err 185 186 187 # --------------------------------------------------------------------------- 188 # CRUD operations 189 # --------------------------------------------------------------------------- 190 191 192 class TestCreateSkill: 193 def test_create_skill(self, tmp_path): 194 with _skill_dir(tmp_path): 195 result = _create_skill("my-skill", VALID_SKILL_CONTENT) 196 assert result["success"] is True 197 assert (tmp_path / "my-skill" / "SKILL.md").exists() 198 199 def test_create_with_category(self, tmp_path): 200 with _skill_dir(tmp_path): 201 result = _create_skill("my-skill", VALID_SKILL_CONTENT, category="devops") 202 assert result["success"] is True 203 assert (tmp_path / "devops" / "my-skill" / "SKILL.md").exists() 204 assert result["category"] == "devops" 205 206 def test_create_duplicate_blocked(self, tmp_path): 207 with _skill_dir(tmp_path): 208 _create_skill("my-skill", VALID_SKILL_CONTENT) 209 result = _create_skill("my-skill", VALID_SKILL_CONTENT) 210 assert result["success"] is False 211 assert "already exists" in result["error"] 212 213 def test_create_invalid_name(self, tmp_path): 214 with _skill_dir(tmp_path): 215 result = _create_skill("Invalid Name!", VALID_SKILL_CONTENT) 216 assert result["success"] is False 217 218 def test_create_invalid_content(self, tmp_path): 219 with _skill_dir(tmp_path): 220 result = _create_skill("my-skill", "no frontmatter here") 221 assert result["success"] is False 222 223 def test_create_rejects_category_traversal(self, tmp_path): 224 skills_dir = tmp_path / "skills" 225 skills_dir.mkdir() 226 227 with patch("tools.skill_manager_tool.SKILLS_DIR", skills_dir), \ 228 patch("agent.skill_utils.get_all_skills_dirs", return_value=[skills_dir]): 229 result = _create_skill("my-skill", VALID_SKILL_CONTENT, category="../escape") 230 231 assert result["success"] is False 232 assert "Invalid category '../escape'" in result["error"] 233 assert not (tmp_path / "escape").exists() 234 235 def test_create_rejects_absolute_category(self, tmp_path): 236 skills_dir = tmp_path / "skills" 237 skills_dir.mkdir() 238 outside = tmp_path / "outside" 239 240 with patch("tools.skill_manager_tool.SKILLS_DIR", skills_dir), \ 241 patch("agent.skill_utils.get_all_skills_dirs", return_value=[skills_dir]): 242 result = _create_skill("my-skill", VALID_SKILL_CONTENT, category=str(outside)) 243 244 assert result["success"] is False 245 assert f"Invalid category '{outside}'" in result["error"] 246 assert not (outside / "my-skill" / "SKILL.md").exists() 247 248 249 class TestEditSkill: 250 def test_edit_existing_skill(self, tmp_path): 251 with _skill_dir(tmp_path): 252 _create_skill("my-skill", VALID_SKILL_CONTENT) 253 result = _edit_skill("my-skill", VALID_SKILL_CONTENT_2) 254 assert result["success"] is True 255 content = (tmp_path / "my-skill" / "SKILL.md").read_text() 256 assert "Updated description" in content 257 258 def test_edit_nonexistent_skill(self, tmp_path): 259 with _skill_dir(tmp_path): 260 result = _edit_skill("nonexistent", VALID_SKILL_CONTENT) 261 assert result["success"] is False 262 assert "not found" in result["error"] 263 264 def test_edit_invalid_content_rejected(self, tmp_path): 265 with _skill_dir(tmp_path): 266 _create_skill("my-skill", VALID_SKILL_CONTENT) 267 result = _edit_skill("my-skill", "no frontmatter") 268 assert result["success"] is False 269 # Original content should be preserved 270 content = (tmp_path / "my-skill" / "SKILL.md").read_text() 271 assert "A test skill" in content 272 273 274 class TestPatchSkill: 275 def test_patch_unique_match(self, tmp_path): 276 with _skill_dir(tmp_path): 277 _create_skill("my-skill", VALID_SKILL_CONTENT) 278 result = _patch_skill("my-skill", "Do the thing.", "Do the new thing.") 279 assert result["success"] is True 280 content = (tmp_path / "my-skill" / "SKILL.md").read_text() 281 assert "Do the new thing." in content 282 283 def test_patch_nonexistent_string(self, tmp_path): 284 with _skill_dir(tmp_path): 285 _create_skill("my-skill", VALID_SKILL_CONTENT) 286 result = _patch_skill("my-skill", "this text does not exist", "replacement") 287 assert result["success"] is False 288 assert "not found" in result["error"].lower() or "could not find" in result["error"].lower() 289 290 def test_patch_ambiguous_match_rejected(self, tmp_path): 291 content = """\ 292 --- 293 name: test-skill 294 description: A test skill. 295 --- 296 297 # Test 298 299 word word 300 """ 301 with _skill_dir(tmp_path): 302 _create_skill("my-skill", content) 303 result = _patch_skill("my-skill", "word", "replaced") 304 assert result["success"] is False 305 assert "match" in result["error"].lower() 306 307 def test_patch_replace_all(self, tmp_path): 308 content = """\ 309 --- 310 name: test-skill 311 description: A test skill. 312 --- 313 314 # Test 315 316 word word 317 """ 318 with _skill_dir(tmp_path): 319 _create_skill("my-skill", content) 320 result = _patch_skill("my-skill", "word", "replaced", replace_all=True) 321 assert result["success"] is True 322 323 def test_patch_supporting_file(self, tmp_path): 324 with _skill_dir(tmp_path): 325 _create_skill("my-skill", VALID_SKILL_CONTENT) 326 _write_file("my-skill", "references/api.md", "old text here") 327 result = _patch_skill("my-skill", "old text", "new text", file_path="references/api.md") 328 assert result["success"] is True 329 330 def test_patch_skill_not_found(self, tmp_path): 331 with _skill_dir(tmp_path): 332 result = _patch_skill("nonexistent", "old", "new") 333 assert result["success"] is False 334 335 def test_patch_supporting_file_symlink_escape_blocked(self, tmp_path): 336 outside_file = tmp_path / "outside.txt" 337 outside_file.write_text("old text here") 338 339 with _skill_dir(tmp_path): 340 _create_skill("my-skill", VALID_SKILL_CONTENT) 341 link = tmp_path / "my-skill" / "references" / "evil.md" 342 link.parent.mkdir(parents=True, exist_ok=True) 343 try: 344 link.symlink_to(outside_file) 345 except OSError: 346 pytest.skip("Symlinks not supported") 347 348 result = _patch_skill("my-skill", "old text", "new text", file_path="references/evil.md") 349 350 assert result["success"] is False 351 assert "escapes" in result["error"].lower() 352 assert outside_file.read_text() == "old text here" 353 354 355 class TestDeleteSkill: 356 def test_delete_existing(self, tmp_path): 357 with _skill_dir(tmp_path): 358 _create_skill("my-skill", VALID_SKILL_CONTENT) 359 result = _delete_skill("my-skill") 360 assert result["success"] is True 361 assert not (tmp_path / "my-skill").exists() 362 363 def test_delete_nonexistent(self, tmp_path): 364 with _skill_dir(tmp_path): 365 result = _delete_skill("nonexistent") 366 assert result["success"] is False 367 368 def test_delete_cleans_empty_category_dir(self, tmp_path): 369 with _skill_dir(tmp_path): 370 _create_skill("my-skill", VALID_SKILL_CONTENT, category="devops") 371 _delete_skill("my-skill") 372 assert not (tmp_path / "devops").exists() 373 374 def test_delete_with_absorbed_into_valid_target(self, tmp_path): 375 with _skill_dir(tmp_path): 376 _create_skill("umbrella", VALID_SKILL_CONTENT) 377 _create_skill("narrow", VALID_SKILL_CONTENT) 378 result = _delete_skill("narrow", absorbed_into="umbrella") 379 assert result["success"] is True 380 assert "absorbed into 'umbrella'" in result["message"] 381 assert not (tmp_path / "narrow").exists() 382 assert (tmp_path / "umbrella").exists() 383 384 def test_delete_with_absorbed_into_empty_string_means_pruned(self, tmp_path): 385 with _skill_dir(tmp_path): 386 _create_skill("stale-skill", VALID_SKILL_CONTENT) 387 result = _delete_skill("stale-skill", absorbed_into="") 388 assert result["success"] is True 389 # Empty absorbed_into is explicit prune — no "absorbed into" suffix in message 390 assert "absorbed into" not in result["message"] 391 392 def test_delete_with_absorbed_into_nonexistent_target_rejected(self, tmp_path): 393 with _skill_dir(tmp_path): 394 _create_skill("narrow", VALID_SKILL_CONTENT) 395 result = _delete_skill("narrow", absorbed_into="ghost-umbrella") 396 assert result["success"] is False 397 assert "does not exist" in result["error"] 398 # Skill must NOT have been deleted on validation failure 399 assert (tmp_path / "narrow").exists() 400 401 def test_delete_with_absorbed_into_equals_self_rejected(self, tmp_path): 402 with _skill_dir(tmp_path): 403 _create_skill("narrow", VALID_SKILL_CONTENT) 404 result = _delete_skill("narrow", absorbed_into="narrow") 405 assert result["success"] is False 406 assert "cannot equal" in result["error"] 407 assert (tmp_path / "narrow").exists() 408 409 def test_delete_with_absorbed_into_whitespace_only_treated_as_prune(self, tmp_path): 410 # Leading/trailing whitespace only: .strip() → "" → pruned path 411 with _skill_dir(tmp_path): 412 _create_skill("narrow", VALID_SKILL_CONTENT) 413 result = _delete_skill("narrow", absorbed_into=" ") 414 assert result["success"] is True 415 assert "absorbed into" not in result["message"] 416 417 def test_delete_without_absorbed_into_backward_compat(self, tmp_path): 418 # Legacy callers that don't pass the arg still work — the curator 419 # reconciler falls back to its heuristic+YAML logic for such deletes. 420 with _skill_dir(tmp_path): 421 _create_skill("my-skill", VALID_SKILL_CONTENT) 422 result = _delete_skill("my-skill") 423 assert result["success"] is True 424 425 426 # --------------------------------------------------------------------------- 427 # write_file / remove_file 428 # --------------------------------------------------------------------------- 429 430 431 class TestWriteFile: 432 def test_write_reference_file(self, tmp_path): 433 with _skill_dir(tmp_path): 434 _create_skill("my-skill", VALID_SKILL_CONTENT) 435 result = _write_file("my-skill", "references/api.md", "# API\nEndpoint docs.") 436 assert result["success"] is True 437 assert (tmp_path / "my-skill" / "references" / "api.md").exists() 438 439 def test_write_to_nonexistent_skill(self, tmp_path): 440 with _skill_dir(tmp_path): 441 result = _write_file("nonexistent", "references/doc.md", "content") 442 assert result["success"] is False 443 444 def test_write_to_disallowed_path(self, tmp_path): 445 with _skill_dir(tmp_path): 446 _create_skill("my-skill", VALID_SKILL_CONTENT) 447 result = _write_file("my-skill", "secret/evil.py", "malicious") 448 assert result["success"] is False 449 450 def test_write_symlink_escape_blocked(self, tmp_path): 451 outside_dir = tmp_path / "outside" 452 outside_dir.mkdir() 453 454 with _skill_dir(tmp_path): 455 _create_skill("my-skill", VALID_SKILL_CONTENT) 456 link = tmp_path / "my-skill" / "references" / "escape" 457 link.parent.mkdir(parents=True, exist_ok=True) 458 try: 459 link.symlink_to(outside_dir, target_is_directory=True) 460 except OSError: 461 pytest.skip("Symlinks not supported") 462 463 result = _write_file("my-skill", "references/escape/owned.md", "malicious") 464 465 assert result["success"] is False 466 assert "escapes" in result["error"].lower() 467 assert not (outside_dir / "owned.md").exists() 468 469 470 class TestRemoveFile: 471 def test_remove_existing_file(self, tmp_path): 472 with _skill_dir(tmp_path): 473 _create_skill("my-skill", VALID_SKILL_CONTENT) 474 _write_file("my-skill", "references/api.md", "content") 475 result = _remove_file("my-skill", "references/api.md") 476 assert result["success"] is True 477 assert not (tmp_path / "my-skill" / "references" / "api.md").exists() 478 479 def test_remove_nonexistent_file(self, tmp_path): 480 with _skill_dir(tmp_path): 481 _create_skill("my-skill", VALID_SKILL_CONTENT) 482 result = _remove_file("my-skill", "references/nope.md") 483 assert result["success"] is False 484 485 def test_remove_symlink_escape_blocked(self, tmp_path): 486 outside_dir = tmp_path / "outside" 487 outside_dir.mkdir() 488 outside_file = outside_dir / "keep.txt" 489 outside_file.write_text("content") 490 491 with _skill_dir(tmp_path): 492 _create_skill("my-skill", VALID_SKILL_CONTENT) 493 link = tmp_path / "my-skill" / "references" / "escape" 494 link.parent.mkdir(parents=True, exist_ok=True) 495 try: 496 link.symlink_to(outside_dir, target_is_directory=True) 497 except OSError: 498 pytest.skip("Symlinks not supported") 499 500 result = _remove_file("my-skill", "references/escape/keep.txt") 501 502 assert result["success"] is False 503 assert "escapes" in result["error"].lower() 504 assert outside_file.exists() 505 506 507 # --------------------------------------------------------------------------- 508 # skill_manage dispatcher 509 # --------------------------------------------------------------------------- 510 511 512 class TestSkillManageDispatcher: 513 def test_unknown_action(self, tmp_path): 514 with _skill_dir(tmp_path): 515 raw = skill_manage(action="explode", name="test") 516 result = json.loads(raw) 517 assert result["success"] is False 518 assert "Unknown action" in result["error"] 519 520 def test_create_without_content(self, tmp_path): 521 with _skill_dir(tmp_path): 522 raw = skill_manage(action="create", name="test") 523 result = json.loads(raw) 524 assert result["success"] is False 525 assert "content" in result["error"].lower() 526 527 def test_patch_without_old_string(self, tmp_path): 528 with _skill_dir(tmp_path): 529 raw = skill_manage(action="patch", name="test") 530 result = json.loads(raw) 531 assert result["success"] is False 532 533 def test_full_create_via_dispatcher(self, tmp_path): 534 """Foreground create does NOT mark the skill as agent-created. 535 536 Skills created by user-directed foreground turns belong to the user; 537 only the background self-improvement review fork should mark its 538 own sediment as agent-created (so the curator can later consolidate 539 or prune it). 540 """ 541 with _skill_dir(tmp_path): 542 raw = skill_manage(action="create", name="test-skill", content=VALID_SKILL_CONTENT) 543 from tools.skill_usage import load_usage 544 usage = load_usage() 545 result = json.loads(raw) 546 assert result["success"] is True 547 # No provenance marker on a foreground create — record either missing 548 # entirely (telemetry best-effort) or present with created_by unset. 549 rec = usage.get("test-skill") or {} 550 assert rec.get("created_by") in (None, "", False) 551 552 def test_create_from_background_review_marks_agent_created(self, tmp_path): 553 """Background-review fork creates ARE marked as agent-created.""" 554 from tools.skill_provenance import set_current_write_origin, BACKGROUND_REVIEW 555 token = set_current_write_origin(BACKGROUND_REVIEW) 556 try: 557 with _skill_dir(tmp_path): 558 raw = skill_manage( 559 action="create", name="review-sediment", content=VALID_SKILL_CONTENT 560 ) 561 from tools.skill_usage import load_usage 562 usage = load_usage() 563 finally: 564 from tools.skill_provenance import reset_current_write_origin 565 reset_current_write_origin(token) 566 result = json.loads(raw) 567 assert result["success"] is True 568 assert usage["review-sediment"]["created_by"] == "agent" 569 570 def test_delete_via_dispatcher_threads_absorbed_into(self, tmp_path): 571 # Dispatcher must plumb absorbed_into through to _delete_skill so the 572 # validation + message suffix paths are exercised end-to-end. 573 with _skill_dir(tmp_path): 574 skill_manage(action="create", name="umbrella", content=VALID_SKILL_CONTENT) 575 skill_manage(action="create", name="narrow", content=VALID_SKILL_CONTENT) 576 raw = skill_manage(action="delete", name="narrow", absorbed_into="umbrella") 577 result = json.loads(raw) 578 assert result["success"] is True 579 assert "absorbed into 'umbrella'" in result["message"] 580 581 def test_delete_via_dispatcher_rejects_missing_absorbed_target(self, tmp_path): 582 with _skill_dir(tmp_path): 583 skill_manage(action="create", name="narrow", content=VALID_SKILL_CONTENT) 584 raw = skill_manage(action="delete", name="narrow", absorbed_into="ghost") 585 result = json.loads(raw) 586 assert result["success"] is False 587 assert "does not exist" in result["error"] 588 589 590 class TestSecurityScanGate: 591 """_security_scan_skill is gated by skills.guard_agent_created config flag.""" 592 593 def test_scan_noop_when_flag_off(self, tmp_path): 594 """Default config (flag off) short-circuits before running scan_skill.""" 595 from tools.skill_manager_tool import _security_scan_skill 596 597 with patch("tools.skill_manager_tool._guard_agent_created_enabled", return_value=False), \ 598 patch("tools.skill_manager_tool.scan_skill") as mock_scan: 599 result = _security_scan_skill(tmp_path) 600 601 assert result is None 602 mock_scan.assert_not_called() # scan never ran 603 604 def test_scan_runs_when_flag_on(self, tmp_path): 605 """When flag is on, scan_skill is invoked and its verdict is honored.""" 606 from tools.skill_manager_tool import _security_scan_skill 607 from tools.skills_guard import ScanResult 608 609 # Fake a safe scan result — caller should return None (allow) 610 fake_result = ScanResult( 611 skill_name="test", 612 source="agent-created", 613 trust_level="agent-created", 614 verdict="safe", 615 findings=[], 616 summary="ok", 617 ) 618 with patch("tools.skill_manager_tool._guard_agent_created_enabled", return_value=True), \ 619 patch("tools.skill_manager_tool.scan_skill", return_value=fake_result) as mock_scan: 620 result = _security_scan_skill(tmp_path) 621 622 assert result is None 623 mock_scan.assert_called_once() 624 625 def test_scan_blocks_dangerous_when_flag_on(self, tmp_path): 626 """Dangerous verdict + flag on → returns an error string for the agent.""" 627 from tools.skill_manager_tool import _security_scan_skill 628 from tools.skills_guard import ScanResult, Finding 629 630 finding = Finding( 631 pattern_id="test", severity="critical", category="exfiltration", 632 file="SKILL.md", line=1, match="curl $TOKEN", description="test", 633 ) 634 fake_result = ScanResult( 635 skill_name="test", 636 source="agent-created", 637 trust_level="agent-created", 638 verdict="dangerous", 639 findings=[finding], 640 summary="dangerous", 641 ) 642 with patch("tools.skill_manager_tool._guard_agent_created_enabled", return_value=True), \ 643 patch("tools.skill_manager_tool.scan_skill", return_value=fake_result): 644 result = _security_scan_skill(tmp_path) 645 646 assert result is not None 647 assert "Security scan blocked" in result 648 649 def test_guard_flag_reads_config_default_false(self): 650 """_guard_agent_created_enabled returns False when config doesn't set it.""" 651 from tools.skill_manager_tool import _guard_agent_created_enabled 652 653 with patch("hermes_cli.config.load_config", return_value={"skills": {}}): 654 assert _guard_agent_created_enabled() is False 655 656 def test_guard_flag_reads_config_when_set(self): 657 """_guard_agent_created_enabled returns True when user explicitly enables.""" 658 from tools.skill_manager_tool import _guard_agent_created_enabled 659 660 with patch("hermes_cli.config.load_config", 661 return_value={"skills": {"guard_agent_created": True}}): 662 assert _guard_agent_created_enabled() is True 663 664 def test_guard_flag_handles_config_error(self): 665 """If load_config raises, _guard_agent_created_enabled defaults to False (fail-safe off).""" 666 from tools.skill_manager_tool import _guard_agent_created_enabled 667 668 with patch("hermes_cli.config.load_config", side_effect=RuntimeError("boom")): 669 assert _guard_agent_created_enabled() is False 670 671 def test_guard_flag_quoted_false_stays_disabled(self): 672 """Quoted 'false' from YAML edits must not enable the guard.""" 673 from tools.skill_manager_tool import _guard_agent_created_enabled 674 675 for quoted in ("false", "False", "0", "no", "off"): 676 with patch("hermes_cli.config.load_config", 677 return_value={"skills": {"guard_agent_created": quoted}}): 678 assert _guard_agent_created_enabled() is False, \ 679 f"guard_agent_created={quoted!r} must coerce to False" 680 681 def test_guard_flag_quoted_true_enables(self): 682 """Quoted truthy strings must enable the guard.""" 683 from tools.skill_manager_tool import _guard_agent_created_enabled 684 685 for quoted in ("true", "True", "1", "yes", "on"): 686 with patch("hermes_cli.config.load_config", 687 return_value={"skills": {"guard_agent_created": quoted}}): 688 assert _guard_agent_created_enabled() is True, \ 689 f"guard_agent_created={quoted!r} must coerce to True" 690 691 692 # --------------------------------------------------------------------------- 693 # External skills directories (skills.external_dirs) — mutations in place 694 # --------------------------------------------------------------------------- 695 696 697 @contextmanager 698 def _two_roots(local_dir: Path, external_dir: Path): 699 """Patch the skill manager so local SKILLS_DIR = local_dir and 700 get_all_skills_dirs() returns [local_dir, external_dir] in order.""" 701 with patch("tools.skill_manager_tool.SKILLS_DIR", local_dir), \ 702 patch("agent.skill_utils.get_all_skills_dirs", 703 return_value=[local_dir, external_dir]): 704 yield 705 706 707 def _write_external_skill(external_dir: Path, name: str = "ext-skill") -> Path: 708 skill_dir = external_dir / name 709 skill_dir.mkdir(parents=True) 710 (skill_dir / "SKILL.md").write_text( 711 f"---\nname: {name}\ndescription: An external skill.\n---\n\n" 712 "# External\n\nBody with OLD_MARKER here.\n" 713 ) 714 return skill_dir 715 716 717 class TestExternalSkillMutations: 718 """Verify skill_manage can patch/edit/write/remove/delete skills that live 719 under skills.external_dirs — in place, without duplicating to local. 720 721 Regression for issues #4759 and #4381: the read-only gate used to refuse 722 with 'Skill X is in an external directory and cannot be modified', which 723 caused agents to create duplicate copies in ~/.hermes/skills/ as a 724 workaround. 725 """ 726 727 def test_patch_external_skill_writes_in_place(self, tmp_path): 728 local = tmp_path / "local" 729 external = tmp_path / "vault" 730 local.mkdir(); external.mkdir() 731 skill_dir = _write_external_skill(external) 732 733 with _two_roots(local, external): 734 result = _patch_skill("ext-skill", "OLD_MARKER", "NEW_MARKER") 735 736 assert result["success"] is True, result 737 assert "NEW_MARKER" in (skill_dir / "SKILL.md").read_text() 738 # No duplicate in local 739 assert not (local / "ext-skill").exists() 740 741 def test_edit_external_skill_writes_in_place(self, tmp_path): 742 local = tmp_path / "local" 743 external = tmp_path / "vault" 744 local.mkdir(); external.mkdir() 745 skill_dir = _write_external_skill(external) 746 747 new_content = ( 748 "---\nname: ext-skill\ndescription: Rewritten.\n---\n\n" 749 "# Rewritten\n\nBrand new body.\n" 750 ) 751 with _two_roots(local, external): 752 result = _edit_skill("ext-skill", new_content) 753 754 assert result["success"] is True, result 755 assert "Brand new body" in (skill_dir / "SKILL.md").read_text() 756 assert not (local / "ext-skill").exists() 757 758 def test_write_file_on_external_skill(self, tmp_path): 759 local = tmp_path / "local" 760 external = tmp_path / "vault" 761 local.mkdir(); external.mkdir() 762 skill_dir = _write_external_skill(external) 763 764 with _two_roots(local, external): 765 result = _write_file("ext-skill", "references/notes.md", "# Notes\n") 766 767 assert result["success"] is True, result 768 assert (skill_dir / "references" / "notes.md").read_text() == "# Notes\n" 769 assert not (local / "ext-skill").exists() 770 771 def test_remove_file_on_external_skill(self, tmp_path): 772 local = tmp_path / "local" 773 external = tmp_path / "vault" 774 local.mkdir(); external.mkdir() 775 skill_dir = _write_external_skill(external) 776 (skill_dir / "references").mkdir() 777 (skill_dir / "references" / "notes.md").write_text("# Notes\n") 778 779 with _two_roots(local, external): 780 result = _remove_file("ext-skill", "references/notes.md") 781 782 assert result["success"] is True, result 783 assert not (skill_dir / "references" / "notes.md").exists() 784 785 def test_delete_external_skill_removes_skill_not_root(self, tmp_path): 786 local = tmp_path / "local" 787 external = tmp_path / "vault" 788 local.mkdir(); external.mkdir() 789 skill_dir = _write_external_skill(external) 790 791 with _two_roots(local, external): 792 result = _delete_skill("ext-skill") 793 794 assert result["success"] is True, result 795 assert not skill_dir.exists() 796 # The external root must NOT be rmdir'd, even when empty after deletion 797 assert external.exists() and external.is_dir() 798 799 def test_delete_external_skill_cleans_empty_category(self, tmp_path): 800 """When a skill lives under external/<category>/<name>, deleting the 801 last skill in the category should rmdir the empty category dir but 802 stop at the external root.""" 803 local = tmp_path / "local" 804 external = tmp_path / "vault" 805 local.mkdir(); external.mkdir() 806 cat_dir = external / "team" 807 cat_dir.mkdir() 808 skill_dir = cat_dir / "ext-skill" 809 skill_dir.mkdir() 810 (skill_dir / "SKILL.md").write_text( 811 "---\nname: ext-skill\ndescription: An external skill.\n---\n\n" 812 "# External\n\nBody.\n" 813 ) 814 815 with _two_roots(local, external): 816 result = _delete_skill("ext-skill") 817 818 assert result["success"] is True, result 819 assert not skill_dir.exists() 820 assert not cat_dir.exists() # empty category cleaned up 821 assert external.exists() # but never the external root 822 823 def test_create_still_writes_to_local_root(self, tmp_path): 824 """Creating a new skill always lands in local SKILLS_DIR, never 825 external_dirs — create is unchanged by this PR.""" 826 local = tmp_path / "local" 827 external = tmp_path / "vault" 828 local.mkdir(); external.mkdir() 829 830 with _two_roots(local, external): 831 result = _create_skill("fresh-skill", VALID_SKILL_CONTENT.replace( 832 "name: test-skill", "name: fresh-skill")) 833 834 assert result["success"] is True, result 835 assert (local / "fresh-skill" / "SKILL.md").exists() 836 assert not (external / "fresh-skill").exists() 837 838 839 840 # --------------------------------------------------------------------------- 841 # Pinned-skill guard — skill_manage refuses all writes to pinned skills. 842 # The user unpins via `hermes curator unpin <name>`. 843 # --------------------------------------------------------------------------- 844 845 class TestPinnedGuard: 846 """Every mutation action must refuse when the skill is pinned.""" 847 848 @staticmethod 849 def _pin(name: str): 850 """Return a patch context that marks *name* as pinned in skill_usage.""" 851 def _fake_get_record(skill_name, _name=name): 852 return {"pinned": True} if skill_name == _name else {"pinned": False} 853 return patch("tools.skill_usage.get_record", side_effect=_fake_get_record) 854 855 def test_edit_refuses_pinned(self, tmp_path): 856 with _skill_dir(tmp_path): 857 _create_skill("my-skill", VALID_SKILL_CONTENT) 858 with self._pin("my-skill"): 859 result = _edit_skill("my-skill", VALID_SKILL_CONTENT_2) 860 assert result["success"] is False 861 assert "pinned" in result["error"].lower() 862 assert "hermes curator unpin my-skill" in result["error"] 863 # Original content preserved 864 content = (tmp_path / "my-skill" / "SKILL.md").read_text() 865 assert "A test skill" in content 866 867 def test_patch_refuses_pinned(self, tmp_path): 868 with _skill_dir(tmp_path): 869 _create_skill("my-skill", VALID_SKILL_CONTENT) 870 with self._pin("my-skill"): 871 result = _patch_skill("my-skill", "Do the thing.", "Do the new thing.") 872 assert result["success"] is False 873 assert "pinned" in result["error"].lower() 874 assert "hermes curator unpin my-skill" in result["error"] 875 content = (tmp_path / "my-skill" / "SKILL.md").read_text() 876 assert "Do the thing." in content # unchanged 877 878 def test_patch_supporting_file_refuses_pinned(self, tmp_path): 879 """Pin covers supporting files too, not just SKILL.md.""" 880 with _skill_dir(tmp_path): 881 _create_skill("my-skill", VALID_SKILL_CONTENT) 882 _write_file("my-skill", "references/api.md", "original") 883 with self._pin("my-skill"): 884 result = _patch_skill( 885 "my-skill", "original", "modified", 886 file_path="references/api.md", 887 ) 888 assert result["success"] is False 889 assert "pinned" in result["error"].lower() 890 assert (tmp_path / "my-skill" / "references" / "api.md").read_text() == "original" 891 892 def test_delete_refuses_pinned(self, tmp_path): 893 with _skill_dir(tmp_path): 894 _create_skill("my-skill", VALID_SKILL_CONTENT) 895 with self._pin("my-skill"): 896 result = _delete_skill("my-skill") 897 assert result["success"] is False 898 assert "pinned" in result["error"].lower() 899 # Skill still exists 900 assert (tmp_path / "my-skill" / "SKILL.md").exists() 901 902 def test_write_file_refuses_pinned(self, tmp_path): 903 with _skill_dir(tmp_path): 904 _create_skill("my-skill", VALID_SKILL_CONTENT) 905 with self._pin("my-skill"): 906 result = _write_file("my-skill", "references/api.md", "content") 907 assert result["success"] is False 908 assert "pinned" in result["error"].lower() 909 assert not (tmp_path / "my-skill" / "references" / "api.md").exists() 910 911 def test_remove_file_refuses_pinned(self, tmp_path): 912 with _skill_dir(tmp_path): 913 _create_skill("my-skill", VALID_SKILL_CONTENT) 914 _write_file("my-skill", "references/api.md", "content") 915 with self._pin("my-skill"): 916 result = _remove_file("my-skill", "references/api.md") 917 assert result["success"] is False 918 assert "pinned" in result["error"].lower() 919 # File still there 920 assert (tmp_path / "my-skill" / "references" / "api.md").exists() 921 922 def test_unpinned_skills_still_editable(self, tmp_path): 923 """Sanity check: the guard doesn't fire for unpinned skills. 924 925 Only the specifically-pinned skill is refused; a sibling skill must 926 still be freely editable. 927 """ 928 with _skill_dir(tmp_path): 929 _create_skill("pinned-one", VALID_SKILL_CONTENT) 930 _create_skill("free-one", VALID_SKILL_CONTENT) 931 with self._pin("pinned-one"): 932 blocked = _edit_skill("pinned-one", VALID_SKILL_CONTENT_2) 933 allowed = _edit_skill("free-one", VALID_SKILL_CONTENT_2) 934 assert blocked["success"] is False 935 assert allowed["success"] is True 936 937 def test_broken_sidecar_fails_open(self, tmp_path): 938 """If skill_usage.get_record raises, we allow the write through. 939 940 Rationale: a corrupted telemetry file shouldn't lock the agent out 941 of skills it would otherwise be allowed to touch. 942 """ 943 with _skill_dir(tmp_path): 944 _create_skill("my-skill", VALID_SKILL_CONTENT) 945 with patch("tools.skill_usage.get_record", 946 side_effect=RuntimeError("sidecar broken")): 947 result = _edit_skill("my-skill", VALID_SKILL_CONTENT_2) 948 assert result["success"] is True