fix: emit provider-safe host tool names - #511
Merged
chubes4 merged 1 commit intoAug 22, 2026
Merged
Conversation
Host tool declarations reach the provider under their canonical namespaced name, and provider tool-name validation rejects the `/`. This is the same constraint Automattic#320 addressed for client runtime declarations, on the half that Automattic#320 did not cover. Client runtime tools could sidestep it, because the consumer chooses the name and can simply declare `filesystem_write`. Host tools cannot: they are derived from abilities, and the Abilities API requires a namespaced `namespace/name`, refusing to register anything else. So every ability-backed declaration carries a `/`, and `WP_Agent_Default_Provider_Turn_Adapter` emits it verbatim. Observed against a live OpenAI-compatible endpoint, using the shipped default chat handler with an ability-backed toolset: tools.0.custom.name: String should match pattern '^[a-zA-Z0-9_-]{1,128}$' The alias this needs is already computed and already carried: `normalizeForConversationRequest()` records `provider_safe_name` on any declaration whose canonical name is not provider-safe, and `WP_Agent_Provider_Turn_Request` normalizes declarations on construction, so it is present by the time the adapter maps them. The return leg is already wired too — `WP_Agent_Tool_Execution_Core` resolves the provider's emitted name back through `canonicalNameForProviderToolName()`, so mediation still matches the canonical declaration and tool observability still reports the canonical name. Only the outbound mapping was missing. Three existing assertions changed expectation rather than being added to, and they are the ones worth reviewing: they asserted that the canonical namespaced name is what reaches the provider, which is the behaviour being corrected. The suite could not have caught this otherwise, because every provider result in it is stubbed, so no test sends a tool name to anything that validates it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
@aaronware thanks for the contribution! looks good to me |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
provider_safe_namefor host tool declarations inWP_Agent_Default_Provider_Turn_Adapter::function_declarations()Why
Host tool declarations reach the provider under their canonical namespaced name, and provider tool-name validation rejects the
/. This is the same constraint #320 fixed for client runtime declarations — on the half #320 did not cover.Client runtime tools could sidestep it, because the consumer picks the name and can simply declare
filesystem_write. Host tools cannot: they are derived from abilities, and the Abilities API requires a namespacednamespace/nameand refuses to register anything else. So every ability-backed declaration carries a/.Observed against a live OpenAI-compatible endpoint, using the shipped default chat handler with an ability-backed toolset:
The same request succeeds with this patch applied, executes the ability, and reports the canonical name back in
tool_observability.For what it is worth, this is not specific to one connector:
FunctionDeclarationstores the name verbatim,AbstractOpenAiCompatibleTextGenerationModel::prepareToolsParam()emits it verbatim, and the officialai-provider-for-anthropicconnector does the same ('name' => $functionDeclaration->getName()). Nothing between the declaration and the wire sanitizes it.What was already in place
Only the outbound mapping was missing:
normalizeForConversationRequest()already recordsprovider_safe_namewhenever the canonical name is not provider-safeWP_Agent_Provider_Turn_Requestnormalizes declarations on construction, so the alias is present by the time the adapter maps themWP_Agent_Tool_Execution_Corealready resolves the provider's emitted name back viacanonicalNameForProviderToolName(), so mediation still matches the canonical declarationReview note
Three existing assertions change expectation rather than being added to, and they are the ones worth a close look:
function declaration carries the … tool nameclient/lookupclient__lookupdispatch payload function declaration carries the … tool nameclient/lookupclient__lookupclient/lookupclient__lookupThey asserted that the canonical namespaced name is what reaches the provider, which is the behaviour this corrects. If that was load-bearing intent rather than recorded behaviour, this PR is the wrong shape and I would rather know.
The suite could not have caught this on its own: every provider result in it is stubbed via
$make_result(), so no test sends a tool name to anything that validates one. Added assertions now check every emitted name against^[a-zA-Z0-9_-]{1,128}$.Testing
composer phpstan— no errorscomposer smoke— all suites pass (default-provider-turn-adapter-smoke.php: 116 assertions)AI assistance