feat(skills): discover installed user, project, and plugin skills (#362) - #369
Merged
Merged
Conversation
`runtime.skill_discovery` picks up skills already installed on the machine
so a workflow can use a personal or team skill library without enumerating
it. Off by default.
runtime:
skill_discovery:
sources: [personal, project, plugins]
exclude: [scratch-notes]
Conductor scans the union of both CLIs' locations itself rather than
enabling each provider's own discovery. That is the substance of the
change, not an implementation detail: discovery locations are
provider-specific, so a per-provider flag would give a `copilot` agent and
a `claude-agent-sdk` agent different skill sets inside a single run.
Scanning centrally also keeps `enable_config_discovery` off on Copilot,
which would otherwise auto-load MCP servers from any `.mcp.json` in the
working directory, and `setting_sources=[]` on claude-agent-sdk.
Every mapped location is a skills root, so all three sources expand through
`expand_skills_root`, extracted from `_resolve_path_entry` — discovery adds
no second opinion about what a skill directory is. Sources scan in a fixed
canonical order (project, personal, plugins) whatever order they are
written in, so reordering cannot change which of two same-named skills
wins.
Discovered skills join the workflow-level default set, leaving the existing
tri-state unchanged: an agent declaring its own `skills:` (including
`skills: []`) overrides discovery too.
The organising principle is a strict/lenient asymmetry — the author wrote
the explicit entries and did not write the discovered ones. Broken
frontmatter, a claimed name, an unreadable directory, or a provider that
cannot deliver it are an error for a declared skill and a warning-plus-skip
for a discovered one. That last case is not theoretical: only 1 of 13
installed Copilot plugins on a real machine ships
`.claude-plugin/plugin.json`, so erroring would bury a claude-agent-sdk
user in failures for content they never wrote.
`claude` and `hermes` refuse discovery at validation time. Measured, the
discovered set is ~260KB (~65K tokens), twice the default `max_bytes`, and
it varies by machine — there is no limit to tune that makes eager injection
safe, so the error names the remedy instead.
`conductor validate` lists what was found, where each skill came from, and
the total size if eagerly injected. An ambient set is the one part of a
workflow the YAML does not capture, so making it inspectable is part of the
feature rather than a debugging aid.
Closes #362
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jason Robert (jrob5756)
marked this pull request as ready for review
August 4, 2026 14:11
Blocking: the refusal of skill discovery on providers with no native
skill surface (`claude`, `hermes`) existed only in `config/validator.py`,
which `conductor run` never calls — so the documented guarantee held for
`conductor validate` and was silently contradicted at run time, eagerly
injecting the whole ambient set into every prompt.
`runtime.skill_injection` was not a backstop: a set under `max_bytes`
passed silently, and one over it reported a budget problem rather than
the real one. Mirrored the check in
`AgentExecutor._reject_discovery_without_native_skills`, the same reason
`_reject_unsupported_skills` already exists. An engine test asserted the
wrong behaviour and now asserts the raise.
Reporting: `conductor validate` scanned rather than resolved, so it
listed skills the run would drop and billed their bytes. It now resolves
and filters to discovered entries. Its warning sink discarded everything
on the claim the validator had already printed the same lines — but the
validator only resolves skills for agents that *inherit*, so a workflow
whose agents all declare their own `skills:` had no reporting path at
all. Diagnostics are now forwarded unless already shown.
Resilience:
- One unreadable child directory discarded every readable sibling in the
same root, and blamed the root. `expand_skills_root` now contains
per-child failures and names them.
- `_to_resolved` built `ResolvedSkill` outside its `try`, and
`__post_init__` raises a `SkillError` — an escape hatch out of the
warn-and-skip path that promises never to raise. A relative `home` or
`base_dir` reproduced it; both are now absolutised at the boundary.
- `_has_repo_marker` swallowed `OSError` silently. When the real repo
root is unreadable the walk collapses to the workflow directory and
drops the repository's own skills, so it now reports.
- "Found no skills" fired after a read failure, contradicting the
warning above it and advising a remedy that would make the problem
permanent. It is suppressed after a failed scan, and distinguishes
"nowhere to look" from "nothing there".
Types: `SkillDiscoveryConfig` was `frozen=True` but its `list` fields
left it mutable via `append` and unhashable despite Pydantic generating
`__hash__`. Now tuples.
Dead code: the `usable` accumulation in the `claude-agent-sdk` branch
was never read — `uses_native_skills("claude-agent-sdk")` is `True`, so
the budget check it fed is unreachable there.
Messages and docs: `_reject_unsupported_skills` reported `skills=[]`,
the documented opt-out, as the cause when discovery alone enabled
skills. The "a provider that cannot load it → warning" row was false for
`claude`/`hermes` and had propagated verbatim into four documents.
Tests: `test_agent_opt_out_escapes_the_refusal` asserted a list was not
`None`; the bundled-examples regression test had started reading the
developer's real home directory. Added coverage for `.git` as a file
(worktrees), unreadable siblings, cache keying across inherit/override,
`aca`, relative paths, and the report's effective-set contract. The
mutation harness grew from 13 to 21 cases, all caught.
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.
Closes #362.
runtime.skill_discoverypicks up skills already installed on the machine, so a workflow can use a personal or team skill library without enumerating each one. Off by default.personal~/.copilot/skills,~/.claude/skillsproject.github/skillsand.claude/skills, from the workflow file's directory up to the repo rootplugins~/.copilot/installed-plugins/*/*/skills,~/.claude/plugins/*/*/skillsWhy Conductor scans instead of each provider
This is the substance of the change, not an implementation detail. #350 deferred discovery precisely because discovery locations are provider-specific — a single
discover_skills: trueflag would surface different skill sets to different agents inside one run, depending on which provider each resolved to.Scanning the union centrally means every agent sees the identical set whatever provider it uses, and all of #350's machinery (frontmatter validation, collision handling, native vs. eager delivery, the injection budget) applies unchanged. It also keeps the providers' own discovery switched off — Copilot's
enable_config_discoverywould additionally auto-load MCP servers from any.mcp.jsonin the working directory, which is the constraint #362 carried over.The organising principle: strict/lenient asymmetry
The author wrote the explicit entries; they did not write the discovered ones. So the same problem is reported differently:
SKILL.mdfrontmatterThat last row is load-bearing rather than theoretical: only 1 of 13 installed Copilot plugins on a real machine ships
.claude-plugin/plugin.json, so erroring would bury aclaude-agent-sdkuser in failures for content they never wrote.Answers to the three questions #362 left open
claudeandhermesrefuse discovery at validation time. Measured, the discovered set is 260,182 bytes ≈ 65K tokens: 2× the defaultmax_bytes, and machine-dependent. There is no limit to tune that makes this safe, so the error names the remedy (enumerate inruntime.skills, or move those agents to a progressive-disclosure provider). Same call Skills: discover user-installed, project, and standalone skills (follow-up to #215) #350 made for non-plugin path skills onclaude-agent-sdk.Other decisions
conductorskill on the machine, andresolve_skillswould otherwise hard-error.skills: []stays the one opt-out. No new per-agent knob.project→personal→plugins) independent of YAML order, so reorderingsources:cannot change which of two same-named skills wins.projectwalk stops at the repo root, and does not walk at all when there is no.git— an unversioned tree must not sweep in whatever sits above it.Visibility
conductor validatelists what was found and what it would cost:An ambient set is the one part of a workflow the YAML does not capture, so making it inspectable is part of the feature.
Implementation notes
skills/discovery.py;expand_skills_rootextracted fromregistry.py::_resolve_path_entryso discovery adds no second opinion about what a skill directory is.resolve_effective_skillsis the single composition point used by bothAgentExecutorandconductor validate, so a run and its validation cannot disagree.ResolvedSkill.discovered: boolcarries provenance, so callers branch on a field rather than sniffing a string.discover_skills(..., home=...)takes the home directory as a parameter specifically so no test reads the developer's real~.Testing
main).AgentExecutorsites (the mutation that escaped review in Skills: discover user-installed, project, and standalone skills (follow-up to #215) #350), the explicit-wins precedence, theclaude/hermesrefusal, the repo-root walk bounds, and the canonical ordering. Registry mode is now covered by a stub-registry test, which is the only way to exercise the second executor site.make checkclean;make validate-examplesexit 0.