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.

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)

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

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,