Repository navigation
feat(attention): an agent mention nobody answered for a week is missed, not "needs you" - #2364
lilyshen0722 wants to merge 2 commits into
Conversation
…d, not "needs you" A thread reply is the only reply (Sam, 2026-09-06), so an agent's mention that nobody answered in its thread stayed in the needs-you queue for good. Seats post status and gate notes that name a person; the 2026-10-20 audit found 640 such rows older than a week (535 on one operator account, 104 on Sam's, whose queue held 119, the oldest from 09-11). - AttentionItem.resolvedBy gains `missed`: resolved, never counted as acted on. The row stays in the store and its message stays in the pod. - sweepMissedAgentMentions resolves an open mention as `missed` once its source is AGENT_MENTION_MISSED_DAYS (7) old and its author is an agent, re-checking `status: open` at write time so a reply or acknowledgement that lands during the pass keeps its own resolution. A person's mention, a decision, an approval and a handoff never expire this way, and a row with no recorded actor is left alone. - schedulerService runs it hourly and logs every pass. Reply semantics are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wkz5axAzCvoLEFkZ5KtkKG
…list Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wkz5axAzCvoLEFkZ5KtkKG
lilyshen0722
left a comment
There was a problem hiding this comment.
Code gate — #2364 at 3bc4f0a0a324ad41f1b5661200a1a19ca54c2a11
origin/main = cd37b32e0f9288ee24922362d3b86d17c5590368, which is this PR's merge-base. CI at the exact head: 14 pass / 2 skipping; Test & Coverage and Service Tests (Tier 1 — real DBs) both SUCCESS in run 39484258341, whose head_sha I read back as the head above. BASE: 3 suites / 52 tests.
No defects. Ten mutations, every one discriminates — including the two I expected to come back green.
mutation (attentionItemService.ts) |
site | result |
|---|---|---|
drop kind: 'mention' from the find |
:305 |
1 failed |
drop status: 'open' from the write filter |
:322 |
1 failed |
| treat every actor as an agent | :317 |
1 failed |
| invert the bot filter | :315 |
1 failed |
drop the createdAt fallback branch |
:310 |
1 failed |
| window 7 → 8 days | :298 |
2 failed — including the window is seven days |
apply default true → false |
:301 |
1 failed |
drop the actorUserId existence clause |
:307 |
1 failed |
resolvedBy: 'missed' → 'acknowledged' |
:323 |
1 failed |
A person's items never expiring is structural, not incidental
I checked what the guarantee actually rests on rather than taking the test's word. Kind is 'mention' | 'approval' | 'decision' | 'handoff' (:23), and recordTaskAttention writes kind: 'handoff' as Kind (:414) — so decisions, approvals and handoffs are outside the sweep's find by type, not by timing, and a person's mention is excluded by the isBot: true actor filter. Three mutations each break it from a different side, which is the right shape for a guarantee with two independent terms.
The $or on sourceCreatedAt with the createdAt fallback is this file's own established idiom — resolveMentionAttentionForReply carries the identical pair at :195-200, with the comment explaining why ("sourceCreatedAt is written on all new rows"). Reused verbatim, so it is consistency rather than a new risk. Worth noting that the single top-level $or is also safe here: the pre-existing site needs $and: [{ $or: targets }] precisely because two top-level $ors would clobber, and this query has only one.
Two things to know, neither a change
The apply default is the inverse of both sibling sweeps, deliberately. sweepResolvedMentionAttention (:250) and sweepDoneTaskHandoffs (:445) both default apply = false; this one defaults true, and the scheduler calls it bare. That is pinned — flipping the default to false fails a test — so it is a decision, not a slip. The cost is that someone reading across the three, or calling this one from a script on the house assumption of dry-run, writes rows instead of counting them. One sentence in the docblock would close that; I would not change the default now that a test depends on it.
resolvedBy has no reader in this repo — only writers. grep finds four write sites and no branch that reads it; getOpenQueue filters on status. So of the stated property, the "resolved" half is verifiable and pinned here, and the "never counted as acted on" half lives entirely in Growth's NEEDSYOU allowlist, which is outside this repo and cannot be checked from it. The comment you added to the enum is the right mitigation and is the only thing carrying it — keep it in any rebase.
The query is unindexed (no AttentionItem index leads with kind or status; the two status indexes lead with recipientUserId and podId), so the hourly pass is a collection scan — exactly like both sibling sweeps, and the eligible set collapses after the first pass. Not worth an index on 640 rows.
The shared rebase: not one line, and I ran it
The note says whichever merges second takes "a one-line rebase that keeps both." Measured — git merge-tree reports content conflicts in three files, and performing the merge needs four resolutions:
AttentionItem.ts— the TS union and the block comment above it. Both sides rewrote the same comment differently, so this is a prose merge, not a line union.AttentionItem.ts— the Mongooseenumarray.attentionItemService.ts— theexport defaultandmodule.exportslines. This one is a rename collision, not a union: #2363 renamesTASK_HANDOFF_REtoTASK_HANDOFF_CANDIDATE_REand redefines its pattern, while this PR's export line still names the old symbol. Keeping either side verbatim loses real names; both naive choices fail loudly (a missing identifier is a compile error, a dropped one reddens the other PR's tests), so nothing goes silent — but "keep both" is ambiguous here in a way it is not for the enum.backend/__tests__/unit/services/attentionItemService.test.js— and this one does not parse if you resolve it the obvious way. Both sides append adescribeblock, and the});\n});after the conflict region closes only one of them. I concatenated the two sides and gotTS1005: '}' expectedat line 781, with that suite failing to load while the other four passed. The correct resolution inserts ours' two closers before theirs opens.
Good news, measured rather than assumed: the two PRs are compatible. With all four resolved properly, the production files typecheck clean (50 pre-existing tsc errors on the tree, all in cleanup-test-data.ts and friends, none in any merged file) and the combined suites are 5 suites / 86 tests, all passing — 78 from #2363's scope plus this PR's 8. So the only thing wrong with the note is the word "one-line", and the one place a careless resolution bites is the test file rather than the enum everyone is watching.
Written by sprint-review, a Commonly agent — https://commonly.me/v2/pods/6a692a1be833c668acdb84cf
GATE: PASS
|
Folded into #2363, which now carries this change as commit |
Why
Only a thread reply counts as a reply (Sam, 09-06). So an agent's mention that nobody answers in its thread stays in the needs-you queue for good. Seats post status and gate notes that name a person, and nothing ever expired them.
A read-only dry run on production, 10-20, found 640 open agent-mention rows older than a week:
Sam's queue held 119 rows, the oldest from 09-11, and almost none of them still needed him. A separate check of every ask in it found none that did.
What
AttentionItem.resolvedBygainsmissed. It means resolved, but never counted as acted on. The row stays in the store, and its message stays in the pod.sweepMissedAgentMentions({ now, apply })resolves an openmentionasmissedwhen both hold:AGENT_MENTION_MISSED_DAYS(7) days old:sourceCreatedAt, falling back tocreatedAt;actorUserIdresolves to anisBotuser.The write re-checks
status: 'open', so a reply or acknowledgement that lands during a pass keeps its own resolution.Never expired: a person's mention, a decision, an approval or a handoff. A row with no recorded actor is left alone too.
schedulerServiceruns the sweep hourly at :17 and logs every pass.When this sweep goes live, Sam's queue drops from 119 to about 15, by rule, with nothing deleted.
Tests
attentionItemService.test.js:missedand a person's doesn't;createdAtfallback;models/AttentionItem.resolvedBy.test.js: the real schema acceptsreplied,acknowledgedandmissed, and refuses anything else.Mutations. Each fails 1 test:
missedacknowledgedinsteadstatus: 'open'guard at write34 related suites pass (445 tests). Lint shows 0 errors and
tscis clean on the changed files.Also needed outside this PR
Growth's daily NEEDSYOU line counts "acted" as
resolved && resolvedBy !== 'acknowledged', so it would countmissedas acted on. That script also has to excludemissed. It's Growth's script, so the change has been flagged there.Merge
Draft. It goes in Wednesday's second deploy if it's gated by then, otherwise the next one.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Wkz5axAzCvoLEFkZ5KtkKG