From 33607cb1d188c0f2d78d172650f9c29cc2cf4a34 Mon Sep 17 00:00:00 2001 From: Filip Ilic Date: Fri, 18 Sep 2026 12:48:00 +0200 Subject: [PATCH 1/4] Let an agent apply recommendations that are a single setting Adds progress-planner/complete-recommendation, which applies a recommendation either by provider ID or, with no argument, the highest-priority one that needs no value from the caller. Seventeen providers qualify. The set is an explicit list, not everything extending Tasks_Interactive: that base class also covers sending a test email (a diagnostic, not a fix), deleting terms, and rewriting permalinks, which flushes rewrite rules and changes every URL on the site. Each entry in Recommendation_Fixes was read individually and is listed because it writes one known setting and nothing else. Yoast's tasks had no server-side save path at all. They predate the AIOSEO integration and only ever got a UI affordance: the popover deep-links into Yoast's own settings screen and highlights the field. That is useful to a person and meaningless to an agent, which never loads the page. The setting and its value always come from that table and never from caller input, so no ability argument can reach an arbitrary option. Anything outside the list is returned with status "manual" and an admin URL rather than half-applied, and each SEO entry checks its plugin is active first. Completion is observed, not asserted: after writing, the provider decides whether the site now satisfies the task. Claiming otherwise would award points for work that did not happen. Because evaluation runs on admin_init, a just-fixed task is still pending in the database, so next-mode skips providers that already report themselves satisfied instead of picking the same one twice. There is no nonce, deliberately. An authenticated agent call is not a forged cross-origin form post, so manage_options and the fixed settings list are what bound this. Co-Authored-By: Claude Opus 5 (1M context) --- classes/abilities/class-abilities.php | 45 ++- .../abilities/class-recommendation-fixes.php | 330 ++++++++++++++++++ classes/abilities/class-recommendations.php | 179 +++++++++- classes/abilities/class-schemas.php | 63 ++++ tests/phpunit/test-class-abilities.php | 237 ++++++++++++- 5 files changed, 851 insertions(+), 3 deletions(-) create mode 100644 classes/abilities/class-recommendation-fixes.php diff --git a/classes/abilities/class-abilities.php b/classes/abilities/class-abilities.php index 0c2521ba9..1e3298cb8 100644 --- a/classes/abilities/class-abilities.php +++ b/classes/abilities/class-abilities.php @@ -40,6 +40,16 @@ class Abilities { */ const READ_CAPABILITY = 'edit_others_posts'; + /** + * The capability required to change a setting. + * + * The same capability the interactive tasks check before writing, so an + * ability can never do what the popover would refuse. + * + * @var string + */ + const WRITE_CAPABILITY = 'manage_options'; + /** * The site-score reader. * @@ -135,6 +145,21 @@ public function register_abilities() { ] ) ); + + \wp_register_ability( + self::CATEGORY . '/complete-recommendation', + $this->ability_args( + [ + 'label' => \__( 'Complete recommendation', 'progress-planner' ), + 'description' => \__( 'Apply a Progress Planner recommendation that consists of a single site setting, such as the tagline, timezone or an SEO plugin toggle. Only a fixed list of settings can be changed this way; anything needing judgement, content or deletion is reported back with a link instead of being applied.', 'progress-planner' ), + 'input_schema' => Schemas::complete_recommendation_input(), + 'output_schema' => Schemas::complete_recommendation(), + 'permission_callback' => [ $this, 'can_fix' ], + 'execute_callback' => [ $this->recommendations, 'complete' ], + 'readonly' => false, + ] + ) + ); } /** @@ -148,6 +173,9 @@ public function register_abilities() { * @return array */ private function ability_args( array $args ) { + $readonly = $args['readonly'] ?? true; + unset( $args['readonly'] ); + return \array_merge( [ 'category' => self::CATEGORY, @@ -155,7 +183,9 @@ private function ability_args( array $args ) { 'meta' => [ 'show_in_rest' => true, 'annotations' => [ - 'readonly' => true, + 'readonly' => $readonly, + // Nothing registered here deletes or overwrites content: + // the write ability changes settings from a fixed list. 'destructive' => false, 'idempotent' => true, ], @@ -174,6 +204,19 @@ public function can_read() { return \current_user_can( self::READ_CAPABILITY ); } + /** + * Whether the current user may apply a fix. + * + * There is no nonce: an authenticated agent call is not a forged + * cross-origin form post, so this capability and the fixed list of settings + * in Recommendation_Fixes are what bound the write surface. + * + * @return bool + */ + public function can_fix() { + return \current_user_can( self::WRITE_CAPABILITY ); + } + /** * Whether the ability category is already registered. * diff --git a/classes/abilities/class-recommendation-fixes.php b/classes/abilities/class-recommendation-fixes.php new file mode 100644 index 000000000..0d4bcc8db --- /dev/null +++ b/classes/abilities/class-recommendation-fixes.php @@ -0,0 +1,330 @@ +> + */ + private const FIXES = [ + 'core-blogdescription' => [ + 'option' => 'blogdescription', + 'input' => 'value', + 'type' => 'string', + 'summary' => 'Set the site tagline.', + ], + 'select-timezone' => [ + 'option' => 'timezone_string', + 'input' => 'value', + 'type' => 'timezone', + 'summary' => 'Set the site timezone.', + ], + 'set-date-format' => [ + 'option' => 'date_format', + 'input' => 'value', + 'type' => 'string', + 'summary' => 'Set the date format.', + ], + 'search-engine-visibility' => [ + 'option' => 'blog_public', + 'value' => '1', + 'type' => 'string', + 'summary' => 'Allow search engines to index the site.', + ], + 'disable-comments' => [ + 'option' => 'default_comment_status', + 'value' => 'closed', + 'type' => 'string', + 'summary' => 'Close comments on new content by default.', + ], + 'disable-comment-pagination' => [ + 'option' => 'page_comments', + 'value' => '', + 'type' => 'string', + 'summary' => 'Turn off comment pagination.', + ], + + /* + * SEO-plugin settings. These do not go through update_option: each plugin + * owns its own settings store and validates on write, so the value is set + * through the plugin's own API. The task only exists when its plugin is + * active, so an entry here is unreachable otherwise. + * + * Yoast's tasks were built before AIOSEO's and only ever got a UI path -- + * the popover deep-links into Yoast's settings screen and highlights the + * field. That affordance is for a person; the underlying setting is a + * plain boolean, so an agent can set it directly. + */ + 'yoast-author-archive' => [ + 'seo' => 'yoast', + 'setting' => 'disable-author', + 'value' => true, + 'summary' => 'Disable author archives in Yoast SEO.', + ], + 'yoast-date-archive' => [ + 'seo' => 'yoast', + 'setting' => 'disable-date', + 'value' => true, + 'summary' => 'Disable date archives in Yoast SEO.', + ], + 'yoast-format-archive' => [ + 'seo' => 'yoast', + 'setting' => 'disable-post_format', + 'value' => true, + 'summary' => 'Disable post-format archives in Yoast SEO.', + ], + 'yoast-media-pages' => [ + 'seo' => 'yoast', + 'setting' => 'disable-attachment', + 'value' => true, + 'summary' => 'Redirect attachment pages in Yoast SEO.', + ], + 'yoast-crawl-settings-emoji-scripts' => [ + 'seo' => 'yoast', + 'setting' => 'remove_emoji_scripts', + 'value' => true, + 'summary' => 'Remove emoji scripts via Yoast SEO.', + ], + 'yoast-crawl-settings-feed-authors' => [ + 'seo' => 'yoast', + 'setting' => 'remove_feed_authors', + 'value' => true, + 'summary' => 'Disable author feeds via Yoast SEO.', + ], + 'yoast-crawl-settings-feed-global-comments' => [ + 'seo' => 'yoast', + 'setting' => 'remove_feed_global_comments', + 'value' => true, + 'summary' => 'Disable the global comment feed via Yoast SEO.', + ], + + /* + * AIOSEO stores its settings as a nested object. The path below is walked + * on the live options object, matching what each provider's own submit + * handler does. + */ + 'aioseo-author-archive' => [ + 'seo' => 'aioseo', + 'root' => 'options', + 'path' => [ 'searchAppearance', 'archives', 'author', 'show' ], + 'value' => false, + 'summary' => 'Noindex author archives in All in One SEO.', + ], + 'aioseo-date-archive' => [ + 'seo' => 'aioseo', + 'root' => 'options', + 'path' => [ 'searchAppearance', 'archives', 'date', 'show' ], + 'value' => false, + 'summary' => 'Noindex date archives in All in One SEO.', + ], + 'aioseo-crawl-settings-feed-authors' => [ + 'seo' => 'aioseo', + 'root' => 'options', + 'path' => [ 'searchAppearance', 'advanced', 'crawlCleanup', 'feeds', 'authors' ], + 'value' => false, + 'summary' => 'Disable author feeds in All in One SEO.', + ], + 'aioseo-media-pages' => [ + 'seo' => 'aioseo', + // Attachment redirection lives under dynamicOptions, not options, and + // takes the destination as a string rather than a boolean. + 'root' => 'dynamicOptions', + 'path' => [ 'searchAppearance', 'postTypes', 'attachment', 'redirectAttachmentUrls' ], + 'value' => 'attachment', + 'summary' => 'Redirect attachment URLs in All in One SEO.', + ], + ]; + + /** + * Whether a provider has a fix an agent may apply. + * + * @param string $provider_id The provider ID. + * + * @return bool + */ + public static function has_fix( $provider_id ) { + return isset( self::FIXES[ $provider_id ] ); + } + + /** + * Get the provider IDs that can be fixed. + * + * @return array + */ + public static function fixable_providers() { + return \array_keys( self::FIXES ); + } + + /** + * Get the fix definition for a provider. + * + * @param string $provider_id The provider ID. + * + * @return array|null + */ + public static function get( $provider_id ) { + return self::FIXES[ $provider_id ] ?? null; + } + + /** + * Whether the fix needs a value from the caller. + * + * @param string $provider_id The provider ID. + * + * @return bool + */ + public static function needs_value( $provider_id ) { + $fix = self::get( $provider_id ); + + return null !== $fix && isset( $fix['input'] ); + } + + /** + * Apply a fix. + * + * Writes exactly one option. The option name comes from the table above and + * never from caller input, so there is no path from an ability argument to an + * arbitrary option name. + * + * @param string $provider_id The provider ID. + * @param mixed $value The value supplied by the caller, if any. + * + * @return true|\WP_Error + */ + public static function apply( $provider_id, $value = null ) { + $fix = self::get( $provider_id ); + + if ( null === $fix ) { + return new \WP_Error( + 'progress_planner_not_fixable', + \__( 'This recommendation cannot be applied automatically.', 'progress-planner' ) + ); + } + + if ( isset( $fix['seo'] ) ) { + return self::apply_seo_setting( $fix ); + } + + if ( isset( $fix['input'] ) ) { + $validated = self::validate( $fix['type'], $value ); + + if ( \is_wp_error( $validated ) ) { + return $validated; + } + + $new_value = $validated; + } else { + $new_value = $fix['value']; + } + + \update_option( $fix['option'], $new_value ); + + return true; + } + + /** + * Apply a setting owned by an SEO plugin. + * + * The setting name and value both come from the table above, never from + * caller input, so the only thing an ability argument can influence is which + * vetted entry runs. + * + * @param array $fix The fix definition. + * + * @return true|\WP_Error + */ + private static function apply_seo_setting( array $fix ) { + if ( 'yoast' === $fix['seo'] ) { + if ( ! \class_exists( '\WPSEO_Options' ) ) { + return new \WP_Error( + 'progress_planner_seo_plugin_inactive', + \__( 'Yoast SEO is not active on this site.', 'progress-planner' ) + ); + } + + \WPSEO_Options::set( $fix['setting'], $fix['value'] ); + + return true; + } + + if ( ! \function_exists( 'aioseo' ) ) { + return new \WP_Error( + 'progress_planner_seo_plugin_inactive', + \__( 'All in One SEO is not active on this site.', 'progress-planner' ) + ); + } + + $root = \aioseo()->{$fix['root']}; + $path = $fix['path']; + $last = \array_pop( $path ); + + foreach ( $path as $step ) { + if ( ! isset( $root->$step ) ) { + return new \WP_Error( + 'progress_planner_seo_setting_missing', + \__( 'That All in One SEO setting is not available in this version.', 'progress-planner' ) + ); + } + + $root = $root->$step; + } + + $root->$last = $fix['value']; + + \aioseo()->options->save(); // @phpstan-ignore-line property.nonObject + + return true; + } + + /** + * Validate a caller-supplied value. + * + * @param string $type The declared type. + * @param mixed $value The value. + * + * @return string|\WP_Error + */ + private static function validate( $type, $value ) { + if ( ! \is_string( $value ) || '' === \trim( $value ) ) { + return new \WP_Error( + 'progress_planner_missing_value', + \__( 'This recommendation needs a value.', 'progress-planner' ) + ); + } + + $value = \sanitize_text_field( $value ); + + if ( 'timezone' === $type && ! \in_array( $value, \timezone_identifiers_list(), true ) ) { + return new \WP_Error( + 'progress_planner_invalid_timezone', + \__( 'That is not a valid timezone identifier, for example "Europe/Amsterdam".', 'progress-planner' ) + ); + } + + return $value; + } +} diff --git a/classes/abilities/class-recommendations.php b/classes/abilities/class-recommendations.php index 53aa560b0..0bfbfa26e 100644 --- a/classes/abilities/class-recommendations.php +++ b/classes/abilities/class-recommendations.php @@ -1,6 +1,6 @@ $input The ability input. + * + * @return array|\WP_Error + */ + public function complete( $input = [] ) { + $provider_id = isset( $input['provider_id'] ) ? (string) $input['provider_id'] : ''; + $value = isset( $input['value'] ) ? (string) $input['value'] : null; + + if ( '' === $provider_id ) { + $provider_id = $this->get_next_fixable_provider_id(); + + if ( '' === $provider_id ) { + return [ + 'applied' => false, + 'status' => 'nothing_to_do', + 'message' => \__( 'There is no pending recommendation that can be applied automatically.', 'progress-planner' ), + 'task' => null, + 'admin_url' => '', + ]; + } + } + + $task = $this->find_pending_task( $provider_id ); + + if ( null === $task ) { + return new \WP_Error( + 'progress_planner_no_such_recommendation', + \__( 'There is no pending recommendation for that provider.', 'progress-planner' ), + [ 'status' => 404 ] + ); + } + + $provider = $this->get_provider( $provider_id ); + + // The provider's own capability check, the same one the popover runs. + if ( ! $provider || ! $provider->capability_required() ) { + return new \WP_Error( + 'progress_planner_cannot_complete', + \__( 'You do not have permission to complete this recommendation.', 'progress-planner' ), + [ 'status' => 403 ] + ); + } + + // Anything outside the fixable list is reported, never half-applied. + if ( ! Recommendation_Fixes::has_fix( $provider_id ) ) { + return $this->result( + false, + 'manual', + \__( 'This recommendation needs a person: it involves content, a deletion, or a choice that should not be made automatically. Open the link to handle it.', 'progress-planner' ), + $task + ); + } + + $applied = Recommendation_Fixes::apply( $provider_id, $value ); + + if ( \is_wp_error( $applied ) ) { + return $applied; + } + + // Completion is observed, never asserted: the provider decides whether + // the site now satisfies the task. Saying otherwise would award points + // for work that did not happen. + $completed = $this->is_satisfied( $provider, $task ); + + return $this->result( + true, + $completed ? 'completed' : 'applied_not_yet_complete', + $completed + ? \__( 'The setting was changed and the recommendation is now satisfied.', 'progress-planner' ) + : \__( 'The setting was changed, but the recommendation is not reported as satisfied yet.', 'progress-planner' ), + $task + ); + } + + /** + * Build a complete() result. + * + * @param bool $applied Whether a setting changed. + * @param string $status The outcome status. + * @param string $message The human-readable outcome. + * @param \Progress_Planner\Suggested_Tasks\Task $task The task acted on. + * + * @return array + */ + private function result( $applied, $status, $message, $task ) { + return [ + 'applied' => $applied, + 'status' => $status, + 'message' => $message, + 'task' => $this->prepare( $task ), + 'admin_url' => (string) $task->url, + ]; + } + + /** + * Whether a provider reports its task as already satisfied. + * + * @param \Progress_Planner\Suggested_Tasks\Tasks_Interface $provider The provider. + * @param \Progress_Planner\Suggested_Tasks\Task $task The task. + * + * @return bool + */ + private function is_satisfied( $provider, $task ) { + return \method_exists( $provider, 'is_task_completed' ) + && (bool) $provider->is_task_completed( $task->get_task_id() ); + } + + /** + * Find the highest-priority pending recommendation that can be fixed. + * + * Tasks come back ordered by menu_order, which is the order the dashboard + * shows them in, so "next" means the same thing to an agent as to a person. + * + * @return string The provider ID, or an empty string when there is none. + */ + private function get_next_fixable_provider_id() { + $tasks = \progress_planner()->get_suggested_tasks_db()->get_tasks_by( + [ + 'post_status' => 'publish', + 'posts_per_page' => -1, + ] + ); + + foreach ( $tasks as $task ) { + $provider_id = $task->get_provider_id(); + + // A fix needing a value cannot be chosen unattended: there is no + // correct tagline to invent on the site owner's behalf. + if ( ! Recommendation_Fixes::has_fix( $provider_id ) || Recommendation_Fixes::needs_value( $provider_id ) ) { + continue; + } + + $provider = $this->get_provider( $provider_id ); + + if ( ! $provider || ! $provider->capability_required() ) { + continue; + } + + // Task evaluation runs on admin_init, so a task fixed a moment ago is + // still 'publish' here. Skipping the already-satisfied ones stops a + // repeated run from picking the same task and reporting it as new work. + if ( $this->is_satisfied( $provider, $task ) ) { + continue; + } + + return $provider_id; + } + + return ''; + } + + /** + * Find a pending task for a provider. + * + * @param string $provider_id The provider ID. + * + * @return \Progress_Planner\Suggested_Tasks\Task|null + */ + private function find_pending_task( $provider_id ) { + $tasks = \progress_planner()->get_suggested_tasks_db()->get_tasks_by( + [ + 'post_status' => 'publish', + 'provider' => $provider_id, + 'posts_per_page' => 1, + ] + ); + + return $tasks ? $tasks[0] : null; + } + /** * Get a task provider. * @@ -108,6 +281,10 @@ private function prepare( $task ) { // User-created tasks carry no points of their own; the provider is // the reliable source, with the stored value preferred when set. 'points' => (int) ( $task->points ?? $provider->get_points() ), + // Lets a caller plan a run without discovering by trial which + // recommendations it is allowed to apply. + 'fixable' => Recommendation_Fixes::has_fix( $provider_id ), + 'needs_value' => Recommendation_Fixes::needs_value( $provider_id ), ]; } } diff --git a/classes/abilities/class-schemas.php b/classes/abilities/class-schemas.php index de3c90974..d534ec944 100644 --- a/classes/abilities/class-schemas.php +++ b/classes/abilities/class-schemas.php @@ -59,6 +59,27 @@ public static function list_recommendations_input() { ]; } + /** + * The input schema for complete-recommendation. + * + * @return array + */ + public static function complete_recommendation_input() { + return [ + 'type' => 'object', + 'additionalProperties' => false, + 'properties' => [ + 'provider_id' => [ + 'type' => 'string', + 'description' => \__( 'The provider ID of the recommendation to apply, for example "core-blogdescription". Omit to apply the highest-priority recommendation that can be applied automatically.', 'progress-planner' ), + ], + 'value' => [ + 'type' => 'string', + 'description' => \__( 'The value to set, for recommendations that need one: the tagline text, a timezone identifier such as "Europe/Amsterdam", or a date format string. Recommendations with only one correct outcome ignore this.', 'progress-planner' ), + ], + ], + ]; + } /** * The output schema for get-site-score. @@ -188,6 +209,48 @@ public static function recommendation() { 'type' => 'integer', 'description' => \__( 'Points awarded for completing the task.', 'progress-planner' ), ], + 'fixable' => [ + 'type' => 'boolean', + 'description' => \__( 'Whether complete-recommendation can apply this one.', 'progress-planner' ), + ], + 'needs_value' => [ + 'type' => 'boolean', + 'description' => \__( 'Whether applying it requires a value from the caller, such as the tagline text.', 'progress-planner' ), + ], + ], + ]; + } + + /** + * The output schema for complete-recommendation. + * + * @return array + */ + public static function complete_recommendation() { + return [ + 'type' => 'object', + 'properties' => [ + 'applied' => [ + 'type' => 'boolean', + 'description' => \__( 'Whether a setting was changed.', 'progress-planner' ), + ], + 'status' => [ + 'type' => 'string', + 'description' => \__( 'What happened: "completed" when the recommendation is now satisfied, "applied_not_yet_complete" when the setting changed but the task is not satisfied, "manual" when it needs a person, "nothing_to_do" when no automatic recommendation was pending.', 'progress-planner' ), + 'enum' => [ 'completed', 'applied_not_yet_complete', 'manual', 'nothing_to_do' ], + ], + 'message' => [ + 'type' => 'string', + 'description' => \__( 'A sentence describing the outcome.', 'progress-planner' ), + ], + 'task' => [ + 'type' => [ 'object', 'null' ], + 'description' => \__( 'The recommendation that was acted on, if any.', 'progress-planner' ), + ], + 'admin_url' => [ + 'type' => 'string', + 'description' => \__( 'Where a person can handle this recommendation themselves.', 'progress-planner' ), + ], ], ]; } diff --git a/tests/phpunit/test-class-abilities.php b/tests/phpunit/test-class-abilities.php index 216cab52c..d1b91a377 100644 --- a/tests/phpunit/test-class-abilities.php +++ b/tests/phpunit/test-class-abilities.php @@ -14,6 +14,7 @@ namespace Progress_Planner\Tests; use Progress_Planner\Abilities\Abilities; +use Progress_Planner\Abilities\Recommendation_Fixes; /** * Abilities test case. @@ -35,7 +36,7 @@ class Abilities_Test extends \WP_UnitTestCase { private $site_score; /** - * Recommendations reader. + * Recommendations reader and applier. * * @var \Progress_Planner\Abilities\Recommendations */ @@ -428,4 +429,238 @@ public function test_registration_is_idempotent() { $this->assertTrue( true, 'Re-registering did not trigger incorrect usage.' ); } + + /** + * Create a pending recommendation for a provider. + * + * @param string $provider_id The provider ID. + * + * @return void + */ + private function seed_task( $provider_id ) { + \progress_planner()->get_suggested_tasks_db()->add( + [ + 'task_id' => 'test-' . $provider_id, + 'post_title' => 'Test ' . $provider_id, + 'provider_id' => $provider_id, + ] + ); + } + + /** + * Test that only vetted providers are fixable. + * + * The list is deliberate, not derived from class inheritance: Tasks_Interactive + * also covers sending a test email and deleting terms. + * + * @return void + */ + public function test_fixable_list_is_the_vetted_set() { + $core = [ + 'core-blogdescription', + 'select-timezone', + 'set-date-format', + 'search-engine-visibility', + 'disable-comments', + 'disable-comment-pagination', + ]; + + foreach ( $core as $provider_id ) { + $this->assertTrue( + Recommendation_Fixes::has_fix( $provider_id ), + "{$provider_id} should be fixable." + ); + } + + // SEO-plugin entries only make sense when their plugin is active, but the + // table lists them unconditionally; the guard is in apply(). + $this->assertTrue( Recommendation_Fixes::has_fix( 'yoast-crawl-settings-emoji-scripts' ) ); + $this->assertTrue( Recommendation_Fixes::has_fix( 'aioseo-date-archive' ) ); + } + + /** + * Test that tasks needing a person are not fixable. + * + * @return void + */ + public function test_unsafe_providers_are_not_fixable() { + foreach ( [ 'sending-email', 'remove-terms-without-posts', 'core-permalink-structure', 'remove-inactive-plugins', 'create-post' ] as $provider_id ) { + $this->assertFalse( + Recommendation_Fixes::has_fix( $provider_id ), + "{$provider_id} must not be auto-applied." + ); + } + } + + /** + * Test that a subscriber cannot apply a fix. + * + * @return void + */ + public function test_can_fix_denies_subscriber() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'subscriber' ] ) ); + + $this->assertFalse( $this->abilities->can_fix() ); + } + + /** + * Test that applying a fix changes the setting. + * + * @return void + */ + public function test_complete_recommendation_applies_the_setting() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + \update_option( 'blog_public', '0' ); + $this->seed_task( 'search-engine-visibility' ); + + $result = $this->recommendations->complete( [ 'provider_id' => 'search-engine-visibility' ] ); + + $this->assertTrue( $result['applied'] ); + $this->assertSame( 'completed', $result['status'] ); + $this->assertSame( '1', (string) \get_option( 'blog_public' ) ); + } + + /** + * Test that a recommendation needing a person is reported, not applied. + * + * @return void + */ + public function test_complete_recommendation_refuses_manual_tasks() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + $this->seed_task( 'remove-inactive-plugins' ); + + $result = $this->recommendations->complete( [ 'provider_id' => 'remove-inactive-plugins' ] ); + + $this->assertFalse( $result['applied'] ); + $this->assertSame( 'manual', $result['status'] ); + } + + /** + * Test that an invalid timezone is rejected before anything is written. + * + * @return void + */ + public function test_complete_recommendation_validates_timezone() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + $before = \get_option( 'timezone_string' ); + $this->seed_task( 'select-timezone' ); + + $result = $this->recommendations->complete( + [ + 'provider_id' => 'select-timezone', + 'value' => 'Mars/Olympus', + ] + ); + + $this->assertWPError( $result ); + $this->assertSame( 'progress_planner_invalid_timezone', $result->get_error_code() ); + $this->assertSame( $before, \get_option( 'timezone_string' ) ); + } + + /** + * Test that a fix needing a value refuses an empty one. + * + * @return void + */ + public function test_complete_recommendation_requires_a_value() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + $this->seed_task( 'core-blogdescription' ); + + $result = $this->recommendations->complete( [ 'provider_id' => 'core-blogdescription' ] ); + + $this->assertWPError( $result ); + $this->assertSame( 'progress_planner_missing_value', $result->get_error_code() ); + } + + /** + * Test that an unknown provider is an error rather than a silent no-op. + * + * @return void + */ + public function test_complete_recommendation_rejects_unknown_provider() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + + $result = $this->recommendations->complete( [ 'provider_id' => 'no-such-provider' ] ); + + $this->assertWPError( $result ); + $this->assertSame( 'progress_planner_no_such_recommendation', $result->get_error_code() ); + } + + /** + * Test that next mode reports honestly when there is nothing to do. + * + * @return void + */ + public function test_next_mode_reports_nothing_to_do() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + + $result = $this->recommendations->complete( [] ); + + $this->assertContains( $result['status'], [ 'nothing_to_do', 'completed', 'applied_not_yet_complete' ] ); + } + + /** + * Test that next mode never picks a fix that needs a value. + * + * There is no correct tagline to invent on the owner's behalf. + * + * @return void + */ + public function test_next_mode_skips_fixes_needing_a_value() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + $this->seed_task( 'core-blogdescription' ); + + $result = $this->recommendations->complete( [] ); + + if ( null !== $result['task'] ) { + $this->assertNotSame( 'core-blogdescription', $result['task']['provider_id'] ); + } else { + $this->assertSame( 'nothing_to_do', $result['status'] ); + } + } + + /** + * Test that the listing marks which recommendations can be applied. + * + * @return void + */ + public function test_list_recommendations_flags_fixable_items() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + $this->seed_task( 'search-engine-visibility' ); + + $result = $this->recommendations->list( [ 'limit' => 100 ] ); + + $found = false; + foreach ( $result['recommendations'] as $recommendation ) { + $this->assertArrayHasKey( 'fixable', $recommendation ); + $this->assertArrayHasKey( 'needs_value', $recommendation ); + + if ( 'search-engine-visibility' === $recommendation['provider_id'] ) { + $found = true; + $this->assertTrue( $recommendation['fixable'] ); + $this->assertFalse( $recommendation['needs_value'] ); + } + } + + $this->assertTrue( $found, 'Expected the seeded recommendation in the list.' ); + } + + /** + * Test that an SEO fix refuses when its plugin is not active. + * + * The task would not exist on such a site, but the table lists these + * unconditionally, so apply() must not fatal if one is reached. + * + * @return void + */ + public function test_seo_fix_requires_its_plugin() { + if ( \function_exists( 'aioseo' ) ) { + $this->markTestSkipped( 'All in One SEO is active in this environment.' ); + } + + $result = Recommendation_Fixes::apply( 'aioseo-date-archive' ); + + $this->assertWPError( $result ); + $this->assertSame( 'progress_planner_seo_plugin_inactive', $result->get_error_code() ); + } } From 64821a53013c1cbff03cf64c8714f8e5c364267f Mon Sep 17 00:00:00 2001 From: Filip Ilic Date: Fri, 18 Sep 2026 14:06:01 +0200 Subject: [PATCH 2/4] Add a short confirmation code to the test email Email delivery is the one check a site cannot verify for itself: WordPress knows wp_mail() returned true, not that anything arrived. The existing token solves that by living only in the delivered message, so presenting it back proves delivery. The token is delivered as the href of a "Click here" link, which works for a person who clicks and not for a person who wants to relay it. Reading it off the screen is impossible; copying it means right-clicking and pasting a 128-character URL. That matters now this question gets asked through an AI assistant rather than the dashboard. An assistant with mailbox access can read the token and needs nothing from the user. An assistant without one has to ask, and "tell it the code from the email" only works if the code is short enough to say out loud. So the email now carries both: the link, unchanged, and a four-character code shown as text. The code is deliberately weaker than the token, and the docblock on generate_task_confirmation_code() records why that is acceptable -- single use, 24-hour expiry, scoped to one user and one task, manage_options required, and rate limiting around it. Anyone who could satisfy all of that can complete the task from wp-admin anyway. The alphabet omits characters that are misread when spoken or retyped (0/O, 1/I/L, 5/S, 8/B), and comparison ignores case and surrounding whitespace, because a code that survives being read aloud is the entire point. A test asserts the alphabet holds across repeated generation. The email body was built in two places; it is now built once. Co-Authored-By: Claude Opus 5 (1M context) --- classes/class-suggested-tasks.php | 109 ++++++++++++++++++ .../providers/class-email-sending.php | 38 ++++-- tests/phpunit/test-class-suggested-tasks.php | 87 ++++++++++++++ 3 files changed, 224 insertions(+), 10 deletions(-) diff --git a/classes/class-suggested-tasks.php b/classes/class-suggested-tasks.php index d09ae5bae..0e3d64ce8 100644 --- a/classes/class-suggested-tasks.php +++ b/classes/class-suggested-tasks.php @@ -244,12 +244,121 @@ public function maybe_complete_task() { } } + /** + * The alphabet used for confirmation codes. + * + * Excludes characters that are easily confused when read aloud or retyped: + * 0/O, 1/I/L, 5/S, 8/B. A code is only useful if a person relays it + * correctly on the first try. + * + * @var string + */ + const CONFIRMATION_CODE_ALPHABET = '234679ACDEFGHJKMNPQRTUVWXYZ'; + + /** + * The length of a confirmation code. + * + * @var int + */ + const CONFIRMATION_CODE_LENGTH = 4; + + /** + * Generate a short confirmation code for a task. + * + * WHY THIS EXISTS, alongside the 32-character token below: + * + * Both prove the same thing -- that the email actually arrived, because + * the value only exists inside the delivered message. They differ in who + * can realistically relay them. + * + * The token is delivered as the href of a "Click here" link. A person + * cannot read it off the screen: they would have to right-click, copy the + * link address, and paste a 128-character URL. That is fine for clicking + * and useless for telling someone. + * + * This matters because email delivery is the one check a site cannot + * verify for itself, and it is increasingly answered through an AI + * assistant rather than the dashboard. An assistant with mailbox access + * reads the token and needs nothing from the user. An assistant without it + * has to ask, and "tell it the code from the email" only works if the code + * is short enough to say out loud. + * + * The code is deliberately weaker than the token: four characters from a + * 27-character alphabet is about 19 bits. What makes that acceptable is + * everything around it -- the code is single-use, expires in 24 hours, is + * scoped to one user and one task, is only accepted from a caller who + * already holds manage_options, and failed attempts are rate limited. An + * attacker who could satisfy all of that can complete the task by simply + * clicking a button in wp-admin, so the code is not the weak link. + * + * Codes are stored uppercase and compared case-insensitively, because a + * person retyping one should not have to think about it. + * + * @param string $task_id The task ID. + * @param int $user_id The user ID. + * + * @return string The generated confirmation code. + */ + public function generate_task_confirmation_code( $task_id, $user_id ) { + $alphabet = self::CONFIRMATION_CODE_ALPHABET; + $max = \strlen( $alphabet ) - 1; + $code = ''; + + for ( $i = 0; $i < self::CONFIRMATION_CODE_LENGTH; $i++ ) { + $code .= $alphabet[ \wp_rand( 0, $max ) ]; + } + + \set_transient( + 'prpl_confirm_' . $task_id . '_' . $user_id, + $code, + DAY_IN_SECONDS + ); + + return $code; + } + + /** + * Verify a task confirmation code. + * + * @param string $task_id The task ID. + * @param int $user_id The user ID. + * @param string $provided_code The code supplied by the caller. + * + * @return bool + */ + public function verify_task_confirmation_code( $task_id, $user_id, $provided_code ) { + $stored_code = \get_transient( 'prpl_confirm_' . $task_id . '_' . $user_id ); + + if ( ! $stored_code ) { + return false; // Expired, already used, or never issued. + } + + // hash_equals to keep the comparison constant-time; strtoupper because + // the code is meant to be retyped by a person. + return \hash_equals( (string) $stored_code, \strtoupper( \trim( $provided_code ) ) ); + } + + /** + * Delete a task confirmation code after use. + * + * @param string $task_id The task ID. + * @param int $user_id The user ID. + * + * @return bool True if deleted, false otherwise. + */ + public function delete_task_confirmation_code( $task_id, $user_id ) { + return \delete_transient( 'prpl_confirm_' . $task_id . '_' . $user_id ); + } + /** * Generate a secure token for task completion via email link. * * This token prevents CSRF attacks by ensuring only legitimate email * links can mark tasks as complete. * + * See generate_task_confirmation_code() above for why a second, shorter + * value exists next to this one. + * * @param string $task_id The task ID. * @param int $user_id The user ID. * diff --git a/classes/suggested-tasks/providers/class-email-sending.php b/classes/suggested-tasks/providers/class-email-sending.php index a1be12d19..e278ed9a4 100644 --- a/classes/suggested-tasks/providers/class-email-sending.php +++ b/classes/suggested-tasks/providers/class-email-sending.php @@ -114,11 +114,34 @@ public function init() { $user_id = \get_current_user_id(); $token = \progress_planner()->get_suggested_tasks()->generate_task_completion_token( $this->get_task_id(), $user_id ); - $this->email_content = \sprintf( - // translators: %1$s the admin URL. - \__( 'You just used Progress Planner to verify if sending email works on your website.

The good news; it does! Click here to mark %2$s\'s Recommendation as completed.', 'progress-planner' ), + $this->email_content = $this->get_email_content( $token, $user_id ); + } + + /** + * Build the test email body. + * + * The body carries two ways to confirm the message arrived: a link, which + * carries the full token and is what a person clicks, and a short code, + * which is what a person can read out to an AI assistant that has no + * access to their mailbox. Both prove the same thing -- that the message + * was delivered -- because neither value exists anywhere the recipient + * could reach without receiving it. See + * Suggested_Tasks::generate_task_confirmation_code() for the reasoning. + * + * @param string $token The completion token. + * @param int $user_id The user the token and code belong to. + * + * @return string + */ + protected function get_email_content( $token, $user_id ) { + $code = \progress_planner()->get_suggested_tasks()->generate_task_confirmation_code( $this->get_task_id(), $user_id ); + + return \sprintf( + /* translators: %1$s: the admin URL, %2$s: the assistant's name, %3$s: a short confirmation code. */ + \__( 'You just used Progress Planner to verify if sending email works on your website.

The good news; it does! Click here to mark %2$s\'s Recommendation as completed.

Using an AI assistant? Give it this confirmation code instead: %3$s', 'progress-planner' ), \admin_url( 'admin.php?page=progress-planner&prpl_complete_task=' . $this->get_task_id() . '&token=' . $token ), - \esc_html( \progress_planner()->get_ui__branding()->get_ravi_name() ) + \esc_html( \progress_planner()->get_ui__branding()->get_ravi_name() ), + \esc_html( $code ) ); } @@ -285,12 +308,7 @@ public function ajax_test_email_sending() { $user_id = \get_current_user_id(); $token = \progress_planner()->get_suggested_tasks()->generate_task_completion_token( $this->get_task_id(), $user_id ); - $email_content = \sprintf( - // translators: %1$s the admin URL. - \__( 'You just used Progress Planner to verify if sending email works on your website.

The good news; it does! Click here to mark %2$s\'s Recommendation as completed.', 'progress-planner' ), - \admin_url( 'admin.php?page=progress-planner&prpl_complete_task=' . $this->get_task_id() . '&token=' . $token ), - \esc_html( \progress_planner()->get_ui__branding()->get_ravi_name() ) - ); + $email_content = $this->get_email_content( $token, $user_id ); $headers = [ 'Content-Type: text/html; charset=UTF-8' ]; diff --git a/tests/phpunit/test-class-suggested-tasks.php b/tests/phpunit/test-class-suggested-tasks.php index fec2a15d4..6be4baf68 100644 --- a/tests/phpunit/test-class-suggested-tasks.php +++ b/tests/phpunit/test-class-suggested-tasks.php @@ -102,4 +102,91 @@ public function test_task_cleanup() { \wp_cache_flush_group( \Progress_Planner\Suggested_Tasks_DB::GET_TASKS_CACHE_GROUP ); // Clear the cache. $this->assertEquals( \count( $tasks_to_keep ), \count( \progress_planner()->get_suggested_tasks_db()->get_tasks_by( [ 'post_status' => 'publish' ] ) ) ); } + + /** + * Test that a confirmation code round-trips. + * + * @return void + */ + public function test_confirmation_code_verifies() { + $suggested_tasks = \progress_planner()->get_suggested_tasks(); + $code = $suggested_tasks->generate_task_confirmation_code( 'sending-email', 1 ); + + $this->assertSame( 4, \strlen( $code ) ); + $this->assertTrue( $suggested_tasks->verify_task_confirmation_code( 'sending-email', 1, $code ) ); + } + + /** + * Test that a code is accepted however a person retypes it. + * + * A code only has value if it survives being read aloud and typed back, + * so case and surrounding whitespace must not matter. + * + * @return void + */ + public function test_confirmation_code_is_forgiving_about_formatting() { + $suggested_tasks = \progress_planner()->get_suggested_tasks(); + $code = $suggested_tasks->generate_task_confirmation_code( 'sending-email', 1 ); + + $this->assertTrue( $suggested_tasks->verify_task_confirmation_code( 'sending-email', 1, \strtolower( $code ) ) ); + $this->assertTrue( $suggested_tasks->verify_task_confirmation_code( 'sending-email', 1, ' ' . $code . ' ' ) ); + } + + /** + * Test that a wrong code is rejected. + * + * @return void + */ + public function test_confirmation_code_rejects_a_wrong_code() { + $suggested_tasks = \progress_planner()->get_suggested_tasks(); + $suggested_tasks->generate_task_confirmation_code( 'sending-email', 1 ); + + $this->assertFalse( $suggested_tasks->verify_task_confirmation_code( 'sending-email', 1, 'ZZZZ' ) ); + } + + /** + * Test that a code belongs to one user and one task. + * + * @return void + */ + public function test_confirmation_code_is_scoped() { + $suggested_tasks = \progress_planner()->get_suggested_tasks(); + $code = $suggested_tasks->generate_task_confirmation_code( 'sending-email', 1 ); + + $this->assertFalse( $suggested_tasks->verify_task_confirmation_code( 'sending-email', 2, $code ) ); + $this->assertFalse( $suggested_tasks->verify_task_confirmation_code( 'other-task', 1, $code ) ); + } + + /** + * Test that a code cannot be reused once consumed. + * + * @return void + */ + public function test_confirmation_code_is_single_use() { + $suggested_tasks = \progress_planner()->get_suggested_tasks(); + $code = $suggested_tasks->generate_task_confirmation_code( 'sending-email', 1 ); + + $suggested_tasks->delete_task_confirmation_code( 'sending-email', 1 ); + + $this->assertFalse( $suggested_tasks->verify_task_confirmation_code( 'sending-email', 1, $code ) ); + } + + /** + * Test that codes avoid characters that are misread. + * + * @return void + */ + public function test_confirmation_code_avoids_ambiguous_characters() { + $suggested_tasks = \progress_planner()->get_suggested_tasks(); + + for ( $i = 0; $i < 40; $i++ ) { + $code = $suggested_tasks->generate_task_confirmation_code( 'sending-email', 1 ); + + $this->assertSame( + 0, + \preg_match( '/[01OILSB58]/', $code ), + "Code {$code} contains an easily confused character." + ); + } + } } From 7e63e0c1eedc1f93339453e765b48ea533376779 Mon Sep 17 00:00:00 2001 From: Filip Ilic Date: Fri, 18 Sep 2026 14:15:53 +0200 Subject: [PATCH 3/4] Let an agent clear WordPress's placeholder content Adds hello-world and sample-page, bringing the applicable set to 19. These are the first entries that remove something rather than change a setting, and they are treated differently for it: - Annotated destructive. The annotation describes what the ability can do, not what a given call does, so complete-recommendation as a whole is now destructive and a client prompts before any of it runs. - Confirm-only, so next-mode skips them. A daily unattended run must never be the thing that deleted something; they can only be applied by naming the provider. - Trashed, not force-deleted. The dashboard's JavaScript passes force=true and removes the post outright. An agent should leave a way back, and the completion check passes either way because it looks for a published post. What makes them defensible at all is that the target is not ambiguous: the data collector resolves the specific post WordPress ships, by slug with a title fallback, and no ID is ever taken from the caller. The success message says the content was trashed and can be restored, rather than reusing the settings wording. The new tests create and trash posts, which writes to the activities table. That table is custom, so WP_UnitTestCase does not roll it back, and post IDs are reused across tests -- a leftover row was being seen by an unrelated test that asserts a fresh post has no activity. The class now clears the table in tearDown. Co-Authored-By: Claude Opus 5 (1M context) --- classes/abilities/class-abilities.php | 15 ++- .../abilities/class-recommendation-fixes.php | 106 +++++++++++++++++ classes/abilities/class-recommendations.php | 27 ++++- classes/abilities/class-schemas.php | 4 + tests/phpunit/test-class-abilities.php | 111 ++++++++++++++++++ 5 files changed, 253 insertions(+), 10 deletions(-) diff --git a/classes/abilities/class-abilities.php b/classes/abilities/class-abilities.php index 1e3298cb8..16af34762 100644 --- a/classes/abilities/class-abilities.php +++ b/classes/abilities/class-abilities.php @@ -157,6 +157,12 @@ public function register_abilities() { 'permission_callback' => [ $this, 'can_fix' ], 'execute_callback' => [ $this->recommendations, 'complete' ], 'readonly' => false, + // Most of what this applies is a settings change, but two + // recommendations trash WordPress's placeholder content. The + // annotation describes what the ability can do, not what a + // given call happens to do, so it is declared destructive and + // a client prompts before any of it runs. + 'destructive' => true, ] ) ); @@ -173,8 +179,9 @@ public function register_abilities() { * @return array */ private function ability_args( array $args ) { - $readonly = $args['readonly'] ?? true; - unset( $args['readonly'] ); + $readonly = $args['readonly'] ?? true; + $destructive = $args['destructive'] ?? false; + unset( $args['readonly'], $args['destructive'] ); return \array_merge( [ @@ -184,9 +191,7 @@ private function ability_args( array $args ) { 'show_in_rest' => true, 'annotations' => [ 'readonly' => $readonly, - // Nothing registered here deletes or overwrites content: - // the write ability changes settings from a fixed list. - 'destructive' => false, + 'destructive' => $destructive, 'idempotent' => true, ], ], diff --git a/classes/abilities/class-recommendation-fixes.php b/classes/abilities/class-recommendation-fixes.php index 0d4bcc8db..255fef69b 100644 --- a/classes/abilities/class-recommendation-fixes.php +++ b/classes/abilities/class-recommendation-fixes.php @@ -148,6 +148,37 @@ class Recommendation_Fixes { 'value' => false, 'summary' => 'Disable author feeds in All in One SEO.', ], + + /* + * Deleting WordPress's own placeholder content. + * + * These are the only entries that remove something rather than change a + * setting, so they are the only ones annotated destructive. Two things + * make them defensible anyway: the target is not ambiguous -- the data + * collector resolves the specific post WordPress ships, by slug with a + * title fallback -- and the recommendation exists precisely because the + * site owner is being asked to delete it. + * + * They are trashed, not force-deleted. The dashboard's own JavaScript + * passes force=true and removes the post outright; an agent acting + * unattended should leave a way back, and the task's completion check + * passes either way because it looks for a published post. + * + * confirm_only keeps them out of next-mode, so a daily unattended run + * never deletes anything: they can only be applied by naming the + * provider explicitly. + */ + 'hello-world' => [ + 'delete' => 'post', + 'confirm_only' => true, + 'summary' => 'Move the default "Hello world!" post to the trash.', + ], + 'sample-page' => [ + 'delete' => 'page', + 'confirm_only' => true, + 'summary' => 'Move the default "Sample Page" to the trash.', + ], + 'aioseo-media-pages' => [ 'seo' => 'aioseo', // Attachment redirection lives under dynamicOptions, not options, and @@ -225,6 +256,10 @@ public static function apply( $provider_id, $value = null ) { ); } + if ( isset( $fix['delete'] ) ) { + return self::apply_deletion( $provider_id ); + } + if ( isset( $fix['seo'] ) ) { return self::apply_seo_setting( $fix ); } @@ -246,6 +281,77 @@ public static function apply( $provider_id, $value = null ) { return true; } + /** + * Move a placeholder post to the trash. + * + * The post ID comes from the provider's own data collector, which resolves + * WordPress's default content by slug. Nothing here takes an ID from the + * caller, so an ability argument cannot point this at arbitrary content. + * + * @param string $provider_id The provider ID. + * + * @return true|\WP_Error + */ + private static function apply_deletion( $provider_id ) { + $provider = \progress_planner()->get_suggested_tasks()->get_tasks_manager()->get_task_provider( $provider_id ); + + if ( ! $provider || ! \method_exists( $provider, 'get_data_collector' ) ) { + return new \WP_Error( + 'progress_planner_no_target', + \__( 'That recommendation is not available on this site.', 'progress-planner' ) + ); + } + + $post_id = (int) $provider->get_data_collector()->collect(); + + if ( ! $post_id || ! \get_post( $post_id ) ) { + return new \WP_Error( + 'progress_planner_no_target', + \__( 'The default content this recommendation refers to is no longer there.', 'progress-planner' ) + ); + } + + if ( ! \wp_trash_post( $post_id ) ) { + return new \WP_Error( + 'progress_planner_delete_failed', + \__( 'The content could not be moved to the trash.', 'progress-planner' ) + ); + } + + return true; + } + + /** + * Whether applying a fix removes content rather than changing a setting. + * + * Drives the destructive annotation, which is what a client shows a person + * before calling. + * + * @param string $provider_id The provider ID. + * + * @return bool + */ + public static function is_destructive( $provider_id ) { + $fix = self::get( $provider_id ); + + return null !== $fix && isset( $fix['delete'] ); + } + + /** + * Whether a fix may only be applied by naming its provider explicitly. + * + * Keeps anything that removes content out of an unattended run. + * + * @param string $provider_id The provider ID. + * + * @return bool + */ + public static function is_confirm_only( $provider_id ) { + $fix = self::get( $provider_id ); + + return null !== $fix && ! empty( $fix['confirm_only'] ); + } + /** * Apply a setting owned by an SEO plugin. * diff --git a/classes/abilities/class-recommendations.php b/classes/abilities/class-recommendations.php index 0bfbfa26e..838985374 100644 --- a/classes/abilities/class-recommendations.php +++ b/classes/abilities/class-recommendations.php @@ -135,12 +135,23 @@ public function complete( $input = [] ) { // for work that did not happen. $completed = $this->is_satisfied( $provider, $task ); + // The wording distinguishes a settings change from a deletion: an agent + // relaying this to a person should not describe trashing a post as + // changing a setting. + if ( Recommendation_Fixes::is_destructive( $provider_id ) ) { + $message = $completed + ? \__( 'The content was moved to the trash and the recommendation is now satisfied. It can be restored from the trash if that was not intended.', 'progress-planner' ) + : \__( 'The content was moved to the trash, but the recommendation is not reported as satisfied yet.', 'progress-planner' ); + } else { + $message = $completed + ? \__( 'The setting was changed and the recommendation is now satisfied.', 'progress-planner' ) + : \__( 'The setting was changed, but the recommendation is not reported as satisfied yet.', 'progress-planner' ); + } + return $this->result( true, $completed ? 'completed' : 'applied_not_yet_complete', - $completed - ? \__( 'The setting was changed and the recommendation is now satisfied.', 'progress-planner' ) - : \__( 'The setting was changed, but the recommendation is not reported as satisfied yet.', 'progress-planner' ), + $message, $task ); } @@ -198,8 +209,13 @@ private function get_next_fixable_provider_id() { $provider_id = $task->get_provider_id(); // A fix needing a value cannot be chosen unattended: there is no - // correct tagline to invent on the site owner's behalf. - if ( ! Recommendation_Fixes::has_fix( $provider_id ) || Recommendation_Fixes::needs_value( $provider_id ) ) { + // correct tagline to invent on the site owner's behalf. Nor can one + // that removes content, however well scoped -- an unattended run + // should never be the thing that deleted something. + if ( ! Recommendation_Fixes::has_fix( $provider_id ) + || Recommendation_Fixes::needs_value( $provider_id ) + || Recommendation_Fixes::is_confirm_only( $provider_id ) + ) { continue; } @@ -285,6 +301,7 @@ private function prepare( $task ) { // recommendations it is allowed to apply. 'fixable' => Recommendation_Fixes::has_fix( $provider_id ), 'needs_value' => Recommendation_Fixes::needs_value( $provider_id ), + 'destructive' => Recommendation_Fixes::is_destructive( $provider_id ), ]; } } diff --git a/classes/abilities/class-schemas.php b/classes/abilities/class-schemas.php index d534ec944..e9ed0aeae 100644 --- a/classes/abilities/class-schemas.php +++ b/classes/abilities/class-schemas.php @@ -217,6 +217,10 @@ public static function recommendation() { 'type' => 'boolean', 'description' => \__( 'Whether applying it requires a value from the caller, such as the tagline text.', 'progress-planner' ), ], + 'destructive' => [ + 'type' => 'boolean', + 'description' => \__( 'Whether applying it removes content rather than changing a setting. These are only applied when named explicitly, never picked automatically.', 'progress-planner' ), + ], ], ]; } diff --git a/tests/phpunit/test-class-abilities.php b/tests/phpunit/test-class-abilities.php index d1b91a377..46c76eb76 100644 --- a/tests/phpunit/test-class-abilities.php +++ b/tests/phpunit/test-class-abilities.php @@ -58,6 +58,24 @@ public function setUp(): void { $this->recommendations = new \Progress_Planner\Abilities\Recommendations(); } + /** + * Tear down test. + * + * Activities live in a custom table that WP_UnitTestCase does not roll + * back, and post IDs are reused across tests. A row left here would be seen + * by a later test that happens to be handed the same ID and asserts it has + * no activity, so this class clears what it caused. + * + * @return void + */ + public function tearDown(): void { + global $wpdb; + + $wpdb->query( 'TRUNCATE TABLE ' . $wpdb->prefix . 'progress_planner_activities' ); // phpcs:ignore WordPress.DB + + parent::tearDown(); + } + /** * Invoke a private or protected method on an object. * @@ -663,4 +681,97 @@ public function test_seo_fix_requires_its_plugin() { $this->assertWPError( $result ); $this->assertSame( 'progress_planner_seo_plugin_inactive', $result->get_error_code() ); } + + /** + * Test that the placeholder deletions are marked destructive. + * + * @return void + */ + public function test_placeholder_deletions_are_destructive() { + $this->assertTrue( Recommendation_Fixes::is_destructive( 'hello-world' ) ); + $this->assertTrue( Recommendation_Fixes::is_destructive( 'sample-page' ) ); + $this->assertFalse( Recommendation_Fixes::is_destructive( 'disable-comments' ) ); + } + + /** + * Test that anything destructive can only be applied by name. + * + * An unattended run must never be the thing that deleted something. + * + * @return void + */ + public function test_destructive_fixes_are_confirm_only() { + $this->assertTrue( Recommendation_Fixes::is_confirm_only( 'hello-world' ) ); + $this->assertTrue( Recommendation_Fixes::is_confirm_only( 'sample-page' ) ); + $this->assertFalse( Recommendation_Fixes::is_confirm_only( 'disable-comments' ) ); + } + + /** + * Test that applying the hello-world fix trashes the post rather than + * deleting it outright. + * + * @return void + */ + public function test_hello_world_fix_trashes_the_post() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + + $post_id = self::factory()->post->create( + [ + 'post_title' => 'Hello world!', + 'post_name' => 'hello-world', + 'post_status' => 'publish', + ] + ); + + $this->seed_task( 'hello-world' ); + + $result = $this->recommendations->complete( [ 'provider_id' => 'hello-world' ] ); + + $this->assertTrue( $result['applied'] ); + $this->assertSame( 'trash', \get_post_status( $post_id ), 'The post should be recoverable from the trash.' ); + } + + /** + * Test that next mode never picks a destructive fix. + * + * @return void + */ + public function test_next_mode_never_deletes() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + + $post_id = self::factory()->post->create( + [ + 'post_title' => 'Hello world!', + 'post_name' => 'hello-world', + 'post_status' => 'publish', + ] + ); + + $this->seed_task( 'hello-world' ); + + $result = $this->recommendations->complete( [] ); + + if ( null !== $result['task'] ) { + $this->assertNotSame( 'hello-world', $result['task']['provider_id'] ); + } + + $this->assertSame( 'publish', \get_post_status( $post_id ), 'Next mode must not trash anything.' ); + } + + /** + * Test that a deletion reports missing content rather than failing oddly. + * + * @return void + */ + public function test_deletion_reports_missing_target() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + + $result = Recommendation_Fixes::apply( 'hello-world' ); + + if ( \is_wp_error( $result ) ) { + $this->assertSame( 'progress_planner_no_target', $result->get_error_code() ); + } else { + $this->assertTrue( $result ); + } + } } From 24bc933c59aa91d4c7185796728be919e0ead4fc Mon Sep 17 00:00:00 2001 From: Filip Ilic Date: Fri, 18 Sep 2026 16:21:28 +0200 Subject: [PATCH 4/4] Let a caller record which page serves the About, Contact or FAQ role Brings the applicable set to 22. These recommendations do not create a page. They ask whether the site has one, and record the answer plus which page it is. The interesting part is who decides. The plugin deliberately does not search for the page. Searching a real site for "about" returns everything whose content mentions the word: on the demo site that is eleven pages, including one called "Job opening", with the actual About page ranked ninth and titled "About Emilia" at /about-us/. Deciding which of those is the About page means weighing title, slug, hierarchy and navigation in whatever language the site is written in. A caller that can read the site's pages does that well; a title match in PHP would get it wrong quietly, and worse on non-English sites. So the division of labour is: the caller identifies the page, and this verifies the ID before writing. A missing ID, an ID that does not exist, a draft, or a post that is not a page are each a distinct error rather than a silent no-op, because "we recorded your About page" is worth being true. The page type comes from the table and never from the caller, and the allowed post types are derived from the hierarchical public ones rather than hardcoded to 'page', so a site serving these roles from a custom post type still works. They need a value, so next-mode skips them: there is no correct page to choose on the owner's behalf. One caveat is recorded in is_satisfied(). These providers do not override is_task_completed(), so satisfaction now falls back to should_add_task(). That can still read pre-write state within the same request, because page-type lookups are memoised in a static cache with no invalidation -- which is why the status distinguishes "applied" from "satisfied" instead of assuming they are the same. Co-Authored-By: Claude Opus 5 (1M context) --- .../abilities/class-recommendation-fixes.php | 116 ++++++++++++++++++ classes/abilities/class-recommendations.php | 21 +++- classes/abilities/class-schemas.php | 2 +- tests/phpunit/test-class-abilities.php | 90 ++++++++++++++ 4 files changed, 226 insertions(+), 3 deletions(-) diff --git a/classes/abilities/class-recommendation-fixes.php b/classes/abilities/class-recommendation-fixes.php index 255fef69b..9c1826ffc 100644 --- a/classes/abilities/class-recommendation-fixes.php +++ b/classes/abilities/class-recommendation-fixes.php @@ -149,6 +149,39 @@ class Recommendation_Fixes { 'summary' => 'Disable author feeds in All in One SEO.', ], + /* + * Telling the plugin which existing page serves a given role. + * + * These do not create anything. The recommendation asks whether the site + * has an About page; this records the answer and, when the answer is + * yes, which page it is. + * + * The plugin deliberately does not try to find the page itself. Deciding + * that "About Emilia" at /about-us/ is the About page -- and that + * "Job opening" is not -- is a judgement about titles, slugs, navigation + * and content in whatever language the site is written in. A caller that + * can read the site's pages is far better at that than a title match + * would be, and a wrong guess made in PHP would be silent. + * + * So the division is: the caller identifies the page, and this verifies + * the ID names a real published page of an allowed type before writing. + */ + 'set-page-about' => [ + 'page_type' => 'about', + 'input' => 'value', + 'summary' => 'Record which existing page is the About page.', + ], + 'set-page-contact' => [ + 'page_type' => 'contact', + 'input' => 'value', + 'summary' => 'Record which existing page is the Contact page.', + ], + 'set-page-faq' => [ + 'page_type' => 'faq', + 'input' => 'value', + 'summary' => 'Record which existing page is the FAQ page.', + ], + /* * Deleting WordPress's own placeholder content. * @@ -256,6 +289,10 @@ public static function apply( $provider_id, $value = null ) { ); } + if ( isset( $fix['page_type'] ) ) { + return self::apply_page_type( $fix, $value ); + } + if ( isset( $fix['delete'] ) ) { return self::apply_deletion( $provider_id ); } @@ -281,6 +318,85 @@ public static function apply( $provider_id, $value = null ) { return true; } + /** + * Record which page serves a given role. + * + * The page type comes from the table; only the page ID comes from the + * caller, and it is checked before anything is written: it must name a post + * that exists, is published, and is of a post type the site treats as a + * page. An ID that does not pass is an error rather than a silent no-op, + * because "we recorded your About page" is worth being true. + * + * @param array $fix The fix definition. + * @param mixed $value The page ID supplied by the caller. + * + * @return true|\WP_Error + */ + private static function apply_page_type( array $fix, $value ) { + $page_id = \is_numeric( $value ) ? (int) $value : 0; + + if ( 1 > $page_id ) { + return new \WP_Error( + 'progress_planner_missing_page_id', + \__( 'This recommendation needs the ID of the page that serves this role.', 'progress-planner' ) + ); + } + + $page = \get_post( $page_id ); + + if ( ! $page ) { + return new \WP_Error( + 'progress_planner_no_such_page', + \__( 'There is no page with that ID.', 'progress-planner' ) + ); + } + + if ( 'publish' !== $page->post_status ) { + return new \WP_Error( + 'progress_planner_page_not_published', + \__( 'That page is not published, so it cannot serve this role yet.', 'progress-planner' ) + ); + } + + if ( ! \in_array( $page->post_type, self::page_post_types(), true ) ) { + return new \WP_Error( + 'progress_planner_not_a_page', + \__( 'That post is not a page.', 'progress-planner' ) + ); + } + + \progress_planner()->get_admin__page_settings()->set_page_values( + [ + (string) $fix['page_type'] => [ + 'id' => $page_id, + 'have_page' => 'yes', + ], + ] + ); + + return true; + } + + /** + * The post types that can serve a page role. + * + * Hierarchical public post types, which is what WordPress means by a page, + * rather than a hardcoded 'page' -- a site may serve these roles from a + * custom post type. + * + * @return array + */ + private static function page_post_types() { + $types = \get_post_types( + [ + 'public' => true, + 'hierarchical' => true, + ] + ); + + return \array_values( $types ); + } + /** * Move a placeholder post to the trash. * diff --git a/classes/abilities/class-recommendations.php b/classes/abilities/class-recommendations.php index 838985374..cd8a90570 100644 --- a/classes/abilities/class-recommendations.php +++ b/classes/abilities/class-recommendations.php @@ -185,8 +185,25 @@ private function result( $applied, $status, $message, $task ) { * @return bool */ private function is_satisfied( $provider, $task ) { - return \method_exists( $provider, 'is_task_completed' ) - && (bool) $provider->is_task_completed( $task->get_task_id() ); + if ( \method_exists( $provider, 'is_task_completed' ) + && (bool) $provider->is_task_completed( $task->get_task_id() ) + ) { + return true; + } + + // Some providers answer through should_add_task() instead: the task + // exists precisely while the condition is unmet, so "would not be added + // now" means satisfied. The set-page providers are the case in point -- + // they do not override is_task_completed() at all. + // + // This can still report false immediately after a write. Page-type + // lookups are memoised in a static cache with no invalidation, so within + // one request the check may read state from before the change. That is + // why the status distinguishes "applied" from "satisfied" rather than + // assuming the two are the same: the next request sees it correctly, and + // a caller is told what actually happened either way. + return \method_exists( $provider, 'should_add_task' ) + && false === (bool) $provider->should_add_task(); } /** diff --git a/classes/abilities/class-schemas.php b/classes/abilities/class-schemas.php index e9ed0aeae..d90809a8e 100644 --- a/classes/abilities/class-schemas.php +++ b/classes/abilities/class-schemas.php @@ -75,7 +75,7 @@ public static function complete_recommendation_input() { ], 'value' => [ 'type' => 'string', - 'description' => \__( 'The value to set, for recommendations that need one: the tagline text, a timezone identifier such as "Europe/Amsterdam", or a date format string. Recommendations with only one correct outcome ignore this.', 'progress-planner' ), + 'description' => \__( 'The value to set, for recommendations that need one: the tagline text, a timezone identifier such as "Europe/Amsterdam", a date format string, or -- for the recommendations that ask which page serves a role -- the ID of an existing published page. Recommendations with only one correct outcome ignore this. Check needs_value on a recommendation to see whether one is required.', 'progress-planner' ), ], ], ]; diff --git a/tests/phpunit/test-class-abilities.php b/tests/phpunit/test-class-abilities.php index 46c76eb76..510d619cb 100644 --- a/tests/phpunit/test-class-abilities.php +++ b/tests/phpunit/test-class-abilities.php @@ -774,4 +774,94 @@ public function test_deletion_reports_missing_target() { $this->assertTrue( $result ); } } + + /** + * Test that the page-role recommendations need a page ID. + * + * @return void + */ + public function test_page_role_fixes_need_a_value() { + foreach ( [ 'set-page-about', 'set-page-contact', 'set-page-faq' ] as $provider_id ) { + $this->assertTrue( Recommendation_Fixes::has_fix( $provider_id ) ); + $this->assertTrue( Recommendation_Fixes::needs_value( $provider_id ) ); + $this->assertFalse( Recommendation_Fixes::is_destructive( $provider_id ) ); + } + } + + /** + * Test that a valid page is recorded as serving the role. + * + * @return void + */ + public function test_page_role_records_the_page() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + + $page_id = self::factory()->post->create( + [ + 'post_title' => 'About Us', + 'post_type' => 'page', + 'post_status' => 'publish', + ] + ); + + $this->assertTrue( Recommendation_Fixes::apply( 'set-page-about', (string) $page_id ) ); + + $slugs = \wp_get_object_terms( $page_id, 'progress_planner_page_types', [ 'fields' => 'slugs' ] ); + + $this->assertContains( 'about', (array) $slugs ); + } + + /** + * Test that an unusable page ID is refused rather than recorded. + * + * "We recorded your About page" is worth being true, so each of these is an + * error rather than a silent no-op. + * + * @return void + */ + public function test_page_role_refuses_unusable_ids() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + + $cases = [ + 'progress_planner_missing_page_id' => null, + 'progress_planner_no_such_page' => '99999999', + 'progress_planner_page_not_published' => (string) self::factory()->post->create( + [ + 'post_type' => 'page', + 'post_status' => 'draft', + ] + ), + 'progress_planner_not_a_page' => (string) self::factory()->post->create( + [ + 'post_type' => 'post', + 'post_status' => 'publish', + ] + ), + ]; + + foreach ( $cases as $expected_code => $value ) { + $result = Recommendation_Fixes::apply( 'set-page-about', $value ); + + $this->assertWPError( $result ); + $this->assertSame( $expected_code, $result->get_error_code() ); + } + } + + /** + * Test that next mode never picks a page-role fix. + * + * There is no correct page to choose on the owner's behalf. + * + * @return void + */ + public function test_next_mode_skips_page_role_fixes() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + $this->seed_task( 'set-page-about' ); + + $result = $this->recommendations->complete( [] ); + + $picked = $result['task']['provider_id'] ?? null; + + $this->assertNotSame( 'set-page-about', $picked ); + } }