fix(skills): let remove_file/write_file handle binary support files and reject directories (#5893)

* fix(skills): let remove_file/write_file handle binary support files and reject directories

A .skill archive may carry binary support files such as assets/logo.png:
the installer only rejects executable binaries by magic bytes. But both
skill_manage file actions read the previous content as UTF-8 text before
touching the file — purely to fill the history record's prev_content — so
a binary member raised UnicodeDecodeError out of
SkillStorage.remove_custom_skill_file (and out of the tool's write_file
prelude) and was never removed or overwritten.

A bare support directory ("assets") also slipped through
ensure_safe_support_path, because it resolves to its own allowed root and
passes the containment check, and then raised IsADirectoryError from the
same read.

Add read_text_or_none for the history reads: UTF-8 text is returned as
before, undecodable content yields None (the record simply has no previous
text), and IO errors still propagate. Require a filename below the support
subdirectory in ensure_safe_support_path, and reject an existing directory
with ValueError in both remove_custom_skill_file and the tool's write_file,
so the model gets an actionable validation error instead of a crash.
prev_content is informational for support files — rollback only restores
SKILL.md — so recording None loses nothing that could be used.

* docs(changelog): reference #5893 in the support-file entries

---------

Co-authored-by: Willem Jiang <willem.jiang@gmail.com>

CHANGELOG.md

@@ -323,6 +323,15 @@ This release closes that milestone with **181 merged pull requests**.
 
 ### Fixed
 
+- **skills:** `skill_manage(action="remove_file")` and `write_file` now work on
+  binary support files, and reject directories cleanly. A `.skill` archive may
+  carry `assets/logo.png` (the installer only rejects *executable* binaries),
+  but both actions read the previous content as UTF-8 text — purely for the
+  history record — before touching the file, so a binary member raised
+  `UnicodeDecodeError` and was never removed or overwritten. A bare support
+  directory such as `assets` also slipped through path validation and raised
+  `IsADirectoryError`. Non-text content is now recorded as no previous text,
+  and a directory path is a validation error instead of a crash. ([#5893])
 - **projects:** The conversation-files view no longer shows an empty heading
   for a member thread that has no title yet. A thread's `display_name` is
   `null` on the wire until title generation has run (or if it never does), but
@@ -6110,4 +6119,5 @@ with **180 merged pull requests** since the first 2.0 milestone tag.
 [#5859]: https://github.com/bytedance/deer-flow/pull/5859
 [#5879]: https://github.com/bytedance/deer-flow/pull/5879
 [#5881]: https://github.com/bytedance/deer-flow/pull/5881
+[#5893]: https://github.com/bytedance/deer-flow/pull/5893
 

CHANGELOG_zh.md

@@ -328,6 +328,12 @@
 
 ### 修复
 
+- **技能：** `skill_manage(action="remove_file")` 与 `write_file` 现在可以处理二进制支持文件，
+  并会干净地拒绝目录。`.skill` 压缩包可以包含 `assets/logo.png`（安装器只拒绝*可执行*二进制），
+  但这两个操作在改动文件之前会先把原有内容按 UTF-8 文本读出（仅用于历史记录），于是二进制
+  成员会抛出 `UnicodeDecodeError`，文件既不会被删除也不会被覆盖。像 `assets` 这样的裸支持目录
+  也能通过路径校验并抛出 `IsADirectoryError`。现在非文本内容会记录为“无原有文本”，目录路径
+  则作为校验错误返回，而不是崩溃。([#5893])
 - **项目：** 会话文件视图不再为尚无标题的成员会话显示空标题。会话的 `display_name` 在标题
   生成运行之前（或从未运行时）在接口上为 `null`，但文件分组类型将其声明为必填字符串并原样
   渲染，因此在首次回复之前上传的文件会挂在一行空白之下。这类分组现在显示为“未命名”，与
@@ -5198,3 +5204,4 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子
 [#5859]: https://github.com/bytedance/deer-flow/pull/5859
 [#5879]: https://github.com/bytedance/deer-flow/pull/5879
 [#5881]: https://github.com/bytedance/deer-flow/pull/5881
+[#5893]: https://github.com/bytedance/deer-flow/pull/5893

backend/packages/harness/deerflow/skills/storage/local_skill_storage.py

@@ -111,7 +111,7 @@ class LocalSkillStorage(SkillStorage):
                 if tmp_path is not None:
                     tmp_path.unlink(missing_ok=True)
 
-    def remove_custom_skill_file(self, name: str, relative_path: str) -> str:
+    def remove_custom_skill_file(self, name: str, relative_path: str) -> str | None:
         removal = ((SkillCategory.CUSTOM, Path(name)),)
         with self._skill_projection_mutation(remove=removal):
             return super().remove_custom_skill_file(name, relative_path)

backend/packages/harness/deerflow/skills/storage/skill_storage.py

@@ -17,6 +17,19 @@ logger = logging.getLogger(__name__)
 _SKILL_NAME_PATTERN = re.compile(r"^[a-z0-9]+(?:-[a-z0-9]+)*$")
 
 
+def read_text_or_none(path: Path) -> str | None:
+    """Return *path*'s content as UTF-8 text, or ``None`` when it is not text.
+
+    For history records only: a skill's support files may be binary (the
+    installer rejects executable binaries, not images or other assets), and
+    recording "no previous text" must never abort the mutation being logged.
+    """
+    try:
+        return path.read_bytes().decode("utf-8")
+    except UnicodeDecodeError:
+        return None
+
+
 class SkillStorage(ABC):
     """Abstract base for skill storage backends.
 
@@ -96,7 +109,13 @@ class SkillStorage(ABC):
         return len(relative_parent.parts) == 1 and skill_file.parent.resolve().is_dir()
 
     def ensure_safe_support_path(self, name: str, relative_path: str) -> Path:
-        """Validate and return the resolved absolute path for a support file."""
+        """Validate and return the resolved absolute path for a support file.
+
+        The path must name a file *inside* one of the support directories: a
+        bare ``assets`` resolves to its own allowed root and would otherwise
+        pass the containment check, letting callers try to remove or overwrite
+        a directory.
+        """
         _ALLOWED_SUPPORT_SUBDIRS = {"references", "templates", "scripts", "assets"}
         skill_dir = self.get_custom_skill_dir(self.validate_skill_name(name)).resolve()
         if not relative_path or relative_path.endswith("/"):
@@ -109,6 +128,8 @@ class SkillStorage(ABC):
         top_level = relative.parts[0] if relative.parts else ""
         if top_level not in _ALLOWED_SUPPORT_SUBDIRS:
             raise ValueError(f"Supporting files must live under one of: {', '.join(sorted(_ALLOWED_SUPPORT_SUBDIRS))}.")
+        if len(relative.parts) < 2:
+            raise ValueError(f"Supporting file path must name a file inside '{top_level}/'.")
         target = (skill_dir / relative).resolve()
         allowed_root = (skill_dir / top_level).resolve()
         try:
@@ -181,12 +202,20 @@ class SkillStorage(ABC):
         Origin: ``deerflow.skills.manager.atomic_write``.
         """
 
-    def remove_custom_skill_file(self, name: str, relative_path: str) -> str:
-        """Remove a supporting file and return its previous text content."""
+    def remove_custom_skill_file(self, name: str, relative_path: str) -> str | None:
+        """Remove a supporting file and return its previous text content.
+
+        Returns ``None`` when the file was not UTF-8 text (a binary asset such
+        as ``assets/logo.png`` from a ``.skill`` archive): the previous content
+        only feeds the history record, so it must never block the removal.
+        A directory is rejected rather than removed.
+        """
         target = self.ensure_safe_support_path(name, relative_path)
+        if target.is_dir():
+            raise ValueError(f"Supporting file path '{relative_path}' is a directory, not a file.")
         if not target.exists():
             raise FileNotFoundError(f"Supporting file '{relative_path}' not found for skill '{name}'.")
-        previous_content = target.read_text(encoding="utf-8")
+        previous_content = read_text_or_none(target)
         target.unlink()
         return previous_content
 

backend/packages/harness/deerflow/tools/skill_manage_tool.py

@@ -23,7 +23,7 @@ from deerflow.skills.security_static_scanner import (
     enforce_static_scan,
 )
 from deerflow.skills.storage import get_or_new_user_skill_storage
-from deerflow.skills.storage.skill_storage import SkillStorage
+from deerflow.skills.storage.skill_storage import SkillStorage, read_text_or_none
 from deerflow.skills.types import SKILL_MD_FILE
 from deerflow.tools.sync import make_sync_tool_wrapper
 from deerflow.tools.types import Runtime
@@ -223,8 +223,12 @@ async def _skill_manage_impl(
             if path is None or content is None:
                 raise ValueError("path and content are required for write_file.")
             target = await _to_thread(skill_storage.ensure_safe_support_path, name, path)
+            if await _to_thread(target.is_dir):
+                raise ValueError(f"Supporting file path '{path}' is a directory, not a file.")
             exists = await _to_thread(target.exists)
-            prev_content = await _to_thread(target.read_text, encoding="utf-8") if exists else None
+            # A binary asset (e.g. from a .skill archive) has no previous *text*;
+            # the history record takes None rather than aborting the write.
+            prev_content = await _to_thread(read_text_or_none, target) if exists else None
             executable = "scripts/" in path or path.startswith("scripts/")
             static_findings = await _scan_static_candidate_or_raise(name, {path: content}, skill_storage)
             scan = await _scan_or_raise(content, executable=executable, location=f"{name}/{path}", static_findings=static_findings)

backend/tests/test_skill_manage_tool.py

@@ -252,6 +252,79 @@ def test_skill_manage_remove_file_updates_sandbox_projection_before_return(monke
     assert not projected_file.exists()
 
 
+def _setup_demo_skill_with_binary_asset(monkeypatch, tmp_path):
+    skills_root = tmp_path / "skills"
+    config = _make_config(skills_root)
+    monkeypatch.setattr("deerflow.config.get_app_config", lambda: config)
+    monkeypatch.setattr("deerflow.skills.security_scanner.get_app_config", lambda: config)
+    from deerflow.config.paths import Paths
+
+    monkeypatch.setattr("deerflow.config.paths.get_paths", lambda: Paths(base_dir=tmp_path))
+    monkeypatch.setattr("deerflow.config.paths._paths", None)
+
+    async def _refresh(user_id: str):
+        return None
+
+    monkeypatch.setattr(skill_manage_module, "refresh_user_skills_system_prompt_cache_async", _refresh)
+    monkeypatch.setattr(skill_manage_module, "scan_skill_content", lambda *args, **kwargs: _async_result("allow", "ok"))
+    runtime = _make_runtime(user_id="default")
+    anyio.run(skill_manage_module.skill_manage_tool.coroutine, runtime, "create", "demo-skill", _skill_content("demo-skill"))
+    # A binary support file as a .skill archive install would leave it (the
+    # installer only rejects *executable* binaries).
+    skill_dir = tmp_path / "users" / "default" / "skills" / "custom" / "demo-skill"
+    (skill_dir / "assets" / "nested").mkdir(parents=True)
+    (skill_dir / "assets" / "logo.png").write_bytes(b"\x89PNG\r\n\x1a\n\xff\xfe\x00")
+    (skill_dir / "assets" / "nested" / "inner.txt").write_text("inner", encoding="utf-8")
+    return runtime, skill_dir
+
+
+def _history_records(skill_dir: Path) -> list[dict]:
+    import json
+
+    history_file = skill_dir.parent.parent / "history" / "demo-skill.jsonl"
+    if not history_file.exists():  # storage layouts differ; fall back to a search
+        history_file = next(p for p in skill_dir.parent.parent.rglob("*.jsonl") if "demo-skill" in p.name)
+    return [json.loads(line) for line in history_file.read_text(encoding="utf-8").splitlines() if line.strip()]
+
+
+def test_skill_manage_remove_file_handles_a_binary_support_file(monkeypatch, tmp_path):
+    runtime, skill_dir = _setup_demo_skill_with_binary_asset(monkeypatch, tmp_path)
+
+    result = anyio.run(skill_manage_module.skill_manage_tool.coroutine, runtime, "remove_file", "demo-skill", None, "assets/logo.png")
+
+    assert result == "Removed 'assets/logo.png' from custom skill 'demo-skill'."
+    assert not (skill_dir / "assets" / "logo.png").exists()
+    record = _history_records(skill_dir)[-1]
+    assert record["action"] == "remove_file"
+    assert record["file_path"] == "assets/logo.png"
+    assert record["prev_content"] is None
+
+
+def test_skill_manage_write_file_can_overwrite_a_binary_support_file(monkeypatch, tmp_path):
+    runtime, skill_dir = _setup_demo_skill_with_binary_asset(monkeypatch, tmp_path)
+
+    result = anyio.run(skill_manage_module.skill_manage_tool.coroutine, runtime, "write_file", "demo-skill", "now text", "assets/logo.png")
+
+    assert result == "Wrote 'assets/logo.png' for custom skill 'demo-skill'."
+    assert (skill_dir / "assets" / "logo.png").read_text(encoding="utf-8") == "now text"
+    record = _history_records(skill_dir)[-1]
+    assert record["action"] == "write_file"
+    assert record["prev_content"] is None
+    assert record["new_content"] == "now text"
+
+
+@pytest.mark.parametrize("action, content", [("remove_file", None), ("write_file", "x")])
+@pytest.mark.parametrize("path", ["assets", "assets/nested"], ids=["bare-subdir", "nested-dir"])
+def test_skill_manage_rejects_a_support_directory_path(monkeypatch, tmp_path, action, content, path):
+    runtime, skill_dir = _setup_demo_skill_with_binary_asset(monkeypatch, tmp_path)
+
+    with pytest.raises(ValueError, match="file"):
+        anyio.run(skill_manage_module.skill_manage_tool.coroutine, runtime, action, "demo-skill", content, path)
+
+    assert (skill_dir / "assets" / "logo.png").exists()
+    assert (skill_dir / "assets" / "nested" / "inner.txt").read_text(encoding="utf-8") == "inner"
+
+
 def test_skill_manage_static_critical_blocks_create_before_llm(monkeypatch, tmp_path):
     skills_root = tmp_path / "skills"
     config = _make_config(skills_root)

backend/tests/test_skill_storage_support_files.py

@@ -0,0 +1,89 @@
+"""Support-file removal must work for binary members and reject directories.
+
+``.skill`` archives may carry binary support files (``assets/logo.png``): the
+installer only rejects executable binaries by magic bytes. The storage's
+``remove_custom_skill_file`` read the file as UTF-8 text *before* unlinking
+it, purely to hand the previous content to the history record, so a binary
+member raised ``UnicodeDecodeError`` and was never removed, and a support
+*directory* (which ``ensure_safe_support_path`` let through because a bare
+``assets`` resolves to its own allowed root) raised ``IsADirectoryError``.
+"""
+
+from __future__ import annotations
+
+from pathlib import Path
+
+import pytest
+
+from deerflow.skills.storage import get_or_new_skill_storage, reset_skill_storage
+from deerflow.skills.storage.skill_storage import read_text_or_none
+
+PNG_BYTES = b"\x89PNG\r\n\x1a\n\xff\xfe\x00"
+
+
+@pytest.fixture(autouse=True)
+def _reset_storages():
+    reset_skill_storage()
+    yield
+    reset_skill_storage()
+
+
+@pytest.fixture()
+def storage(tmp_path: Path):
+    return get_or_new_skill_storage(skills_path=str(tmp_path))
+
+
+@pytest.fixture()
+def skill_dir(tmp_path: Path, storage) -> Path:
+    d = tmp_path / "custom" / "demo-skill"
+    (d / "assets" / "nested").mkdir(parents=True)
+    (d / "SKILL.md").write_text("---\nname: demo-skill\ndescription: x\n---\nbody\n", encoding="utf-8")
+    (d / "assets" / "logo.png").write_bytes(PNG_BYTES)
+    (d / "assets" / "notes.md").write_text("notes", encoding="utf-8")
+    (d / "assets" / "nested" / "inner.txt").write_text("inner", encoding="utf-8")
+    return d
+
+
+def test_remove_binary_support_file_removes_it_and_reports_no_text(storage, skill_dir):
+    previous = storage.remove_custom_skill_file("demo-skill", "assets/logo.png")
+
+    assert previous is None
+    assert not (skill_dir / "assets" / "logo.png").exists()
+
+
+def test_remove_text_support_file_still_returns_its_content(storage, skill_dir):
+    previous = storage.remove_custom_skill_file("demo-skill", "assets/notes.md")
+
+    assert previous == "notes"
+    assert not (skill_dir / "assets" / "notes.md").exists()
+
+
+@pytest.mark.parametrize("path", ["assets", "assets/", "assets/nested"], ids=["bare-subdir", "bare-subdir-slash", "nested-dir"])
+def test_remove_rejects_a_directory_and_leaves_it_in_place(storage, skill_dir, path):
+    with pytest.raises(ValueError, match="file"):
+        storage.remove_custom_skill_file("demo-skill", path)
+
+    assert (skill_dir / "assets" / "nested" / "inner.txt").read_text(encoding="utf-8") == "inner"
+    assert (skill_dir / "assets" / "logo.png").read_bytes() == PNG_BYTES
+
+
+def test_ensure_safe_support_path_requires_a_filename_below_the_subdir(storage, skill_dir):
+    with pytest.raises(ValueError, match="file"):
+        storage.ensure_safe_support_path("demo-skill", "assets")
+    assert storage.ensure_safe_support_path("demo-skill", "assets/logo.png") == (skill_dir / "assets" / "logo.png").resolve()
+
+
+def test_read_text_or_none_decodes_text_and_returns_none_for_binary(tmp_path: Path):
+    text = tmp_path / "t.md"
+    text.write_text("héllo", encoding="utf-8")
+    binary = tmp_path / "b.png"
+    binary.write_bytes(PNG_BYTES)
+
+    assert read_text_or_none(text) == "héllo"
+    assert read_text_or_none(binary) is None
+
+
+def test_read_text_or_none_only_masks_undecodable_content(tmp_path: Path):
+    """``None`` means "not text", never "could not read": IO errors still propagate."""
+    with pytest.raises(FileNotFoundError):
+        read_text_or_none(tmp_path / "missing.md")