From ab1a6b0062768718ae59382dc841b93b6466d85f Mon Sep 17 00:00:00 2001 From: jfilipiuk Date: Wed, 5 Aug 2026 11:50:32 +0200 Subject: [PATCH] feat: adopt the AGENTS.md expert-skill contract (#404) * feat: detect expert skills by AGENTS.md presence * feat: let the orchestrator choose the expert dispatch tool * refactor: drop per-skill expert dispatch classification * chore: remove duplicate TestSkillManager test classes left by rebase * fix: clear legacy actor fields when AGENTS.md declares the expert * refactor: move the expert prompt into ActiveTeamMiddleware --- EvoScientist/EvoScientist.py | 25 +- .../commands/implementation/experts.py | 21 +- EvoScientist/langgraph_dev/http.py | 20 +- EvoScientist/middleware/active_team.py | 188 +++---- EvoScientist/prompts.py | 4 +- EvoScientist/subagents/expert_container.py | 157 +++--- .../subagents/expert_container_async.py | 55 +- EvoScientist/tools/skill_manager.py | 12 +- EvoScientist/tools/skills_manager.py | 233 +++++++-- tests/test_active_team_middleware.py | 166 ++---- tests/test_expert_container.py | 222 +++++--- tests/test_expert_container_async.py | 46 ++ tests/test_experts_command.py | 36 +- tests/test_route_async_specs.py | 115 ++-- tests/test_skills_manager.py | 493 ++++++++---------- 15 files changed, 971 insertions(+), 822 deletions(-) diff --git a/EvoScientist/EvoScientist.py b/EvoScientist/EvoScientist.py index 9908ae9..dba2fea 100644 --- a/EvoScientist/EvoScientist.py +++ b/EvoScientist/EvoScientist.py @@ -411,9 +411,10 @@ 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. + the main agent's ``task`` tool (and the QuickJS ``task()`` global) can + dispatch to it in-turn by name. The same experts independently get a + background reach via ``build_expert_async_subagent_specs``; the two + reaches land on separate tool schemas, so sharing the name is safe. Skips (with a warning) any expert whose ``name`` collides with a subagent already in ``subs`` or with ``general-purpose``. The reserved @@ -563,7 +564,7 @@ def _route_async_specs_through_evo_middleware( specs from ``subs`` here and hand them to our middleware. Also folds in ``AsyncSubAgent`` specs for installed - ``default_dispatch: async`` expert skills — all pointing at the shared + installed expert skills — all pointing at the shared ``expert-container-async`` graph, marked ``is_expert=True`` so the middleware requires a payload with ``skill_name``. @@ -962,13 +963,6 @@ def _get_default_middleware( ErrorNormalizationMiddleware(), ToolHistoryRepairMiddleware(), ConfigurableModelMiddleware(), - # Team-binding cue for the main agent only. Reads - # `configurable.active_teams: list[str]` and appends a cue biasing - # the main agent to consult the invited expert(s). Skipped for - # async subagents (a running expert graph shouldn't inject a - # "prefer expert X" hint into its own system prompt — the persona - # is already baked in). See agent-teams-design.md. - *([] if for_async_subagent else [create_active_team_middleware()]), create_context_editing_middleware(model), ModelFallbackMiddleware(events=events), ContextOverflowMapperMiddleware(), @@ -1009,6 +1003,15 @@ def _get_default_middleware( mw.insert(0, AskUserMiddleware()) + # Expert prompt for the main agent — injects the ## Experts concept every + # turn (plus the invited-expert list when experts are invited). Inserted + # AFTER AskUser so it sits ahead of AskUser in the stack and runs first, + # landing its block right after ## Skills System (experts mirror skills). + # Main agent only: a running expert graph must not inject the expert prompt + # into its own baked-in persona. + if not for_async_subagent: + mw.insert(0, create_active_team_middleware()) + # Background-process tools (run_in_background / check_process / stop_process / # list_processes) — main agent only. Async sub-agents run on langgraph-dev and # must not spawn local OS processes. diff --git a/EvoScientist/commands/implementation/experts.py b/EvoScientist/commands/implementation/experts.py index 27c237a..77e1e1d 100644 --- a/EvoScientist/commands/implementation/experts.py +++ b/EvoScientist/commands/implementation/experts.py @@ -104,14 +104,12 @@ class ExpertsCommand(Command): table = Table(title=f"Expert Skills ({len(experts)})", show_header=True) table.add_column("Name", style="cyan") table.add_column("Role", style="dim") - table.add_column("Dispatch", style="dim") table.add_column("Active", style="green") for skill in experts: marker = "*" if skill.name in active else "" table.add_row( skill.name, skill.role or skill.description, - skill.default_dispatch or "sync", marker, ) ctx.ui.mount_renderable(table) @@ -203,30 +201,19 @@ class ExpertCommand(Command): dispatchable = {s.name for s in _dispatchable_experts()} if target not in dispatchable: - from ...subagents.expert_container import is_async_dispatch_available from ...tools.skills_manager import list_expert_skills - installed = {s.name: s for s in list_expert_skills(include_system=True)} - match = installed.get(target) - if match is None: + installed = {s.name for s in list_expert_skills(include_system=True)} + if target not in installed: ctx.ui.append_system( f"No expert skill named '{target}'. `/experts` lists " "installed ones.", style="red", ) - elif ( - match.default_dispatch == "async" and not is_async_dispatch_available() - ): - ctx.ui.append_system( - f"Expert '{target}' declares async dispatch but async " - "dispatch is unavailable (enable_async_subagents is off " - "or langgraph dev is not reachable).", - style="red", - ) else: ctx.ui.append_system( - f"Expert '{target}' can't be dispatched (empty SKILL.md body " - "or name collision with a built-in sub-agent).", + f"Expert '{target}' can't be dispatched (empty actor " + "definition or name collision with a built-in sub-agent).", style="red", ) return diff --git a/EvoScientist/langgraph_dev/http.py b/EvoScientist/langgraph_dev/http.py index 3b2375d..f5f1ae4 100644 --- a/EvoScientist/langgraph_dev/http.py +++ b/EvoScientist/langgraph_dev/http.py @@ -78,14 +78,22 @@ 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. + A "team" in the WebUI vocabulary is an installed expert skill — a skill + directory carrying a sibling ``AGENTS.md`` (or, on the deprecated path, + ``type: expert`` SKILL.md frontmatter). The response is a curated, + gallery-safe projection: name + description, plus optional ``byline`` / + ``capability_tags`` / ``avatar_hint`` when the skill populates them. + + Cards for experts on the current contract carry name + description only: + the decoration fields were actor metadata in SKILL.md frontmatter, which + that contract removes rather than relocates (``AGENTS.md`` has no + frontmatter to hold them). The omit-when-unpopulated projection below is + what makes those cards degrade rather than break; restoring richer cards + means sourcing decoration from index metadata, not re-adding frontmatter + fields. Backend implementation details (SKILL.md body / system prompt, role - line, default_dispatch, tool list, source tier, filesystem path, + line, 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. diff --git a/EvoScientist/middleware/active_team.py b/EvoScientist/middleware/active_team.py index 58d7667..e39a86d 100644 --- a/EvoScientist/middleware/active_team.py +++ b/EvoScientist/middleware/active_team.py @@ -1,46 +1,28 @@ -"""ActiveTeamMiddleware for EvoScientist agent-teams v1. +"""ActiveTeamMiddleware: the expert prompt for the main agent. -Reads ``configurable.active_teams: list[str]`` on every model call and -appends a system-prompt cue biasing the main agent to consult the -user-invited expert(s). +Injects the ``## Experts`` concept into the system message on every +main-agent turn, so the expert mechanism is always visible — mirroring how +the skill system's guidance is always present. When the user has invited +experts (``configurable.active_teams``), an ```` block naming +the reachable ones is appended on top. -The cue is dispatch-aware: for each active expert the middleware looks up -its ``default_dispatch`` (via ``list_expert_skills()``) and emits the -tool-shape cue that matches how the expert actually runs: - -- ``sync`` / ``panel`` / unset -> ``task({subagent_type: 'X', ...})``. -- ``async`` -> ``start_async_task(subagent_type: 'X', payload: {skill_name: - 'X', output_path: '...'})``, plus a reminder that ``check_async_task`` - returns the status/result later. - -Without the dispatch-aware branch, an ``async`` expert like -``literature-review`` gets told to use ``task()``, which routes it back -through the sync ``SubAgentMiddleware`` rather than the async graph the -container registers for it. +An expert is a fractal of a skill, so this block is ordered (via the +middleware stack in ``_get_default_middleware``) to land right after +``## Skills System``. Gating the whole block on invitation is the trap this +design avoids: the expert mechanism must not disappear when nothing is +invited, and the invited-expert list must not read as a standalone "always +dispatch an expert" directive. Backend-stateless team binding: WebUI sends ``active_teams`` on every ``stream.submit()`` for as long as the invited expert is active; this -middleware reads it fresh per turn via ``langgraph.config.get_config()``. -Matches the plan's decision to reach for the ``configurable`` primitive -rather than a server-side thread-state store (CLAUDE.md #5). +middleware reads it fresh per turn via ``langgraph.config.get_config()`` — +the ``configurable`` primitive, not a server-side thread-state store +(CLAUDE.md #5). The wire key stays ``active_teams`` (plural, legacy from the +earlier "teams" framing); the semantic content is a list of expert names. -Naming note: the WIRE FORMAT is ``configurable.active_teams`` (plural, -legacy from the earlier "teams" framing that survived the pivot per the -WebUI section of the design note). Under the current expert-skill -mechanism the semantic content is a list of expert names, but the -wire key stays ``active_teams`` for WebUI compatibility. Internal -system-prompt tags use ```` / ```` -because that matches what the LLM sees as the semantic target. - -No-op when: -- ``configurable.active_teams`` is absent, empty, non-list, or contains - no non-empty string entries. -- The middleware is invoked outside a runnable context (``get_config`` - raises). - -Not included in the async-subagent middleware stack: an expert running -as its own graph would otherwise inject a "prefer expert X" cue into -its own system prompt, where the persona is already baked in. See +Not included in the async-subagent middleware stack: an expert running as +its own graph would otherwise inject the expert prompt into its own system +message, where its persona is already baked in. See ``EvoScientist.py::_get_default_middleware``. """ @@ -54,46 +36,32 @@ from langchain.agents.middleware.types import ( ModelResponse, ) -# Per-expert cue shapes. Composed inside ``_TEMPLATE_SINGLE`` / -# ``_TEMPLATE_MULTI`` at render time so the wrapping tags stay in sync -# with the count of active experts. -_SYNC_CUE = ( - "Consult it via `task({{subagent_type: '{expert}', description: '...'}})`. " - "It runs synchronously and returns its result to the same turn." -) +# The expert concept — injected on every main-agent turn so the mechanism is +# always visible, mirroring how ``## Skills System`` is always present. Moved +# here from ``DELEGATION_STRATEGY`` (an expert is a fractal of a skill, so its +# guidance belongs next to the skill system's). The invited-expert list is +# appended below only when the user has invited experts this session. +EXPERTS_CONCEPT = """## Experts +An expert is an installed skill that also ships an actor definition — a persona and a result-envelope contract. Every installed expert is reachable both ways, and the choice is yours per task, not fixed per expert: -_ASYNC_CUE = ( - "Consult it via `start_async_task(description: '.md`` verbatim>', " - "subagent_type: '{expert}')`. It runs in the background and returns a " - "task_id immediately; the result artifact is written to the path you " - "named in the description. Use ``check_async_task`` to poll status " - "when the user asks. On ``status: 'success'`` the ``result`` field " - "contains a JSON envelope with ``output_path``, a one-paragraph " - "``summary``, and a skill-defined ``metadata`` block (fields vary by " - "expert — e.g. ``word_count`` / ``section_count`` / ``citations_used`` " - "for surveys). Render ``summary`` and ``metadata`` directly to the " - "user — do not re-read the artifact to build a synopsis." -) +- `task({subagent_type: '', description: ...})` — runs in-turn and returns into the current turn. Use when the answer is short and the user is waiting on it. +- `start_async_task(subagent_type: '', description: ...)` — runs in the background, returns a task ID immediately. Use when the work is long-running or its deliverable is a file. **Name a concrete output path in the description** (e.g. "write to `./artifacts//.md`") — the expert honours the path you give it. On `status: 'success'`, `check_async_task` returns a `result` envelope with `output_path`, a one-paragraph `summary`, and an expert-defined `metadata` block; render `summary` and `metadata` to the user directly rather than re-reading the artifact to build a synopsis. -_TEMPLATE_SINGLE = ( - "\n" - "The user has invited the expert `{expert}` to this thread. " - "{cue} " - "It stays available for the whole session until the user dismisses it.\n" +Prefer the background form when unsure — expert work is usually multi-step, and it keeps the conversation responsive. + +You need not dispatch at all. An expert's `SKILL.md` is ordinary knowledge on the `/skills/` mount: read it and do the work yourself when the task is small, or when the full conversation context matters more than a fresh sub-agent would. If an `` block appears below, the user invited that expert specifically — prefer it for requests in its scope.""" + +# Appended to ``EXPERTS_CONCEPT`` only when the user has invited reachable +# experts. One ```` tag handles one or many names. +_INVITE_TEMPLATE = ( + "\n\n\n" + "The user has invited {experts} to this thread. Prefer the right one for " + "requests within its scope; do not consult an expert if the request is " + "clearly outside its scope. They stay available for the whole session " + "until the user dismisses them.\n" "" ) -_TEMPLATE_MULTI_HEADER = ( - "\n" - "The user has invited the following experts to this thread: {experts}. " - "Consult the right one for the current request; do not consult an expert " - "if the request is clearly outside its scope. Per-expert dispatch:\n" -) - -_TEMPLATE_MULTI_FOOTER = "" - def _read_active_teams() -> list[str]: """Read ``configurable.active_teams`` from the current RunnableConfig. @@ -120,37 +88,29 @@ def _read_active_teams() -> list[str]: return [t for t in raw if isinstance(t, str) and t] -def _dispatch_by_name() -> dict[str, str]: - """Return ``{skill_name: default_dispatch}`` for currently dispatchable experts. +def _dispatchable_names() -> set[str]: + """Return the names of experts the orchestrator can currently reach. Fresh filesystem read every call so a ``skill_manager install `` is visible on the next turn without an agent rebuild. Cheap at current scale (a handful of skills, cached bodies). - Sourced from ``list_dispatchable_experts`` — which filters empty-body - experts, name collisions with reserved sub-agents, AND async-declared - experts when async dispatch is unavailable (``enable_async_subagents`` - off or langgraph dev unreachable). Keeps the cue honest: any expert - named here can actually be reached by the tool shape the cue advertises. + Sourced from ``list_dispatchable_experts``, which drops empty-body + experts and names colliding with reserved sub-agents. Keeps the cue + honest: naming an expert the model cannot reach is worse than saying + nothing. - On import failure returns an empty dict — the middleware then emits no + On import failure returns an empty set — the middleware then emits no cue, matching the outside-runnable-context no-op path. """ try: from ..subagents.expert_container import list_dispatchable_experts except Exception: - return {} + return set() try: - return {s.name: s.default_dispatch for s in list_dispatchable_experts()} + return {s.name for s in list_dispatchable_experts()} except Exception: - return {} - - -def _cue_shape_for(dispatch: str, expert: str) -> str: - """Return the ``task()`` / ``start_async_task(...)`` cue for one expert.""" - if dispatch == "async": - return _ASYNC_CUE.format(expert=expert) - return _SYNC_CUE.format(expert=expert) + return set() class ActiveTeamMiddleware(AgentMiddleware): @@ -158,46 +118,32 @@ class ActiveTeamMiddleware(AgentMiddleware): name = "active_team" - def _cue_for(self, experts: list[str]) -> str: - """Render the cue over the dispatchable subset of ``experts``. + def _invite_block(self, experts: list[str]) -> str: + """Render the ```` block over the dispatchable subset. Invited experts that aren't currently dispatchable (uninstalled, - empty body, name collision, or async-declared while async - dispatch is unavailable) are dropped from the cue — pointing the - model at a tool shape that will fail is worse than saying nothing. - Returns the empty string when nothing survives the filter; caller - skips the system-prompt append in that case. + empty actor definition, name collision) are dropped — naming an + expert the model cannot reach is worse than saying nothing. Returns + the empty string when nothing survives the filter. """ - dispatch_map = _dispatch_by_name() - experts = [e for e in experts if e in dispatch_map] + reachable = _dispatchable_names() + experts = [e for e in experts if e in reachable] if not experts: return "" - if len(experts) == 1: - expert = experts[0] - cue = _cue_shape_for(dispatch_map[expert], expert) - return _TEMPLATE_SINGLE.format(expert=expert, cue=cue) - experts_str = ", ".join(f"`{e}`" for e in experts) - per_expert_lines = "\n".join( - f"- `{e}`: {_cue_shape_for(dispatch_map[e], e)}" for e in experts - ) - return ( - _TEMPLATE_MULTI_HEADER.format(experts=experts_str) - + per_expert_lines - + "\n" - + _TEMPLATE_MULTI_FOOTER - ) + names = ", ".join(f"`{e}`" for e in experts) + return _INVITE_TEMPLATE.format(experts=names) def modify_request(self, request: ModelRequest) -> ModelRequest: - """Append the active-expert cue to the request's system message.""" - experts = _read_active_teams() - if not experts: - return request - cue = self._cue_for(experts) - if not cue: - return request + """Append the expert concept (always) plus the invited-expert block + (when the user has invited reachable experts) to the system message. + """ + block = EXPERTS_CONCEPT + invited = _read_active_teams() + if invited: + block += self._invite_block(invited) from .utils import append_to_system_message - new_system = append_to_system_message(request.system_message, cue) + new_system = append_to_system_message(request.system_message, block) return request.override(system_message=new_system) def wrap_model_call( diff --git a/EvoScientist/prompts.py b/EvoScientist/prompts.py index 1eb45ff..ccc59d4 100644 --- a/EvoScientist/prompts.py +++ b/EvoScientist/prompts.py @@ -341,9 +341,9 @@ Launch multiple sub-agents only when experiments are independent: ## Dispatch Mechanisms Three ways to reach a sub-agent — pick based on what you need: -- **Sequential `task`** — the default. Emit one `task({subagent_type: ..., description: ...})` tool call, wait for the result, integrate, continue. Use for a single-shot consult, including an **expert consult** when the user has invited an expert to the thread (see any `` cue in the system prompt). +- **Sequential `task`** — the default. Emit one `task({subagent_type: ..., description: ...})` tool call, wait for the result, integrate, continue. Use for a single-shot consult. -- **In-eval `task()` fan-out via `code_interpreter`** — write a short JS script that dispatches N `task()` calls concurrently and synthesises results in the same eval. Use for independent parallel work: **expert panels** (dispatch to multiple invited experts and synthesise), ELO-style tournaments, N-way method / dataset comparisons where results are independent. Prefer `Promise.allSettled` over `Promise.all` so one failed dispatch does not fail the whole eval — inspect each entry's `status` and retry only the failed subset. +- **In-eval `task()` fan-out via `code_interpreter`** — write a short JS script that dispatches N `task()` calls concurrently and synthesises results in the same eval. Use for independent parallel work: expert panels, ELO-style tournaments, N-way method / dataset comparisons where results are independent. Prefer `Promise.allSettled` over `Promise.all` so one failed dispatch does not fail the whole eval — inspect each entry's `status` and retry only the failed subset. - **`start_async_task`** — spawn a long-running background job that returns a task ID immediately; poll with `check_async_task` or continue when the async notification arrives. Use for work that will take minutes to hours (long training runs, exhaustive experiments, whole pipelines). The user can keep working in the main conversation while it runs. diff --git a/EvoScientist/subagents/expert_container.py b/EvoScientist/subagents/expert_container.py index 8ab96eb..c27549a 100644 --- a/EvoScientist/subagents/expert_container.py +++ b/EvoScientist/subagents/expert_container.py @@ -1,17 +1,23 @@ -"""Expert-subagent-spec factory for the v1 agent-teams feature. +"""Expert-subagent-spec factory for the 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. +each installed expert in-turn; the same registry is reused by the QuickJS +`task()` global for in-eval fan-out. + +A skill declares itself an expert by carrying a sibling `AGENTS.md`, whose body +is the actor definition and therefore the runtime prompt — see +`skills_manager._parse_skill_md`. THIS module builds the in-turn (`task()`) spec +for such experts and for legacy `type: expert` frontmatter experts alike; +`expert_container_async.py` independently builds the background +(`start_async_task`) spec for the same experts, so every expert is reachable +both ways and the orchestrator picks per task. 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. +rather than a per-expert YAML. """ from __future__ import annotations @@ -40,14 +46,35 @@ _DEFAULT_EXPERT_TOOLS: tuple[str, ...] = ("think_tool", "skill_manager") _DEFAULT_EXPERT_SKILLS: tuple[str, ...] = ("/skills/",) -def _body_of(skill_info: SkillInfo) -> str: - """Return the SKILL.md body (post-frontmatter content). +def expert_prompt_body(skill_info: SkillInfo) -> str: + """Return the text that becomes *skill_info*'s runtime prompt. - 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. + The single answer to "which file carries this expert's persona", for + every path that builds or validates an expert — sync spec factory, async + spec factory, and the async loader middleware. The two contracts source + it differently, and splitting that decision across call sites is how a + skill ends up registered off one file and prompted off another: + + - ``expert_source == "agents_md"`` — the sibling AGENTS.md body (persona + + result envelope). SKILL.md stays pure knowledge and stays readable + in-turn off the ``/skills/`` mount, so it is deliberately NOT the + prompt here. + - legacy frontmatter experts — the SKILL.md body, which is where those + skills put their persona before the actor/knowledge split. + + Prefers the text cached on ``SkillInfo`` by ``_parse_skill_md``. Falls + back to reading from disk when the cached field is empty — that handles + ``SkillInfo`` objects constructed by hand (external callers) without the + body populated. Returns an empty string if the file can't be read; the + callers treat that as "refuse to register this expert". """ + if skill_info.expert_source == "agents_md": + if skill_info.agents_body: + return skill_info.agents_body + from ..tools.skills_manager import _read_agents_md + + return _read_agents_md(skill_info.path) or "" + if skill_info.body: return skill_info.body skill_md = skill_info.path / "SKILL.md" @@ -66,12 +93,17 @@ def _body_of(skill_info: SkillInfo) -> str: def _compose_system_prompt(skill_info: SkillInfo, body: str) -> str: - """Compose the expert's system_prompt from its role + SKILL.md body. + """Compose the expert's system_prompt from its role + prompt 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). + *body* is what :func:`expert_prompt_body` resolved — an AGENTS.md actor + definition, or a legacy expert's SKILL.md body. Either way it carries + the persona voice, rubrics, and output-style instructions, written in + second person addressing the expert itself. + + The legacy `role` frontmatter (one-line role summary) is prepended as an + orientation line when set. AGENTS.md experts declare no `role` — their + persona section opens with the same orientation in prose — so for those + the body passes through untouched. """ if skill_info.role: return f"You are {skill_info.role}.\n\n{body}".rstrip() + "\n" @@ -100,7 +132,7 @@ def build_expert_subagent_spec( to append to the main agent's `subagents=[...]` list. """ tool_registry = tool_registry or {} - body = _body_of(skill_info) + body = expert_prompt_body(skill_info) system_prompt = _compose_system_prompt(skill_info, body) resolved_tools: list[Any] = [] @@ -166,71 +198,37 @@ def _reserved_subagent_names() -> frozenset[str]: return _reserved_subagent_names_cache -def is_async_dispatch_available(cfg: Any | None = None) -> bool: - """Return True when async-declared experts can actually be dispatched. - - Both gates must hold: ``enable_async_subagents`` opt-in AND langgraph - dev subprocess reachable. Same predicate - ``build_expert_async_subagent_specs`` uses at spec-build time, factored - out so ``list_dispatchable_experts`` (invite whitelist) and - ``ActiveTeamMiddleware`` (system-prompt cue) can honour it without - each re-deriving. - """ - if cfg is None: - from ..config import get_effective_config - - cfg = get_effective_config() - if not getattr(cfg, "enable_async_subagents", False): - return False - from ..langgraph_dev.manager import is_async_subagents_available - - return is_async_subagents_available() - - def list_dispatchable_experts( *, include_system: bool = True, cfg: Any | None = None ) -> list[SkillInfo]: - """Experts eligible for the ``/expert`` invite whitelist. + """Experts the orchestrator can actually reach. - Covers **both** sync (``task()``) and async (``start_async_task``) - dispatch shapes — an async-declared expert must remain invitable when - async dispatch is registered, or the active-team cue that instructs - ``start_async_task(...)`` never fires. Do NOT add a ``default_dispatch - == "async"`` exclusion here; the sync-specific exclusion lives in - ``build_expert_subagent_specs``, which is a different surface. + Every installed expert is reachable, so this applies only the two + filters that would otherwise register a broken entry: an empty actor + definition (nothing to prompt with) and a name collision with a yaml + sub-agent or ``general-purpose`` (which would shadow the built-in). - Async-declared experts are dropped when async dispatch is NOT - registered (``enable_async_subagents=false`` or langgraph dev - unreachable). Advertising an expert that resolves to a - ``start_async_task`` tool that either doesn't exist or doesn't list - the expert is worse than an honest refusal — see reviewer thread on - PR #391. + Deliberately does NOT filter on async availability. An expert whose + background reach is unavailable is still reachable in-turn via + ``task()``, so dropping it here would hide a working expert — the + failure mode that made installed experts vanish whenever langgraph + dev was unhealthy. - Combines ``list_expert_skills`` with the two filters that - construction-time paths already apply (empty body via - ``build_expert_subagent_specs``; name collision with yaml sub-agents - or ``general-purpose`` via ``_fold_expert_subagents``). Callers - surfacing experts to the user (e.g. the ``/expert`` slash command) - should use this instead of ``list_expert_skills`` directly, otherwise - they can accept a name that will silently misroute or per-turn error - at dispatch time. - - Read-only filter — construction-time warnings for empty-body / - colliding experts are emitted by ``build_expert_subagent_specs`` and + ``cfg`` is accepted and ignored; kept so callers that thread config + through don't have to special-case this one. Read-only filter — + construction-time warnings for empty-body / colliding experts are + emitted by ``build_expert_subagent_specs`` and ``_fold_expert_subagents`` respectively, so nothing is logged here. """ from ..tools.skills_manager import list_expert_skills reserved = _reserved_subagent_names() - async_available = is_async_dispatch_available(cfg=cfg) dispatchable: list[SkillInfo] = [] for info in list_expert_skills(include_system=include_system): - if not _body_of(info).strip(): + if not expert_prompt_body(info).strip(): continue if info.name in reserved: continue - if info.default_dispatch == "async" and not async_available: - continue dispatchable.append(info) return dispatchable @@ -240,34 +238,33 @@ def build_expert_subagent_specs( *, include_system: bool = True, ) -> list[dict[str, Any]]: - """Build spec dicts for every installed sync-dispatched expert skill. + """Build the in-turn (``task``) spec 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 + Every expert gets a spec here, and ``build_expert_async_subagent_specs`` + independently gives every expert a background spec. The same name in + both is intentional and safe: the two land on different tools + (``task`` vs ``start_async_task``) with separate schemas, and + deepagents' duplicate-name check is scoped to the async list alone. + Two reaches, one expert — the orchestrator picks per task. + + Skips (with a warning) any expert whose actor definition 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. - - Experts declared with ``default_dispatch: async`` are excluded — those - are folded via ``build_expert_async_subagent_specs`` in - ``expert_container_async.py`` and reached through - ``EvoAsyncSubAgentMiddleware.start_async_task`` instead of the sync - ``task`` tool. A skill that appears in both lists would produce two - competing tool schemas for the same subagent name. """ from ..tools.skills_manager import list_expert_skills specs: list[dict[str, Any]] = [] for info in list_expert_skills(include_system=include_system): - if info.default_dispatch == "async": - continue - if not _body_of(info).strip(): + if not expert_prompt_body(info).strip(): _logger.warning( - "Expert skill %r: SKILL.md body is empty; skipping registration.", + "Expert skill %r: %s body is empty; skipping registration.", info.name, + "AGENTS.md" if info.expert_source == "agents_md" else "SKILL.md", ) continue specs.append(build_expert_subagent_spec(info, tool_registry=tool_registry)) diff --git a/EvoScientist/subagents/expert_container_async.py b/EvoScientist/subagents/expert_container_async.py index e0b75fa..97a669b 100644 --- a/EvoScientist/subagents/expert_container_async.py +++ b/EvoScientist/subagents/expert_container_async.py @@ -1,9 +1,11 @@ """Async container graph for expert-skill dispatch. One generic graph that reads ``skill_name`` from initial state and loads -that expert skill's ``SKILL.md`` body as the sub-agent's system prompt at -invocation time. Registered once in ``langgraph.json``; parameterised per -run via the payload the main agent's ``start_async_task`` passes through +that expert skill's actor definition as the sub-agent's system prompt at +invocation time — its ``AGENTS.md`` body under the current skill contract, +or its ``SKILL.md`` body for legacy ``type: expert`` skills. Registered once +in ``langgraph.json``; parameterised per run via the payload the main +agent's ``start_async_task`` passes through :class:`EvoScientist.middleware.expert_async_subagent.EvoAsyncSubAgentMiddleware`. Why one shared graph rather than one graph per expert: the per-expert @@ -65,7 +67,7 @@ class ExpertContainerState(DeepAgentState): ``subagent_type`` so the loader middleware knows which persona to load. ``output_path`` is NOT in state; the main agent embeds the desired artifact path in the task description (natural language) and - the expert's SKILL.md contract instructs the LLM to pin it in + the expert's actor definition instructs the LLM to pin it in ``write_todos`` on turn 1 — surviving summarization via langgraph's todo composition. """ @@ -74,13 +76,15 @@ class ExpertContainerState(DeepAgentState): class ExpertSkillLoaderMiddleware(AgentMiddleware[Any, Any, Any]): - """Load the expert skill's SKILL.md body as system prompt on every model call. + """Load the expert's actor definition as system prompt on every model call. Reads ``state.skill_name``, resolves the corresponding installed expert skill via ``list_expert_skills()``, composes the system message from - ``role`` + SKILL.md body (mirrors the sync path in - ``EvoScientist.subagents.expert_container._compose_system_prompt``), - and overrides ``request.system_message`` before the handler runs. + ``role`` + the skill's prompt body (AGENTS.md under the current contract, + SKILL.md for legacy frontmatter experts — resolved by + ``expert_container.expert_prompt_body``, mirroring the sync path in + ``expert_container._compose_system_prompt``), and overrides + ``request.system_message`` before the handler runs. The container graph's static ``system_prompt`` at construction time is a minimal fallback; this middleware is the load-bearing component. If the @@ -134,9 +138,21 @@ class ExpertSkillLoaderMiddleware(AgentMiddleware[Any, Any, Any]): # persona-less system prompt — a worse failure mode than the expert # being absent. Prefer a well-formed error envelope over silent # nonsense. - if not (match.body or "").strip(): + # + # Which file is checked follows the skill's contract: + # ``expert_prompt_body`` reads AGENTS.md for experts declared that + # way and SKILL.md for legacy frontmatter experts. Checking ``.body`` + # directly would clear a paper-review-shaped skill on the strength of + # its knowledge file while its actor definition is empty. + from .expert_container import expert_prompt_body + + body = expert_prompt_body(match) + if not body.strip(): + source_file = ( + "AGENTS.md" if match.expert_source == "agents_md" else "SKILL.md" + ) return ( - f"ERROR: Expert skill '{skill_name}' has an empty SKILL.md " + f"ERROR: Expert skill '{skill_name}' has an empty {source_file} " "body — the persona / pipeline the sub-agent needs is missing. " "This is a skill-authoring bug; the sub-agent cannot proceed. " "Return an error envelope with status='error' naming the " @@ -144,7 +160,6 @@ class ExpertSkillLoaderMiddleware(AgentMiddleware[Any, Any, Any]): ) # Compose: role prepend (if present) + body + runtime-context tail. - body = match.body or "" head = f"You are {match.role}.\n\n" if match.role else "" runtime_block = ( "\n---\n\n" @@ -203,7 +218,12 @@ class ExpertSkillLoaderMiddleware(AgentMiddleware[Any, Any, Any]): def build_expert_async_subagent_specs(cfg: Any | None = None) -> list[dict[str, Any]]: - """Build ``AsyncSubAgent``-shaped specs for every ``default_dispatch: async`` expert. + """Build ``AsyncSubAgent``-shaped specs for every installed expert skill. + + Every expert gets a background reach here, and + ``build_expert_subagent_specs`` independently gives every expert an + in-turn reach. Nothing classifies a skill into one or the other — the + orchestrator chooses per task. Each spec is a dict pointing at the shared ``expert-container-async`` graph with ``is_expert=True``. The main agent's @@ -229,7 +249,7 @@ def build_expert_async_subagent_specs(cfg: Any | None = None) -> list[dict[str, return [] from ..tools.skills_manager import list_expert_skills - from .expert_container import _reserved_subagent_names + from .expert_container import _reserved_subagent_names, expert_prompt_body port = int(getattr(cfg, "langgraph_dev_port", 6174)) # Mirror the sync fold-in's name guard in ``_fold_expert_subagents``: @@ -244,18 +264,17 @@ def build_expert_async_subagent_specs(cfg: Any | None = None) -> list[dict[str, taken = set(_reserved_subagent_names()) specs: list[dict[str, Any]] = [] for skill in list_expert_skills(include_system=True): - if skill.default_dispatch != "async": - continue # Same empty-body skip the sync fold-in enforces in # ``expert_container.py::build_expert_subagent_specs``. Advertising # a body-less expert in ``start_async_task``'s tool schema, then # rejecting it at loader time, wastes a launch round-trip; filter # upstream so ``start_async_task`` never sees the broken skill. - if not (skill.body or "").strip(): + if not expert_prompt_body(skill).strip(): _logger.warning( - "Expert skill %r: SKILL.md body is empty; skipping " + "Expert skill %r: %s body is empty; skipping " "async-dispatch registration.", skill.name, + "AGENTS.md" if skill.expert_source == "agents_md" else "SKILL.md", ) continue if skill.name in taken: @@ -284,7 +303,7 @@ def build_expert_container_async_graph() -> Any: Called once at langgraph dev startup. The returned graph accepts ``{messages, skill_name}`` as initial state; the :class:`ExpertSkillLoaderMiddleware` resolves ``skill_name`` on every - model call and injects the matching SKILL.md body as system prompt. + model call and injects that expert's actor definition as system prompt. Tool set is intentionally minimal (``think_tool`` only) — matches the sync ``expert_container`` factory. Once the per-skill ``allowed-tools`` diff --git a/EvoScientist/tools/skill_manager.py b/EvoScientist/tools/skill_manager.py index bb2b62b..179fead 100644 --- a/EvoScientist/tools/skill_manager.py +++ b/EvoScientist/tools/skill_manager.py @@ -40,8 +40,8 @@ def skill_manager( action="info" (requires name): Get details (description, source, path, tags) about a specific skill by name. - Expert skills also surface their role, byline, capability tags, and default - dispatch mode. Searches both user and system skills. + Expert skills also surface their role, byline, and capability tags. + Searches both user and system skills. action="uninstall" (requires name): Remove a user-installed skill by name. System skills cannot be uninstalled. @@ -206,8 +206,10 @@ def skill_manager( ] 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. + # Expert surface: shown when the skill can also act as an expert — + # declared by a sibling AGENTS.md, or by legacy `type: expert` + # frontmatter. The decoration lines below exist only on the legacy + # path, so an AGENTS.md expert renders the type line and stops. if info.type == "expert": lines.append("Type: expert") if info.role: @@ -218,8 +220,6 @@ def skill_manager( 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: diff --git a/EvoScientist/tools/skills_manager.py b/EvoScientist/tools/skills_manager.py index a995b99..9f8e873 100644 --- a/EvoScientist/tools/skills_manager.py +++ b/EvoScientist/tools/skills_manager.py @@ -118,12 +118,21 @@ class SkillInfo: 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" | "async" (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 = "" + # How this skill came to be classified as an expert: + # "agents_md" — a sibling AGENTS.md is present (current contract) + # "frontmatter" — legacy ``type: expert`` in SKILL.md (deprecated) + # "" — not an expert + expert_source: str = "" + # AGENTS.md body — the actor definition (persona + result envelope) that + # becomes the expert's runtime prompt. Empty for non-experts and for + # experts still on the legacy frontmatter path, whose runtime prompt is + # still sourced from ``body``. + agents_body: str = "" # Shared frontmatter split regex. Captures three parts: @@ -153,6 +162,74 @@ def _split_frontmatter_and_body(content: str) -> tuple[str, str]: return match.group(1), body +# --- expert declaration ----------------------------------------------------- +# +# A skill directory declares itself an expert by containing this file. The +# rule the skill system already lives by, applied one level down: a directory +# is a package, a contract file is a capability, presence is the declaration, +# content is the contract. SKILL.md's presence makes a directory a skill; +# AGENTS.md's presence makes it an actor. +# +# The file carries no frontmatter — the directory name is the expert's +# identity, its presence is the declaration, and its body (persona + result +# envelope) is the runtime prompt. Nothing about it is machine-read beyond +# "does it exist" and "what does it say". +# +# ``metadata.type: [skill, expert]`` in SKILL.md frontmatter is deliberately +# NOT consulted here. That field is a projection of this declaration for +# index-facing consumers (the remote skill index, gallery chips) which can't +# cheaply stat the directory; the runtime resolves presence directly, so +# reading it would create a second classifier that can drift from the file. +_AGENTS_FILENAME = "AGENTS.md" + +# Legacy ``type: expert`` frontmatter is a deprecated fallback, so the warning +# is one-shot per skill name — ``_parse_skill_md`` runs on every ``list_skills`` +# call (main-agent construction, /expert completion, GET /api/teams), and a +# per-parse warning would bury real diagnostics under repeats. +_legacy_expert_warned: set[str] = set() + + +def _warn_once(key: str, message: str, *args: object) -> None: + """Emit *message* at WARNING the first time *key* is seen this process.""" + if key in _legacy_expert_warned: + return + _legacy_expert_warned.add(key) + _logger.warning(message, *args) + + +def _read_agents_md(skill_dir: Path) -> str | None: + """Return the AGENTS.md body for *skill_dir*, or None when absent. + + Returns a string (possibly empty) whenever the file exists, so callers + can distinguish "not an expert" (None) from "declared expert with an + empty actor definition" (""). The latter is a skill-authoring bug the + container paths refuse loudly rather than papering over. + + Frontmatter is stripped if present. The contract says AGENTS.md carries + none, but an author who adds some should not have raw YAML leak into the + expert's system prompt. + """ + agents_path = skill_dir / _AGENTS_FILENAME + try: + if not agents_path.is_file(): + return None + content = agents_path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError) as exc: + # Presence was the declaration, so an unreadable AGENTS.md is a + # broken expert, not a plain skill. Return "" to keep the expert + # classification and let the empty-body guards in the container + # factories refuse it with a skill-name-bearing warning. + _logger.warning( + "Skill %r: could not read %s (%s); treating actor definition as empty.", + skill_dir.name, + agents_path, + exc, + ) + return "" + _, body = _split_frontmatter_and_body(content) + return body + + def _normalize_tags(raw: object) -> list[str]: """Normalize a tags value to a list of strings.""" if isinstance(raw, list): @@ -300,25 +377,50 @@ 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, tags, and - (for expert skills) role / byline / capability_tags / avatar_hint / - default_dispatch. + """Parse a skill directory into a :class:`SkillInfo`. + + Reads name / description / tags from SKILL.md frontmatter, and resolves + whether the skill is also an **expert** — an actor with a persona and a + result-envelope contract, dispatchable as its own agent. + + Expert classification is structural: a sibling ``AGENTS.md`` in the same + directory declares the skill an expert, and that file's body is the + expert's runtime prompt. SKILL.md stays pure knowledge — portable to any + harness, and readable in-turn off the ``/skills/`` mount whether or not + the skill also acts:: + + my-expert/ + SKILL.md # knowledge: when to use, workflow, references + AGENTS.md # actor: persona + result envelope <- the declaration + scripts/ + references/ + + Skills predating that convention declare ``type: expert`` in SKILL.md + frontmatter, alongside actor fields (``role`` / ``byline`` / + ``capability_tags`` / ``avatar_hint`` / ``default_dispatch``). That path + still resolves, with a deprecation warning, and is still the source of + the WebUI gallery's decoration fields. New experts declare via AGENTS.md + and set none of those fields; their gallery cards carry name and + description only. + + ``default_dispatch`` is read from neither contract. Every expert is + reachable both in-turn and in the background, and the orchestrator picks + per task — a skill does not declare one dispatch shape for all time. + + When both are present, AGENTS.md wins on every axis (prompt source, + dispatch) and the frontmatter actor fields are reported as ignored — a + skill mid-migration must not behave as a blend of the two contracts. + + SKILL.md format:: - SKILL.md format: --- name: skill-name description: A brief description... + allowed-tools: "read_file write_file think_tool" 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 + tags: [tag1, tag2] # fallback location + type: [skill, expert] # index-facing only; never read here --- # Skill Title ... @@ -331,6 +433,8 @@ 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 + agents_body = _read_agents_md(parent) + declares_actor = agents_body is not None try: content = skill_md_path.read_text(encoding="utf-8") except (OSError, UnicodeDecodeError) as exc: @@ -347,11 +451,17 @@ def _parse_skill_md(skill_md_path: Path, *, source: str = "") -> SkillInfo: skill_md_path, exc, ) + # An unreadable SKILL.md doesn't undo an AGENTS.md declaration — the + # actor definition is a separate file and may be perfectly intact — + # so the expert classification is applied here too. return SkillInfo( name=parent.name, description="(unreadable)", path=parent, source=source, + type="expert" if declares_actor else "utility", + expert_source="agents_md" if declares_actor else "", + agents_body=agents_body or "", ) def _info( @@ -364,9 +474,58 @@ def _parse_skill_md(skill_md_path: Path, *, source: str = "") -> SkillInfo: byline: str = "", capability_tags: list[str] | None = None, avatar_hint: str = "", - default_dispatch: str = "", + legacy_dispatch: str = "", body: str = "", ) -> SkillInfo: + # AGENTS.md presence overrides whatever the frontmatter says about + # actor-hood: the declaration is the file, and a skill mid-migration + # (both present) must resolve to exactly one contract. + if declares_actor: + if ( + type_ == "expert" + or role + or byline + or capability_tags + or avatar_hint + or legacy_dispatch + ): + _warn_once( + f"{parent}:mixed", + "Skill %r: %s is present, so the skill is an expert and its " + "actor definition comes from that file; legacy actor " + "frontmatter in SKILL.md (type / role / byline / " + "capability_tags / avatar_hint / default_dispatch) is " + "ignored and should be removed.", + name, + _AGENTS_FILENAME, + ) + type_ = "expert" + expert_source = "agents_md" + # AGENTS.md is authoritative on every axis: the actor definition is + # that file, so legacy decoration from SKILL.md frontmatter must not + # leak onto SkillInfo. ``role`` would otherwise be prepended to the + # AGENTS.md prompt (``_compose_system_prompt`` / the async head), and + # a mismatched frontmatter ``name`` would desync the registry + # identity from the skill directory. + name = parent.name + role = "" + byline = "" + capability_tags = [] + avatar_hint = "" + elif type_ == "expert": + _warn_once( + f"{parent}:legacy", + "Skill %r declares `type: expert` in SKILL.md frontmatter. That " + "is deprecated: add a sibling %s carrying the persona and " + "result envelope instead, and drop the actor fields from " + "frontmatter. The frontmatter path still works for now.", + name, + _AGENTS_FILENAME, + ) + expert_source = "frontmatter" + else: + expert_source = "" + return SkillInfo( name=name, description=description, @@ -378,8 +537,9 @@ def _parse_skill_md(skill_md_path: Path, *, source: str = "") -> SkillInfo: byline=byline, capability_tags=capability_tags or [], avatar_hint=avatar_hint, - default_dispatch=default_dispatch, body=body, + expert_source=expert_source, + agents_body=agents_body or "", ) frontmatter_yaml, body = _split_frontmatter_and_body(content) @@ -414,21 +574,12 @@ def _parse_skill_md(skill_md_path: Path, *, source: str = "") -> SkillInfo: 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", "async") else "" - ) - # Mirror the ``raw_type`` handling above. A typo like - # ``default_dispatch: asnyc`` would otherwise be indistinguishable - # from an unset field and would register the skill under sync - # dispatch — the opposite of the author's intent. - if raw_dispatch is not None and raw_dispatch != default_dispatch: - _logger.warning( - "Skill %r: unrecognized default_dispatch %r in frontmatter; " - "treating as unset (skill will register under sync dispatch).", - frontmatter.get("name") or parent.name, - raw_dispatch, - ) + # ``default_dispatch`` is no longer read: an expert is reachable both + # in-turn and in the background, and the orchestrator picks per task + # rather than the skill declaring one shape for all time. Still + # detected here so the mixed-contract warning can name it among the + # legacy actor fields an AGENTS.md skill should drop. + legacy_dispatch = str(frontmatter.get("default_dispatch") or "") # ``.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`` @@ -442,7 +593,7 @@ def _parse_skill_md(skill_md_path: Path, *, source: str = "") -> SkillInfo: byline=str(byline), capability_tags=capability_tags, avatar_hint=str(avatar_hint), - default_dispatch=default_dispatch, + legacy_dispatch=legacy_dispatch, body=body, ) except yaml.YAMLError as exc: @@ -967,13 +1118,19 @@ def list_skills(include_system: bool = False) -> list[SkillInfo]: def list_expert_skills(include_system: bool = True) -> list[SkillInfo]: - """List installed expert skills (agent-teams v1). + """List installed expert skills. - 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. + Filters ``list_skills()`` output to entries classified as experts by + ``_parse_skill_md`` — a sibling ``AGENTS.md`` (current contract), or + legacy ``type: expert`` SKILL.md frontmatter. Read ``expert_source`` on + the returned entries to tell the two apart; use + ``expert_container.expert_prompt_body`` rather than ``.body`` to get an + expert's runtime prompt, since the two contracts source it from + different files. + + Defaults ``include_system=True`` because first-party experts 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). diff --git a/tests/test_active_team_middleware.py b/tests/test_active_team_middleware.py index 3ad140e..afca36a 100644 --- a/tests/test_active_team_middleware.py +++ b/tests/test_active_team_middleware.py @@ -102,165 +102,105 @@ def test_read_active_teams_returns_empty_outside_runnable_context(mock_get_confi @patch("langgraph.config.get_config") -def test_middleware_no_op_when_active_teams_absent(mock_get_config): +def test_middleware_injects_concept_when_no_experts_invited(mock_get_config): + """The ## Experts concept is injected every turn, even with no invites. + + Gating the whole block on invitation would make the expert mechanism + vanish when nothing is invited — the trap the design avoids. + """ mock_get_config.return_value = {"configurable": {}} middleware = ActiveTeamMiddleware() - request = _request() - modified = middleware.modify_request(request) - # No override applied: original request returned as-is. - assert modified is request + modified = middleware.modify_request(_request()) + text = _system_text(modified) + assert "## Experts" in text + assert "The user has invited" not in text # no invite block without invitees + assert "base system" in text @patch("langgraph.config.get_config") -def test_middleware_no_op_when_active_teams_empty_list(mock_get_config): +def test_middleware_injects_concept_when_active_teams_empty_list(mock_get_config): mock_get_config.return_value = {"configurable": {"active_teams": []}} middleware = ActiveTeamMiddleware() - request = _request() - modified = middleware.modify_request(request) - assert modified is request + modified = middleware.modify_request(_request()) + text = _system_text(modified) + assert "## Experts" in text + assert "The user has invited" not in text -def _mock_expert(name: str, dispatch: str) -> MagicMock: - """Build a MagicMock ``SkillInfo`` with the given dispatch shape. +def _mock_expert(name: str) -> MagicMock: + """Build a MagicMock ``SkillInfo`` for a dispatchable expert. ``name`` on ``MagicMock`` must be set via attribute assignment; passing ``name=`` to the constructor names the mock instance itself. """ - info = MagicMock(default_dispatch=dispatch) + info = MagicMock() info.name = name return info @patch("EvoScientist.subagents.expert_container.list_dispatchable_experts") @patch("langgraph.config.get_config") -def test_middleware_appends_single_expert_cue(mock_get_config, mock_dispatchable): +def test_middleware_appends_invite_for_single_expert( + mock_get_config, mock_dispatchable +): mock_get_config.return_value = { "configurable": {"active_teams": ["idea-brainstorm"]}, } - mock_dispatchable.return_value = [_mock_expert("idea-brainstorm", "sync")] + mock_dispatchable.return_value = [_mock_expert("idea-brainstorm")] middleware = ActiveTeamMiddleware() modified = middleware.modify_request(_request()) text = _system_text(modified) - assert "" in text + assert "## Experts" in text # concept always present + assert "The user has invited" in text # plus the invite block assert "`idea-brainstorm`" in text - assert "Consult it via `task(" in text assert "base system" in text # original preserved @patch("EvoScientist.subagents.expert_container.list_dispatchable_experts") @patch("langgraph.config.get_config") -def test_middleware_appends_multi_expert_cue(mock_get_config, mock_dispatchable): +def test_middleware_appends_invite_for_multiple_experts( + mock_get_config, mock_dispatchable +): + """One tag names one or many invited experts.""" mock_get_config.return_value = { "configurable": {"active_teams": ["idea-brainstorm", "literature-review"]}, } mock_dispatchable.return_value = [ - _mock_expert("idea-brainstorm", "sync"), - _mock_expert("literature-review", "sync"), + _mock_expert("idea-brainstorm"), + _mock_expert("literature-review"), ] middleware = ActiveTeamMiddleware() modified = middleware.modify_request(_request()) text = _system_text(modified) - assert "" in text + assert "## Experts" in text + assert "The user has invited" in text + # One tag for any number of names — no separate plural tag. + assert "" not in text assert "`idea-brainstorm`" in text assert "`literature-review`" in text - # Multi-cue: header names both experts, then per-expert dispatch lines follow. - assert "The user has invited the following experts" in text - assert "Per-expert dispatch" in text assert "base system" in text @patch("EvoScientist.subagents.expert_container.list_dispatchable_experts") @patch("langgraph.config.get_config") -def test_middleware_omits_cue_for_undispatchable_names( +def test_middleware_omits_invite_for_undispatchable_names( mock_get_config, mock_dispatchable ): - """Names not in ``list_dispatchable_experts`` are dropped from the cue. + """Names not in ``list_dispatchable_experts`` are dropped from the invite. - Covers uninstalled experts, empty-body experts, name collisions, and - async-declared experts when async dispatch is unavailable — anything - the model would find missing at dispatch time. + Covers uninstalled experts, empty actor definitions, and name collisions + — anything the model would find missing at dispatch time. The concept + still shows; only the invite block is suppressed. """ mock_get_config.return_value = { "configurable": {"active_teams": ["nonexistent-expert"]}, } mock_dispatchable.return_value = [] # nothing dispatchable - request = _request() - middleware = ActiveTeamMiddleware() - modified = middleware.modify_request(request) - # No cue appended — modify_request returns the original request untouched. - assert modified is request - - -@patch("EvoScientist.subagents.expert_container.list_dispatchable_experts") -@patch("langgraph.config.get_config") -def test_middleware_uses_start_async_task_cue_for_async_dispatch( - mock_get_config, mock_dispatchable -): - """An expert declared ``default_dispatch: async`` gets the async cue. - - Only reaches the cue when async dispatch is actually registered — the - honest-surface filter in ``list_dispatchable_experts`` drops - async-declared experts otherwise. - """ - mock_get_config.return_value = { - "configurable": {"active_teams": ["literature-review"]}, - } - mock_dispatchable.return_value = [_mock_expert("literature-review", "async")] - middleware = ActiveTeamMiddleware() modified = middleware.modify_request(_request()) text = _system_text(modified) - assert "" in text - assert "start_async_task(" in text - assert "subagent_type: 'literature-review'" in text - # Post-X-4: no payload dict. The cue instructs the main agent to embed - # the desired output path directly in the description string. - assert "payload" not in text - assert "output path" in text.lower() or "output_path" in text - assert "check_async_task" in text - # Sync cue must NOT be advertised for async experts. - assert "Consult it via `task(" not in text - - -@patch("EvoScientist.subagents.expert_container.list_dispatchable_experts") -@patch("langgraph.config.get_config") -def test_middleware_uses_task_cue_for_sync_dispatch(mock_get_config, mock_dispatchable): - """Sync-dispatched experts get the ``task()`` cue.""" - mock_get_config.return_value = { - "configurable": {"active_teams": ["idea-brainstorm"]}, - } - mock_dispatchable.return_value = [_mock_expert("idea-brainstorm", "sync")] - - middleware = ActiveTeamMiddleware() - modified = middleware.modify_request(_request()) - text = _system_text(modified) - assert "Consult it via `task(" in text - assert "runs synchronously" in text - # No async-specific fragments for a sync expert. - assert "start_async_task(" not in text - assert "output_path" not in text - - -@patch("EvoScientist.subagents.expert_container.list_dispatchable_experts") -@patch("langgraph.config.get_config") -def test_middleware_multi_mixed_dispatch(mock_get_config, mock_dispatchable): - """When both sync and async experts are active, each gets its own cue.""" - mock_get_config.return_value = { - "configurable": {"active_teams": ["idea-brainstorm", "literature-review"]}, - } - mock_dispatchable.return_value = [ - _mock_expert("idea-brainstorm", "sync"), - _mock_expert("literature-review", "async"), - ] - - middleware = ActiveTeamMiddleware() - modified = middleware.modify_request(_request()) - text = _system_text(modified) - # Both cue shapes appear once each in the per-expert block. - assert text.count("`task(") == 1 - assert text.count("start_async_task(") == 1 - assert "`idea-brainstorm`:" in text - assert "`literature-review`:" in text + assert "## Experts" in text + assert "The user has invited" not in text @patch("EvoScientist.subagents.expert_container.list_dispatchable_experts") @@ -268,18 +208,16 @@ def test_middleware_multi_mixed_dispatch(mock_get_config, mock_dispatchable): def test_middleware_drops_invited_expert_that_is_not_dispatchable( mock_get_config, mock_dispatchable ): - """An async-declared expert stays invited across a config change, but - when async dispatch turns unavailable it drops out of - ``list_dispatchable_experts``. The cue must not mention it — otherwise - the model is told to reach for a tool that either doesn't exist or - doesn't list the expert.""" + """An invited expert that stops being dispatchable — uninstalled, or its + actor definition emptied — must drop out of the cue. Naming an expert + the model cannot reach is worse than saying nothing.""" mock_get_config.return_value = { "configurable": { "active_teams": ["idea-brainstorm", "literature-review"], }, } # literature-review invited but not dispatchable this turn. - mock_dispatchable.return_value = [_mock_expert("idea-brainstorm", "sync")] + mock_dispatchable.return_value = [_mock_expert("idea-brainstorm")] middleware = ActiveTeamMiddleware() modified = middleware.modify_request(_request()) @@ -288,15 +226,17 @@ def test_middleware_drops_invited_expert_that_is_not_dispatchable( assert "" in text assert "`idea-brainstorm`" in text assert "literature-review" not in text - assert "start_async_task(" not in text @patch("langgraph.config.get_config", side_effect=RuntimeError("outside context")) -def test_middleware_no_op_outside_runnable_context(mock_get_config): +def test_middleware_injects_concept_outside_runnable_context(mock_get_config): + """Outside a runnable context there are no invites, but the concept still + injects — ``_read_active_teams`` degrades to an empty list, not a raise.""" middleware = ActiveTeamMiddleware() - request = _request() - modified = middleware.modify_request(request) - assert modified is request + modified = middleware.modify_request(_request()) + text = _system_text(modified) + assert "## Experts" in text + assert "The user has invited" not in text # ---- composition tests: _get_default_middleware ---------------------------- diff --git a/tests/test_expert_container.py b/tests/test_expert_container.py index 6299951..2edbf05 100644 --- a/tests/test_expert_container.py +++ b/tests/test_expert_container.py @@ -7,11 +7,10 @@ from types import SimpleNamespace from unittest.mock import patch from EvoScientist.subagents.expert_container import ( - _body_of, _compose_system_prompt, build_expert_subagent_spec, build_expert_subagent_specs, - is_async_dispatch_available, + expert_prompt_body, list_dispatchable_experts, ) from EvoScientist.tools.skills_manager import SkillInfo @@ -70,15 +69,15 @@ class _FakeTool: # ============================================================================= -# _body_of +# expert_prompt_body # ============================================================================= -class TestBodyOf: +class TestExpertPromptBody: 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) + body = expert_prompt_body(info) assert body.startswith("You are a test expert.") assert "Do the thing." in body assert "---" not in body @@ -88,7 +87,7 @@ class TestBodyOf: # 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) + body = expert_prompt_body(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) @@ -101,7 +100,7 @@ class TestBodyOf: 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) + body = expert_prompt_body(info) assert body == "" assert any("could not read SKILL.md" in r.message for r in caplog.records) @@ -111,13 +110,13 @@ class TestBodyOf: 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) + body = expert_prompt_body(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 + ``expert_prompt_body`` uses it directly without touching disk. Guards against the double-read regression flagged by pre-PR review.""" info = SkillInfo( name="cached", @@ -127,9 +126,69 @@ class TestBodyOf: type="expert", body="Cached body content from SkillInfo.", ) - body = _body_of(info) + body = expert_prompt_body(info) assert body == "Cached body content from SkillInfo." + def test_agents_md_expert_reads_actor_definition_not_skill_md(self, tmp_path): + """An AGENTS.md expert is prompted from its actor definition. + + SKILL.md stays pure knowledge under the current contract — it is + reachable in-turn via ``load_skill`` — so leaking it into the system + prompt would both bloat the prompt and hand the expert a document + written for a different reader. + """ + info = SkillInfo( + name="paper-review", + description="d", + path=tmp_path / "paper-review", + source="builtin", + type="expert", + expert_source="agents_md", + body="# Knowledge\n\nThe 5-aspect checklist.\n", + agents_body="## Persona\n\nYou are an adversarial reviewer.\n", + ) + body = expert_prompt_body(info) + assert body == "## Persona\n\nYou are an adversarial reviewer.\n" + assert "5-aspect checklist" not in body + + def test_agents_md_expert_falls_back_to_disk(self, tmp_path): + """A hand-built SkillInfo without ``agents_body`` still resolves. + + Mirrors the SKILL.md fallback below it — external callers construct + SkillInfo objects without going through ``_parse_skill_md``. + """ + skill_dir = tmp_path / "on-disk" + skill_dir.mkdir() + (skill_dir / "AGENTS.md").write_text("## Persona\n\nFrom disk.\n") + info = SkillInfo( + name="on-disk", + description="d", + path=skill_dir, + source="workspace", + type="expert", + expert_source="agents_md", + body="SKILL.md body that must not be used.", + ) + assert expert_prompt_body(info) == "## Persona\n\nFrom disk.\n" + + def test_agents_md_expert_returns_empty_when_file_missing(self, tmp_path): + """Declared expert, no resolvable actor definition -> empty. + + Empty is the signal every registration path checks; falling back to + the SKILL.md body here would register a knowledge document as a + persona instead of refusing. + """ + info = SkillInfo( + name="gone", + description="d", + path=tmp_path / "gone", + source="workspace", + type="expert", + expert_source="agents_md", + body="SKILL.md body that must not be used.", + ) + assert expert_prompt_body(info) == "" + # ============================================================================= # _compose_system_prompt @@ -315,6 +374,76 @@ role: blank for r in caplog.records ) + def test_agents_md_expert_included_in_sync_registry(self, tmp_path): + """Both contracts land in the in-turn registry, and its prompt is AGENTS.md. + + Every expert gets both reaches, so an AGENTS.md skill appears here + as well as in the async specs. Sharing the name across the two is + safe: they land on different tools with separate schemas. + """ + _write_expert_skill_file(tmp_path, "legacy-expert") + actor = tmp_path / "new-expert" + actor.mkdir() + (actor / "SKILL.md").write_text( + """--- +name: new-expert +description: Knowledge only +--- + +# Knowledge + +The workflow. +""" + ) + (actor / "AGENTS.md").write_text("## Persona\n\nYou are the new expert.\n") + + 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={}) + + by_name = {s["name"]: s for s in specs} + assert set(by_name) == {"legacy-expert", "new-expert"} + # Prompted from the actor definition, not the knowledge file. + assert "You are the new expert." in by_name["new-expert"]["system_prompt"] + assert "The workflow." not in by_name["new-expert"]["system_prompt"] + + def test_legacy_async_dispatch_still_in_sync_registry(self, tmp_path): + """A legacy ``default_dispatch: async`` skill still gets an in-turn spec. + + ``default_dispatch`` is no longer read: every expert is reachable both + ways and the orchestrator picks per task, so an async-declared legacy + skill must not be dropped from the sync registry. This is the + behavioural counterpart to the parser's not-read contract. + """ + actor = tmp_path / "async-legacy" + actor.mkdir() + (actor / "SKILL.md").write_text( + """--- +name: async-legacy +description: Legacy expert that declared async dispatch +type: expert +role: legacy async expert +default_dispatch: async +--- + +You are the legacy async expert. +""" + ) + 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 "async-legacy" in {s["name"] for s in specs} + def test_returns_empty_when_no_expert_skills(self, tmp_path): # A utility skill only — no experts. util = tmp_path / "util-only" @@ -447,41 +576,21 @@ class TestFoldExpertSubagents: # ============================================================================= -# is_async_dispatch_available / list_dispatchable_experts honest surface +# list_dispatchable_experts honest surface # ============================================================================= -class TestIsAsyncDispatchAvailable: - """The gate ``list_dispatchable_experts`` and ``ActiveTeamMiddleware`` - both consult to decide whether async-declared experts can be surfaced.""" +class TestListDispatchableExpertsSurvivesAsyncOutage: + """``list_dispatchable_experts`` never drops an expert for async reasons. - def test_false_when_flag_disabled(self): - cfg = SimpleNamespace(enable_async_subagents=False) - assert is_async_dispatch_available(cfg=cfg) is False + Under the old per-skill classification an async-declared expert vanished + from every surface whenever ``enable_async_subagents`` was off or + langgraph dev was unreachable — installed, listed in the gallery, and + reachable by nothing. Every expert now keeps its in-turn reach, so an + async outage degrades the reach rather than removing the expert. + """ - def test_false_when_dev_unreachable(self): - cfg = SimpleNamespace(enable_async_subagents=True) - with patch( - "EvoScientist.langgraph_dev.manager.is_async_subagents_available", - return_value=False, - ): - assert is_async_dispatch_available(cfg=cfg) is False - - def test_true_when_both_gates_pass(self): - cfg = SimpleNamespace(enable_async_subagents=True) - with patch( - "EvoScientist.langgraph_dev.manager.is_async_subagents_available", - return_value=True, - ): - assert is_async_dispatch_available(cfg=cfg) is True - - -class TestListDispatchableExpertsAsyncFilter: - """``list_dispatchable_experts`` drops async-declared experts when async - dispatch isn't registered — sync-declared experts pass through, mirroring - honest advertising per the reviewer's ask on PR #391.""" - - def _skill(self, name: str, dispatch: str) -> SkillInfo: + def _skill(self, name: str) -> SkillInfo: return SkillInfo( name=name, description=f"{name} description", @@ -489,23 +598,22 @@ class TestListDispatchableExpertsAsyncFilter: source="builtin", type="expert", role=f"{name} role", - default_dispatch=dispatch, body="persona body\n", ) - def test_async_expert_dropped_when_flag_disabled(self): + def test_experts_survive_async_flag_disabled(self): cfg = SimpleNamespace(enable_async_subagents=False) - skills = [self._skill("idea-brainstorm", "sync"), self._skill("lit", "async")] + skills = [self._skill("idea-brainstorm"), self._skill("lit")] with patch( "EvoScientist.tools.skills_manager.list_expert_skills", return_value=skills, ): result = list_dispatchable_experts(cfg=cfg) - assert [s.name for s in result] == ["idea-brainstorm"] + assert {s.name for s in result} == {"idea-brainstorm", "lit"} - def test_async_expert_dropped_when_dev_unreachable(self): + def test_experts_survive_dev_unreachable(self): cfg = SimpleNamespace(enable_async_subagents=True) - skills = [self._skill("idea-brainstorm", "sync"), self._skill("lit", "async")] + skills = [self._skill("idea-brainstorm"), self._skill("lit")] with ( patch( "EvoScientist.tools.skills_manager.list_expert_skills", @@ -517,20 +625,16 @@ class TestListDispatchableExpertsAsyncFilter: ), ): result = list_dispatchable_experts(cfg=cfg) - assert [s.name for s in result] == ["idea-brainstorm"] + assert {s.name for s in result} == {"idea-brainstorm", "lit"} - def test_async_expert_included_when_registered(self): + def test_empty_actor_definition_still_dropped(self): + """The filters that remain are about broken experts, not reach.""" cfg = SimpleNamespace(enable_async_subagents=True) - skills = [self._skill("idea-brainstorm", "sync"), self._skill("lit", "async")] - with ( - patch( - "EvoScientist.tools.skills_manager.list_expert_skills", - return_value=skills, - ), - patch( - "EvoScientist.langgraph_dev.manager.is_async_subagents_available", - return_value=True, - ), + blank = self._skill("blank") + blank.body = " \n" + with patch( + "EvoScientist.tools.skills_manager.list_expert_skills", + return_value=[self._skill("idea-brainstorm"), blank], ): result = list_dispatchable_experts(cfg=cfg) - assert {s.name for s in result} == {"idea-brainstorm", "lit"} + assert [s.name for s in result] == ["idea-brainstorm"] diff --git a/tests/test_expert_container_async.py b/tests/test_expert_container_async.py index e9006be..c2bc379 100644 --- a/tests/test_expert_container_async.py +++ b/tests/test_expert_container_async.py @@ -133,6 +133,52 @@ class TestComposePrompt: assert composed.startswith("ERROR:") assert "empty SKILL.md body" in composed + def test_agents_md_expert_prompted_from_actor_definition(self): + """AGENTS.md experts are prompted from AGENTS.md, not SKILL.md. + + Both files exist for these skills, so composing from ``.body`` would + silently work — and hand the expert a knowledge document written for + a different reader in place of its persona. + """ + mw = ExpertSkillLoaderMiddleware() + info = _skill_info( + name="paper-review", + role="", + body="# Knowledge\n\nThe 5-aspect checklist.\n", + ) + info.expert_source = "agents_md" + info.agents_body = "## Persona\n\nYou are an adversarial reviewer.\n" + with patch( + "EvoScientist.tools.skills_manager.list_expert_skills", + return_value=[info], + ): + composed = mw._compose_prompt({"skill_name": "paper-review"}) + assert "You are an adversarial reviewer." in composed + assert "5-aspect checklist" not in composed + + def test_empty_actor_definition_names_agents_md_in_error(self): + """The error cue names the file the author has to fix. + + An AGENTS.md expert with a healthy SKILL.md would otherwise be told + its SKILL.md body is empty, sending the author to the wrong file. + """ + mw = ExpertSkillLoaderMiddleware() + info = _skill_info( + name="paper-review", + role="", + body="# Knowledge\n\nPlenty of content here.\n", + ) + info.expert_source = "agents_md" + info.agents_body = " \n" + with patch( + "EvoScientist.tools.skills_manager.list_expert_skills", + return_value=[info], + ): + composed = mw._compose_prompt({"skill_name": "paper-review"}) + assert composed.startswith("ERROR:") + assert "empty AGENTS.md body" in composed + assert "paper-review" in composed + def test_runtime_context_tail_surfaces_skill_name(self): """The tail block re-asserts ``skill_name`` on every model call so the expert knows its own persona name after summarization. Since diff --git a/tests/test_experts_command.py b/tests/test_experts_command.py index 799aa0e..5533ec6 100644 --- a/tests/test_experts_command.py +++ b/tests/test_experts_command.py @@ -50,7 +50,6 @@ class _FakeSkillInfo: name: str description: str = "" role: str = "" - default_dispatch: str = "" type: str = "expert" tags: list[str] = field(default_factory=list) source: str = "builtin" @@ -58,6 +57,11 @@ class _FakeSkillInfo: # ``list_dispatchable_experts``. Tests that specifically want to # exercise the empty-body reject path pass ``body=""``. body: str = "persona" + # Legacy-frontmatter expert by default: ``expert_prompt_body`` reads + # ``body`` for these. Set ``expert_source="agents_md"`` plus + # ``agents_body`` to fake an expert on the current contract. + expert_source: str = "frontmatter" + agents_body: str = "" def _make_ctx(active_teams: list[str] | None = None) -> tuple[CommandContext, _FakeUI]: @@ -83,7 +87,6 @@ class TestExpertsList: _FakeSkillInfo( name="idea-brainstorm", role="Research idea brainstormer", - default_dispatch="sync", ), ], ): @@ -110,7 +113,6 @@ class TestExpertsList: _FakeSkillInfo( name="idea-brainstorm", role="Research idea brainstormer", - default_dispatch="sync", ), ], ): @@ -136,31 +138,27 @@ class TestExpertToggle: ) assert ctx.channel_runtime.active_teams == [] - async def test_async_expert_refused_with_reason_when_async_unavailable(self): - """When an installed expert declares ``default_dispatch: async`` but - async dispatch is unavailable, ``/expert`` must refuse with the specific - reason — not the empty-body / name-collision default — so the user - knows to enable ``enable_async_subagents`` or start langgraph dev. - Reviewer thread on PR #391.""" - ctx, ui = _make_ctx() - async_expert = _FakeSkillInfo( - name="literature-review", default_dispatch="async" - ) + async def test_async_outage_does_not_block_invite(self): + """An async outage must not make an installed expert un-invitable. + + The old per-skill classification refused here whenever + ``enable_async_subagents`` was off or langgraph dev was unreachable. + Every expert now keeps its in-turn reach, so the outage degrades the + reach rather than removing the expert. + """ + ctx, _ui = _make_ctx() with ( patch( "EvoScientist.tools.skills_manager.list_expert_skills", - return_value=[async_expert], + return_value=[_FakeSkillInfo(name="literature-review")], ), patch( - "EvoScientist.subagents.expert_container.is_async_dispatch_available", + "EvoScientist.langgraph_dev.manager.is_async_subagents_available", return_value=False, ), ): await ExpertCommand().execute(ctx, args=["literature-review"]) - assert any("async dispatch is unavailable" in text for text, _ in ui.lines), ( - f"expected honest async-unavailable message, got: {ui.lines}" - ) - assert ctx.channel_runtime.active_teams == [] + assert ctx.channel_runtime.active_teams == ["literature-review"] async def test_invite_adds_to_active_teams(self): ctx, ui = _make_ctx() diff --git a/tests/test_route_async_specs.py b/tests/test_route_async_specs.py index bf6f109..10f8fbb 100644 --- a/tests/test_route_async_specs.py +++ b/tests/test_route_async_specs.py @@ -3,11 +3,11 @@ Covers: - ``_route_async_specs_through_evo_middleware`` splits AsyncSubAgent specs out of the ``subs`` list and folds them into the base middleware. -- ``build_expert_async_subagent_specs`` filters by - ``default_dispatch == "async"`` and respects the async-enable flag + - langgraph dev reachability. -- ``build_expert_subagent_specs`` (sync fold-in) excludes async experts so - a single skill never surfaces twice in the main agent's tool schema. +- ``build_expert_async_subagent_specs`` gives every installed expert a + background reach, gated only on the async-enable flag + langgraph dev + reachability. +- ``build_expert_subagent_specs`` (in-turn fold-in) covers the same + experts, so each name holds both reaches without colliding. """ from __future__ import annotations @@ -23,7 +23,7 @@ from EvoScientist.subagents.expert_container_async import ( from EvoScientist.tools.skills_manager import SkillInfo -def _skill(name: str, dispatch: str) -> SkillInfo: +def _skill(name: str) -> SkillInfo: return SkillInfo( name=name, description=f"{name} description", @@ -31,7 +31,6 @@ def _skill(name: str, dispatch: str) -> SkillInfo: source="builtin", type="expert", role=f"{name} role", - default_dispatch=dispatch, body="body\n", ) @@ -46,7 +45,7 @@ class TestBuildExpertAsyncSubagentSpecs: cfg = SimpleNamespace(enable_async_subagents=False) with patch( "EvoScientist.tools.skills_manager.list_expert_skills", - return_value=[_skill("literature-review", "async")], + return_value=[_skill("literature-review")], ): specs = build_expert_async_subagent_specs(cfg=cfg) assert specs == [] @@ -56,7 +55,7 @@ class TestBuildExpertAsyncSubagentSpecs: with ( patch( "EvoScientist.tools.skills_manager.list_expert_skills", - return_value=[_skill("literature-review", "async")], + return_value=[_skill("literature-review")], ), patch( "EvoScientist.langgraph_dev.manager.is_async_subagents_available", @@ -66,13 +65,13 @@ class TestBuildExpertAsyncSubagentSpecs: specs = build_expert_async_subagent_specs(cfg=cfg) assert specs == [] - def test_filters_by_default_dispatch(self): - """Only ``default_dispatch: async`` skills become AsyncSubAgent specs.""" + def test_every_expert_gets_a_background_reach(self): + """No classification: every installed expert becomes an AsyncSubAgent spec.""" cfg = SimpleNamespace(enable_async_subagents=True, langgraph_dev_port=6174) skills = [ - _skill("idea-brainstorm", "sync"), - _skill("literature-review", "async"), - _skill("panel-expert", "panel"), + _skill("idea-brainstorm"), + _skill("literature-review"), + _skill("panel-expert"), ] with ( patch( @@ -85,11 +84,15 @@ class TestBuildExpertAsyncSubagentSpecs: ), ): specs = build_expert_async_subagent_specs(cfg=cfg) - assert len(specs) == 1 - assert specs[0]["name"] == "literature-review" - assert specs[0]["graph_id"] == "expert-container-async" - assert specs[0]["is_expert"] is True - assert "http://localhost:6174" in specs[0]["url"] + assert {s["name"] for s in specs} == { + "idea-brainstorm", + "literature-review", + "panel-expert", + } + for spec in specs: + assert spec["graph_id"] == "expert-container-async" + assert spec["is_expert"] is True + assert "http://localhost:6174" in spec["url"] def test_empty_body_experts_skipped(self): """Empty-body async experts are filtered out at spec-build time so @@ -98,8 +101,8 @@ class TestBuildExpertAsyncSubagentSpecs: ``expert_container.py::build_expert_subagent_specs``.""" cfg = SimpleNamespace(enable_async_subagents=True, langgraph_dev_port=6174) skills = [ - _skill("literature-review", "async"), # normal body from _skill() - _skill("empty-persona", "async"), + _skill("literature-review"), # normal body from _skill() + _skill("empty-persona"), ] # Second skill has no body — dataclass field default is ``""``, but # helper sets it to "body\n" — override to empty. @@ -117,6 +120,32 @@ class TestBuildExpertAsyncSubagentSpecs: specs = build_expert_async_subagent_specs(cfg=cfg) assert [s["name"] for s in specs] == ["literature-review"] + def test_agents_md_expert_registered_and_gated_on_its_own_file(self): + """AGENTS.md experts reach async dispatch, and their gate is AGENTS.md. + + The empty-persona gate has to follow the skill's contract: a healthy + SKILL.md must not vouch for a skill whose actor definition is blank. + """ + cfg = SimpleNamespace(enable_async_subagents=True, langgraph_dev_port=6174) + healthy = _skill("paper-review") + healthy.expert_source = "agents_md" + healthy.agents_body = "## Persona\n\nYou are an adversarial reviewer.\n" + blank_actor = _skill("blank-actor") + blank_actor.expert_source = "agents_md" + blank_actor.agents_body = " \n" # SKILL.md body is fine; actor isn't + with ( + patch( + "EvoScientist.tools.skills_manager.list_expert_skills", + return_value=[healthy, blank_actor], + ), + patch( + "EvoScientist.langgraph_dev.manager.is_async_subagents_available", + return_value=True, + ), + ): + specs = build_expert_async_subagent_specs(cfg=cfg) + assert [s["name"] for s in specs] == ["paper-review"] + def test_reserved_name_collision_skipped(self, caplog): """A skill named after a yaml async sub-agent (or ``general-purpose``) must skip async-dispatch registration with a warning, not raise. Without @@ -127,8 +156,8 @@ class TestBuildExpertAsyncSubagentSpecs: cfg = SimpleNamespace(enable_async_subagents=True, langgraph_dev_port=6174) skills = [ - _skill("writing-agent", "async"), # collides with yaml async agent - _skill("literature-review", "async"), + _skill("writing-agent"), # collides with yaml async agent + _skill("literature-review"), ] with ( patch( @@ -165,8 +194,8 @@ class TestBuildExpertAsyncSubagentSpecs: cfg = SimpleNamespace(enable_async_subagents=True, langgraph_dev_port=6174) skills = [ - _skill("literature-review", "async"), - _skill("literature-review", "async"), # duplicate name + _skill("literature-review"), + _skill("literature-review"), # duplicate name ] with ( patch( @@ -196,35 +225,35 @@ class TestBuildExpertAsyncSubagentSpecs: # ============================================================================= -# build_expert_subagent_specs (sync side) — must exclude async experts +# build_expert_subagent_specs (in-turn side) — same experts, second reach # ============================================================================= -class TestBuildExpertSubagentSpecsExcludesAsync: - """The sync fold-in must not emit specs for ``default_dispatch: async`` skills. +class TestBuildExpertSubagentSpecsCoversEveryExpert: + """The in-turn fold-in emits a spec for every expert, async ones included. - A skill in both lists would produce two competing tool schemas — one - under ``task(subagent_type='')`` and one under - ``start_async_task(subagent_type='')`` — from the main agent's - perspective. Ambiguous. The partition is: async goes async, everything - else goes sync. + Sharing a name across the two registries is intentional. They land on + different tools with separate schemas (``task`` vs + ``start_async_task``), and deepagents' duplicate-name check is scoped to + the async list alone, so one expert holding both reaches never collides. """ - def test_async_expert_skipped_by_sync_fold_in(self): + def test_every_expert_gets_an_in_turn_reach(self): skills = [ - _skill("idea-brainstorm", "sync"), - _skill("literature-review", "async"), - _skill("panel-expert", "panel"), + _skill("idea-brainstorm"), + _skill("literature-review"), + _skill("panel-expert"), ] with patch( "EvoScientist.tools.skills_manager.list_expert_skills", return_value=skills, ): specs = build_expert_subagent_specs(tool_registry={}) - names = [s["name"] for s in specs] - assert "idea-brainstorm" in names - assert "panel-expert" in names - assert "literature-review" not in names + assert {s["name"] for s in specs} == { + "idea-brainstorm", + "literature-review", + "panel-expert", + } # ============================================================================= @@ -307,7 +336,7 @@ class TestRouteAsyncSpecs: with ( patch( "EvoScientist.tools.skills_manager.list_expert_skills", - return_value=[_skill("literature-review", "async")], + return_value=[_skill("literature-review")], ), patch( "EvoScientist.langgraph_dev.manager.is_async_subagents_available", @@ -370,7 +399,7 @@ class TestRouteAsyncSpecs: with ( patch( "EvoScientist.tools.skills_manager.list_expert_skills", - return_value=[_skill("literature-review", "async")], + return_value=[_skill("literature-review")], ), patch( "EvoScientist.langgraph_dev.manager.is_async_subagents_available", diff --git a/tests/test_skills_manager.py b/tests/test_skills_manager.py index 8ebeb95..07e2ca0 100644 --- a/tests/test_skills_manager.py +++ b/tests/test_skills_manager.py @@ -962,7 +962,6 @@ def _write_expert_skill( 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.""" @@ -978,7 +977,6 @@ role: {role} byline: {byline} capability_tags: {tags_str} avatar_hint: {avatar_hint} -default_dispatch: {default_dispatch} --- # {name} @@ -1000,7 +998,6 @@ class TestParseSkillMdExpertFields: 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( @@ -1010,7 +1007,6 @@ class TestParseSkillMdExpertFields: 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" @@ -1018,7 +1014,6 @@ class TestParseSkillMdExpertFields: 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 @@ -1063,14 +1058,20 @@ role: This should be ignored for r in caplog.records ) - def test_valid_async_default_dispatch(self, tmp_path): - """``default_dispatch: async`` is a recognized value (agent-teams v2 dispatch mode).""" + def test_default_dispatch_frontmatter_is_not_read(self, tmp_path): + """A skill cannot pin its own dispatch shape. + + ``default_dispatch`` used to partition experts into two disjoint + registries, which is how an installed expert could end up reachable + by nothing. Nothing consumes the field now — the orchestrator picks + a reach per task — so it must not reappear on ``SkillInfo``. + """ skill_dir = tmp_path / "async-skill" skill_dir.mkdir() (skill_dir / "SKILL.md").write_text( """--- name: async-skill -description: uses async dispatch +description: declares a dispatch shape the runtime ignores type: expert role: Some role default_dispatch: async @@ -1080,39 +1081,8 @@ default_dispatch: async """ ) result = _parse_skill_md(skill_dir / "SKILL.md") - assert result.default_dispatch == "async" - - def test_invalid_default_dispatch_falls_back_to_empty(self, tmp_path, caplog): - 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 -""" - ) - import logging - - with caplog.at_level( - logging.WARNING, logger="EvoScientist.tools.skills_manager" - ): - result = _parse_skill_md(skill_dir / "SKILL.md") assert result.type == "expert" - assert result.default_dispatch == "" # rejected, not passed through - # A typo like ``asynchronous`` is silently indistinguishable from - # unset without the warning; the log line must name the offending - # value so authors can see why their expert didn't register async. - assert any( - "unrecognized default_dispatch" in rec.message - and "asynchronous" in rec.message - for rec in caplog.records - ) + assert not hasattr(result, "default_dispatch") def test_capability_tags_accepts_comma_string(self, tmp_path): """capability_tags falls back to comma-separated string parsing (like `tags`).""" @@ -1188,6 +1158,198 @@ Body. assert any("invalid frontmatter YAML" in r.message for r in caplog.records) +def _write_actor_skill( + parent: Path, + name: str, + *, + agents_body: str = "## Persona\n\nYou are the test expert.\n\n## Envelope\n\n{}\n", + skill_frontmatter: str = "", + skill_body: str = "# Knowledge\n\nThe portable workflow.\n", +) -> Path: + """Write a skill declaring itself an expert via a sibling AGENTS.md.""" + skill_dir = parent / name + skill_dir.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_text( + f"---\nname: {name}\ndescription: A skill that can also act\n" + f"{skill_frontmatter}---\n\n{skill_body}" + ) + (skill_dir / "AGENTS.md").write_text(agents_body) + return skill_dir + + +class TestAgentsMdExpertContract: + """AGENTS.md presence is the expert declaration; its body is the prompt.""" + + def test_presence_classifies_as_expert(self, tmp_path): + skill_dir = _write_actor_skill(tmp_path, "actor-skill") + result = _parse_skill_md(skill_dir / "SKILL.md") + assert result.type == "expert" + assert result.expert_source == "agents_md" + assert result.agents_body.startswith("## Persona") + # SKILL.md stays pure knowledge and is still cached for in-turn use. + assert "The portable workflow." in result.body + + def test_no_actor_frontmatter_needed(self, tmp_path): + """The decoration fields the contract removed stay empty, not invented.""" + skill_dir = _write_actor_skill(tmp_path, "actor-skill") + result = _parse_skill_md(skill_dir / "SKILL.md") + assert result.role == "" + assert result.byline == "" + assert result.capability_tags == [] + assert result.avatar_hint == "" + + def test_metadata_type_alone_does_not_classify(self, tmp_path): + """``metadata.type: [skill, expert]`` is index-facing only. + + It is a projection of the AGENTS.md declaration for consumers that + can't stat the directory. Reading it in the runtime would create a + second classifier free to drift from the file that actually holds the + persona — a skill would register as an expert with nothing to say. + """ + skill_dir = tmp_path / "index-only" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + """--- +name: index-only +description: Declares expert to the index but ships no actor definition +metadata: + type: [skill, expert] + tags: [core] +--- + +# Knowledge +""" + ) + result = _parse_skill_md(skill_dir / "SKILL.md") + assert result.type == "utility" + assert result.expert_source == "" + # The tags path still reads metadata — only `type` is ignored there. + assert result.tags == ["core"] + + def test_frontmatter_stripped_from_actor_definition(self, tmp_path): + """AGENTS.md carries no frontmatter, but YAML must never reach the prompt.""" + skill_dir = _write_actor_skill( + tmp_path, + "fm-actor", + agents_body="---\nname: ignored\n---\n\n## Persona\n\nBody only.\n", + ) + result = _parse_skill_md(skill_dir / "SKILL.md") + assert result.agents_body == "## Persona\n\nBody only.\n" + + def test_empty_actor_definition_stays_classified(self, tmp_path): + """An empty AGENTS.md is still a declaration — a broken expert. + + Downgrading it to a utility skill would hide the authoring bug; the + registration paths refuse it by name instead. + """ + skill_dir = _write_actor_skill(tmp_path, "blank-actor", agents_body=" \n") + result = _parse_skill_md(skill_dir / "SKILL.md") + assert result.type == "expert" + assert result.expert_source == "agents_md" + assert result.agents_body.strip() == "" + + def test_unreadable_skill_md_keeps_expert_declaration(self, tmp_path): + """SKILL.md and AGENTS.md are separate files with separate failures.""" + skill_dir = tmp_path / "half-broken" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_bytes(b"---\nname: bad\n---\n\xff\xfe") + (skill_dir / "AGENTS.md").write_text("## Persona\n\nStill valid.\n") + result = _parse_skill_md(skill_dir / "SKILL.md") + assert result.description == "(unreadable)" + assert result.type == "expert" + assert result.expert_source == "agents_md" + assert result.agents_body == "## Persona\n\nStill valid.\n" + + def test_agents_md_overrides_legacy_frontmatter(self, tmp_path, caplog): + """A skill mid-migration resolves to one contract, not a blend.""" + import logging + + skill_dir = _write_actor_skill( + tmp_path, + "migrating", + skill_frontmatter=( + "type: expert\nrole: legacy role\nbyline: Legacy\n" + "capability_tags: [legacy]\navatar_hint: legacy-avatar\n" + "default_dispatch: sync\n" + ), + ) + with caplog.at_level( + logging.WARNING, logger="EvoScientist.tools.skills_manager" + ): + result = _parse_skill_md(skill_dir / "SKILL.md") + assert result.expert_source == "agents_md" + # AGENTS.md wins on every axis: the legacy decoration fields must be + # cleared, not merely warned about — otherwise `role` is prepended to + # the AGENTS.md prompt and the gallery chips leak stale frontmatter. + assert result.role == "" + assert result.byline == "" + assert result.capability_tags == [] + assert result.avatar_hint == "" + assert any( + "is ignored and should be removed" in r.message and "migrating" in r.message + for r in caplog.records + ) + + def test_agents_md_expert_name_is_directory_name(self, tmp_path): + """Registry identity for an AGENTS.md expert is the directory name. + + A frontmatter ``name:`` that disagrees with the directory would desync + the dispatch registry key from the skill the orchestrator names. + """ + skill_dir = tmp_path / "real-dir" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + "---\nname: mismatched-name\ndescription: d\n---\n\n# Knowledge\n" + ) + (skill_dir / "AGENTS.md").write_text("## Persona\n\nBody.\n") + result = _parse_skill_md(skill_dir / "SKILL.md") + assert result.name == "real-dir" + + def test_legacy_frontmatter_expert_warns_once(self, tmp_path, caplog): + """The deprecated path keeps working, and says so — once per skill. + + ``_parse_skill_md`` runs on every ``list_skills`` call (agent + construction, /expert completion, GET /api/teams), so a per-parse + warning would bury real diagnostics under repeats. + """ + import logging + + skill_dir = tmp_path / "legacy-expert" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + """--- +name: legacy-expert +description: Declares itself the old way +type: expert +role: legacy role +--- + +# Persona body +""" + ) + with caplog.at_level( + logging.WARNING, logger="EvoScientist.tools.skills_manager" + ): + first = _parse_skill_md(skill_dir / "SKILL.md") + _parse_skill_md(skill_dir / "SKILL.md") + assert first.type == "expert" + assert first.expert_source == "frontmatter" + assert first.role == "legacy role" + deprecations = [r for r in caplog.records if "deprecated" in r.message.lower()] + assert len(deprecations) == 1 + + def test_utility_skill_has_no_expert_source(self, tmp_path): + skill_dir = tmp_path / "plain" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + "---\nname: plain\ndescription: Plain skill\n---\n\n# Body\n" + ) + result = _parse_skill_md(skill_dir / "SKILL.md") + assert result.type == "utility" + assert result.expert_source == "" + assert result.agents_body == "" + + class TestListExpertSkills: """`list_expert_skills()` filters `list_skills()` to `type == 'expert'`.""" @@ -1258,7 +1420,6 @@ class TestSkillManagerToolExpertSurface: byline="Ideation persona", capability_tags=["Iteration", "ELO"], avatar_hint="lightbulb", - default_dispatch="sync", ), SkillInfo( name="util-b", @@ -1361,7 +1522,8 @@ class TestSkillManagerToolExpertSurface: 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 + # No dispatch line: an expert does not pin its own reach. + assert "Default dispatch" not in out def test_info_omits_expert_block_for_utility_skills(self): from EvoScientist.tools.skill_manager import skill_manager @@ -1688,250 +1850,3 @@ class TestSkillsChangedCallback: result = install_skill("/nonexistent/path", str(temp_skills_dir)) assert result["success"] is False assert good == [True] - - -class TestSkillManagerInfo: - """Tests for the skill_manager() tool's action='info' output. - - Guards the sandbox-visible ``Path: /skills/`` shape and the absence - of any host filesystem path in the agent-visible response. Agents burn - turns on ``cd && …`` chains whenever the host path leaks. - """ - - def _make_skill(self, parent, name, description="A skill"): - skill_dir = parent / name - skill_dir.mkdir() - (skill_dir / "SKILL.md").write_text( - f"---\nname: {name}\ndescription: {description}\n---\n" - ) - return skill_dir - - def test_info_reports_virtual_mount_path(self, tmp_path): - """``Path:`` is the sandbox-visible ``/skills/``, not the host path.""" - from EvoScientist.tools.skill_manager import skill_manager - - workspace_dir = tmp_path / "workspace" - workspace_dir.mkdir() - global_dir = tmp_path / "global-empty" - global_dir.mkdir() - self._make_skill(tmp_path, "info-skill") - install_skill(str(tmp_path / "info-skill"), str(workspace_dir)) - - with ( - patch("EvoScientist.paths.USER_SKILLS_DIR", workspace_dir), - patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", global_dir), - ): - result = skill_manager.invoke({"action": "info", "name": "info-skill"}) - - assert "Path: /skills/info-skill" in result - - def test_info_omits_host_path(self, tmp_path): - """No host filesystem path leaks into the response. - - Stronger than a label-only check: catches any future refactor that - keeps the path visible under a different label (``Local:``, - ``Installed at:``, embedded in ``Source: …``). - """ - from EvoScientist.tools.skill_manager import skill_manager - from EvoScientist.tools.skills_manager import get_skill_info - - workspace_dir = tmp_path / "workspace" - workspace_dir.mkdir() - global_dir = tmp_path / "global-empty" - global_dir.mkdir() - self._make_skill(tmp_path, "host-leak-guard") - install_skill(str(tmp_path / "host-leak-guard"), str(workspace_dir)) - - with ( - patch("EvoScientist.paths.USER_SKILLS_DIR", workspace_dir), - patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", global_dir), - ): - info = get_skill_info("host-leak-guard") - result = skill_manager.invoke({"action": "info", "name": "host-leak-guard"}) - - assert str(info.path) not in result - - -class TestSkillManagerInstall: - """Tests for the skill_manager() tool's action='install' output shape. - - Covers both single-install and batch-install returns: - - Single: ``{"success": True, "name": ..., "path": ..., "description": ...}``. - - Batch: ``{"success": ..., "batch": True, "installed": [...], "failed": [...]}`` - with no top-level ``name``, ``path``, ``description``, or ``error``. - """ - - def _make_skill(self, parent, name, description="A skill"): - skill_dir = parent / name - skill_dir.mkdir() - (skill_dir / "SKILL.md").write_text( - f"---\nname: {name}\ndescription: {description}\n---\n" - ) - return skill_dir - - def test_install_single_reports_virtual_mount_path(self, tmp_path): - """Single install: ``Path: /skills/``, no host path.""" - from EvoScientist.tools.skill_manager import skill_manager - - workspace_dir = tmp_path / "workspace" - workspace_dir.mkdir() - global_dir = tmp_path / "global-empty" - global_dir.mkdir() - self._make_skill(tmp_path, "solo-skill") - - with ( - patch("EvoScientist.paths.USER_SKILLS_DIR", workspace_dir), - patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", global_dir), - ): - result = skill_manager.invoke( - {"action": "install", "source": str(tmp_path / "solo-skill")} - ) - - assert "Successfully installed skill: solo-skill" in result - assert "Path: /skills/solo-skill" in result - - def test_install_single_omits_host_path(self, tmp_path): - """Single install: no host filesystem path leaks into the response. - - ``install_skill(source)`` defaults to ``global_install=True``, so the - skill lands under ``GLOBAL_SKILLS_DIR`` rather than ``USER_SKILLS_DIR``. - Checking against a narrower directory (e.g. workspace_dir) would pass - even without the scrub - the check has to cover every path the tool - might resolve to. ``tmp_path`` covers both patched dirs and the source - path used by ``install_skill``. - """ - from EvoScientist.tools.skill_manager import skill_manager - - workspace_dir = tmp_path / "workspace" - workspace_dir.mkdir() - global_dir = tmp_path / "global-empty" - global_dir.mkdir() - self._make_skill(tmp_path, "leak-guard-install") - - with ( - patch("EvoScientist.paths.USER_SKILLS_DIR", workspace_dir), - patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", global_dir), - ): - result = skill_manager.invoke( - {"action": "install", "source": str(tmp_path / "leak-guard-install")} - ) - - assert str(tmp_path) not in result - - def test_install_batch_lists_each_skill_with_virtual_path(self, tmp_path): - """Batch install: one block per installed skill, each with ``Path: /skills/``.""" - from EvoScientist.tools.skill_manager import skill_manager - - workspace_dir = tmp_path / "workspace" - workspace_dir.mkdir() - global_dir = tmp_path / "global-empty" - global_dir.mkdir() - pack = tmp_path / "pack" - pack.mkdir() - self._make_skill(pack, "alpha", description="first") - self._make_skill(pack, "beta", description="second") - - with ( - patch("EvoScientist.paths.USER_SKILLS_DIR", workspace_dir), - patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", global_dir), - ): - result = skill_manager.invoke({"action": "install", "source": str(pack)}) - - assert "Successfully installed skill: alpha" in result - assert "Successfully installed skill: beta" in result - assert "Path: /skills/alpha" in result - assert "Path: /skills/beta" in result - # Same leak guard as ``test_install_single_omits_host_path``: batch - # returns must not surface any host path either. Cover every dir the - # install might resolve to. - assert str(tmp_path) not in result - - def test_install_batch_all_fail_returns_error_list(self, tmp_path): - """Batch install where every skill fails must not KeyError on the - missing top-level ``error`` field. - - Pre-fix behavior: ``result['error']`` crashed because - ``_batch_install_local`` returns ``{"success": False, "batch": True, - "installed": [], "failed": [{"name": ..., "error": ...}]}`` with no - top-level ``error`` key. This test pins the guard. - """ - from EvoScientist.tools.skill_manager import skill_manager - - workspace_dir = tmp_path / "workspace" - workspace_dir.mkdir() - global_dir = tmp_path / "global-empty" - global_dir.mkdir() - - batch_result = { - "success": False, - "batch": True, - "installed": [], - "failed": [ - {"name": "broken-a", "error": "corrupt frontmatter"}, - {"name": "broken-b", "error": "missing SKILL.md"}, - ], - } - - with ( - patch("EvoScientist.paths.USER_SKILLS_DIR", workspace_dir), - patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", global_dir), - patch( - "EvoScientist.tools.skills_manager.install_skill", - return_value=batch_result, - ), - ): - result = skill_manager.invoke( - {"action": "install", "source": "some/source"} - ) - - # No KeyError, and every failure surfaced. - assert "broken-a" in result - assert "corrupt frontmatter" in result - assert "broken-b" in result - assert "missing SKILL.md" in result - - def test_install_batch_partial_fail_surfaces_both(self, tmp_path): - """Batch install with partial failure lists successes AND failures. - - Pre-fix behavior: partial failures were silently dropped; only the - success blocks reached the agent. - """ - from EvoScientist.tools.skill_manager import skill_manager - - workspace_dir = tmp_path / "workspace" - workspace_dir.mkdir() - global_dir = tmp_path / "global-empty" - global_dir.mkdir() - - partial_result = { - "success": True, - "batch": True, - "installed": [ - { - "name": "worked", - "path": str(workspace_dir / "worked"), - "description": "installed cleanly", - }, - ], - "failed": [ - {"name": "broken", "error": "corrupt frontmatter"}, - ], - } - - with ( - patch("EvoScientist.paths.USER_SKILLS_DIR", workspace_dir), - patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", global_dir), - patch( - "EvoScientist.tools.skills_manager.install_skill", - return_value=partial_result, - ), - ): - result = skill_manager.invoke( - {"action": "install", "source": "some/source"} - ) - - assert "Successfully installed skill: worked" in result - - assert "Path: /skills/worked" in result - assert "broken" in result - assert "corrupt frontmatter" in result