Prevent exception when constructing `WP_AI_Client_Prompt_Builder` when invalid timeout is provided by filter by westonruter · Pull Request #11596 · WordPress/wordpress-develop

westonruter · 2026-04-18T03:56:25Z

Trac ticket: https://core.trac.wordpress.org/ticket/65094

The primary change here is in WP_AI_Client_Prompt_Builder::__construct() to check that the return value of the wp_ai_client_default_request_timeout filter is in fact a non-negative float. It also newly allows the value null. Otherwise, the filtered value is discarded in favor of using the original default of 30, and a _doing_it_wrong() is issued. Without this check, a fatal error would ensue due to an exception being thrown by \WordPress\AiClient\Providers\Http\DTO\RequestOptions::validateTimeout().

In addition to this change, static analysis issues were resolved:

Use float instead of int for the wp_ai_client_default_request_timeout.
Adding missing PHP imports for Message and MessagePart for the PHPDoc for wp_ai_client_prompt().
Adding PHP type hints for the return values of wp_ai_client_prompt() and \WP_AI_Client_Cache::getMultiple().
Use native property type hints in WP_AI_Client_HTTP_Client.

The following PHPStan level 10 issues are resolved:

src/wp-includes/ai-client.php: Parameter $prompt of function wp_ai_client_prompt() has invalid type Message.
src/wp-includes/ai-client.php: Parameter $prompt of function wp_ai_client_prompt() has invalid type MessagePart.
src/wp-includes/ai-client/class-wp-ai-client-prompt-builder.php: Cannot cast mixed to int.

Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 4.7
Used for: Research

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

github-actions · 2026-04-18T03:56:36Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props westonruter, justlevine.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2026-04-18T20:48:02Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

justlevine

@westonruter what benefit does changing wp_ai_client_default_request_timeout to support ?float instead of just float?

0 (or 0.0) can still be the disabled value without the elseif and typecheck. Also gets rid of the separate $timeout<->$default_timeout assignment juggling which IMO is confusing.

The other changes (type inlining) LGTM 🚀

justlevine · 2026-04-20T14:30:46Z

+		$timeout = apply_filters( 'wp_ai_client_default_request_timeout', $default_timeout );
+		if ( is_numeric( $timeout ) && (float) $timeout >= 0.0 ) {
+			$default_timeout = (float) $timeout;
+		} elseif ( null === $timeout ) {
+			$default_timeout = null;
+		} else {
+			_doing_it_wrong(
+				__METHOD__,
+				sprintf(
+					/* translators: %s: wp_ai_client_default_request_timeout */
+					__( 'The %s filter must return a non-negative number or null.' ),
+					'<code>wp_ai_client_default_request_timeout</code>'
+				),
+				'7.0.0'
+			);
+		}


Even if there is a reason to support a separate null , I'd still coerce to avoid the confusing $default_timeout -> $timeout -> $default_timeout flow. But I don't think it's necessary.

We definitely don't need the extra handholding for < 0 validation, since that's handled by setTimeout directly.

Suggested change

$timeout = apply_filters( 'wp_ai_client_default_request_timeout', $default_timeout );

if ( is_numeric( $timeout ) && (float) $timeout >= 0.0 ) {

$default_timeout = (float) $timeout;

} elseif ( null === $timeout ) {

$default_timeout = null;

} else {

_doing_it_wrong(

__METHOD__,

sprintf(

/* translators: %s: wp_ai_client_default_request_timeout */

__( 'The %s filter must return a non-negative number or null.' ),

'<code>wp_ai_client_default_request_timeout</code>'

),

'7.0.0'

);

}

$default_timeout = (float) apply_filters( 'wp_ai_client_default_request_timeout', 30.0 );

// Coerce falsy values to null. But IMO even this is unnecessary and `0.0` is fine.

$default_timeout = 0.0 === $default_timeout ? null : $default_timeout;

westonruter · 2026-04-20T15:18:10Z

