diff --git a/backend/packages/harness/deerflow/skills/validation.py b/backend/packages/harness/deerflow/skills/validation.py index c62849f8a..3da808a4a 100644 --- a/backend/packages/harness/deerflow/skills/validation.py +++ b/backend/packages/harness/deerflow/skills/validation.py @@ -64,11 +64,14 @@ def _validate_skill_frontmatter(skill_dir: Path) -> tuple[bool, str, str | None] if not isinstance(description, str): return False, f"Description must be a string, got {type(description).__name__}", None description = description.strip() - if description: - if "<" in description or ">" in description: - return False, "Description cannot contain angle brackets (< or >)", None - if len(description) > 1024: - return False, f"Description is too long ({len(description)} characters). Maximum is 1024 characters.", None + # The loader (parse_skill_file) drops a skill whose description is blank, so + # accepting one here would write a SKILL.md that never loads again. + if not description: + return False, "Description cannot be empty", None + if "<" in description or ">" in description: + return False, "Description cannot contain angle brackets (< or >)", None + if len(description) > 1024: + return False, f"Description is too long ({len(description)} characters). Maximum is 1024 characters.", None try: parse_allowed_tools(frontmatter.get("allowed-tools"), skill_md) diff --git a/backend/tests/test_skills_custom_router.py b/backend/tests/test_skills_custom_router.py index 622a94b2f..52926f331 100644 --- a/backend/tests/test_skills_custom_router.py +++ b/backend/tests/test_skills_custom_router.py @@ -550,6 +550,56 @@ def test_custom_skill_update_static_scan_failure_blocks_edit_before_llm(monkeypa assert (custom_dir / "SKILL.md").read_text(encoding="utf-8") == original_content +def test_custom_skill_update_rejects_blank_description_before_write(monkeypatch, tmp_path): + """A blank description must be rejected at the write gate, not after the file is committed. + + The loader drops a skill whose description is blank, so letting the edit + through would overwrite a working SKILL.md with content that no longer + loads -- the skill would vanish from every consumer. + """ + skills_root = tmp_path / "skills" + from deerflow.config.paths import Paths + + custom_dir = _user_custom_dir(tmp_path, "default") / "demo-skill" + custom_dir.mkdir(parents=True, exist_ok=True) + original_content = _skill_content("demo-skill") + (custom_dir / "SKILL.md").write_text(original_content, encoding="utf-8") + config = SimpleNamespace( + skills=SimpleNamespace(get_skills_path=lambda: skills_root, container_path="/mnt/skills", use="deerflow.skills.storage.local_skill_storage:LocalSkillStorage"), + skill_evolution=SimpleNamespace(enabled=True, moderation_model_name=None), + ) + monkeypatch.setattr("deerflow.config.get_app_config", lambda: config) + monkeypatch.setattr("deerflow.config.paths.get_paths", lambda: Paths(base_dir=tmp_path)) + monkeypatch.setattr("deerflow.config.paths._paths", None) + refresh_calls = [] + scan_calls = [] + + async def _refresh(user_id: str): + refresh_calls.append(("refresh", user_id)) + + async def _scan(*args, **kwargs): + scan_calls.append({"args": args, "kwargs": kwargs}) + return await _async_scan("allow", "ok") + + monkeypatch.setattr("app.gateway.routers.skills.refresh_user_skills_system_prompt_cache_async", _refresh) + monkeypatch.setattr("app.gateway.routers.skills.get_effective_user_id", lambda: "default") + monkeypatch.setattr("app.gateway.routers.skills.scan_skill_content", _scan) + + app = _make_test_app(config) + + with TestClient(app) as client: + response = client.put( + "/api/skills/custom/demo-skill", + json={"content": "---\nname: demo-skill\ndescription: ''\n---\n\n# demo-skill\n"}, + ) + + assert response.status_code == 400 + assert "empty" in response.json()["detail"].lower() + assert scan_calls == [] + assert refresh_calls == [] + assert (custom_dir / "SKILL.md").read_text(encoding="utf-8") == original_content + + def test_custom_skill_rollback_blocked_by_scanner(monkeypatch, tmp_path): skills_root = tmp_path / "skills" from deerflow.config.paths import Paths @@ -603,6 +653,72 @@ def test_custom_skill_rollback_blocked_by_scanner(monkeypatch, tmp_path): assert history_response.json()["history"][-1]["scanner"]["decision"] == "block" +def test_custom_skill_rollback_rejects_blank_description_before_write(monkeypatch, tmp_path): + """Rolling back to a history entry whose stored content has a blank + description must be refused at the gate, not committed. + + The loader drops a skill whose description is blank, so restoring such an + entry would overwrite a working SKILL.md with content that never loads + again. This pins the status-code contract for the rollback path: it must + return 400 (validation) rather than the old destructive 404, and the + on-disk file must be left untouched. + """ + skills_root = tmp_path / "skills" + from deerflow.config.paths import Paths + + user_custom = _user_custom_dir(tmp_path, "default") + custom_dir = user_custom / "demo-skill" + custom_dir.mkdir(parents=True, exist_ok=True) + current_content = _skill_content("demo-skill", "Edited skill") + (custom_dir / "SKILL.md").write_text(current_content, encoding="utf-8") + # A previous revision whose description is an empty string. + blank_content = "---\nname: demo-skill\ndescription: ''\n---\n\n# demo-skill\n" + + config = SimpleNamespace( + skills=SimpleNamespace(get_skills_path=lambda: skills_root, container_path="/mnt/skills", use="deerflow.skills.storage.local_skill_storage:LocalSkillStorage"), + skill_evolution=SimpleNamespace(enabled=True, moderation_model_name=None), + ) + monkeypatch.setattr("deerflow.config.get_app_config", lambda: config) + monkeypatch.setattr("deerflow.config.paths.get_paths", lambda: Paths(base_dir=tmp_path)) + monkeypatch.setattr("deerflow.config.paths._paths", None) + + # Roll back target is prev_content, so store the blank revision there. + storage = UserScopedSkillStorage("default", host_path=str(skills_root)) + history_file = storage.get_skill_history_file("demo-skill") + history_file.parent.mkdir(parents=True, exist_ok=True) + history_file.write_text( + '{"action":"human_edit","prev_content":' + json.dumps(blank_content) + ',"new_content":' + json.dumps(current_content) + "}\n", + encoding="utf-8", + ) + + refresh_calls = [] + scan_calls = [] + + async def _refresh(user_id: str): + refresh_calls.append(("refresh", user_id)) + + async def _scan(*args, **kwargs): + scan_calls.append({"args": args, "kwargs": kwargs}) + return await _async_scan("allow", "ok") + + monkeypatch.setattr("app.gateway.routers.skills.refresh_user_skills_system_prompt_cache_async", _refresh) + monkeypatch.setattr("app.gateway.routers.skills.get_effective_user_id", lambda: "default") + monkeypatch.setattr("app.gateway.routers.skills.scan_skill_content", _scan) + + app = _make_test_app(config) + + with TestClient(app) as client: + rollback_response = client.post("/api/skills/custom/demo-skill/rollback", json={"history_index": -1}) + + assert rollback_response.status_code == 400 + assert "Description cannot be empty" in rollback_response.json()["detail"] + # The gate runs before the scanner and before any write, so the current + # file survives untouched and no prompt-cache refresh is triggered. + assert scan_calls == [] + assert refresh_calls == [] + assert (custom_dir / "SKILL.md").read_text(encoding="utf-8") == current_content + + def test_custom_skill_delete_preserves_history_and_allows_restore(monkeypatch, tmp_path): skills_root = tmp_path / "skills" from deerflow.config.paths import Paths diff --git a/backend/tests/test_skills_validation.py b/backend/tests/test_skills_validation.py index 51ca9ae2e..de7f000c3 100644 --- a/backend/tests/test_skills_validation.py +++ b/backend/tests/test_skills_validation.py @@ -200,6 +200,37 @@ class TestValidateSkillFrontmatter: assert valid is False assert "too long" in msg.lower() + def test_description_at_max_length_accepted(self, tmp_path): + max_desc = "a" * 1024 + skill_dir = _write_skill( + tmp_path, + f"---\nname: my-skill\ndescription: {max_desc}\n---\n\nBody\n", + ) + valid, msg, name = _validate_skill_frontmatter(skill_dir) + assert valid is True + assert msg == "Skill is valid!" + assert name == "my-skill" + + def test_empty_description_rejected(self, tmp_path): + skill_dir = _write_skill( + tmp_path, + "---\nname: my-skill\ndescription: ''\n---\n\nBody\n", + ) + valid, msg, name = _validate_skill_frontmatter(skill_dir) + assert valid is False + assert "empty" in msg.lower() + assert name is None + + def test_whitespace_only_description_rejected(self, tmp_path): + skill_dir = _write_skill( + tmp_path, + "---\nname: my-skill\ndescription: ' '\n---\n\nBody\n", + ) + valid, msg, name = _validate_skill_frontmatter(skill_dir) + assert valid is False + assert "empty" in msg.lower() + assert name is None + def test_empty_name_rejected(self, tmp_path): skill_dir = _write_skill( tmp_path,