fix(claude-agent-sdk): load skills natively and stop inheriting ambient ones - #354
Conversation
|
Addressed review feedback in d6a70ed. Seven review passes; no blocking bugs, but several findings converged on one theme: the first pass was loud about the single case where it couldn't build a skill name, and silent about every case where the name it built was wrong — the same failure mode #352 exists to fix. Resolution now refuses, with the real reason attached
Test gap that mattered most The executor→provider seam had zero coverage. Four mutations there — including
Docs corrections — several claims were wrong, including mine:
A confirmation pass then caught one bug introduced by the fixes: Verification: 4760 passed with the Still draft — the qualified |
…nt ones The provider declared supports_native_skills=False on the grounds that the upstream SDK had no skill surface. That is out of date, and expensive: the full SKILL.md plus the entire references/ tree was injected into every rendered prompt, on every call, retry, and validator pass. It also never set setting_sources, so the claude CLI discovered and enabled skills from ~/.claude/skills, every .claude/skills up the directory tree, and enabled plugins - none declared by the workflow, all varying by machine and launch directory. Conductor documents `skills: []` as an explicit opt-out; on this provider it opted out of nothing. - Register the Claude Code plugin owning each skill via ClaudeAgentOptions.plugins and enable it by its <plugin>:<skill> name, so the CLI reads only the frontmatter up front and loads the body on demand. - Set setting_sources=[] unconditionally, the skills counterpart to the unconditional strict_mcp_config. Note skills=[] and skills=None are not interchangeable upstream: None means "CLI defaults apply", so only the explicit empty list makes the opt-out real. - Grant back the single Skill tool for an explicit `tools: []` when skills are enabled, since an empty base tool set would otherwise leave the declared skill unreachable. - Ship plugins/conductor/.claude-plugin/ in the wheel. Only the skill body was packaged, so a non-editable install resolved a plugin root the CLI could not load. Behaviour change: agents on this provider no longer inherit ambient CLAUDE.md, .claude/rules/*.md, project settings.json, or hooks. Use --workspace-instructions to supply that content explicitly. Tests assert the actual argv the SDK builds across all four skills x tools combinations rather than stopping at the options object. Closes #352 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…hing Review follow-up. The first pass was loud about the one case where it could not build a skill name, and silent about every case where the name it built was wrong - which is the same failure #352 exists to fix. Resolution now refuses, with the real reason attached: - A plugin manifest found above a skill no longer makes that plugin its owner; the skill must live under the candidate's skills/ directory, or the walk continues. - SKILL.md must exist and its frontmatter name must equal the directory name. The CLI resolves enabled skills by frontmatter name while we send the directory name, so drift hides the skill rather than failing. - Names are restricted to [A-Za-z0-9_.-]+. They are joined into a comma-delimited --allowedTools value, where a ',' or ':' would split into extra permission rules. - Two plugins claiming one qualified name are refused rather than deduped, since deduping drops a declared skill. - resolve_skill_plugin returns None only for "no owning plugin root" and raises SkillPluginError when a plugin is present but unusable, so the provider stops reporting a missing manifest for a broken one. - Every resulting ProviderError sets is_retryable=False, matching the sibling config errors: the message interpolates a filesystem path, and the default heuristic sniffs messages for "connection" / "timeout". Tests: the executor -> provider seam had no coverage, so four mutations in it (including skill_directories=None) suppressed every skill with the suite green. That seam and the frontmatter/directory pin are now asserted; both were verified to fail under mutation. Docs: several claims were wrong. A missing wheel manifest fails loudly rather than silently resolving to nothing; the skills: [] opt-out needs setting_sources=[] *and* an explicit skills list, because the SDK re-defaults setting_sources when skills is set without it; and the skill list is a context filter, not a sandbox. Also updates the user-facing provider docs, which had no mention of skills or the settings isolation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ls change Rebasing onto #348 left three statements that were true when written and are not any more. Each told a reader that cwd drags the CLI's ambient instructions, settings, and hooks in, which the unconditional setting_sources=[] now prevents -- and AGENTS.md said outright that Conductor never sets setting_sources, three lines above the bullet explaining why it always does. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
d6a70ed to
3c82f44
Compare
|
Rebased onto The two changes are independent — Worth flagging one thing the rebase surfaced that a clean auto-merge would have hidden: #348 documented, correctly at the time, that cwd drags the CLI's ambient The two features compose rather than collide: Verification on the rebased tip: 4773 passed with the Marking ready for review. |
Closes #352
What
claude_agent_sdk.pydeclaredsupports_native_skills=Falseon the grounds that the upstream SDK had no skill surface. That is out of date, and expensive: the fullSKILL.mdplus the entirereferences/tree was injected into every rendered prompt, on every call, every retry, and every validator pass (~27K tokens for the bundledconductorskill).It also never set
setting_sources, so theclaudeCLI discovered and enabled skills from~/.claude/skills/, every.claude/skills/up the directory tree, and enabled plugins — none of which the workflow declared, all of which varied by developer machine and launch directory. Conductor documentsskills: []as an explicit opt-out; on this provider it opted out of nothing.Same class of hole
strict_mcp_config=Truealready closes for MCP servers, a few lines away in the same file.How
ClaudeAgentOptions.pluginsand the skill enabled by its<plugin>:<skill>name, so the CLI reads only theSKILL.mdfrontmatter up front and loads the body on demand._resolve_skill_pluginsmaps each resolved skill directory back to its plugin (skills/registry.py::resolve_skill_pluginwalks up for.claude-plugin/plugin.json); a directory with no manifest raisesProviderErrorrather than being silently dropped.setting_sources=[]unconditionally — the skills counterpart to the unconditionalstrict_mcp_config. Noteskills=[]andskills=Noneare not interchangeable upstream:Nonemeans "CLI defaults apply", so only the explicit empty list makes the opt-out real.tools: []+ skills now grants back the singleSkilltool. An explicittools: []sends--tools ""(empty base tool set), which would otherwise leave the declared skill unreachable. No permission bypass needed — the SDK auto-allows it viaSkill(<name>)inallowed_tools.force-includeshipped only the skill body, notplugins/conductor/.claude-plugin/. Without the manifest, a non-editable install resolved a plugin root the CLI cannot load. Verified by building the wheel and resolving the skill from an actual wheel install.Agents on this provider no longer inherit ambient
CLAUDE.md,.claude/rules/*.md, projectsettings.json, or hooks. Use--workspace-instructions/--instructionsto supply that content explicitly. Called out in the CHANGELOG.Verification
The SDK surface was checked against the real installed package at the pin floor
0.2.82as well as current0.2.128—skills,setting_sources, andpluginsall exist at both, so no pin bump is needed.Tests assert the actual argv the SDK builds rather than stopping at the options object, across all four skills x tools combinations:
tools: []--tools Skill --allowedTools Skill(conductor:conductor) --setting-sources= --plugin-dir <root>--tools default --allowedTools Skill(conductor:conductor) --setting-sources= --plugin-dir <root>tools: []--tools "" --setting-sources=(no--plugin-dir)--tools default --setting-sources=(no--plugin-dir)claude-agent-sdkextra (ci.yml:109)make check(ruff + ty, CI config)make validate-examplesconductor:conductorresolvesReview notes
conductor:conductoror the bareconductor. We send the documented plugin-qualified form (and the repo's own local-dev flow isclaude --plugin-dir plugins/conductor). It is a one-line change in_resolve_skill_pluginsif a live run says otherwise.claudeCLI before merging.references/yaml-schema.mdstill saysmcp_serversis "ignored by claude-agent-sdk", which claude-agent-sdk provider: implement MCP server translation (mcp_tools=False is a gap, not an SDK limit) #335 made false. Worth a follow-up.