@justlevine:

what benefit does changing wp_ai_client_default_request_timeout to support ?float instead of just float?

I suppose because this matches the types supported by RequestOptions. Also, doing strict equality checks between float values is not safe. For example: https://3v4l.org/QYUem

So I don't think it is completely reliable to use 0.0 as the value for disabling the timeout. Better to use null to match RequestOptions.

justlevine · 2026-04-20T15:39:34Z

@justlevine:

what benefit does changing wp_ai_client_default_request_timeout to support ?float instead of just float?

I suppose because this matches the types supported by RequestOptions. Also, doing strict equality checks between float values is not safe. For example: https://3v4l.org/QYUem

So I don't think it is completely reliable to use 0.0 as the value for disabling the timeout. Better to use null to match RequestOptions.

@westonruter Not sure I follow your reliability concerns: https://3v4l.org/vk3Pc . But worst case we can coerce to whatever "stable" check you feel comfortable with: $default_timeout = (int) $default_timeout === 0 ? ... or ! empty( $timeout ) ? ... or whatever all feel easier to reason with than multiple conditional paths.

As far as matching RequestOptions, personally I don't see a need or justification for an internal hook to match the shape of a DTO, if our internal implementation needs the whole shape.

However, if folks feel a need to align the two, then I'd suggest making the type on the RequestOptions DTO stricter, not loosening our public API (the hook signature) to accommodate the unnecessary polymorphism and edge cases.

westonruter · 2026-04-20T17:02:35Z

Not sure I follow your reliability concerns: https://3v4l.org/vk3Pc . But worst case we can coerce to whatever "stable" check you feel comfortable with: $default_timeout = (int) $default_timeout === 0 ? ... or ! empty( $timeout ) ? ... or whatever all feel easier to reason with than multiple conditional paths.

It's not reliable to check if a float value is equal to 0.0, especially if arithmetic has been done on the value. Since the value has come from a filter, we do not know what operations have been done on it.

Casting the value to int is not viable either because it will prevent values like 0.5 from being usable.

justlevine · 2026-04-20T17:08:14Z

Not sure I follow your reliability concerns: https://3v4l.org/vk3Pc . But worst case we can coerce to whatever "stable" check you feel comfortable with: $default_timeout = (int) $default_timeout === 0 ? ... or ! empty( $timeout ) ? ... or whatever all feel easier to reason with than multiple conditional paths.

It's not reliable to check if a float value is equal to 0.0, especially if arithmetic has been done on the value. Since the value has come from a filter, we do not know what operations have been done on it.

Casting the value to int is not viable either because it will prevent values like 0.5 from being usable.

what arithmetic though? we're casting it on our side after the filter, and we in full control of the value and what happens to it afterwards. And yeah casting locally to int just for a falsely check wouldn't prevent 0.5 at all... #11596 (comment)

Copilot

Pull request overview

This PR hardens the AI client prompt builder construction so a misbehaving wp_ai_client_default_request_timeout filter does not trigger exceptions/fatal errors, and it also addresses several PHPStan/static-analysis issues across the AI client integration.

Changes:

Add validation/guardrails for wp_ai_client_default_request_timeout (accept non-negative numeric values or null, otherwise fall back + _doing_it_wrong()).
Extend PHPUnit coverage for valid/invalid timeout overrides, including null.
Static-analysis improvements: add missing imports for Message / MessagePart, add return types, and add native property types in the HTTP client adapter.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`src/wp-includes/ai-client/class-wp-ai-client-prompt-builder.php`	Validates filtered default timeout and avoids exceptions from invalid values.
`tests/phpunit/tests/ai-client/wpAiClientPromptBuilder.php`	Adds/updates tests for timeout override behavior (float, null, invalid).
`src/wp-includes/ai-client/adapters/class-wp-ai-client-http-client.php`	Adds native property type hints for factories.
`src/wp-includes/ai-client/adapters/class-wp-ai-client-cache.php`	Adds an explicit `array` return type for `getMultiple()`.
`src/wp-includes/ai-client.php`	Adds missing imports for PHPDoc types and adds a return type to `wp_ai_client_prompt()`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

 		 */
