From d53ec767afa8fda7b5f65b10a530ded351ab5119 Mon Sep 17 00:00:00 2001 From: Felix Shi <3283655504@qq.com> Date: Mon, 26 Oct 2026 18:45:49 +0800 Subject: [PATCH] fix(persistence): keep LF bytes in skill and agent text writes (#7501) * fix(persistence): keep LF bytes in skill and agent text writes Text mode translates a newline into os.linesep on write, so on Windows every caller-supplied LF reached disk as CRLF while readers use universal newlines or decode bytes explicitly. The difference stayed invisible: the API returned LF, the file held CRLF, and a skill history record's prev_content described a revision no API view could return (#7174). Pass newline="" at the ten writers that persist or judge caller text: the local and user-scoped skill writers, the skill history JSONL, the user skill-states JSON, the SOUL.md/config.yaml staging in FileAgentStore, the managed-subagent store, setup_agent's SOUL.md publish, the USER.md route, and the two temp copies the frontmatter validator and the static scanner judge. The tests emulate the Windows translation on io.open, so the regression runs on the Linux CI shards instead of only on a Windows host: 10 failed before this commit, 10 pass after. * fix(skills): preserve candidate newlines during static scan --------- Co-authored-by: Felix Shi <204130665+shi-felix@users.noreply.github.com> Co-authored-by: Willem Jiang --- README.md | 3 + backend/app/gateway/routers/agents.py | 5 +- backend/app/gateway/routers/skills.py | 5 +- .../deerflow/persistence/agents/file.py | 5 +- .../persistence/managed_subagents/file.py | 2 +- .../skills/storage/local_skill_storage.py | 10 +- .../deerflow/skills/storage/skill_storage.py | 5 +- .../storage/user_scoped_skill_storage.py | 4 +- .../packages/harness/deerflow/tools/AGENTS.md | 6 + .../tools/builtins/setup_agent_tool.py | 2 +- .../deerflow/tools/skill_manage_tool.py | 4 +- .../tests/test_text_writes_keep_lf_bytes.py | 259 ++++++++++++++++++ 12 files changed, 299 insertions(+), 11 deletions(-) create mode 100644 backend/tests/test_text_writes_keep_lf_bytes.py diff --git a/README.md b/README.md index 422c561aeb..dc3f074c40 100644 --- a/README.md +++ b/README.md @@ -1711,6 +1711,9 @@ Discovery follows operator-managed directory symlinks, but skips links back to a Skill Markdown and bundled text resources use UTF-8. Skill-creator CLI and review utilities read and write text explicitly as UTF-8 so localized skills behave consistently across operating systems. +Custom-skill writes and security-scan staging preserve submitted line endings +on every platform, so the scanner checks the bytes that will be stored. + Skill-creator benchmark aggregation uses recorded `timing.json` token counts even when `grading.json` already contains the run duration. Recorded zero counts are preserved; output character counts remain a legacy proxy only when token data is missing. diff --git a/backend/app/gateway/routers/agents.py b/backend/app/gateway/routers/agents.py index e9fc00eb27..e081139d6f 100644 --- a/backend/app/gateway/routers/agents.py +++ b/backend/app/gateway/routers/agents.py @@ -747,7 +747,10 @@ async def update_user_profile(body: UserProfileUpdateRequest, request: Request) def _write_profile() -> Path: user_md_path = get_paths().user_md_file(user_id) user_md_path.parent.mkdir(parents=True, exist_ok=True) - user_md_path.write_text(body.content, encoding="utf-8") + # newline="" stores the submitted text verbatim; a default text write + # would turn its LF into CRLF on Windows, so the bytes on disk no longer + # matched the content this endpoint accepts and returns. + user_md_path.write_text(body.content, encoding="utf-8", newline="") return user_md_path try: diff --git a/backend/app/gateway/routers/skills.py b/backend/app/gateway/routers/skills.py index b4b7833662..9d32f5a576 100644 --- a/backend/app/gateway/routers/skills.py +++ b/backend/app/gateway/routers/skills.py @@ -272,7 +272,10 @@ async def _scan_static_skill_markdown_or_raise(skill_name: str, content: str, *, with tempfile.TemporaryDirectory() as tmp: skill_dir = Path(tmp) / skill_name skill_dir.mkdir(parents=True) - (skill_dir / SKILL_MD_FILE).write_text(content, encoding="utf-8") + # The static scan must see the bytes the caller submitted: on + # Windows a default text write stores CRLF, so the scan would judge + # text that never reaches the skill file. + (skill_dir / SKILL_MD_FILE).write_text(content, encoding="utf-8", newline="") return enforce_static_scan(skill_dir, skill_name=skill_name, app_config=app_config) try: diff --git a/backend/packages/harness/deerflow/persistence/agents/file.py b/backend/packages/harness/deerflow/persistence/agents/file.py index c9fd69ebfd..349dd25865 100644 --- a/backend/packages/harness/deerflow/persistence/agents/file.py +++ b/backend/packages/harness/deerflow/persistence/agents/file.py @@ -264,6 +264,9 @@ class FileAgentStore(AgentStore): def _stage_temp(target: Path, text: str) -> Path: - with tempfile.NamedTemporaryFile("w", encoding="utf-8", dir=target.parent, suffix=".tmp", delete=False) as f: + # newline="" keeps the operator's/agent's bytes: the default text write + # translates LF into os.linesep, so SOUL.md and config.yaml landed with + # CRLF on Windows while every reader saw LF. + with tempfile.NamedTemporaryFile("w", encoding="utf-8", newline="", dir=target.parent, suffix=".tmp", delete=False) as f: f.write(text) return Path(f.name) diff --git a/backend/packages/harness/deerflow/persistence/managed_subagents/file.py b/backend/packages/harness/deerflow/persistence/managed_subagents/file.py index a88ae67714..3ab7e95c24 100644 --- a/backend/packages/harness/deerflow/persistence/managed_subagents/file.py +++ b/backend/packages/harness/deerflow/persistence/managed_subagents/file.py @@ -89,7 +89,7 @@ class FileManagedSubagentStore(ManagedSubagentStore): payload = definition.model_dump_json(indent=2) + "\n" tmp_path: Path | None = None try: - with tempfile.NamedTemporaryFile("w", encoding="utf-8", dir=path.parent, suffix=".tmp", delete=False) as handle: + with tempfile.NamedTemporaryFile("w", encoding="utf-8", newline="", dir=path.parent, suffix=".tmp", delete=False) as handle: handle.write(payload) handle.flush() os.fsync(handle.fileno()) diff --git a/backend/packages/harness/deerflow/skills/storage/local_skill_storage.py b/backend/packages/harness/deerflow/skills/storage/local_skill_storage.py index 8eeca6ab17..cbf3d13689 100644 --- a/backend/packages/harness/deerflow/skills/storage/local_skill_storage.py +++ b/backend/packages/harness/deerflow/skills/storage/local_skill_storage.py @@ -109,7 +109,10 @@ class LocalSkillStorage(SkillStorage): target.parent.mkdir(parents=True, exist_ok=True) tmp_path = None try: - with tempfile.NamedTemporaryFile("w", encoding="utf-8", delete=False, dir=str(target.parent)) as tmp_file: + # newline="" publishes the caller's bytes: a default text write + # turns LF into os.linesep, so a SKILL.md written on Windows + # reached disk with CRLF that the API never returned. + with tempfile.NamedTemporaryFile("w", encoding="utf-8", newline="", delete=False, dir=str(target.parent)) as tmp_file: tmp_path = Path(tmp_file.name) tmp_file.write(content) tmp_path.replace(target) @@ -267,7 +270,10 @@ class LocalSkillStorage(SkillStorage): payload = {"ts": datetime.now(UTC).isoformat(), **record} history_path = self.get_skill_history_file(name) history_path.parent.mkdir(parents=True, exist_ok=True) - with history_path.open("a", encoding="utf-8") as f: + # JSONL records are LF-delimited on every host: a translated record + # boundary made read_history and any raw-byte reader describe different + # revisions of the same entry. + with history_path.open("a", encoding="utf-8", newline="") as f: f.write(json.dumps(payload, ensure_ascii=False)) f.write("\n") diff --git a/backend/packages/harness/deerflow/skills/storage/skill_storage.py b/backend/packages/harness/deerflow/skills/storage/skill_storage.py index 6e27fd5958..c1ea77c810 100644 --- a/backend/packages/harness/deerflow/skills/storage/skill_storage.py +++ b/backend/packages/harness/deerflow/skills/storage/skill_storage.py @@ -116,7 +116,10 @@ class SkillStorage(ABC): with tempfile.TemporaryDirectory() as tmp_dir: temp_skill_dir = Path(tmp_dir) / SkillStorage.validate_skill_name(name) temp_skill_dir.mkdir(parents=True, exist_ok=True) - (temp_skill_dir / SKILL_MD_FILE).write_text(content, encoding="utf-8") + # The validator must see the bytes the caller submitted: on Windows + # a default text write stores CRLF, so the scan would judge text the + # skill file never holds. + (temp_skill_dir / SKILL_MD_FILE).write_text(content, encoding="utf-8", newline="") is_valid, message, parsed_name = _validate_skill_frontmatter(temp_skill_dir) if not is_valid: raise ValueError(message) diff --git a/backend/packages/harness/deerflow/skills/storage/user_scoped_skill_storage.py b/backend/packages/harness/deerflow/skills/storage/user_scoped_skill_storage.py index ec055fef76..dd8ce825fa 100644 --- a/backend/packages/harness/deerflow/skills/storage/user_scoped_skill_storage.py +++ b/backend/packages/harness/deerflow/skills/storage/user_scoped_skill_storage.py @@ -133,7 +133,7 @@ class UserScopedSkillStorage(LocalSkillStorage): ) tmp_path = Path(tmp_path_str) try: - handle = os.fdopen(fd, "w", encoding="utf-8") + handle = os.fdopen(fd, "w", encoding="utf-8", newline="") fd = None # The file object now owns the descriptor. with handle: json.dump(states, handle, indent=2) @@ -412,7 +412,7 @@ class UserScopedSkillStorage(LocalSkillStorage): target.parent.mkdir(parents=True, exist_ok=True) tmp_path = None try: - with tempfile.NamedTemporaryFile("w", encoding="utf-8", delete=False, dir=str(target.parent)) as tmp_file: + with tempfile.NamedTemporaryFile("w", encoding="utf-8", newline="", delete=False, dir=str(target.parent)) as tmp_file: tmp_path = Path(tmp_file.name) tmp_file.write(content) tmp_path.replace(target) diff --git a/backend/packages/harness/deerflow/tools/AGENTS.md b/backend/packages/harness/deerflow/tools/AGENTS.md index 417a4fef69..66a8af3308 100644 --- a/backend/packages/harness/deerflow/tools/AGENTS.md +++ b/backend/packages/harness/deerflow/tools/AGENTS.md @@ -1,5 +1,11 @@ ### Tool System (`packages/harness/deerflow/tools/`) +`skill_manage_tool.py` stages candidate text with UTF-8 and `newline=""`, +matching skill storage. Scan the exact submitted bytes for `SKILL.md` and +supporting files, including overlays on existing trees. +`tests/test_text_writes_keep_lf_bytes.py` emulates Windows writes and compares +scanner input with published bytes for LF and CRLF content. + Artifact capture accepts standard top-level `url`/`mime_type` content blocks and legacy `source.url`/`source.mime_type`. A present top-level `url` selects that shape, even when invalid; never mix its MIME metadata with a conflicting legacy diff --git a/backend/packages/harness/deerflow/tools/builtins/setup_agent_tool.py b/backend/packages/harness/deerflow/tools/builtins/setup_agent_tool.py index 925edb8dd9..8979bf8e36 100644 --- a/backend/packages/harness/deerflow/tools/builtins/setup_agent_tool.py +++ b/backend/packages/harness/deerflow/tools/builtins/setup_agent_tool.py @@ -39,7 +39,7 @@ def _publish_text(target: Path, text: str) -> None: mode = 0o644 temporary: Path | None = None try: - with tempfile.NamedTemporaryFile("w", encoding="utf-8", dir=target.parent, suffix=".tmp", delete=False) as handle: + with tempfile.NamedTemporaryFile("w", encoding="utf-8", newline="", dir=target.parent, suffix=".tmp", delete=False) as handle: temporary = Path(handle.name) handle.write(text) handle.flush() diff --git a/backend/packages/harness/deerflow/tools/skill_manage_tool.py b/backend/packages/harness/deerflow/tools/skill_manage_tool.py index 0c34782d2c..7a63dc2180 100644 --- a/backend/packages/harness/deerflow/tools/skill_manage_tool.py +++ b/backend/packages/harness/deerflow/tools/skill_manage_tool.py @@ -165,7 +165,9 @@ async def _scan_static_candidate_or_raise(name: str, updates: dict[str, str], sk for relative_path, content in updates.items(): target = skill_dir / relative_path target.parent.mkdir(parents=True, exist_ok=True) - target.write_text(content, encoding="utf-8") + # Match the bytes skill storage publishes, including on Windows. + # Default text mode would rewrite LF before the scanner reads it. + target.write_text(content, encoding="utf-8", newline="") return enforce_static_scan(skill_dir, skill_name=name) try: diff --git a/backend/tests/test_text_writes_keep_lf_bytes.py b/backend/tests/test_text_writes_keep_lf_bytes.py new file mode 100644 index 0000000000..3b2d0d107f --- /dev/null +++ b/backend/tests/test_text_writes_keep_lf_bytes.py @@ -0,0 +1,259 @@ +"""Persistence writers publish the caller's bytes on every host (#7174). + +Python's default text mode translates ``\\n`` into ``os.linesep`` on write, so +on a Windows host every one of these writers stored CRLF for content the caller +submitted with LF. Readers use universal newlines (or decode explicitly), so +the mismatch stayed invisible: the API returned LF, the file held CRLF, and a +skill-history record's ``prev_content`` described a revision no API view could +return. + +Each test emulates the Windows translation instead of running on Windows, so +the regression is exercised on the Linux CI shards too. +""" + +from __future__ import annotations + +import builtins +import io +from contextlib import contextmanager +from pathlib import Path +from unittest.mock import patch + +import pytest + +from deerflow.config.paths import Paths, get_paths +from deerflow.persistence.agents.file import FileAgentStore +from deerflow.persistence.managed_subagents import ManagedSubagentDefinition +from deerflow.persistence.managed_subagents.file import FileManagedSubagentStore +from deerflow.runtime.user_context import get_effective_user_id +from deerflow.skills.storage import get_or_new_skill_storage, reset_skill_storage +from deerflow.skills.storage.user_scoped_skill_storage import UserScopedSkillStorage +from deerflow.tools.builtins.setup_agent_tool import _publish_text + +LF_CONTENT = "---\nname: demo-skill\ndescription: Demo skill\n---\n\n# Demo\nsecond line\n" + + +@contextmanager +def _windows_text_writes(monkeypatch): + """Make text-mode writes translate ``\\n`` the way Windows does by default. + + ``Path.open``, ``Path.write_text``, ``tempfile.NamedTemporaryFile`` and + ``os.fdopen`` all funnel through ``io.open``, so one patch covers every + writer under test. Binary modes are left alone. + """ + real_open = io.open + + def translating_open( + file, + mode="r", + buffering=-1, + encoding=None, + errors=None, + newline=None, + closefd=True, + opener=None, + ): + # ``Path.open`` passes newline positionally while ``os.fdopen`` and + # ``NamedTemporaryFile`` pass it by keyword, so both are spelled out. + if "b" not in mode and any(flag in mode for flag in "wax+") and newline is None: + newline = "\r\n" + return real_open(file, mode, buffering, encoding, errors, newline, closefd, opener) + + monkeypatch.setattr(io, "open", translating_open) + # ``builtins.open`` is the same function object under a separate binding; + # writers that call the builtin directly must be translated too. + monkeypatch.setattr(builtins, "open", translating_open) + try: + yield + finally: + monkeypatch.setattr(io, "open", real_open) + monkeypatch.setattr(builtins, "open", real_open) + + +@pytest.fixture(autouse=True) +def _reset_skill_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 / "skills")) + + +@pytest.fixture() +def user_storage(tmp_path: Path): + with patch("deerflow.config.paths.get_paths", return_value=Paths(base_dir=tmp_path)): + with patch("deerflow.config.paths._paths", None): + yield UserScopedSkillStorage("test-user", host_path=str(tmp_path)) + + +def test_local_skill_write_stores_lf_bytes(storage, monkeypatch) -> None: + with _windows_text_writes(monkeypatch): + storage.write_custom_skill("demo-skill", "SKILL.md", LF_CONTENT) + + stored = storage.get_custom_skill_dir("demo-skill") / "SKILL.md" + assert stored.read_bytes() == LF_CONTENT.encode("utf-8") + + +def test_skill_history_records_lf_bytes(storage, monkeypatch) -> None: + """A translated record boundary makes the stored revision unreproducible. + + ``prev_content`` is read back raw for rollback; if the record separating it + from the next entry became CRLF, the history described text the skill API + never returns. + """ + storage.write_custom_skill("demo-skill", "SKILL.md", LF_CONTENT) + with _windows_text_writes(monkeypatch): + storage.append_history("demo-skill", {"action": "update", "prev_content": LF_CONTENT}) + + raw = storage.get_skill_history_file("demo-skill").read_bytes() + assert b"\r\n" not in raw + assert raw.endswith(b"\n") + assert raw.count(b"\n") == 1 + + +def test_user_scoped_skill_write_stores_lf_bytes(user_storage, monkeypatch) -> None: + with _windows_text_writes(monkeypatch): + user_storage.write_custom_skill("demo-skill", "SKILL.md", LF_CONTENT) + + stored = user_storage.get_custom_skill_dir("demo-skill") / "SKILL.md" + assert stored.read_bytes() == LF_CONTENT.encode("utf-8") + + +def test_user_scoped_skill_states_store_lf_bytes(user_storage, tmp_path: Path, monkeypatch) -> None: + with _windows_text_writes(monkeypatch): + user_storage.set_skill_enabled_state("demo-skill", False) + + states_files = list(tmp_path.rglob("_skill_states.json")) + assert states_files, "skill state file was not written" + assert b"\r\n" not in states_files[0].read_bytes() + + +def test_agent_store_soul_keeps_lf_bytes(tmp_path: Path, monkeypatch) -> None: + monkeypatch.setenv("DEER_FLOW_HOME", str(tmp_path)) + monkeypatch.setattr("deerflow.config.paths._paths", None) + soul = "# Persona\nAnswer briefly.\n" + with _windows_text_writes(monkeypatch): + FileAgentStore().create("lf-agent", {"description": "demo"}, soul) + + soul_path = get_paths().user_agent_dir(get_effective_user_id(), "lf-agent") / "SOUL.md" + assert soul_path.read_bytes() == soul.encode("utf-8") + + +def test_managed_subagent_file_store_keeps_lf_bytes(tmp_path: Path, monkeypatch) -> None: + monkeypatch.setenv("DEER_FLOW_HOME", str(tmp_path)) + monkeypatch.setattr("deerflow.config.paths._paths", None) + definition = ManagedSubagentDefinition( + name="lf-researcher", + description="Researches a bounded topic", + system_prompt="Line one\nLine two\n", + ) + with _windows_text_writes(monkeypatch): + FileManagedSubagentStore().create(definition) + + stored = get_paths().managed_subagent_file("lf-researcher") + assert stored.read_bytes() == (definition.model_dump_json(indent=2) + "\n").encode("utf-8") + + +def test_setup_agent_publish_keeps_lf_bytes(tmp_path: Path, monkeypatch) -> None: + target = tmp_path / "SOUL.md" + with _windows_text_writes(monkeypatch): + _publish_text(target, "# Persona\nStay terse.\n") + + assert target.read_bytes() == b"# Persona\nStay terse.\n" + + +@pytest.mark.asyncio +async def test_user_profile_update_keeps_lf_bytes(tmp_path: Path, monkeypatch) -> None: + from app.gateway.routers.agents import UserProfileUpdateRequest, update_user_profile + from deerflow.config.agents_api_config import load_agents_api_config_from_dict + + monkeypatch.setenv("DEER_FLOW_HOME", str(tmp_path)) + monkeypatch.setattr("deerflow.config.paths._paths", None) + load_agents_api_config_from_dict({"enabled": True}) + try: + with _windows_text_writes(monkeypatch): + await update_user_profile(UserProfileUpdateRequest(content="I prefer terse answers.\n")) + finally: + load_agents_api_config_from_dict({}) + + user_md = get_paths().user_md_file(get_effective_user_id()) + assert user_md.read_bytes() == b"I prefer terse answers.\n" + + +def test_skill_markdown_validation_reads_lf_bytes(monkeypatch) -> None: + """The frontmatter validator has to see the bytes the caller submitted.""" + import deerflow.skills.validation as validation + from deerflow.skills.storage.skill_storage import SkillStorage + + seen: list[bytes] = [] + + def fake_validate(skill_dir: Path): + seen.append((Path(skill_dir) / "SKILL.md").read_bytes()) + return True, "", "demo-skill" + + monkeypatch.setattr(validation, "_validate_skill_frontmatter", fake_validate) + with _windows_text_writes(monkeypatch): + SkillStorage.validate_skill_markdown_content("demo-skill", LF_CONTENT) + + assert seen == [LF_CONTENT.encode("utf-8")] + + +@pytest.mark.asyncio +async def test_static_skill_scan_reads_lf_bytes(monkeypatch) -> None: + """The static scan has to see the bytes the caller submitted.""" + import app.gateway.routers.skills as skills_router + + seen: list[bytes] = [] + + def fake_scan(skill_dir, *, skill_name, app_config): + seen.append((Path(skill_dir) / "SKILL.md").read_bytes()) + return [] + + monkeypatch.setattr(skills_router, "enforce_static_scan", fake_scan) + with _windows_text_writes(monkeypatch): + findings = await skills_router._scan_static_skill_markdown_or_raise( + "demo-skill", + LF_CONTENT, + app_config=None, + ) + + assert findings == [] + assert seen == [LF_CONTENT.encode("utf-8")] + + +@pytest.mark.asyncio +@pytest.mark.parametrize("existing", [False, True], ids=["new-skill", "existing-skill"]) +@pytest.mark.parametrize("newline", ["\n", "\r\n"], ids=["lf", "crlf"]) +async def test_skill_manage_static_scan_matches_stored_bytes(storage, monkeypatch, existing, newline) -> None: + """Agent-managed candidates must be scanned exactly as storage publishes them.""" + import deerflow.tools.skill_manage_tool as skill_manage + + if existing: + storage.write_custom_skill("demo-skill", "SKILL.md", LF_CONTENT) + storage.write_custom_skill("demo-skill", "notes.txt", "Keep this file.\n") + updates = { + "SKILL.md": LF_CONTENT.replace("Demo skill", "中文 skill").replace("\n", newline), + "scripts/helper.py": f'print("你好"){newline}', + } + seen: list[dict[str, bytes]] = [] + + def fake_scan(skill_dir, *, skill_name): + assert skill_name == "demo-skill" + seen.append({relative: (Path(skill_dir) / relative).read_bytes() for relative in updates}) + if existing: + assert (Path(skill_dir) / "notes.txt").read_bytes() == b"Keep this file.\n" + return [] + + monkeypatch.setattr(skill_manage, "enforce_static_scan", fake_scan) + with _windows_text_writes(monkeypatch): + findings = await skill_manage._scan_static_candidate_or_raise("demo-skill", updates, storage if existing else None) + for relative, content in updates.items(): + storage.write_custom_skill("demo-skill", relative, content) + + published = {relative: (storage.get_custom_skill_dir("demo-skill") / relative).read_bytes() for relative in updates} + assert findings == [] + assert published == {relative: content.encode("utf-8") for relative, content in updates.items()} + assert seen == [published]