fix: prove the typebot upload path, make history import durable, and order the message hooks - #52
Merged
Merged
Conversation
The upload success path had never been executed: typebot-client.test.ts covers uploadFile in isolation and turn.test.ts covers only the failure branch, so the seam between a WhatsApp media message and the upload request was guaranteed by types alone. Wires the real client into the real turn handler for both upload branches, pinning the decoded byte length, the multipart field ordering, the attachment bytes, and the fileUrl threaded into continueChat.
…ch failures getChatHistory has no cursor, so paging is impossible; requesting media for a deep window makes the host download every blob serially and blow the 30 s per-capability budget. Media is now requested only at a limit of 25 or below. fetchHistory also stops returning [] on error, which was indistinguishable from an engine with no history support and is what made the failure silent.
…criminate The bulk-sweep test added for the media-budget fix asserted only that no conversation was created for a chat whose history fetch failed. That assertion also holds if the null-safety guard regresses (an unguarded ordered.length throws on null, but the surrounding per-chat catch swallows it with the same observable outcome), and it duplicated existing coverage. Replaced it with a two-chat sweep where the first chat fails and the second succeeds, asserting the second chat's conversation and history are unaffected, plus a log assertion that catches the guard regression: a reverted guard produces a second, spurious 'bulk backfill failed' log for what is really just an already-explained fetch failure. Also renamed the 'swallows a getChatHistory failure (best-effort)' test to reflect the false-return contract it now proves.
Whether a chat's history had been imported was never stored — it was inferred from whether the conversation had just been created. Once the conversation existed the inference read false forever, so a single failed fetch meant that chat never got its history and nothing said so. The state now lives on the chat's own mapping document, with a bounded retry so a chat that can never be imported does not add a 30 s timeout to every message it receives. InboundDeps now requires onBackfillExhausted, so buildDeps in index.ts wires it to a log line to keep the plugin compiling; turning that into a counted, health-check-visible signal is follow-up work.
…marker Three defects surfaced by review of the durable per-chat backfill marker: - The `!backfill.done` guard in relayInbound had no test forcing it; deleting it left the suite green while an already-imported chat would re-run a full getChatHistory and replay on every later message, forever. - backfillHistory returned true whenever the history FETCH succeeded, even if every post in the replay then failed (Chatwoot down as a chat's first message arrived). The caller wrote backfillDone on that true, permanently skipping the chat's history with no retry and no onBackfillExhausted — the exact silent loss this feature exists to remove, now recorded as done. replayHistory now reports whether every attempted message actually posted, and both backfillHistory and the bulk sweep only record a completed import when it did. A genuinely empty history still counts as success. - onBackfillExhausted reported the raw (possibly @lid) chatId instead of the key the marker is actually patched under, so on the dual-lookup path an operator would be shown an id matching no stored state.
…kfill window A chat that exhausts its import attempts now appears on the plugin health surface instead of being a silent gap, and the config field states that attachments are only imported for a window that fits the host budget.
Nothing described co-installation, so plugins shipped without a priority and the chain order was whatever the loader happened to produce. Documents the ascending sort, the bands, the rule that an observer must never claim, and what a claim means for a handler that works off-dispatch.
Both ran at the default priority of 100, behind plugins that already end the chain — chat-flow and group-translate both return continue:false today — so a claimed message was silently absent from the spreadsheet and the Chatwoot thread, and which messages went missing changed with every restart.
… band Both act on a message before any responder should see it, but ran at the default priority alongside the auto-repliers, so whether a translation reached the group before or after a bot answered the untranslated text depended on enable order.
faq-bot and after-hours already knew whether they had replied and threw the answer away, so installing them together meant one customer message could draw several replies, with the survivor depending on enable order. Both now claim when — and only when — a message was actually delivered, and all three take an explicit responder priority.
Both return before their work finishes, so neither could claim on the outcome — and http-action runs first among the responders, so without a claim every plugin behind it answered the same message too. Both now decide ownership from a pure synchronous predicate: a matching command prefix, and an in-scope chat.
…gelog and comment
…ated Whether a chat needed its history imported was inferred from the absence of `backfillDone` on its mapping. Every mapping written by an earlier release carries no backfill fields at all, so on the first message after an upgrade each already-imported chat read as "never imported" and replayed its whole window into Chatwoot again — duplicates posted ahead of the message that was actually arriving, in every open conversation. The `seen` dedup markers expire after three days, so they do not protect an install upgraded later than that. `ensureConversation` now stamps `backfillDone: false` when it creates a mapping, and the trigger tests for that value rather than for a falsy one. An absent field means the mapping predates the marker and is left alone, which is exactly the behaviour those installs have today. A chat whose import failed before the upgrade is therefore never retried; every chat created from here on gets the durable retry. Also guard both marker writes. They sit between the import and the live relay, so a rejected `set` — the host rejects every write once the plugin is at its 50 MiB quota — threw past `relayMessage` and turned a perfectly relayable message into a retry-queue entry, with another full 30-second import behind it on the drain. Failing to record the marker now costs at most one redundant import, never the message.
Both plugins document a specific claiming contract, and neither had a test
that would notice it changing. Mutating faq-bot's fallback claim to return
false left all 12 of its tests green; mutating group-translate's hook to
return `{continue: true}` left all 47 of its tests green, coordinator suites
included — the coordinator pins `swallow`, but nothing pinned that the plugin
forwards it.
faq-bot gains a case for an unmatched message with `fallbackReply` set, which
asserts both that the reply was actually delivered and that the message was
claimed, so a regression that claims without sending cannot pass.
group-translate gains two: a `/tr` admin command must be claimed, an ordinary
translated message must not. Its context fake needed real messaging and engine
implementations first — with the empty stubs, every hook fire threw inside the
coordinator and was swallowed by the handler's catch, which is why no existing
test could see the mutation.
Also add the `onBackfillExhausted` member to five chatwoot-adapter fixtures
whose comment claimed every required callback was listed explicitly. It was
not, which made the comment worse than none.
The "Known interaction" note said faq-bot claims every message when `fallbackReply` is set, so after-hours never sends. The fallback is cooldown-gated at 600 seconds by default, so it claims only the first message in each window and after-hours answers the rest — two different replies to one out-of-hours conversation, not silence. The section now describes that, and keeps the existing advice to leave `fallbackReply` empty when both plugins are enabled. The starvation that IS unconditional was undocumented: typebot-connector claims on scope alone, with no cooldown and nothing an operator can narrow for a one-to-one chat, so after-hours never fires while it is enabled. That is intended — a Typebot bot owns the chats it is in scope for — and out-of-hours messaging belongs inside the flow. Documented alongside the faq-bot case; no priority changed. The chatwoot-adapter README still described the history import as including media at any window size. Attachments have been limited to windows of 25 or smaller since the 30-second import budget was addressed; the manifest already said so, the README did not. The voice-transcription and group-translate changelogs claimed their new priority guarantees them the message "regardless of what other plugins are installed". That holds across the official plugins but not against a third-party plugin registered lower. Both now claim what is actually guaranteed: ahead of every responder.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes three defects left open after the v0.12.0 alignment round. They are independent and can be read separately, but they ship together because the third one touches every plugin.
1.
typebot-connector: the file-upload path was never provenuploadFilewas covered in isolation andhandleTurnwas covered only on its failure branch, so the success path — a WhatsApp media message becoming a Typebot attachment — had never been executed end to end. The seam betweenmsg.media.{mimetype,filename,data}anduploadFile({mime, filename, data})was guaranteed by types alone.upload-e2e.test.tswires the realTypebotClientinto the realhandleTurn, faking only the network and the WhatsApp send, and covers both upload branches. It pins the things types cannot: the decoded byte length ingenerate-upload-url(not the base64 length), the multipart boundary matching between header and body, every S3 policy field appearing before thefilepart, byte-exact attachment content, and thefileUrl— not the single-usepresignedUrl— being threaded intocontinueChat.A written smoke procedure covers what a fake fetch cannot: whether a real S3 endpoint accepts the multipart body. It gates the next
typebot-connectortag and has not been run yet, so nothing in this PR claims it has.2.
chatwoot-adapter: history import could fail permanently and silentlyWhether a chat's history had been imported was never stored. It was inferred from whether the Chatwoot conversation had just been created — and once that conversation existed, the inference read
falseforever. A single failed fetch meant that chat never got its history, with one log line and nothing on the health surface.Three things caused or compounded it, and all three are fixed:
engine.getChatHistoryis bounded at 30 seconds host-side, the host downloads each blob serially, and it does not cancel the work on timeout. There is no cursor on that capability, so paging is not available. Attachments are now requested only when the configured window is 25 messages or smaller; above that, older media arrives as a placeholder line and the conversation's shape survives intact.fetchHistoryreturned[]on any error, which reads identically to an engine with no history support. It now returnsnullfor a failed fetch and[]only for a genuinely empty chat.Import state now lives on the chat's own mapping document — no new storage keys, since the host re-measures its quota by stat-ing every key on every write. A failed import retries on the chat's next message and gives up after three attempts, at which point the chat appears in the plugin's health status instead of vanishing.
Upgrade behavior: the marker is written only for chats this version creates, so upgrading does not re-import conversations that already exist. Enabling the opt-in one-time bulk sweep afterward still re-imports them; the changelog says so.
3. Nine plugins could all answer the same message
No plugin declared a hook
priority. The chain order was therefore the loader's directory scan at boot, or click order after an operator enabled a plugin by hand — so installingafter-hours,faq-botandchat-flowtogether could draw three replies to one customer message, with a different one surviving after every restart.PLUGIN-STANDARD.mdgains a "Co-installation" section defining the contract, and every plugin now declares its band:gsheets-logger10,chatwoot-adapter20voice-transcription40,group-translate50http-action70,chat-flow75,faq-bot80,typebot-connector85,after-hours95Hooks sort ascending, so observers running first is a correctness requirement rather than a preference: a responder's
{continue:false}ends the chain, and an observer ordered behind one silently loses those messages.chat-flowandgroup-translatealready claimed conditionally, so that hole was live before this change.Responders now claim the messages addressed to them. A claim means "this message is mine", decided from a synchronous predicate — which is what lets the two off-dispatch plugins claim at all, since they return before knowing whether the reply will succeed.
http-actionreuses the samematchActioncall its handler uses, andtypebot-connectorreuses the sameinScopepredicatehandleTurnre-checks, so a claim and the reply that follows it cannot disagree about which messages belong to the plugin. A reply that was suppressed by a cooldown or that threw is never claimed — claiming there would suppress the message for every later plugin while nothing was actually sent.Two interactions follow from the ordering and are documented rather than engineered around:
faq-botwith a fallback configured claims the first message in each cooldown window, and atypebot-connectorbot owns every chat in its scope, so out-of-hours messaging belongs inside the Typebot flow rather than inafter-hours.Versions
gsheets-loggertakes a patch bump — its only change is an explicit priority. The other eight take a minor bump: each gained or changed claim behavior, which changes what other installed plugins observe.plugins.jsonand the READMEs are regenerated.Verification
520 tests pass (487 before),
tsc --noEmitis clean, andcatalog:checkreports no drift. Every new assertion was checked against a mutation of the behavior it claims to pin — including the negative ones, where a falsy default makes a vacuous test easy to write by accident.