feat(session): let callers name which messages a read receipt covers - #1397
Conversation
rmyndharis
left a comment
There was a problem hiding this comment.
Thanks for this, and for the unusually clear write-up. The diagnosis is correct: Baileys acknowledges individual message keys, and the receipt node enumerates ids explicitly rather than carrying a read-up-to watermark, so acknowledging only lastMessage() really does leave the earlier messages of a burst unacknowledged. Upstream says as much in its own README ("A set of message keys must be explicitly marked read now. You cannot mark an entire 'chat' read"). Thank you also for flagging the group limitation yourself instead of letting it land quietly.
On the question you asked
You offered two ways to fix the missing participant, and asked which I would prefer. Neither, as it turns out: the blocker you describe does not exist.
BaileysAdapter builds a single host object and hands the same one to every delegate, so BaileysContacts already receives getStoredMessage at runtime (baileys.adapter.ts:153, passed at :199 alongside BaileysMessaging at :198). The only thing missing is the declaration on the narrow BaileysContactsHost interface (baileys-contacts.ts:14). It is one line.
Resolving each supplied id through the store, and using the stored WAMessage.key instead of a synthesised one, fixes four things at once:
participant, so group receipts are attributed.fromMe, which is currently hardcodedfalseand will be wrong for an id that belongs to an outbound message.- the JID dialect, since the stored key carries the address the message actually arrived on.
- chat ownership, which nothing checks today: a caller can currently acknowledge a message id belonging to a different chat.
That keeps the request shape as a plain string array, so { id, participant } objects are not needed.
Two caveats so this does not surprise you. getMessage is a single findOne, so 100 ids means 100 sequential queries; it is worth adding a batched lookup (In(...)) rather than a loop. And messages that arrived while the gateway was down are never stored, because history-sync backfill is skipped before putStoredMessage, so keep the synthesised key as a fallback when a lookup misses rather than failing the call.
Blocking
CI Lint is red. npm run check:contract-shapes fails with MarkChatRequest ↔ MarkChatReadDto: contract has "messageIds" - hand does not, and Build plus Docker Build are skipped behind it. This one has a trap in it, so please read before fixing: MarkChatRequest is the body type for three methods in sdk/javascript/src/resources/chats.ts (subscribePresence:50, markRead:70, markUnread:79), and subscribePresence is served by SubscribePresenceDto, which has only chatId. Global validation runs forbidNonWhitelisted, so simply widening the shared type lets a caller send messageIds to subscribePresence and take a 400. A dedicated read request type is the right shape, but adding one is not sufficient on its own: scripts/check-contract-shapes.mjs:85 still declares the MarkChatRequest: 'MarkChatReadDto' pair, so that mapping has to move too.
The DTO is shared with the mark-unread route. MarkChatReadDto is the @Body of both markChatRead (session.controller.ts:462) and markChatUnread (:567), and openapi.json $refs one schema from both paths. As it stands, POST /chats/unread now publishes and accepts a field described as "Specific message IDs to mark read" and then discards it. docs/06-api-specification.md:869 already documents that body as reused, so the human-facing spec inherits the same problem. Please give the read route its own DTO.
Group receipts are wrong while the response still says success: true. Covered above; the store-backed key resolves it. Worth noting that the existing fallback path is correct here, because it carries the stored key whole, so this is a regression for group callers who adopt the field.
Please also address
An empty array means "acknowledge the newest message". baileys-contacts.ts:328 gates on messageIds?.length, so [] and undefined collapse to the same branch. A caller that computes its unread set and gets an empty answer is asking for nothing to be acknowledged, and gets the newest message acknowledged instead. The current spec locks that in as intended behaviour. Either reject an empty array with @ArrayNotEmpty(), or treat it as an explicit no-op.
The JID fold is not covered. The shared helper in baileys-send-seen.spec.ts stubs toEngineJid as the identity function, so removing the toEngineJid(chatId) call entirely leaves the whole engine suite green, tsc included. The divergence is visible inside the spec itself: the fallback case asserts 628123@s.whatsapp.net while the new cases assert 628123@c.us. A stub that performs the real @c.us to @s.whatsapp.net fold would catch it, and would also pin the behaviour the store-backed key introduces.
mark-chat-read.dto.spec.ts already exists and was not touched. It covers chatId only. The four new validators, and in particular the 100 cap, have no test.
The cap is invisible to clients. @ArrayMaxSize(100) is runtime only; @nestjs/swagger does not derive maxItems from it, so the published schema advertises an unbounded array and a caller batching 250 ids discovers the limit as a 400. Please add maxItems to the decorator and hoist 100 into a named constant, matching GROUP_PARTICIPANTS_MAX and friends.
Entries have no format validation. @IsNotEmpty({ each: true }) accepts whitespace, so messageIds: [' '] is currently valid and reaches sendNode as a receipt key. A @MaxLength(..., { each: true }), or a @Matches for the id shape, would pin it.
Docs and the agent tool. docs/06-api-specification.md:837 and docs/07-api-collection.md:209 both publish a request body that lists chatId only; the contract gate compares route headings against the published operations, not body fields, so neither goes red. SessionMarkChatRead (src/core/agent-tools/tools/session.tools.ts:93) forwards only chatId, so MCP callers keep both bugs this PR is about.
Smaller points
- The fallback is an immediately invoked arrow inside a ternary. A plain early return reads better and matches
markUnreadandarchiveChatdirectly below. toEngineJid(chatId)sits inside the.map, so it is recomputed per element for a loop-invariant value.- On the checklist:
npm run lintis ESLint only. The CILintjob runs ten further steps, andcheck:contract-shapesis one of them, which is why this came back red despite a clean local run.
The problem is real and the shape is close. Once the keys come from the store I think this lands quickly. Happy to help with the contract-shapes mapping or the batched lookup if either is fiddly.
Baileys acknowledges individual messages, not chats, so POST
/sessions/{id}/chats/read could only mark the newest message the engine
still held in memory. A burst of inbound messages left everything but the
last one unread for good, and a session that restarted since the message
arrived had nothing to acknowledge at all, answering false under a 200.
The request body now takes an optional messageIds array. A caller that
persists inbound message IDs names exactly what to acknowledge and is
subject to neither case; omitting it keeps the previous last-message
behaviour. The list is capped at 100 so one request cannot hand the engine
an unbounded key list to round-trip.
whatsapp-web.js ignores the field: its sendSeen is chat-level and already
marks every message in the chat.
Group chats are only partly served: the keys built here carry remoteJid and
id but no participant, which Baileys aggregates receipts by, so a supplied
id in a group is acknowledged without the sender it belongs to. Direct
chats are unaffected, as is the last-message fallback in both, which
carries the stored key whole.
Address review on the messageIds PR. A synthesised receipt key carried no participant, so a group receipt named no sender and WhatsApp could not attribute it while the API answered success: true. Its hardcoded fromMe was also wrong for an outbound id, and its jid was whichever dialect the caller happened to send. Each supplied id is now resolved through the message store and the stored key is used whole. Ids the store has never seen - history backfill is emitted but not persisted - keep the synthesised key, which is what the 1:1 case ran on before. The store gains a batched getMessages: the receipt path resolves up to a hundred ids per request, and a findOne apiece was a hundred sequential round trips. POST /chats/unread gets its own MarkChatUnreadDto instead of sharing MarkChatReadDto. Sharing published messageIds on a route that discards it. The JS SDK splits the same way: MarkChatReadRequest for markRead, with MarkChatRequest left to markUnread and subscribePresence, which is served by a chatId-only DTO and would have taken a 400 on the extra field under forbidNonWhitelisted. check-contract-shapes maps both pairs, taking the JavaScript floor from 78 to 79. An empty messageIds array is refused rather than read as "acknowledge the newest message", so a caller that computed an empty unread set no longer acknowledges a message it never named. The cap is hoisted to MARK_READ_MESSAGE_IDS_MAX and published as maxItems, since @ArrayMaxSize is runtime-only and the schema advertised an unbounded array. Entries are matched against a non-whitespace token, because @isnotempty accepted ' '. The send-seen spec stubs the real @c.us to @s.whatsapp.net fold instead of an identity function, which had let the whole suite pass with the toEngineJid call deleted. The DTO spec covers the four new validators, and SessionMarkChatRead forwards the list to MCP callers.
Rebasing on main brought the shape gate's request-body coverage to the Python, Go and Java clients, each of which maps MarkChatRequest to MarkChatReadDto. That pair now fails the same way the JavaScript one did: the contract carries messageIds and the hand-written type does not. Each client gets a MarkChatReadRequest carrying the optional list, with MarkChatRequest left to markUnread and subscribePresence, which take the chat id alone. The gate maps both pairs for all four clients and each coverage floor rises by one.
e38d363 to
f5820a6
Compare
|
Thanks, this was a genuinely useful review. Everything you flagged is addressed in 4d7f987, and you were right that the host wiring was not a blocker: Keys now come from the store. Each supplied id is resolved through a new batched Batched, not a loop. DTO split. SDK and gate. Empty array is now refused by JID fold is covered: the spec stub performs the real Cap and entry format. Tests. Docs and MCP. Rebased on main, which widened this. The branch conflicted only in One thing I did not do, on purpose: Chat ownership is not filtered. With the stored key the receipt goes to the address the message actually arrived on, so a mismatched id can no longer be misrouted, only acknowledged through the wrong route by a caller that already holds an OPERATOR key for that session. A filter would have to compare a caller-supplied jid against the stored one, and those legitimately diverge across the lid and phone-number dialects, so it would silently drop valid receipts to prevent something the stored key already makes harmless. Happy to add it if you would still rather have it. On the checklist point: understood, |
External contributions carry "Thanks @handle." on their CHANGELOG entry; this one was missing it.
rmyndharis
left a comment
There was a problem hiding this comment.
All eleven points are addressed. I checked the two claims that are easy to make and hard to verify: deleting the toEngineJid call now fails four tests where it used to pass silently, and dropping the store lookup fails exactly the participant, fromMe and synthesised-key cases. check-contract-shapes reports 320 pairs across five clients, so the new pair is compared rather than skipped.
Agreed on leaving chat ownership unfiltered: the lookup is bound to dbSessionId by the adapter rather than by caller input, so a foreign session's id cannot resolve, and the rest sits inside a session the caller's key already covers.
I pushed one commit adding Thanks @m7fz7. to the changelog entry, which is the convention here for outside contributions.
Two small things go to separate follow-ups rather than hold this up: the MCP tool copies the count bounds but not the entry format, so [' '] still reaches the engine on that path, and MarkChatRequest now serves both markUnread and subscribePresence, which will need the same split if SubscribePresenceDto ever gains a field.
Description
POST /sessions/{sessionId}/chats/readhad no way to say which messages it was acknowledging, and Baileys acknowledges individual messages rather than chats. The engine filled the gap with the newest message it still held in memory, which fails in two ordinary cases:falseunder a200, and no receipt was ever sent.This adds an optional
messageIdsarray to the request body. A caller that persists inbound message IDs (most webhook consumers do) names exactly what to acknowledge, and neither case applies. Omitting the field keeps the existing last-message behaviour, so nothing changes for current callers.Engine handling:
readMessages.sendSeenis chat-level and already marks everything in the chat, so there is nothing to narrow.Input is validated as a string array, each entry non-empty, capped at
@ArrayMaxSize(100)so one request cannot hand the engine an unbounded key list to round-trip. A caller with more than 100 to acknowledge is catching up on history rather than marking a conversation read.Known limitation: group chats
The keys built from
messageIdscarryremoteJidandidbut noparticipant. Baileys aggregates receipt keys byremoteJid:participant(Utils/messages.ts,aggregateMessageKeysNotFromMe) and passes that participant straight tosendReceipt, so in a group a caller-supplied ID is acknowledged without the sender it belongs to. Direct chats are unaffected, sinceparticipantis legitimately absent there, and the existing last-message fallback is unaffected in both, since it carries the stored key whole.Fixing it properly means either resolving the participant from the message store (the engine's contacts delegate has no store handle today) or accepting
{ id, participant }objects instead of bare strings, which is a request-shape decision. Happy to take direction on which you would prefer, or to land this direct-chat-only and follow up.Type of Change
Additive only: the field is optional and the existing request body stays valid, so no generated client breaks.
Checklist
Four new specs: every supplied message acknowledged rather than just the newest, supplied IDs working when the store holds nothing (the restart case), an empty array falling back to the cached last message, and the service passing the field through. Documentation is the
CHANGELOG.md[Unreleased]entry plus the regeneratedopenapi.json;npm run openapi:checkis clean.Verified with
npm run build,npm test,npm run lint,npm run format, andnpm --prefix dashboard run build.Screenshots (if applicable)
Not applicable, no dashboard surface changes.
Related Issues
No existing issue tracks this. Raising it here per CONTRIBUTING's note about discussing REST contract changes first: the change is additive and optional, but it does widen a public request body, so say the word if you would rather have an issue for it.