-		$default_timeout = (int) apply_filters( 'wp_ai_client_default_request_timeout', 30 );
+		$timeout = apply_filters( 'wp_ai_client_default_request_timeout', $default_timeout );
+		if ( is_numeric( $timeout ) && (float) $timeout >= 0.0 ) {


westonruter · 2026-04-20T18:50:28Z

what arithmetic though? we're casting it on our side after the filter, and we in full control of the value and what happens to it afterwards. And yeah casting locally to int just for a falsely check wouldn't prevent 0.5 at all... #11596 (comment)

For example:

add_filter( 'wp_ai_client_default_request_timeout', function ( $value ) {
    return $value - (float) get_option( 'my_timeout_delta' );
} );

The value being passed in is a float to begin with here. So the casting being added here is just in case there is a string value in one of the filter callbacks.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

westonruter · 2026-04-20T19:08:27Z

Something else to consider is the fact that float values can be infinity (INF). That would effectively be the same as null. In that case, should \WordPress\AI_Client\HTTP\WordPress_HTTP_Client::prepare_wp_args() be updated to account for that?

--- a/src/wp-includes/ai-client/adapters/class-wp-ai-client-http-client.php
+++ b/src/wp-includes/ai-client/adapters/class-wp-ai-client-http-client.php
@@ -138,7 +138,7 @@ class WP_AI_Client_HTTP_Client implements ClientInterface, ClientWithOptionsInte
 		);
 
 		if ( null !== $options ) {
-			if ( null !== $options->getTimeout() ) {
+			if ( null !== $options->getTimeout() && is_finite( $options->getTimeout() ) ) {
 				$args['timeout'] = $options->getTimeout();
 			}

While the timeout arg for WP HTTP says it takes a float, it's not clear that INF would be allowed.

Oh, something more important I'm seeing as well: currently when null is passed, this is preventing prepare_wp_args() from setting the timeout, which is causing the default timeout of 5 to be used when the request is actually set. This doesn't seem entirely expected. If null (or INF) is supplied, then it would seem that something should be set to the default value from being used.

justlevine · 2026-04-20T20:49:43Z

what arithmetic though? we're casting it on our side after the filter, and we in full control of the value and what happens to it afterwards. And yeah casting locally to int just for a falsely check wouldn't prevent 0.5 at all... #11596 (comment)

For example:

I feel like that example bolsters my point? Adding a line:

+ add_filter( 'wp_ai_client_default_request_timeout', '__return_null' ); // somewhere else
add_filter( 'wp_ai_client_default_request_timeout', function ( $value ) {
    return $value - (float) get_option( 'my_timeout_delta' );
} );

since $value is assumed a float, if someone forgets to cast it from null like in your example it won't be an issue. Meanwhile the return (float) apply_filters() means that even if they return an int or null or empty string, we can pass to to our dto... e.g. add_filter('wp_ai_client_default_request_timeout', '__return_false' ); where itl be caught by PHPStan but only error if a filter like yours conflicts.

Perhaps you can add a test that would red if someone refactors this in the future to cast the results of apply_filters(), but pass because we're doing it with null|float and explicit checking? Would prevent the regression you're clearly concerned about, and be a easier for me to follow concretely.

If null (or INF) is supplied, then it would seem that something should be set to the default value from being used.

(Putting aside that the null issue is solved by not allowing it/casting to float), remember this whole thing is just a shallow wrapper for WordPress/requests. If that library is missing handling for one of these edge cases, then IMO the behavior should be handled there, not absorbed downstream in our (or any other component). even without the type safety these aren't particularly common foot guns I don't think.

westonruter · 2026-04-20T21:36:05Z

since $value is assumed a float, if someone forgets to cast it from null like in your example it won't be an issue. Meanwhile the return (float) apply_filters() means that even if they return an int or null or empty string, we can pass to to our dto... e.g. add_filter('wp_ai_client_default_request_timeout', '__return_false' ); where itl be caught by PHPStan but only error if a filter like yours conflicts.

I'm just trying to say that the return value if an applied filter will be a float and we can't reliably compare it with 0.0 because of floating point math.

Because RequestOptions::validateTimeout() allows a float or null, where null apparently is supposed to mean that there is no timeout enforced (but maybe I'm wrong). It seems there should be an ability to disable a timeout, such as in a WP-CLI context. Returning null would make sense as the way to disable the timeout given what is allowed by RequestOptions. Otherwise, if we don't want to allow null, we could document INF as a valid value. But we can't use 0.0 because testing equality with floats is not reliable.

justlevine · 2026-04-20T22:02:38Z

Because RequestOptions::validateTimeout() allows a float or null, where null apparently is supposed to mean that there is no timeout enforced (but maybe I'm wrong).

If I'm not mistaken and as you also noted, wp_safe_remote_request() / WordPress/Requests treats 0 (or 0.0 ) as no timeout, whereas null falls back to the default fallback of 5s and not our 30s that we use for the hook here. The dto as implemented follows that, so IMO it make sense to lock down the dto not to change the meaning of null to use it in our hook. Similarly, if the intent would for null to default to 30.0, we could (imo should) still coerce that here, and chance the DTO to not accept null and only float.

I still don't understand the point youre trying to make about float-math. There's never a time wed need to do a float === float check, literally any falsey value can be coerced to the 0 or 0.0 that we to send the DTO, and unless you're saying you want it to be positive-float|0 (which imo is still an improvement over *|null), I'm lost as to what edge case this protects against. So with that I'm going to respectfully tap out (unless a test case gets added that covers it) 🙇‍♂️

justlevine · 2026-04-20T22:15:02Z

( fwiw http_request_timeout is also typed as a float

wordpress-develop/src/wp-includes/class-wp-http.php

Lines 178 to 181 in 074792e

    
           			 * @param float  $timeout_value Time in seconds until a request times out. Default 5. 
        
           			 * @param string $url           The request URL. 
        
           			 */ 
        
           			'timeout'             => apply_filters( 'http_request_timeout', 5, $url ),

, but consistency for its own sake isn't an argument, if it's deserving we can just as easily broaden the type there too. )

westonruter · 2026-04-20T22:28:53Z

There's never a time wed need to do a float === float check

This is what you suggested in #11596 (comment).

justlevine · 2026-04-20T22:59:36Z

There's never a time wed need to do a float === float check

This is what you suggested in #11596 (comment).

yes and as the comment there and the rest of our conversation indicate, that was me attempting to understanding your concern, but IMO even this is unnecessary and we can just let folks pass any float value they want and not do any coercing beyond the negative-int check in the dto. 🤷‍♂️ But if we do need some coersion that can just as easily be (int) float === (int) float or ! empty (float) or anything in between.

westonruter · 2026-04-20T23:04:15Z

If I'm not mistaken and as you also noted, wp_safe_remote_request() / WordPress/Requests treats 0 (or 0.0 ) as no timeout

I don't think I suggested this. A timeout of zero actually gets increased to 1 second:

wordpress-develop/src/wp-includes/Requests/src/Transport/Curl.php

Lines 425 to 431 in 047ef80

    
           // cURL requires a minimum timeout of 1 second when using the system 
        
           // DNS resolver, as it uses `alarm()`, which is second resolution only. 
        
           // There's no way to detect which DNS resolver is being used from our 
        
           // end, so we need to round up regardless of the supplied timeout. 
        
           // 
        
           // https://github.com/curl/curl/blob/4f45240bc84a9aa648c8f7243be7b79e9f9323a5/lib/hostip.c#L606-L609 
        
           $timeout = max($options['timeout'], 1);

I tested this by creating a file called sleep-2.php locally:

<?php
sleep( 2 );
echo "Awake!";

Then, from that directory I started a server:

php -S 0.0.0.0:8888

Then in the wordpress-develop environment, I put this file:

<?php
$response = wp_remote_get(
	'http://host.docker.internal:8888/sleep-2.php  ',
	array( 'timeout' => 0.0 )
);

if ( is_wp_error( $response ) ) {
	WP_CLI::error( $response->get_error_message() );
}

WP_CLI::log( 'Code: ' . wp_remote_retrieve_response_code( $response ) );
WP_CLI::log( 'Body: ' . wp_remote_retrieve_body( $response ) );

And when I do this:

npm run env:cli eval-file -- try-timeout.php

I get this:

Error: cURL error 28: Operation timed out after 1004 milliseconds with 0 bytes received

So a timeout of 0.0 does not mean the timeout is disabled in WordPress HTTP API.

If I change the timeout to 3.0, then I get this:

Code: 200
Body: Awake!

Note that if I use INF here:

$response = wp_remote_get(
	'http://host.docker.internal:8888/sleep-2.php  ',
	array( 'timeout' => INF )
);

Then this also does not time-out, so it seems to be valid at least for the cURL transport.

So we cannot use a value of 0.0 to indicate the timeout is disabled. Either we have the filter return null, INF, or just have the user return a super big number. To override the WP HTTP API's 5 second default timeout, we'll need to provide something, regardless of the return value from wp_ai_client_default_request_timeout. Either INF or a super big number seem to work, so if null is returned we'll need to map to either of those values.

westonruter added 6 commits April 17, 2026 19:33

Add missing PHP imports

438011b

Add missing return types

7eb2c19

Use PHP class member variable type hints

8006183

Ensure default timeout is number casted to float and is at least zero

63542e4

Use float value explicitly for timeout

ed8d8d6

Allow filtered timeout to be null and warn when invalid value provided

9925c25

westonruter added 2 commits April 17, 2026 21:01

Update test to use float value

f01e50d

Merge branch 'trunk' into fix/request-timeout-exception

a14916b

justlevine reviewed Apr 20, 2026

View reviewed changes

westonruter requested a review from felixarntz April 20, 2026 15:47

westonruter requested a review from Copilot April 20, 2026 17:06

Copilot started reviewing on behalf of westonruter April 20, 2026 17:06 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

westonruter and others added 2 commits April 20, 2026 11:51

Fix grammar typo

0068422

Use a data provider for invalid request timeout tests

8e4c3df

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Conversation

westonruter commented Apr 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Use of AI Tools

Uh oh!

github-actions bot commented Apr 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 18, 2026

Test using WordPress Playground

Some things to be aware of

Uh oh!

justlevine left a comment

Choose a reason for hiding this comment

Uh oh!

justlevine Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

westonruter commented Apr 20, 2026

Uh oh!

justlevine commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonruter commented Apr 20, 2026

Uh oh!

justlevine commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

westonruter commented Apr 20, 2026

Uh oh!

westonruter commented Apr 20, 2026

Uh oh!

justlevine commented Apr 20, 2026

Uh oh!

westonruter commented Apr 20, 2026

Uh oh!

justlevine commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justlevine commented Apr 20, 2026

Uh oh!

westonruter commented Apr 20, 2026

Uh oh!

justlevine commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonruter commented Apr 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

westonruter commented Apr 18, 2026 •

edited

Loading

github-actions bot commented Apr 18, 2026 •

edited

Loading

justlevine commented Apr 20, 2026 •

edited

Loading

justlevine commented Apr 20, 2026 •

edited

Loading

justlevine commented Apr 20, 2026 •

edited

Loading

justlevine commented Apr 20, 2026 •

edited

Loading