fix(skills): enforce capabilities.skills in conductor validate - #351
Merged
Conversation
The ProviderCapabilities.skills docstring promised that "workflows that declare runtime.skills or per-agent skills: against a provider with skills=False fail validation", but config/validator.py never checked the flag. Every other capability has a cross-check; skills did not. Verified before the fix: a workflow setting both runtime.skills and a per-agent skills: on the aca provider (which declares skills=False) validated cleanly, then silently dropped the skill content at run time — exactly the regression the descriptor exists to prevent. Any third-party provider omitting skills=True inherits the safe-looking False default and hits the same silent drop. - Per-agent check in _check_agent_capabilities, so top-level agents and for_each inline agents are both covered (the inline path already routes through this helper). An empty list is an explicit opt-out, so only a non-empty skills list errors. - Workflow-level check over all_llm_agents for inherited runtime.skills, skipping agents whose own skills: (including []) overrides it. - AGENTS.md: state that skills is not an allowed experimental carve-out, since eager preamble injection is provider-agnostic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #215 (merged). Closes the validator gap flagged in review there.
Problem
ProviderCapabilities.skillsdocuments this contract:That check was never implemented. Every other capability has a cross-check in
config/validator.py—mcp_tools,workflow_tools_passthrough,reasoning_effort,max_session_seconds,working_dir,concurrent_safe— butskillshad none.Verified on
mainbefore this change, using theacaprovider (which declaresskills=Falsebecause skill directories are host paths its in-sandbox runner cannot read):It passed, then silently dropped the skill content at run time — precisely the regression the descriptor exists to prevent. The blast radius is wider than
aca: any third-party or future provider that omitsskills=Trueinherits the safe-lookingFalsedefault and hits the same silent drop.After this change:
Changes
_check_agent_capabilities. This covers top-level agents andfor_eachinline agents, since the inline path already routes through the same helper (Extend per-agent capability validation to for_each inline agents #270) — no separate for-each branch needed. An empty list is an explicit opt-out, so only a non-emptyskills:errors.all_llm_agentsfor inheritedruntime.skills, skipping any agent whose ownskills:(including[]) overrides it. Mirrors the existingmax_session_seconds/working_dirinheritance checks.skillsis not an allowed experimental carve-out. A provider reachesskills=Trueeither natively (supports_native_skills=True) or throughAgentExecutor's eager preamble injection, which is provider-agnostic — soFalseis only correct when neither path can work.Tests
11 new tests in
tests/test_config/test_validator_capabilities.py:skills:against askills=Falseprovider → errorruntime.skillsagainst askills=Falseprovider → errorskills: []opt-out → no error, both standalone and overriding a runtime defaultfor_eachinline agent, both explicit and inheritedAcaRuntimeProviderdescriptor, so a future accidental widening of its capabilities is caught here rather than passing silently_caps()gainsskills: Truein its defaults so the helper keeps describing a fully-capable stable provider.Full suite: 4598 passed, 27 skipped.
make checkclean.Related
capabilities.skillsand the descriptor text this enforces