fix(transport): validate skill names in ClaudeAgentOptions.skills

[ant-kurt]

Summary

Skill names from ClaudeAgentOptions(skills=[...]) were formatted into the --allowedTools value unchecked. The CLI splits that value into permission rules on commas and spaces outside parentheses, and its tokenizer honors no escape sequences, so a name carrying one of those delimiters cannot be passed through reliably. The transport now validates each name and fails closed.

Rejected: parentheses, commas, control characters (C0, DEL, C1), empty names, and a bare string passed in place of a list; a literal * and wildcard suffixes (:*, *); and shapes that parse but can never match the listed skill — surrounding whitespace, a leading /, consecutive backslashes, a trailing unpaired backslash, and surrogate code points.

Ordinary names are unaffected, including plugin-qualified names, interior spaces, single backslashes, and non-ASCII.

Breaking: skills=["plugin:*"] and skills=["*"] now raise — use skills="all", or a Skill(...) rule in allowed_tools for prefix matching. skills=[" name"] and skills=["/name"] now raise as well; both previously built a rule that could never match, so the skill was silently unavailable.

Test plan

python -m pytest tests/ — 1349 passed, 4 skipped, including new parametrized rejection and acceptance cases in tests/test_transport.py. ruff check, ruff format --check, and mypy src/ are clean. Separately drove ClaudeSDKClient and query() to confirm each rejected shape raises at connect while ordinary names build identical argv to before.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 src/claude_agent_sdk/_internal/transport/subprocess_cli.py:534-542 — All skill-name validation added by this PR lives inside SubprocessCLITransport._apply_skills_defaults, so the public custom-transport entry points (ClaudeSDKClient(transport=...) and query(transport=...)) bypass it entirely — Query.initialize then sends options.skills verbatim as the session skills filter, so skills=[" pdf"] or ["/pdf"] silently hides every skill and skills="pdf" is silently dropped (fail-open), contradicting the new types.py docstring promise that invalid names "raise at connect()". Consider hoisting the transport-independent checks (bare string, non-string entries, empty, surrounding whitespace, leading /, surrogates, control chars) into Query or the shared client layer so every transport fails closed.

  Extended reasoning...

  What the bug is

  This PR adds _reject_bare_string_skills and _validate_skill_name, but both are private helpers of SubprocessCLITransport, invoked only from _build_command() → _apply_skills_defaults() (src/claude_agent_sdk/_internal/transport/subprocess_cli.py:534,541). Skill names have a second, SDK-owned consumer that is reachable without ever constructing that transport: Query.initialize sends request["skills"] = self._skills verbatim (src/claude_agent_sdk/_internal/query.py:237-240) whenever the value is a list — with no validation of any kind.

  The code path that triggers it

  Both public custom-transport entry points route around the new guards entirely:

  • ClaudeSDKClient(transport=...): client.py:188-189 uses the caller's transport directly (no SubprocessCLITransport is built), then client.py:240 passes skills=self.options.skills into Query.
  • query(transport=...): _internal/client.py:128-129 does the same, then line 185 passes skills=configured_options.skills.

  A custom Transport is a public, documented constructor parameter (e.g. a transport tunneling the same CLI over SSH or WebSocket), so this is a supported path, not an internal one.

  Step-by-step proof

  1. A user constructs ClaudeSDKClient(transport=my_transport, options=ClaudeAgentOptions(skills=[" pdf"])).
  2. connect() takes the self._custom_transport branch — SubprocessCLITransport._build_command() never runs, so neither _reject_bare_string_skills nor _validate_skill_name executes. Nothing raises.
  3. Query.initialize sees isinstance([" pdf"], list) is true and sends request["skills"] = [" pdf"] to the CLI.
  4. The CLI installs the session skills filter and matches it verbatim against discovered skill names (per this PR's own leading-slash rationale: "The session allowlist matches verbatim"). No discovered name has leading whitespace, so the filter matches nothing — every skill is silently hidden.

  The same holds for ["/pdf"] and [42] (sent as-is), while skills="pdf" fails the isinstance(..., list) gate and is silently dropped from initialize — fail-open, no filter installed at all.

  Why this matters

  Step 4 is exactly the silently-dead-config failure mode this PR sets out to eliminate, and it contradicts the unconditional promise the PR adds to the ClaudeAgentOptions.skills docstring in types.py: "Names must be exact: wildcards, delimiters, and surrounding whitespace raise at connect()." That promise only holds for the default subprocess transport. Note this is distinct from the existing bot comment about widening _reject_bare_string_skills for tuple/set/generator: that fix lives inside the transport and never executes on the custom-transport path — the root cause here is validation placement (coupled to one Transport implementation), not guard breadth. The dead-rule checks (whitespace, leading /, surrogates) are justified by session-filter verbatim matching, which happens in SDK-owned Query.initialize regardless of transport — so scoping them to one transport is not an intentional design boundary.

  How to fix

  Hoist the transport-independent subset of the checks — bare string, non-string entries, empty names, surrounding whitespace, leading /, surrogates, control characters — into Query.__init__/initialize or the two shared client call sites, so every transport fails closed as documented. The subprocess transport can keep the --allowedTools-tokenizer-specific checks (parentheses, commas, backslash rules, wildcard suffixes) as defense in depth.

  Why nit rather than blocking

  Custom transports are a niche path, the initialize skills field is structured JSON (no injection surface), and the outcome for these inputs is identical to pre-PR behavior — silent unavailability, not a regression. This is a completeness/doc-contract gap in the new validation net, not a defect that breaks anything that previously worked.

[claude]

Warning

This review may be incomplete: some analysis steps could not run due to a temporary API capacity limit.

I didn't find any new bugs in this revision — my two earlier comments (non-list skills values slipping past the bare-string guard, and the U+FEFF trim divergence) are both addressed: _reject_non_list_skills now rejects tuple/set/generator values to mirror the isinstance(..., list) gate in query.py, and U+FEFF is folded into _SKILL_NAME_INVALID_CHARS, each with new parametrized tests. The "all" sentinel is also now a shared constant per the earlier review suggestion. I'm leaving the final sign-off to a human because the PR intentionally changes public behavior (skills=["*"], plugin:*, and padded names now raise) and constructs permission rules.

What was reviewed:

• Every rejection branch in _validate_skill_name against the new tests — each check only narrows what reaches --allowedTools; accepted names build argv identical to before.
• Cross-checked the transport guard against Query.initialize's skills gating so both consumers of options.skills now agree.
• Checked _SKILLS_ALL usages in types.py (shadow-warning path) and the transport for consistency.

Extended reasoning...

The PR adds fail-closed validation of ClaudeAgentOptions.skills in the subprocess transport before names are formatted into --allowedTools, plus a shared _SKILLS_ALL constant and ~200 lines of parametrized tests. All three pieces of prior review feedback (two bot nits, one human suggestion) are resolved in this revision, and the bug hunting system found no new issues.

The change is strictly narrowing: every new code path either raises or preserves the previous argv byte-for-byte for accepted names, so there is no way for it to widen permissions. The security direction is therefore safe — the residual risk is false rejection of a legitimate skill name, not injection. However, the PR is a deliberate breaking change to a public option (wildcards, padded names, and slash-prefixed names now raise at connect), and several checks encode claims about the CLI's tokenizer and Skill-tool trim semantics that cannot be verified from this repository alone. Those are product/design calls a maintainer should confirm, and a human reviewer is already engaged on the thread.

Scrutiny level: this is permission-rule construction, which the approval guidelines treat as security-sensitive, and it carries documented breaking behavior — both disqualify shadow-approval regardless of code quality. Test coverage is thorough (rejection and acceptance cases for every branch, including the shapes from prior findings), and the author reports the full suite, ruff, and mypy clean.