fix(slack-agent): decide thread follow-ups after the ack, not inside it - #358
Merged
Conversation
Two production failures, both surfacing as the bot going completely
silent, both from one root cause: the engagement decision was made inside
the webhook verifier — i.e. inside Slack's ~3s ack budget, against an
in-memory cache a deploy wipes.
1. Restart. Cold cache, so the first follow-up paid the full workspace
resolve + `conversations.replies` round-trip in-budget, blew
PROMOTION_DEADLINE_MS and was dropped. Observed: a webhook that took
exactly 2009ms, no turn, no log, not even the `:eyes:` ack — three
minutes after the bot's own last post in that thread.
2. Delay. Engagement expired after ENGAGEMENT_MAX_AGE_SECONDS (30 min),
and the thread fetch derived its `oldest` bound from that same
constant. For a reply 61.3 minutes after the bot's last post the
fetch window therefore began 31 minutes AFTER the bot had last
spoken: the check could not see the bot's own messages and could
only answer "not engaged". The webhook completed in 406ms — no
timeout, just confidently wrong.
Widening the window would have made (1) worse: a longer window is a
heavier in-budget fetch. So both are fixed together, by moving the
decision off the budget entirely.
Promote first, confirm afterwards. `promoteThreadFollowUp` is now
synchronous and network-free — it parses, decides the event is a
plausible candidate, registers it pending, and rewrites `event.type`.
`confirmThreadFollowUp` runs from the mention handler, which eve invokes
inside `waitUntil` after the 200, against the thread that handler already
loads for turn context. No second Slack fetch, no deadline, nothing on
the webhook's budget. The verifier is once again parse-only, which is
what it always should have been: it is the only awaited work before eve's
200.
New bounds, both must hold:
- The thread is not dormant, measured from the message immediately
before the reply rather than from the bot's last post. That
distinction is the fix for (2): a thread people are still using is a
live conversation whether or not the bot has spoken lately, and an
hour of human back-and-forth is a normal shape for an incident
thread. 24h of silence is not.
- The bot's engagement is within the trailing 15 messages
(ENGAGEMENT_RECENT_MESSAGE_WINDOW, unchanged) — the real guard the
clock was always a poor proxy for.
It fails open. Where the evidence is absent rather than negative — the
thread unreadable, or truncated past eve's 50-message page — the
follow-up is promoted anyway. A wrong drop costs a user their message
with nothing on screen to explain it, which is exactly how both incidents
presented; a wrong dispatch costs one turn in a thread the bot was
already in. Not symmetric.
Now that a decline is distinguishable from "the bot was never here",
disengaging from a thread the bot HAD worked in DMs everyone who spoke in
it — permalink, why, and the one @mention that brings it back
(agent/lib/disengage-notice.ts). TTL-guarded to once per thread per
episode, fire-and-forget, never throws. Every decline also emits a
structured log; ids only, never message text.
The `:eyes:` ack for promoted follow-ups moves to the same place, so the
bot stops acking messages it then drops — the ack is a promise. Real
app_mentions keep acking in the verifier as before.
Removed as dead: PROMOTION_DEADLINE_MS, withDeadline (and its Bun 1.3.14
timer note), fetchThreadRepliesFromSlack, ThreadFollowUpDeps /
ThreadReplyMessage, ENGAGEMENT_MAX_AGE_SECONDS and the `oldest` bound
derived from it, and the negative engagement cache — it existed to spare
a busy channel one `conversations.replies` call per message, and the
confirm path spends no network at all. The positive cache stays: it is
the instant fast path, and the only thing covering threads longer than
eve's page.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hread runs past the bot The notice fanned out to every human in the thread for both disengagement reasons, but the two describe opposite situations and one fan-out rule cannot serve both. `thread-dormant` is a conversation that went quiet for a day: nobody is mid-discussion, everyone who took part has a stake in knowing the bot has stepped out, and the DM is the only thing that will tell them. Everyone still gets it. `engagement-buried` is the reverse — the thread is *lively*, it has simply run past the bot's last involvement. Fanning out there DMs people who moved on long ago, about a message they did not write, in a thread that is still busy: the notification becomes the noise it was meant to prevent. Only the author of the unanswered reply needs it. Falls back to the full list when the replier is unknown or resolves to the bot itself — telling the wrong set of people is recoverable, telling nobody is the failure this path exists to prevent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🍁 Maple PR previewNote Preview resources were removed when this pull request closed. Final commit |
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
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.
Two production failures, both surfacing as the bot going completely silent in a Slack thread, both from one root cause: the follow-up engagement decision was made inside the webhook verifier — i.e. inside Slack's ~3s ack budget, against an in-memory cache a deploy wipes.
conversations.repliesround-trip in-budget, blewPROMOTION_DEADLINE_MS(2s), and was dropped. Observed: a webhook that took exactly 2009ms, no turn, no log, not even the:eyes:ack — three minutes after the bot's own last post in that same thread.ENGAGEMENT_MAX_AGE_SECONDS(30 min), and the thread fetch derived itsoldestbound from that same constant. So for a reply 61.3 minutes after the bot's last post, the fetch window began 31 minutes after the bot had last spoken: the check literally could not see the bot's own messages, and could only ever answer "not engaged". The webhook completed in 406ms — no timeout, just confidently wrong.Widening the window would have made (1) worse (a longer window is a heavier in-budget fetch), so both are fixed together by taking the decision off the budget entirely.
Promote first, confirm afterwards
promoteThreadFollowUpis now synchronous and network-free: parse, decide the event is a plausible candidate, register it pending, rewriteevent.typetoapp_mention. The verifier is parse-only again — which is what it always should have been, since it is the only awaited work before eve's 200.confirmThreadFollowUpruns from the mention handler, which eve invokes insidewaitUntilafter the 200, against the thread that handler already loads for turn context. No second Slack fetch, no deadline, nothing on the webhook's budget.@mentionthrough a pending registry keyedteamId:channelId:messageTs— not by sniffingmessage.text, which eve may have re-rendered by then.dispatchInboundMessagecatches a failing handler and abandons the turn before the model sees anything).New bounds
Both must hold, else disengage:
ENGAGEMENT_RECENT_MESSAGE_WINDOW, unchanged) — the real guard the clock was always a poor proxy for.It fails open. Where evidence is absent rather than negative — the thread unreadable, or truncated past eve's one 50-message page — the follow-up is promoted anyway. A wrong drop costs a user their message with nothing on screen to explain it (exactly how both incidents presented); a wrong dispatch costs one turn in a thread the bot was already in. Not symmetric.
Telling people it stopped
Disengaging is now distinguishable from "the bot was never in this thread", so a disengagement that ends a conversation the bot had worked in DMs everyone who spoke in it: permalink, why, and the one
@mentionthat brings it back (agent/lib/disengage-notice.ts). TTL-guarded to once per thread per episode, fire-and-forget, never throws. Every decline also emits a structured log — ids only, never message text;warnwhen the bot was in the thread,infofor the routine case.The
:eyes:ack for promoted follow-ups moves to the same place, so the bot stops acking messages it then drops. Realapp_mentions keep acking in the verifier as today.Removed as dead
PROMOTION_DEADLINE_MS,withDeadline(and its Bun 1.3.14 timer note),fetchThreadRepliesFromSlack,ThreadFollowUpDeps/ThreadReplyMessage,ENGAGEMENT_MAX_AGE_SECONDSand theoldestbound derived from it, and the negative engagement cache — it existed to spare a busy channel oneconversations.repliescall per message, and the confirm path spends no network at all, so a cached "no" would save nothing and could only go stale. The positive cache stays: it is the instant fast path, and the only thing covering threads longer than eve's page.loadThreadContextsplits intoloadThreadMessages+formatThreadContextso the rawSlackThreadMessage[]feeds both the engagement check and the rendered context from one fetch.nullnow means "Slack could not be read", distinct from an empty thread — the follow-up check reads that difference.Tests
bun test251 pass / 0 fail (18 files),bun typecheckclean,oxlintclean — all fromapps/slack-agent.thread-follow-up.test.tsreworked to the new shape (48 tests) plus a newdisengage-notice.test.ts(22). Both incidents have named regression tests:fetchstub asserting zero network calls;Worth double-checking
Every threaded reply in a channel the bot is in is now optimistically promoted, so a non-follow-up costs one
thread.refresh()and a dropped dispatch (with an eve "handler failed" line) instead of a cached "no". That is inherent to confirming after the ack — there is no way to answer for a thread we know nothing about without reading it — but it does mean moreconversations.repliestraffic in busy channels, which matters if this app is on Slack's post-2025-05 non-Marketplace rate-limit tier.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.