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

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.

@lilyshen0722
lilyshen0722 merged commit 255688e into main Aug 26, 2026
11 checks passed
@lilyshen0722
lilyshen0722 deleted the test/pin-addressing-not-thread-scoped branch August 26, 2026 07:11
lilyshen0722 added a commit that referenced this pull request Sep 1, 2026
… what the fields do (#1216)

* feat(agents): the three-verb cue tells agents how to CHOOSE, not just what the fields do

Sam's ask (57672) was "teach agents when to use reply, or in thread, or
quote." #1176 shipped the mechanics — what a plain post, `replyToMessageId`
and `threadRootId` each do — and that is the other question. A description of
three fields does not answer a choice, so an agent that has read the whole
paragraph still re-derives which verb its next message wants, every time,
from field semantics.

Adds @ux-lead's decision rule (57678), close to their phrasing on purpose:

    Rule of thumb: if your message answers one person, reply; if it
    continues a topic, thread; if it starts one, post. A reply inside a
    thread is allowed and still addresses its author.

It is written as a test the agent applies to its own draft rather than as
three more facts. The trailing clause is load-bearing: without it the rule
reads as three mutually exclusive branches and an agent concludes it must
pick between quoting and threading, when the two fields are independent.

Verified rather than taken on the copy's word — ux-lead's framing says each
verb "says who is woken", and that claim is checkable. It holds:
threadWakeScopeService.narrowToThread scopes ambient thread activity to the
thread's effective followers and can only NARROW an already-computed opt-in
list, so "wakes followers only" is the real behaviour, not aspirational.

Three tests in the existing inline-cue suite, pinning the decision rule
rather than the paragraph around it — the cue ships as one opaque string, so
"the frame mentions threads" stays green on the mechanics clauses alone.
The third is a control proving the assertions can tell the two halves apart.
Probe: replacing the rule with a mechanics-only tail reddens exactly the two
behavioural tests and leaves the control green. Suite 110 passed; tsc clean
for this file.

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

* fix(agents): the overflow cue must not send substance where nobody wakes

@sprint-review (57706) found the hole in the prose-overflow sentence, and it
is the expensive kind — the cue was obeyable and wrong.

"Post your headline to the channel, continue under your own root" reads as
license to make the top-level message a pointer. It cannot be.
`effectiveFollowerIds` derives `participants` as authors only — `SELECT
DISTINCT user_id FROM messages WHERE thread_root_id = $1 OR id = $1`. At the
instant you open a thread under your own root you are its only author, so you
are its only follower, and `narrowToThread` empties the wake list for every
peer. An agent following the cue literally broadcasts a title and writes the
substance where zero agents are woken.

Two clauses close it, both naming kernel mechanisms rather than preferences:
the top-level message must stand alone (the channel post is the only delivery
the room is guaranteed), and an @mention inside the thread reaches a named
peer regardless of scope — the mention path runs before this narrowing, and
`followMentionedThreadUsers` then writes `following IS TRUE` for that target,
enrolling them for the ambient remainder.

The comment recording the pre-ship verification is corrected too. "Wakes
followers only" was true and insufficient: it confirmed the SET the wake is
narrowed to and never asked what that set contains on the path the cue tells
agents to take. Confirming a predicate is not confirming its extension.

Four guards, including a control that pins the exact unqualified sentence
that shipped before this — so a revert reddens rather than passing on the
shared "thread, not an attachment" phrase. Negative control: dropping the two
clauses reddens exactly 2 of 69, the other 67 stay green.

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

* fix(agents): name what threading does, not only what follows from it

@sprint-review (57707): "continue in-thread" reads as RELOCATING a message
when it is actually UN-ADDRESSING it. That is the intuition behind the
mistake, and the two clauses added in the previous commit do not correct it —
they state mechanisms, and a mechanism does not dislodge a wrong model.

One sentence, guarded separately so a future trim cannot read it as a
flourish on clauses that already "cover it". It is the only line in the frame
that tells an agent threading REMOVES something rather than moving it.

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

* fix(agents): the @mention escape does not survive a mute, and the cue said it did

@sprint-review (58348). The clause added two commits ago promised that
addressing a peer inside a thread "enrols them for the rest of it", flat.
It does not when they have muted the thread: `followByParticipation` writes
only `WHERE thread_user_state.following IS NULL`, and `effectiveFollowerIds`
subtracts `muted` last, so an explicit mute survives both paths.

The mention itself still wakes them — addressing outranks a mute, by design.
What fails is the subscription, which is exactly the half the cue was selling.

Their diagnosis is the reusable part and it is the same shape as the bug it
corrects: I checked that the write HAPPENS and not the condition it is
guarded on. `followByParticipation`'s own docstring names the case outright
("muting a thread and then being mentioned in it is the ordinary case, not an
edge one") and I read past it.

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

* feat(agents): a human is addressed by handle, and the frame never said so (#1244)

* feat(agents): a human is addressed by handle, and the frame never said so

Sam observed 2026-08-25 that seats write about him by name and nothing
routes. The pod-context frame taught three addressing verbs — plain post,
replyToMessageId, threadRootId — and all three move attention between
AGENTS. None reaches a person, and the paragraph never said so, so an
agent that had read it correctly could still conclude that naming a human
was a way of addressing one.

Verified rather than assumed, because the cue is only worth shipping if
the escape it teaches actually works:

- activityService.ts:517-521 builds `mentionNeedle = '@' + lowerUsername`
  and sets `isMention` from `content.includes(needle)`; :591 is the
  `mentions` filter that reads it. Substring on the literal handle.
- resolveHumanMentionUserIds (agentMentionService.ts:1033) extracts
  handles from `[a-z0-9_-]` after an `@`, anchored and case-insensitive.

So `@handle` surfaces in the human's mentions filter and a bare name
matches neither test. The failure is silent — nothing errors, the message
posts, no attention routes — which is why the cue names the outcome and
not just the prescription.

This is the human-facing twin of the gap ADR-018 D6.3 closed for bots: a
message plainly ABOUT someone still has to be addressed TO them before
anything routes. There the fix was a missing implicit-reply wake; here
only the author can supply the handle.

Deliberately teaches the escape and not a heuristic. Whether a bare name
SHOULD route is an open decision (TASK-070b) precisely because name
matching is fuzzy — every message about Sam is not for Sam — so the cue
must not imply that writing the name is enough.

Tests pin the two halves separately (prescription, and the silent-failure
outcome) plus a control built from the pre-change clauses most likely to
keep a loose assertion green: the frame already contains "human" twice and
"@" many times. Mutation-checked — softening "A bare name notifies nobody"
fails the second test and leaves the other two green.

Stacked on #1216, which edits the same frame string; based on its head
rather than main so the two clauses do not conflict.

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

* fix(cue): the handle is necessary, not sufficient — state the ceiling too

@sprint-review's review of #1244: every clause about the failure was
precise and nothing stated the ceiling of the remedy, so an agent reads
"a bare name notifies nobody" as "and the @handle notifies somebody". It
does not. Humans have no AgentEvent delivery row, so the handle buys the
`isMention` flag on the activity feed — a pull surface ADR-017 keeps off
the push channel. Re-derived the narrower half myself rather than
borrowing it: `resolveHumanMentionUserIds` is called only inside
`if (threadRootId)` (:1743), so a plain channel post gets the flag alone
and not even the thread follow.

That would have been a new false model replacing an old one, and harder
to catch — the message now looks correctly addressed while the seat sits
waiting on an answer nobody was told to give.

Two assertions, both mutation-checked; the control gains the same pair.

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

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

* test(mentions): pin the human-handle mechanism and put a budget on the wake frame

TASK-074. The pod-context frame makes assertions to agents about how the
kernel behaves, to a reader who cannot falsify them: a seat acts on the cue
and has no view of `enqueueMentions`. Every test on #1216/#1244 is a
string-presence assertion, so a cue can become FALSE while its text is
untouched and the suite stays green.

Two files, both mutation-checked against the pre-existing 113.

**Claim 4 — "the handle is necessary and not sufficient; nothing pushes."**
`agentMentionService.humansAreNotWoken.test.js`, 6 cases, each negative
paired with a control:

- a human @handle enqueues no AgentEvent of any type; the same sentence to
  an installed seat does; one message naming both routes only to the seat.
- the thread-follow half is guarded: a plain channel post makes no
  `followByParticipation` call and does not even run the lookup; the same
  message inside a thread does follow that human; and a follow is not a
  wake — the threaded case still enqueues nothing.

Blind-mutation baseline, run with the new file REMOVED, per @pod-architect's
method on #1249:

| mutation | pre-existing 113 | with this file |
|---|---|---|
| enqueue a chat.mention per resolved human handle (TASK-070b answered "push it") | **113 green** | 4 red |
| hoist `resolveHumanMentionUserIds` out of `if (threadRootId)` | **113 green** | 1 red |

Both are the realistic future edit, not a crude break. The first is the
literal open decision in TASK-070b; the second reads as a consistency fix.

**The frame's own size.** `agentMentionService.frameBudget.test.js` measures
the rendered `chat.mention` content for a reference wake — plain chat pod,
one seat, explicit mention, no thread, no wake-on-message — currently 2,875
chars, and asserts it two-sided against 2,600/3,000. A ceiling alone is
satisfied by deleting the frame, and the copy assertions elsewhere pin
sentences one at a time; neither notices a section going missing. Verified in
both directions: +200 chars fails the ceiling, gutting the Collaboration
block fails the floor.

Not a cap. Raising `BUDGET_MAX` is one line, and that line is the point — it
turns an invisible per-wake, fleet-wide spend into a deliberate one a
reviewer can argue with. **#1216 will fail this and should raise it in its
own diff**; that is the mechanism working, not a conflict.

**Two corrections to the task row I filed, both found by running it.**

Claims 2 and 3 were already pinned, behaviourally, on the shipped SQL —
`threadWakeScope.test.js` runs `effectiveFollowerIds` against pg-mem with the
real DDL, 24 cases. Dropping `OR id = $1` fails 15; dropping the muted
subtraction fails 6; dropping `following IS NULL` from
`followByParticipation` fails exactly the one test written for it. The row's
claim that "every test on both PRs is a string-presence assertion" was wrong
about those two, and nothing here re-covers them.

And #1244 is NOT on main — it merged into #1216's branch, which is still
open. The human-handle cue is unshipped; these tests pin the mechanism at
main, so they hold either way and become that cue's missing companion when
#1216 lands.

122/122 green across all seven agentMentionService suites on Node 22.

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

* test(mentions): carry #1265's budget and raise it for the three-verbs clause

@sprint-review sharpened the merge-order note correctly: order was necessary,
not sufficient. `BUDGET_MAX` lives only on #1265's branch, so this PR could not
raise a constant it did not have — which meant a bulk press turned `main` red in
EITHER order (this first, then #1265 lands on an over-budget frame; #1265 first,
then this one lands red).

Merging #1265's branch here removes the ordering hazard instead of documenting
it. The raise now travels with the growth that caused it, so this PR is safe to
merge in any order, and #1265 stays mergeable on its own.

The band is 3550/4100, kept as tight around the new 3,935-character reference as
2600/3000 was around 2,877. Leaving MIN at 2,600 would have let a third of the
frame disappear without failing — the exact hole the lower bound was added to
close.

What the fleet buys for the extra ~1,058 characters (+37%), per the constant's
own instruction to state the trade: the three addressing verbs, spelled out.
Agents were choosing between plain post / replyToMessageId / threadRootId with
no statement of what each one does to attention, and picking wrong in both
directions — broadcasting what should have been threaded, and threading what
needed a ping.

135 passing across `agentMentionService`.

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

* docs(mentions): cite the call site by symbol, not by line

@sprint-review caught that this comment's `:1743` had drifted to `:1773` — my
own #1265 merge moved the call and left the citation pointing 30 lines short.
Inside the paragraph arguing that claims decay, which is a fair place to be
caught.

Their call was that it is not worth a push of its own, and for a line-number
correction I agree. This is not that: a raw line number in a comment is a
citation that expires on the next edit above it, so fixing the number restores
the same defect for the next person. `resolveHumanMentionUserIds` has exactly
one call site and it is inside the `if (threadRootId)` branch of
`enqueueMentions` — both of which survive an edit that moves the line.

The reason for the change is left in the comment, so the next author sees why
the form is a symbol rather than a number and does not helpfully convert it
back.

Comment-only; the budget test measures string literals on non-comment lines, so
the frame is unchanged. 135 passing across `agentMentionService`.

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

* style(mentions): reflow the over-long comment line left by the citation fix

Comment-only. 4bb0e6d replaced the stale `:1743` citation but left one
line running well past the wrap the rest of the paragraph keeps.

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

* test(cues): give the mechanics half of the three-verb cue a live reader

Self-review gap in this PR, found by mutation on 2026-08-29.

This PR pinned the CHOOSING half of the three-verb cue against the live
frame and left the MECHANICS half unread. The existing control test is
not a reader of it: `mechanicsOnly` is a literal in the test file
asserted against itself, which is the right shape for proving the
choosing assertions discriminate and the wrong shape for noticing that
the cue changed.

Measured before: deleting any mechanics clause from the live cue left all
140 tests green. So did INVERTING one — rewriting the frame to tell every
woken agent that a threaded continuation "pings every member of the pod,
loudly", which is the exact opposite of what `effectiveFollowerIds` does.
Deleting a choosing clause reds 1, so the instrument worked and the gap
was one half of one sentence.

Adds three tests that read the live frame:

  names all three verbs
  states that replyToMessageId pings the author it addresses
  states that threadRootId does NOT ping, and never claims it does

The third excludes the contradiction as well as asserting the negative,
because a cue can carry both sentences at once.

Mutation table, each anchor asserted at exactly one occurrence before
applying, each restored after:

  drop the threadRootId defining clause      was 140 pass -> now 1 FAILED
  drop "its author is pinged"                was 140 pass -> now 2 FAILED
  drop the three-verb opener                 was 140 pass -> now 1 FAILED
  invert to "pings every member of the pod"  was 140 pass -> now 1 FAILED
  copy-edit: colon -> semicolon, reworded         143 pass (unchanged)
  copy-edit: reword the plain-post clause         143 pass (unchanged)

The two copy-edit controls are the point: these are clause-level rather
than whole-paragraph, so ordinary editing does not red the build while a
claim reversal does.

One assertion was tightened after its own mutation came back green.
`toContain('threadRootId')` passes even when the clause defining that
verb is deleted, because the name appears again later in the same frame
("continue the detail under your own root with threadRootId"). It now
asserts 'continues a thread', which reds. A bare name match on a string
that repeats is not a reader of the sentence you meant.

Suite: 140 -> 143, 8 suites, all passing.

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

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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