From 62130caa8285826863e4779cd58a9bb1f8298018 Mon Sep 17 00:00:00 2001 From: Jay0130-a <3421906768@qq.com> Date: Sun, 18 Oct 2026 22:03:27 +0800 Subject: [PATCH] fix(skills): strip a leading BOM from the SKILL.md content handed to the model (#6984) * fix(skills): strip a leading BOM from the SKILL.md content handed to the model * fix(agents): hash the same BOM-free skill content the middleware injects * docs(skills): state the BOM mechanism as it is, and sync the encoding convention * test(skills): name the real BOM mechanism in the activation test docstring * fix(ci): satisfy lint checks for skill BOM handling --------- Co-authored-by: Jay0130-a Co-authored-by: Jay0130-a <309071492+Jay0130-a@users.noreply.github.com> Co-authored-by: Willem Jiang --- AGENTS.md | 6 +-- .../deerflow/agents/assembly_descriptor.py | 5 ++- .../skill_activation_middleware.py | 9 ++++- .../skills/storage/local_skill_storage.py | 6 ++- .../tests/test_agent_assembly_descriptor.py | 22 +++++++++++ backend/tests/test_slash_skills.py | 37 +++++++++++++++++++ 6 files changed, 79 insertions(+), 6 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 89b75d2716..552ecc7132 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -214,9 +214,9 @@ Repo-wide conventions: with the optional import unavailable and mock HTTP transport to keep tests offline. - **Before pushing** — backend `make format`, frontend `pnpm check`. Backend CI enforces `ruff format --check`. -- **Skill text encoding** — treat `SKILL.md` and other textual skill resources as UTF-8; - Python utilities that read or write them must pass `encoding="utf-8"` rather than - relying on the platform locale. +- **Skill text encoding** — use explicit UTF-8 for textual skill resources. + Read `SKILL.md` with `encoding="utf-8-sig"` to strip a leading BOM; write with + `encoding="utf-8"`. Never rely on the platform locale. - **Version sources must stay in lockstep** — a release version must match identically in `backend/pyproject.toml`, `frontend/package.json`, and `deploy/helm/deer-flow/Chart.yaml` (`version` + `appVersion`), and `backend/uv.lock` must record the same version for the root diff --git a/backend/packages/harness/deerflow/agents/assembly_descriptor.py b/backend/packages/harness/deerflow/agents/assembly_descriptor.py index 504d8d80f5..522dbaf1d7 100644 --- a/backend/packages/harness/deerflow/agents/assembly_descriptor.py +++ b/backend/packages/harness/deerflow/agents/assembly_descriptor.py @@ -429,7 +429,10 @@ def _skill_content_hash(skill: object) -> str | None: if not isinstance(skill_file, Path): return None try: - content = skill_file.read_text(encoding="utf-8") + # Same read as ``skill_activation_middleware._read_skill_content``, whose + # content this digest is defined to describe: ``utf-8-sig`` so a BOM a + # Windows editor wrote is not part of the digest. + content = skill_file.read_text(encoding="utf-8-sig") except OSError: return None return canonical_hash(content) diff --git a/backend/packages/harness/deerflow/agents/middlewares/skill_activation_middleware.py b/backend/packages/harness/deerflow/agents/middlewares/skill_activation_middleware.py index d839a5e2a6..7c44d7b06a 100644 --- a/backend/packages/harness/deerflow/agents/middlewares/skill_activation_middleware.py +++ b/backend/packages/harness/deerflow/agents/middlewares/skill_activation_middleware.py @@ -304,7 +304,14 @@ class SkillActivationMiddleware(AgentMiddleware): raise ValueError("Resolved skill file must stay within the configured skills root.") from exc if not resolved_file.is_file(): raise FileNotFoundError(resolved_file) - return resolved_file.read_text(encoding="utf-8") + # ``utf-8-sig`` drops a leading UTF-8 BOM (Windows Notepad / PowerShell + # 5.1). The frontmatter path tolerates one already, but not by reading + # ``utf-8-sig``: ``_FRONTMATTER_RE`` matches an optional ``\ufeff`` before + # the opening fence and consumes it, so that tolerance covers the front + # matter only. This read returns the whole file, so plain ``utf-8`` would + # hand the model a body starting with U+FEFF and fingerprint that BOM in + # ``content_hash``. + return resolved_file.read_text(encoding="utf-8-sig") def _resolve_activation(self, text: str, *, activation_decisions: ActivationDecisions | None = None) -> _ActivationResolution | None: reference = parse_slash_skill_reference(text) 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 c5ab2a0c45..a0d189bcf8 100644 --- a/backend/packages/harness/deerflow/skills/storage/local_skill_storage.py +++ b/backend/packages/harness/deerflow/skills/storage/local_skill_storage.py @@ -96,7 +96,11 @@ class LocalSkillStorage(SkillStorage): with managed_read(self): if not self.custom_skill_exists(name): raise FileNotFoundError(f"Custom skill '{name}' not found.") - return (self.get_custom_skill_dir(name) / SKILL_MD_FILE).read_text(encoding="utf-8") + # Read BOM-tolerantly so a SKILL.md saved by a Windows editor does not + # return its BOM as the first character of the content this API hands + # back. The frontmatter path tolerates the BOM via ``_FRONTMATTER_RE`` + # instead, which only covers the front matter, not this whole-file read. + return (self.get_custom_skill_dir(name) / SKILL_MD_FILE).read_text(encoding="utf-8-sig") def write_custom_skill(self, name: str, relative_path: str, content: str) -> None: with self._skill_projection_mutation(names=(name,)): diff --git a/backend/tests/test_agent_assembly_descriptor.py b/backend/tests/test_agent_assembly_descriptor.py index 5e91f1e20f..17165e1ed5 100644 --- a/backend/tests/test_agent_assembly_descriptor.py +++ b/backend/tests/test_agent_assembly_descriptor.py @@ -740,6 +740,28 @@ class TestSkillCatalogHashesContent: assert before.fingerprint != after.fingerprint assert before.enabled_skills == after.enabled_skills # the catalog's visible identity is unchanged + def test_a_bom_does_not_change_the_fingerprint(self, tmp_path): + """A BOM is the editor's, not the skill's. + + ``_skill_content_hash`` digests the body ``SkillActivationMiddleware`` + injects — its docstring says so and points at ``_read_skill_content`` — + and the middleware reads that file ``utf-8-sig``. The two must therefore + agree for a SKILL.md a Windows editor saved with a BOM, or the descriptor + would fingerprint content the agent never receives. + """ + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + skill = self._skill(skill_dir) + body = "---\nname: my-skill\n---\nSame instructions.\n" + + (skill_dir / "SKILL.md").write_text(body, encoding="utf-8") + without_bom = self._build([skill]) + + (skill_dir / "SKILL.md").write_text("\ufeff" + body, encoding="utf-8") + with_bom = self._build([skill]) + + assert with_bom.fingerprint == without_bom.fingerprint + def test_required_secrets_flag_changes_the_fingerprint(self): from deerflow.skills.types import SecretRequirement diff --git a/backend/tests/test_slash_skills.py b/backend/tests/test_slash_skills.py index 6494ba542b..1323954cc2 100644 --- a/backend/tests/test_slash_skills.py +++ b/backend/tests/test_slash_skills.py @@ -556,6 +556,43 @@ def test_skill_activation_middleware_reactivates_on_new_user_slash_command(monke assert sum(1 for kwargs in recorded if kwargs.get("action") == "activate") == 2 +def test_skill_activation_strips_a_leading_bom_from_skill_content(monkeypatch, tmp_path): + """A SKILL.md saved by a Windows editor must reach the model without its BOM. + + The frontmatter path already tolerates one — ``_FRONTMATTER_RE`` + (``deerflow/skills/frontmatter.py``) consumes an optional leading BOM — so a + skill written by Notepad or PowerShell 5.1 parses and installs fine. That + tolerance covers the front matter only. Activation then reads the whole file + for the model, and reading it as plain ``utf-8`` there would hand the model a + body starting with U+FEFF and fingerprint that BOM in the ``sha256`` the + reminder publishes. This pins both halves. + """ + body = "# Data Analysis\nUse pandas." + skill = _make_skill(tmp_path, "data-analysis", content=body) + # Re-write the same content with a UTF-8 BOM, the way those editors save. + skill.skill_file.write_text("\ufeff" + body, encoding="utf-8") + + monkeypatch.setattr(middleware_module, "get_or_new_skill_storage", lambda **kwargs: _make_storage(tmp_path, [skill])) + + middleware = SkillActivationMiddleware(slash_source_owner_token=_SLASH_SOURCE_OWNER_TOKEN) + original = HumanMessage(content="/data-analysis analyze uploads/foo.csv", id="msg-1") + captured = {} + + def handler(model_request: ModelRequest): + captured["messages"] = model_request.messages + return AIMessage(content="ok") + + middleware.wrap_model_call(_make_model_request([original], runtime=SimpleNamespace(context=None)), handler) + + reminders = [message for message in captured["messages"] if is_slash_skill_activation_reminder(message)] + assert len(reminders) == 1 + content = reminders[0].content + assert "\ufeff" not in content + # The published digest fingerprints the skill, not the editor's byte-order mark: + # it equals the digest of the same file written without one. + assert f'sha256="{hashlib.sha256(body.encode("utf-8")).hexdigest()}"' in content + + def test_skill_activation_middleware_activates_per_call_when_run_context_is_none(monkeypatch, tmp_path): # Regression for the degraded-path contract: when runtime.context is None # (e.g. no run-scoped context is threaded through - the #3989 case), _run_context()