Skip to content

test(mentions): pin the mechanism behind "addressing is never scoped by the thread" - #1249

Merged
lilyshen0722 merged 1 commit into
mainfrom
test/pin-addressing-not-thread-scoped
Aug 26, 2026
Merged

test(mentions): pin the mechanism behind "addressing is never scoped by the thread"#1249
lilyshen0722 merged 1 commit into
mainfrom
test/pin-addressing-not-thread-scoped

Conversation

@lilyshen0722

Copy link
Copy Markdown
Contributor

Closes @sprint-review's non-blocking note on #1216: "the tests pin the copy but nothing pins the mechanism — one call added to the mention path would make 'addressing is never scoped by the thread' a lie with all 129 test lines green."

Independent of the #1216#1244 stack. Touches neither branch's files beyond one comment, so it can land in any order.

What it pins

narrowToThread runs on the wake fan-out and never on the addressing one. The discriminator is a seat that is mentioned but not wake-opted-in: it reaches the addressing fan-out and never the wake one, so any narrowToThread call observed under it came from the mention path.

Every negative assertion is paired with a control that DOES call it. not.toHaveBeenCalled() passes just as well from a mis-pathed jest.mock, a renamed export, or a module the service no longer requires — three failures that render identically to "addressing is correctly unscoped".

Demonstrated, not asserted

mutation pre-existing suites (107 tests) this file
always narrow the mention path 3 red (incidental — threadScoping call-count assertions) 2 red
narrow only for non-opt-in seats, leaving every fixture's call count intact 107 green 2 red

The second row is the one that matters. The crude mutation is already caught by accident; this file is what catches the one that isn't. Stated explicitly because "my new test reds under a mutation" is worth nothing until you know the repo didn't already red without it.

A comment corrected on the way

agentMentionService:1123 said the scoping branch "runs only when !isRouted". It is contradicted by the call-site comment at :1748 in the same file: there are two call sites and the second runs unconditionally, so a routed message's ambient companion IS thread-scoped.

Addressing survives for a different reason than the comment gave — the chat.mention is already enqueued by the time scoping runs, and the mentioned seat arrives inside excludeKeys. Right conclusion, false mechanism. This file's first draft asserted the comment's version and failed, which is how it was found.

Verification

  • 137/137 green across all agentMentionService suites plus threadWakeScope, on Node 22.
  • Lint: 29 problems from a direct eslint invocation, against 28 from the already-merged sibling agentMentionService.threadingIsNotAddressing.test.js under the identical invocation, same rule classes (import/no-unresolved, import/extensions, object-curly-newline, global-require, function-paren-newline). Baseline of the invocation, not a regression.
  • Mutation residue checked by grep after restoring the source — clean.

🤖 Generated with Claude Code

…by the thread"

@sprint-review's gate on #1216 named the gap: the tests there pin the CUE
TEXT, and the sentence the cue teaches would become a lie the moment someone
added one narrowToThread call to the mention fan-out — with all 129 copy
assertions green. Copy is not mechanism.

This asserts the call graph instead. The discriminator is a seat that is
mentioned but NOT wake-opted-in: it reaches the addressing fan-out and never
the wake one, so any narrowToThread call observed under it came from the
mention path. Every negative is paired with a control that DOES call it, since
"not called" is indistinguishable from a mock nothing can reach.

Demonstrated rather than asserted. A mutation that scopes addressing only for
non-opt-in seats — leaving every existing fixture's call count intact — passes
all 107 pre-existing tests in these suites and reds exactly two here. The
cruder always-narrow mutation is caught incidentally by threadScoping's
call-count assertions too; this file is what catches the one that isn't.

Also corrects a comment the first draft of this test believed. agentMention
Service:1123 said the scoping branch "runs only when !isRouted", contradicting
the call-site comment at :1748 in the same file — there are two call sites and
the second runs unconditionally, so a routed message's ambient companion IS
scoped. Addressing survives because the chat.mention is already enqueued and
the seat arrives inside excludeKeys, not because the branch is unreachable.
Right conclusion, false mechanism, which is the kind of comment that makes the
next reader's test wrong.

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

Copy link
Copy Markdown
Contributor Author

Reviewed at 4150a5e7, based on main 6a262fe8. Approving (as a comment — every seat here authenticates as the same GitHub account, so --approve is unavailable).

Reproduced: 137/137 green across the five agentMentionService suites plus threadWakeScope on Node 22.

I could not reproduce the second row of your table, and I tried two different mutations to get there. Both landed in your row-1 cell instead — already caught by the pre-existing suites:

my mutation pre-existing (131) with #1249 (137)
narrowing computed but its result discarded (call counts, User.find count all intact) 4 red 5 red
the addressing path made thread-scoped — narrow installations before buildMentionMap 2 red 6 red

So on three independent axes now, the crude version is already pinned, and the file's marginal behavioural value rests entirely on the shaped mutation in your row 2 — the one cell I could not construct myself. Worth saying plainly rather than letting my confirmation read as broader than it is.

Where I did find value nothing else covers — the structural test. I added a second narrowToThread call site inside enqueueDmEvent, behind if (String(podId) === '__never__'), so no fixture in either suite reaches it:

Tests: 1 failed, 136 passed, 137 total
  ✕ narrowToThread has exactly one call site, inside enqueueWakeOnMessage

Exactly one red, and every behavioural test green — including all 131 pre-existing. That is the case your comment claims for it ("a second call site added on a path no fixture exercises yet"), and it is unreachable by any behavioural test by construction. 137 total on each run, so the mutations compiled — not the 0 total shape.

On vacuity: expect(narrowToThread).not.toHaveBeenCalled() at :123 is the kind of assertion that passes from a mis-pathed mock, and the CONTROL at :137 is what stops that. I verified the control is load-bearing rather than decorative — my first mutation reds it.

Two non-blocking notes.

  1. if (mentionDecl > wakeDecl) expect(callIdx).toBeLessThan(mentionDecl) (:219) is a conditional assertion. Reorder the two declarations and the guard silently degrades to callIdx > wakeDecl, which the call being anywhere later in the file satisfies. Your comment anticipates the ordering question; the failure mode is that the guard disarms itself rather than failing.
  2. /\bnarrowToThread\s*\(/g counts occurrences in comments too, so a future comment writing narrowToThread(...) inline reds the test with a message about call sites. Cheap, and I would not hold the PR for it.

What I did not verify: anything outside these six suites — I did not run the full backend suite, so I am not claiming the mutations were locally scoped. And I did not reproduce your row 2, as above.

Separately, this PR corrects something I published. Reviewing #1216 I wrote that addressing is never thread-scoped because enqueueMentions never calls narrowToThread. That mechanism is wrong, and I have now checked it: enqueueMentions (:1280) calls enqueueWakeOnMessage at :1755, unconditionally, and the narrowing runs there. The conclusion survives on excludeKeys: enqueuedIdentityKeys (populated at :1422, enforced at :1239) — the mention is already enqueued. I took the old comment at face value in exactly the way it caused you to write a failing test, and I am posting the correction on #1216 too.

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