mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 13:16:24 +00:00
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 <willem.jiang@gmail.com>
This commit is contained in:
parent
ddfa45eaaf
commit
d53ec767af
@ -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.
|
||||
|
||||
@ -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:
|
||||
|
||||
@ -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:
|
||||
|
||||
@ -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)
|
||||
|
||||
@ -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())
|
||||
|
||||
@ -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")
|
||||
|
||||
|
||||
@ -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)
|
||||
|
||||
@ -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)
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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()
|
||||
|
||||
@ -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:
|
||||
|
||||
259
backend/tests/test_text_writes_keep_lf_bytes.py
Normal file
259
backend/tests/test_text_writes_keep_lf_bytes.py
Normal file
@ -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]
|
||||
Loading…
x
Reference in New Issue
Block a user