diff --git a/src/claude_agent_sdk/_internal/transport/subprocess_cli.py b/src/claude_agent_sdk/_internal/transport/subprocess_cli.py index 833cba4c..254bb454 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,10 +208,16 @@ 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: @@ -213,10 +228,17 @@ def _apply_skills_defaults( if pattern not in allowed_tools: allowed_tools.append(pattern) + # 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"] - return allowed_tools, setting_sources + return allowed_tools, setting_sources, tools def _build_command(self) -> list[str]: """Build CLI command with arguments.""" @@ -237,22 +259,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..66bca21b 100644 --- a/tests/test_transport.py +++ b/tests/test_transport.py @@ -627,6 +627,71 @@ 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 + 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" + + 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() + 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): + """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() + 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.""" + transport = SubprocessCLITransport( + prompt="test", + options=make_options(tools=["Read"]), + ) + cmd = transport._build_command() + 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.""" + 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"), [