diff --git a/EvoScientist/EvoScientist.py b/EvoScientist/EvoScientist.py index 6aab0d6..a48ab35 100644 --- a/EvoScientist/EvoScientist.py +++ b/EvoScientist/EvoScientist.py @@ -407,6 +407,39 @@ def _ensure_general_purpose_subagent(subs: list[dict]) -> None: ) +def _fold_expert_subagents(subs: list[dict], tool_registry: dict) -> None: + """Append expert-skill sub-agent specs to ``subs``, guarding names. + + Each installed expert skill becomes an in-process sub-agent entry so + the main agent's ``task`` tool (and the QuickJS ``task()`` global for + panel mode) can dispatch to it by name. Async-graph deploy of experts + is v2 territory — they live purely in the sync in-process registry. + + Skips (with a warning) any expert whose ``name`` collides with a + subagent already in ``subs`` or with ``general-purpose``. The reserved + name matters because ``_ensure_general_purpose_subagent`` runs right + after this and early-returns when it sees the slot occupied — an expert + named ``general-purpose`` would silently take the slot and deepagents' + default subagent prompt would never reach the agent. + """ + from deepagents.middleware.subagents import GENERAL_PURPOSE_SUBAGENT + + from .subagents.expert_container import build_expert_subagent_specs + + logger = logging.getLogger(__name__) + taken = {s.get("name") for s in subs} | {GENERAL_PURPOSE_SUBAGENT["name"]} + for spec in build_expert_subagent_specs(tool_registry=tool_registry): + name = spec["name"] + if name in taken: + logger.warning( + "Expert skill %r collides with an existing sub-agent name; skipping.", + name, + ) + continue + taken.add(name) + subs.append(spec) + + def _maybe_swap_async_subagents( subs: list, middleware: list | None = None, *, cfg=None ) -> list: @@ -524,7 +557,12 @@ def _build_base_kwargs( from .utils import load_subagents cfg = cfg if cfg is not None else _ensure_config() - tool_registry = {"think_tool": think_tool} + # `skill_manager` is registered here in addition to `base_tools` because + # expert subagents resolve their default toolset from `tool_registry` (see + # `_DEFAULT_EXPERT_TOOLS` in expert_container.py). Without this entry the + # tool silently misses from every expert sub-agent — e.g. idea-brainstorm + # can't run its `paper-navigator` precondition check. + tool_registry = {"think_tool": think_tool, "skill_manager": skill_manager} if os.environ.get("TAVILY_API_KEY"): tool_registry["tavily_search"] = tavily_search base_tools = [think_tool, skill_manager] @@ -538,6 +576,7 @@ def _build_base_kwargs( tool_registry=tool_registry, async_swap_pending=True, ) + _fold_expert_subagents(subs, tool_registry) _ensure_general_purpose_subagent(subs) _inject_subagent_middleware( subs, workspace_dir=workspace_dir, cfg=cfg, chat_model=chat_model @@ -595,7 +634,10 @@ def load_mcp_and_build_kwargs( workspace_dir=workspace_dir, ) - tool_registry = {"think_tool": think_tool} + # Match `_build_base_kwargs`: register `skill_manager` in the registry so + # expert subagents (which resolve tools via `_DEFAULT_EXPERT_TOOLS` from + # `expert_container.py`) actually get it. + tool_registry = {"think_tool": think_tool, "skill_manager": skill_manager} if os.environ.get("TAVILY_API_KEY"): tool_registry["tavily_search"] = tavily_search base_tools = [think_tool, skill_manager] @@ -615,6 +657,7 @@ def load_mcp_and_build_kwargs( tool_registry=registry, async_swap_pending=True, ) + _fold_expert_subagents(subs, registry) _ensure_general_purpose_subagent(subs) _inject_subagent_middleware( diff --git a/EvoScientist/langgraph_dev/http.py b/EvoScientist/langgraph_dev/http.py index 0d9c6f6..3b2375d 100644 --- a/EvoScientist/langgraph_dev/http.py +++ b/EvoScientist/langgraph_dev/http.py @@ -75,8 +75,58 @@ async def get_models(_request: Request) -> JSONResponse: ) +async def get_teams(_request: Request) -> JSONResponse: + """Return installed expert skills as ``{teams: [...]}`` for the WebUI gallery. + + A "team" in the WebUI vocabulary is an installed expert skill — + ``SKILL.md`` with ``type: expert`` frontmatter. The response is a + curated, gallery-safe projection: name + description, plus optional + ``byline`` / ``capability_tags`` / ``avatar_hint`` when the skill + populates them. + + Backend implementation details (SKILL.md body / system prompt, role + line, default_dispatch, tool list, source tier, filesystem path, + tags) are intentionally NOT projected. The gallery only needs + identity + descriptor fields to render the card; anything richer + belongs in a dedicated info endpoint. + + Sourced from ``list_expert_skills(include_system=True)`` so + first-party experts shipped as builtin skills surface alongside + workspace/global installs. + + Offloaded to a thread because the skill loader does synchronous + filesystem walking + yaml parsing, which langgraph-dev's + ``blockbuster`` middleware refuses on the async event loop. + + Response shape (each entry): ``{name, description, byline?, + capability_tags?, avatar_hint?}`` — the WebUI gallery consumes these. + """ + from EvoScientist.tools.skills_manager import list_expert_skills + + experts = await asyncio.to_thread(list_expert_skills, True) + teams = [] + for info in experts: + entry = { + "name": info.name, + "description": info.description, + } + # Optional gallery fields — omit when unpopulated so the WebUI + # card degrades gracefully (SkillInfo defaults `byline` / + # `avatar_hint` to "" and `capability_tags` to [], which we + # treat as "not declared"). + if info.byline: + entry["byline"] = info.byline + if info.capability_tags: + entry["capability_tags"] = list(info.capability_tags) + if info.avatar_hint: + entry["avatar_hint"] = info.avatar_hint + teams.append(entry) + return JSONResponse({"teams": teams}) + + app = Starlette( routes=[ Route("/api/models", get_models, methods=["GET"]), + Route("/api/teams", get_teams, methods=["GET"]), ] ) diff --git a/EvoScientist/subagents/expert_container.py b/EvoScientist/subagents/expert_container.py new file mode 100644 index 0000000..e08bc2b --- /dev/null +++ b/EvoScientist/subagents/expert_container.py @@ -0,0 +1,156 @@ +"""Expert-subagent-spec factory for the v1 agent-teams feature. + +Turns an installed **expert skill** (a `SkillInfo` with `type == "expert"`) into +a deepagents subagent spec dict compatible with `subagents=[...]` on +`create_deep_agent`. The main agent's `_build_base_kwargs` folds these specs +into its subagent list at construction time so the `task` tool can dispatch to +each installed expert for sync consult; the same registry is reused by the +QuickJS `task()` global for panel mode. + +The generic-container principle from #361 lives in THIS FUNCTION — one +construction path for all experts, sourcing behaviour from the skill file +rather than a per-expert YAML. There's no deployed graph per expert in v1; +that's async-thread territory (v2) and blocked on the deepagents +`AsyncSubAgent` config-passthrough gap. +""" + +from __future__ import annotations + +import logging +from typing import Any + +from ..tools.skills_manager import SkillInfo, _split_frontmatter_and_body + +_logger = logging.getLogger(__name__) + +# Default toolset for expert subagents. Kept minimal — most experts are +# "reason about the incoming description and produce structured output"; +# they can reach installed utility skills via the `/skills/` mount. +# +# `skill_manager` is included so experts can inspect what utility skills are +# available at runtime (e.g. `idea-brainstorm` checks for `paper-navigator` +# before starting its literature-review phase). Widening beyond these two +# defaults should be a deliberate decision (e.g. adding `execute` only when +# we know experts need to run scripts — deepagents' built-in file/execute +# tools are already available regardless of this list). +_DEFAULT_EXPERT_TOOLS: tuple[str, ...] = ("think_tool", "skill_manager") + +# Default skills mount — expert subagents get the same read-only skills view +# as any other subagent (matches `research.yaml` / `writing.yaml` shape). +_DEFAULT_EXPERT_SKILLS: tuple[str, ...] = ("/skills/",) + + +def _body_of(skill_info: SkillInfo) -> str: + """Return the SKILL.md body (post-frontmatter content). + + Prefers the body cached on ``SkillInfo`` by ``_parse_skill_md``. Falls + back to reading SKILL.md fresh if the cached body is empty — that + handles skills constructed by hand (external callers) without a body + field populated. Returns an empty string if the file can't be read. + """ + if skill_info.body: + return skill_info.body + skill_md = skill_info.path / "SKILL.md" + try: + content = skill_md.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError) as exc: + _logger.warning( + "Expert skill %r: could not read SKILL.md at %s (%s)", + skill_info.name, + skill_md, + exc, + ) + return "" + _, body = _split_frontmatter_and_body(content) + return body + + +def _compose_system_prompt(skill_info: SkillInfo, body: str) -> str: + """Compose the expert's system_prompt from its role + SKILL.md body. + + The `role` frontmatter (one-line role summary) is prepended as an + orientation line; the body carries the persona voice, rubrics, and + output-style instructions (all written in second person addressing the + expert itself, per the expert-skill authoring convention). + """ + if skill_info.role: + return f"You are {skill_info.role}.\n\n{body}".rstrip() + "\n" + return body if body.endswith("\n") else body + "\n" + + +def build_expert_subagent_spec( + skill_info: SkillInfo, + tool_registry: dict[str, Any] | None = None, +) -> dict[str, Any]: + """Build a deepagents subagent spec dict from an expert skill. + + Args: + skill_info: An expert skill (``type == "expert"``). The caller is + responsible for filtering — passing a utility skill here builds a + spec anyway (utility skills just don't have persona content in + the body, so the result is nonsensical rather than broken). + tool_registry: Same registry `load_subagents` uses to resolve tool + names to callables (e.g. `{"think_tool": think_tool, ...}`). + Unresolved tools are skipped with a warning, matching + `_build_one` in `EvoScientist/utils.py`. + + Returns: + A subagent spec dict with the same shape ``load_subagents`` produces: + ``{name, description, system_prompt, tools, skills, _async}``. Ready + to append to the main agent's `subagents=[...]` list. + """ + tool_registry = tool_registry or {} + body = _body_of(skill_info) + system_prompt = _compose_system_prompt(skill_info, body) + + resolved_tools: list[Any] = [] + for tool_name in _DEFAULT_EXPERT_TOOLS: + if tool_name in tool_registry: + resolved_tools.append(tool_registry[tool_name]) + else: + _logger.warning( + "Expert skill %r: default tool %r not in registry, skipping", + skill_info.name, + tool_name, + ) + + return { + "name": skill_info.name, + "description": skill_info.description, + "system_prompt": system_prompt, + "tools": resolved_tools, + "skills": list(_DEFAULT_EXPERT_SKILLS), + # v1 is sync-consult + panel only; both use the in-process subagent + # registry, not the async graph path. Async-thread mode = v2. + "_async": False, + } + + +def build_expert_subagent_specs( + tool_registry: dict[str, Any] | None = None, + *, + include_system: bool = True, +) -> list[dict[str, Any]]: + """Build spec dicts for every installed expert skill. + + Thin wrapper over ``list_expert_skills()`` + ``build_expert_subagent_spec``. + Called by the main-agent construction path (``_build_base_kwargs``) to + fold experts into the ``subagents=[...]`` list. + + Skips (with a warning) any expert whose SKILL.md body is empty — a + personaless expert advertised in the ``task`` tool schema would let the + orchestrator dispatch to a blank system prompt, a worse failure mode + than the expert being absent. + """ + from ..tools.skills_manager import list_expert_skills + + specs: list[dict[str, Any]] = [] + for info in list_expert_skills(include_system=include_system): + if not _body_of(info).strip(): + _logger.warning( + "Expert skill %r: SKILL.md body is empty; skipping registration.", + info.name, + ) + continue + specs.append(build_expert_subagent_spec(info, tool_registry=tool_registry)) + return specs diff --git a/EvoScientist/tools/skill_manager.py b/EvoScientist/tools/skill_manager.py index ee9e612..37c58cd 100644 --- a/EvoScientist/tools/skill_manager.py +++ b/EvoScientist/tools/skill_manager.py @@ -13,6 +13,7 @@ def skill_manager( name: str = "", tag: str = "", include_system: bool = False, + skill_type: Literal["all", "expert", "utility"] = "all", ) -> str: """Manage user-installable skills: install from GitHub or local path, list available skills, browse remote skills, get details, or uninstall. @@ -28,6 +29,8 @@ def skill_manager( action="list": List installed skills. By default only shows user-installed skills. Set include_system=True to also show built-in system skills. + Set skill_type="expert" to show only expert skills (agent teams); + set skill_type="utility" to exclude them; leave as "all" (default) for no filter. Built-in skills evolve over time, so use action="list" to see the current set. action="browse" (optional tag): @@ -37,7 +40,8 @@ def skill_manager( action="info" (requires name): Get details (description, source, path, tags) about a specific skill by name. - Searches both user and system skills. + Expert skills also surface their role, byline, capability tags, and default + dispatch mode. Searches both user and system skills. action="uninstall" (requires name): Remove a user-installed skill by name. System skills cannot be uninstalled. @@ -48,6 +52,7 @@ def skill_manager( name: Required for info and uninstall — the skill name (for example, one returned by action="list") tag: Optional for browse — filter by tag (e.g. "core", "writing", "experiments", "research") include_system: Only for list — set True to include built-in system skills in the output + skill_type: Only for list — one of "all" (default; no filter), "expert" (agent-team skills), or "utility" Returns: Result message @@ -112,7 +117,16 @@ def skill_manager( elif action == "list": skills = list_skills(include_system=include_system) + # `skill_type="all"` (default) is the no-filter case. A specific + # value ("expert" or "utility") narrows the list to that type. + # Empty string is NOT a legal input — Gemini's function-declaration + # schema rejects empty enum values (was the root cause of a live + # smoke failure); the `Literal` above defends against it. + if skill_type in ("expert", "utility"): + skills = [s for s in skills if s.type == skill_type] if not skills: + if skill_type in ("expert", "utility"): + return f"No {skill_type} skills found." if include_system: return "No skills found." return "No user skills installed. Use action='install' to add skills, or set include_system=True to see built-in skills." @@ -180,17 +194,33 @@ def skill_manager( f"Skill not found: {name}. " f"Use action='list' with include_system=True to see all available skills." ) - tags_str = f"\nTags: {', '.join(info.tags)}" if info.tags else "" + lines = [ + f"Name: {info.name}", + f"Description: {info.description}", + f"Source: {info.source}", + ] # ``Path`` reports the sandbox-visible virtual mount segment (the skill's # directory name under ``/skills/``), not the host filesystem path. # Surfacing the host path (e.g. ``/home/.../EvoScientist/skills/``) # invited ``cd && …`` chains that fail in the sandbox. - return ( - f"Name: {info.name}\n" - f"Description: {info.description}\n" - f"Source: {info.source}\n" - f"Path: /skills/{info.path.name}{tags_str}" - ) + lines.append(f"Path: /skills/{info.path.name}") + if info.tags: + lines.append(f"Tags: {', '.join(info.tags)}") + # Expert-skill surface (agent-teams v1): only shown when the + # skill declared `type: expert` in its SKILL.md frontmatter. + if info.type == "expert": + lines.append("Type: expert") + if info.role: + lines.append(f"Role: {info.role}") + if info.byline: + lines.append(f"Byline: {info.byline}") + if info.capability_tags: + lines.append(f"Capability tags: {', '.join(info.capability_tags)}") + if info.avatar_hint: + lines.append(f"Avatar hint: {info.avatar_hint}") + if info.default_dispatch: + lines.append(f"Default dispatch: {info.default_dispatch}") + return "\n".join(lines) else: return f"Unknown action: {action}. Use 'install', 'list', 'browse', 'uninstall', or 'info'." diff --git a/EvoScientist/tools/skills_manager.py b/EvoScientist/tools/skills_manager.py index dea21d7..a994f40 100644 --- a/EvoScientist/tools/skills_manager.py +++ b/EvoScientist/tools/skills_manager.py @@ -57,6 +57,47 @@ class SkillInfo: path: Path source: str # "workspace", "global", or "builtin" tags: list[str] = field(default_factory=list) + # Expert-skill fields for the v1 agent-teams feature. All default + # to utility-skill values so existing skills stay unchanged and their + # SKILL.md files need no edits. + type: str = "utility" # "utility" (default) or "expert" + role: str = "" # one-line role summary shown to the orchestrator LLM + byline: str = "" # WebUI gallery byline + capability_tags: list[str] = field(default_factory=list) # WebUI chips + avatar_hint: str = "" # WebUI icon hint + default_dispatch: str = "" # "sync" | "panel" (expert skills only) + # SKILL.md body (post-frontmatter). Populated by ``_parse_skill_md`` so + # the expert-container factory doesn't have to re-read the file on every + # main-agent construction. Empty for skills built by hand or when the + # source SKILL.md has no body content. + body: str = "" + + +# Shared frontmatter split regex. Captures three parts: +# 1. leading fence (``^---\s*\n``) +# 2. frontmatter YAML (``.*?``) +# 3. trailing fence + optional newline (``\n---\s*\n?``) +# Used by both ``_parse_skill_md`` (needs the YAML) and the expert-container +# factory (needs the body); a single source of truth for "what is the +# frontmatter block" so schema extensions don't drift across call sites. +_FRONTMATTER_SPLIT_RE = re.compile(r"^---\s*\n(.*?)\n---\s*\n?", re.DOTALL) + + +def _split_frontmatter_and_body(content: str) -> tuple[str, str]: + """Return ``(frontmatter_yaml, body)`` for a SKILL.md text. + + ``frontmatter_yaml`` is the raw YAML block between the ``---`` fences + (still a string, not parsed). ``body`` is the post-fence content with + leading whitespace stripped. When there is no valid frontmatter block, + ``frontmatter_yaml`` is ``""`` and the whole content becomes the body — + matches the legacy expert-container behaviour of passing plain-body + files through unchanged. + """ + match = _FRONTMATTER_SPLIT_RE.match(content) + if not match: + return "", content.lstrip() + body = content[match.end() :].lstrip() + return match.group(1), body def _normalize_tags(raw: object) -> list[str]: @@ -206,7 +247,9 @@ def installed_provenance() -> dict[str, dict[str, str | None]]: def _parse_skill_md(skill_md_path: Path, *, source: str = "") -> SkillInfo: - """Parse SKILL.md frontmatter to extract name, description, and tags. + """Parse SKILL.md frontmatter to extract name, description, tags, and + (for expert skills) role / byline / capability_tags / avatar_hint / + default_dispatch. SKILL.md format: --- @@ -215,6 +258,14 @@ def _parse_skill_md(skill_md_path: Path, *, source: str = "") -> SkillInfo: tags: [tag1, tag2] metadata: tags: [tag1, tag2] # fallback location + # Expert-skill fields (all optional; only meaningful when + # ``type: expert`` — see agent-teams-design.md): + type: expert + role: One-line role summary + byline: Short persona name for gallery + capability_tags: [Tag One, Tag Two] + avatar_hint: lightbulb + default_dispatch: sync --- # Skill Title ... @@ -227,40 +278,115 @@ def _parse_skill_md(skill_md_path: Path, *, source: str = "") -> SkillInfo: SkillInfo with path set to the skill's parent directory. """ parent = skill_md_path.parent - content = skill_md_path.read_text(encoding="utf-8") + try: + content = skill_md_path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError) as exc: + # ``_parse_skill_md`` runs during agent construction (via + # ``_fold_expert_subagents`` → ``list_skills``), so an unreadable + # SKILL.md in any tier must not abort the whole main-agent build or + # ``GET /api/teams``. Degrade to an "(unreadable)" placeholder so + # the entry stays visible in ``skill_manager list`` for debugging + # but never advances into expert registration (empty body → + # ``build_expert_subagent_specs`` skips it). + _logger.warning( + "Skill %r: could not read %s (%s); skipping.", + parent.name, + skill_md_path, + exc, + ) + return SkillInfo( + name=parent.name, + description="(unreadable)", + path=parent, + source=source, + ) - def _info(name: str, description: str, tags: list[str] | None = None) -> SkillInfo: + def _info( + name: str, + description: str, + tags: list[str] | None = None, + *, + type_: str = "utility", + role: str = "", + byline: str = "", + capability_tags: list[str] | None = None, + avatar_hint: str = "", + default_dispatch: str = "", + body: str = "", + ) -> SkillInfo: return SkillInfo( name=name, description=description, path=parent, source=source, tags=tags or [], + type=type_, + role=role, + byline=byline, + capability_tags=capability_tags or [], + avatar_hint=avatar_hint, + default_dispatch=default_dispatch, + body=body, ) - # Extract YAML frontmatter - frontmatter_match = re.match(r"^---\s*\n(.*?)\n---", content, re.DOTALL) - if not frontmatter_match: - # No frontmatter, use directory name - return _info(parent.name, "(no description)") + frontmatter_yaml, body = _split_frontmatter_and_body(content) + if not frontmatter_yaml: + # No frontmatter, use directory name; still cache the body so + # the expert-container factory doesn't have to re-read. + return _info(parent.name, "(no description)", body=body) try: - frontmatter = yaml.safe_load(frontmatter_match.group(1)) + frontmatter = yaml.safe_load(frontmatter_yaml) if not isinstance(frontmatter, dict): - return _info(parent.name, "(empty frontmatter)") + return _info(parent.name, "(empty frontmatter)", body=body) # Tags: check top-level first, fall back to metadata.tags tags = _normalize_tags(frontmatter.get("tags")) if not tags: metadata = frontmatter.get("metadata") if isinstance(metadata, dict): tags = _normalize_tags(metadata.get("tags")) + # Expert-skill fields. Only trusted when explicit; unknown + # ``type`` values fall back to "utility" so a typo doesn't + # accidentally register a skill as an expert. Log the fallback + # so authors can debug a "why isn't my expert appearing" case. + raw_type = frontmatter.get("type") + type_ = raw_type if raw_type in ("expert", "utility") else "utility" + if raw_type is not None and raw_type != type_: + _logger.warning( + "Skill %r: unrecognized type %r in frontmatter; treating as utility.", + frontmatter.get("name") or parent.name, + raw_type, + ) + role = frontmatter.get("role") or "" + byline = frontmatter.get("byline") or "" + capability_tags = _normalize_tags(frontmatter.get("capability_tags")) + avatar_hint = frontmatter.get("avatar_hint") or "" + raw_dispatch = frontmatter.get("default_dispatch") + default_dispatch = raw_dispatch if raw_dispatch in ("sync", "panel") else "" + # ``.get("name", parent.name)`` only defaults on missing key; a + # present-but-empty ``name:`` yields None, which would flow into + # ``SkillInfo.name`` and slip past the ``_fold_expert_subagents`` + # collision guard. Coerce to the parent dir name in both cases. return _info( - frontmatter.get("name", parent.name), + frontmatter.get("name") or parent.name, frontmatter.get("description", "(no description)"), tags, + type_=type_, + role=str(role), + byline=str(byline), + capability_tags=capability_tags, + avatar_hint=str(avatar_hint), + default_dispatch=default_dispatch, + body=body, ) - except yaml.YAMLError: - return _info(parent.name, "(invalid frontmatter)") + except yaml.YAMLError as exc: + _logger.warning( + "Skill %r: invalid frontmatter YAML in %s (%s); using placeholder.", + parent.name, + skill_md_path, + exc, + ) + return _info(parent.name, "(invalid frontmatter)", body=body) def _parse_github_url(url: str) -> tuple[str, str | None, str | None]: @@ -763,6 +889,28 @@ def list_skills(include_system: bool = False) -> list[SkillInfo]: return skills +def list_expert_skills(include_system: bool = True) -> list[SkillInfo]: + """List installed expert skills (agent-teams v1). + + Filters ``list_skills()`` output to entries whose SKILL.md frontmatter + declares ``type: expert``. Defaults ``include_system=True`` because + first-party experts (`idea-brainstorm`, etc.) ship under the builtin + skills tier; user-installed experts still surface from the workspace + and global tiers alongside them. + + Consumed by: + - ``GET /api/teams`` (WebUI gallery listing). + - Main-agent construction (folds expert skills into the + ``subagents=[...]`` list so the ``task`` tool can dispatch to + them for sync consult). + - The ``skill_manager`` @tool's ``type=expert`` filter. + + Returns: + List of ``SkillInfo`` for each expert skill. + """ + return [s for s in list_skills(include_system=include_system) if s.type == "expert"] + + def uninstall_skill(name: str) -> dict: """Uninstall a skill from workspace or global tier. diff --git a/tests/test_expert_container.py b/tests/test_expert_container.py new file mode 100644 index 0000000..107976f --- /dev/null +++ b/tests/test_expert_container.py @@ -0,0 +1,545 @@ +"""Tests for EvoScientist.subagents.expert_container factory.""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import patch + +from EvoScientist.subagents.expert_container import ( + _body_of, + _compose_system_prompt, + build_expert_subagent_spec, + build_expert_subagent_specs, +) +from EvoScientist.tools.skills_manager import SkillInfo + +# ============================================================================= +# Fixtures +# ============================================================================= + + +def _write_expert_skill_file( + parent: Path, + name: str, + *, + body: str = "You are a test expert.\n\nDo the thing.\n", + role: str = "test expert", + description: str = "A test expert skill", +) -> Path: + """Write a minimal expert SKILL.md file and return the parent directory.""" + skill_dir = parent / name + skill_dir.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_text( + f"""--- +name: {name} +description: {description} +type: expert +role: {role} +--- +{body}""" + ) + return skill_dir + + +def _skill_info( + path: Path, + *, + name: str = "expert-a", + description: str = "A test expert skill", + role: str = "test expert", +) -> SkillInfo: + return SkillInfo( + name=name, + description=description, + path=path, + source="workspace", + type="expert", + role=role, + ) + + +class _FakeTool: + """Stand-in for a resolved tool callable — the factory only cares that + the value is present in the registry, not what it is.""" + + def __init__(self, name: str) -> None: + self.name = name + + +# ============================================================================= +# _body_of +# ============================================================================= + + +class TestBodyOf: + def test_extracts_body_after_frontmatter(self, tmp_path): + skill_dir = _write_expert_skill_file(tmp_path, "expert-a") + info = _skill_info(skill_dir) + body = _body_of(info) + assert body.startswith("You are a test expert.") + assert "Do the thing." in body + assert "---" not in body + assert "type: expert" not in body + + def test_returns_empty_on_missing_file(self, tmp_path, caplog): + # A SkillInfo pointing at a nonexistent SKILL.md — factory should + # gracefully degrade with a warning rather than raise. + info = _skill_info(tmp_path / "nonexistent") + body = _body_of(info) + assert body == "" + # Warning surfaced — SEV so the malformed skill isn't invisible. + assert any("could not read SKILL.md" in r.message for r in caplog.records) + + def test_returns_empty_on_non_utf8_file(self, tmp_path, caplog): + # A SKILL.md whose bytes aren't valid UTF-8. `read_text` raises + # UnicodeDecodeError (not OSError); the factory must degrade to an + # empty body rather than aborting agent construction. + skill_dir = tmp_path / "bad-utf8" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_bytes(b"\xff\xfe garbage") + info = _skill_info(skill_dir, name="bad-utf8") + body = _body_of(info) + assert body == "" + assert any("could not read SKILL.md" in r.message for r in caplog.records) + + def test_handles_no_frontmatter(self, tmp_path): + """A SKILL.md with no frontmatter — body is the whole file.""" + skill_dir = tmp_path / "no-fm" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text("# Body Only\n\nContent here.\n") + info = _skill_info(skill_dir, name="no-fm") + body = _body_of(info) + assert "# Body Only" in body + assert "Content here." in body + + def test_prefers_cached_body_over_disk_read(self, tmp_path): + """When ``SkillInfo.body`` is populated (the ``_parse_skill_md`` path), + ``_body_of`` uses it directly without touching disk. Guards against + the double-read regression flagged by pre-PR review.""" + info = SkillInfo( + name="cached", + description="d", + path=tmp_path / "does-not-exist", + source="workspace", + type="expert", + body="Cached body content from SkillInfo.", + ) + body = _body_of(info) + assert body == "Cached body content from SkillInfo." + + +# ============================================================================= +# _compose_system_prompt +# ============================================================================= + + +class TestComposeSystemPrompt: + def test_prepends_role_line_when_present(self): + info = SkillInfo( + name="expert-a", + description="d", + path=Path("/tmp"), + source="workspace", + type="expert", + role="research idea brainstormer", + ) + prompt = _compose_system_prompt(info, "Follow these rules.\n") + assert prompt.startswith("You are research idea brainstormer.\n") + assert "Follow these rules." in prompt + + def test_omits_role_line_when_absent(self): + info = SkillInfo( + name="expert-a", + description="d", + path=Path("/tmp"), + source="workspace", + type="expert", + role="", + ) + prompt = _compose_system_prompt(info, "Do the thing.\n") + assert not prompt.startswith("You are") + assert prompt.rstrip() == "Do the thing." + + +# ============================================================================= +# build_expert_subagent_spec +# ============================================================================= + + +class TestBuildExpertSubagentSpec: + def test_produces_expected_shape(self, tmp_path): + skill_dir = _write_expert_skill_file( + tmp_path, + "expert-a", + body="Second-person persona instructions.\n", + role="research idea brainstormer", + description="Brainstorms research ideas", + ) + info = _skill_info( + skill_dir, + name="expert-a", + description="Brainstorms research ideas", + role="research idea brainstormer", + ) + registry = { + "think_tool": _FakeTool("think_tool"), + "skill_manager": _FakeTool("skill_manager"), + } + spec = build_expert_subagent_spec(info, tool_registry=registry) + + # Same field set as `load_subagents._build_one` returns for a YAML subagent. + assert set(spec.keys()) == { + "name", + "description", + "system_prompt", + "tools", + "skills", + "_async", + } + assert spec["name"] == "expert-a" + assert spec["description"] == "Brainstorms research ideas" + assert spec["_async"] is False + assert spec["skills"] == ["/skills/"] + assert spec["tools"] == [ + registry["think_tool"], + registry["skill_manager"], + ] + # Role prepended, body preserved. + assert spec["system_prompt"].startswith("You are research idea brainstormer.\n") + assert "Second-person persona instructions." in spec["system_prompt"] + + def test_missing_tool_in_registry_is_skipped_not_raised(self, tmp_path, caplog): + skill_dir = _write_expert_skill_file(tmp_path, "expert-a") + info = _skill_info(skill_dir) + # Registry has no `think_tool`. Factory logs a warning and returns + # an empty tools list rather than raising. + spec = build_expert_subagent_spec(info, tool_registry={}) + assert spec["tools"] == [] + assert any( + "default tool 'think_tool' not in registry" in r.message + for r in caplog.records + ) + + def test_tool_registry_optional(self, tmp_path): + """Passing no registry is legal (used by tests / adhoc introspection).""" + skill_dir = _write_expert_skill_file(tmp_path, "expert-a") + info = _skill_info(skill_dir) + spec = build_expert_subagent_spec(info) + # No registry → tools empty; other fields still populated. + assert spec["tools"] == [] + assert spec["name"] == "expert-a" + assert spec["system_prompt"] + + +# ============================================================================= +# build_expert_subagent_specs (bulk over list_expert_skills) +# ============================================================================= + + +class TestBuildExpertSubagentSpecs: + def test_returns_one_spec_per_installed_expert_skill(self, tmp_path): + # Two expert skills + one utility skill. + _write_expert_skill_file(tmp_path, "expert-a") + _write_expert_skill_file(tmp_path, "expert-b") + util = tmp_path / "util-c" + util.mkdir() + (util / "SKILL.md").write_text( + """--- +name: util-c +description: Not an expert +--- + +# Body +""" + ) + + registry = { + "think_tool": _FakeTool("think_tool"), + "skill_manager": _FakeTool("skill_manager"), + } + # Patch USER_SKILLS_DIR to point at our temp dir; patch GLOBAL and + # SKILLS_DIR to empty locations so `list_expert_skills(include_system=True)` + # only surfaces our two experts. + empty_dir = tmp_path / "empty" + empty_dir.mkdir() + + with ( + patch("EvoScientist.paths.USER_SKILLS_DIR", tmp_path), + patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", empty_dir), + patch("EvoScientist.EvoScientist.SKILLS_DIR", str(empty_dir)), + ): + specs = build_expert_subagent_specs(tool_registry=registry) + + names = sorted(s["name"] for s in specs) + assert names == ["expert-a", "expert-b"] + for s in specs: + assert s["_async"] is False + assert s["skills"] == ["/skills/"] + assert s["tools"] == [ + registry["think_tool"], + registry["skill_manager"], + ] + + def test_skips_expert_with_empty_body(self, tmp_path, caplog): + # A well-formed expert-frontmatter skill whose body is only whitespace. + # Registering it would advertise a personaless expert in the `task` + # schema — cleaner to drop it and log. + _write_expert_skill_file(tmp_path, "expert-a") + blank = tmp_path / "expert-blank" + blank.mkdir() + (blank / "SKILL.md").write_text( + """--- +name: expert-blank +description: An expert with no body +type: expert +role: blank +--- + +""" + ) + empty_dir = tmp_path / "empty" + empty_dir.mkdir() + with ( + patch("EvoScientist.paths.USER_SKILLS_DIR", tmp_path), + patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", empty_dir), + patch("EvoScientist.EvoScientist.SKILLS_DIR", str(empty_dir)), + ): + specs = build_expert_subagent_specs(tool_registry={}) + + assert [s["name"] for s in specs] == ["expert-a"] + assert any( + "SKILL.md body is empty" in r.message and "expert-blank" in r.message + for r in caplog.records + ) + + def test_returns_empty_when_no_expert_skills(self, tmp_path): + # A utility skill only — no experts. + util = tmp_path / "util-only" + util.mkdir() + (util / "SKILL.md").write_text( + """--- +name: util-only +description: Utility +--- + +# Body +""" + ) + empty_dir = tmp_path / "empty" + empty_dir.mkdir() + with ( + patch("EvoScientist.paths.USER_SKILLS_DIR", tmp_path), + patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", empty_dir), + patch("EvoScientist.EvoScientist.SKILLS_DIR", str(empty_dir)), + ): + specs = build_expert_subagent_specs(tool_registry={}) + assert specs == [] + + +# ============================================================================= +# _fold_expert_subagents (name-collision guard shared by both construction paths) +# ============================================================================= + + +def _spec(name: str) -> dict: + """Minimal expert spec — the fold helper only reads ``name``.""" + return {"name": name, "description": f"{name} expert"} + + +class TestFoldExpertSubagents: + """Both ``_build_base_kwargs`` and ``load_mcp_and_build_kwargs`` delegate + to ``_fold_expert_subagents``, so testing the helper directly covers the + "same behaviour in both paths" reviewer requirement.""" + + def test_appends_expert_specs_when_no_collisions(self): + from EvoScientist.EvoScientist import _fold_expert_subagents + + subs: list[dict] = [{"name": "research"}, {"name": "code"}] + with patch( + "EvoScientist.subagents.expert_container.build_expert_subagent_specs", + return_value=[_spec("idea-brainstorm"), _spec("critic")], + ): + _fold_expert_subagents(subs, tool_registry={}) + + assert [s["name"] for s in subs] == [ + "research", + "code", + "idea-brainstorm", + "critic", + ] + + def test_skips_expert_that_collides_with_yaml_subagent(self, caplog): + from EvoScientist.EvoScientist import _fold_expert_subagents + + subs: list[dict] = [{"name": "research"}, {"name": "planner"}] + with patch( + "EvoScientist.subagents.expert_container.build_expert_subagent_specs", + return_value=[_spec("planner"), _spec("idea-brainstorm")], + ): + _fold_expert_subagents(subs, tool_registry={}) + + # Colliding expert dropped; non-colliding one appended. + assert [s["name"] for s in subs] == [ + "research", + "planner", + "idea-brainstorm", + ] + # Original YAML `planner` untouched (not shadowed by the expert). + assert subs[1] == {"name": "planner"} + assert any( + "collides with an existing sub-agent name" in r.message + and "planner" in r.message + for r in caplog.records + ) + + def test_skips_duplicate_expert_names(self, caplog): + from EvoScientist.EvoScientist import _fold_expert_subagents + + subs: list[dict] = [] + with patch( + "EvoScientist.subagents.expert_container.build_expert_subagent_specs", + return_value=[_spec("critic"), _spec("critic")], + ): + _fold_expert_subagents(subs, tool_registry={}) + + assert [s["name"] for s in subs] == ["critic"] + assert any( + "collides with an existing sub-agent name" in r.message + and "critic" in r.message + for r in caplog.records + ) + + def test_reserves_general_purpose_name(self, caplog): + """The default subagent slot is reserved even when no ``general-purpose`` + entry exists in ``subs`` yet — ``_ensure_general_purpose_subagent`` + runs right after the fold and would otherwise treat the expert entry + as the default subagent, silently losing the DeepAgents default prompt.""" + from EvoScientist.EvoScientist import _fold_expert_subagents + + subs: list[dict] = [{"name": "research"}] + with patch( + "EvoScientist.subagents.expert_container.build_expert_subagent_specs", + return_value=[_spec("general-purpose")], + ): + _fold_expert_subagents(subs, tool_registry={}) + + assert [s["name"] for s in subs] == ["research"] + assert any( + "collides with an existing sub-agent name" in r.message + and "general-purpose" in r.message + for r in caplog.records + ) + + def test_forwards_tool_registry_to_specs_factory(self): + from EvoScientist.EvoScientist import _fold_expert_subagents + + registry = {"think_tool": object()} + with patch( + "EvoScientist.subagents.expert_container.build_expert_subagent_specs", + return_value=[], + ) as mock_specs: + _fold_expert_subagents([], tool_registry=registry) + + mock_specs.assert_called_once_with(tool_registry=registry) + + +# ============================================================================= +# End-to-end wiring — both kwargs builders register experts with skill_manager +# ============================================================================= + + +class TestExpertWiringInBuildKwargs: + """Regression pin on the two-line wiring in both construction paths: + (a) ``tool_registry["skill_manager"]`` is populated so + ``_DEFAULT_EXPERT_TOOLS`` resolves and (b) ``_fold_expert_subagents`` is + invoked with that registry so the expert spec's ``tools`` carry the real + callable. Drift on either half silently drops ``skill_manager`` from every + expert — the exact regression fix commit ``34586c7`` addressed. A single + installed expert exercises both halves in one call.""" + + @staticmethod + def _install_one_expert(root: Path) -> Path: + skill_dir = root / "expert-a" + skill_dir.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_text( + """--- +name: expert-a +description: A test expert +type: expert +role: test expert +--- + +Second-person persona body content. +""" + ) + empty = root / "empty" + empty.mkdir(exist_ok=True) + return empty + + def test_build_base_kwargs_registers_expert_with_skill_manager(self, tmp_path): + from EvoScientist import EvoScientist as evo + from EvoScientist.tools import skill_manager + + empty = self._install_one_expert(tmp_path) + + with ( + patch("EvoScientist.paths.USER_SKILLS_DIR", tmp_path), + patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", empty), + patch("EvoScientist.EvoScientist.SKILLS_DIR", str(empty)), + patch.object(evo, "_inject_subagent_middleware", lambda subs, **k: None), + patch.object( + evo, + "_maybe_swap_async_subagents", + lambda subs, mw, cfg=None: subs, + ), + ): + kwargs = evo._build_base_kwargs( + base_backend=None, + base_middleware=[], + chat_model=object(), + ) + + expert = next( + (s for s in kwargs["subagents"] if s.get("name") == "expert-a"), None + ) + assert expert is not None, "expert-a not folded into subagents" + assert skill_manager in expert["tools"], ( + "skill_manager missing from expert tools — tool_registry wiring drifted" + ) + + def test_load_mcp_and_build_kwargs_registers_expert_with_skill_manager( + self, tmp_path + ): + from EvoScientist import EvoScientist as evo + from EvoScientist.tools import skill_manager + + empty = self._install_one_expert(tmp_path) + + # Non-empty mcp_by_agent forces the MCP branch. Without it, + # load_mcp_and_build_kwargs delegates to _build_base_kwargs and we + # re-test the first path only. + with ( + patch("EvoScientist.paths.USER_SKILLS_DIR", tmp_path), + patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", empty), + patch("EvoScientist.EvoScientist.SKILLS_DIR", str(empty)), + patch.object(evo, "_load_mcp_tools_cached", return_value={"main": []}), + patch.object(evo, "_inject_subagent_middleware", lambda subs, **k: None), + patch.object( + evo, + "_maybe_swap_async_subagents", + lambda subs, mw, cfg=None: subs, + ), + ): + kwargs = evo.load_mcp_and_build_kwargs( + base_backend=None, + base_middleware=[], + chat_model=object(), + ) + + expert = next( + (s for s in kwargs["subagents"] if s.get("name") == "expert-a"), None + ) + assert expert is not None, "expert-a not folded into subagents (MCP path)" + assert skill_manager in expert["tools"], ( + "skill_manager missing from expert tools on MCP path" + ) diff --git a/tests/test_langgraph_dev_http.py b/tests/test_langgraph_dev_http.py index 88cd3ca..3d01e26 100644 --- a/tests/test_langgraph_dev_http.py +++ b/tests/test_langgraph_dev_http.py @@ -1,6 +1,6 @@ -"""Smoke test for the /api/models route mounted via langgraph.json's -``http`` field. We test the FastAPI app directly — no need to spin up -langgraph dev. +"""Smoke tests for the /api/models and /api/teams routes mounted via +langgraph.json's ``http`` field. We test the Starlette app directly — no +need to spin up langgraph dev. """ from __future__ import annotations @@ -161,3 +161,138 @@ def test_ollama_discovery_skipped_when_base_url_absent(): {"name": n, "model_id": m, "provider": p} for n, m, p in list_models_by_provider() ] + + +# ---- /api/teams ----------------------------------------------------------- + + +def _expert_info( + name: str, + *, + description: str = "", + byline: str = "", + capability_tags: list[str] | None = None, + avatar_hint: str = "", +): + """Build a SkillInfo for an expert skill (agent-teams v1).""" + from pathlib import Path + + from EvoScientist.tools.skills_manager import SkillInfo + + return SkillInfo( + name=name, + description=description or f"{name} description", + path=Path(f"/skills/{name}"), + source="builtin", + type="expert", + byline=byline, + capability_tags=list(capability_tags or []), + avatar_hint=avatar_hint, + ) + + +def test_get_teams_returns_installed_expert_skills(): + experts = [ + _expert_info("expert-a", description="First expert"), + _expert_info("expert-b", description="Second expert"), + ] + with patch( + "EvoScientist.tools.skills_manager.list_expert_skills", + return_value=experts, + ): + resp = client.get("/api/teams") + assert resp.status_code == 200 + body = resp.json() + assert "teams" in body + names = [t["name"] for t in body["teams"]] + assert names == ["expert-a", "expert-b"] + + +def test_get_teams_omits_backend_implementation_fields(): + """Never leak SKILL.md body / role / dispatch / source / path / etc. + onto the gallery endpoint — those are backend-only.""" + experts = [_expert_info("expert-a")] + with patch( + "EvoScientist.tools.skills_manager.list_expert_skills", + return_value=experts, + ): + body = client.get("/api/teams").json() + entry = body["teams"][0] + forbidden = { + "system_prompt", + "role", + "default_dispatch", + "type", + "source", + "path", + "tools", + "skills", + "tags", + "_async", + } + assert not (set(entry.keys()) & forbidden), ( + f"leaked backend fields: {set(entry.keys()) & forbidden}" + ) + + +def test_get_teams_projects_optional_gallery_metadata_when_present(): + experts = [ + _expert_info( + "idea-brainstorm", + description="Multi-round brainstorm", + byline="Research idea brainstormer", + capability_tags=["Iteration", "ELO ranking"], + avatar_hint="lightbulb", + ), + ] + with patch( + "EvoScientist.tools.skills_manager.list_expert_skills", + return_value=experts, + ): + body = client.get("/api/teams").json() + entry = body["teams"][0] + assert entry["name"] == "idea-brainstorm" + assert entry["description"] == "Multi-round brainstorm" + assert entry["byline"] == "Research idea brainstormer" + assert entry["capability_tags"] == ["Iteration", "ELO ranking"] + assert entry["avatar_hint"] == "lightbulb" + + +def test_get_teams_omits_optional_fields_when_absent(): + """Gallery card should degrade gracefully when an expert declares + only the minimum (name, description, type: expert).""" + experts = [_expert_info("minimal-expert")] # no byline / tags / avatar + with patch( + "EvoScientist.tools.skills_manager.list_expert_skills", + return_value=experts, + ): + body = client.get("/api/teams").json() + entry = body["teams"][0] + assert set(entry.keys()) == {"name", "description"} + + +def test_get_teams_returns_empty_list_when_no_experts_installed(): + with patch( + "EvoScientist.tools.skills_manager.list_expert_skills", + return_value=[], + ): + body = client.get("/api/teams").json() + assert body == {"teams": []} + + +def test_get_teams_calls_loader_with_include_system_true(): + """First-party experts ship as builtin skills; the endpoint must + include the builtin tier or the gallery will be empty on a fresh + workspace with no user-installed experts.""" + calls = [] + + def spy(include_system=False): + calls.append(include_system) + return [] + + with patch( + "EvoScientist.tools.skills_manager.list_expert_skills", + new=spy, + ): + client.get("/api/teams") + assert calls == [True] diff --git a/tests/test_skills_manager.py b/tests/test_skills_manager.py index f541239..7ed7edc 100644 --- a/tests/test_skills_manager.py +++ b/tests/test_skills_manager.py @@ -6,6 +6,7 @@ from unittest.mock import patch import pytest from EvoScientist.tools.skills_manager import ( + SkillInfo, _is_github_url, _load_manifest, _parse_github_url, @@ -17,6 +18,7 @@ from EvoScientist.tools.skills_manager import ( install_skill, installed_provenance, installed_sources, + list_expert_skills, list_skills, list_skills_by_tag, resolve_remote_head, @@ -945,6 +947,403 @@ class TestSkillManagerList: assert "No user skills installed" in result +# ============================================================================= +# Tests for expert-skill fields (agent-teams v1) +# ============================================================================= + + +def _write_expert_skill( + parent: Path, + name: str, + *, + role: str = "One-line role", + byline: str = "Test persona", + capability_tags: list[str] | None = None, + avatar_hint: str = "star", + default_dispatch: str = "sync", + include_description: bool = True, +) -> Path: + """Write an expert SKILL.md under parent// and return the skill dir.""" + skill_dir = parent / name + skill_dir.mkdir(parents=True, exist_ok=True) + tags_str = "[" + ", ".join(capability_tags or []) + "]" if capability_tags else "[]" + desc_line = f"description: A {name} expert skill\n" if include_description else "" + (skill_dir / "SKILL.md").write_text( + f"""--- +name: {name} +{desc_line}type: expert +role: {role} +byline: {byline} +capability_tags: {tags_str} +avatar_hint: {avatar_hint} +default_dispatch: {default_dispatch} +--- + +# {name} + +Expert-skill body. +""" + ) + return skill_dir + + +class TestParseSkillMdExpertFields: + """`_parse_skill_md` extracts expert-skill frontmatter fields onto SkillInfo.""" + + def test_utility_default_when_type_absent(self, sample_skill_dir): + """Existing skills (no `type` field) default to utility with empty expert fields.""" + result = _parse_skill_md(sample_skill_dir / "SKILL.md") + assert result.type == "utility" + assert result.role == "" + assert result.byline == "" + assert result.capability_tags == [] + assert result.avatar_hint == "" + assert result.default_dispatch == "" + + def test_expert_fields_extracted(self, tmp_path): + skill_dir = _write_expert_skill( + tmp_path, + "expert-a", + role="Expert in A", + byline="A Byline", + capability_tags=["tag-1", "tag-2"], + avatar_hint="atom", + default_dispatch="panel", + ) + result = _parse_skill_md(skill_dir / "SKILL.md") + assert result.type == "expert" + assert result.role == "Expert in A" + assert result.byline == "A Byline" + assert result.capability_tags == ["tag-1", "tag-2"] + assert result.avatar_hint == "atom" + assert result.default_dispatch == "panel" + + def test_body_populated_from_skill_md(self, tmp_path): + """`SkillInfo.body` carries the post-frontmatter content so the expert + container factory can build the system_prompt without re-reading disk.""" + skill_dir = _write_expert_skill(tmp_path, "expert-body") + result = _parse_skill_md(skill_dir / "SKILL.md") + assert "# expert-body" in result.body + assert "Expert-skill body." in result.body + # Frontmatter fences must NOT leak into the body. + assert "---" not in result.body + + def test_body_captured_when_no_frontmatter(self, tmp_path): + """Skills without a frontmatter block still cache their full text as + the body — the expert-container factory can then use it without a + second disk read.""" + skill_dir = tmp_path / "no-frontmatter" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text("Just body content, no fences.\n") + result = _parse_skill_md(skill_dir / "SKILL.md") + assert result.body.strip() == "Just body content, no fences." + + def test_unknown_type_falls_back_to_utility(self, tmp_path, caplog): + """A typo in `type` (e.g. `charcter`) must not silently register as an expert + and must log so authors can debug a missing-expert case.""" + skill_dir = tmp_path / "typo-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + """--- +name: typo-skill +description: Has a bad type value +type: charcter +role: This should be ignored +--- + +# Body +""" + ) + result = _parse_skill_md(skill_dir / "SKILL.md") + assert result.type == "utility" + assert any( + "unrecognized type" in r.message and "charcter" in r.message + for r in caplog.records + ) + + def test_invalid_default_dispatch_falls_back_to_empty(self, tmp_path): + skill_dir = tmp_path / "bad-dispatch" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + """--- +name: bad-dispatch +description: Has a bad default_dispatch +type: expert +role: Some role +default_dispatch: asynchronous +--- + +# Body +""" + ) + result = _parse_skill_md(skill_dir / "SKILL.md") + assert result.type == "expert" + assert result.default_dispatch == "" # rejected, not passed through + + def test_capability_tags_accepts_comma_string(self, tmp_path): + """capability_tags falls back to comma-separated string parsing (like `tags`).""" + skill_dir = tmp_path / "comma-tags" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + """--- +name: comma-tags +description: Comma-separated capability tags +type: expert +capability_tags: alpha, beta, gamma +--- + +# Body +""" + ) + result = _parse_skill_md(skill_dir / "SKILL.md") + assert result.capability_tags == ["alpha", "beta", "gamma"] + + def test_unreadable_skill_md_returns_placeholder(self, tmp_path, caplog): + """A non-UTF-8 SKILL.md must degrade to an ``(unreadable)`` placeholder + rather than raise. ``list_skills`` sits on the agent-construction hot + path (via ``_fold_expert_subagents``); an uncaught ``UnicodeDecodeError`` + would abort CLI startup for a single malformed skill in any tier.""" + skill_dir = tmp_path / "bad-utf8" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_bytes(b"---\nname: bad\n---\n\xff\xfe") + result = _parse_skill_md(skill_dir / "SKILL.md") + assert result.name == "bad-utf8" + assert result.description == "(unreadable)" + assert result.type == "utility" + assert result.body == "" + assert any("could not read" in r.message for r in caplog.records) + + def test_empty_name_value_coerces_to_parent_dir(self, tmp_path): + """A ``name:`` line with no value parses to ``None`` in YAML. Without + the ``.get(...) or parent.name`` guard, ``SkillInfo.name`` becomes + ``None`` and slips past the ``_fold_expert_subagents`` collision + check (a ``None``-named expert would land in the ``task`` schema).""" + skill_dir = tmp_path / "empty-name" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + """--- +name: +description: A skill with an empty name value +type: expert +role: some role +--- + +Body content. +""" + ) + result = _parse_skill_md(skill_dir / "SKILL.md") + assert result.name == "empty-name" + + def test_invalid_frontmatter_yaml_warns(self, tmp_path, caplog): + """Malformed YAML frontmatter must return the ``(invalid frontmatter)`` + placeholder AND log so the author sees why the skill didn't register.""" + skill_dir = tmp_path / "bad-yaml" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + """--- +name: bad-yaml +description: broken YAML below +tags: [unterminated +--- + +Body. +""" + ) + result = _parse_skill_md(skill_dir / "SKILL.md") + assert result.description == "(invalid frontmatter)" + assert any("invalid frontmatter YAML" in r.message for r in caplog.records) + + +class TestListExpertSkills: + """`list_expert_skills()` filters `list_skills()` to `type == 'expert'`.""" + + def test_returns_only_expert_skills(self, tmp_path): + workspace_dir = tmp_path / "workspace" + workspace_dir.mkdir() + global_dir = tmp_path / "global" + global_dir.mkdir() + # An expert skill and a utility skill, both in workspace tier. + _write_expert_skill(workspace_dir, "expert-a") + util = workspace_dir / "util-b" + util.mkdir() + (util / "SKILL.md").write_text( + """--- +name: util-b +description: Plain utility skill +--- + +# Body +""" + ) + with ( + patch("EvoScientist.paths.USER_SKILLS_DIR", workspace_dir), + patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", global_dir), + ): + all_skills = list_skills() + expert_skills = list_expert_skills(include_system=False) + assert {s.name for s in all_skills} == {"expert-a", "util-b"} + assert [s.name for s in expert_skills] == ["expert-a"] + + def test_empty_when_no_expert_skills_installed(self, tmp_path): + workspace_dir = tmp_path / "workspace" + workspace_dir.mkdir() + global_dir = tmp_path / "global" + global_dir.mkdir() + util = workspace_dir / "util-only" + util.mkdir() + (util / "SKILL.md").write_text( + """--- +name: util-only +description: Utility +--- + +# Body +""" + ) + with ( + patch("EvoScientist.paths.USER_SKILLS_DIR", workspace_dir), + patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", global_dir), + ): + expert_skills = list_expert_skills(include_system=False) + assert expert_skills == [] + + +class TestSkillManagerToolExpertSurface: + """`skill_manager` @tool exposes the expert-skill fields and filter.""" + + def _mock_skills(self): + return [ + SkillInfo( + name="expert-a", + description="A brainstorm expert", + path=Path("/skills/expert-a"), + source="builtin", + tags=["research"], + type="expert", + role="Research idea brainstormer", + byline="Ideation persona", + capability_tags=["Iteration", "ELO"], + avatar_hint="lightbulb", + default_dispatch="sync", + ), + SkillInfo( + name="util-b", + description="A utility skill", + path=Path("/skills/util-b"), + source="workspace", + tags=["core"], + ), + ] + + def test_list_filters_to_expert_when_skill_type_set(self): + from EvoScientist.tools.skill_manager import skill_manager + + with patch( + "EvoScientist.tools.skills_manager.list_skills", + return_value=self._mock_skills(), + ): + out = skill_manager.invoke( + {"action": "list", "include_system": True, "skill_type": "expert"} + ) + assert "expert-a" in out + assert "util-b" not in out + + def test_skill_type_enum_contains_no_empty_string(self): + """Gemini's function-declaration schema rejects empty enum values + (`GenerateContentRequest.tools[N].function_declarations[N].parameters.properties[skill_type].enum[0]: cannot be empty`). + The `skill_type` argument must use a non-empty sentinel (`"all"`) + as its no-filter default, never `""`. + + This test guards against silently reintroducing the empty-string + default that broke the live agent-teams smoke on 2026-07-17. + """ + from EvoScientist.tools.skill_manager import skill_manager + + schema = skill_manager.args_schema.model_json_schema() + skill_type_prop = schema.get("properties", {}).get("skill_type", {}) + # Pydantic/JSON-schema serialization of a Literal[...] shows up as + # `enum` on the property directly OR nested under `anyOf`. + enum_values: list[str] = [] + if "enum" in skill_type_prop: + enum_values = list(skill_type_prop["enum"]) + else: + for branch in skill_type_prop.get("anyOf", []): + if "enum" in branch: + enum_values.extend(branch["enum"]) + assert enum_values, "skill_type Literal should surface as enum in the schema" + assert "" not in enum_values, ( + f"Empty string in skill_type enum will break Gemini: {enum_values}" + ) + + def test_list_all_sentinel_is_no_filter(self): + """`skill_type='all'` (the default) must return every skill — + it's the no-filter case, not a bucket that only 'all' skills fall into.""" + from EvoScientist.tools.skill_manager import skill_manager + + with patch( + "EvoScientist.tools.skills_manager.list_skills", + return_value=self._mock_skills(), + ): + out = skill_manager.invoke( + {"action": "list", "include_system": True, "skill_type": "all"} + ) + # Both should appear — 'all' is not a filter to a bucket named "all". + assert "expert-a" in out + assert "util-b" in out + + def test_list_all_when_skill_type_absent(self): + from EvoScientist.tools.skill_manager import skill_manager + + with patch( + "EvoScientist.tools.skills_manager.list_skills", + return_value=self._mock_skills(), + ): + out = skill_manager.invoke({"action": "list", "include_system": True}) + assert "expert-a" in out + assert "util-b" in out + + def test_list_returns_message_when_filter_matches_nothing(self): + from EvoScientist.tools.skill_manager import skill_manager + + with patch( + "EvoScientist.tools.skills_manager.list_skills", + return_value=self._mock_skills()[1:], # only the utility + ): + out = skill_manager.invoke( + {"action": "list", "include_system": True, "skill_type": "expert"} + ) + assert "No expert skills found" in out + + def test_info_surfaces_expert_fields(self): + from EvoScientist.tools.skill_manager import skill_manager + + with patch( + "EvoScientist.tools.skills_manager.get_skill_info", + return_value=self._mock_skills()[0], + ): + out = skill_manager.invoke({"action": "info", "name": "expert-a"}) + assert "Type: expert" in out + assert "Role: Research idea brainstormer" in out + assert "Byline: Ideation persona" in out + assert "Capability tags: Iteration, ELO" in out + assert "Avatar hint: lightbulb" in out + assert "Default dispatch: sync" in out + + def test_info_omits_expert_block_for_utility_skills(self): + from EvoScientist.tools.skill_manager import skill_manager + + with patch( + "EvoScientist.tools.skills_manager.get_skill_info", + return_value=self._mock_skills()[1], + ): + out = skill_manager.invoke({"action": "info", "name": "util-b"}) + assert "Type: expert" not in out + assert "Role:" not in out + assert "Byline:" not in out + assert "Capability tags:" not in out + assert "Default dispatch:" not in out + + class TestSkillManagerInfo: """Tests for the skill_manager() tool's action='info' output.