From 7b5dc0b1495712da7b09c2fe0f67b422aec49906 Mon Sep 17 00:00:00 2001 From: Bretton Date: Sat, 08 Aug 2026 19:55:38 +0000 Subject: [PATCH] fix(posts): re-review round on Opus 5 — postv2 aggregation + edit-path validation parity The task 6-8 reviews were re-run on a stronger model after discovering the original panel had fallen back to Opus 4.8 mid-loop. Task 6's re-review found two classes of defect the earlier pass missed. AGGREGATION (product-breaking): vote and comment counting still switched on the legacy "social.coves.community.post" literal with no postv2 arm, so every author-owned post silently never accumulated votes or comments — the row was indexed, the count was never touched, and the failure logged as an "unsupported collection" line nobody read. The reindex-votes repair tool had the same gap, so it could not even fix the drift. Routes both collections through one shared posts.IsPostCollection predicate. Also fixes comment hydration stamping the deprecated $type (and a fabricated author field) on author-owned posts served through getComments. EDIT-PATH VALIDATION: UpdatePost never ran normalizeEmbedURIs or NormalizeLinkURIs, so an author could create a clean post and edit it into a signed record create would have refused — javascript:/data: embed and facet link URIs, unencoded URIs, and an uncapped external.sources array. Normalization now lives inside the shared gate (validatePostContent -> normalizeAndValidatePostContent) so both paths get it once. tags/langs were also written with zero validation against a lexicon that caps them, and were silently discarded on create while honored on update; both now carry through create and are validated in the shared gate. Plus: dead error return on enhanceExternalEmbed, a stale STUB header on postv2.go, and the update lexicon's error vocabulary reconciled additively. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QDvRJ45k6E5KrBARDHUtiM --- cmd/reindex-votes/main.go | 53 ++++++++++++++++++++++++++++++++++++----------------- cmd/reindex-votes/main_test.go | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ cmd/rematerialize-posts/main.go | 8 ++++---- internal/atproto/jetstream/comment_consumer.go | 14 +++++++++++--- internal/atproto/jetstream/post_consumer.go | 2 +- internal/atproto/jetstream/postv2_aggregation_test.go | 162 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/atproto/jetstream/vote_consumer.go | 31 +++++++++++++++++++++---------- internal/atproto/lexicon/social/coves/community/post/update.json | 12 ++++++++++-- internal/core/comments/comment_post_record_test.go | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/core/comments/comment_service.go | 56 +++++++++++++++++++++++++++++++++++++++++++++----------- internal/core/posts/admit_matrix_test.go | 42 ++++++++++++++++++++++++++++++++++++++++-- internal/core/posts/collection_routing_test.go | 34 ++++++++++++++++++++++++++++++++++ internal/core/posts/community_writer.go | 12 ++++++++++++ internal/core/posts/embed_validation.go | 19 +++++++++++++------ internal/core/posts/post.go | 21 +++++++++++++++++++++ internal/core/posts/post_content_validation_test.go | 270 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/core/posts/postv2.go | 8 ++++++-- internal/core/posts/service.go | 277 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------- internal/core/posts/service_get_posts_test.go | 22 +++++++++++----------- internal/core/posts/service_update_validation_test.go | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/db/postgres/post_repo.go | 2 +- internal/validation/lexicon.go | 10 +++++++++- tests/lexicon_post_update_errors_test.go | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 23 file(s) changed, 1275 insertion(s)(+), 128 deletion(s)(-) diff --git a/cmd/reindex-votes/main.go b/cmd/reindex-votes/main.go --- a/cmd/reindex-votes/main.go +++ b/cmd/reindex-votes/main.go @@ -14,6 +14,8 @@ "os" "strings" "time" + "Coves/internal/core/posts" + _ "github.com/lib/pq" ) @@ -229,23 +231,8 @@ return fmt.Errorf("failed to insert vote: %w", err) } // Update post/comment counts - collection := extractCollectionFromURI(subjectURI) - var updateQuery string - - switch collection { - case "social.coves.community.post": - if direction == "up" { - updateQuery = `UPDATE posts SET upvote_count = upvote_count + 1, score = upvote_count + 1 - downvote_count WHERE uri = $1 AND deleted_at IS NULL` - } else { - updateQuery = `UPDATE posts SET downvote_count = downvote_count + 1, score = upvote_count - (downvote_count + 1) WHERE uri = $1 AND deleted_at IS NULL` - } - case "social.coves.community.comment": - if direction == "up" { - updateQuery = `UPDATE comments SET upvote_count = upvote_count + 1, score = upvote_count + 1 - downvote_count WHERE uri = $1 AND deleted_at IS NULL` - } else { - updateQuery = `UPDATE comments SET downvote_count = downvote_count + 1, score = upvote_count - (downvote_count + 1) WHERE uri = $1 AND deleted_at IS NULL` - } - default: + updateQuery := voteCountUpdateQuery(extractCollectionFromURI(subjectURI), direction) + if updateQuery == "" { // Unknown collection, just index the vote return tx.Commit() } @@ -255,6 +242,38 @@ return fmt.Errorf("failed to update vote counts: %w", err) } return tx.Commit() +} + +// commentCollection is the collection a vote's subject names when the vote was +// cast on a comment. +const commentCollection = "social.coves.community.comment" + +// voteCountUpdateQuery returns the statement that folds one vote into its +// subject's denormalized counters, or "" when the subject is something this tool +// does not count. +// +// It is a function rather than an inline switch so the routing can be tested +// without a database: this tool exists to REPAIR counts that drifted, so a +// subject it silently declines to route is a rerun that reports success and +// changes nothing — the least visible failure the tool can have. +func voteCountUpdateQuery(subjectCollection, direction string) string { + switch { + // Both post collections, because both are indexed into the `posts` table for + // as long as the author-owned flip's dual-collection window is open + // (posts.IsPostCollection). + case posts.IsPostCollection(subjectCollection): + if direction == "up" { + return `UPDATE posts SET upvote_count = upvote_count + 1, score = upvote_count + 1 - downvote_count WHERE uri = $1 AND deleted_at IS NULL` + } + return `UPDATE posts SET downvote_count = downvote_count + 1, score = upvote_count - (downvote_count + 1) WHERE uri = $1 AND deleted_at IS NULL` + case subjectCollection == commentCollection: + if direction == "up" { + return `UPDATE comments SET upvote_count = upvote_count + 1, score = upvote_count + 1 - downvote_count WHERE uri = $1 AND deleted_at IS NULL` + } + return `UPDATE comments SET downvote_count = downvote_count + 1, score = upvote_count - (downvote_count + 1) WHERE uri = $1 AND deleted_at IS NULL` + default: + return "" + } } func extractCollectionFromURI(uri string) string { diff --git a/cmd/reindex-votes/main_test.go b/cmd/reindex-votes/main_test.go new file mode 100644 --- /dev/null +++ b/cmd/reindex-votes/main_test.go @@ -0,0 +1,54 @@ +package main + +import ( + "strings" + "testing" + + "Coves/internal/core/posts" +) + +// The reconciliation tool exists to REPAIR vote counts, so a subject collection +// it cannot route is a subject it silently refuses to repair — the failure mode +// is a rerun that reports success and changes nothing. +func TestVoteCountUpdateQuery_RoutesBothPostCollectionsToPosts(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + collection string + direction string + wantTable string + wantColumn string + }{ + {"author-owned post, upvote", posts.PostV2Collection, "up", "UPDATE posts", "upvote_count"}, + {"author-owned post, downvote", posts.PostV2Collection, "down", "UPDATE posts", "downvote_count"}, + {"deprecated community-repo post, upvote", posts.LegacyPostCollection, "up", "UPDATE posts", "upvote_count"}, + {"deprecated community-repo post, downvote", posts.LegacyPostCollection, "down", "UPDATE posts", "downvote_count"}, + {"comment, upvote", commentCollection, "up", "UPDATE comments", "upvote_count"}, + {"comment, downvote", commentCollection, "down", "UPDATE comments", "downvote_count"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + query := voteCountUpdateQuery(tc.collection, tc.direction) + if !strings.Contains(query, tc.wantTable) || !strings.Contains(query, tc.wantColumn) { + t.Errorf("voteCountUpdateQuery(%q, %q) = %q; want a %q touching %s", + tc.collection, tc.direction, query, tc.wantTable, tc.wantColumn) + } + }) + } +} + +// A subject the tool does not understand must produce no statement at all: an +// UPDATE aimed at a guessed table would corrupt counts rather than leave them +// unrepaired. +func TestVoteCountUpdateQuery_UnknownCollectionUpdatesNothing(t *testing.T) { + t.Parallel() + + for _, collection := range []string{"", "app.bsky.feed.post", "social.coves.community.acceptance"} { + if query := voteCountUpdateQuery(collection, "up"); query != "" { + t.Errorf("voteCountUpdateQuery(%q, \"up\") = %q; want no statement", collection, query) + } + } +} diff --git a/cmd/rematerialize-posts/main.go b/cmd/rematerialize-posts/main.go --- a/cmd/rematerialize-posts/main.go +++ b/cmd/rematerialize-posts/main.go @@ -47,10 +47,10 @@ _ "github.com/lib/pq" ) // legacyPostCollection is the deprecated community-repo post collection the tool -// drains. It is the lexicon NSID, spelled here rather than imported because the -// posts package keeps its copy private; the two must agree, and there is exactly -// one correct string. -const legacyPostCollection = "social.coves.community.post" +// drains — the domain's own constant, not a second spelling of the NSID: the +// tool enumerates exactly the records the consumers still index, and a private +// copy here would be free to disagree with them. +const legacyPostCollection = posts.LegacyPostCollection // listPageSize bounds each communities/records page so an instance with a large // catalogue is enumerated in bounded queries rather than one unbounded read. diff --git a/internal/atproto/jetstream/comment_consumer.go b/internal/atproto/jetstream/comment_consumer.go --- a/internal/atproto/jetstream/comment_consumer.go +++ b/internal/atproto/jetstream/comment_consumer.go @@ -3,6 +3,7 @@ import ( "Coves/internal/atproto/utils" "Coves/internal/core/comments" + "Coves/internal/core/posts" "Coves/internal/core/richtext" "context" "database/sql" @@ -783,10 +784,17 @@ } return nil } + // The parent's collection is the only thing that says which table to count + // into. BOTH post collections route to `posts`: a top-level comment on an + // author-owned post (social.coves.community.postv2) and one on a pre-flip + // post are the same thread shape over the same table, and until §11's drain + // retires the deprecated NSID production holds rows of both kinds. Knowing + // only one of them fails silently — the comment indexes, comment_count never + // moves, and the thread renders under a post that reports zero replies. collection := utils.ExtractCollectionFromURI(comment.ParentURI) - switch collection { - case "social.coves.community.post": + switch { + case posts.IsPostCollection(collection): // Top-level comment on post - increment posts.comment_count // NOTE: No deleted_at filter - we increment even for deleted parents to match reconciliation behavior updateQuery := ` @@ -806,7 +814,7 @@ if rowsAffected == 0 { log.Printf("Warning: Post not found: %s (comment indexed anyway)", comment.ParentURI) } - case "social.coves.community.comment": + case collection == CommentCollection: // Nested reply to comment - update BOTH: // 1. Parent comment's reply_count (for thread structure) // 2. Root post's comment_count (for total thread count display) diff --git a/internal/atproto/jetstream/post_consumer.go b/internal/atproto/jetstream/post_consumer.go --- a/internal/atproto/jetstream/post_consumer.go +++ b/internal/atproto/jetstream/post_consumer.go @@ -102,7 +102,7 @@ switch commit.Collection { // The DEPRECATED community-repo post (§3.0). Here the repo DID must EQUAL // the record's community; the three collections below invert that. - case "social.coves.community.post": + case posts.LegacyPostCollection: switch commit.Operation { case "create": return c.createPost(ctx, event.Did, commit, event.TimeUS) diff --git a/internal/atproto/jetstream/postv2_aggregation_test.go b/internal/atproto/jetstream/postv2_aggregation_test.go new file mode 100644 --- /dev/null +++ b/internal/atproto/jetstream/postv2_aggregation_test.go @@ -0,0 +1,162 @@ +//go:build integration + +package jetstream + +import ( + "context" + "database/sql" + "testing" + "time" + + "Coves/internal/core/users" + "Coves/internal/db/postgres" + "Coves/tests/testkit" + + _ "github.com/lib/pq" + "github.com/stretchr/testify/require" +) + +// The denormalized counters a post row carries — upvote_count, downvote_count, +// score, comment_count — when the post is an AUTHOR-OWNED one +// (social.coves.community.postv2, docs/PRD_AUTHOR_OWNED_POSTS.md §3.1). +// +// Votes and comments name their subject by AT-URI, and neither consumer has any +// other way to learn which table to count into: it reads the collection segment +// out of the subject's URI and routes on it. The author-owned flip changed that +// segment for every new post, so a router that knows only the deprecated NSID +// silently stops counting — the vote row and the comment row are both indexed, +// the counter is simply never touched, and nothing anywhere reports an error. +// That is the failure these tests exist to make loud. +// +// They run against real SQL because the counting IS the SQL: the increment and +// the row it lands on are one statement inside the consumer's transaction, so a +// fake repository would be asserting that the fake counts. + +const ( + aggPrefix = "did:plc:agg" + aggCommunity = aggPrefix + "community" + aggAuthor = aggPrefix + "author" + aggVoter = aggPrefix + "voter" + aggCommenter = aggPrefix + "commenter" +) + +// indexAuthorOwnedPost drives a real postv2 create through the post consumer and +// returns the indexed post's URI and CID. +// +// The post is written by the consumer rather than by an INSERT so that the row +// under test is exactly the row production produces — in particular its URI, +// which is the only input the vote and comment consumers route on. +func indexAuthorOwnedPost(t *testing.T, db *sql.DB) (postURI, postCID string) { + t.Helper() + + insertBridgedUser(t, db, aggAuthor, "aggauthor.test") + insertBridgedUser(t, db, aggVoter, "aggvoter.test") + insertBridgedUser(t, db, aggCommenter, "aggcommenter.test") + insertBridgedCommunity(t, db, aggCommunity, "aggcommunity.test", aggAuthor) + + userService := newMockUserService() + userService.users[aggAuthor] = &users.User{DID: aggAuthor, Handle: "aggauthor.test"} + + consumer := NewPostEventConsumer( + postgres.NewPostRepository(db), + postgres.NewCommunityRepository(db), + userService, + db, + WithAdmissions(postgres.NewAdmissionRepository(db)), + ) + + const rkey = "aggpost" + postURI = pv2URI(aggAuthor, rkey) + postCID = "bafyreiaggpost" + + require.NoError(t, consumer.HandleEvent(context.Background(), pv2Event( + aggAuthor, "create", rkey, testkit.TID(), postCID, time.Now().UnixMicro(), + pv2Record(aggCommunity, "an author-owned post", "the words being voted on"), + ))) + return postURI, postCID +} + +// aggVoteEvent builds a vote commit in the VOTER's repo. The subject is passed +// whole so a test can point a vote at either post collection. +func aggVoteEvent(op, rkey, direction, subjectURI, subjectCID string) *JetstreamEvent { + return revCommitEvent(aggVoter, "social.coves.feed.vote", op, rkey, testkit.TID(), + "bafyreivote"+rkey, time.Now().UnixMicro(), map[string]interface{}{ + "$type": "social.coves.feed.vote", + "subject": map[string]interface{}{"uri": subjectURI, "cid": subjectCID}, + "direction": direction, + "createdAt": "2026-03-02T00:00:00Z", + }) +} + +// aggCommentEvent builds a top-level comment commit in the COMMENTER's repo: +// root and parent are both the post, which is what makes it top-level and what +// sends the increment at posts.comment_count rather than at a parent comment. +func aggCommentEvent(rkey, postURI, postCID string) *JetstreamEvent { + return revCommitEvent(aggCommenter, CommentCollection, "create", rkey, testkit.TID(), + "bafyreicomment"+rkey, time.Now().UnixMicro(), + commentRecord("a reply to an author-owned post", postURI, postCID, postURI, postCID, nil)) +} + +// readAggregates returns the counter columns of a post row. +func readAggregates(t *testing.T, db *sql.DB, uri string) (upvotes, downvotes, score, commentCount int) { + t.Helper() + require.NoErrorf(t, db.QueryRow( + `SELECT upvote_count, downvote_count, score, comment_count FROM posts WHERE uri = $1`, uri, + ).Scan(&upvotes, &downvotes, &score, &commentCount), "no post row for %s", uri) + return upvotes, downvotes, score, commentCount +} + +// A vote on an author-owned post must move that post's counters, in every +// direction and on every operation the consumer handles. +// +// The whole arc is one test because the counters are cumulative state: proving +// the increment in isolation would leave the decrement free to subtract from a +// number it never added to, which is the shape of the defect this covers. +func TestVoteConsumer_CountsVotesOnAuthorOwnedPosts(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + ctx := context.Background() + + postURI, postCID := indexAuthorOwnedPost(t, db) + votes := newVoteConsumer(db) + + require.NoError(t, votes.HandleEvent(ctx, aggVoteEvent("create", "up1", "up", postURI, postCID))) + upvotes, downvotes, score, _ := readAggregates(t, db, postURI) + require.Equalf(t, 1, upvotes, + "an upvote on %s must increment upvote_count: the vote consumer routes on the collection in the subject URI, and an author-owned post's URI names postv2", postURI) + require.Equal(t, 0, downvotes) + require.Equal(t, 1, score, "score is stored, not derived, so a missed increment is invisible to every feed that sorts by it") + + // The change path: a second vote on the same subject from the same voter, + // under a different rkey. The consumer soft-deletes the standing vote and + // must UNDO its contribution before applying the new one — a decrement that + // routes nowhere leaves the old direction counted forever. + require.NoError(t, votes.HandleEvent(ctx, aggVoteEvent("create", "down1", "down", postURI, postCID))) + upvotes, downvotes, score, _ = readAggregates(t, db, postURI) + require.Equal(t, 0, upvotes, "switching to a downvote must retract the upvote it replaces") + require.Equal(t, 1, downvotes) + require.Equal(t, -1, score) + + // The delete path. + require.NoError(t, votes.HandleEvent(ctx, aggVoteEvent("delete", "down1", "down", postURI, postCID))) + upvotes, downvotes, score, _ = readAggregates(t, db, postURI) + require.Equal(t, 0, upvotes) + require.Equal(t, 0, downvotes, "deleting the vote must decrement the count it added") + require.Equal(t, 0, score) +} + +// A top-level comment on an author-owned post must increment that post's +// comment_count. +func TestCommentConsumer_CountsCommentsOnAuthorOwnedPosts(t *testing.T) { + t.Parallel() + db := testkit.DB(t) + ctx := context.Background() + + postURI, postCID := indexAuthorOwnedPost(t, db) + + require.NoError(t, newCommentConsumer(db).HandleEvent(ctx, aggCommentEvent("aggc1", postURI, postCID))) + + _, _, _, commentCount := readAggregates(t, db, postURI) + require.Equalf(t, 1, commentCount, + "a top-level comment on %s must increment posts.comment_count: the comment consumer routes on the collection in the parent URI, and an author-owned post's URI names postv2", postURI) +} diff --git a/internal/atproto/jetstream/vote_consumer.go b/internal/atproto/jetstream/vote_consumer.go --- a/internal/atproto/jetstream/vote_consumer.go +++ b/internal/atproto/jetstream/vote_consumer.go @@ -2,6 +2,7 @@ package jetstream import ( "Coves/internal/atproto/utils" + "Coves/internal/core/posts" "Coves/internal/core/users" "Coves/internal/core/votes" "context" @@ -12,6 +13,16 @@ "log" "strings" "time" ) + +// A vote names its subject by AT-URI and nothing else, so the collection segment +// of that URI is the ONLY thing that says which table the count belongs in. Both +// post collections route to `posts` — posts.IsPostCollection is the shared +// predicate — because the author-owned flip put new posts in a new collection +// (social.coves.community.postv2) while every pre-flip post kept the deprecated +// one, and both are indexed into the same table until §11's drain retires the +// legacy NSID. A router that knows only one of them stops counting the other +// SILENTLY: the vote row is still indexed and no error is raised, the score just +// never moves. // VoteEventConsumer consumes vote-related events from Jetstream // Handles CREATE and DELETE operations for social.coves.feed.vote @@ -214,8 +225,8 @@ // Parse collection from subject URI to determine target table collection := utils.ExtractCollectionFromURI(subjectURI) var updateQuery string - switch collection { - case "social.coves.community.post": + switch { + case posts.IsPostCollection(collection): // Vote on post - update posts table if direction == "up" { updateQuery = ` @@ -233,7 +244,7 @@ WHERE uri = $1 AND deleted_at IS NULL ` } - case "social.coves.community.comment": + case collection == CommentCollection: // Vote on comment - update comments table if direction == "up" { updateQuery = ` @@ -349,15 +360,15 @@ // Decrement the old vote's count (will be re-incremented below if same direction) collection := utils.ExtractCollectionFromURI(vote.SubjectURI) var decrementQuery string if existingDirection.String == "up" { - if collection == "social.coves.community.post" { + if posts.IsPostCollection(collection) { decrementQuery = `UPDATE posts SET upvote_count = GREATEST(0, upvote_count - 1), score = GREATEST(0, upvote_count - 1) - downvote_count + bridged_upvote_count - bridged_downvote_count WHERE uri = $1 AND deleted_at IS NULL` - } else if collection == "social.coves.community.comment" { + } else if collection == CommentCollection { decrementQuery = `UPDATE comments SET upvote_count = GREATEST(0, upvote_count - 1), score = GREATEST(0, upvote_count - 1) - downvote_count + bridged_upvote_count - bridged_downvote_count WHERE uri = $1 AND deleted_at IS NULL` } } else { - if collection == "social.coves.community.post" { + if posts.IsPostCollection(collection) { decrementQuery = `UPDATE posts SET downvote_count = GREATEST(0, downvote_count - 1), score = upvote_count - GREATEST(0, downvote_count - 1) + bridged_upvote_count - bridged_downvote_count WHERE uri = $1 AND deleted_at IS NULL` - } else if collection == "social.coves.community.comment" { + } else if collection == CommentCollection { decrementQuery = `UPDATE comments SET downvote_count = GREATEST(0, downvote_count - 1), score = upvote_count - GREATEST(0, downvote_count - 1) + bridged_upvote_count - bridged_downvote_count WHERE uri = $1 AND deleted_at IS NULL` } } @@ -423,8 +434,8 @@ // Parse collection from subject URI to determine target table collection := utils.ExtractCollectionFromURI(vote.SubjectURI) var updateQuery string - switch collection { - case "social.coves.community.post": + switch { + case posts.IsPostCollection(collection): // Vote on post - update posts table if vote.Direction == "up" { updateQuery = ` @@ -442,7 +453,7 @@ WHERE uri = $1 AND deleted_at IS NULL ` } - case "social.coves.community.comment": + case collection == CommentCollection: // Vote on comment - update comments table if vote.Direction == "up" { updateQuery = ` diff --git a/internal/atproto/lexicon/social/coves/community/post/update.json b/internal/atproto/lexicon/social/coves/community/post/update.json --- a/internal/atproto/lexicon/social/coves/community/post/update.json +++ b/internal/atproto/lexicon/social/coves/community/post/update.json @@ -114,12 +114,20 @@ "name": "NotAuthorized", "description": "User is not authorized to edit this post" }, { + "name": "ConcurrentModification", + "description": "The post changed between being read and being written, so the edit was composed against content that no longer stands. Re-read the post and re-apply the edit; the server does not merge, because re-applying server-side would silently erase a change the editor never saw." + }, + { + "name": "NoAuthorCredentials", + "description": "The service holds no credentials it can write to the author's repository with. Distinct from an expired caller session: signing in again is the fix for a person and no fix at all for a non-interactive author whose stored grant was revoked." + }, + { "name": "EditWindowExpired", - "description": "Edit window has expired (posts can only be edited within 24 hours)" + "description": "DEPRECATED, retained for backward compatibility: no edit window is enforced - a post is editable by its author for as long as it exists. This name ships on main; removing a declared error is a non-additive break, so it stays declared until a new-NSID change retires it." }, { "name": "InvalidUpdate", - "description": "Invalid update operation (e.g., changing post type)" + "description": "DEPRECATED, retained for backward compatibility: never emitted. Refusals this once covered are answered by name instead - an edit naming a different community is an InvalidRequest validation error, and a post whose collection is not editable is the same. This name ships on main; removing a declared error is a non-additive break, so it stays declared until a new-NSID change retires it." } ] } diff --git a/internal/core/comments/comment_post_record_test.go b/internal/core/comments/comment_post_record_test.go new file mode 100644 --- /dev/null +++ b/internal/core/comments/comment_post_record_test.go @@ -0,0 +1,71 @@ +package comments + +import ( + "testing" + "time" + + "Coves/internal/core/posts" + + "github.com/stretchr/testify/assert" +) + +// The post record a comment thread is served with. +// +// getComments hydrates the post it is a thread on and hands the client back +// `postView.record` — the post record verbatim. Its `$type` is what tells a +// consumer which lexicon the rest of the object obeys, and for the two post +// collections those lexicons disagree about the one field that matters: +// the deprecated community-repo record carries an `author`, and the author-repo +// successor deliberately has none, because under §3.1 authorship IS the repo the +// record lives in. Stamping the deprecated NSID on an author-owned post +// therefore does the exact thing PRD §3.0 says a new NSID exists to prevent — it +// tells the reader to derive community = repo DID for a record whose repo is the +// AUTHOR — and hands back a fabricated author field alongside it. +func TestBuildPostRecord_TypeFollowsThePostsCollection(t *testing.T) { + t.Parallel() + + const ( + authorDID = "did:plc:recordauthor" + communityDID = "did:plc:recordcommunity" + ) + createdAt := time.Date(2026, 3, 1, 0, 0, 0, 0, time.UTC) + + post := func(uri string) *posts.Post { + return &posts.Post{ + URI: uri, + AuthorDID: authorDID, + CommunityDID: communityDID, + CreatedAt: createdAt, + } + } + service := &commentService{} + + t.Run("an author-owned post is labelled postv2 and carries no author", func(t *testing.T) { + t.Parallel() + record := service.buildPostRecord(post("at://" + authorDID + "/" + posts.PostV2Collection + "/abc123")) + + assert.Equal(t, posts.PostV2Collection, record["$type"], + "a postv2 post labelled with the deprecated NSID tells the client to read the authority of its URI as a community, when it is the author") + assert.NotContains(t, record, "author", + "the postv2 lexicon has no author field: synthesising one hands the reader back exactly the forgeable claim the flip removed") + assert.Equal(t, communityDID, record["community"], + "postv2 keeps community — it is the author's submission target") + }) + + t.Run("a legacy post keeps its own NSID and its author field", func(t *testing.T) { + t.Parallel() + record := service.buildPostRecord(post("at://" + communityDID + "/" + posts.LegacyPostCollection + "/abc123")) + + assert.Equal(t, posts.LegacyPostCollection, record["$type"]) + assert.Equal(t, authorDID, record["author"], + "the deprecated record does carry an author, and it is part of what a client may verify against the community's repo") + }) + + t.Run("a URI naming neither collection falls back to the pre-flip shape", func(t *testing.T) { + t.Parallel() + record := service.buildPostRecord(post("not-an-at-uri")) + + assert.Equal(t, posts.LegacyPostCollection, record["$type"]) + assert.Equal(t, authorDID, record["author"]) + }) +} diff --git a/internal/core/comments/comment_service.go b/internal/core/comments/comment_service.go --- a/internal/core/comments/comment_service.go +++ b/internal/core/comments/comment_service.go @@ -1205,21 +1205,55 @@ } // buildPostRecord constructs a minimal PostRecord from a Post entity // Satisfies the lexicon requirement that postView.record is a required field +// +// THE SHAPE FOLLOWS THE URI, exactly as the repository's own hydration does +// (postgres.scanPostView): the two post collections are different records rather +// than two spellings of one. An author-owned post lives in the AUTHOR's repo +// under social.coves.community.postv2 and has NO author field — its removal is +// what makes authorship unforgeable (PRD §3.1) — so stamping the deprecated NSID +// here would tell a client to read the authority of that URI as a community, and +// hand it a self-asserted author this AppView invented, which is precisely the +// mis-indexing §3.0 gave the successor a new NSID to prevent. +// +// It returns a map rather than a typed posts.PostRecord for the same reason: +// that struct carries a non-optional author field, so a postv2 record built from +// it would serialize one whatever this code assigned. A map lets the field be +// ABSENT, which is the only honest rendering — and it is the shape every other +// producer of postView.record already emits (postgres.scanPostView, the feed +// queries), so the two paths can no longer disagree about what a post record +// looks like. +// // TODO (Phase 2C): Unmarshal JSON fields (embed, facets, labels) for complete record -func (s *commentService) buildPostRecord(post *posts.Post) *posts.PostRecord { - record := &posts.PostRecord{ - Type: "social.coves.community.post", - Community: post.CommunityDID, - Author: post.AuthorDID, - CreatedAt: post.CreatedAt.Format(time.RFC3339), - Title: post.Title, - Content: post.Content, +func (s *commentService) buildPostRecord(post *posts.Post) map[string]interface{} { + collection := posts.CollectionOfPostURI(post.URI) + if !posts.IsPostCollection(collection) { + // A URI naming neither collection is not something this AppView indexed + // as a post, so there is no better answer than the shape every pre-flip + // row has. + collection = posts.LegacyPostCollection + } + + record := map[string]interface{}{ + "$type": collection, + "community": post.CommunityDID, + "createdAt": post.CreatedAt.Format(time.RFC3339), + } + if collection == posts.LegacyPostCollection { + // The deprecated community-repo record DOES carry an author, and it is + // part of what a client may verify against the community's repo. + record["author"] = post.AuthorDID + } + if post.Title != nil { + record["title"] = *post.Title + } + if post.Content != nil { + record["content"] = *post.Content } // TODO (Phase 2C): Parse JSON fields from database for complete record: - // - Unmarshal post.Embed (*string) → record.Embed (map[string]interface{}) - // - Unmarshal post.ContentFacets (*string) → record.Facets ([]interface{}) - // - Unmarshal post.ContentLabels (*string) → record.Labels (*SelfLabels) + // - Unmarshal post.Embed (*string) → record["embed"] + // - Unmarshal post.ContentFacets (*string) → record["facets"] + // - Unmarshal post.ContentLabels (*string) → record["labels"] // These fields are stored as JSONB in the database and need proper deserialization return record diff --git a/internal/core/posts/admit_matrix_test.go b/internal/core/posts/admit_matrix_test.go --- a/internal/core/posts/admit_matrix_test.go +++ b/internal/core/posts/admit_matrix_test.go @@ -1175,6 +1175,42 @@ assert.NotEmpty(t, submissionFingerprint(base(), nil), "an empty fingerprint would make every submission collide with every other") }) + t.Run("the hash of a submission carrying no langs and no tags is unchanged", func(t *testing.T) { + t.Parallel() + + // A GOLDEN VALUE, and the only kind of assertion that can catch what it + // is for. The fingerprint is a live dedupe key in post_submissions, so a + // change to PostRecord's JSON encoding silently repartitions the ledger: + // an author mid-retry when the binary rolls would miss their own + // reservation and be admitted as a second post. + // + // langs and tags were APPENDED to PostRecord with omitempty precisely so + // that every submission carrying neither — which is every row on the + // ledger, because create discarded both fields until they were added — + // hashes to exactly what it hashed before. This value was computed + // against the struct as it stood BEFORE they were added. If it fails, + // the encoding moved and the ledger is about to be repartitioned; that + // is a deliberate, migration-shaped decision, not something to re-pin. + // + // The literals are spelled out rather than taken from the constants + // above, because a golden value has to be independent of anything a + // future edit might renumber. + title, content := "A title", "Some body text" + pinned := PostRecord{ + Type: "social.coves.community.postv2", + Community: "did:plc:aaaaaaaaaaaaaaaaaaaaaaaa", + Author: "did:plc:bbbbbbbbbbbbbbbbbbbbbbbb", + Title: &title, + Content: &content, + CreatedAt: "2026-08-01T12:00:00Z", + } + assert.Equal(t, + "8f98863589cb9853b3ea4918febd44fde9aaa019da766ae42d433b695bf83403", + submissionFingerprint(pinned, nil), + "the fingerprint of a tag-free, lang-free submission moved: every live post_submissions row "+ + "has just been repartitioned, and retries spanning the deploy will be admitted as new posts") + }) + t.Run("a different thumbnail is a different submission", func(t *testing.T) { t.Parallel() @@ -1196,6 +1232,8 @@ {"author", func(r *PostRecord) { r.Author = "did:plc:eeeeeeeeeeeeeeeeeeeeeeee" }}, {"embed", func(r *PostRecord) { r.Embed = map[string]interface{}{"$type": "social.coves.embed.external"} }}, + {"langs", func(r *PostRecord) { r.Langs = []string{"fr"} }}, + {"tags", func(r *PostRecord) { r.Tags = []string{"gardening"} }}, } { t.Run("a different "+tc.field+" is a different submission", func(t *testing.T) { t.Parallel() @@ -1221,7 +1259,7 @@ h := newAdmitHarness() title, content := "The same post", "the same body" record := PostRecord{ - Type: postCollection, + Type: LegacyPostCollection, Author: admitAuthorDID, Title: &title, Content: &content, @@ -1251,7 +1289,7 @@ h := newAdmitHarness() title := "The same link, a different thumbnail" record := PostRecord{ - Type: postCollection, + Type: LegacyPostCollection, Community: admitCommunityHandle, Author: admitAuthorDID, Title: &title, diff --git a/internal/core/posts/collection_routing_test.go b/internal/core/posts/collection_routing_test.go new file mode 100644 --- /dev/null +++ b/internal/core/posts/collection_routing_test.go @@ -0,0 +1,34 @@ +package posts + +import "testing" + +// IsPostCollection is the one question every aggregation site asks of a subject +// URI, so its table is the specification of what "a post" means during the +// dual-collection window. +func TestIsPostCollection(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + collection string + want bool + }{ + {"the author-repo record every new post is written to", PostV2Collection, true}, + {"the deprecated community-repo record production still holds", LegacyPostCollection, true}, + {"comments count into their own table", "social.coves.community.comment", false}, + {"a community's acceptance is a decision about a post, not a post", AcceptanceCollection, false}, + {"a removal likewise", RemovalCollection, false}, + {"an unparseable URI yields no collection at all", "", false}, + {"a near-miss NSID must not be admitted by a prefix match", "social.coves.community.postv3", false}, + {"nor must a longer name that merely starts with the legacy one", "social.coves.community.post.get", false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := IsPostCollection(tc.collection); got != tc.want { + t.Errorf("IsPostCollection(%q) = %v, want %v", tc.collection, got, tc.want) + } + }) + } +} diff --git a/internal/core/posts/community_writer.go b/internal/core/posts/community_writer.go --- a/internal/core/posts/community_writer.go +++ b/internal/core/posts/community_writer.go @@ -32,6 +32,18 @@ // its own so that the reader and the writer cannot come to disagree about // what a post record is called. PostV2Collection = "social.coves.community.postv2" + // LegacyPostCollection is the DEPRECATED community-repo post record (§3.0): + // the collection every post written before the author-owned flip still + // lives in. + // + // It is exported for the same reason PostV2Collection is. The two + // collections are one table (see IsPostCollection), so the consumers that + // count votes and comments onto a post row, the repository that renders + // one, and the reconciliation tools all have to name the deprecated NSID — + // and a literal repeated across those layers is a literal one of them + // eventually fails to update when §11's drain retires it. + LegacyPostCollection = "social.coves.community.post" + // AcceptanceCollection is the community-repo collection holding a // community's attestation that it accepts a post. AcceptanceCollection = "social.coves.community.acceptance" diff --git a/internal/core/posts/embed_validation.go b/internal/core/posts/embed_validation.go --- a/internal/core/posts/embed_validation.go +++ b/internal/core/posts/embed_validation.go @@ -137,18 +137,25 @@ // normalizeEmbedURIs rewrites, in place, every field of an external embed that // the lexicon declares as `format: uri` — external.uri and each // external.sources[].uri — into a form that satisfies that format. // -// The AppView signs community post records into the community's PDS itself, and -// the PDS does not validate custom social.coves.* lexicons. That makes this the -// only point in the pipeline that can guarantee a schema-conforming record no -// matter which client produced it, which matters because these URIs federate: +// The AppView signs post records into the AUTHOR's PDS (§4.2) with +// `validate: false`, because the PDS does not know custom social.coves.* +// lexicons. That makes this the only point in the pipeline that can guarantee a +// schema-conforming record no matter which client produced it — for the records +// this AppView signs, at least; an author writing to their own repo directly +// passes nowhere near here. It matters because these URIs federate: // any third-party tool that resolves our lexicons and validates the firehose // judges the bytes we wrote. An unencoded character in a URL is a client bug, // not user intent, so it is repaired rather than rejected — see // validation.NormalizeURI. Input that carries no recoverable URI at all still // fails loudly instead of being persisted as a broken link. // -// Must run after validateEmbed, which establishes the structure this walks. -// Non-external embeds carry no `format: uri` fields and are left untouched. +// Must run after validateEmbed, which establishes the structure this walks — +// which is why both are called from the one shared gate +// (normalizeAndValidatePostContent) rather than from each write path: an edit +// path that had its own idea of the order, or skipped this half entirely, is +// precisely the drift that let an author post a clean link and then edit it into +// a javascript: URI. Non-external embeds carry no `format: uri` fields and are +// left untouched. func normalizeEmbedURIs(embed map[string]interface{}) error { if embed == nil { return nil diff --git a/internal/core/posts/post.go b/internal/core/posts/post.go --- a/internal/core/posts/post.go +++ b/internal/core/posts/post.go @@ -62,6 +62,15 @@ Labels *SelfLabels `json:"labels,omitempty"` Community string `json:"community"` AuthorDID string `json:"authorDid"` Facets []interface{} `json:"facets,omitempty"` + + // Langs and Tags are declared by the post.create lexicon and by the postv2 + // record, and were the two fields this struct did not carry — so a client + // that sent them had them silently dropped on submission and could then set + // them one second later with an edit, which DID honour them. Carrying them + // here closes that asymmetry; their caps are enforced in the shared content + // gate, so create and update bound them identically. + Langs []string `json:"langs,omitempty"` + Tags []string `json:"tags,omitempty"` } // CreatePostResponse represents the response from creating a post @@ -277,6 +286,18 @@ Community string `json:"community"` Author string `json:"author"` CreatedAt string `json:"createdAt"` Facets []interface{} `json:"facets,omitempty"` + + // Langs and Tags are APPENDED, and the position matters as much as the + // fields do. This struct is what submissionFingerprint hashes, so its JSON + // encoding is a live dedupe key: appending two `omitempty` fields leaves the + // bytes of every submission that carries neither — which is every submission + // on the ledger today, because create discarded both — byte-identical, and + // therefore leaves every standing reservation valid. Inserting them earlier, + // or without omitempty, would repartition the whole ledger. See + // submissionFingerprint, whose comment states the rule this obeys: a field on + // the record and not here is a field the fingerprint cannot see. + Langs []string `json:"langs,omitempty"` + Tags []string `json:"tags,omitempty"` } // PostView represents the full view of a post with all metadata diff --git a/internal/core/posts/post_content_validation_test.go b/internal/core/posts/post_content_validation_test.go new file mode 100644 --- /dev/null +++ b/internal/core/posts/post_content_validation_test.go @@ -0,0 +1,270 @@ +package posts + +import ( + "fmt" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The shared content gate, exercised through the CREATE entry point. +// +// Its edit-path twin is service_update_validation_test.go, which proves the +// same inputs are refused on the way through UpdatePost. These two files +// together are the parity claim: what is refused here is refused there, because +// it is the same function. +// +// # WHY THE URI RULES BELONG IN THE SHARED GATE +// +// They used to live in CreatePost itself, one step after validation, so an edit +// never ran them. validateEmbed only asks that external.uri be a NON-EMPTY +// STRING — it never parses it, and it does not look at external.sources at all — +// and richtext's structural check deliberately has no #link arm. So without the +// normalization step nothing anywhere refuses a javascript: URI, a schemeless +// one, or a sources array past the lexicon's cap. +// +// This is defence in depth and schema conformance, not the system's only XSS +// defence: an author can write any record they like straight into their own PDS +// repo, and the firehose ingest path does not scheme-check what it indexes. What +// it does guarantee is that a record THIS AppView signs conforms to the lexicon +// it claims, on both the paths that produce one. + +func embedExternal(uri string, extra map[string]interface{}) map[string]interface{} { + external := map[string]interface{}{"uri": uri} + for k, v := range extra { + external[k] = v + } + return map[string]interface{}{ + "$type": embedTypeExternal, + "external": external, + } +} + +func linkFacet(uri string) []interface{} { + return []interface{}{map[string]interface{}{ + "index": map[string]interface{}{"byteStart": 0, "byteEnd": 5}, + "features": []interface{}{map[string]interface{}{ + "$type": "social.coves.richtext.facet#link", + "uri": uri, + }}, + }} +} + +func sourcesOfLength(n int) []interface{} { + sources := make([]interface{}, 0, n) + for i := 0; i < n; i++ { + sources = append(sources, map[string]interface{}{ + "uri": fmt.Sprintf("https://example.com/source-%d", i), + }) + } + return sources +} + +// createRequestWith is a minimally valid create request with content long +// enough for the facets these cases attach to it. +func createRequestWith(mutate func(*CreatePostRequest)) CreatePostRequest { + content := "hello world" + req := CreatePostRequest{ + Community: "did:plc:community1234567890", + AuthorDID: "did:plc:author1234567890abc", + Content: &content, + } + mutate(&req) + return req +} + +func TestValidateCreateRequest_RefusesURIsTheRecordMayNotCarry(t *testing.T) { + t.Parallel() + + service := &postService{} + + for _, tc := range []struct { + mutate func(*CreatePostRequest) + name string + field string + }{ + { + name: "a javascript: external embed URI", + field: "embed.external.uri", + mutate: func(r *CreatePostRequest) { r.Embed = embedExternal("javascript:alert(1)", nil) }, + }, + { + name: "a data: external embed URI", + field: "embed.external.uri", + mutate: func(r *CreatePostRequest) { r.Embed = embedExternal("data:text/html,