From df68b59149043a75c247339021112b9c4cbc5fda Mon Sep 17 00:00:00 2001 From: Madan kumar Date: Tue, 1 Sep 2026 22:03:21 +0530 Subject: [PATCH] fix(skills): reject a blank SKILL.md description at the write gate (#4867) * fix(skills): reject a blank SKILL.md description at the write gate _validate_skill_frontmatter only applied its description rules when the value was truthy, so a blank or whitespace-only description passed the gate. The loader rejects it, so the file was written to disk and then disappeared from every consumer. On the PUT edit endpoint that meant the write committed first and the response was a 404 -- the previously working skill was gone with no rollback. Same shape via the .skill install path and the agent-facing skill_manage tool, which reported success for a skill that never loads. Empty names were already rejected; description was the only field where the write gate and the loader disagreed. * test(skills): pin rollback rejection of a blank-description history entry Add a regression test for the rollback path: restoring a history entry whose stored content has an empty description must return 400 ("Description cannot be empty") and leave the on-disk SKILL.md untouched, rather than the previous destructive path that wrote the unloadable content and then 404'd. On main this returns 404, so the test also pins the intended status-code change on this path. --- .../harness/deerflow/skills/validation.py | 13 +- backend/tests/test_skills_custom_router.py | 116 ++++++++++++++++++ backend/tests/test_skills_validation.py | 31 +++++ 3 files changed, 155 insertions(+), 5 deletions(-) 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,