feat(slack): per-message acknowledgment reactions, phases, reconnect supervision, and Block Kit - #1166
Open
penso wants to merge 25 commits into
Open
feat(slack): per-message acknowledgment reactions, phases, reconnect supervision, and Block Kit#1166penso wants to merge 25 commits into
penso wants to merge 25 commits into
Conversation
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Collaborator
Author
|
@greptile review |
Contributor
Greptile SummaryThis PR implements a comprehensive per-message acknowledgment reaction lifecycle for Slack, adding phase emoji transitions (👀 → 🛠️/🌐/… → ✅/❌), bounded fair-queue admission for Socket Mode and Events API callbacks, Block Kit rendering with safe content splitting, native-stream delivery, and supervised Socket Mode reconnection with exponential backoff.
Confidence Score: 5/5Safe to merge. The reaction lifecycle, fair-queue admission, Block Kit renderer, and native-stream sender are all well-tested and the logic correctly handles queuing, collect mode, supersession, cancellation, and delivery failure. The two observations are minor housekeeping items that do not affect runtime correctness. The two findings are both non-blocking style observations. Files Needing Attention:
|
| Filename | Overview |
|---|---|
| crates/gateway/src/channel_reactions.rs | New file: per-turn reaction controller and registry with correct locking, terminal-wins semantics, stall timer, debounce, and comprehensive tests. |
| crates/channels/src/fair_queue.rs | New bounded per-account FIFO queue; one minor double-log when a worker panics (both specific and generic error messages fire) but otherwise correct drain-on-cancel, rollback, and panic isolation logic. |
| crates/slack/src/blocks.rs | New Block Kit renderer; the previous issue with unbalanced code fences on split is fixed by splitting raw content before re-wrapping each chunk in its own fence; test coverage confirms this. |
| crates/slack/src/native_stream.rs | New Slack native-stream sender; correctly enforces 12,000-char limit per call, falls back to stop-without-markdown on final delivery failure, and has thorough unit tests for chunking and error paths. |
| crates/slack/src/callback_worker.rs | Socket Mode callback queue; correctly commits dedup only on successful admission, returns Cancelled on shutdown, and has tests for the rejection-does-not-commit invariant. |
| crates/httpd/src/server/slack_callbacks.rs | New Events API callback dispatcher; uses a fresh CancellationToken (no external shutdown handle, by design), handles url_verification preflight, and correctly returns 503 so Slack retries on queue saturation. |
| crates/gateway/src/channel_webhook_rate_limit.rs | Token-bucket rate limiter; evict_if_full is defined and documented but never called from production code, so the max_buckets cap (10,000) is dead. Practically safe because account_ids are admin-controlled, but the guard is unreachable. |
| crates/slack/src/socket_reconnect.rs | Reconnect backoff helpers; jitter uses wall-clock subsecond nanos (acknowledged as non-security entropy), backoff advance and reset logic match the call site in socket.rs. |
| crates/slack/src/outbound.rs | response_url validation correctly enforces HTTPS, no credentials, no custom port, and host allow-list (hooks.slack.com / hooks.slack-gov.com) with redirect policy set to none. |
| crates/chat/src/channel_acks.rs | New ack-key helpers for propagating per-message acknowledgment identities through queueing and collect mode; straightforward and well-documented. |
Sequence Diagram
sequenceDiagram
participant Slack
participant HTTPD as HTTPD / Socket
participant Queue as FairQueue
participant Registry as ReactionRegistry
participant Worker as ChannelReactionController
participant Agent as Agent Loop
Slack->>HTTPD: POST /events (HMAC signed)
HTTPD->>HTTPD: verify signature + timestamp
HTTPD->>HTTPD: rate-limit check
HTTPD->>Queue: admit(job) [atomic dedup]
Queue-->>Slack: 200 ACK (immediate)
Queue->>Worker: process_callback
Worker->>Registry: register_pending(ack_key)
Registry->>Worker(ctrl): add_reaction(👀)
Worker->>Agent: chat.send(params + ack_keys)
Agent->>Registry: activate(activity_id, ack_keys)
Registry->>Registry: transfer pending → active
loop Agent turn
Agent->>Registry: note(Tool("web_search"))
Registry->>Worker(ctrl): Phase(🌐) [debounced]
Worker(ctrl)->>Slack: add_reaction(🌐), remove(👀)
end
Agent->>Registry: note(Finished(Success))
Registry->>Worker(ctrl): Finish(Success) [terminal]
Worker(ctrl)->>Slack: add_reaction(✅), remove(🌐)
Worker(ctrl)->>Worker(ctrl): exit
Reviews (14): Last reviewed commit: "fix(httpd): gate Slack callback throttle..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
penso
added a commit
that referenced
this pull request
Jul 24, 2026
Address Greptile P1 on #1166. A fenced code block longer than the 3000-char section limit was wrapped as ```…``` and then split at line boundaries, so the chunks had unbalanced fences (opening with no close, later chunks closing with no open) and Slack rendered them as broken code blocks. A single large code block stays under the 50-block cap, so the plain-text fallback never fired. Split the *raw* code first (reserving room for the delimiters) and re-wrap each chunk in its own ```…``` fence, so every emitted section is a self-contained, balanced code block. Adds a test asserting each chunk has matched fences and stays within the section limit. Claude-Session: https://claude.ai/code/session_016mWjaPu4o3E5dBhZU6tPUL
Collaborator
Author
|
@greptile review |
penso
added a commit
that referenced
this pull request
Jul 25, 2026
…ient Address the two edge cases Greptile noted on #1166 (5/5, non-blocking): - Reaction controller worker now has a hard lifetime cap. Normally the run signals completion long before it, but if a run never finalizes (e.g. the spawned task panics and skips the terminal signal) the cap ensures the worker task — and the 👀 reaction — cannot leak; it strips the marker and exits. - `send_typing` (and native streaming) reuse a shared `reqwest::Client` instead of building one per call. reqwest pools connections; `send_typing` runs on a repeating loop while a turn is active, so per-call construction wasted the pool. Claude-Session: https://claude.ai/code/session_016mWjaPu4o3E5dBhZU6tPUL
Collaborator
Author
|
@greptile review |
…ure terminal `chat.send` spawns the agent run and returns immediately (fire-and-forget), so the previous ack ✅/❌ — applied right after `send` returned in `dispatch_to_chat` — fired before the reply was actually delivered. This reworks acknowledgment reactions into a per-turn controller driven by the run lifecycle. - New `ChannelReactionController` (gateway): a serialized worker owning a single reaction slot on the inbound message. Adds 👀 on receipt, swaps to a phase emoji (🌐/💻/✏️/🛠️/…) classified from the running tool, and finalizes to ✅/❌ — or nothing on cancel. Phase changes are debounced (700ms) to avoid flicker; a stall marker (⏳) appears after prolonged inactivity; terminal transitions win over late phases. - Seam: `ChatRuntime::note_channel_activity(session_key, ChannelActivity)` (default no-op). The agent loop emits `Tool(name)` on each tool start (via the existing `send_tool_status_to_channels` choke point); the run completion emits `Finished(Success|Failure)`; the abort path emits `Finished(Cancelled)`. - Gateway owns a session-keyed controller registry (bounded by register-on-receive / remove-on-finish). `dispatch_to_chat` registers the controller (👀) and only finalizes here for early returns where the run never ran (send error, or an Ok payload with a terminal state). - Cancelled turns now strip 👀 and leave no ✅/❌ (matches hermes) via the new `ChannelAckOutcome` enum. - Glyph→shortcode normalization (`crates/slack/src/emoji.rs`) applied to all outbound reactions so config/phase/model-specified names are Slack-safe (Slack silently drops raw glyphs and colon-wrapped names). Unit tests cover tool→emoji classification, outcome→emoji mapping, and emoji normalization. Existing chat/channels/slack/gateway suites pass. Claude-Session: https://claude.ai/code/session_016mWjaPu4o3E5dBhZU6tPUL
…t status
- Reconnect resilience: `run_socket_listener` now supervises the Socket Mode
connection. slack-morphism's `serve()` returns on disconnect; previously that
silently killed inbound delivery. It now rebuilds the listener and retries
with exponential backoff + jitter (1s→30s cap), resetting after a healthy
connection, with cancellation always winning.
- Rich Block Kit rendering (opt-in `rich_blocks`): replies render block-level
markdown (headings, dividers, fenced code, sections) as Block Kit, enforcing
Slack's limits and always falling back to plain chunked text when content
can't fit — a rich render never loses a message.
- Live status (opt-in `assistant_status`): `send_typing` now calls
`assistant.threads.setStatus` ("is thinking…") in assistant threads. Off by
default and best-effort, since it only applies to AI/Assistant apps.
Adds config fields (+ redaction), edit-modal UI toggles, docs, config template,
and unit tests for the Block Kit renderer and reconnect backoff. E2E extended to
cover the new toggles.
Claude-Session: https://claude.ai/code/session_016mWjaPu4o3E5dBhZU6tPUL
…imit The new `ChannelActivity`/`ChannelAckOutcome` enums and the turn-finished emit pushed `channels/plugin.rs` and `chat/.../send.rs` past the 1500-line limit. Move the enums to `crates/channels/src/activity.rs` (re-exported unchanged) and inline the terminal emit compactly. No behavior change. Claude-Session: https://claude.ai/code/session_016mWjaPu4o3E5dBhZU6tPUL
`just lint` runs clippy with `--all-targets`, which lints test code where the workspace denies `unwrap()`. Match the existing convention used by the other Slack test modules. Claude-Session: https://claude.ai/code/session_016mWjaPu4o3E5dBhZU6tPUL
Address Greptile P1 on #1166. A fenced code block longer than the 3000-char section limit was wrapped as ```…``` and then split at line boundaries, so the chunks had unbalanced fences (opening with no close, later chunks closing with no open) and Slack rendered them as broken code blocks. A single large code block stays under the 50-block cap, so the plain-text fallback never fired. Split the *raw* code first (reserving room for the delimiters) and re-wrap each chunk in its own ```…``` fence, so every emitted section is a self-contained, balanced code block. Adds a test asserting each chunk has matched fences and stays within the section limit. Claude-Session: https://claude.ai/code/session_016mWjaPu4o3E5dBhZU6tPUL
…ient Address the two edge cases Greptile noted on #1166 (5/5, non-blocking): - Reaction controller worker now has a hard lifetime cap. Normally the run signals completion long before it, but if a run never finalizes (e.g. the spawned task panics and skips the terminal signal) the cap ensures the worker task — and the 👀 reaction — cannot leak; it strips the marker and exits. - `send_typing` (and native streaming) reuse a shared `reqwest::Client` instead of building one per call. reqwest pools connections; `send_typing` runs on a repeating loop while a turn is active, so per-call construction wasted the pool. Claude-Session: https://claude.ai/code/session_016mWjaPu4o3E5dBhZU6tPUL
…machine Two improvements to the reaction controller found in self-review: - Same-session collision leak: when a second message for a session arrived while the first was still running, registering the new controller dropped the old handle, so its worker exited on the closed channel and left the previous message's 👀 stuck forever. Finalize the replaced controller (strip its 👀) before installing the new one. - Add state-machine tests with a recording mock outbound covering the full lifecycle: 👀 → ✅ (success), 👀 → ❌ (failure), 👀 → stripped (cancelled, no terminal), and 👀 → tool phase → ✅ across the debounce window. The controller logic was previously only covered by pure-function tests.
…re removing
Two improvements from re-comparing against openclaw and hermes.
Channel-neutral emoji vocabulary (DRY + layering). The reaction controller
lives in the gateway and drives a generic `ChannelOutbound`, but it hardcoded
Slack shortcodes ("eyes", "white_check_mark", …). Matrix uses the emoji glyph
itself as the reaction key and Teams has its own vocabulary, so any non-Slack
channel that populated `ack_message_id` would have reacted with the literal
text "eyes". Per the cross-crate DRY rule, the canonical set now lives once in
`moltis_channels::activity::ack_emoji` as Unicode glyphs, and each channel
translates at its own boundary — Slack's existing `normalize_reaction_name`
maps them to shortcodes, Matrix passes them straight through. This is the same
split openclaw gets from its `StatusReactionAdapter`. A new Slack test walks
`ack_emoji::ALL` and fails if any canonical emoji lacks a shortcode mapping, so
the two crates cannot drift.
Add-then-remove ordering, matching openclaw's `finishWithEmoji` (which applies
the terminal emoji before clearing the rest). Removing first left a window with
no reaction at all, and if the subsequent add failed the message was left bare —
indistinguishable from the bot ignoring it. Adding first means the message
always carries a marker and a failed call degrades to the previous one. Cancel
still just strips the marker. Tests assert the exact emitted sequence.
penso
force-pushed
the
slack-reactions-v2
branch
from
July 25, 2026 07:02
bb25e58 to
b31ccfc
Compare
External review found the acknowledgment reactions were routed by session key, which is wrong whenever a session handles more than one inbound message. Queued messages hijacked the running turn. The controller was registered before `chat.send` decided whether the message had to queue. A second message replaced the first's controller, then returned early without starting a run — so the first run's phases and terminal landed on the second message, the first was left unresolved, and a replayed message never registered anything at all. Terminal cleanup had a matching race: it removed whatever was stored for the session without checking identity, so a slow terminal could release a newer turn's controller and strand its marker. Acknowledgments are now keyed by the message itself (account:chat:ts) and parked on receipt, so 👀 still appears immediately even while queued. A run claims its acknowledgments only once it starts executing — after the queue decision — and releases them under an identity check against a turn id. In collect mode one run legitimately owns several messages and the terminal fans out to all of them. Keys travel through queueing and replay in the request params. Also from the review: - Reaction triggers now require the reacted-to message to be bot-authored (via item_user, failing closed), so a member cannot aim the agent at an unrelated person's message. - Socket Mode acknowledges envelopes immediately and processes off the callback path, with bounded event-id dedup so a Slack retry cannot start a second turn. - /sh runs and hook-rejected messages now finalize their acknowledgment instead of leaving a marker until the lifetime cap; a reply that fails to deliver resolves as ❌ rather than ✅. - Block Kit no longer truncates overlong headings (rendered as sections) and is skipped for replies that need chunking, since the plain-text fallback must carry the whole message. rich_blocks now disables streaming, which otherwise silently bypassed block rendering entirely. - Reconnect jitter was always negative (0.75-0.87x) because nanoseconds were divided by u32::MAX instead of a second. - Reaction state only advances when the API call actually succeeded. - Setup UI and docs list the reaction scopes and event subscriptions. assistant_status is removed: it is unverifiable without an AI/Assistant app and its thread matching needs an exact per-turn identity to be correct. Adds registry tests for queued-message isolation, collect fan-out, superseded turns, no-run finalization, and stray activity. Extracts the queue-drain logic, which was duplicated verbatim across the two run paths.
penso
changed the title
Collaborator
Author
|
@greptile review |
…ued messages Follow-up to the Greptile review, which flagged both spots. The jitter bounds test asserted 0.5x-1.5x while the implementation produces 0.75x-1.25x, so it would have passed against the original one-sided bug it was meant to guard. Tighten it to the documented range and extract the pure factor so a second test can assert the distribution reaches both sides of the base delay directly, instead of sampling the clock and hoping. Collect-mode replay returned early when nothing was replayable — every queued message non-text, or an empty queue — abandoning those messages without resolving their acknowledgments, so their 👀 lingered until the lifetime cap. Those paths now finalize the acknowledgments, since no reply is coming.
…randed acks The Events API was unusable whenever gateway auth was on. is_public_path exempted /api/channels/msteams/ but never Slack, so Slack's callbacks to /events, /interactions and /commands were rejected with 401 before their handlers — and therefore before signature verification — could run. Only those three exact suffixes are exempted, and they remain protected by Slack's HMAC and timestamp checks; anything else under the Slack namespace stays authenticated. A test pins both halves of that boundary. The setup modal also advertised /webhook, which is not a route. It and the docs now list the real Event Subscriptions, Interactivity and Slash Commands URLs. Acknowledgments that could previously linger until the lifetime cap: - A replay that fails, or returns a terminal payload with no run behind it, now settles its keys instead of leaving them owned by a run that never started. - Cancelling a queued message clears its marker. - A controller displaced by a duplicate registration or by the pending cap is finalized rather than dropped; dropping the handle closed the worker channel, whose exit path deliberately leaves the message untouched. - finalize_keys now also drops active-turn ownership, so a later terminal cannot address controllers that are already resolved. Block Kit fence parsing is delimiter-aware: previously any line starting with three backticks toggled the fence, so a longer fence used as content truncated the block, and trailing newlines inside a snippet were stripped. Closing now requires a bare run of at least the opening length, and only the parser's own appended newline is removed. Round-trip tests cover nested fences, info strings and interior blank lines.
…lowing them A streamed reply whose final flush, stop, update or overflow post failed still returned Ok(()). The dispatcher then recorded the target as delivered, skipped the plain-text fallback, and the acknowledgment settled on ✅ — for a reply the user never fully received. Those final calls now propagate. The caller already treats a stream error as 'not delivered', so the reply falls back to a normal send and the acknowledgment reflects what actually happened. Mid-stream throttled edits stay best-effort, since a later edit supersedes them.
…g content Abort could resolve the wrong turn. AbortHandle::abort() kills the run future outright, so it never emits its own terminal; the abort handler emitted one afterwards, addressed by session. Because killing the task also drops the session permit, a new turn could activate in between and receive the old cancellation. The handler now takes ownership of the active turn's acknowledgments *before* aborting, via a registry call that removes them under the lock — turn-addressed by construction, so a turn starting later cannot be caught by it. Abort also skipped queue draining entirely: the killed future never reached the drain step at its end, so anything queued behind it waited forever. The handler now drains after the aborted task has released the session permit (waiting for it first, or the replay would just re-queue itself). Collect mode silently discarded content. It merged only the "text" field while attaching acknowledgment keys from every queued message, and send() prefers "content" over "text" — so one multimodal message anywhere in the batch dropped every other message's words while still reporting them all successful. All messages are now flattened into a single ordered block list, preserving images and text alike. Also: - The terminal reaction retries once. It is the last thing the worker does, so unlike an intermediate phase there is no later transition to correct it. - Losing the outbound entirely, or a panicked delivery task, no longer settles as a success. - Slack interactions are deduplicated by trigger id, so a retried envelope cannot run a button's action twice.
penso
force-pushed
the
slack-reactions-v2
branch
from
July 25, 2026 19:15
9b612b1 to
8db430d
Compare
Collaborator
Author
|
@greptileai review |
Slack callbacks and channel acknowledgments could be reordered, stranded, or finalized against a newer turn because admission, deduplication, abort cleanup, and reaction ownership used different identities and lifetimes. Native streaming also used an obsolete API shape and could silently lose final content. Use run-addressed terminal ownership, bounded ordered callback workers, retry-window deduplication, official native stream payloads, and run-scoped delivery cleanup. Keep callback verification behind exact public routes with pre-auth throttling, and document the required Slack scopes and events.
The reasoning-variant E2E assertion counted ACP agents after main moved them into the model picker because both row types expose provider metadata. Add explicit row identities and scope the assertion to model rows so it continues validating reasoning variant filtering.
Collaborator
Author
|
@greptile review |
# Conflicts: # crates/web/ui/e2e/specs/reasoning-toggle.spec.js # crates/web/ui/src/models.ts
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
withered-breeze-e956 | 93d1b64 | Commit Preview URL Branch Preview URL |
Jul 29 2026, 04:14 PM |
Slack response URL replies rebuilt their restricted HTTP client for every callback, preventing connection reuse. Keep one lazily initialized client while preserving redirect and timeout restrictions, and cover the pending acknowledgment cap boundary explicitly.\n\nCoverage also exposed a sled reopen race: the owning database handle was dropped before its cloned tree handles. Drop the database last so all trees release before the file lock is reopened.
Targeted local validation extracted package names with a trailing carriage return when a crate manifest used CRLF line endings, causing Cargo to reject the package argument. Consume trailing line whitespace when parsing manifest names so LF and CRLF files produce identical commands.
Collaborator
Author
|
@greptile review |
1 similar comment
Collaborator
Author
|
@greptile review |
The three Slack callback endpoints (events, interactions, commands) were registered as three near-identical 70-line closures inside gateway.rs, which had grown to 1470 of the 1500-line CI limit as a result. Extract one `handle_slack_callback` parameterized by `SlackCallbackKind`, with the per-endpoint differences expressed as data: - `endpoint()` names the rate-limit/dedupe scope (already existed) - `ack_response()` holds the one real behavioral difference — slash commands render their response body in the Slack client, so they acknowledge with a bare 200 while events and interactions answer JSON - `event_preflight()` keeps the events-only cases that must answer synchronously: the url_verification handshake and a malformed body gateway.rs drops from 1470 to 1297 lines, and the extracted units are now directly testable (7 new tests covering the handshake, malformed bodies, per-endpoint ack shape, and the 503-on-saturation contract). Separately, restore documentation this branch deleted to fit files under the size limit. channels.rs was 1487 lines on main and the branch added ~250 lines of ack-tracking, so it paid for them by stripping 60 comment lines — including the frontend `sessionPath()` contract, the compaction notice's user-visible behavior, and the "targets are peeked, not drained" rationale that explains why an in-flight run can still reply. Splitting instead of stripping is what CLAUDE.md asks for, so move TTS synthesis into channels/tts.rs and flatten the deeply nested Telegram voice branch into `deliver_reply_to_target` with guard clauses, which buys back the room and restores every comment. Also fixed along the way: - Two doc comments had been left attached to the wrong item. A deleted const's docs dangled on top of `register_channel_reaction_controller` (and claimed it keys by session, not by ack key), and `finalize_channel_acks`'s docs had migrated onto `finalize_active_channel_acks`, leaving the former undocumented and the latter describing the wrong function. - `prepare_collected` set `_ack_keys` from a computed Vec and then re-parsed that same Vec back out of the JSON to return it. Compute it once; the two branches now share the tail instead of duplicating it. - Replace a hand-rolled percent-decoder in the Slack webhook verifier with `form_urlencoded`. Slack posts form bodies where a space is `+`, and a mature decoder is the right tool. Because `form_urlencoded` decodes leniently (a malformed `%GG` survives verbatim), keep the branch's strictness explicitly: a trigger id containing `%` or U+FFFD is decode garbage and must not become a dedupe key, since a low-entropy key would let unrelated callbacks collide and be dropped. - The gateway wrote the literal `"_ack_keys"` while moltis-chat read it from a private const. Export the const and use it on both sides.
Collaborator
Author
|
@greptile review |
# Conflicts: # crates/chat/src/channels.rs # crates/chat/src/lib.rs
Collaborator
Author
|
@greptile review |
Three defects in the reaction/callback work on this branch. `_ack_keys` was trusted from the wire. The `chat.send` RPC handler cloned client params and only *added* `_conn_id`/`_accept_language`/ `_remote_ip`/`_timezone`, so every other `_`-prefixed param a WebSocket client sent survived into the chat service — including the ack keys the channel dispatch path mints to name an inbound Slack message's acknowledgment-reaction slot. One authenticated user could therefore drive the 👀/✅/❌ reactions on an unrelated message in any workspace the bot is in. The strip is targeted rather than a blanket removal of `_`-prefixed keys, because that prefix means "internal plumbing", not "server-only": the web UI legitimately sends `_session_key`, `_audio_filename` and `_document_files`. Only `_ack_keys` confers authority a client cannot hold, and only the channel dispatch path needs it — that path calls the chat service directly rather than through this registry, so removing it here cannot affect it. Folded the duplicated context injection from `chat.send` and `chat.send_sync` into one helper while doing it, and locked in that `_conn_id` is server-set with a test. (`_channel_reply_target` is spoofable the same way and is the more serious of the two — it would let a client address an outbound message to any chat the bot can reach. It predates this branch, so it is not fixed here; it needs its own change and a decision about whether web-originated sends may ever name a channel target.) A panicking Socket Mode callback silenced the bot permanently. There is a single worker draining that queue and `process(job).await` was unguarded, so one panic killed the task and every later callback with it — the bot would go quiet with only the panic in the log to explain why. The sibling HTTP callback workers already catch this; Socket Mode now matches, running each job on its own task so the unwind is localized while awaiting it immediately keeps callbacks strictly ordered. Block Kit headings leaked their markdown syntax. A `header` block's text is a `plain_text` field that Slack renders verbatim, but `heading_text` handed it the raw heading, so `## Install \`foo\`` reached users with the backticks visible — and LLM headings carry inline formatting constantly. Only unambiguously paired constructs are unwrapped (links keep their label; inline code, bold and strikethrough lose their delimiters). Single `*`/`_` are left alone on purpose: in a heading they are far likelier to be part of an identifier (`API_KEY`, `snake_case`) than an emphasis marker, so stripping them would corrupt the text instead of tidying it.
…queue Closes the two follow-ups filed against the reaction/callback work. ## moltis-32nj: chat.send trusted _channel_reply_target from the wire The open question was whether a web-originated send ever legitimately names a channel target. It does not: `chat_impl::send` derives the target itself from the session's persisted `channel_binding`, and only when that session is still the active one for the chat. A client-supplied value was pure spoofing surface — set `_session_key` and the server's own injection is skipped (`is_web_message` requires its absence), so the client's value reached `deferred_channel_target` unchallenged and addressed an outbound message to any chat the bot can reach. So it joins `_ack_keys` in the set stripped at the RPC boundary. Rather than a second ad-hoc removal, the keys that cross the crate boundary now have one home in `moltis_chat::params`, with `SERVER_ONLY` naming the subset a client may never supply and `strip_server_only` applying it. The underscore prefix alone is not the criterion — the web UI legitimately sets `_session_key`, `_audio_filename` and `_document_files` — so the list is explicit and documents why each entry is on it. The literals at the set/read/strip sites are gone, so those three can no longer drift. ## moltis-9d0k: two callback paths, two concurrency models HTTP callbacks had a 16-worker account-fair pool; Socket Mode had a single worker for every account, so a slow workspace head-of-line blocked all the others. Both now use one scheduler, extracted to `moltis_channels::fair_queue` — the lower-level crate both depend on, per the DRY rule in CLAUDE.md. The extraction is generic over a job type that reports its account, and keeps the properties the HTTP path had: per-account ordering (an account is never active on two workers), round-robin fairness with a per-account share of the queue, non-blocking admission that rejects rather than awaits, and a permit carried by the job so capacity bounds queued *and* in-flight work. Panic isolation moved in too, so it now covers Socket Mode, and completion is reported even after a panic — otherwise the account would stay marked active and its remaining callbacks would never be served. Shutdown is one place where the shared queue is deliberately not a straight lift: it watches a *child* of the caller's token, so dropping a queue cannot cancel the token that also governs the connection feeding it, while a cancelled parent still drains the jobs already admitted (those were acknowledged to the platform and will not be redelivered). Scheduler tests moved to `fair_queue` alongside the code, covering ordering, cross-account independence, the per-account cap, capacity return, panic isolation and drain-on-cancel. Each call site keeps the tests for what it still owns: dedupe-commits-only-on-admission on both sides, and the HTTP response shapes. Both suites also gained a direct regression test for the head-of-line blocking this fixes. Note for reviewers: the `blocking_queue` test helpers gate on a `CancellationToken`, not a `Notify`. A token stays signalled once cancelled, so a job reaching its await after the release still proceeds; `notify_waiters` drops that wakeup and hangs the test.
Collaborator
Author
|
@greptile review |
The Swift bridge intentionally builds moltis-httpd without the Slack feature, but the callback throttle variant and limits were still compiled and reported as dead code. Gate the Slack-only state and match paths while retaining them for tests, so both minimal and Slack-enabled builds remain warning-free.
Collaborator
Author
|
@greptile review |
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.
Summary
Builds on the acknowledgment reactions merged in #1165. Slack bots cannot show a typing indicator, so reactions provide the receipt and progress signal. This change makes that lifecycle safe under queueing, cancellation, retries, callback bursts, and delivery failures, while adding phase feedback, Block Kit rendering, and reconnect supervision.
Acknowledgment reactions, per message
Reaction triggers
reaction_triggers.Slack callbacks and Socket Mode
Native streaming and rendering
chat.startStream,chat.appendStream, andchat.stopStreampayload/response contract.thread_replies = falseuses top-level edit-in-place streaming because Slack native streams require a thread.response_urlcalls accept only approved HTTPS Slack hosts, do not follow redirects, and use bounded timeouts.Setup guidance
files:write, and exact Events API callback URLs in settings and onboarding.Validation
Completed\n- [x]
./scripts/local-validate.sh 1166\n- [x]LOCAL_VALIDATE_E2E_CMD='cd crates/web/ui && npx playwright test e2e/specs/channels-slack.spec.js e2e/specs/reasoning-toggle.spec.js' ./scripts/local-validate.sh 1166just release-preflightcargo test -p moltis-chatcargo test -p moltis-slackcd crates/web/ui && npm run build && npx tsc --noEmitcargo fmt --all -- --check./scripts/check-file-size.shRemaining
Manual QA
message_queue_mode = "collect", send several messages quickly and verify every source message receives the terminal reaction.reaction_triggers: verify reactions to another user's message are ignored and a reaction to the bot's message gets a threaded reply.rich_blocks: send headings and a long fenced block and verify balanced blocks with complete plain-text fallback.