feat(skills): accept paths, validate SKILL.md, and bound eager injection (#350) - #363
Open
jrob5756 wants to merge 2 commits into
Open
feat(skills): accept paths, validate SKILL.md, and bound eager injection (#350)#363jrob5756 wants to merge 2 commits into
jrob5756 wants to merge 2 commits into
Conversation
…ion (#350) The skills mechanism shipped in #215 with a one-entry registry: `skills:` accepted exactly one hardcoded name and get_skill_directory raised for anything else, even though the underlying plumbing takes arbitrary paths. A team could not version a skill alongside the workflow that uses it. `skills:` now accepts filesystem paths as well as built-in names. Classification is syntactic - path when it starts with ~ or . or contains a separator, otherwise a built-in name - so a bare `conductor` can never be shadowed by a same-named local directory and resolution never depends on what happens to exist locally. A path may be a single skill directory or a root of them, which expands to every immediate child holding a SKILL.md. Relative paths resolve against the workflow file's directory, the same rule working_dir uses, so a workflow behaves identically from any cwd. Conductor expands roots itself rather than passing them through, because eager injection needs a name per skill and claude-agent-sdk needs a <plugin>:<skill> name; doing it centrally keeps every provider seeing the same set. Paths are trusted input by design - the same file can already run arbitrary shell via `type: script`, so a skill path grants strictly less. A SKILL.md whose YAML frontmatter fails to parse was silently skipped by both the Copilot CLI and Claude Code - no warning, no error, the skill simply absent. The trap is ordinary: a description containing "Triggers: " as an unquoted plain scalar is invalid YAML. Conductor now parses the frontmatter itself (ruamel, not PyYAML), requires a non-empty name and description, and reports the YAML error with the block-scalar fix. This runs inside resolve_skills rather than only in the static validator, because `conductor run` never calls validate_workflow_config. Eager injection was unbounded. Providers without a native skill surface have no progressive disclosure: the whole SKILL.md plus the entire references/ tree is prepended on every call, every retry, and every validator: call. The bundled conductor skill alone is ~117KB (~29K tokens). runtime.skill_injection now bounds it - warn_bytes (64KB) warns, max_bytes (128KB) fails - enforced both in AgentExecutor and statically, measured against the exact string prepended and reported with a per-skill breakdown. The defaults deliberately straddle the bundled skill, so enabling it on claude warns rather than breaking an existing workflow. hermes declared no `skills` capability, defaulting to False, so the validator rejected `skills:` on it while its own execute() docstring described eager injection working. Injection happens upstream in AgentExecutor, so the path was always reachable and the declaration was simply inaccurate; it now declares True and is bounded like claude. Finally, claude-agent-sdk has no bare skill-directory option - a skill is enabled by name through the plugin that ships it - so a path skill outside a Claude Code plugin is unreachable there. That previously surfaced as a runtime ProviderError; it is now refused at validate time, naming the directory and both remedies. Synthesizing a temporary plugin root would also work, but would stack a second unverified CLI assumption on #352's. Discovery of skills already installed in the user's environment is deliberately not included: discovery locations differ per provider, so a single flag would surface different skill sets to different agents inside one run. Split out as #362. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Applies findings from seven review agents on PR #363. Correctness: * `_check_skill_injection_budget` let `SkillManifestError` escape `validate_workflow_config` as a bare traceback. `read_skill_frontmatter` only ever opens `SKILL.md`, so an unreadable `references/*.md` first surfaces inside `load_skill_content` — the one file class in a skill directory whose failure was reported differently from every other. * Introduces `skills/errors.py` with a shared `SkillError` base so a call site that can trigger both resolution and manifest failures has one correct thing to catch, instead of enumerating subclasses from two modules and forgetting one. * `conductor validate` now checks absolute skill paths even without a workflow file path. They need no base directory, so skipping them was wrong — and the warning claiming otherwise was untrue for those entries. * `SkillInjectionConfig` is frozen. Its cross-field validator does not re-fire on attribute assignment even under the enclosing `RuntimeConfig`'s `validate_assignment`, the same Pydantic gotcha `ProviderSettings` records. * `ResolvedSkill` asserts its two documented invariants in `__post_init__`, matching `SkillPlugin`. It is an exported constructor and `name` is interpolated unescaped into the `<skill name="...">` tag. Dead code: * Removes `resolve_skill_directories`, orphaned when its two callers moved to `resolve_skills`. Its own docstring warned about the non-index-parallel result that only exists because it discards the name and source the new API returns. * Removes `render_prompt`'s `event_callback` parameter. No caller passed it, and wiring it at the sole call site — the validator's re-render — would duplicate a warning `execute()` has already emitted. Simplification: * `_resolve_path_entry` returns just the directory list; its first tuple element was discarded by the only caller. * `resolve_skills` drops a redundant `by_directory` set now that the `ResolvedSkill` name/basename invariant makes the name map sufficient, putting the dedup and clash cases adjacent. * `_enforce_injection_budget` hoists two thrice-recomputed values. Documentation: * The path-classification rule was stated six ways across code and docs; three disagreed with the implementation on absolute paths. All six now match `is_path_entry`. * Fixes a stale `#352` changelog entry claiming injection is re-paid on every validator pass (it is not — validator calls bypass prompt rendering) and quoting a superseded token count. * Corrects hatchling `artifacts` to `force-include`, a precedent citation that described the opposite of what the cited function does, and a comment asserting a message convention the code does not follow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jrob5756
marked this pull request as ready for review
August 3, 2026 23:34
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.
Closes #350.
What this does
#215 shipped the skills mechanism with a one-entry registry.
skills:accepted exactly one hardcoded name andget_skill_directoryraised for anything else, even though the underlying plumbing already took arbitrary paths. This generalizes it and fixes two silent-failure modes found along the way.Four of the five deliverables in the issue. Discovery is deliberately deferred — see below.
1.
skills:accepts filesystem paths~/.or contains a separator, otherwise a built-in name. So a bareconductorcan never be shadowed by a same-named local directory, and resolution never depends on what happens to exist locally.SKILL.md) or a root of them, expanding to every immediate child holding one (not recursive).working_diruses, so a workflow behaves identically from any cwd.normpath, notresolve(), so symlink aliases stay distinct.claude-agent-sdkneeds a<plugin>:<skill>name. Doing it centrally keeps every provider seeing the same set.type: scriptrunning arbitrary shell, so a skill path grants strictly less. Documented as trusted input.AgentDef.validate_skillsstill checks bare names eagerly at load time (they need no base directory), so error timing for the common typo case does not regress. Only paths defer.2. A malformed
SKILL.mdno longer fails silentlyBoth the Copilot CLI and Claude Code skip an unparseable skill without a word. The trap is ordinary and cost the issue author several debugging rounds:
New
skills/frontmatter.pyparses it with ruamel.yaml (the project's YAML library — PyYAML is not a dependency), requires a non-emptynameanddescription, and reports the underlying error with thedescription: |fix spelled out.Enforced inside
resolve_skills, not only in the static validator, becauseconductor runnever callsvalidate_workflow_config.3.
runtime.skill_injectionbounds eager injectionProviders without a native skill surface have no progressive disclosure:
AgentExecutorprepends each skill'sSKILL.mdplus its entirereferences/tree on every call, every retry, and everyvalidator:call — previously with no ceiling at all. The bundledconductorskill alone is 117,375 bytes (~29K tokens).Enforced both at runtime and statically in
conductor validate, measured against the exact string prepended, reported with a per-skill breakdown naming the offender. The defaults deliberately straddle the bundled skill soskills: [conductor]onclaude— which works today — warns rather than breaking. Awarn_bytesabovemax_bytesis rejected as unreachable. Native providers are exempt.4.
hermesdeclaresskills=TrueIt omitted the field, defaulting to
False, so the validator rejectedskills:on it — while its ownexecute()docstring described eager injection working. Injection happens upstream inAgentExecutor, so the path was always reachable and the declaration was just inaccurate. Per AGENTS.md ("declareFalseonly when neither path can work"),Trueis correct. Now bounded by the budget above.Also:
claude-agent-sdkrefuses non-plugin skills at validate timeThat SDK exposes no bare skill-directory option — only plugin roots plus skill names (verified against the installed package; the only relevant options are
plugins,skills,setting_sources,add_dirs,agents). A path skill outside a Claude Code plugin is therefore unreachable there, thoughcopilotloads the identical skill fine. It previously surfaced as a runtimeProviderError; it now fails before the run starts, naming the directory and both remedies.Judgement call worth flagging: synthesizing a temporary plugin root would make the headline "team skill in the repo" use case work on that provider too. I chose not to — it stacks a second unverified CLI assumption on top of #352's unverified qualified-name assumption. Consequence: that use case is
copilot-only for now (the default provider).Not included: discovery
The issue's own "Update" section argues the
discover_skillsdesign does not generalize — Copilot discovers from~/.copilot/…, Claude Code from~/.claude/…, so one boolean would surface different skill sets to different agents inside the same run. Split out as #362, which carries the rationale, the measured 253KB/63K-token cost, and theenable_config_discoverywarning.Verification
uv sync --group dev, no extras); 4897 / 24 with theclaude-agent-sdkextra.make checkclean,make validate-examplesexit 0.workflow_dir; engine dropsskill_injection; resolution skips frontmatter validation; executor skips the budget;hermesreverts toskills=False; validator skips the plugin check.workflow_dir— because tests that build anAgentExecutordirectly hand it that argument themselves. That gap is whytests/test_skills/test_engine_integration.pyexists.code-reviewagent: no significant issues. Two things it surfaced and I fixed:references/docs. Prose corrected; the real invariant (warn < size < max) is now pinned by a test rather than by prose that rots.EACCESfromPath.is_file(), so an unreadable skill directory leaked a barePermissionError._resolve_path_entrynow catchesOSErroraround the whole probe.Docs
docs/workflow-syntax.mdgains its first Skills section (it had zero mentions). Also updated:AGENTS.md,CHANGELOG.md,docs/providers/comparison.md, the example workflow, and the bundled skill's ownauthoring.md/yaml-schema.md— including a stale "mcp_serversignored by claude-agent-sdk" line that #335 had already made false.