Skip to content

Guard comment and hard-delete cleanup against wrong-repo deletes - #216

Open
pfefferle wants to merge 9 commits into
trunkfrom
add/did-guards-comment-hard-delete
Open

Guard comment and hard-delete cleanup against wrong-repo deletes#216
pfefferle wants to merge 9 commits into
trunkfrom
add/did-guards-comment-hard-delete

Conversation

@pfefferle

Copy link
Copy Markdown
Member

Fixes #70
Fixes #71

Proposed changes:

#63 added a guard so deleting a post never fires an applyWrites#delete against the wrong repo after you disconnect and reconnect to a different Bluesky account. That guard only covered delete_post(). The comment cleanup paths and the permanent-delete cron path had the same hole: they would issue the delete against the currently-connected account for a record that actually lives on the old one, get an idempotent 200 back, clear the local meta, and leave the original record stranded with no breadcrumb.

This extends the same guard to those paths:

  • Comment::META_DID is now written on every publish, mirroring Post::get_rkey(), so a reply carries the account it was minted under. It is compared before writing, so a republish of an unchanged comment is a no-op.
  • delete_comment() reads it directly. delete_comment_by_tid() and delete_post_by_tids() take the origin DID as an argument, because by the time their cron fires the row and its meta are already gone. The DID is captured at schedule time (on_before_delete, on_comment_before_delete, reconcile_comment_after_publish) and threaded through the cron args.
  • All four call sites now share one record_is_foreign() helper, so the "empty origin or empty current DID disables the guard" rule lives in one place. delete_post() is folded onto it too.

An empty origin DID (records from before provenance existed, or a cron event queued before this shipped) disables the guard, so nothing regresses and already-queued 3-arg / 1-arg cron events still fire cleanly (the handlers default the new params).

Known gap, split into #215

The comment batch inside the post hard-delete cascade still guards only on the post's root DID, not each reply's. In practice the reply also gets its own guarded per-comment delete event on permanent deletion, so the mismatch is still logged; the cascade just issues a redundant wrong-repo no-op alongside it. Closing it properly means touching the shipped delete_post() the same way, so I split it into #215 rather than grow this PR.

Other information:

  • Have you written new tests for your changes, if applicable?

Testing instructions:

Full end-to-end needs two Bluesky accounts:

  1. Connect to account A, publish a post, approve a comment so it publishes as a reply.
  2. Disconnect, reconnect to account B.
  3. Permanently delete the post (and separately: unapprove or delete the comment).
  4. Confirm the delete is refused with an atmosphere_did_mismatch breadcrumb in the debug log instead of silently clearing meta, and the records on A are untouched.

Or just run the guard tests:

npm run env-test -- --filter='did_mismatch|persists_origin_did'

Changelog entry

Already added in .github/changelog/fix-did-guard-comment-hard-delete (patch / fixed), so the changelog form below is left unchecked.

…count

Extends the wrong-repo-delete guard (Post::META_DID, PR #63) to the comment
cleanup paths and the post hard-delete cascade. Adds Comment::META_DID,
written on every publish, and threads the origin DID captured at
delete-schedule time through the delete crons so a record minted under a
previous account is surfaced instead of silently no-op-deleted against the
current repo. Folds delete_post() onto the shared record_is_foreign() helper.

Closes #70. Closes #71.
Copilot AI review requested due to automatic review settings July 24, 2026 13:47
@pfefferle pfefferle self-assigned this Jul 24, 2026
@pfefferle
pfefferle requested a review from a team July 24, 2026 13:47
@github-actions github-actions Bot added [Feature] Publisher Publishing to AT Protocol [Feature] Transformer AT Protocol record transformers [Tests] Includes Tests PR includes test changes labels Jul 24, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Extends the existing “wrong-repo delete” DID-provenance guard (added earlier for posts) to comment cleanup and hard-delete cron cleanup paths, so disconnecting and reconnecting to a different Bluesky account won’t silently orphan records on the previous repo.

Changes:

  • Persist comment origin DID (Comment::META_DID) during reply record key minting and thread it through delete paths that run after WP rows/meta are gone (cron handlers).
  • Add a shared Publisher::record_is_foreign() guard and apply it to delete_comment(), delete_comment_by_tid(), and delete_post_by_tids() (plus refactor delete_post() to use it).
  • Add/adjust PHPUnit coverage for DID-mismatch behavior and updated cron arg signatures; add a patch changelog entry.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
includes/transformer/class-comment.php Adds Comment::META_DID and persists DID provenance during get_rkey() to support later foreign-repo delete guards.
includes/class-publisher.php Introduces record_is_foreign() and applies DID-mismatch guards to comment deletes and hard-delete (delete_post_by_tids) cleanup.
includes/class-atmosphere.php Captures origin DIDs at scheduling time and threads them through cron args; updates cron handlers to accept new parameters with legacy-safe defaults.
tests/phpunit/tests/transformer/class-test-comment.php Adds test ensuring comment origin DID is persisted/refreshed as expected by the transformer.
tests/phpunit/tests/class-test-publisher.php Adds tests asserting DID-mismatch bails issue no writes and preserve meta; covers hard-delete DID guard behavior.
tests/phpunit/tests/class-test-atmosphere.php Updates cron scheduling assertions to match new arg shapes (including legacy empty-origin behavior).
.github/changelog/fix-did-guard-comment-hard-delete Adds patch changelog entry documenting the fix.

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

Comment thread includes/transformer/class-comment.php
The new comment DID-provenance key was missing from uninstall's
commentmeta cleanup list, leaving orphaned rows behind.
…eta cleanup

Fold the four inline atmosphere_did_mismatch WP_Errors onto a shared
did_mismatch_error() helper, and the repeated comment record-meta
deletion onto Publisher::clear_comment_record_meta(), so a future meta
key is added in one place. No behavior change.
jeherve
jeherve previously approved these changes Aug 28, 2026

@jeherve jeherve left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good. I only have a minor comment, not a blocker. 🚢

Comment thread includes/transformer/class-comment.php Outdated
Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Publisher Publishing to AT Protocol [Feature] Transformer AT Protocol record transformers [Tests] Includes Tests PR includes test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add DID-mismatch guard to delete_post_by_tids (hard-delete path) Add Comment::META_DID guard for comment cleanup paths

3 participants