From 3bdeac22f2ac747df15fba3c2f83371a6b7ac081 Mon Sep 17 00:00:00 2001 From: Zeffut Date: Sat, 23 May 2026 02:00:50 +0200 Subject: [PATCH 1/2] fix(skills): inject Skill into --tools when explicit tools= is set (#977) 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) --- .../_internal/transport/subprocess_cli.py | 47 ++++++++++---- tests/test_transport.py | 61 +++++++++++++++++++ 2 files changed, 96 insertions(+), 12 deletions(-) diff --git a/src/claude_agent_sdk/_internal/transport/subprocess_cli.py b/src/claude_agent_sdk/_internal/transport/subprocess_cli.py index 833cba4c..1d8a17c7 100644 --- a/src/claude_agent_sdk/_internal/transport/subprocess_cli.py +++ b/src/claude_agent_sdk/_internal/transport/subprocess_cli.py @@ -182,8 +182,8 @@ def _build_settings_value(self) -> str | None: def _apply_skills_defaults( self, - ) -> tuple[list[str], list[str] | None]: - """Compute effective allowed_tools and setting_sources for skills. + ) -> tuple[list[str], list[str] | None, list[str] | None]: + """Compute effective allowed_tools, setting_sources, and tools for skills. When ``options.skills`` is ``"all"``, injects the bare ``Skill`` tool; when it is a list, injects ``Skill(name)`` for each entry. In either @@ -191,6 +191,15 @@ def _apply_skills_defaults( unset so the CLI discovers installed skills without the caller having to wire up both options manually. ``None`` is a no-op. + When the caller provides an explicit ``tools`` list (the base set of + tools the CLI is allowed to load), the bare ``Skill`` tool is also + appended to that list when missing. Without this, ``skills="all"`` + would add ``Skill`` to ``allowed_tools`` but ``Skill`` would still be + absent from the base tool set, so the model could not invoke it + (issue #977). When ``tools`` is ``None`` or a preset object, no + injection into ``tools`` is needed because the default tool set + already includes ``Skill``. + Does not mutate the original options object. """ allowed_tools: list[str] = list(self._options.allowed_tools) @@ -199,24 +208,38 @@ def _apply_skills_defaults( if self._options.setting_sources is not None else None ) + # Only mirror an explicit list-form ``tools`` option here. Preset + # objects and ``None`` are returned as ``None`` so ``_build_command`` + # uses its existing path for them. + tools: list[str] | None = ( + list(self._options.tools) + if isinstance(self._options.tools, list) + else None + ) skills = self._options.skills if skills is None: - return allowed_tools, setting_sources + return allowed_tools, setting_sources, tools if skills == "all": if "Skill" not in allowed_tools: allowed_tools.append("Skill") + 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") if setting_sources is None: setting_sources = ["user", "project"] - return allowed_tools, setting_sources + return allowed_tools, setting_sources, tools def _build_command(self) -> list[str]: """Build CLI command with arguments.""" @@ -237,22 +260,22 @@ def _build_command(self) -> list[str]: ["--append-system-prompt", cast(SystemPromptPreset, sp)["append"]] ) + effective_allowed_tools, effective_setting_sources, effective_tools = ( + self._apply_skills_defaults() + ) + # Handle tools option (base set of tools) if self._options.tools is not None: - tools = self._options.tools - if isinstance(tools, list): - if len(tools) == 0: + if effective_tools is not None: + # List-form tools, possibly with Skill injected by skills option. + if len(effective_tools) == 0: cmd.extend(["--tools", ""]) else: - cmd.extend(["--tools", ",".join(tools)]) + cmd.extend(["--tools", ",".join(effective_tools)]) else: # Preset object - 'claude_code' preset maps to 'default' cmd.extend(["--tools", "default"]) - effective_allowed_tools, effective_setting_sources = ( - self._apply_skills_defaults() - ) - if effective_allowed_tools: cmd.extend(["--allowedTools", ",".join(effective_allowed_tools)]) diff --git a/tests/test_transport.py b/tests/test_transport.py index 1e61e9ad..6af428a6 100644 --- a/tests/test_transport.py +++ b/tests/test_transport.py @@ -627,6 +627,67 @@ def test_build_command_skills_does_not_duplicate_entries(self): cmd = transport._build_command() assert cmd[cmd.index("--allowedTools") + 1] == "Skill(pdf)" + def test_build_command_skills_all_injects_skill_into_explicit_tools(self): + """Regression for #977: skills='all' alongside an explicit tools list must + add Skill to --tools so the model can actually invoke it.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options( + tools=["WebSearch", "WebFetch", "Read"], + skills="all", + ), + ) + cmd = transport._build_command() + assert "--tools" in cmd + assert cmd[cmd.index("--tools") + 1] == "WebSearch,WebFetch,Read,Skill" + # And Skill is in allowedTools too. + assert cmd[cmd.index("--allowedTools") + 1] == "Skill" + + def test_build_command_skills_list_injects_skill_into_explicit_tools(self): + """Named skills list must also add the bare Skill tool to --tools so the + per-name patterns in allowedTools can take effect.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options( + tools=["Read"], + skills=["pdf"], + ), + ) + cmd = transport._build_command() + assert cmd[cmd.index("--tools") + 1] == "Read,Skill" + assert cmd[cmd.index("--allowedTools") + 1] == "Skill(pdf)" + + def test_build_command_skills_tools_injection_is_idempotent(self): + """If the caller already put Skill in tools, do not duplicate it.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options( + tools=["Read", "Skill"], + skills="all", + ), + ) + cmd = transport._build_command() + assert cmd[cmd.index("--tools") + 1] == "Read,Skill" + + def test_build_command_skills_none_does_not_inject_into_tools(self): + """Without skills set, an explicit tools list must be passed through unchanged.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options(tools=["Read"]), + ) + cmd = transport._build_command() + assert cmd[cmd.index("--tools") + 1] == "Read" + + def test_build_command_skills_tools_injection_does_not_mutate_options(self): + """Tools injection must not leak back into the caller's options object.""" + options = make_options( + tools=["Read"], + skills="all", + ) + transport = SubprocessCLITransport(prompt="test", options=options) + transport._build_command() + assert options.tools == ["Read"] + @pytest.mark.parametrize( ("skills", "extra", "want_tools", "want_sources", "want_init_skills"), [ From 3bbee1d816debc79975d151787aaeaf12faf45a3 Mon Sep 17 00:00:00 2001 From: Zeffut Date: Sun, 24 May 2026 21:09:01 +0000 Subject: [PATCH 2/2] refactor(skills): consolidate Skill tools injection and harden tests 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. --- .../_internal/transport/subprocess_cli.py | 17 ++++++++--------- tests/test_transport.py | 12 ++++++++---- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/claude_agent_sdk/_internal/transport/subprocess_cli.py b/src/claude_agent_sdk/_internal/transport/subprocess_cli.py index 1d8a17c7..254bb454 100644 --- a/src/claude_agent_sdk/_internal/transport/subprocess_cli.py +++ b/src/claude_agent_sdk/_internal/transport/subprocess_cli.py @@ -212,9 +212,7 @@ def _apply_skills_defaults( # objects and ``None`` are returned as ``None`` so ``_build_command`` # uses its existing path for them. tools: list[str] | None = ( - list(self._options.tools) - if isinstance(self._options.tools, list) - else None + list(self._options.tools) if isinstance(self._options.tools, list) else None ) skills = self._options.skills @@ -224,17 +222,18 @@ def _apply_skills_defaults( if skills == "all": if "Skill" not in allowed_tools: allowed_tools.append("Skill") - 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") + + # Both branches enable skills, so the bare ``Skill`` tool must be + # loadable when the caller supplied an explicit ``tools`` list. The + # CLI's --tools flag matches bare tool names; ``allowed_tools`` + # supplies any per-skill filtering. + if tools is not None and "Skill" not in tools: + tools.append("Skill") if setting_sources is None: setting_sources = ["user", "project"] diff --git a/tests/test_transport.py b/tests/test_transport.py index 6af428a6..66bca21b 100644 --- a/tests/test_transport.py +++ b/tests/test_transport.py @@ -639,7 +639,8 @@ def test_build_command_skills_all_injects_skill_into_explicit_tools(self): ) cmd = transport._build_command() assert "--tools" in cmd - assert cmd[cmd.index("--tools") + 1] == "WebSearch,WebFetch,Read,Skill" + tools_arg = cmd[cmd.index("--tools") + 1].split(",") + assert tools_arg == ["WebSearch", "WebFetch", "Read", "Skill"] # And Skill is in allowedTools too. assert cmd[cmd.index("--allowedTools") + 1] == "Skill" @@ -654,7 +655,8 @@ def test_build_command_skills_list_injects_skill_into_explicit_tools(self): ), ) cmd = transport._build_command() - assert cmd[cmd.index("--tools") + 1] == "Read,Skill" + tools_arg = cmd[cmd.index("--tools") + 1].split(",") + assert tools_arg == ["Read", "Skill"] assert cmd[cmd.index("--allowedTools") + 1] == "Skill(pdf)" def test_build_command_skills_tools_injection_is_idempotent(self): @@ -667,7 +669,8 @@ def test_build_command_skills_tools_injection_is_idempotent(self): ), ) cmd = transport._build_command() - assert cmd[cmd.index("--tools") + 1] == "Read,Skill" + tools_arg = cmd[cmd.index("--tools") + 1].split(",") + assert tools_arg == ["Read", "Skill"] def test_build_command_skills_none_does_not_inject_into_tools(self): """Without skills set, an explicit tools list must be passed through unchanged.""" @@ -676,7 +679,8 @@ def test_build_command_skills_none_does_not_inject_into_tools(self): options=make_options(tools=["Read"]), ) cmd = transport._build_command() - assert cmd[cmd.index("--tools") + 1] == "Read" + tools_arg = cmd[cmd.index("--tools") + 1].split(",") + assert tools_arg == ["Read"] def test_build_command_skills_tools_injection_does_not_mutate_options(self): """Tools injection must not leak back into the caller's options object."""