From 5c5cf24ff24a5e02becee7dca344ca2a4d6e4d42 Mon Sep 17 00:00:00 2001 From: Hyacinth-of-Security <144213008+lxcxjxhx@users.noreply.github.com> Date: Mon, 9 Nov 2026 21:49:46 +0800 Subject: [PATCH] fix(skills): read undecodable skill state as missing (#7963) * fix(skills): read undecodable skill state as missing `_read_manifest` and `_read_skill_states` document that an unreadable file means "no manifest" / "no overrides", and both catch OSError and JSONDecodeError. A file that is not valid UTF-8 raises UnicodeDecodeError, which is a ValueError and matches neither, so the projection and the skill state read raise instead of falling back. Catch it alongside the existing exceptions. * fix(skills): keep the reason when profile state cannot be decoded Review feedback on #7963: the state reader dropped the exception, so a decode failure that silently discards the user's disabled set left nothing to diagnose, and the next toggle overwrites those bytes. Log the exception, and log a debug line when an unreadable projection manifest forces a rebuild. * docs(skills): state that undecodable skill state fails open _read_skill_states already degraded a malformed file to {} before this change; making undecodable bytes take the same path made that explicit, so say so next to the sandbox check that documents the opposite verdict for a single named skill. --------- Co-authored-by: lxcxjxhx --- .../harness/deerflow/skills/projection.py | 5 ++++- .../storage/user_scoped_skill_storage.py | 17 ++++++++++++++--- backend/tests/test_skill_projection.py | 18 ++++++++++++++++++ 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/backend/packages/harness/deerflow/skills/projection.py b/backend/packages/harness/deerflow/skills/projection.py index c7aafaa82a..8142b8b7b1 100644 --- a/backend/packages/harness/deerflow/skills/projection.py +++ b/backend/packages/harness/deerflow/skills/projection.py @@ -422,7 +422,10 @@ def _manifest_path(scope_root: Path) -> Path: def _read_manifest(scope_root: Path) -> dict | None: try: value = json.loads(_manifest_path(scope_root).read_text(encoding="utf-8")) - except (OSError, json.JSONDecodeError): + except (OSError, UnicodeDecodeError, json.JSONDecodeError) as exc: + # Discarding the manifest forces a full rebuild on the next ensure; log + # once so "why did my projection rebuild" has an answer. + logger.debug("Ignoring unreadable projection manifest %s: %s", _manifest_path(scope_root), exc) return None return value if isinstance(value, dict) else None 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 a7c6c23a5a..fa2ce56c35 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 @@ -101,7 +101,16 @@ class UserScopedSkillStorage(LocalSkillStorage): Returns a dict keyed by skill name, each value being ``{"enabled": True/False}``. Returns an empty dict if the file - does not exist or is unreadable. + does not exist or cannot be decoded. + + Undecodable bytes degrade to "no overrides", i.e. FAIL OPEN: every + skill the user had disabled reads as enabled again. That matches what + malformed JSON already did, and it is deliberate - the alternative, + denying every custom skill, locks a user out of their own tree over one + stray byte. ``sandbox/tools.py::_is_disabled_skill_path`` says corrupt + state must fail CLOSED on purpose: it decides access for one already + named skill, where denying is the safe verdict. The two are not in + conflict; do not "align" them without deciding which side loses data. """ if not self._skill_states_file.exists(): return {} @@ -110,8 +119,10 @@ class UserScopedSkillStorage(LocalSkillStorage): data = json.load(f) if isinstance(data, dict): return data - except (json.JSONDecodeError, OSError): - logger.warning("Failed to read skill states file %s", self._skill_states_file) + except (json.JSONDecodeError, OSError, UnicodeDecodeError) as exc: + # A decode failure means the user's disabled set is discarded, and the + # next toggle rewrites this file: keep the reason in the log. + logger.warning("Failed to read skill states file %s: %s", self._skill_states_file, exc) return {} def _write_skill_states(self, states: dict[str, dict[str, bool]]) -> None: diff --git a/backend/tests/test_skill_projection.py b/backend/tests/test_skill_projection.py index 4f98eec63e..a3d382480f 100644 --- a/backend/tests/test_skill_projection.py +++ b/backend/tests/test_skill_projection.py @@ -1080,3 +1080,21 @@ def test_thread_projection_upgrades_metadata_only_manifest(projection_env) -> No ensure_thread_skill_projection(env.storage, "thread-old-manifest", {"alpha"}) assert target.read_bytes() == source.read_bytes() + + +def test_undecodable_manifest_is_treated_as_missing(tmp_path: Path) -> None: + """A manifest we cannot decode must not stop the projection from being rebuilt.""" + from deerflow.skills.projection import _read_manifest + + (tmp_path / ".projection-manifest.json").write_bytes(b"\xff\xfe") + + assert _read_manifest(tmp_path) is None + + +def test_undecodable_skill_states_file_is_treated_as_empty(projection_env) -> None: + """An undecodable states file reads as 'no overrides', not as a crash.""" + state_file = projection_env.paths.user_skills_dir("alice") / "_skill_states.json" + state_file.parent.mkdir(parents=True, exist_ok=True) + state_file.write_bytes(b"\xff\xfe") + + assert projection_env.storage._read_skill_states() == {}