diff --git a/AGENTS.md b/AGENTS.md index 79ab7078..7f61dfc2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -385,6 +385,15 @@ descriptor undermines the framework. - `AgentOutput` shape on every successful execution (fields may be `None`). - Raise real exceptions on real errors — no silent failure swallowing. - Declare accurate `ProviderCapabilities` matching observed behavior. +- Declare `skills` accurately. Skills are **not** an allowed carve-out — a + provider reaches `skills=True` either natively (`supports_native_skills=True`, + forwarding `skill_directories` to its SDK) or via `AgentExecutor`'s eager + preamble injection, which is provider-agnostic. Declare `False` only when + neither path can work (e.g. `aca`, where skill directories are host paths the + in-sandbox runner cannot read). `config/validator.py` cross-checks per-agent + `skills:` and inherited `runtime.skills` against this flag, so an inaccurate + `False` turns into a spurious validate error and an inaccurate `True` silently + drops the skill content at run time. - Provide a smoke test that exercises construct + execute paths against a mocked SDK. - Maintain `concurrent_safe: true`, or fail validation when used in diff --git a/src/conductor/config/validator.py b/src/conductor/config/validator.py index 0e244477..2e613406 100644 --- a/src/conductor/config/validator.py +++ b/src/conductor/config/validator.py @@ -1583,6 +1583,7 @@ def _validate_provider_capabilities( runtime_default_effort = config.workflow.runtime.default_reasoning_effort runtime_max_session_seconds = config.workflow.runtime.max_session_seconds runtime_working_dir = config.workflow.runtime.working_dir + runtime_skills = config.workflow.runtime.skills # Cache per provider name so we don't re-resolve for every agent. cache: dict[str, ProviderCapabilities] = {} @@ -1763,6 +1764,18 @@ def _check_agent_capabilities( f"directories (capabilities.working_dir=False)." ) + # skills: a provider that cannot surface skill content would drop it + # silently — the agent still runs, just without the knowledge the + # author asked for. An empty list is an explicit opt-out, so only a + # non-empty list is an error. + if agent.skills and not caps.skills: + errors.append( + f"Agent '{agent.name}' declares skills={agent.skills!r} but provider " + f"'{provider_name}' does not support skills " + f"(capabilities.skills=False). Remove the skills, opt out with " + f"'skills: []', or override the agent to a skill-aware provider." + ) + # All provider-backed agents that run at workflow scope: top-level agents # PLUS for_each inline agents (``ForEachDef.agent``), which inherit the # workflow-level ``mcp_servers`` / ``max_session_seconds`` and run with @@ -1854,6 +1867,34 @@ def _check_agent_capabilities( f"working_dir." ) + # ----- Workflow-level: skills ----- + # A runtime-wide skills list is inherited by every LLM agent that does not + # declare its own (``skills: []`` is an explicit opt-out and counts as an + # override). A provider that cannot surface skill content would drop it + # silently, so error against every resolved provider that actually + # receives the setting. + if runtime_skills: + providers_inheriting_skills: dict[str, list[str]] = {} + for agent in all_llm_agents: + # Any per-agent ``skills:`` — including the empty-list opt-out — + # replaces the runtime default; that case is checked in + # ``_check_agent_capabilities`` instead. + if agent.skills is not None: + continue + pname = _resolved_provider_name(agent, default_provider) + providers_inheriting_skills.setdefault(pname, []).append(agent.name) + for pname, agent_names in providers_inheriting_skills.items(): + pcaps = _caps_for(pname) + if pcaps is not None and not pcaps.skills: + errors.append( + f"Workflow declares 'runtime.skills'={sorted(runtime_skills)!r} " + f"but provider '{pname}' does not support skills " + f"(capabilities.skills=False) and is used by agent(s): " + f"{sorted(agent_names)!r}. Override these agents to a " + f"skill-aware provider, opt out per-agent with 'skills: []', " + f"or remove the workflow-level skills." + ) + # ----- Per-agent checks ----- for agent in config.agents: if not _is_llm_agent(agent): diff --git a/tests/test_config/test_validator_capabilities.py b/tests/test_config/test_validator_capabilities.py index 7e793670..73d6cc64 100644 --- a/tests/test_config/test_validator_capabilities.py +++ b/tests/test_config/test_validator_capabilities.py @@ -37,6 +37,7 @@ def _caps(**overrides: object) -> ProviderCapabilities: "checkpoint_resume": True, "usage_tracking": True, "concurrent_safe": True, + "skills": True, } base.update(overrides) return ProviderCapabilities(**base) # type: ignore[arg-type] @@ -49,10 +50,13 @@ def _build_workflow( for_each: list[ForEachDef] | None = None, mcp_servers: dict[str, MCPServerDef] | None = None, tools: list[str] | None = None, + skills: list[str] | None = None, ) -> WorkflowConfig: runtime_kwargs: dict[str, Any] = {"provider": "copilot"} if mcp_servers is not None: runtime_kwargs["mcp_servers"] = mcp_servers + if skills is not None: + runtime_kwargs["skills"] = skills workflow_kwargs: dict[str, Any] = {} if tools is not None: workflow_kwargs["tools"] = tools @@ -74,6 +78,7 @@ def _for_each_workflow( inline: AgentDef, tools: list[str] | None = None, mcp_servers: dict[str, MCPServerDef] | None = None, + skills: list[str] | None = None, ) -> WorkflowConfig: """Build a workflow whose only for_each group carries ``inline``. @@ -86,6 +91,7 @@ def _for_each_workflow( agents=[AgentDef(name="entry", prompt="hi", tools=[])], tools=tools, mcp_servers=mcp_servers, + skills=skills, for_each=[ ForEachDef( name="loop", @@ -1559,3 +1565,146 @@ def test_non_empty_tools_is_still_rejected(self, patch_caps: Any) -> None: config = self._sdk_workflow(agents=[AgentDef(name="a", prompt="hi", tools=["search"])]) with pytest.raises(ConfigurationError, match="does not honor per-agent tool allowlists"): validate_workflow_config(config) + + +class TestSkillsCrossCheck: + """Requirement: ``skills`` (per-agent or runtime-wide) against a provider + declaring ``skills=False`` is a hard validate error. + + ``capabilities.skills`` promises this cross-check in its docstring, but the + check was missing — a workflow on a skill-blind provider validated cleanly + and then silently dropped the skill content at run time (follow-up to #180). + """ + + def test_agent_skills_against_unsupported_provider_errors(self, patch_caps: Any) -> None: + patch_caps({"copilot": _caps(skills=False)}) + config = _build_workflow( + agents=[AgentDef(name="a", prompt="hi", skills=["conductor"])], + ) + with pytest.raises(ConfigurationError, match="does not support skills"): + validate_workflow_config(config) + + def test_agent_skills_against_supported_provider_passes(self, patch_caps: Any) -> None: + patch_caps({"copilot": _caps(skills=True)}) + config = _build_workflow( + agents=[AgentDef(name="a", prompt="hi", skills=["conductor"])], + ) + validate_workflow_config(config) # no raise + + def test_empty_skills_list_is_opt_out_not_an_error(self, patch_caps: Any) -> None: + """``skills: []`` asks for nothing, so a skill-blind provider is fine.""" + patch_caps({"copilot": _caps(skills=False)}) + config = _build_workflow( + agents=[AgentDef(name="a", prompt="hi", skills=[])], + ) + validate_workflow_config(config) # no raise + + def test_runtime_skills_against_unsupported_provider_errors(self, patch_caps: Any) -> None: + """runtime.skills is inherited by every LLM agent that does not override.""" + patch_caps({"copilot": _caps(skills=False)}) + config = _build_workflow( + agents=[AgentDef(name="a", prompt="hi")], + skills=["conductor"], + ) + with pytest.raises(ConfigurationError, match="runtime.skills"): + validate_workflow_config(config) + + def test_runtime_skills_per_agent_opt_out_passes(self, patch_caps: Any) -> None: + """A per-agent ``skills: []`` overrides the runtime default, so the + skill-blind provider never receives it.""" + patch_caps({"copilot": _caps(skills=False)}) + config = _build_workflow( + agents=[AgentDef(name="a", prompt="hi", skills=[])], + skills=["conductor"], + ) + validate_workflow_config(config) # no raise + + def test_runtime_skills_all_agents_override_to_capable_provider_passes( + self, patch_caps: Any + ) -> None: + patch_caps( + { + "copilot": _caps(skills=False), + "claude": _caps(skills=True), + } + ) + config = _build_workflow( + agents=[AgentDef(name="a", prompt="hi", provider="claude")], + skills=["conductor"], + ) + validate_workflow_config(config) # no raise + + def test_per_agent_provider_override_to_skill_blind_provider_errors( + self, patch_caps: Any + ) -> None: + patch_caps( + { + "copilot": _caps(skills=True), + "claude": _caps(skills=False), + } + ) + config = _build_workflow( + agents=[AgentDef(name="a", prompt="hi", provider="claude", skills=["conductor"])], + ) + with pytest.raises(ConfigurationError, match="does not support skills"): + validate_workflow_config(config) + + def test_for_each_inline_agent_skills_errors(self, patch_caps: Any) -> None: + """A for_each inline agent is not in ``config.agents``; it must still be + cross-checked (#270 pattern).""" + patch_caps({"copilot": _caps(skills=False)}) + config = _for_each_workflow( + inline=AgentDef(name="inline", prompt="hi", tools=[], skills=["conductor"]), + ) + with pytest.raises(ConfigurationError, match="does not support skills"): + validate_workflow_config(config) + + def test_for_each_inline_agent_inherits_runtime_skills_errors(self, patch_caps: Any) -> None: + """The inline agent inherits runtime.skills just like a top-level agent.""" + patch_caps({"copilot": _caps(skills=False)}) + config = _for_each_workflow( + inline=AgentDef(name="inline", prompt="hi", tools=[]), + skills=["conductor"], + ) + with pytest.raises(ConfigurationError, match="runtime.skills"): + validate_workflow_config(config) + + +class TestAcaSkillsRealCapabilities: + """Cross-check against the REAL ``AcaRuntimeProvider`` descriptor, which + declares ``skills=False`` — skill directories are host paths the in-sandbox + runner cannot read. ``aca`` is workflow-level only (no ``AgentDef.provider`` + literal), so it is set via ``runtime.provider``. + """ + + def _aca_workflow(self, *, agents: list[AgentDef], skills: list[str] | None = None): + from conductor.config.schema import ProviderSettings + + runtime_kwargs: dict[str, Any] = { + "provider": ProviderSettings(name="aca", pool_endpoint="https://pool.example.com"), + } + if skills is not None: + runtime_kwargs["skills"] = skills + return WorkflowConfig( + workflow=WorkflowDef( + name="test", + entry_point=agents[0].name, + runtime=RuntimeConfig(**runtime_kwargs), + ), + agents=agents, + ) + + def test_real_aca_rejects_agent_skills(self) -> None: + config = self._aca_workflow( + agents=[AgentDef(name="a", prompt="hi", skills=["conductor"])], + ) + with pytest.raises(ConfigurationError, match="does not support skills"): + validate_workflow_config(config) + + def test_real_aca_rejects_runtime_skills(self) -> None: + config = self._aca_workflow( + agents=[AgentDef(name="a", prompt="hi")], + skills=["conductor"], + ) + with pytest.raises(ConfigurationError, match="runtime.skills"): + validate_workflow_config(config)