Skip to content

test(threading): the agreement set has two members, and only one was pinned - #1226

Open
lilyshen0722 wants to merge 1 commit into
mainfrom
fix/reply-edge-pings-the-parent-not-the-root
Open

test(threading): the agreement set has two members, and only one was pinned#1226
lilyshen0722 wants to merge 1 commit into
mainfrom
fix/reply-edge-pings-the-parent-not-the-root

Conversation

@lilyshen0722

Copy link
Copy Markdown
Contributor

From @sprint-review's read of the handleSend invariant note (56879).

resolveThreadRoot accepts replyToMessageId + threadRootId together whenever explicit === COALESCE(parent.thread_root_id, parent.id) — whenever the parent is anywhere in the thread being aimed at. That is a set with two members, and they differ in who gets pinged, because resolveImplicitReplyTarget resolves the author of replyToMessageId (the parent), not the root:

shape accepted pinged
parent mid-thread — replyTo 101, root 100 yes 101's author
parent is the root — replyTo 100, root 100 yes the root's author

threadRootResolver.test.js held only the first (explicit wins over derivation when both agree, seeded with 101 = a reply inside thread 100).

The second is the one worth having. It is the shape V2PodChat's handleSend comment is written about — the only one where the reply edge pings the root's author — and the resolver structurally cannot refuse it: a root's thread_root_id is NULL, so derived falls through to parent.id, which is the named root. The two statements are equal by construction and thread_root_mismatch can never fire. aimAtThread/aimAtMessage clearing each other is the only thing preventing it, and nothing made that dependency visible.

Adds the accept case and a CONTROL one message over (replyTo 100, root 200) that still 400s, so the acceptance is demonstrably the COALESCE fall-through rather than a resolver waving through any same-pod pair.

Mutation-checked, per rule 1: gating the accept path with || parentId === explicit reddens exactly the new test, 1 failed / 19 passed, no collateral. Restored after.

Also corrects the handleSend comment's stated reason — a reply edge pings the author of whatever it points at, which collapses onto "the root's author" only in the second shape. Rule 15: the rule stands, the reason was narrow.

20 tests green (was 18). No production code changed.

…pinned

@sprint-review: resolveThreadRoot accepts replyToMessageId + threadRootId
whenever explicit === COALESCE(parent.thread_root_id, parent.id) — the
parent being anywhere in the aimed-at thread. Two members, not one:

  parent mid-thread (101 in thread 100)  -> pinged: 101's author
  parent IS the root (replyTo 100, 100)  -> pinged: the root's author

The suite held only the first. The second is the shape V2PodChat's
handleSend comment is actually written about, because it is the only one
where the reply edge pings the ROOT's author, and it is structurally
unrefusable: a root's thread_root_id is NULL, so derived falls through to
parent.id and equals the named root by construction. thread_root_mismatch
can never fire there. The client rule is the only thing stopping it, and
nothing made that dependency visible.

Adds the accept case plus a CONTROL one message over that still 400s, so
the acceptance is the COALESCE fall-through and not a permissive resolver.
Mutation-checked: gating the accept path reddens exactly the new test.

Corrects the handleSend comment's stated reason — a reply edge pings the
author of whatever it points at, which coincides with the root's author
only in the second shape. Rule 15: the rule survives, the reason changes.

20 tests green (was 18).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Verified end to end at 8cc984da. Suite runs 20 passed. Your rule-1 mutation reproduces exactly as described: gating the mismatch throw with || parentId === explicit gives 1 failed / 19 passed, and the single red is replying to the root while aimed at its thread — no collateral. The derivation is right too: derived = COALESCE(parent.thread_root_id, parent.id), and since thread_root_not_a_root has already established explicit is a root, derived === explicit holds by construction whenever parentId === explicit. thread_root_mismatch is dead on that pair. And resolveImplicitReplyTarget does resolve the parent's author (message.replyTo?.userId, falling back to PGMessage.findById(replyToMessageId)), never the root — so the two-member split is real.

One scope qualifier on the new comment, and it is the same class as the reason you just corrected in it. The table reads:

parent mid-thread (reply 101 in thread 100) -> pings 101's author
parent IS the root (replyTo 100, root 100)  -> pings the root's author

Both hold only when that author is a bot with an active installation. resolveImplicitReplyTarget returns null on !agentName || !instanceId (a non-bot author) and again on !activeInstallation. A human parent author produces no implicit ping on either row. The client rule still stands and the two-member distinction is unaffected — but the harm the comment names to justify the rule is narrower than the table reads, and the unqualified form is what a future reader will quote.

Cross-PR check, since #1168 edits the same test file: git merge-tree on the pair is clean, and the merged tree is coherent rather than merely conflict-free — it carries applyTable(mockPool, 'pods') from #1168, both new tests from this PR, and zero remaining createTableFor references. They can land in either order.

Not blocking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant