diff --git a/tests/tools/test_skills_tool_discovery_cache.py b/tests/tools/test_skills_tool_discovery_cache.py index b262077d479..55908118cfa 100644 --- a/tests/tools/test_skills_tool_discovery_cache.py +++ b/tests/tools/test_skills_tool_discovery_cache.py @@ -40,13 +40,21 @@ def _write_skill(root, category, name, description="a skill"): return d -def test_cache_hit_serves_same_result_without_rescan(tmp_path): +def test_cache_hit_serves_copies_not_cache_objects(tmp_path): + """Callers mutate the returned dicts (web_server annotates + s['enabled']/s['usage']) — the cache must hand out per-call copies.""" _write_skill(tmp_path, "cat-a", "skill-one") first = st._find_all_skills() assert [s["name"] for s in first] == ["skill-one"] + # Mutate what the first caller got; the next (cached) call must be clean. + first[0]["enabled"] = False + first.append({"name": "junk"}) + second = st._find_all_skills() - assert second is first # cache hit returns the SAME object, no rescan + assert [s["name"] for s in second] == ["skill-one"] + assert "enabled" not in second[0], "cache poisoned by caller mutation" + assert second is not first def test_nested_category_skill_add_invalidates(tmp_path): diff --git a/tools/skills_tool.py b/tools/skills_tool.py index ae063e38e0e..a5613f62c4c 100644 --- a/tools/skills_tool.py +++ b/tools/skills_tool.py @@ -707,7 +707,10 @@ def _find_all_skills(*, skip_disabled: bool = False) -> List[Dict[str, Any]]: and cached[0] == signature and (now - cached[1]) < _SKILLS_CACHE_TTL_SECONDS ): - return cached[2] + # Per-call shallow copies: callers mutate the returned dicts + # (e.g. web_server annotates s["enabled"]/s["usage"]) — handing + # out the cached objects would poison the cache for everyone else. + return [dict(s) for s in cached[2]] skills = [] seen_names: set = set() @@ -768,9 +771,10 @@ def _find_all_skills(*, skip_disabled: bool = False) -> List[Dict[str, Any]]: # Store in cache keyed by the scan signature computed BEFORE the scan # (a write racing the scan changes the signature, so the next call - # re-scans rather than serving the torn result past the TTL). + # re-scans rather than serving the torn result past the TTL). Same + # shallow-copy contract as the hit path — the caller may mutate. _SKILLS_CACHE[cache_key] = (signature, now, skills) - return skills + return [dict(s) for s in skills] def _sort_skills(skills: List[Dict[str, Any]]) -> List[Dict[str, Any]]: