Make WordPress Core

Opened 6 weeks ago

Closed 4 weeks ago

#65782 closed defect (bug) (fixed)

AI Client: the wp_ai_client_prevent_prompt filter gets a shallow clone and can change the prompt

Reported by: bejignesh Owned by: gziolo
Priority: normal Milestone: 7.2
Component: AI Version: 7.0
Severity: normal Keywords: has-patch has-unit-tests
Cc: Focuses:

Description

WP_AI_Client_Prompt_Builder::__call() hands a clone of itself to the wp_ai_client_prevent_prompt filter, and the hook doc calls it read only:

/**
 * @param bool                        $prevent Whether to prevent the prompt. Default false.
 * @param WP_AI_Client_Prompt_Builder $builder A clone of the prompt builder instance (read-only).
 */
$prevent = (bool) apply_filters( 'wp_ai_client_prevent_prompt', false, clone $this );

The class does not define __clone(), so that is a shallow copy and the clone still points at the same wrapped PromptBuilder. The wrapped builder mutates itself rather than returning new instances:

public function withText(string $text): self
{
        $part = new MessagePart($text);
        $this->appendPartToMessages($part);
        return $this;
}

So whatever a filter callback does to the clone lands on the original, and the prompt that is then sent to the provider is not the prompt the caller built.

Steps to reproduce

add_filter(
        'wp_ai_client_prevent_prompt',
        static function ( $prevent, $builder ) {
                $builder->with_text( 'Added by the filter' );
                return $prevent;
        },
        10,
        2
);

$builder = wp_ai_client_prompt( 'Original prompt' );
$builder->is_supported();

// The prompt is now 'Original promptAdded by the filter'.

This runs on every call that reaches the filter, so support checks and generating methods both.

Expected

A filter receiving the clone cannot change the prompt that gets generated, which is what the hook doc promises.

Patch

Add __clone() to WP_AI_Client_Prompt_Builder so the wrapped builder is cloned as well. The bundled client already implements PromptBuilder::__clone() to deep clone its messages, model config and request options. It was written for exactly this, it just never ran because only the WordPress wrapper was being cloned.

Any stored WP_Error is copied too. Nothing today clones the wrapper while it holds an error, since the filter is only reached when the error is still null, but __clone() is a magic method any caller can trigger and leaving half the state shared would be the same bug in a different place.

The existing test test_prevent_prompt_filter_receives_cloned_builder_instance() only checks that the outer object differs, which is why this went unnoticed. It still passes.

Change History (4)

This ticket was mentioned in PR #12801 on WordPress/wordpress-develop by @bejignesh.


6 weeks ago
#1

  • Keywords has-patch has-unit-tests added

WP_AI_Client_Prompt_Builder::__call() hands a clone of itself to the wp_ai_client_prevent_prompt filter, and the hook doc calls it read only:

/**
 * @param bool                        $prevent Whether to prevent the prompt. Default false.
 * @param WP_AI_Client_Prompt_Builder $builder A clone of the prompt builder instance (read-only).
 */
$prevent = (bool) apply_filters( 'wp_ai_client_prevent_prompt', false, clone $this );

The class has no __clone(), so that is a shallow copy and the clone still points at the same wrapped PromptBuilder. The wrapped builder mutates itself rather than returning new instances:

public function withText(string $text): self
{
        $part = new MessagePart($text);
        $this->appendPartToMessages($part);
        return $this;
}

So whatever a filter does to the clone lands on the original, and the prompt sent to the provider is not the one the caller built.

add_filter(
        'wp_ai_client_prevent_prompt',
        static function ( $prevent, $builder ) {
                $builder->with_text( 'Added by the filter' );
                return $prevent;
        },
        10,
        2
);

$builder = wp_ai_client_prompt( 'Original prompt' );
$builder->is_supported();

// The prompt is now 'Original promptAdded by the filter'.

This runs on every call that reaches the filter, so support checks and generating methods both.

## Fix

Add __clone() so the wrapped builder is cloned too. The bundled client already implements PromptBuilder::__clone() to deep clone messages, model config and request options. It was written for this, it just never ran because only the wrapper was being cloned.

A stored WP_Error is copied as well. Nothing today clones the wrapper while it holds an error, since the filter is only reached when the error is still null, but __clone() is a magic method any caller can trigger and leaving half the state shared would be the same bug in a different place.

## Testing

Two tests, both failing on trunk:

1) test_clone_does_not_share_the_wrapped_builder
A clone should wrap its own builder instance
Failed asserting that two variables don't reference the same object.

2) test_prevent_prompt_filter_cannot_mutate_the_original_prompt
A filter should not be able to change the prompt
-'Original prompt'
+'Original promptAdded by the filter'

Both pass with the patch. The existing test_prevent_prompt_filter_receives_cloned_builder_instance() only asserted the outer objects differ, which is why this went unnoticed, and it still passes. The full ai-client group is green, 259 tests. phpcs passes on both changed files.

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Reading through the AI client for places where the code disagrees with its own documented behaviour, which is how this turned up, and drafting this description. I confirmed the mutation by hand, checked that the bundled client already has a deep __clone(), wrote and ran the tests, checked they fail without the patch, ran phpcs, and I take responsibility for the change.

#2 @gziolo
5 weeks ago

  • Milestone Awaiting Review7.2
  • Owner set to gziolo
  • Status newreviewing

@bejignesh commented on PR #12801:


4 weeks ago
#3

Good catch, corrected to @since 7.2.0 in 5c4c3bcf. Confirmed trunk is on 7.2-alpha-63166-src.

ai-client group still passes at 259 tests and phpcs is clean. The sibling PR #12800 adds no @since, so nothing to change there.

#4 @gziolo
4 weeks ago

  • Resolutionfixed
  • Status reviewingclosed

In 63299:

Clone the wrapped builder when cloning WP_AI_Client_Prompt_Builder

WP_AI_Client_Prompt_Builder had no __clone(), so cloning it copied the reference to the wrapped PromptBuilder instead of the builder itself. That builder mutates its own state, so the clone handed to the wp_ai_client_prevent_prompt filter shared its messages and configuration with the original, and a filter could change the prompt that was then sent to the provider, despite the clone being documented as read-only.

Add __clone() so the wrapped builder, and any stored error, are copied too. The bundled client already implements PromptBuilder::__clone() to deep clone messages, model config and request options, it just never ran because only the wrapper was being cloned.

Props bejignesh, gziolo.
Fixes #65782.

Note: See TracTickets for help on using tickets.