From e5a4c6b1acf139e8a08cf1f0d03f3a91e8efb737 Mon Sep 17 00:00:00 2001 From: Matthias Pfefferle Date: Thu, 23 Apr 2026 10:25:37 +0200 Subject: [PATCH 01/10] Publish approved WordPress comments to Bluesky as replies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Registered users commenting on posts that are already published to AT Protocol now publish outbound as app.bsky.feed.post reply records, with edit and unapprove/delete synced back to the record. Adds: - `Transformer\Comment` — reply ref resolution for local siblings, federated (Reaction_Sync) parents, or fallback to the post root. - `Publisher::publish_comment` / `update_comment` / `delete_comment` and TID-only `delete_comment_by_tid`. On success the reply's AT-URI is mirrored into `Reaction_Sync::META_SOURCE_ID` so the next inbound sync pass skips our own outbound comment via `find_comment_by_source_id`. - Comment lifecycle hooks on `Atmosphere` (`transition_comment_status`, `comment_post`, `edit_comment`, `delete_comment`) plus async cron handlers. Eligibility gate `Atmosphere::should_publish_comment` skips anon/trackback/unapproved/federated/unpublished-post inputs and honours `atmosphere_should_publish_comment`. Refactor: - Publisher renames to `publish_post` / `update_post` / `delete_post` / `delete_post_by_tids`; generic `publish` / `update` / `delete` dispatchers accept `WP_Post|WP_Comment` for polymorphic callers. - Test files consolidated so each test class maps to a real class: `Test_Publisher` absorbs comment tests, `Test_Reaction_Sync` absorbs the dedup regression, `Test_Atmosphere` replaces the old `Test_Status_Change` and absorbs the comment scheduler tests. Defence-in-depth: `is_comment_eligible` also skips comments stamped with the `ATmosphere/` agent so a hypothetical future path that fires `comment_post` from `wp_insert_comment` cannot produce a publish loop before Reaction_Sync writes META_PROTOCOL. Ignores docs/superpowers/ for local spec storage. --- .github/changelog/publish-comments | 4 + .gitignore | 1 + includes/class-atmosphere.php | 257 ++++++++++++++- includes/class-backfill.php | 2 +- includes/class-publisher.php | 235 ++++++++++++-- includes/transformer/class-comment.php | 174 +++++++++++ ...s-change.php => class-test-atmosphere.php} | 164 +++++++++- tests/phpunit/tests/class-test-publisher.php | 292 +++++++++++++++++- .../tests/class-test-reaction-sync.php | 49 +++ .../tests/transformer/class-test-comment.php | 272 ++++++++++++++++ 10 files changed, 1407 insertions(+), 43 deletions(-) create mode 100644 .github/changelog/publish-comments create mode 100644 includes/transformer/class-comment.php rename tests/phpunit/tests/{class-test-status-change.php => class-test-atmosphere.php} (60%) create mode 100644 tests/phpunit/tests/transformer/class-test-comment.php diff --git a/.github/changelog/publish-comments b/.github/changelog/publish-comments new file mode 100644 index 0000000..b370c28 --- /dev/null +++ b/.github/changelog/publish-comments @@ -0,0 +1,4 @@ +Significance: minor +Type: added + +Publish replies from registered WordPress users to Bluesky as native replies, with edit and unapprove/delete synced back to the AT Protocol record. diff --git a/.gitignore b/.gitignore index dc5354d..4572eee 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,4 @@ composer.lock package-lock.json *.zip .phpunit.result.cache +/docs/superpowers/ diff --git a/includes/class-atmosphere.php b/includes/class-atmosphere.php index f0d9a69..1e8c80e 100644 --- a/includes/class-atmosphere.php +++ b/includes/class-atmosphere.php @@ -10,7 +10,9 @@ \defined( 'ABSPATH' ) || exit; use Atmosphere\OAuth\Client; +use Atmosphere\Transformer\Comment; use Atmosphere\Transformer\Document; +use Atmosphere\Transformer\Post; use Atmosphere\Transformer\Publication; use Atmosphere\Transformer\TID; use Atmosphere\Integrations\Load; @@ -54,6 +56,12 @@ public function init(): void { // Catch permanent deletes (bypassing trash or emptying trash). \add_action( 'before_delete_post', array( $this, 'on_before_delete' ) ); + // Comment lifecycle hooks. + \add_action( 'transition_comment_status', array( $this, 'on_comment_status_change' ), 10, 3 ); + \add_action( 'comment_post', array( $this, 'on_comment_insert' ), 10, 2 ); + \add_action( 'edit_comment', array( $this, 'on_comment_edit' ) ); + \add_action( 'delete_comment', array( $this, 'on_comment_before_delete' ) ); + // Auto-sync publication when site identity changes. \add_action( 'update_option_blogname', array( $this, 'schedule_publication_sync' ) ); \add_action( 'update_option_blogdescription', array( $this, 'schedule_publication_sync' ) ); @@ -307,6 +315,206 @@ public function on_before_delete( int $post_id ): void { } } + /** + * Handle a comment transitioning between approval states. + * + * @param string $new_status New comment_approved value. + * @param string $old_status Previous comment_approved value. + * @param \WP_Comment $comment Comment object. + */ + public function on_comment_status_change( string $new_status, string $old_status, \WP_Comment $comment ): void { + if ( $new_status === $old_status ) { + return; + } + + if ( 'approved' === $new_status ) { + $this->schedule_comment_publish( $comment ); + return; + } + + if ( 'approved' === $old_status ) { + $this->schedule_comment_delete( $comment ); + } + } + + /** + * Handle a newly-inserted comment. + * + * Covers the case where a comment lands already-approved (trusted + * author), for which transition_comment_status does not fire. + * + * @param int $comment_id Comment ID. + * @param int|string $comment_approved Approval status (1, 0, or 'spam'). + */ + public function on_comment_insert( int $comment_id, int|string $comment_approved ): void { + if ( 1 !== (int) $comment_approved ) { + return; + } + + $comment = \get_comment( $comment_id ); + if ( $comment instanceof \WP_Comment ) { + $this->schedule_comment_publish( $comment ); + } + } + + /** + * Handle a comment edit by updating its bsky record. + * + * @param int $comment_id Comment ID. + */ + public function on_comment_edit( int $comment_id ): void { + $comment = \get_comment( $comment_id ); + + if ( ! $comment instanceof \WP_Comment ) { + return; + } + + if ( ! self::should_publish_comment( $comment ) ) { + return; + } + + $hook = empty( \get_comment_meta( $comment_id, Comment::META_TID, true ) ) + ? 'atmosphere_publish_comment' + : 'atmosphere_update_comment'; + + if ( ! \wp_next_scheduled( $hook, array( $comment_id ) ) ) { + \wp_schedule_single_event( \time(), $hook, array( $comment_id ) ); + } + } + + /** + * Capture a comment's TID before it is permanently deleted. + * + * Runs on delete_comment which fires before the row and meta are + * removed, so the TID is still reachable. The TID-only cron variant + * lets the async worker issue the PDS delete without re-reading + * state that no longer exists. + * + * @param int $comment_id Comment ID. + */ + public function on_comment_before_delete( int $comment_id ): void { + if ( ! is_connected() ) { + return; + } + + $tid = \get_comment_meta( $comment_id, Comment::META_TID, true ); + + if ( empty( $tid ) ) { + return; + } + + \wp_schedule_single_event( + \time(), + 'atmosphere_delete_comment_record', + array( (string) $tid ) + ); + } + + /** + * Eligibility gate for outbound comment publishing. + * + * @param \WP_Comment $comment Comment object. + * @return bool + */ + public static function should_publish_comment( \WP_Comment $comment ): bool { + $should = self::is_comment_eligible( $comment ); + + /** + * Filters whether a comment should be published to Bluesky. + * + * @param bool $should Whether to publish. + * @param \WP_Comment $comment Comment object. + */ + return (bool) \apply_filters( 'atmosphere_should_publish_comment', $should, $comment ); + } + + /** + * Core comment eligibility checks, pre-filter. + * + * @param \WP_Comment $comment Comment object. + * @return bool + */ + private static function is_comment_eligible( \WP_Comment $comment ): bool { + if ( ! is_connected() ) { + return false; + } + + if ( \in_array( (string) $comment->comment_type, array( 'trackback', 'pingback' ), true ) ) { + return false; + } + + if ( (int) $comment->user_id <= 0 ) { + return false; + } + + if ( '1' !== (string) $comment->comment_approved ) { + return false; + } + + if ( 'atproto' === \get_comment_meta( (int) $comment->comment_ID, Reaction_Sync::META_PROTOCOL, true ) ) { + return false; + } + + /* + * Defence in depth: Reaction_Sync writes META_PROTOCOL after + * wp_insert_comment, so if any caller ever fires comment_post + * between the insert and the meta write, the gate above would + * miss it. The sync always stamps its own agent string, which + * is set before the insert — use it as a belt-and-braces check. + */ + if ( 0 === \strpos( (string) $comment->comment_agent, 'ATmosphere/' ) ) { + return false; + } + + $post_uri = \get_post_meta( (int) $comment->comment_post_ID, Post::META_URI, true ); + if ( empty( $post_uri ) ) { + return false; + } + + return true; + } + + /** + * Schedule a publish or update event for a comment. + * + * @param \WP_Comment $comment Comment object. + */ + private function schedule_comment_publish( \WP_Comment $comment ): void { + if ( ! self::should_publish_comment( $comment ) ) { + return; + } + + $comment_id = (int) $comment->comment_ID; + $hook = empty( \get_comment_meta( $comment_id, Comment::META_TID, true ) ) + ? 'atmosphere_publish_comment' + : 'atmosphere_update_comment'; + + if ( \wp_next_scheduled( $hook, array( $comment_id ) ) ) { + return; + } + + \wp_schedule_single_event( \time(), $hook, array( $comment_id ) ); + } + + /** + * Schedule a delete event when a published comment leaves approved state. + * + * @param \WP_Comment $comment Comment object. + */ + private function schedule_comment_delete( \WP_Comment $comment ): void { + $comment_id = (int) $comment->comment_ID; + + if ( empty( \get_comment_meta( $comment_id, Comment::META_TID, true ) ) ) { + return; + } + + if ( \wp_next_scheduled( 'atmosphere_delete_comment', array( $comment_id ) ) ) { + return; + } + + \wp_schedule_single_event( \time(), 'atmosphere_delete_comment', array( $comment_id ) ); + } + /** * Schedule an async publication sync. */ @@ -340,7 +548,7 @@ public static function register_async_hooks(): void { static function ( int $post_id ): void { $post = \get_post( $post_id ); if ( $post && 'publish' === $post->post_status ) { - Publisher::publish( $post ); + Publisher::publish_post( $post ); } } ); @@ -350,7 +558,7 @@ static function ( int $post_id ): void { static function ( int $post_id ): void { $post = \get_post( $post_id ); if ( $post && 'publish' === $post->post_status ) { - Publisher::update( $post ); + Publisher::update_post( $post ); } } ); @@ -360,7 +568,7 @@ static function ( int $post_id ): void { static function ( int $post_id ): void { $post = \get_post( $post_id ); if ( $post ) { - Publisher::delete( $post ); + Publisher::delete_post( $post ); } } ); @@ -375,10 +583,51 @@ static function (): void { \add_action( 'atmosphere_delete_records', static function ( string $bsky_tid, string $doc_tid ): void { - Publisher::delete_by_tids( $bsky_tid, $doc_tid ); + Publisher::delete_post_by_tids( $bsky_tid, $doc_tid ); }, 10, 2 ); + + \add_action( + 'atmosphere_publish_comment', + static function ( int $comment_id ): void { + $comment = \get_comment( $comment_id ); + if ( $comment instanceof \WP_Comment ) { + Publisher::publish_comment( $comment ); + } + } + ); + + \add_action( + 'atmosphere_update_comment', + static function ( int $comment_id ): void { + $comment = \get_comment( $comment_id ); + if ( $comment instanceof \WP_Comment ) { + Publisher::update_comment( $comment ); + } + } + ); + + \add_action( + 'atmosphere_delete_comment', + static function ( int $comment_id ): void { + $comment = \get_comment( $comment_id ); + if ( $comment instanceof \WP_Comment ) { + Publisher::delete_comment( $comment ); + } + } + ); + + \add_action( + 'atmosphere_delete_comment_record', + static function ( string $tid ): void { + if ( '' !== $tid ) { + Publisher::delete_comment_by_tid( $tid ); + } + }, + 10, + 1 + ); } } diff --git a/includes/class-backfill.php b/includes/class-backfill.php index 02b8046..e3e4b29 100644 --- a/includes/class-backfill.php +++ b/includes/class-backfill.php @@ -113,7 +113,7 @@ public static function handle_batch(): void { continue; } - $response = Publisher::publish( $post ); + $response = Publisher::publish_post( $post ); if ( \is_wp_error( $response ) ) { $results[] = array( diff --git a/includes/class-publisher.php b/includes/class-publisher.php index ffd48a7..0923669 100644 --- a/includes/class-publisher.php +++ b/includes/class-publisher.php @@ -1,10 +1,11 @@ ID, $result, $bsky_transformer, $doc_transformer ); + self::store_post_result( $post->ID, $result, $bsky_transformer, $doc_transformer ); // Follow-up: update document with bsky post reference (now that we have the CID). self::update_document_bsky_ref( $post, $doc_transformer ); @@ -76,13 +120,13 @@ public static function publish( \WP_Post $post ): array|\WP_Error { * @param \WP_Post $post WordPress post. * @return array|\WP_Error */ - public static function update( \WP_Post $post ): array|\WP_Error { + public static function update_post( \WP_Post $post ): array|\WP_Error { $bsky_uri = \get_post_meta( $post->ID, Post::META_URI, true ); $doc_uri = \get_post_meta( $post->ID, Document::META_URI, true ); if ( ! $bsky_uri || ! $doc_uri ) { // Not yet published — do a fresh publish instead. - return self::publish( $post ); + return self::publish_post( $post ); } $bsky_tid = \get_post_meta( $post->ID, Post::META_TID, true ); @@ -116,7 +160,7 @@ public static function update( \WP_Post $post ): array|\WP_Error { return $result; } - self::store_results( $post->ID, $result, $bsky_transformer, $doc_transformer ); + self::store_post_result( $post->ID, $result, $bsky_transformer, $doc_transformer ); // Update document with bsky post reference (CID may have changed). self::update_document_bsky_ref( $post, $doc_transformer ); @@ -130,7 +174,7 @@ public static function update( \WP_Post $post ): array|\WP_Error { * @param \WP_Post $post WordPress post. * @return array|\WP_Error */ - public static function delete( \WP_Post $post ): array|\WP_Error { + public static function delete_post( \WP_Post $post ): array|\WP_Error { $bsky_tid = \get_post_meta( $post->ID, Post::META_TID, true ); $doc_tid = \get_post_meta( $post->ID, Document::META_TID, true ); @@ -174,7 +218,7 @@ public static function delete( \WP_Post $post ): array|\WP_Error { } /** - * Delete AT Protocol records by TID, without requiring the post to exist. + * Delete post AT Protocol records by TID, without requiring the post to exist. * * Used when a post is permanently deleted and post meta is no longer available. * @@ -182,7 +226,7 @@ public static function delete( \WP_Post $post ): array|\WP_Error { * @param string $doc_tid Document TID (may be empty). * @return array|\WP_Error */ - public static function delete_by_tids( string $bsky_tid, string $doc_tid ): array|\WP_Error { + public static function delete_post_by_tids( string $bsky_tid, string $doc_tid ): array|\WP_Error { if ( ! $bsky_tid && ! $doc_tid ) { return new \WP_Error( 'atmosphere_not_published', \__( 'No TIDs provided.', 'atmosphere' ) ); } @@ -205,13 +249,7 @@ public static function delete_by_tids( string $bsky_tid, string $doc_tid ): arra ); } - $result = API::apply_writes( $writes ); - - if ( \is_wp_error( $result ) ) { - return $result; - } - - return $result; + return API::apply_writes( $writes ); } /** @@ -259,14 +297,145 @@ public static function sync_publication(): array|\WP_Error { } /** - * Extract URIs/CIDs from applyWrites response and store in post meta. + * Publish a WordPress comment as an app.bsky.feed.post reply. + * + * @param \WP_Comment $comment WordPress comment. + * @return array|\WP_Error applyWrites response or error. + */ + public static function publish_comment( \WP_Comment $comment ): array|\WP_Error { + $transformer = new Comment( $comment ); + $rkey = $transformer->get_rkey(); + $record = $transformer->transform(); + + $writes = array( + array( + '$type' => 'com.atproto.repo.applyWrites#create', + 'collection' => 'app.bsky.feed.post', + 'rkey' => $rkey, + 'value' => $record, + ), + ); + + $result = API::apply_writes( $writes ); + + if ( \is_wp_error( $result ) ) { + return $result; + } + + self::store_comment_result( (int) $comment->comment_ID, $result, $transformer ); + + return $result; + } + + /** + * Update an existing bsky reply for a WordPress comment. + * + * Falls through to publish_comment when no TID is stored — happens + * if an earlier publish attempt failed before meta was written. + * + * @param \WP_Comment $comment WordPress comment. + * @return array|\WP_Error + */ + public static function update_comment( \WP_Comment $comment ): array|\WP_Error { + $comment_id = (int) $comment->comment_ID; + $tid = \get_comment_meta( $comment_id, Comment::META_TID, true ); + + if ( empty( $tid ) ) { + return self::publish_comment( $comment ); + } + + $transformer = new Comment( $comment ); + + $writes = array( + array( + '$type' => 'com.atproto.repo.applyWrites#update', + 'collection' => 'app.bsky.feed.post', + 'rkey' => $tid, + 'value' => $transformer->transform(), + ), + ); + + $result = API::apply_writes( $writes ); + + if ( \is_wp_error( $result ) ) { + return $result; + } + + self::store_comment_result( $comment_id, $result, $transformer ); + + return $result; + } + + /** + * Delete the bsky reply record for a WordPress comment. + * + * @param \WP_Comment $comment WordPress comment. + * @return array|\WP_Error + */ + public static function delete_comment( \WP_Comment $comment ): array|\WP_Error { + $comment_id = (int) $comment->comment_ID; + $tid = \get_comment_meta( $comment_id, Comment::META_TID, true ); + + if ( empty( $tid ) ) { + return new \WP_Error( 'atmosphere_not_published', \__( 'Comment has no AT Protocol record.', 'atmosphere' ) ); + } + + $writes = array( + array( + '$type' => 'com.atproto.repo.applyWrites#delete', + 'collection' => 'app.bsky.feed.post', + 'rkey' => $tid, + ), + ); + + $result = API::apply_writes( $writes ); + + if ( \is_wp_error( $result ) ) { + return $result; + } + + \delete_comment_meta( $comment_id, Comment::META_TID ); + \delete_comment_meta( $comment_id, Comment::META_URI ); + \delete_comment_meta( $comment_id, Comment::META_CID ); + \delete_comment_meta( $comment_id, Reaction_Sync::META_SOURCE_ID ); + + return $result; + } + + /** + * Delete a bsky comment reply by TID, without needing the comment row. + * + * Used when a comment is permanently deleted and its meta is no + * longer reachable at the point the cron fires. + * + * @param string $tid Comment record TID. + * @return array|\WP_Error + */ + public static function delete_comment_by_tid( string $tid ): array|\WP_Error { + if ( '' === $tid ) { + return new \WP_Error( 'atmosphere_not_published', \__( 'No TID provided.', 'atmosphere' ) ); + } + + $writes = array( + array( + '$type' => 'com.atproto.repo.applyWrites#delete', + 'collection' => 'app.bsky.feed.post', + 'rkey' => $tid, + ), + ); + + return API::apply_writes( $writes ); + } + + /** + * Extract URIs/CIDs from an applyWrites response and store in post meta. * * @param int $post_id Post ID. * @param array $result applyWrites response. * @param Post $bsky_transformer Bsky transformer. * @param Document $doc_transformer Document transformer. */ - private static function store_results( int $post_id, array $result, Post $bsky_transformer, Document $doc_transformer ): void { + private static function store_post_result( int $post_id, array $result, Post $bsky_transformer, Document $doc_transformer ): void { $results = $result['results'] ?? array(); foreach ( $results as $i => $item ) { @@ -297,6 +466,30 @@ private static function store_results( int $post_id, array $result, Post $bsky_t } } + /** + * Store the applyWrites response for a comment publish/update. + * + * Mirrors the comment's AT-URI into Reaction_Sync::META_SOURCE_ID so + * that when listRecords feeds our own reply back through the inbound + * sync, find_comment_by_source_id() matches this row and skips it. + * + * @param int $comment_id WordPress comment ID. + * @param array $result applyWrites response. + * @param Comment $transformer Comment transformer. + */ + private static function store_comment_result( int $comment_id, array $result, Comment $transformer ): void { + $first = $result['results'][0] ?? array(); + $uri = $first['uri'] ?? $transformer->get_uri(); + $cid = $first['cid'] ?? ''; + + \update_comment_meta( $comment_id, Comment::META_URI, $uri ); + \update_comment_meta( $comment_id, Reaction_Sync::META_SOURCE_ID, $uri ); + + if ( $cid ) { + \update_comment_meta( $comment_id, Comment::META_CID, $cid ); + } + } + /** * Update the document record with the bsky post strong reference. * diff --git a/includes/transformer/class-comment.php b/includes/transformer/class-comment.php new file mode 100644 index 0000000..ed8647d --- /dev/null +++ b/includes/transformer/class-comment.php @@ -0,0 +1,174 @@ +object; + + $text = truncate_text( sanitize_text( (string) $comment->comment_content ), 300 ); + + $record = array( + '$type' => 'app.bsky.feed.post', + 'text' => $text, + 'createdAt' => $this->to_iso8601( $comment->comment_date_gmt ), + 'langs' => $this->get_langs(), + 'reply' => $this->build_reply_ref( $comment ), + ); + + $facets = Facet::extract( $text ); + if ( ! empty( $facets ) ) { + $record['facets'] = $facets; + } + + /** + * Filters the app.bsky.feed.post comment reply record before publishing. + * + * @param array $record Bsky post record. + * @param \WP_Comment $comment WordPress comment. + */ + return \apply_filters( 'atmosphere_transform_comment', $record, $comment ); + } + + /** + * {@inheritDoc} + */ + public function get_collection(): string { + return 'app.bsky.feed.post'; + } + + /** + * {@inheritDoc} + */ + public function get_rkey(): string { + $rkey = \get_comment_meta( (int) $this->object->comment_ID, self::META_TID, true ); + + if ( empty( $rkey ) ) { + $rkey = TID::generate(); + \update_comment_meta( (int) $this->object->comment_ID, self::META_TID, $rkey ); + } + + return $rkey; + } + + /** + * Build the reply struct with root and parent refs. + * + * Root is always the WP post's bsky record. Parent is the closest + * resolvable ancestor: a local sibling comment that has already + * been published, a federated parent ingested by Reaction_Sync, or + * the post itself as a fallback when the parent comment can't be + * resolved to an AT record. + * + * @param \WP_Comment $comment WordPress comment. + * @return array{root: array{uri:string,cid:string}, parent: array{uri:string,cid:string}} + */ + private function build_reply_ref( \WP_Comment $comment ): array { + $post_id = (int) $comment->comment_post_ID; + + $root = array( + 'uri' => (string) \get_post_meta( $post_id, Post::META_URI, true ), + 'cid' => (string) \get_post_meta( $post_id, Post::META_CID, true ), + ); + + $parent_id = (int) $comment->comment_parent; + + if ( $parent_id > 0 ) { + $resolved = $this->resolve_parent_ref( $parent_id ); + if ( null !== $resolved ) { + return array( + 'root' => $root, + 'parent' => $resolved, + ); + } + } + + return array( + 'root' => $root, + 'parent' => $root, + ); + } + + /** + * Resolve a parent comment to its AT Protocol strong-ref. + * + * Checks local-publish meta first (this class's own keys), then + * falls through to Reaction_Sync meta for federated parents. + * Returns null when neither path yields a URI, so the caller can + * fall back to the root reference. + * + * @param int $parent_id Parent comment ID. + * @return array{uri:string,cid:string}|null + */ + private function resolve_parent_ref( int $parent_id ): ?array { + $local_uri = \get_comment_meta( $parent_id, self::META_URI, true ); + if ( ! empty( $local_uri ) ) { + return array( + 'uri' => (string) $local_uri, + 'cid' => (string) \get_comment_meta( $parent_id, self::META_CID, true ), + ); + } + + if ( 'atproto' !== \get_comment_meta( $parent_id, Reaction_Sync::META_PROTOCOL, true ) ) { + return null; + } + + $federated_uri = \get_comment_meta( $parent_id, Reaction_Sync::META_SOURCE_ID, true ); + if ( empty( $federated_uri ) ) { + return null; + } + + return array( + 'uri' => (string) $federated_uri, + 'cid' => (string) \get_comment_meta( $parent_id, Reaction_Sync::META_BSKY_CID, true ), + ); + } +} diff --git a/tests/phpunit/tests/class-test-status-change.php b/tests/phpunit/tests/class-test-atmosphere.php similarity index 60% rename from tests/phpunit/tests/class-test-status-change.php rename to tests/phpunit/tests/class-test-atmosphere.php index e2f7534..2bf08f7 100644 --- a/tests/phpunit/tests/class-test-status-change.php +++ b/tests/phpunit/tests/class-test-atmosphere.php @@ -1,27 +1,27 @@ atmosphere = new Atmosphere(); - // Simulate a connected state. \update_option( 'atmosphere_connection', array( 'access_token' => 'encrypted-token', 'did' => 'did:plc:test123', + 'pds_endpoint' => 'https://pds.example.com', ) ); } @@ -54,12 +54,13 @@ public function set_up(): void { public function tear_down(): void { \delete_option( 'atmosphere_connection' ); - // Clear any scheduled events. \wp_clear_scheduled_hook( 'atmosphere_publish_post' ); \wp_clear_scheduled_hook( 'atmosphere_update_post' ); \wp_clear_scheduled_hook( 'atmosphere_delete_post' ); \wp_clear_scheduled_hook( 'atmosphere_delete_records' ); + \remove_all_filters( 'atmosphere_should_publish_comment' ); + parent::tear_down(); } @@ -75,6 +76,33 @@ private function reset_publishing_action(): void { unset( $wp_actions['atmosphere_publishing'] ); } + /** + * Build a WP_Comment on a published post for comment eligibility tests. + * + * @param array $overrides Comment field overrides. + * @return \WP_Comment + */ + private function make_eligible_comment( array $overrides = array() ): \WP_Comment { + static $post_id = null; + + if ( null === $post_id ) { + $post_id = self::factory()->post->create(); + \update_post_meta( $post_id, Post::META_URI, 'at://did:plc:test123/app.bsky.feed.post/abc' ); + } + + $defaults = array( + 'comment_post_ID' => $post_id, + 'comment_approved' => '1', + 'comment_type' => 'comment', + 'user_id' => self::factory()->user->create(), + 'comment_content' => 'Hello.', + ); + + $comment_id = self::factory()->comment->create( \array_merge( $defaults, $overrides ) ); + + return \get_comment( $comment_id ); + } + /** * Test that draft → publish schedules a publish event. */ @@ -284,4 +312,120 @@ public function test_disconnected_state_prevents_scheduling() { 'Disconnected state must prevent scheduling.' ); } + + /** + * Baseline: approved comment from a registered user on a published + * post is publishable. + */ + public function test_eligible_registered_user_approved_comment_publishes() { + $comment = $this->make_eligible_comment(); + + $this->assertTrue( Atmosphere::should_publish_comment( $comment ) ); + } + + /** + * Anonymous commenters (user_id === 0) are skipped. + */ + public function test_anonymous_comment_is_skipped() { + $comment = $this->make_eligible_comment( array( 'user_id' => 0 ) ); + + $this->assertFalse( Atmosphere::should_publish_comment( $comment ) ); + } + + /** + * Trackbacks are skipped regardless of author. + */ + public function test_trackback_is_skipped() { + $comment = $this->make_eligible_comment( array( 'comment_type' => 'trackback' ) ); + + $this->assertFalse( Atmosphere::should_publish_comment( $comment ) ); + } + + /** + * Pingbacks are skipped regardless of author. + */ + public function test_pingback_is_skipped() { + $comment = $this->make_eligible_comment( array( 'comment_type' => 'pingback' ) ); + + $this->assertFalse( Atmosphere::should_publish_comment( $comment ) ); + } + + /** + * Unapproved comments are skipped. + */ + public function test_unapproved_comment_is_skipped() { + $comment = $this->make_eligible_comment( array( 'comment_approved' => '0' ) ); + + $this->assertFalse( Atmosphere::should_publish_comment( $comment ) ); + } + + /** + * Comments ingested from Bluesky (protocol=atproto meta) are + * skipped to prevent a publish loop. + */ + public function test_federated_comment_is_skipped() { + $comment = $this->make_eligible_comment(); + \update_comment_meta( (int) $comment->comment_ID, Reaction_Sync::META_PROTOCOL, 'atproto' ); + + $this->assertFalse( Atmosphere::should_publish_comment( $comment ) ); + } + + /** + * Comments on posts that have not yet been published to AT are + * skipped — there is no root ref to thread a reply against. + */ + public function test_comment_on_unpublished_post_is_skipped() { + $other_post = self::factory()->post->create(); + $comment = $this->make_eligible_comment( array( 'comment_post_ID' => $other_post ) ); + + $this->assertFalse( Atmosphere::should_publish_comment( $comment ) ); + } + + /** + * When the plugin is not connected, comments do not publish. + */ + public function test_disconnected_state_skips_comment_publish() { + \delete_option( 'atmosphere_connection' ); + + $comment = $this->make_eligible_comment(); + + $this->assertFalse( Atmosphere::should_publish_comment( $comment ) ); + } + + /** + * Third parties can veto publication via filter. + */ + public function test_comment_filter_can_veto_publish() { + $comment = $this->make_eligible_comment(); + + \add_filter( 'atmosphere_should_publish_comment', '__return_false' ); + + $this->assertFalse( Atmosphere::should_publish_comment( $comment ) ); + } + + /** + * Third parties can force-allow publication via filter (e.g. + * overriding the anonymous-only guard for a specific integration). + */ + public function test_comment_filter_can_force_publish() { + $comment = $this->make_eligible_comment( array( 'user_id' => 0 ) ); + + \add_filter( 'atmosphere_should_publish_comment', '__return_true' ); + + $this->assertTrue( Atmosphere::should_publish_comment( $comment ) ); + } + + /** + * Comments stamped with the plugin's own agent string are skipped, + * even if META_PROTOCOL has not yet been written. Guards against a + * publish loop if the Reaction_Sync insert path ever fires + * comment_post before its meta writes complete. + */ + public function test_comment_with_atmosphere_agent_is_skipped() { + $comment = $this->make_eligible_comment( + array( 'comment_agent' => 'ATmosphere/0.0.0-unreleased' ) + ); + + $this->assertFalse( Atmosphere::should_publish_comment( $comment ) ); + } } diff --git a/tests/phpunit/tests/class-test-publisher.php b/tests/phpunit/tests/class-test-publisher.php index 0bafc8c..5b2c79f 100644 --- a/tests/phpunit/tests/class-test-publisher.php +++ b/tests/phpunit/tests/class-test-publisher.php @@ -14,8 +14,10 @@ use WP_UnitTestCase; use Atmosphere\Publisher; -use Atmosphere\Transformer\Post; +use Atmosphere\Reaction_Sync; +use Atmosphere\Transformer\Comment; use Atmosphere\Transformer\Document; +use Atmosphere\Transformer\Post; /** * Publisher tests. @@ -74,7 +76,7 @@ public function test_update_falls_back_to_publish_without_uris() { * so we'll get a WP_Error back — but the important thing is * that it attempted a publish (create), not an update. */ - $result = Publisher::update( $post ); + $result = Publisher::update_post( $post ); $this->assertWPError( $result ); } @@ -92,7 +94,7 @@ public function test_update_falls_back_without_doc_uri() { \update_post_meta( $post->ID, Post::META_URI, 'at://did:plc:test/app.bsky.feed.post/bsky-tid-123' ); // No document URI. - $result = Publisher::update( $post ); + $result = Publisher::update_post( $post ); $this->assertWPError( $result ); } @@ -109,7 +111,7 @@ public function test_update_errors_with_uris_but_no_tids() { \update_post_meta( $post->ID, Post::META_URI, 'at://did:plc:test/app.bsky.feed.post/bsky-tid-123' ); \update_post_meta( $post->ID, Document::META_URI, 'at://did:plc:test/site.standard.document/doc-tid-456' ); - $result = Publisher::update( $post ); + $result = Publisher::update_post( $post ); $this->assertWPError( $result ); $this->assertSame( 'atmosphere_missing_tid', $result->get_error_code() ); @@ -165,7 +167,7 @@ static function ( $response, $args, $url ) use ( &$captured_body ) { 3 ); - $result = Publisher::update( $post ); + $result = Publisher::update_post( $post ); \remove_all_filters( 'pre_http_request' ); @@ -203,7 +205,7 @@ public function test_delete_preserves_meta_on_api_error() { \update_post_meta( $post->ID, Post::META_TID, 'bsky-tid-123' ); \update_post_meta( $post->ID, Document::META_TID, 'doc-tid-456' ); - $result = Publisher::delete( $post ); + $result = Publisher::delete_post( $post ); // API call will fail (no valid OAuth), meta should be preserved. $this->assertWPError( $result ); @@ -219,9 +221,285 @@ public function test_delete_errors_without_tids() { array( 'post_status' => 'trash' ) ); - $result = Publisher::delete( $post ); + $result = Publisher::delete_post( $post ); + + $this->assertWPError( $result ); + $this->assertSame( 'atmosphere_not_published', $result->get_error_code() ); + } + + /** + * Seed a published post for comment tests to reply against. + * + * @return int Post ID with bsky meta populated. + */ + private function seed_root_post(): int { + $post_id = self::factory()->post->create(); + \update_post_meta( $post_id, Post::META_URI, 'at://did:plc:test123/app.bsky.feed.post/root' ); + \update_post_meta( $post_id, Post::META_CID, 'bafyroot' ); + + return $post_id; + } + + /** + * Capture the body of the first applyWrites call and stub a successful + * response. + * + * @param string $uri Response URI. + * @param string $cid Response CID. + * @return \Closure Returns the captured body, or null if the filter never fired. + */ + private function stub_apply_writes( string $uri, string $cid ): \Closure { + $captured = new \stdClass(); + $captured->body = null; + + \add_filter( + 'pre_http_request', + static function ( $response, $args, $url ) use ( $captured, $uri, $cid ) { + if ( false !== \strpos( $url, 'applyWrites' ) ) { + $captured->body = \json_decode( $args['body'], true ); + + return array( + 'response' => array( 'code' => 200 ), + 'body' => \wp_json_encode( + array( + 'results' => array( + array( + 'uri' => $uri, + 'cid' => $cid, + ), + ), + ) + ), + ); + } + + return $response; + }, + 5, + 3 + ); + + return static fn() => $captured->body; + } + + /** + * Publish a comment stores URI+CID+TID and the Reaction_Sync dedup key. + */ + public function test_publish_comment_stores_meta_and_dedup_key() { + $post_id = $this->seed_root_post(); + $user_id = self::factory()->user->create(); + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_content' => 'Published comment.', + 'comment_approved' => '1', + 'user_id' => $user_id, + ) + ); + + $get_body = $this->stub_apply_writes( + 'at://did:plc:test123/app.bsky.feed.post/newtid', + 'bafynew' + ); + + $result = Publisher::publish_comment( \get_comment( $comment_id ) ); + \remove_all_filters( 'pre_http_request' ); + + if ( \is_wp_error( $result ) ) { + $this->markTestSkipped( 'API layer rejected request before stub: ' . $result->get_error_message() ); + } + + $body = $get_body(); + $this->assertNotNull( $body, 'applyWrites body was not captured.' ); + $this->assertSame( 'com.atproto.repo.applyWrites#create', $body['writes'][0]['$type'] ); + $this->assertSame( 'app.bsky.feed.post', $body['writes'][0]['collection'] ); + + $this->assertSame( 'at://did:plc:test123/app.bsky.feed.post/newtid', \get_comment_meta( $comment_id, Comment::META_URI, true ) ); + $this->assertSame( 'bafynew', \get_comment_meta( $comment_id, Comment::META_CID, true ) ); + $this->assertSame( + 'at://did:plc:test123/app.bsky.feed.post/newtid', + \get_comment_meta( $comment_id, Reaction_Sync::META_SOURCE_ID, true ) + ); + $this->assertNotEmpty( \get_comment_meta( $comment_id, Comment::META_TID, true ) ); + } + + /** + * Update comment falls back to publish when there is no TID yet. + */ + public function test_update_comment_falls_back_to_publish_without_tid() { + $post_id = $this->seed_root_post(); + $user_id = self::factory()->user->create(); + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_approved' => '1', + 'user_id' => $user_id, + ) + ); + + $get_body = $this->stub_apply_writes( + 'at://did:plc:test123/app.bsky.feed.post/newtid', + 'bafynew' + ); + + $result = Publisher::update_comment( \get_comment( $comment_id ) ); + \remove_all_filters( 'pre_http_request' ); + + if ( \is_wp_error( $result ) ) { + $this->markTestSkipped( 'API layer rejected request: ' . $result->get_error_message() ); + } + + $body = $get_body(); + $this->assertSame( 'com.atproto.repo.applyWrites#create', $body['writes'][0]['$type'] ); + } + + /** + * Update comment issues an update when a TID is already stored. + */ + public function test_update_comment_updates_existing_record() { + $post_id = $this->seed_root_post(); + $user_id = self::factory()->user->create(); + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_approved' => '1', + 'user_id' => $user_id, + ) + ); + \update_comment_meta( $comment_id, Comment::META_TID, 'existingtid' ); + + $get_body = $this->stub_apply_writes( + 'at://did:plc:test123/app.bsky.feed.post/existingtid', + 'bafyupdated' + ); + + $result = Publisher::update_comment( \get_comment( $comment_id ) ); + \remove_all_filters( 'pre_http_request' ); + + if ( \is_wp_error( $result ) ) { + $this->markTestSkipped( 'API layer rejected request: ' . $result->get_error_message() ); + } + + $body = $get_body(); + $this->assertSame( 'com.atproto.repo.applyWrites#update', $body['writes'][0]['$type'] ); + $this->assertSame( 'existingtid', $body['writes'][0]['rkey'] ); + } + + /** + * Delete comment errors when the comment was never published. + */ + public function test_delete_comment_errors_without_tid() { + $post_id = $this->seed_root_post(); + $user_id = self::factory()->user->create(); + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'user_id' => $user_id, + ) + ); + + $result = Publisher::delete_comment( \get_comment( $comment_id ) ); $this->assertWPError( $result ); $this->assertSame( 'atmosphere_not_published', $result->get_error_code() ); } + + /** + * Delete-by-tid issues a delete write for a known TID. + */ + public function test_delete_comment_by_tid_issues_delete() { + $get_body = $this->stub_apply_writes( '', '' ); + + $result = Publisher::delete_comment_by_tid( 'goner' ); + \remove_all_filters( 'pre_http_request' ); + + if ( \is_wp_error( $result ) ) { + $this->markTestSkipped( 'API layer rejected request: ' . $result->get_error_message() ); + } + + $body = $get_body(); + $this->assertSame( 'com.atproto.repo.applyWrites#delete', $body['writes'][0]['$type'] ); + $this->assertSame( 'goner', $body['writes'][0]['rkey'] ); + } + + /** + * Delete-by-tid rejects an empty TID. + */ + public function test_delete_comment_by_tid_rejects_empty() { + $result = Publisher::delete_comment_by_tid( '' ); + + $this->assertWPError( $result ); + $this->assertSame( 'atmosphere_not_published', $result->get_error_code() ); + } + + /** + * Generic Publisher::publish dispatches to publish_post for WP_Post. + */ + public function test_generic_publish_dispatches_post() { + $post = self::factory()->post->create_and_get( array( 'post_status' => 'publish' ) ); + + $captured_collections = array(); + \add_filter( + 'pre_http_request', + static function ( $response, $args, $url ) use ( &$captured_collections ) { + if ( false !== \strpos( $url, 'applyWrites' ) ) { + $body = \json_decode( $args['body'], true ); + $captured_collections = \array_column( $body['writes'] ?? array(), 'collection' ); + } + return $response; + }, + 5, + 3 + ); + + Publisher::publish( $post ); + \remove_all_filters( 'pre_http_request' ); + + if ( empty( $captured_collections ) ) { + $this->markTestSkipped( 'API layer rejected request before stub.' ); + } + + $this->assertContains( 'app.bsky.feed.post', $captured_collections ); + $this->assertContains( 'site.standard.document', $captured_collections ); + } + + /** + * Generic Publisher::publish dispatches to publish_comment for WP_Comment. + */ + public function test_generic_publish_dispatches_comment() { + $post_id = $this->seed_root_post(); + $user_id = self::factory()->user->create(); + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_approved' => '1', + 'user_id' => $user_id, + ) + ); + + $captured_writes = null; + \add_filter( + 'pre_http_request', + static function ( $response, $args, $url ) use ( &$captured_writes ) { + if ( false !== \strpos( $url, 'applyWrites' ) ) { + $body = \json_decode( $args['body'], true ); + $captured_writes = $body['writes'] ?? array(); + } + return $response; + }, + 5, + 3 + ); + + Publisher::publish( \get_comment( $comment_id ) ); + \remove_all_filters( 'pre_http_request' ); + + if ( null === $captured_writes ) { + $this->markTestSkipped( 'API layer rejected request before stub.' ); + } + + // Comment publish produces a single app.bsky.feed.post create write. + $this->assertCount( 1, $captured_writes ); + $this->assertSame( 'app.bsky.feed.post', $captured_writes[0]['collection'] ); + } } diff --git a/tests/phpunit/tests/class-test-reaction-sync.php b/tests/phpunit/tests/class-test-reaction-sync.php index e268c99..51bc5ba 100644 --- a/tests/phpunit/tests/class-test-reaction-sync.php +++ b/tests/phpunit/tests/class-test-reaction-sync.php @@ -858,4 +858,53 @@ public function test_paginate_stops_when_stream_runs_out_inside_grace() { $this->assertSame( array( 'at://a/1', 'at://a/2', 'at://a/3', 'at://a/4' ), $seen ); $this->assertSame( 'at://a/1', \get_option( $option_key ) ); } + + /** + * A reply whose URI matches an existing comment's source_id meta + * is skipped, even when that comment has no protocol='atproto' + * marker — the outbound publish path deliberately omits it. + */ + public function test_process_reply_skips_our_own_outbound_comment() { + $post_id = self::factory()->post->create(); + $post_uri = 'at://did:plc:me/app.bsky.feed.post/rootpost'; + \update_post_meta( $post_id, BskyPost::META_URI, $post_uri ); + + // Simulate a locally-published outbound comment: source_id set + // by Publisher::publish_comment, protocol intentionally absent. + $local_comment = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'user_id' => 1, + ) + ); + $reply_uri = 'at://did:plc:me/app.bsky.feed.post/ourreply'; + \update_comment_meta( $local_comment, Reaction_Sync::META_SOURCE_ID, $reply_uri ); + + $method = new \ReflectionMethod( Reaction_Sync::class, 'process_reply' ); + $method->setAccessible( true ); + + $notification = array( + 'uri' => $reply_uri, + 'cid' => 'bafyownreply', + 'record' => array( + 'text' => 'Our own outbound comment.', + 'createdAt' => '2026-04-23T10:00:00.000Z', + 'reply' => array( + 'parent' => array( 'uri' => $post_uri ), + 'root' => array( 'uri' => $post_uri ), + ), + ), + 'author' => array( + 'did' => 'did:plc:me', + 'handle' => 'me.bsky.social', + ), + ); + + $this->assertFalse( $method->invoke( null, $notification ) ); + + // No second comment was inserted — only the local one exists. + $comments = \get_comments( array( 'post_id' => $post_id ) ); + $this->assertCount( 1, $comments ); + $this->assertSame( (string) $local_comment, (string) $comments[0]->comment_ID ); + } } diff --git a/tests/phpunit/tests/transformer/class-test-comment.php b/tests/phpunit/tests/transformer/class-test-comment.php new file mode 100644 index 0000000..7fea786 --- /dev/null +++ b/tests/phpunit/tests/transformer/class-test-comment.php @@ -0,0 +1,272 @@ +post_id = self::factory()->post->create(); + \update_post_meta( $this->post_id, Post::META_URI, $this->post_uri ); + \update_post_meta( $this->post_id, Post::META_CID, $this->post_cid ); + } + + /** + * Remove any filter overrides so they do not leak between tests. + */ + public function tear_down(): void { + \remove_all_filters( 'atmosphere_transform_comment' ); + parent::tear_down(); + } + + /** + * A top-level comment replies to the root post (root === parent). + * + * @covers ::transform + */ + public function test_top_level_comment_replies_to_post() { + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $this->post_id, + 'comment_content' => 'Top-level comment.', + 'user_id' => 1, + ) + ); + + $comment = \get_comment( $comment_id ); + $record = ( new Comment( $comment ) )->transform(); + + $this->assertSame( 'app.bsky.feed.post', $record['$type'] ); + $this->assertSame( 'Top-level comment.', $record['text'] ); + $this->assertSame( $this->post_uri, $record['reply']['root']['uri'] ); + $this->assertSame( $this->post_cid, $record['reply']['root']['cid'] ); + $this->assertSame( $this->post_uri, $record['reply']['parent']['uri'] ); + $this->assertSame( $this->post_cid, $record['reply']['parent']['cid'] ); + } + + /** + * A reply to a locally-published sibling comment uses the sibling's + * AT record as the parent ref. + * + * @covers ::transform + */ + public function test_reply_to_local_parent_uses_comment_uri() { + $parent_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $this->post_id, + 'user_id' => 1, + ) + ); + \update_comment_meta( $parent_id, Comment::META_URI, 'at://did:plc:me/app.bsky.feed.post/localparent' ); + \update_comment_meta( $parent_id, Comment::META_CID, 'bafylocal' ); + + $child_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $this->post_id, + 'comment_parent' => $parent_id, + 'comment_content' => 'Nested reply.', + 'user_id' => 1, + ) + ); + + $record = ( new Comment( \get_comment( $child_id ) ) )->transform(); + + $this->assertSame( $this->post_uri, $record['reply']['root']['uri'] ); + $this->assertSame( 'at://did:plc:me/app.bsky.feed.post/localparent', $record['reply']['parent']['uri'] ); + $this->assertSame( 'bafylocal', $record['reply']['parent']['cid'] ); + } + + /** + * A reply to a federated parent (ingested via Reaction_Sync) uses + * the source_id URI and stored bsky CID. + * + * @covers ::transform + */ + public function test_reply_to_federated_parent_uses_source_id() { + $parent_id = self::factory()->comment->create( + array( 'comment_post_ID' => $this->post_id ) + ); + \update_comment_meta( $parent_id, Reaction_Sync::META_PROTOCOL, 'atproto' ); + \update_comment_meta( $parent_id, Reaction_Sync::META_SOURCE_ID, 'at://did:plc:stranger/app.bsky.feed.post/federated' ); + \update_comment_meta( $parent_id, Reaction_Sync::META_BSKY_CID, 'bafyfederated' ); + + $child_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $this->post_id, + 'comment_parent' => $parent_id, + 'user_id' => 1, + ) + ); + + $record = ( new Comment( \get_comment( $child_id ) ) )->transform(); + + $this->assertSame( 'at://did:plc:stranger/app.bsky.feed.post/federated', $record['reply']['parent']['uri'] ); + $this->assertSame( 'bafyfederated', $record['reply']['parent']['cid'] ); + } + + /** + * A federated parent without a stored CID still resolves to the + * federated URI; the CID field is left empty rather than falling + * back to the root — the AT-URI alone identifies the record for + * reply threading. + * + * @covers ::transform + */ + public function test_reply_to_federated_parent_without_cid_uses_source_uri_with_empty_cid() { + $parent_id = self::factory()->comment->create( + array( 'comment_post_ID' => $this->post_id ) + ); + \update_comment_meta( $parent_id, Reaction_Sync::META_PROTOCOL, 'atproto' ); + \update_comment_meta( $parent_id, Reaction_Sync::META_SOURCE_ID, 'at://did:plc:stranger/app.bsky.feed.post/nocid' ); + // No META_BSKY_CID. + + $child_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $this->post_id, + 'comment_parent' => $parent_id, + 'user_id' => 1, + ) + ); + + $record = ( new Comment( \get_comment( $child_id ) ) )->transform(); + + $this->assertSame( 'at://did:plc:stranger/app.bsky.feed.post/nocid', $record['reply']['parent']['uri'] ); + $this->assertSame( '', $record['reply']['parent']['cid'] ); + } + + /** + * A reply whose parent has no AT metadata at all falls through to + * the root post ref so the reply still threads onto the post. + * + * @covers ::transform + */ + public function test_reply_to_unpublishable_parent_falls_back_to_root() { + $parent_id = self::factory()->comment->create( + array( 'comment_post_ID' => $this->post_id ) + ); + + $child_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $this->post_id, + 'comment_parent' => $parent_id, + 'user_id' => 1, + ) + ); + + $record = ( new Comment( \get_comment( $child_id ) ) )->transform(); + + $this->assertSame( $this->post_uri, $record['reply']['parent']['uri'] ); + $this->assertSame( $this->post_cid, $record['reply']['parent']['cid'] ); + } + + /** + * The rkey generates and persists a TID on first call, returning + * the same value on subsequent calls. + * + * @covers ::get_rkey + */ + public function test_get_rkey_generates_and_persists_tid() { + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $this->post_id, + 'user_id' => 1, + ) + ); + + $transformer = new Comment( \get_comment( $comment_id ) ); + $first = $transformer->get_rkey(); + $second = $transformer->get_rkey(); + + $this->assertNotEmpty( $first ); + $this->assertSame( $first, $second ); + $this->assertSame( $first, \get_comment_meta( $comment_id, Comment::META_TID, true ) ); + } + + /** + * The atmosphere_transform_comment filter can mutate the record. + * + * @covers ::transform + */ + public function test_filter_can_override_record() { + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $this->post_id, + 'comment_content' => 'Will be overridden.', + 'user_id' => 1, + ) + ); + + \add_filter( + 'atmosphere_transform_comment', + static function ( array $record ) { + $record['text'] = 'Replaced.'; + return $record; + } + ); + + $record = ( new Comment( \get_comment( $comment_id ) ) )->transform(); + + $this->assertSame( 'Replaced.', $record['text'] ); + } + + /** + * Long comment bodies are truncated to 300 characters (Bluesky cap). + * + * @covers ::transform + */ + public function test_long_content_is_truncated() { + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $this->post_id, + 'comment_content' => \str_repeat( 'a ', 400 ), + 'user_id' => 1, + ) + ); + + $record = ( new Comment( \get_comment( $comment_id ) ) )->transform(); + + $this->assertLessThanOrEqual( 300, \mb_strlen( $record['text'] ) ); + } +} From 4372ef0d721462683673e1b1e023ebc1ce3455ba Mon Sep 17 00:00:00 2001 From: Matthias Pfefferle Date: Thu, 23 Apr 2026 13:22:12 +0200 Subject: [PATCH 02/10] Address Copilot review on outbound comment publishing Four related fixes surfaced by the Copilot review: - Use META_URI (not META_TID) as the "already published" marker in Publisher::update_comment / delete_comment and in the comment schedulers. Comment::get_rkey() persists the TID locally before the API call, so a failed publish would otherwise leave the TID in place and send every subsequent retry down the #update path for a record that does not exist. Matches the pattern already used on the post flow. - Require both META_URI and META_CID on the parent post in is_comment_eligible. The transformer builds reply.root as a strongRef that needs both; missing CID produces an invalid record. - schedule_comment_delete now bails when the plugin is disconnected, matching the post-flow's connection guard. Prevents enqueueing cron events that have no credentials to execute and would only orphan the remote record. - on_comment_before_delete dedupes the TID-only cron event via wp_next_scheduled, matching the guard on the publish/update hooks. New tests cover each change, including a regression guard for the stale-TID retry loop. --- includes/class-atmosphere.php | 45 +++++++--- includes/class-publisher.php | 33 +++++-- tests/phpunit/tests/class-test-atmosphere.php | 87 +++++++++++++++++++ tests/phpunit/tests/class-test-publisher.php | 53 +++++++++-- 4 files changed, 194 insertions(+), 24 deletions(-) diff --git a/includes/class-atmosphere.php b/includes/class-atmosphere.php index 1e8c80e..90b14d5 100644 --- a/includes/class-atmosphere.php +++ b/includes/class-atmosphere.php @@ -373,7 +373,7 @@ public function on_comment_edit( int $comment_id ): void { return; } - $hook = empty( \get_comment_meta( $comment_id, Comment::META_TID, true ) ) + $hook = empty( \get_comment_meta( $comment_id, Comment::META_URI, true ) ) ? 'atmosphere_publish_comment' : 'atmosphere_update_comment'; @@ -386,9 +386,11 @@ public function on_comment_edit( int $comment_id ): void { * Capture a comment's TID before it is permanently deleted. * * Runs on delete_comment which fires before the row and meta are - * removed, so the TID is still reachable. The TID-only cron variant - * lets the async worker issue the PDS delete without re-reading - * state that no longer exists. + * removed, so the TID is still reachable. META_URI gates whether + * there is anything to delete — a TID without URI is from a failed + * earlier publish (no record on the PDS). The TID-only cron + * variant lets the async worker issue the PDS delete without + * re-reading state that no longer exists. * * @param int $comment_id Comment ID. */ @@ -397,17 +399,26 @@ public function on_comment_before_delete( int $comment_id ): void { return; } + $uri = \get_comment_meta( $comment_id, Comment::META_URI, true ); + + if ( empty( $uri ) ) { + return; + } + $tid = \get_comment_meta( $comment_id, Comment::META_TID, true ); if ( empty( $tid ) ) { return; } - \wp_schedule_single_event( - \time(), - 'atmosphere_delete_comment_record', - array( (string) $tid ) - ); + $tid = (string) $tid; + $args = array( $tid ); + + if ( \wp_next_scheduled( 'atmosphere_delete_comment_record', $args ) ) { + return; + } + + \wp_schedule_single_event( \time(), 'atmosphere_delete_comment_record', $args ); } /** @@ -466,8 +477,12 @@ private static function is_comment_eligible( \WP_Comment $comment ): bool { return false; } - $post_uri = \get_post_meta( (int) $comment->comment_post_ID, Post::META_URI, true ); - if ( empty( $post_uri ) ) { + $post_id = (int) $comment->comment_post_ID; + $post_uri = \get_post_meta( $post_id, Post::META_URI, true ); + $post_cid = \get_post_meta( $post_id, Post::META_CID, true ); + + // Both URI and CID are required to build a valid reply.root strongRef. + if ( empty( $post_uri ) || empty( $post_cid ) ) { return false; } @@ -485,7 +500,7 @@ private function schedule_comment_publish( \WP_Comment $comment ): void { } $comment_id = (int) $comment->comment_ID; - $hook = empty( \get_comment_meta( $comment_id, Comment::META_TID, true ) ) + $hook = empty( \get_comment_meta( $comment_id, Comment::META_URI, true ) ) ? 'atmosphere_publish_comment' : 'atmosphere_update_comment'; @@ -502,9 +517,13 @@ private function schedule_comment_publish( \WP_Comment $comment ): void { * @param \WP_Comment $comment Comment object. */ private function schedule_comment_delete( \WP_Comment $comment ): void { + if ( ! is_connected() ) { + return; + } + $comment_id = (int) $comment->comment_ID; - if ( empty( \get_comment_meta( $comment_id, Comment::META_TID, true ) ) ) { + if ( empty( \get_comment_meta( $comment_id, Comment::META_URI, true ) ) ) { return; } diff --git a/includes/class-publisher.php b/includes/class-publisher.php index 0923669..88bae06 100644 --- a/includes/class-publisher.php +++ b/includes/class-publisher.php @@ -330,20 +330,31 @@ public static function publish_comment( \WP_Comment $comment ): array|\WP_Error /** * Update an existing bsky reply for a WordPress comment. * - * Falls through to publish_comment when no TID is stored — happens - * if an earlier publish attempt failed before meta was written. + * Falls through to publish_comment when no URI is stored — the URI + * is only written after a successful API call, so an absent URI + * means the record was never created on the PDS. Keying off the + * TID instead would be unsafe because Comment::get_rkey() persists + * the TID before the API call, so a failed publish would leave the + * TID present and send every subsequent attempt down the #update + * path for a record that does not exist. * * @param \WP_Comment $comment WordPress comment. * @return array|\WP_Error */ public static function update_comment( \WP_Comment $comment ): array|\WP_Error { $comment_id = (int) $comment->comment_ID; - $tid = \get_comment_meta( $comment_id, Comment::META_TID, true ); + $uri = \get_comment_meta( $comment_id, Comment::META_URI, true ); - if ( empty( $tid ) ) { + if ( empty( $uri ) ) { return self::publish_comment( $comment ); } + $tid = \get_comment_meta( $comment_id, Comment::META_TID, true ); + + if ( empty( $tid ) ) { + return new \WP_Error( 'atmosphere_missing_tid', \__( 'Comment URI exists but TID is missing.', 'atmosphere' ) ); + } + $transformer = new Comment( $comment ); $writes = array( @@ -369,17 +380,27 @@ public static function update_comment( \WP_Comment $comment ): array|\WP_Error { /** * Delete the bsky reply record for a WordPress comment. * + * Keys off META_URI rather than META_TID so a previously-failed + * publish (which persisted the TID but never wrote the URI) is + * correctly treated as nothing-to-delete. + * * @param \WP_Comment $comment WordPress comment. * @return array|\WP_Error */ public static function delete_comment( \WP_Comment $comment ): array|\WP_Error { $comment_id = (int) $comment->comment_ID; - $tid = \get_comment_meta( $comment_id, Comment::META_TID, true ); + $uri = \get_comment_meta( $comment_id, Comment::META_URI, true ); - if ( empty( $tid ) ) { + if ( empty( $uri ) ) { return new \WP_Error( 'atmosphere_not_published', \__( 'Comment has no AT Protocol record.', 'atmosphere' ) ); } + $tid = \get_comment_meta( $comment_id, Comment::META_TID, true ); + + if ( empty( $tid ) ) { + return new \WP_Error( 'atmosphere_missing_tid', \__( 'Comment URI exists but TID is missing.', 'atmosphere' ) ); + } + $writes = array( array( '$type' => 'com.atproto.repo.applyWrites#delete', diff --git a/tests/phpunit/tests/class-test-atmosphere.php b/tests/phpunit/tests/class-test-atmosphere.php index 2bf08f7..1cb48b1 100644 --- a/tests/phpunit/tests/class-test-atmosphere.php +++ b/tests/phpunit/tests/class-test-atmosphere.php @@ -15,6 +15,7 @@ use WP_UnitTestCase; use Atmosphere\Atmosphere; use Atmosphere\Reaction_Sync; +use Atmosphere\Transformer\Comment; use Atmosphere\Transformer\Document; use Atmosphere\Transformer\Post; @@ -88,6 +89,7 @@ private function make_eligible_comment( array $overrides = array() ): \WP_Commen if ( null === $post_id ) { $post_id = self::factory()->post->create(); \update_post_meta( $post_id, Post::META_URI, 'at://did:plc:test123/app.bsky.feed.post/abc' ); + \update_post_meta( $post_id, Post::META_CID, 'bafyroot' ); } $defaults = array( @@ -428,4 +430,89 @@ public function test_comment_with_atmosphere_agent_is_skipped() { $this->assertFalse( Atmosphere::should_publish_comment( $comment ) ); } + + /** + * Eligibility requires the root post to have both META_URI and + * META_CID — both are needed to build a valid reply strongRef. + */ + public function test_comment_on_post_without_cid_is_skipped() { + $post_id = self::factory()->post->create(); + \update_post_meta( $post_id, Post::META_URI, 'at://did:plc:test123/app.bsky.feed.post/nocid' ); + // No META_CID on purpose. + + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_approved' => '1', + 'user_id' => self::factory()->user->create(), + ) + ); + + $this->assertFalse( Atmosphere::should_publish_comment( \get_comment( $comment_id ) ) ); + } + + /** + * Approving → unapprove transitions must not schedule a delete + * when the plugin is disconnected — otherwise we'd enqueue a cron + * event that has no credentials to execute and only orphans the + * remote record. + */ + public function test_disconnected_state_does_not_schedule_comment_delete() { + $post_id = self::factory()->post->create(); + \update_post_meta( $post_id, Post::META_URI, 'at://did:plc:test123/app.bsky.feed.post/abc' ); + \update_post_meta( $post_id, Post::META_CID, 'bafyroot' ); + + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_approved' => '1', + 'user_id' => self::factory()->user->create(), + ) + ); + \update_comment_meta( $comment_id, Comment::META_URI, 'at://did:plc:test123/app.bsky.feed.post/reply' ); + + \delete_option( 'atmosphere_connection' ); + + $this->atmosphere->on_comment_status_change( 'unapproved', 'approved', \get_comment( $comment_id ) ); + + $this->assertFalse( + \wp_next_scheduled( 'atmosphere_delete_comment', array( $comment_id ) ), + 'Disconnected state must not schedule a comment delete.' + ); + } + + /** + * Hard-delete hook must not double-schedule the TID-only delete + * cron when it fires more than once for the same TID. + */ + public function test_comment_before_delete_does_not_double_schedule() { + $post_id = self::factory()->post->create(); + \update_post_meta( $post_id, Post::META_URI, 'at://did:plc:test123/app.bsky.feed.post/abc' ); + + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'user_id' => self::factory()->user->create(), + ) + ); + \update_comment_meta( $comment_id, Comment::META_TID, 'deadbeef' ); + \update_comment_meta( $comment_id, Comment::META_URI, 'at://did:plc:test123/app.bsky.feed.post/deadbeef' ); + + $this->atmosphere->on_comment_before_delete( $comment_id ); + $this->atmosphere->on_comment_before_delete( $comment_id ); + + $cron = \_get_cron_array(); + $scheduled = 0; + foreach ( $cron as $events ) { + foreach ( $events['atmosphere_delete_comment_record'] ?? array() as $event ) { + if ( isset( $event['args'][0] ) && 'deadbeef' === $event['args'][0] ) { + ++$scheduled; + } + } + } + + $this->assertSame( 1, $scheduled, 'Expected exactly one delete_comment_record cron event.' ); + + \wp_clear_scheduled_hook( 'atmosphere_delete_comment_record', array( 'deadbeef' ) ); + } } diff --git a/tests/phpunit/tests/class-test-publisher.php b/tests/phpunit/tests/class-test-publisher.php index 5b2c79f..629578e 100644 --- a/tests/phpunit/tests/class-test-publisher.php +++ b/tests/phpunit/tests/class-test-publisher.php @@ -324,9 +324,13 @@ public function test_publish_comment_stores_meta_and_dedup_key() { } /** - * Update comment falls back to publish when there is no TID yet. + * Update comment falls back to publish when there is no URI yet. + * + * A TID may be present (Comment::get_rkey persists it locally) but + * the URI is only set after a successful API call; its absence + * means the record was never created on the PDS. */ - public function test_update_comment_falls_back_to_publish_without_tid() { + public function test_update_comment_falls_back_to_publish_without_uri() { $post_id = $this->seed_root_post(); $user_id = self::factory()->user->create(); $comment_id = self::factory()->comment->create( @@ -354,7 +358,7 @@ public function test_update_comment_falls_back_to_publish_without_tid() { } /** - * Update comment issues an update when a TID is already stored. + * Update comment issues an update when URI and TID are already stored. */ public function test_update_comment_updates_existing_record() { $post_id = $this->seed_root_post(); @@ -367,6 +371,7 @@ public function test_update_comment_updates_existing_record() { ) ); \update_comment_meta( $comment_id, Comment::META_TID, 'existingtid' ); + \update_comment_meta( $comment_id, Comment::META_URI, 'at://did:plc:test123/app.bsky.feed.post/existingtid' ); $get_body = $this->stub_apply_writes( 'at://did:plc:test123/app.bsky.feed.post/existingtid', @@ -386,9 +391,45 @@ public function test_update_comment_updates_existing_record() { } /** - * Delete comment errors when the comment was never published. + * Update comment still falls back to publish when a stale TID + * exists from a previous failed API call but no URI. + * + * This is the regression guard: keying off TID would infinite-loop + * an #update request for a record that never existed. + */ + public function test_update_comment_retries_create_when_tid_persisted_but_no_uri() { + $post_id = $this->seed_root_post(); + $user_id = self::factory()->user->create(); + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_approved' => '1', + 'user_id' => $user_id, + ) + ); + // Simulate a previous publish failure: TID persisted, URI absent. + \update_comment_meta( $comment_id, Comment::META_TID, 'staletid' ); + + $get_body = $this->stub_apply_writes( + 'at://did:plc:test123/app.bsky.feed.post/staletid', + 'bafyretry' + ); + + $result = Publisher::update_comment( \get_comment( $comment_id ) ); + \remove_all_filters( 'pre_http_request' ); + + if ( \is_wp_error( $result ) ) { + $this->markTestSkipped( 'API layer rejected request: ' . $result->get_error_message() ); + } + + $body = $get_body(); + $this->assertSame( 'com.atproto.repo.applyWrites#create', $body['writes'][0]['$type'] ); + } + + /** + * Delete comment errors when the comment was never published (no URI). */ - public function test_delete_comment_errors_without_tid() { + public function test_delete_comment_errors_without_uri() { $post_id = $this->seed_root_post(); $user_id = self::factory()->user->create(); $comment_id = self::factory()->comment->create( @@ -397,6 +438,8 @@ public function test_delete_comment_errors_without_tid() { 'user_id' => $user_id, ) ); + // Even with a stale TID, absent URI means nothing to delete. + \update_comment_meta( $comment_id, Comment::META_TID, 'staletid' ); $result = Publisher::delete_comment( \get_comment( $comment_id ) ); From f2731b0de04a35ec8d197f0c1d5fd7f86f137c08 Mon Sep 17 00:00:00 2001 From: Matthias Pfefferle Date: Thu, 23 Apr 2026 13:33:26 +0200 Subject: [PATCH 03/10] Address second Copilot pass on outbound comments - `Transformer\Comment::resolve_parent_ref` now requires both URI and CID for local and federated parents. A missing CID would produce an invalid strongRef; the resolver returns null so the caller falls back to the root ref instead, matching the post-root guard added in the previous round. - Drop the `static $post_id` cache in `make_eligible_comment`. `WP_UnitTestCase` rolls back DB state between tests, so a cached ID would leave later tests referencing a row that no longer exists. - Update `test_reply_to_federated_parent_without_cid_*` to assert root fallback rather than empty-CID pass-through, and add the matching local-parent-without-CID test. --- includes/transformer/class-comment.php | 15 ++++--- tests/phpunit/tests/class-test-atmosphere.php | 15 ++++--- .../tests/transformer/class-test-comment.php | 43 ++++++++++++++++--- 3 files changed, 53 insertions(+), 20 deletions(-) diff --git a/includes/transformer/class-comment.php b/includes/transformer/class-comment.php index ed8647d..439fc63 100644 --- a/includes/transformer/class-comment.php +++ b/includes/transformer/class-comment.php @@ -142,18 +142,20 @@ private function build_reply_ref( \WP_Comment $comment ): array { * * Checks local-publish meta first (this class's own keys), then * falls through to Reaction_Sync meta for federated parents. - * Returns null when neither path yields a URI, so the caller can - * fall back to the root reference. + * Returns null when neither path yields both a URI and a CID, so + * the caller can fall back to the root reference — strongRef + * requires both fields to be set. * * @param int $parent_id Parent comment ID. * @return array{uri:string,cid:string}|null */ private function resolve_parent_ref( int $parent_id ): ?array { $local_uri = \get_comment_meta( $parent_id, self::META_URI, true ); - if ( ! empty( $local_uri ) ) { + $local_cid = \get_comment_meta( $parent_id, self::META_CID, true ); + if ( ! empty( $local_uri ) && ! empty( $local_cid ) ) { return array( 'uri' => (string) $local_uri, - 'cid' => (string) \get_comment_meta( $parent_id, self::META_CID, true ), + 'cid' => (string) $local_cid, ); } @@ -162,13 +164,14 @@ private function resolve_parent_ref( int $parent_id ): ?array { } $federated_uri = \get_comment_meta( $parent_id, Reaction_Sync::META_SOURCE_ID, true ); - if ( empty( $federated_uri ) ) { + $federated_cid = \get_comment_meta( $parent_id, Reaction_Sync::META_BSKY_CID, true ); + if ( empty( $federated_uri ) || empty( $federated_cid ) ) { return null; } return array( 'uri' => (string) $federated_uri, - 'cid' => (string) \get_comment_meta( $parent_id, Reaction_Sync::META_BSKY_CID, true ), + 'cid' => (string) $federated_cid, ); } } diff --git a/tests/phpunit/tests/class-test-atmosphere.php b/tests/phpunit/tests/class-test-atmosphere.php index 1cb48b1..19bb405 100644 --- a/tests/phpunit/tests/class-test-atmosphere.php +++ b/tests/phpunit/tests/class-test-atmosphere.php @@ -80,17 +80,18 @@ private function reset_publishing_action(): void { /** * Build a WP_Comment on a published post for comment eligibility tests. * + * A fresh post is created each call: WP_UnitTestCase rolls back + * DB state between tests, so reusing an ID across tests via a + * static cache would leave later tests pointing at a row that no + * longer exists. + * * @param array $overrides Comment field overrides. * @return \WP_Comment */ private function make_eligible_comment( array $overrides = array() ): \WP_Comment { - static $post_id = null; - - if ( null === $post_id ) { - $post_id = self::factory()->post->create(); - \update_post_meta( $post_id, Post::META_URI, 'at://did:plc:test123/app.bsky.feed.post/abc' ); - \update_post_meta( $post_id, Post::META_CID, 'bafyroot' ); - } + $post_id = self::factory()->post->create(); + \update_post_meta( $post_id, Post::META_URI, 'at://did:plc:test123/app.bsky.feed.post/abc' ); + \update_post_meta( $post_id, Post::META_CID, 'bafyroot' ); $defaults = array( 'comment_post_ID' => $post_id, diff --git a/tests/phpunit/tests/transformer/class-test-comment.php b/tests/phpunit/tests/transformer/class-test-comment.php index 7fea786..a878df2 100644 --- a/tests/phpunit/tests/transformer/class-test-comment.php +++ b/tests/phpunit/tests/transformer/class-test-comment.php @@ -147,14 +147,13 @@ public function test_reply_to_federated_parent_uses_source_id() { } /** - * A federated parent without a stored CID still resolves to the - * federated URI; the CID field is left empty rather than falling - * back to the root — the AT-URI alone identifies the record for - * reply threading. + * A federated parent without a stored CID falls back to the root + * ref — AT Protocol strongRef requires both URI and CID, and an + * empty CID would be rejected by the PDS. * * @covers ::transform */ - public function test_reply_to_federated_parent_without_cid_uses_source_uri_with_empty_cid() { + public function test_reply_to_federated_parent_without_cid_falls_back_to_root() { $parent_id = self::factory()->comment->create( array( 'comment_post_ID' => $this->post_id ) ); @@ -172,8 +171,38 @@ public function test_reply_to_federated_parent_without_cid_uses_source_uri_with_ $record = ( new Comment( \get_comment( $child_id ) ) )->transform(); - $this->assertSame( 'at://did:plc:stranger/app.bsky.feed.post/nocid', $record['reply']['parent']['uri'] ); - $this->assertSame( '', $record['reply']['parent']['cid'] ); + $this->assertSame( $this->post_uri, $record['reply']['parent']['uri'] ); + $this->assertSame( $this->post_cid, $record['reply']['parent']['cid'] ); + } + + /** + * A local parent published with URI but no stored CID (edge case + * after a PDS response that omitted CID) falls back to root. + * + * @covers ::transform + */ + public function test_reply_to_local_parent_without_cid_falls_back_to_root() { + $parent_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $this->post_id, + 'user_id' => 1, + ) + ); + \update_comment_meta( $parent_id, Comment::META_URI, 'at://did:plc:me/app.bsky.feed.post/localnocid' ); + // No META_CID. + + $child_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $this->post_id, + 'comment_parent' => $parent_id, + 'user_id' => 1, + ) + ); + + $record = ( new Comment( \get_comment( $child_id ) ) )->transform(); + + $this->assertSame( $this->post_uri, $record['reply']['parent']['uri'] ); + $this->assertSame( $this->post_cid, $record['reply']['parent']['cid'] ); } /** From 6875ba83bffb0b5673739665fd99158d957771e0 Mon Sep 17 00:00:00 2001 From: Matthias Pfefferle Date: Thu, 23 Apr 2026 15:06:25 +0200 Subject: [PATCH 04/10] Re-check comment eligibility at cron fire time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A comment's approval state can change between scheduling an async publish/update/delete and when WP-Cron actually fires the event. The transition hooks only schedule — they cannot cancel an already-queued event, and schedule_comment_delete bails when META_URI is absent (i.e. pre-publish), so a sequence like approve → unapprove would still publish the now-ineligible comment when cron ran. Each cron handler now re-runs the eligibility check: - atmosphere_publish_comment / atmosphere_update_comment — skip when should_publish_comment() is false at fire time. - atmosphere_delete_comment — skip when should_publish_comment() is true at fire time, so a re-approval between unapprove and cron keeps the remote record intact. Drive-by docstring fixes: - Publisher header now references site.standard.document (the actual lexicon collection) rather than "standard.site document". - Transformer\Comment header no longer references the removed Comment_Scheduler; Atmosphere::should_publish_comment is the gate. --- includes/class-atmosphere.php | 36 ++++++++-- includes/class-publisher.php | 8 +-- includes/transformer/class-comment.php | 9 +-- tests/phpunit/tests/class-test-atmosphere.php | 69 +++++++++++++++++++ 4 files changed, 108 insertions(+), 14 deletions(-) diff --git a/includes/class-atmosphere.php b/includes/class-atmosphere.php index 90b14d5..18fca27 100644 --- a/includes/class-atmosphere.php +++ b/includes/class-atmosphere.php @@ -608,13 +608,27 @@ static function ( string $bsky_tid, string $doc_tid ): void { 2 ); + /* + * Cron handlers re-evaluate eligibility at fire time so state + * changes between enqueue and execution (approve→unapprove, + * unapprove→re-approve, user deleted, etc.) are respected. The + * separate transition hooks only schedule; they cannot cancel + * an already-queued event, and schedule_comment_delete itself + * bails when META_URI is absent (which it is pre-publish), so + * without these guards a pre-cron unapprove would still + * publish, and a pre-cron re-approve would still delete. + */ \add_action( 'atmosphere_publish_comment', static function ( int $comment_id ): void { $comment = \get_comment( $comment_id ); - if ( $comment instanceof \WP_Comment ) { - Publisher::publish_comment( $comment ); + if ( ! $comment instanceof \WP_Comment ) { + return; + } + if ( ! self::should_publish_comment( $comment ) ) { + return; } + Publisher::publish_comment( $comment ); } ); @@ -622,9 +636,13 @@ static function ( int $comment_id ): void { 'atmosphere_update_comment', static function ( int $comment_id ): void { $comment = \get_comment( $comment_id ); - if ( $comment instanceof \WP_Comment ) { - Publisher::update_comment( $comment ); + if ( ! $comment instanceof \WP_Comment ) { + return; } + if ( ! self::should_publish_comment( $comment ) ) { + return; + } + Publisher::update_comment( $comment ); } ); @@ -632,9 +650,15 @@ static function ( int $comment_id ): void { 'atmosphere_delete_comment', static function ( int $comment_id ): void { $comment = \get_comment( $comment_id ); - if ( $comment instanceof \WP_Comment ) { - Publisher::delete_comment( $comment ); + if ( ! $comment instanceof \WP_Comment ) { + return; + } + // If the comment is eligible again by the time cron + // fires, another transition has superseded the delete. + if ( self::should_publish_comment( $comment ) ) { + return; } + Publisher::delete_comment( $comment ); } ); diff --git a/includes/class-publisher.php b/includes/class-publisher.php index 88bae06..7ce50ed 100644 --- a/includes/class-publisher.php +++ b/includes/class-publisher.php @@ -2,10 +2,10 @@ /** * Orchestrates publishing WordPress content to the AT Protocol. * - * Posts go out as a bsky post + standard.site document pair via a - * single applyWrites call. Comments go out as bsky reply records. - * The generic publish/update/delete entry points dispatch by object - * type so callers can stay polymorphic. + * Posts go out as an app.bsky.feed.post + site.standard.document + * pair via a single applyWrites call. Comments go out as bsky reply + * records. The generic publish/update/delete entry points dispatch + * by object type so callers can stay polymorphic. * * @package Atmosphere */ diff --git a/includes/transformer/class-comment.php b/includes/transformer/class-comment.php index 439fc63..2942a48 100644 --- a/includes/transformer/class-comment.php +++ b/includes/transformer/class-comment.php @@ -4,10 +4,11 @@ * reply record. * * Only comments authored by a registered WordPress user are published; - * the Comment_Scheduler enforces that gate before instantiating this - * transformer. Root is always the parent post's bsky record; parent - * resolves to a previously-published sibling comment, a federated - * reply ingested via Reaction_Sync, or falls through to the root. + * Atmosphere::should_publish_comment() and the comment lifecycle + * hooks enforce that gate before this transformer runs. Root is + * always the parent post's bsky record; parent resolves to a + * previously-published sibling comment, a federated reply ingested + * via Reaction_Sync, or falls through to the root. * * @package Atmosphere */ diff --git a/tests/phpunit/tests/class-test-atmosphere.php b/tests/phpunit/tests/class-test-atmosphere.php index 19bb405..cc73428 100644 --- a/tests/phpunit/tests/class-test-atmosphere.php +++ b/tests/phpunit/tests/class-test-atmosphere.php @@ -516,4 +516,73 @@ public function test_comment_before_delete_does_not_double_schedule() { \wp_clear_scheduled_hook( 'atmosphere_delete_comment_record', array( 'deadbeef' ) ); } + + /** + * The publish cron handler re-checks eligibility at fire time. + * A comment unapproved between schedule and execution must not + * publish; without this guard, the async event would send the + * record even though the gate now says no. + */ + public function test_publish_comment_cron_rechecks_eligibility() { + Atmosphere::register_async_hooks(); + + $comment = $this->make_eligible_comment(); + $comment_id = (int) $comment->comment_ID; + + // Flip the comment to unapproved after "scheduling". + \wp_set_comment_status( $comment_id, 'hold' ); + + $captured = false; + \add_filter( + 'pre_http_request', + static function ( $response, $args, $url ) use ( &$captured ) { + if ( false !== \strpos( $url, 'applyWrites' ) ) { + $captured = true; + } + return $response; + }, + 5, + 3 + ); + + \do_action( 'atmosphere_publish_comment', $comment_id ); + \remove_all_filters( 'pre_http_request' ); + + $this->assertFalse( $captured, 'applyWrites must not be called for a no-longer-eligible comment.' ); + \remove_all_actions( 'atmosphere_publish_comment' ); + } + + /** + * The delete cron handler must not fire when the comment has + * become eligible again between schedule and execution (e.g. + * admin unapproved then re-approved before cron ran). + */ + public function test_delete_comment_cron_skips_when_eligible_again() { + Atmosphere::register_async_hooks(); + + $comment = $this->make_eligible_comment(); + $comment_id = (int) $comment->comment_ID; + // Simulate a prior successful publish. + \update_comment_meta( $comment_id, Comment::META_URI, 'at://did:plc:test123/app.bsky.feed.post/prev' ); + \update_comment_meta( $comment_id, Comment::META_TID, 'prev' ); + + $captured = false; + \add_filter( + 'pre_http_request', + static function ( $response, $args, $url ) use ( &$captured ) { + if ( false !== \strpos( $url, 'applyWrites' ) ) { + $captured = true; + } + return $response; + }, + 5, + 3 + ); + + \do_action( 'atmosphere_delete_comment', $comment_id ); + \remove_all_filters( 'pre_http_request' ); + + $this->assertFalse( $captured, 'applyWrites#delete must not be called for a re-approved comment.' ); + \remove_all_actions( 'atmosphere_delete_comment' ); + } } From 867d0d7c35453f750f8278ca4b415c97a45b12fe Mon Sep 17 00:00:00 2001 From: Matthias Pfefferle Date: Fri, 24 Apr 2026 00:07:12 +0200 Subject: [PATCH 05/10] Address adversarial review: cascade, ordering, error logging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three classes of blocking issue surfaced by @kraftbj's Codex review: - Publisher::delete_post now cascades delete writes to every outbound comment reply on the post. AT Protocol has no cascade semantics; without this, unpublishing a post orphaned its comment reply records on the PDS with no pointer left in WordPress for cleanup. Stale TIDs from previously-failed publishes (META_TID present but META_URI absent) are excluded so the batch does not fail on non-existent records. - The atmosphere_publish_comment handler now defers when the comment's parent is eligible but has not yet published to the PDS. Without this, batch-approving a parent and a child together could fire the child's cron first and publish it flat as a top-level reply on the root post. A bounded counter (PARENT_DEFER_MAX_ATTEMPTS = 3, 30s between hops, stored as comment meta) prevents a stuck parent from blocking the child forever — after the cap the child publishes anyway using the root fallback. - All four comment cron handlers (publish, update, delete, and the TID-only delete_comment_record) now log WP_Error returns from Publisher::* instead of silently dropping them. A transient PDS outage, expired refresh token, or DPoP proof failure now leaves an error_log breadcrumb operators can act on. Tests cover the cascade-vs-stale-TID path, parent-pending deferral, and the proceed-after-cap behavior. --- includes/class-atmosphere.php | 153 +++++++++++++++++- includes/class-publisher.php | 65 +++++++- tests/phpunit/tests/class-test-atmosphere.php | 110 ++++++++++++- tests/phpunit/tests/class-test-publisher.php | 65 ++++++++ 4 files changed, 385 insertions(+), 8 deletions(-) diff --git a/includes/class-atmosphere.php b/includes/class-atmosphere.php index 18fca27..f9932be 100644 --- a/includes/class-atmosphere.php +++ b/includes/class-atmosphere.php @@ -23,6 +23,31 @@ */ class Atmosphere { + /** + * Comment meta key tracking how many times publish has been + * deferred waiting for a parent comment to publish first. + * + * @var string + */ + private const META_PUBLISH_ATTEMPTS = '_atmosphere_publish_attempts'; + + /** + * Maximum re-schedule hops for a child comment waiting on a + * not-yet-published parent. After this many deferrals the child + * publishes as a top-level reply on the post (current fallback + * behavior) so a stuck parent does not block it forever. + * + * @var int + */ + private const PARENT_DEFER_MAX_ATTEMPTS = 3; + + /** + * Seconds between parent-pending re-schedule hops. + * + * @var int + */ + private const PARENT_DEFER_DELAY_SECONDS = 30; + /** * Wire up all hooks. */ @@ -617,6 +642,10 @@ static function ( string $bsky_tid, string $doc_tid ): void { * bails when META_URI is absent (which it is pre-publish), so * without these guards a pre-cron unapprove would still * publish, and a pre-cron re-approve would still delete. + * + * Publisher WP_Error returns are logged rather than silently + * dropped so a flaky PDS window or an expired refresh token + * leaves a breadcrumb operators can find. */ \add_action( 'atmosphere_publish_comment', @@ -628,7 +657,13 @@ static function ( int $comment_id ): void { if ( ! self::should_publish_comment( $comment ) ) { return; } - Publisher::publish_comment( $comment ); + if ( self::defer_when_parent_pending( $comment ) ) { + return; + } + \delete_comment_meta( $comment_id, self::META_PUBLISH_ATTEMPTS ); + + $result = Publisher::publish_comment( $comment ); + self::log_cron_error( 'publish_comment', $comment_id, $result ); } ); @@ -642,7 +677,9 @@ static function ( int $comment_id ): void { if ( ! self::should_publish_comment( $comment ) ) { return; } - Publisher::update_comment( $comment ); + + $result = Publisher::update_comment( $comment ); + self::log_cron_error( 'update_comment', $comment_id, $result ); } ); @@ -658,19 +695,125 @@ static function ( int $comment_id ): void { if ( self::should_publish_comment( $comment ) ) { return; } - Publisher::delete_comment( $comment ); + + $result = Publisher::delete_comment( $comment ); + self::log_cron_error( 'delete_comment', $comment_id, $result ); } ); \add_action( 'atmosphere_delete_comment_record', static function ( string $tid ): void { - if ( '' !== $tid ) { - Publisher::delete_comment_by_tid( $tid ); + if ( '' === $tid ) { + return; + } + + $result = Publisher::delete_comment_by_tid( $tid ); + + if ( \is_wp_error( $result ) ) { + // Worst-case path: the WP comment row is already gone, + // so operators need the TID to clean up the orphan + // record manually. + \error_log( // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log + \sprintf( + '[atmosphere] delete_comment_record tid=%s failed: %s — %s', + $tid, + $result->get_error_code(), + $result->get_error_message() + ) + ); } }, 10, 1 ); } + + /** + * Defer a child comment publish when its parent is eligible but + * has not published to the PDS yet. + * + * Comments are scheduled as independent single events with no + * dependency ordering: if a user approves a parent and its reply + * together, the child's cron event can fire first, see + * resolve_parent_ref() return null, and publish flat as a + * top-level reply on the root post. This defers the child a short + * interval (up to PARENT_DEFER_MAX_ATTEMPTS hops) to give the + * parent time to publish first. After the cap the child publishes + * anyway using the root fallback — a stuck parent must not block + * the child forever. + * + * @param \WP_Comment $comment Comment being published. + * @return bool True when the publish was deferred, false to proceed now. + */ + private static function defer_when_parent_pending( \WP_Comment $comment ): bool { + $parent_id = (int) $comment->comment_parent; + + if ( $parent_id <= 0 ) { + return false; + } + + $parent = \get_comment( $parent_id ); + + if ( ! $parent instanceof \WP_Comment ) { + return false; + } + + if ( ! self::should_publish_comment( $parent ) ) { + // Parent is ineligible (anon, rejected, etc.); resolve_parent_ref + // will fall back to root, which is the correct behavior. + return false; + } + + if ( ! empty( \get_comment_meta( $parent_id, Comment::META_URI, true ) ) ) { + // Parent is already published — nothing to defer for. + return false; + } + + $comment_id = (int) $comment->comment_ID; + $attempts = (int) \get_comment_meta( $comment_id, self::META_PUBLISH_ATTEMPTS, true ); + + if ( $attempts >= self::PARENT_DEFER_MAX_ATTEMPTS ) { + // Give up and publish with root as parent; clear the counter + // so a future re-publish gets a fresh deferral budget. + \delete_comment_meta( $comment_id, self::META_PUBLISH_ATTEMPTS ); + return false; + } + + \update_comment_meta( $comment_id, self::META_PUBLISH_ATTEMPTS, $attempts + 1 ); + \wp_schedule_single_event( + \time() + self::PARENT_DEFER_DELAY_SECONDS, + 'atmosphere_publish_comment', + array( $comment_id ) + ); + + return true; + } + + /** + * Log a WP_Error returned from a comment cron Publisher call. + * + * `wp_schedule_single_event` does not retry, so a silent drop + * here would lose the breadcrumb operators need to diagnose + * auth, transport, or PDS-side failures. + * + * @param string $op One of 'publish_comment' | 'update_comment' | 'delete_comment'. + * @param int $comment_id Comment ID. + * @param mixed $result Publisher call result. + */ + private static function log_cron_error( string $op, int $comment_id, $result ): void { + if ( ! \is_wp_error( $result ) ) { + return; + } + + \error_log( // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log + \sprintf( + '[atmosphere] %s %d failed: %s — %s', + $op, + $comment_id, + $result->get_error_code(), + $result->get_error_message() + ) + ); + } } diff --git a/includes/class-publisher.php b/includes/class-publisher.php index 7ce50ed..b90e17c 100644 --- a/includes/class-publisher.php +++ b/includes/class-publisher.php @@ -169,7 +169,13 @@ public static function update_post( \WP_Post $post ): array|\WP_Error { } /** - * Delete both records for a post. + * Delete both records for a post, plus any outbound comment replies. + * + * Comment reply records live in our own repo keyed by their own + * TIDs — the AT Protocol has no cascade semantics, so deleting the + * root post does not remove them. If we do not include them here, + * unpublishing a post leaves the comment replies on Bluesky with + * no remaining pointer from WordPress that could clean them up. * * @param \WP_Post $post WordPress post. * @return array|\WP_Error @@ -200,6 +206,15 @@ public static function delete_post( \WP_Post $post ): array|\WP_Error { ); } + $comment_tids = self::collect_published_comment_tids( $post->ID ); + foreach ( $comment_tids as $comment_tid ) { + $writes[] = array( + '$type' => 'com.atproto.repo.applyWrites#delete', + 'collection' => 'app.bsky.feed.post', + 'rkey' => $comment_tid['tid'], + ); + } + $result = API::apply_writes( $writes ); if ( \is_wp_error( $result ) ) { @@ -214,9 +229,57 @@ public static function delete_post( \WP_Post $post ): array|\WP_Error { \delete_post_meta( $post->ID, Document::META_URI ); \delete_post_meta( $post->ID, Document::META_CID ); + // Clean up comment meta for every reply we just deleted. + foreach ( $comment_tids as $comment_tid ) { + \delete_comment_meta( $comment_tid['comment_id'], Comment::META_TID ); + \delete_comment_meta( $comment_tid['comment_id'], Comment::META_URI ); + \delete_comment_meta( $comment_tid['comment_id'], Comment::META_CID ); + \delete_comment_meta( $comment_tid['comment_id'], Reaction_Sync::META_SOURCE_ID ); + } + return $result; } + /** + * Collect { comment_id, tid } pairs for all outbound comment replies + * on a post. Only comments that actually reached the PDS (META_URI + * present) are returned — stale TIDs from a previously-failed + * publish would refer to a non-existent record and the delete would + * fail. + * + * @param int $post_id Post ID. + * @return array + */ + private static function collect_published_comment_tids( int $post_id ): array { + $comments = \get_comments( + array( + 'post_id' => $post_id, + 'status' => 'any', + 'meta_query' => array( // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_query + array( + 'key' => Comment::META_URI, + 'compare' => 'EXISTS', + ), + ), + 'fields' => 'ids', + ) + ); + + $out = array(); + + foreach ( $comments as $comment_id ) { + $tid = \get_comment_meta( (int) $comment_id, Comment::META_TID, true ); + if ( ! empty( $tid ) ) { + $out[] = array( + 'comment_id' => (int) $comment_id, + 'tid' => (string) $tid, + ); + } + } + + return $out; + } + /** * Delete post AT Protocol records by TID, without requiring the post to exist. * diff --git a/tests/phpunit/tests/class-test-atmosphere.php b/tests/phpunit/tests/class-test-atmosphere.php index cc73428..af7227c 100644 --- a/tests/phpunit/tests/class-test-atmosphere.php +++ b/tests/phpunit/tests/class-test-atmosphere.php @@ -524,7 +524,6 @@ public function test_comment_before_delete_does_not_double_schedule() { * record even though the gate now says no. */ public function test_publish_comment_cron_rechecks_eligibility() { - Atmosphere::register_async_hooks(); $comment = $this->make_eligible_comment(); $comment_id = (int) $comment->comment_ID; @@ -558,7 +557,6 @@ static function ( $response, $args, $url ) use ( &$captured ) { * admin unapproved then re-approved before cron ran). */ public function test_delete_comment_cron_skips_when_eligible_again() { - Atmosphere::register_async_hooks(); $comment = $this->make_eligible_comment(); $comment_id = (int) $comment->comment_ID; @@ -585,4 +583,112 @@ static function ( $response, $args, $url ) use ( &$captured ) { $this->assertFalse( $captured, 'applyWrites#delete must not be called for a re-approved comment.' ); \remove_all_actions( 'atmosphere_delete_comment' ); } + + /** + * When a parent comment is eligible but has not yet published, + * the child's cron handler reschedules itself and does not call + * the PDS. This prevents a batch approval from publishing the + * child flat as a top-level reply before the parent exists. + */ + public function test_publish_comment_defers_when_parent_pending() { + + $post_id = self::factory()->post->create(); + \update_post_meta( $post_id, Post::META_URI, 'at://did:plc:test123/app.bsky.feed.post/root' ); + \update_post_meta( $post_id, Post::META_CID, 'bafyroot' ); + + $user_id = self::factory()->user->create(); + + $parent_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_approved' => '1', + 'user_id' => $user_id, + ) + ); + // Parent is eligible but not yet published — no META_URI. + + $child_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_parent' => $parent_id, + 'comment_approved' => '1', + 'user_id' => $user_id, + ) + ); + + $captured = false; + \add_filter( + 'pre_http_request', + static function ( $response, $args, $url ) use ( &$captured ) { + if ( false !== \strpos( $url, 'applyWrites' ) ) { + $captured = true; + } + return $response; + }, + 5, + 3 + ); + + \do_action( 'atmosphere_publish_comment', $child_id ); + \remove_all_filters( 'pre_http_request' ); + + $this->assertFalse( $captured, 'Child must not publish while parent is pending.' ); + $this->assertNotFalse( + \wp_next_scheduled( 'atmosphere_publish_comment', array( $child_id ) ), + 'Child must be rescheduled when parent is pending.' + ); + $this->assertSame( + '1', + \get_comment_meta( $child_id, '_atmosphere_publish_attempts', true ), + 'Deferral counter must be incremented on each hop.' + ); + + \remove_all_actions( 'atmosphere_publish_comment' ); + \wp_clear_scheduled_hook( 'atmosphere_publish_comment', array( $child_id ) ); + } + + /** + * After the deferral cap the child publishes anyway so a stuck + * parent cannot block it forever; the root-fallback branch of + * Transformer\Comment::resolve_parent_ref takes over. + */ + public function test_publish_comment_proceeds_after_parent_defer_cap() { + + $post_id = self::factory()->post->create(); + \update_post_meta( $post_id, Post::META_URI, 'at://did:plc:test123/app.bsky.feed.post/root' ); + \update_post_meta( $post_id, Post::META_CID, 'bafyroot' ); + + $user_id = self::factory()->user->create(); + + $parent_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_approved' => '1', + 'user_id' => $user_id, + ) + ); + + $child_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_parent' => $parent_id, + 'comment_approved' => '1', + 'user_id' => $user_id, + ) + ); + // Already at the cap — next fire must proceed rather than defer. + \update_comment_meta( $child_id, '_atmosphere_publish_attempts', 3 ); + + \do_action( 'atmosphere_publish_comment', $child_id ); + + $this->assertFalse( + \wp_next_scheduled( 'atmosphere_publish_comment', array( $child_id ) ), + 'After the cap the handler must not re-enqueue the child.' + ); + $this->assertSame( + '', + \get_comment_meta( $child_id, '_atmosphere_publish_attempts', true ), + 'Counter must be cleared once the child proceeds.' + ); + } } diff --git a/tests/phpunit/tests/class-test-publisher.php b/tests/phpunit/tests/class-test-publisher.php index 629578e..5a1ccc0 100644 --- a/tests/phpunit/tests/class-test-publisher.php +++ b/tests/phpunit/tests/class-test-publisher.php @@ -213,6 +213,71 @@ public function test_delete_preserves_meta_on_api_error() { $this->assertSame( 'doc-tid-456', \get_post_meta( $post->ID, Document::META_TID, true ) ); } + /** + * Delete-post includes delete writes for every published comment + * reply on the post and clears their meta on success. AT Protocol + * has no cascade semantics, so without this the replies would be + * orphaned on the PDS after the root goes away. + */ + public function test_delete_post_cascades_comment_replies() { + $post = self::factory()->post->create_and_get( + array( 'post_status' => 'trash' ) + ); + \update_post_meta( $post->ID, Post::META_TID, 'post-tid' ); + \update_post_meta( $post->ID, Document::META_TID, 'doc-tid' ); + + // Two published comment replies + one never-published comment. + $c1 = self::factory()->comment->create( array( 'comment_post_ID' => $post->ID ) ); + \update_comment_meta( $c1, Comment::META_TID, 'reply-tid-1' ); + \update_comment_meta( $c1, Comment::META_URI, 'at://did:plc:test123/app.bsky.feed.post/reply-tid-1' ); + + $c2 = self::factory()->comment->create( array( 'comment_post_ID' => $post->ID ) ); + \update_comment_meta( $c2, Comment::META_TID, 'reply-tid-2' ); + \update_comment_meta( $c2, Comment::META_URI, 'at://did:plc:test123/app.bsky.feed.post/reply-tid-2' ); + + $c3 = self::factory()->comment->create( array( 'comment_post_ID' => $post->ID ) ); + \update_comment_meta( $c3, Comment::META_TID, 'stale-tid' ); + // No META_URI — previously-failed publish; must not be in the delete batch. + + $captured_body = null; + \add_filter( + 'pre_http_request', + static function ( $response, $args, $url ) use ( &$captured_body ) { + if ( false !== \strpos( $url, 'applyWrites' ) ) { + $captured_body = \json_decode( $args['body'], true ); + + return array( + 'response' => array( 'code' => 200 ), + 'body' => \wp_json_encode( array( 'results' => array() ) ), + ); + } + return $response; + }, + 5, + 3 + ); + + Publisher::delete_post( $post ); + \remove_all_filters( 'pre_http_request' ); + + if ( null === $captured_body ) { + $this->markTestSkipped( 'API layer rejected request before stub.' ); + } + + $rkeys = \array_column( $captured_body['writes'], 'rkey' ); + $this->assertContains( 'post-tid', $rkeys ); + $this->assertContains( 'doc-tid', $rkeys ); + $this->assertContains( 'reply-tid-1', $rkeys ); + $this->assertContains( 'reply-tid-2', $rkeys ); + $this->assertNotContains( 'stale-tid', $rkeys, 'Stale TID without URI must not be included.' ); + + // Meta cleanup on both the post and the published replies. + $this->assertSame( '', \get_comment_meta( $c1, Comment::META_URI, true ) ); + $this->assertSame( '', \get_comment_meta( $c2, Comment::META_TID, true ) ); + // Stale comment's TID is left alone — we did not touch its record. + $this->assertSame( 'stale-tid', \get_comment_meta( $c3, Comment::META_TID, true ) ); + } + /** * Test that delete() returns error when no TIDs exist. */ From a1351924a2e27997e8d5d16a37121d79d2cc0cc6 Mon Sep 17 00:00:00 2001 From: Matthias Pfefferle Date: Fri, 24 Apr 2026 00:14:56 +0200 Subject: [PATCH 06/10] Address remaining adversarial review items - `store_comment_result` no longer falls back to a locally-synthesized URI when applyWrites returns 2xx without `results[0].uri`. It now returns a WP_Error that publish_comment/update_comment propagate, so a malformed response is no longer indistinguishable from a clean publish (which would otherwise poison `META_URI` + the dedup key and steer later update/delete calls at a record that may not exist). Return type changed to `true|\WP_Error` per the WordPress success-or-error convention. - Rewrite the `on_comment_before_delete` docblock. The previous one claimed "TID without URI is from a failed earlier publish"; the TID is persisted eagerly by Comment::get_rkey before the API call, so TID-only is the normal pre-publish state too. The new docblock matches Publisher::update_comment's wording. - Add error-path tests for comment Publisher: publish writes no meta on API error, publish returns atmosphere_missing_uri when the 2xx response omits `results[0].uri`, update_comment + delete_comment preserve meta on API error. - Add schedule-layer tests for every comment lifecycle transition: approve/unapprove status changes, approved/unapproved/spam inserts, edit with/without META_URI, edit on an unapproved comment, and the TID-without-URI hard-delete guard. --- includes/class-atmosphere.php | 13 +- includes/class-publisher.php | 35 +++- tests/phpunit/tests/class-test-atmosphere.php | 152 ++++++++++++++++++ tests/phpunit/tests/class-test-publisher.php | 123 ++++++++++++++ 4 files changed, 311 insertions(+), 12 deletions(-) diff --git a/includes/class-atmosphere.php b/includes/class-atmosphere.php index f9932be..7f537fa 100644 --- a/includes/class-atmosphere.php +++ b/includes/class-atmosphere.php @@ -411,11 +411,14 @@ public function on_comment_edit( int $comment_id ): void { * Capture a comment's TID before it is permanently deleted. * * Runs on delete_comment which fires before the row and meta are - * removed, so the TID is still reachable. META_URI gates whether - * there is anything to delete — a TID without URI is from a failed - * earlier publish (no record on the PDS). The TID-only cron - * variant lets the async worker issue the PDS delete without - * re-reading state that no longer exists. + * removed, so the TID is still reachable. META_URI is the only + * reliable signal that a record exists on the PDS — the TID is + * persisted eagerly by Comment::get_rkey() before the applyWrites + * call, so a TID alone matches both the normal pre-publish state + * and a publish that failed after TID allocation; neither should + * schedule a delete. The TID-only cron variant lets the async + * worker issue the PDS delete without re-reading state that no + * longer exists. * * @param int $comment_id Comment ID. */ diff --git a/includes/class-publisher.php b/includes/class-publisher.php index b90e17c..fcd7147 100644 --- a/includes/class-publisher.php +++ b/includes/class-publisher.php @@ -385,7 +385,10 @@ public static function publish_comment( \WP_Comment $comment ): array|\WP_Error return $result; } - self::store_comment_result( (int) $comment->comment_ID, $result, $transformer ); + $stored = self::store_comment_result( (int) $comment->comment_ID, $result ); + if ( \is_wp_error( $stored ) ) { + return $stored; + } return $result; } @@ -435,7 +438,10 @@ public static function update_comment( \WP_Comment $comment ): array|\WP_Error { return $result; } - self::store_comment_result( $comment_id, $result, $transformer ); + $stored = self::store_comment_result( $comment_id, $result ); + if ( \is_wp_error( $stored ) ) { + return $stored; + } return $result; } @@ -557,21 +563,36 @@ private static function store_post_result( int $post_id, array $result, Post $bs * that when listRecords feeds our own reply back through the inbound * sync, find_comment_by_source_id() matches this row and skips it. * - * @param int $comment_id WordPress comment ID. - * @param array $result applyWrites response. - * @param Comment $transformer Comment transformer. + * Treats a 2xx response that omits `results[0].uri` as a failure + * and returns a WP_Error. A locally-synthesized URI fallback would + * make a malformed server response indistinguishable from a clean + * publish, poison the dedup key, and steer later update/delete + * calls at a record that may not exist. + * + * @param int $comment_id WordPress comment ID. + * @param array $result applyWrites response. + * @return true|\WP_Error True on success, WP_Error on missing URI. */ - private static function store_comment_result( int $comment_id, array $result, Comment $transformer ): void { + private static function store_comment_result( int $comment_id, array $result ): true|\WP_Error { $first = $result['results'][0] ?? array(); - $uri = $first['uri'] ?? $transformer->get_uri(); + $uri = $first['uri'] ?? ''; $cid = $first['cid'] ?? ''; + if ( '' === $uri ) { + return new \WP_Error( + 'atmosphere_missing_uri', + \__( 'applyWrites response did not include a record URI.', 'atmosphere' ) + ); + } + \update_comment_meta( $comment_id, Comment::META_URI, $uri ); \update_comment_meta( $comment_id, Reaction_Sync::META_SOURCE_ID, $uri ); if ( $cid ) { \update_comment_meta( $comment_id, Comment::META_CID, $cid ); } + + return true; } /** diff --git a/tests/phpunit/tests/class-test-atmosphere.php b/tests/phpunit/tests/class-test-atmosphere.php index af7227c..7fa7f8a 100644 --- a/tests/phpunit/tests/class-test-atmosphere.php +++ b/tests/phpunit/tests/class-test-atmosphere.php @@ -647,6 +647,158 @@ static function ( $response, $args, $url ) use ( &$captured ) { \wp_clear_scheduled_hook( 'atmosphere_publish_comment', array( $child_id ) ); } + /** + * Approve transition schedules a publish. + */ + public function test_status_change_unapproved_to_approved_schedules_publish() { + $comment = $this->make_eligible_comment(); + + $this->atmosphere->on_comment_status_change( 'approved', 'unapproved', $comment ); + + $this->assertNotFalse( + \wp_next_scheduled( 'atmosphere_publish_comment', array( (int) $comment->comment_ID ) ), + 'Approve transition must schedule atmosphere_publish_comment.' + ); + + \wp_clear_scheduled_hook( 'atmosphere_publish_comment', array( (int) $comment->comment_ID ) ); + } + + /** + * Unapprove transition on a published comment schedules a delete. + */ + public function test_status_change_approved_to_unapproved_schedules_delete() { + $comment = $this->make_eligible_comment(); + $comment_id = (int) $comment->comment_ID; + \update_comment_meta( $comment_id, Comment::META_URI, 'at://did:plc:test123/app.bsky.feed.post/existing' ); + + $this->atmosphere->on_comment_status_change( 'unapproved', 'approved', $comment ); + + $this->assertNotFalse( + \wp_next_scheduled( 'atmosphere_delete_comment', array( $comment_id ) ), + 'Unapprove transition on a published comment must schedule atmosphere_delete_comment.' + ); + + \wp_clear_scheduled_hook( 'atmosphere_delete_comment', array( $comment_id ) ); + } + + /** + * Comment inserted already-approved schedules a publish. + */ + public function test_insert_approved_schedules_publish() { + $comment = $this->make_eligible_comment(); + + $this->atmosphere->on_comment_insert( (int) $comment->comment_ID, 1 ); + + $this->assertNotFalse( + \wp_next_scheduled( 'atmosphere_publish_comment', array( (int) $comment->comment_ID ) ), + 'Already-approved insert must schedule atmosphere_publish_comment.' + ); + + \wp_clear_scheduled_hook( 'atmosphere_publish_comment', array( (int) $comment->comment_ID ) ); + } + + /** + * Comment inserted unapproved (moderation queue) does not schedule. + */ + public function test_insert_unapproved_does_not_schedule() { + $comment = $this->make_eligible_comment(); + + $this->atmosphere->on_comment_insert( (int) $comment->comment_ID, 0 ); + + $this->assertFalse( + \wp_next_scheduled( 'atmosphere_publish_comment', array( (int) $comment->comment_ID ) ), + 'Pending comment must not schedule a publish.' + ); + } + + /** + * Spam comment never schedules. + */ + public function test_insert_spam_does_not_schedule() { + $comment = $this->make_eligible_comment(); + + $this->atmosphere->on_comment_insert( (int) $comment->comment_ID, 'spam' ); + + $this->assertFalse( + \wp_next_scheduled( 'atmosphere_publish_comment', array( (int) $comment->comment_ID ) ), + 'Spam insert must not schedule a publish.' + ); + } + + /** + * Editing an already-published comment schedules an update. + */ + public function test_edit_with_uri_schedules_update() { + $comment = $this->make_eligible_comment(); + $comment_id = (int) $comment->comment_ID; + \update_comment_meta( $comment_id, Comment::META_URI, 'at://did:plc:test123/app.bsky.feed.post/existing' ); + + $this->atmosphere->on_comment_edit( $comment_id ); + + $this->assertNotFalse( + \wp_next_scheduled( 'atmosphere_update_comment', array( $comment_id ) ), + 'Editing a published comment must schedule atmosphere_update_comment.' + ); + $this->assertFalse( + \wp_next_scheduled( 'atmosphere_publish_comment', array( $comment_id ) ), + 'Editing a published comment must not schedule a publish.' + ); + + \wp_clear_scheduled_hook( 'atmosphere_update_comment', array( $comment_id ) ); + } + + /** + * Editing an approved-but-never-published comment schedules a publish. + * Covers the failed-initial-publish recovery path: the edit catches + * the comment up, rather than silently leaving it at TID-only meta. + */ + public function test_edit_without_uri_schedules_publish() { + $comment = $this->make_eligible_comment(); + $comment_id = (int) $comment->comment_ID; + + $this->atmosphere->on_comment_edit( $comment_id ); + + $this->assertNotFalse( + \wp_next_scheduled( 'atmosphere_publish_comment', array( $comment_id ) ), + 'Editing an unpublished-but-eligible comment must schedule a publish.' + ); + + \wp_clear_scheduled_hook( 'atmosphere_publish_comment', array( $comment_id ) ); + } + + /** + * Editing an unapproved comment does not schedule anything — the + * eligibility gate rejects it before the handler decides publish + * vs. update. + */ + public function test_edit_unapproved_does_not_schedule() { + $comment = $this->make_eligible_comment( array( 'comment_approved' => '0' ) ); + $comment_id = (int) $comment->comment_ID; + + $this->atmosphere->on_comment_edit( $comment_id ); + + $this->assertFalse( \wp_next_scheduled( 'atmosphere_publish_comment', array( $comment_id ) ) ); + $this->assertFalse( \wp_next_scheduled( 'atmosphere_update_comment', array( $comment_id ) ) ); + } + + /** + * Hard-delete of a comment with a TID but no URI (failed earlier + * publish) must not schedule the TID-only delete cron — no record + * exists on the PDS to remove. + */ + public function test_before_delete_with_tid_but_no_uri_does_not_schedule() { + $comment = $this->make_eligible_comment(); + $comment_id = (int) $comment->comment_ID; + \update_comment_meta( $comment_id, Comment::META_TID, 'staletid' ); + + $this->atmosphere->on_comment_before_delete( $comment_id ); + + $this->assertFalse( + \wp_next_scheduled( 'atmosphere_delete_comment_record', array( 'staletid' ) ), + 'TID without URI (failed earlier publish) must not schedule a delete.' + ); + } + /** * After the deferral cap the child publishes anyway so a stuck * parent cannot block it forever; the root-fallback branch of diff --git a/tests/phpunit/tests/class-test-publisher.php b/tests/phpunit/tests/class-test-publisher.php index 5a1ccc0..e3ad56d 100644 --- a/tests/phpunit/tests/class-test-publisher.php +++ b/tests/phpunit/tests/class-test-publisher.php @@ -540,6 +540,129 @@ public function test_delete_comment_by_tid_rejects_empty() { $this->assertSame( 'atmosphere_not_published', $result->get_error_code() ); } + /** + * Publish-comment on API error writes no comment meta. A failed + * API call must not leave synthesized URI/CID/SOURCE_ID behind, + * because later update/delete/dedup paths key off those values. + */ + public function test_publish_comment_writes_no_meta_on_api_error() { + $post_id = $this->seed_root_post(); + $user_id = self::factory()->user->create(); + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_approved' => '1', + 'user_id' => $user_id, + ) + ); + + // No stub — the bootstrap's auth layer returns WP_Error. + $result = Publisher::publish_comment( \get_comment( $comment_id ) ); + + $this->assertWPError( $result ); + $this->assertSame( '', \get_comment_meta( $comment_id, Comment::META_URI, true ) ); + $this->assertSame( '', \get_comment_meta( $comment_id, Comment::META_CID, true ) ); + $this->assertSame( '', \get_comment_meta( $comment_id, Reaction_Sync::META_SOURCE_ID, true ) ); + } + + /** + * Publish-comment on a 2xx response that omits results[0].uri + * returns atmosphere_missing_uri and does not write meta, rather + * than silently mirroring a locally-synthesized URI into the + * dedup key. + */ + public function test_publish_comment_errors_on_response_without_uri() { + $post_id = $this->seed_root_post(); + $user_id = self::factory()->user->create(); + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_approved' => '1', + 'user_id' => $user_id, + ) + ); + + \add_filter( + 'pre_http_request', + static function ( $response, $args, $url ) { + if ( false !== \strpos( $url, 'applyWrites' ) ) { + return array( + 'response' => array( 'code' => 200 ), + 'body' => \wp_json_encode( array( 'results' => array( array() ) ) ), + ); + } + return $response; + }, + 5, + 3 + ); + + $result = Publisher::publish_comment( \get_comment( $comment_id ) ); + \remove_all_filters( 'pre_http_request' ); + + if ( \is_wp_error( $result ) && 'atmosphere_missing_uri' !== $result->get_error_code() ) { + // Auth layer blocked before the stub — the assertion we + // care about cannot run. + $this->markTestSkipped( 'API layer rejected request before stub: ' . $result->get_error_code() ); + } + + $this->assertWPError( $result ); + $this->assertSame( 'atmosphere_missing_uri', $result->get_error_code() ); + $this->assertSame( '', \get_comment_meta( $comment_id, Comment::META_URI, true ) ); + $this->assertSame( '', \get_comment_meta( $comment_id, Reaction_Sync::META_SOURCE_ID, true ) ); + } + + /** + * Update-comment on API error preserves the previously-stored + * URI/CID meta so subsequent retries see the record still exists. + */ + public function test_update_comment_preserves_meta_on_api_error() { + $post_id = $this->seed_root_post(); + $user_id = self::factory()->user->create(); + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_approved' => '1', + 'user_id' => $user_id, + ) + ); + \update_comment_meta( $comment_id, Comment::META_TID, 'existingtid' ); + \update_comment_meta( $comment_id, Comment::META_URI, 'at://did:plc:test123/app.bsky.feed.post/existingtid' ); + \update_comment_meta( $comment_id, Comment::META_CID, 'bafyexisting' ); + + $result = Publisher::update_comment( \get_comment( $comment_id ) ); + + $this->assertWPError( $result ); + $this->assertSame( 'existingtid', \get_comment_meta( $comment_id, Comment::META_TID, true ) ); + $this->assertSame( 'at://did:plc:test123/app.bsky.feed.post/existingtid', \get_comment_meta( $comment_id, Comment::META_URI, true ) ); + $this->assertSame( 'bafyexisting', \get_comment_meta( $comment_id, Comment::META_CID, true ) ); + } + + /** + * Delete-comment on API error preserves the meta so a later retry + * still targets the existing record instead of silently giving up. + */ + public function test_delete_comment_preserves_meta_on_api_error() { + $post_id = $this->seed_root_post(); + $user_id = self::factory()->user->create(); + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'user_id' => $user_id, + ) + ); + \update_comment_meta( $comment_id, Comment::META_TID, 'doomed' ); + \update_comment_meta( $comment_id, Comment::META_URI, 'at://did:plc:test123/app.bsky.feed.post/doomed' ); + \update_comment_meta( $comment_id, Comment::META_CID, 'bafydoomed' ); + + $result = Publisher::delete_comment( \get_comment( $comment_id ) ); + + $this->assertWPError( $result ); + $this->assertSame( 'doomed', \get_comment_meta( $comment_id, Comment::META_TID, true ) ); + $this->assertSame( 'at://did:plc:test123/app.bsky.feed.post/doomed', \get_comment_meta( $comment_id, Comment::META_URI, true ) ); + $this->assertSame( 'bafydoomed', \get_comment_meta( $comment_id, Comment::META_CID, true ) ); + } + /** * Generic Publisher::publish dispatches to publish_post for WP_Post. */ From 2309ee6e8a3e24ea79e4c1159d2b35ebcd94fc07 Mon Sep 17 00:00:00 2001 From: Matthias Pfefferle Date: Fri, 24 Apr 2026 08:38:37 +0200 Subject: [PATCH 07/10] Drop remove_all_actions calls from Atmosphere tests These were leftovers from an earlier revision that also called register_async_hooks in the tests. Removing a global action permanently unregisters the plugin's cron handler for the rest of the test run, making subsequent tests that rely on do_action( 'atmosphere_publish_comment', ... ) order-dependent and flaky. The plugin bootstrap registers these handlers once; the tests only need to clear the scheduled events they created, which they already do. --- tests/phpunit/tests/class-test-atmosphere.php | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/phpunit/tests/class-test-atmosphere.php b/tests/phpunit/tests/class-test-atmosphere.php index 7fa7f8a..7adb306 100644 --- a/tests/phpunit/tests/class-test-atmosphere.php +++ b/tests/phpunit/tests/class-test-atmosphere.php @@ -548,7 +548,6 @@ static function ( $response, $args, $url ) use ( &$captured ) { \remove_all_filters( 'pre_http_request' ); $this->assertFalse( $captured, 'applyWrites must not be called for a no-longer-eligible comment.' ); - \remove_all_actions( 'atmosphere_publish_comment' ); } /** @@ -581,7 +580,6 @@ static function ( $response, $args, $url ) use ( &$captured ) { \remove_all_filters( 'pre_http_request' ); $this->assertFalse( $captured, 'applyWrites#delete must not be called for a re-approved comment.' ); - \remove_all_actions( 'atmosphere_delete_comment' ); } /** @@ -643,7 +641,6 @@ static function ( $response, $args, $url ) use ( &$captured ) { 'Deferral counter must be incremented on each hop.' ); - \remove_all_actions( 'atmosphere_publish_comment' ); \wp_clear_scheduled_hook( 'atmosphere_publish_comment', array( $child_id ) ); } From 0974054e232aad74e09cb1fa8874a806c64f2d61 Mon Sep 17 00:00:00 2001 From: Matthias Pfefferle Date: Wed, 29 Apr 2026 18:42:37 +0200 Subject: [PATCH 08/10] Clear comment crons on deactivate and uninstall MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #35 established the pattern of clearing every plugin-owned cron in deactivate() and uninstall.php so leftover jobs don't linger after the plugin is removed. The four new comment crons added in this PR (atmosphere_publish_comment, atmosphere_update_comment, atmosphere_delete_comment, atmosphere_delete_comment_record) were missing from both lists. uninstall.php's commentmeta sweep was also missing — it cleaned postmeta only. Without this, a queued atmosphere_delete_comment_record from a previous install can fire against a new install's connection after uninstall + reinstall, since delete_comment_by_tid() does not verify the TID belongs to the current connection. --- atmosphere.php | 4 ++++ uninstall.php | 16 ++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/atmosphere.php b/atmosphere.php index 6016a83..4751cb3 100644 --- a/atmosphere.php +++ b/atmosphere.php @@ -68,6 +68,10 @@ function deactivate() { \wp_clear_scheduled_hook( 'atmosphere_sync_reactions' ); \wp_clear_scheduled_hook( 'atmosphere_sync_publication' ); \wp_clear_scheduled_hook( 'atmosphere_delete_records' ); + \wp_clear_scheduled_hook( 'atmosphere_publish_comment' ); + \wp_clear_scheduled_hook( 'atmosphere_update_comment' ); + \wp_clear_scheduled_hook( 'atmosphere_delete_comment' ); + \wp_clear_scheduled_hook( 'atmosphere_delete_comment_record' ); // Clear the legacy hook name in case an earlier PR-6 build scheduled it. \wp_clear_scheduled_hook( 'atmosphere_sync_comments' ); \flush_rewrite_rules(); diff --git a/uninstall.php b/uninstall.php index b09e317..5c57d4e 100644 --- a/uninstall.php +++ b/uninstall.php @@ -25,6 +25,10 @@ wp_clear_scheduled_hook( 'atmosphere_delete_records' ); wp_clear_scheduled_hook( 'atmosphere_sync_publication' ); wp_clear_scheduled_hook( 'atmosphere_sync_reactions' ); +wp_clear_scheduled_hook( 'atmosphere_publish_comment' ); +wp_clear_scheduled_hook( 'atmosphere_update_comment' ); +wp_clear_scheduled_hook( 'atmosphere_delete_comment' ); +wp_clear_scheduled_hook( 'atmosphere_delete_comment_record' ); // Remove post meta. global $wpdb; @@ -43,6 +47,18 @@ $wpdb->delete( $wpdb->postmeta, array( 'meta_key' => $atmosphere_key ) ); // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_key,WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching } +// Remove comment meta written by the outbound-comment publisher. +$atmosphere_comment_meta_keys = array( + '_atmosphere_bsky_tid', + '_atmosphere_bsky_uri', + '_atmosphere_bsky_cid', + '_atmosphere_publish_attempts', +); + +foreach ( $atmosphere_comment_meta_keys as $atmosphere_key ) { + $wpdb->delete( $wpdb->commentmeta, array( 'meta_key' => $atmosphere_key ) ); // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_key,WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching +} + // Remove transients. delete_transient( 'atmosphere_oauth_verifier' ); delete_transient( 'atmosphere_oauth_state' ); From 612fa037ee4ddb7f380be8636bddd86325dc695e Mon Sep 17 00:00:00 2001 From: Matthias Pfefferle Date: Wed, 29 Apr 2026 18:48:08 +0200 Subject: [PATCH 09/10] Cascade outbound comment deletes through the permanent-delete path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The trash path (Publisher::delete_post) already enumerated published comment replies via collect_published_comment_tids() and bundled them into the apply_writes batch. The permanent-delete path (on_before_delete → atmosphere_delete_records → delete_post_by_tids) only carried the post and document TIDs, leaving comment reply records to be cleaned up implicitly by WP's natural delete_comment cascade. That is fragile against future core changes and against sites that narrow the syncable post-type filter after publishing. on_before_delete now enumerates published comment TIDs at the only point where they are still readable (before_delete_post fires before WP iterates child comments) and threads them through the cron event. delete_post_by_tids accepts the new $comment_tids argument and appends one applyWrites#delete per reply to the same batch. The per-comment atmosphere_delete_comment_record cron still fires via WP's cascade and provides a resilient fallback if the consolidated batch errors — at the cost of noisy "record not found" log lines on the happy path, which is the right trade for not losing comment TIDs when apply_writes fails atomically. --- includes/class-atmosphere.php | 36 ++++++++-- includes/class-publisher.php | 26 +++++-- tests/phpunit/tests/class-test-atmosphere.php | 72 +++++++++++++++++++ 3 files changed, 122 insertions(+), 12 deletions(-) diff --git a/includes/class-atmosphere.php b/includes/class-atmosphere.php index 56793f0..173cdd8 100644 --- a/includes/class-atmosphere.php +++ b/includes/class-atmosphere.php @@ -319,8 +319,17 @@ public function on_status_change( string $new_status, string $old_status, \WP_Po /** * Schedule AT Protocol record deletion before a post is permanently deleted. * - * Captures TIDs from post meta before they're lost, then schedules - * an async delete via cron. + * Captures the post TIDs and the TIDs of every published outbound + * comment reply, then schedules a single async batch delete via + * cron. Comment TIDs must be collected here, while WP still has + * the comment rows: `wp_delete_post( $id, true )` fires + * `before_delete_post` first and only then iterates child comments, + * so this is the last opportunity to read them. + * + * The trash path (`Publisher::delete_post()`) already cascades + * comment deletes; this keeps the permanent-delete path symmetric + * so unpublishing or hard-deleting a post does not orphan its + * outbound replies on the PDS. * * @param int $post_id Post ID being deleted. */ @@ -338,8 +347,17 @@ public function on_before_delete( int $post_id ): void { $bsky_tid = \get_post_meta( $post_id, Transformer\Post::META_TID, true ); $doc_tid = \get_post_meta( $post_id, Transformer\Document::META_TID, true ); - if ( $bsky_tid || $doc_tid ) { - \wp_schedule_single_event( \time(), 'atmosphere_delete_records', array( $bsky_tid, $doc_tid ) ); + $comment_tids = \array_column( + Publisher::collect_published_comment_tids( $post_id ), + 'tid' + ); + + if ( $bsky_tid || $doc_tid || ! empty( $comment_tids ) ) { + \wp_schedule_single_event( + \time(), + 'atmosphere_delete_records', + array( $bsky_tid, $doc_tid, $comment_tids ) + ); } } @@ -632,11 +650,15 @@ static function (): void { \add_action( 'atmosphere_delete_records', - static function ( string $bsky_tid, string $doc_tid ): void { - Publisher::delete_post_by_tids( $bsky_tid, $doc_tid ); + static function ( string $bsky_tid, string $doc_tid, $comment_tids = array() ): void { + Publisher::delete_post_by_tids( + $bsky_tid, + $doc_tid, + \is_array( $comment_tids ) ? $comment_tids : array() + ); }, 10, - 2 + 3 ); /* diff --git a/includes/class-publisher.php b/includes/class-publisher.php index fcd7147..7566e5a 100644 --- a/includes/class-publisher.php +++ b/includes/class-publisher.php @@ -247,10 +247,14 @@ public static function delete_post( \WP_Post $post ): array|\WP_Error { * publish would refer to a non-existent record and the delete would * fail. * + * Public so the permanent-delete path (`on_before_delete`) can + * collect the same TIDs while comments still exist, before WP's + * natural cascade removes them. + * * @param int $post_id Post ID. * @return array */ - private static function collect_published_comment_tids( int $post_id ): array { + public static function collect_published_comment_tids( int $post_id ): array { $comments = \get_comments( array( 'post_id' => $post_id, @@ -285,12 +289,13 @@ private static function collect_published_comment_tids( int $post_id ): array { * * Used when a post is permanently deleted and post meta is no longer available. * - * @param string $bsky_tid Bluesky post TID (may be empty). - * @param string $doc_tid Document TID (may be empty). + * @param string $bsky_tid Bluesky post TID (may be empty). + * @param string $doc_tid Document TID (may be empty). + * @param string[] $comment_tids Comment reply TIDs to delete in the same batch. * @return array|\WP_Error */ - public static function delete_post_by_tids( string $bsky_tid, string $doc_tid ): array|\WP_Error { - if ( ! $bsky_tid && ! $doc_tid ) { + public static function delete_post_by_tids( string $bsky_tid, string $doc_tid, array $comment_tids = array() ): array|\WP_Error { + if ( ! $bsky_tid && ! $doc_tid && empty( $comment_tids ) ) { return new \WP_Error( 'atmosphere_not_published', \__( 'No TIDs provided.', 'atmosphere' ) ); } @@ -312,6 +317,17 @@ public static function delete_post_by_tids( string $bsky_tid, string $doc_tid ): ); } + foreach ( $comment_tids as $comment_tid ) { + if ( '' === (string) $comment_tid ) { + continue; + } + $writes[] = array( + '$type' => 'com.atproto.repo.applyWrites#delete', + 'collection' => 'app.bsky.feed.post', + 'rkey' => (string) $comment_tid, + ); + } + return API::apply_writes( $writes ); } diff --git a/tests/phpunit/tests/class-test-atmosphere.php b/tests/phpunit/tests/class-test-atmosphere.php index 7adb306..c31e97d 100644 --- a/tests/phpunit/tests/class-test-atmosphere.php +++ b/tests/phpunit/tests/class-test-atmosphere.php @@ -840,4 +840,76 @@ public function test_publish_comment_proceeds_after_parent_defer_cap() { 'Counter must be cleared once the child proceeds.' ); } + + /** + * Permanent delete must cascade to outbound comment replies. + * + * `before_delete_post` fires before WP iterates child comments, so + * `on_before_delete` is the only point at which we can read those + * comments' TIDs. The scheduled `atmosphere_delete_records` event + * must include them so a single batch removes the post, document, + * and every reply record. + */ + public function test_on_before_delete_includes_published_comment_tids() { + $post_id = self::factory()->post->create( array( 'post_status' => 'publish' ) ); + \update_post_meta( $post_id, Post::META_TID, 'bsky-tid-root' ); + \update_post_meta( $post_id, Document::META_TID, 'doc-tid-root' ); + + // Two published comment replies. + $comment_a = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_approved' => '1', + ) + ); + \update_comment_meta( $comment_a, Comment::META_TID, 'bsky-tid-a' ); + \update_comment_meta( $comment_a, Comment::META_URI, 'at://did:plc:test123/app.bsky.feed.post/bsky-tid-a' ); + + $comment_b = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_approved' => '1', + ) + ); + \update_comment_meta( $comment_b, Comment::META_TID, 'bsky-tid-b' ); + \update_comment_meta( $comment_b, Comment::META_URI, 'at://did:plc:test123/app.bsky.feed.post/bsky-tid-b' ); + + // One reply with a TID but no URI — never reached the PDS, must be excluded. + $comment_unpublished = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_approved' => '1', + ) + ); + \update_comment_meta( $comment_unpublished, Comment::META_TID, 'bsky-tid-orphan' ); + + $this->atmosphere->on_before_delete( $post_id ); + + $expected_args = array( 'bsky-tid-root', 'doc-tid-root', array( 'bsky-tid-a', 'bsky-tid-b' ) ); + + $this->assertNotFalse( + \wp_next_scheduled( 'atmosphere_delete_records', $expected_args ), + 'Expected atmosphere_delete_records to be scheduled with the published comment TIDs.' + ); + } + + /** + * Posts with no published comment replies still schedule the + * existing post + document delete pair — backward compatible. + */ + public function test_on_before_delete_without_comments_schedules_post_only() { + $post_id = self::factory()->post->create( array( 'post_status' => 'publish' ) ); + \update_post_meta( $post_id, Post::META_TID, 'bsky-tid-root' ); + \update_post_meta( $post_id, Document::META_TID, 'doc-tid-root' ); + + $this->atmosphere->on_before_delete( $post_id ); + + $this->assertNotFalse( + \wp_next_scheduled( + 'atmosphere_delete_records', + array( 'bsky-tid-root', 'doc-tid-root', array() ) + ), + 'Expected atmosphere_delete_records with empty comment list when the post has no replies.' + ); + } } From a1b7569071f24447c27b39ff8c5b0509bae15660 Mon Sep 17 00:00:00 2001 From: Matthias Pfefferle Date: Fri, 1 May 2026 09:35:31 +0200 Subject: [PATCH 10/10] Address PR #32 third-pass blocking review - Extract Atmosphere\get_cron_hooks / clear_scheduled_hooks so deactivate, Client::disconnect, and uninstall.php share one canonical hook list; prevents queued events from a previous connection firing against the current repo on reconnect or reactivate. - Log WP_Error from atmosphere_delete_records cron so a failed cascade leaves an operator-visible breadcrumb instead of silently leaking every reply record on the post. - Chunk applyWrites batches into 100-write groups in delete_post and delete_post_by_tids; the lexicon caps a single batch at 200, and cascading 200+ replies through one atomic call would fail with InvalidRequest. WP_Error now carries chunk_index/total/succeeded. - Reconcile after publish_comment: re-check eligibility once applyWrites returns success; if the comment was unapproved or deleted during the in-flight call, clear the just-written meta and schedule the TID-only delete cron so transient PDS errors retry through the standard channel. - Capture findings in code-style and test skills (cron-lifecycle three-way symmetry, never-swallow-WP_Error in cron handlers, in-flight race pattern, applyWrites stubbing for tests). --- .agents/skills/code-style/SKILL.md | 28 ++++ .agents/skills/test/SKILL.md | 39 +++++ .github/changelog/fix-comment-publish-race | 4 + .github/changelog/fix-delete-batch-chunking | 4 + .github/changelog/fix-disconnect-cron-cleanup | 4 + atmosphere.php | 11 +- includes/class-atmosphere.php | 77 ++++++++- includes/class-publisher.php | 77 ++++++++- includes/functions.php | 38 +++++ includes/oauth/class-client.php | 11 +- tests/phpunit/tests/class-test-atmosphere.php | 157 ++++++++++++++++++ tests/phpunit/tests/class-test-publisher.php | 85 ++++++++++ uninstall.php | 21 +-- 13 files changed, 524 insertions(+), 32 deletions(-) create mode 100644 .github/changelog/fix-comment-publish-race create mode 100644 .github/changelog/fix-delete-batch-chunking create mode 100644 .github/changelog/fix-disconnect-cron-cleanup diff --git a/.agents/skills/code-style/SKILL.md b/.agents/skills/code-style/SKILL.md index 323d3d8..ff41b38 100644 --- a/.agents/skills/code-style/SKILL.md +++ b/.agents/skills/code-style/SKILL.md @@ -113,3 +113,31 @@ Handle the full PKCE + DPoP + PAR native OAuth flow. \apply_filters( 'atmosphere_client_metadata', $metadata ); \apply_filters( 'atmosphere_syncable_post_types', array( 'post' ) ); ``` + +## Cron Lifecycle — three-way symmetry + +Every plugin-owned `wp_schedule_*` hook MUST also be in `Atmosphere\get_cron_hooks()` (`includes/functions.php`). That single list drives: + +- `Atmosphere\deactivate()` (`atmosphere.php`) +- `Atmosphere\OAuth\Client::disconnect()` (`includes/oauth/class-client.php`) +- `uninstall.php` + +When adding a new cron hook: + +1. Add the hook name to `get_cron_hooks()` — do not duplicate the literal in deactivate / disconnect / uninstall. +2. If the hook handler issues PDS writes without re-checking `is_connected()` (e.g. `atmosphere_delete_records`, `atmosphere_delete_comment_record`), the symmetry is load-bearing: a queued event from a previous connection would otherwise fire against a different repo on reconnect. +3. If the handler stores or sweeps commentmeta / postmeta keys, mirror those keys in `uninstall.php`. + +This pattern was extracted in PR #32; see review by @kraftbj for the cross-install risk that motivated it. + +## Cron Handler Errors — never swallow `WP_Error` + +Cron handlers in `register_async_hooks()` MUST surface `Publisher::*` errors via `error_log()` (typically through `log_cron_error()`). `wp_schedule_single_event` does not retry, so a silent drop loses the only signal operators have for transient PDS failures, expired refresh tokens, or DPoP nonce drift. + +When the handler operates on records the caller has already lost local state for (e.g. `atmosphere_delete_comment_record` after the WP comment row is gone), include the TID/identifier in the log line so the orphan is recoverable manually. + +## Inflight-state Races + +When a cron handler writes meta both *before* an `apply_writes` call (e.g. `Comment::get_rkey()` persists META_TID) and *after* (e.g. `store_comment_result()` writes META_URI), and a concurrent state change can short-circuit the cleanup gates that key off the *post-call* meta, the handler MUST re-check eligibility after the call returns and roll back if needed. + +Concrete pattern: `atmosphere_publish_comment` → `reconcile_comment_after_publish()`. Re-fetch the WP object, re-run the eligibility gate, schedule the orphan-cleanup cron (not direct delete) so transient PDS failures retry through the standard channel. diff --git a/.agents/skills/test/SKILL.md b/.agents/skills/test/SKILL.md index b047d72..08ccc5a 100644 --- a/.agents/skills/test/SKILL.md +++ b/.agents/skills/test/SKILL.md @@ -92,3 +92,42 @@ npm run env-test -- --stop-on-failure # Run single test method. npm run env-test -- --filter=test_specific_method ``` + +## Stubbing `applyWrites` calls + +The Publisher test fixture (`Test_Publisher`) exposes `register_capture()` plus `$captured_calls` / `$fail_call_indexes` for asserting on the `writes` batch and forcing per-call failures: + +```php +$this->fail_call_indexes = array( + 2 => new \WP_Error( 'atmosphere_pds_500', 'PDS rejected.' ), +); +$this->register_capture( $post_id ); +// ...exercise... +$this->assertCount( 3, $this->captured_calls ); +``` + +Outside the Publisher test, hook the `atmosphere_pre_apply_writes` filter directly (see Publisher::apply_writes — short-circuits before the HTTP layer, so DPoP-less test environments work). + +## Simulating in-flight races + +To reproduce a "state changed during the API call" race in tests, mutate the WP state from inside the `atmosphere_pre_apply_writes` filter callback and return a synthetic 2xx response. The plugin's hooks fire synchronously in the test process — the filter callback is the analogue of "the API call took long enough for another request to land". + +```php +\add_filter( + 'atmosphere_pre_apply_writes', + static function ( $short, $writes ) use ( $comment_id ) { + \wp_set_comment_status( $comment_id, 'hold' ); + return array( 'results' => array( /* synthetic */ ) ); + }, + 10, + 2 +); +``` + +Note: `wp_delete_comment( $id, true )` removes commentmeta synchronously, which can erase TIDs the reconcile path needs. Prefer status transitions (`hold`, `spam`) when possible. + +## Cron handlers in tests + +The plugin's `register_async_hooks()` runs at `plugins_loaded` (via the bootstrap), so cron handlers ARE registered before tests execute. Use `\do_action( 'atmosphere_publish_comment', $comment_id )` to fire a handler synchronously; assert on `\wp_next_scheduled()` for follow-up scheduling. + +Always clean up scheduled hooks in `tear_down()` — leftover events from one test become flaky preconditions for the next. diff --git a/.github/changelog/fix-comment-publish-race b/.github/changelog/fix-comment-publish-race new file mode 100644 index 0000000..958b3c1 --- /dev/null +++ b/.github/changelog/fix-comment-publish-race @@ -0,0 +1,4 @@ +Significance: patch +Type: fixed + +Remove a comment reply from Bluesky if the comment was deleted or unapproved while it was being published, instead of leaving an orphan reply behind. diff --git a/.github/changelog/fix-delete-batch-chunking b/.github/changelog/fix-delete-batch-chunking new file mode 100644 index 0000000..a4d2a4c --- /dev/null +++ b/.github/changelog/fix-delete-batch-chunking @@ -0,0 +1,4 @@ +Significance: patch +Type: fixed + +Break up large cleanup batches when removing a post and its replies so deletion still completes on threads with many comments. diff --git a/.github/changelog/fix-disconnect-cron-cleanup b/.github/changelog/fix-disconnect-cron-cleanup new file mode 100644 index 0000000..9e8497f --- /dev/null +++ b/.github/changelog/fix-disconnect-cron-cleanup @@ -0,0 +1,4 @@ +Significance: patch +Type: fixed + +Clear queued sync events on disconnect, deactivation, and uninstall so leftover jobs cannot fire against a different connected account. diff --git a/atmosphere.php b/atmosphere.php index 4751cb3..227a296 100644 --- a/atmosphere.php +++ b/atmosphere.php @@ -64,16 +64,7 @@ function activate() { * Deactivation hook. */ function deactivate() { - \wp_clear_scheduled_hook( 'atmosphere_refresh_token' ); - \wp_clear_scheduled_hook( 'atmosphere_sync_reactions' ); - \wp_clear_scheduled_hook( 'atmosphere_sync_publication' ); - \wp_clear_scheduled_hook( 'atmosphere_delete_records' ); - \wp_clear_scheduled_hook( 'atmosphere_publish_comment' ); - \wp_clear_scheduled_hook( 'atmosphere_update_comment' ); - \wp_clear_scheduled_hook( 'atmosphere_delete_comment' ); - \wp_clear_scheduled_hook( 'atmosphere_delete_comment_record' ); - // Clear the legacy hook name in case an earlier PR-6 build scheduled it. - \wp_clear_scheduled_hook( 'atmosphere_sync_comments' ); + clear_scheduled_hooks(); \flush_rewrite_rules(); } \register_deactivation_hook( __FILE__, __NAMESPACE__ . '\deactivate' ); diff --git a/includes/class-atmosphere.php b/includes/class-atmosphere.php index c555023..da8f898 100644 --- a/includes/class-atmosphere.php +++ b/includes/class-atmosphere.php @@ -753,11 +753,27 @@ static function (): void { \add_action( 'atmosphere_delete_records', static function ( $bsky_tids, string $doc_tid, $comment_tids = array() ): void { - Publisher::delete_post_by_tids( - $bsky_tids, - $doc_tid, - \is_array( $comment_tids ) ? $comment_tids : array() - ); + $comment_tids = \is_array( $comment_tids ) ? $comment_tids : array(); + $result = Publisher::delete_post_by_tids( $bsky_tids, $doc_tid, $comment_tids ); + + if ( \is_wp_error( $result ) ) { + /* + * One-shot cron event with no retry: dropping this error + * would orphan every record in the cascade (root + thread + * replies + outbound comment replies + document) on the + * PDS with no operator-visible breadcrumb. + */ + \error_log( // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log + \sprintf( + '[atmosphere] delete_records failed (bsky=%d, doc=%s, comments=%d): %s — %s', + \is_array( $bsky_tids ) ? \count( $bsky_tids ) : (int) ! empty( $bsky_tids ), + $doc_tid ? 'yes' : 'no', + \count( $comment_tids ), + $result->get_error_code(), + $result->get_error_message() + ) + ); + } }, 10, 3 @@ -794,6 +810,10 @@ static function ( int $comment_id ): void { $result = Publisher::publish_comment( $comment ); self::log_cron_error( 'publish_comment', $comment_id, $result ); + + if ( ! \is_wp_error( $result ) ) { + self::reconcile_comment_after_publish( $comment_id ); + } } ); @@ -946,4 +966,51 @@ private static function log_cron_error( string $op, int $comment_id, $result ): ) ); } + + /** + * Roll back a successful publish if the comment became ineligible + * during the in-flight applyWrites. + * + * The race: `Comment::get_rkey()` persists META_TID before the API + * call, and META_URI is only written after success. Both + * `schedule_comment_delete` and `on_comment_before_delete` require + * META_URI to schedule cleanup. A moderator who deletes or + * unapproves the comment while applyWrites is in flight therefore + * leaves a live Bluesky reply with no scheduled cleanup once + * `store_comment_result()` finally writes META_URI. + * + * Re-checking eligibility after publish closes that race. If the + * comment is gone or no longer eligible, we clear the meta we just + * wrote and schedule the same TID-only delete event the + * permanent-delete path uses, so transient PDS failures retry via + * the standard cleanup channel rather than getting dropped here. + * + * @param int $comment_id Comment ID just published. + */ + private static function reconcile_comment_after_publish( int $comment_id ): void { + $fresh = \get_comment( $comment_id ); + + if ( $fresh instanceof \WP_Comment && self::should_publish_comment( $fresh ) ) { + return; + } + + $tid = (string) \get_comment_meta( $comment_id, Comment::META_TID, true ); + + \delete_comment_meta( $comment_id, Comment::META_TID ); + \delete_comment_meta( $comment_id, Comment::META_URI ); + \delete_comment_meta( $comment_id, Comment::META_CID ); + \delete_comment_meta( $comment_id, Reaction_Sync::META_SOURCE_ID ); + + if ( '' === $tid ) { + return; + } + + $args = array( $tid ); + + if ( \wp_next_scheduled( 'atmosphere_delete_comment_record', $args ) ) { + return; + } + + \wp_schedule_single_event( \time(), 'atmosphere_delete_comment_record', $args ); + } } diff --git a/includes/class-publisher.php b/includes/class-publisher.php index 6ddeb84..bf139b5 100644 --- a/includes/class-publisher.php +++ b/includes/class-publisher.php @@ -871,6 +871,15 @@ private static function persist_rewrite_failure( ); } + /** + * Maximum writes per `applyWrites` call. + * + * The AT Protocol `com.atproto.repo.applyWrites` lexicon caps the + * `writes` array at 200. We chunk well under that to leave headroom + * and keep request bodies small. + */ + private const APPLY_WRITES_CHUNK_SIZE = 100; + /** * Delete every bsky record (root + thread replies + outbound * comment replies) and the document for a post. @@ -879,8 +888,12 @@ private static function persist_rewrite_failure( * single-record posts (falls back to the mirrored `META_URI` / * `META_TID` / `META_CID` keys). Outbound comment replies live in * our own repo keyed by their own TIDs — the AT Protocol has no - * cascade semantics, so they have to be enumerated and bundled into - * the same `applyWrites` batch or they orphan on Bluesky. + * cascade semantics, so they have to be enumerated alongside the + * post records or they orphan on Bluesky. + * + * Writes are chunked into bounded `applyWrites` calls (the lexicon + * caps a single batch at 200), so a high-traffic post with a long + * reply tail still cleans up cleanly. * * @param \WP_Post $post WordPress post. * @return array|\WP_Error @@ -932,7 +945,7 @@ public static function delete_post( \WP_Post $post ): array|\WP_Error { ); } - $result = API::apply_writes( $writes ); + $result = self::apply_writes_chunked( $writes ); if ( \is_wp_error( $result ) ) { // Leave meta intact so a retry can complete. @@ -1051,7 +1064,63 @@ public static function delete_post_by_tids( $bsky_tids, string $doc_tid, array $ ); } - return API::apply_writes( $writes ); + return self::apply_writes_chunked( $writes ); + } + + /** + * Submit a `writes` batch in lexicon-bounded chunks. + * + * The PDS rejects an `applyWrites` whose `writes` array exceeds 200 + * entries (`InvalidRequest`), so a high-traffic post with hundreds of + * outbound comment replies would otherwise fail the entire cascade + * atomically. + * + * Chunks are submitted sequentially. The first chunk failure is + * returned; the operator-visible error code includes how many chunks + * had already succeeded so the partial-success state is visible. The + * caller is responsible for keeping local meta intact on error so a + * retry can complete the remaining chunks. + * + * On success, results from each chunk are concatenated into a single + * `results` array — preserving the shape callers expect from + * `API::apply_writes()`. + * + * @param array $writes Full write batch. + * @return array|\WP_Error + */ + private static function apply_writes_chunked( array $writes ): array|\WP_Error { + if ( \count( $writes ) <= self::APPLY_WRITES_CHUNK_SIZE ) { + return API::apply_writes( $writes ); + } + + $chunks = \array_chunk( $writes, self::APPLY_WRITES_CHUNK_SIZE ); + $total = \count( $chunks ); + $results = array(); + $succeeded = 0; + + foreach ( $chunks as $index => $chunk ) { + $response = API::apply_writes( $chunk ); + + if ( \is_wp_error( $response ) ) { + $response->add_data( + array( + 'chunk_index' => $index, + 'chunks_total' => $total, + 'chunks_succeeded' => $succeeded, + ), + 'atmosphere_chunked_apply_writes' + ); + return $response; + } + + if ( isset( $response['results'] ) && \is_array( $response['results'] ) ) { + $results = \array_merge( $results, $response['results'] ); + } + + ++$succeeded; + } + + return array( 'results' => $results ); } /** diff --git a/includes/functions.php b/includes/functions.php index 60524e7..ad91d46 100644 --- a/includes/functions.php +++ b/includes/functions.php @@ -129,6 +129,44 @@ function get_pds_endpoint(): string { return get_connection()['pds_endpoint'] ?? ''; } +/** + * Plugin-owned WP-Cron hooks. + * + * Single source of truth for `deactivate()`, `Client::disconnect()`, and + * `uninstall.php`. Keeping the lists in sync prevents queued events from + * a previous install/connection from firing against the current one and + * (worst case) issuing applyWrites against a different repo. + * + * @return string[] + */ +function get_cron_hooks(): array { + return array( + 'atmosphere_refresh_token', + 'atmosphere_sync_reactions', + 'atmosphere_sync_publication', + 'atmosphere_publish_post', + 'atmosphere_update_post', + 'atmosphere_delete_post', + 'atmosphere_delete_records', + 'atmosphere_publish_comment', + 'atmosphere_update_comment', + 'atmosphere_delete_comment', + 'atmosphere_delete_comment_record', + // Legacy hook from an early build of the comment publisher; cleared + // for users upgrading from that snapshot. + 'atmosphere_sync_comments', + ); +} + +/** + * Clear every plugin-owned scheduled hook. + */ +function clear_scheduled_hooks(): void { + foreach ( get_cron_hooks() as $hook ) { + \wp_clear_scheduled_hook( $hook ); + } +} + /** * Get post types that publish to AT Protocol. * diff --git a/includes/oauth/class-client.php b/includes/oauth/class-client.php index b341de8..0aa6b09 100644 --- a/includes/oauth/class-client.php +++ b/includes/oauth/class-client.php @@ -12,6 +12,8 @@ \defined( 'ABSPATH' ) || exit; +use function Atmosphere\clear_scheduled_hooks; + /** * OAuth client that manages the authorization lifecycle. */ @@ -506,10 +508,17 @@ public static function access_token(): string|\WP_Error { } /** - * Disconnect: remove all stored credentials. + * Disconnect: remove all stored credentials and clear queued cron events. + * + * Queued events (`atmosphere_delete_records`, + * `atmosphere_delete_comment_record`) issue applyWrites without a + * connection check, so a disconnect→reconnect-to-different-account + * cycle would otherwise fire deletes against the new account's repo. + * Mirrors the cleanup performed on plugin deactivate / uninstall. */ public static function disconnect(): void { \delete_option( 'atmosphere_connection' ); + clear_scheduled_hooks(); } /** diff --git a/tests/phpunit/tests/class-test-atmosphere.php b/tests/phpunit/tests/class-test-atmosphere.php index 69bfd0f..e7d5a55 100644 --- a/tests/phpunit/tests/class-test-atmosphere.php +++ b/tests/phpunit/tests/class-test-atmosphere.php @@ -1002,4 +1002,161 @@ public function test_new_publish_respects_allowlist_even_when_filter_narrows() { \remove_filter( 'atmosphere_syncable_post_types', $narrow ); } } + + /** + * `Atmosphere\deactivate` clears every plugin-owned cron hook so a + * deactivate→reactivate cycle (or deactivate→reconnect→reactivate) + * cannot fire stale events against the new connection's repo. + */ + public function test_deactivate_clears_all_cron_hooks() { + $hooks = \Atmosphere\get_cron_hooks(); + + foreach ( $hooks as $hook ) { + \wp_schedule_single_event( \time() + 60, $hook, array() ); + } + foreach ( $hooks as $hook ) { + $this->assertNotFalse( \wp_next_scheduled( $hook ), "Setup: {$hook} must be scheduled." ); + } + + \Atmosphere\deactivate(); + + foreach ( $hooks as $hook ) { + $this->assertFalse( + \wp_next_scheduled( $hook ), + "deactivate() must clear scheduled hook: {$hook}" + ); + } + } + + /** + * `Client::disconnect` clears the same crons as `deactivate()`. + * + * A disconnect→reconnect-to-different-account cycle would otherwise + * fire `atmosphere_delete_records` / + * `atmosphere_delete_comment_record` against the new account's + * repo, since neither cron handler re-checks the connection's DID + * before issuing the delete. + */ + public function test_disconnect_clears_all_cron_hooks() { + $hooks = \Atmosphere\get_cron_hooks(); + + foreach ( $hooks as $hook ) { + \wp_schedule_single_event( \time() + 60, $hook, array() ); + } + + \Atmosphere\OAuth\Client::disconnect(); + + foreach ( $hooks as $hook ) { + $this->assertFalse( + \wp_next_scheduled( $hook ), + "Client::disconnect must clear scheduled hook: {$hook}" + ); + } + } + + /** + * Race: a moderator unapproves the comment while applyWrites is in + * flight. `Comment::get_rkey` writes META_TID before the API call, + * but META_URI is only written after the call returns. The status + * transition's cleanup hook requires META_URI, so it silently + * short-circuits — and once the in-flight publish lands, the + * record is live on Bluesky with no scheduled cleanup. + * + * After publish, `reconcile_comment_after_publish` re-fetches the + * comment; if it is no longer eligible the meta we just wrote is + * cleared and the TID-only delete cron used by the permanent-delete + * path is scheduled. + */ + public function test_reconcile_after_publish_schedules_delete_when_comment_unapproved_mid_publish() { + $comment = $this->make_eligible_comment(); + $comment_id = (int) $comment->comment_ID; + + $captured_tid = ''; + \add_filter( + 'atmosphere_pre_apply_writes', + static function ( $short, $writes ) use ( $comment_id, &$captured_tid ) { + $captured_tid = $writes[0]['rkey'] ?? ''; + + /* + * Simulate the moderator unapproving the comment during + * the in-flight applyWrites. The status transition + * fires on_comment_status_change which would normally + * schedule a delete, but META_URI is empty during the + * race window so it short-circuits. + */ + \wp_set_comment_status( $comment_id, 'hold' ); + + return array( + 'results' => array( + array( + 'uri' => 'at://did:plc:test123/app.bsky.feed.post/' . $captured_tid, + 'cid' => 'bafyreibraced', + ), + ), + ); + }, + 10, + 2 + ); + + \do_action( 'atmosphere_publish_comment', $comment_id ); + + $this->assertNotEmpty( $captured_tid, 'applyWrites filter must have fired.' ); + + $this->assertEmpty( + \get_comment_meta( $comment_id, Comment::META_TID, true ), + 'Reconcile must clear the orphan TID meta.' + ); + $this->assertEmpty( + \get_comment_meta( $comment_id, Comment::META_URI, true ), + 'Reconcile must clear the orphan URI meta.' + ); + + $this->assertNotFalse( + \wp_next_scheduled( 'atmosphere_delete_comment_record', array( $captured_tid ) ), + 'Reconcile must schedule delete-by-TID for the orphan record.' + ); + + \remove_all_filters( 'atmosphere_pre_apply_writes' ); + \wp_clear_scheduled_hook( 'atmosphere_delete_comment_record' ); + } + + /** + * If the comment is still eligible after publish (the normal case), + * reconcile is a no-op: meta survives and no delete is scheduled. + */ + public function test_reconcile_after_publish_is_noop_for_still_eligible_comment() { + $comment = $this->make_eligible_comment(); + $comment_id = (int) $comment->comment_ID; + + \add_filter( + 'atmosphere_pre_apply_writes', + static function ( $short, $writes ) { + $results = array(); + foreach ( $writes as $write ) { + $rkey = $write['rkey'] ?? 'tid'; + $results[] = array( + 'uri' => 'at://did:plc:test123/app.bsky.feed.post/' . $rkey, + 'cid' => 'bafyreibtest', + ); + } + return array( 'results' => $results ); + }, + 10, + 2 + ); + + \do_action( 'atmosphere_publish_comment', $comment_id ); + + $this->assertNotEmpty( + \get_comment_meta( $comment_id, Comment::META_URI, true ), + 'Eligible comment must keep its URI meta.' + ); + $this->assertFalse( + \wp_next_scheduled( 'atmosphere_delete_comment_record' ), + 'No delete should be scheduled when the comment is still eligible.' + ); + + \remove_all_filters( 'atmosphere_pre_apply_writes' ); + } } diff --git a/tests/phpunit/tests/class-test-publisher.php b/tests/phpunit/tests/class-test-publisher.php index 4572ba4..41332db 100644 --- a/tests/phpunit/tests/class-test-publisher.php +++ b/tests/phpunit/tests/class-test-publisher.php @@ -1765,4 +1765,89 @@ public function test_delete_legacy_single_post_meta() { $this->assertSame( '', \get_post_meta( $post->ID, Post::META_TID, true ) ); $this->assertSame( '', \get_post_meta( $post->ID, Post::META_CID, true ) ); } + + /** + * `delete_post_by_tids` chunks oversized batches into multiple + * `applyWrites` calls. The lexicon caps a single batch at 200 writes; + * a high-traffic post with hundreds of outbound comment replies must + * still clean up cleanly rather than failing the whole cascade. + */ + public function test_delete_post_by_tids_chunks_oversized_batches() { + $this->fail_call_indexes = array(); + $this->register_capture( 0 ); + + $comment_tids = array(); + for ( $i = 0; $i < 250; $i++ ) { + $comment_tids[] = 'reply-' . $i; + } + + $result = Publisher::delete_post_by_tids( + array( 'root-tid' ), + 'doc-tid', + $comment_tids + ); + + $this->assertIsArray( $result ); + // 252 total writes / 100 per chunk = 3 calls. + $this->assertCount( 3, $this->captured_calls ); + + $total_writes = 0; + foreach ( $this->captured_calls as $call ) { + $total_writes += \count( $call['writes'] ); + $this->assertLessThanOrEqual( 100, \count( $call['writes'] ) ); + } + $this->assertSame( 252, $total_writes ); + } + + /** + * Chunked deletes report the chunk index and how many chunks + * succeeded when one fails partway through, so operators can see + * the partial-success state in the error log rather than treating + * it as a clean failure. + */ + public function test_delete_post_by_tids_chunked_failure_carries_progress_data() { + $this->fail_call_indexes = array( + 2 => new \WP_Error( 'atmosphere_pds_500', 'PDS rejected batch.' ), + ); + $this->register_capture( 0 ); + + $comment_tids = array(); + for ( $i = 0; $i < 250; $i++ ) { + $comment_tids[] = 'reply-' . $i; + } + + $result = Publisher::delete_post_by_tids( + array( 'root-tid' ), + 'doc-tid', + $comment_tids + ); + + $this->assertWPError( $result ); + $this->assertSame( 'atmosphere_pds_500', $result->get_error_code() ); + + $data = $result->get_error_data( 'atmosphere_chunked_apply_writes' ); + $this->assertIsArray( $data ); + $this->assertSame( 1, $data['chunk_index'] ); + $this->assertSame( 3, $data['chunks_total'] ); + $this->assertSame( 1, $data['chunks_succeeded'] ); + } + + /** + * Small batches (<= chunk size) take the single-call path and do + * not touch the chunking layer's results-merging. + */ + public function test_delete_post_by_tids_small_batch_uses_single_call() { + $this->fail_call_indexes = array(); + $this->register_capture( 0 ); + + $result = Publisher::delete_post_by_tids( + array( 'root-tid' ), + 'doc-tid', + array( 'reply-1', 'reply-2' ) + ); + + $this->assertIsArray( $result ); + $this->assertCount( 1, $this->captured_calls ); + $this->assertCount( 4, $this->captured_calls[0]['writes'] ); + } } diff --git a/uninstall.php b/uninstall.php index 5c57d4e..bc685fb 100644 --- a/uninstall.php +++ b/uninstall.php @@ -7,28 +7,25 @@ * @package Atmosphere */ +use function Atmosphere\clear_scheduled_hooks; + if ( ! defined( 'WP_UNINSTALL_PLUGIN' ) ) { exit; } +// Load helpers so the cron-hook list stays in lock-step with deactivate() +// and Client::disconnect(). uninstall.php is loaded by WordPress without +// the plugin itself being booted, so this require is necessary. +require_once __DIR__ . '/includes/functions.php'; + // Remove options. delete_option( 'atmosphere_connection' ); delete_option( 'atmosphere_publication_tid' ); delete_option( 'atmosphere_publication_uri' ); delete_option( 'atmosphere_auto_publish' ); -// Remove scheduled events. -wp_clear_scheduled_hook( 'atmosphere_refresh_token' ); -wp_clear_scheduled_hook( 'atmosphere_publish_post' ); -wp_clear_scheduled_hook( 'atmosphere_update_post' ); -wp_clear_scheduled_hook( 'atmosphere_delete_post' ); -wp_clear_scheduled_hook( 'atmosphere_delete_records' ); -wp_clear_scheduled_hook( 'atmosphere_sync_publication' ); -wp_clear_scheduled_hook( 'atmosphere_sync_reactions' ); -wp_clear_scheduled_hook( 'atmosphere_publish_comment' ); -wp_clear_scheduled_hook( 'atmosphere_update_comment' ); -wp_clear_scheduled_hook( 'atmosphere_delete_comment' ); -wp_clear_scheduled_hook( 'atmosphere_delete_comment_record' ); +// Remove scheduled events via the canonical helper. +clear_scheduled_hooks(); // Remove post meta. global $wpdb;