fix(skills): inject Skill into --tools when explicit tools= is set (#977)#985
fix(skills): inject Skill into --tools when explicit tools= is set (#977)#985Zeffut wants to merge 2 commits into
Conversation
) When users pass an explicit tools=[...] list alongside skills="all", the CLI's --tools argument did not include Skill, so the Skill tool was authorized but never loaded. Centralizes the injection inside _apply_skills_defaults via an effective_tools return value used by _build_command. Idempotent, no-op when skills is unset. Adds 5 regression tests. mypy + ruff clean. 744 passed. Alternative approach to #979. Closes #977. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a regression fix for skills + explicit tools handling in SubprocessCLITransport so that selecting skills ensures the base --tools includes Skill, and extends the test suite to prevent regressions.
Changes:
- Update skills defaulting logic to optionally inject
Skillinto an explicit list-formtoolsoption. - Adjust
_build_command()to use the skills-adjustedtoolslist when building the--toolsflag. - Add regression tests covering injection behavior, idempotency, and immutability.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_transport.py | Adds regression tests ensuring Skill is added to --tools when skills are enabled with explicit tools, without mutating options. |
| src/claude_agent_sdk/_internal/transport/subprocess_cli.py | Extends _apply_skills_defaults() to also compute effective tools, and wires it into _build_command(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if tools is not None and "Skill" not in tools: | ||
| tools.append("Skill") | ||
| else: | ||
| for name in skills: | ||
| pattern = f"Skill({name})" | ||
| if pattern not in allowed_tools: | ||
| allowed_tools.append(pattern) | ||
| # The CLI's --tools flag matches the bare tool name; allowed_tools | ||
| # supplies the per-skill filtering. Ensure Skill is loadable. | ||
| if tools is not None and "Skill" not in tools: | ||
| tools.append("Skill") | ||
|
|
There was a problem hiding this comment.
Good catch — addressed in 3bbee1d. The bare-Skill injection now lives in a single statement after the if/else, so the two branches stay in sync by construction. Test suite still green.
| assert "--tools" in cmd | ||
| assert cmd[cmd.index("--tools") + 1] == "WebSearch,WebFetch,Read,Skill" |
There was a problem hiding this comment.
Agreed. In 3bbee1d the four new regression tests now split the --tools argument on ',' and assert against the resulting list, so they no longer depend on exact comma-joined formatting. Order is still asserted because the injection deterministically appends Skill at the end, which is the contract under test.
Address Copilot review feedback on #985: - Pull the ``Skill not in tools`` check out of the ``"all"`` / list branches in ``_apply_skills_defaults`` into a single post-branch statement. The two branches were always doing the same injection, so consolidating removes duplication and the risk of the branches drifting apart in the future. - Update the four new regression tests to parse ``--tools`` on ``,`` and assert against the resulting list rather than the exact comma-joined string. Ordering is still verified, but the tests no longer depend on the exact formatting of the joined argument. No behavior change.
|
Closing — this PR was opened as part of an automated triage experiment and does not reflect a real user-reported issue I personally validated. Apologies for the noise. |
Summary
Closes #977.
When users pass an explicit
tools=[...]list alongsideskills="all"(orskills=["pdf", ...]), the CLI receives--tools <user_list>and--allowedTools Skill. The CLI loads only the tools listed in--tools, soSkillis authorized but never loaded — the model cannot invoke any skill. This contradicts the documented behavior ofallowed_toolsas the single configuration knob for skills.Root cause
SubprocessCLITransport._apply_skills_defaultsinsrc/claude_agent_sdk/_internal/transport/subprocess_cli.pyinjectsSkillintoallowed_toolsbut never touches the basetoolslist._build_commandemits--toolsfromself._options.toolsdirectly. The two paths never meet.Fix
_apply_skills_defaultsnow returns a third value,effective_tools, which mirrors the caller'stoolslist withSkillappended whenskillsis set. Idempotent; only whentoolsis an explicit list — preset objects andNoneare left untouched. The original options object is not mutated._build_commandemits--toolsfrom this effective list.Behavior matrix
toolsskills--toolsemittedNonedefault(unchanged)[]""(unchanged)["Read"]NoneRead(unchanged)["Read"]"all"Read,Skill(FIXED)["Read"]["pdf"]Read,Skill(FIXED;allowed_toolsisSkill(pdf))["Read","Skill"]"all"Read,Skill(idempotent)Files modified
src/claude_agent_sdk/_internal/transport/subprocess_cli.py—_apply_skills_defaultsnow also returns effectivetools;_build_commanduses it.tests/test_transport.py— 5 new regression tests covering injection, idempotency, no-skills passthrough, and non-mutation.Verification
Python 3.12 venv,
pip install -e ".[dev]":python -m pytest tests/ --ignore=tests/test_build_wheel.py-> 744 passed, 5 skipped (5 new tests; baseline 739 passed).python -m mypy src/-> Success: no issues found in 24 source files.python -m ruff check src/ tests/-> All checks passed!Relation to #979
This is an alternative approach to #979, which is also open and targets the same bug. #979 injects in
_build_commanditself; this PR centralizes all skills-related logic in_apply_skills_defaults(the same helper that already handlesallowed_toolsandsetting_sources), which is easier to reason about and test in isolation. Happy to defer to whichever approach maintainers prefer.Test plan
pytest tests/ --ignore=tests/test_build_wheel.pygreen (744 passed)mypy src/cleanruff check src/ tests/cleantests/test_transport.py