Skip to content

fix(slack): recover broadcast replies after connection gaps - #1187

Merged
mvschwarz merged 1 commit into
mvschwarz:mainfrom
XTLine:fix/slack-broadcast-reply-recovery
Oct 12, 2026
Merged

mvschwarz merged 1 commit into
mvschwarz:mainfrom
XTLine:fix/slack-broadcast-reply-recovery

Conversation

@XTLine

@XTLine XTLine commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

What a user gets

Replies sent with Slack’s Also send to channel option now recover after a connection gap when they appear in the configured default channel’s history. Previously live inbound accepted these thread_broadcast messages, but recovery silently skipped every threaded message and checkpointed past them. Recovered replies now use the existing sender checks, exact thread routing, duplicate identity and durable retry path, with the usual late-delivery marker.

This is a narrow remaining case from #739, following #742 and #910. Issue #739 remains open: non-broadcast thread replies still require separate recovery, and secondary mapped channels remain outside the existing default-channel scan.

How you verified it

Published commit: 147a58edb6a666a3aedf8d9bc464c800128ef2a8, based on upstream 9109360f57124b50c7cccf583485bdb38c4ef242. Its tree 48bbee1a99c4bf7489055b34a2ad4cf393def4c9 exactly matches local tested commit 2dda53c044937e1ca443105820ce1f1358a7ee4b; only publication metadata differs.

  • Fail first: replacing only the candidate recovery source with the exact base version produced ten intended behavioral assertion failures; 22 existing tests passed. Restoring the fix passed all 32 cases in the two changed test files.
  • All 42 Slack daemon test files passed: 458 tests, run in bounded batches with one worker.
  • Coverage includes actual gateway wiring and cached HTTP status, real SQLite queue persistence, open/closed/superseded/unmapped thread routing, live/recovery deduplication, page-local order, short-page continuation, restart checkpoints, repeated reconnects, periodic retry, dead-letter custody and unchanged rejection of bots, unknown senders, malformed broadcasts and ordinary thread replies.
  • npm run build and npm run lint passed. All four Slack CLI files passed: 56 tests.
  • npm run test:repo did not fully pass in this environment. After retrying the script suite with a writable task-local npm cache: 286 passed, four failed because rsync is absent (spawnSync rsync ENOENT), one opt-in test skipped. The separate first-run reference, documentation guard, mirror digest and generated-context checks all passed.
  • Library round-trip restored all 3,535 source files and five symlinks with exact hashes/targets; the restored source passed the 32 focused cases.
  • Full npm test, the complete workspace suites, and installed-package container scenarios were not run.

The tests inject synthetic Slack Socket Mode/history responses into production recovery and routing. The generic stub-agent scenario has no Slack history endpoint, so it cannot exercise this bug; these checks cover the actual transport-to-queue path instead. No live Slack connection, real sleep/wake cycle, native agent execution or browser test was performed. This backend-only fix has no screenshot.

Anything you were unsure about

The change only recovers broadcast replies that Slack exposes in the existing configured-channel history interval. It does not enumerate ordinary threads, change API scopes, add a new endpoint, or rewind intervals checkpointed before this fix. Existing rate, retention, visibility-margin and global-order limitations still apply. Routing remains the live path’s behavior, including preserving the original seat for closed or superseded roots and withholding gate correlation for a superseded root.

One commit, 83 additions and 16 deletions (99 total changed lines). No dependency, migration, version bump or changelog change.

  • One concern per PR; no version bump; no CHANGELOG.md edit
  • Tests added or updated where the change is testable
  • I listed the checks I ran, their results, and any checks I could not run

@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 16e209be-fd27-4bf3-a876-4f7394cbb400

📥 Commits

Reviewing files that changed from the base of the PR and between ada3678 and 147a58e.


📒 Files selected for processing (4)
  • docs/reference/slack-app-setup.md
  • packages/daemon/src/domain/gateway/slack/channel-recovery.ts
  • packages/daemon/test/slack-channel-recovery.test.ts
  • packages/daemon/test/slack-recovery-status.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review.



📝 Walkthrough
📝 Walkthrough

Walkthrough

Channel recovery now includes eligible Slack broadcast replies in the configured channel history. Recovered replies retain thread routing. Tests cover recovery paging, routing, filtering, retries, duplicate handling, and status reporting.

Changes

Slack Broadcast Reply Recovery

Layer / File(s) Summary
Broadcast reply scan and recovery scope
packages/daemon/src/domain/gateway/slack/channel-recovery.ts, docs/reference/slack-app-setup.md
The scan accepts thread_broadcast events with distinct thread timestamps. Documentation describes the updated recovery scope and its exclusions.
Recovery routing and status validation
packages/daemon/test/slack-channel-recovery.test.ts, packages/daemon/test/slack-recovery-status.test.ts
Tests cover broadcast recovery across paging, thread mappings, filtering, duplicate handling, dead-letter retry, and status reporting.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: mvschwarz


Merge Risk: ⚪ Minimal · up to 147a5

The change appears mergeable after normal checks. Live Slack and sleep/wake behavior remain untested, but no concrete failure is established.

Pre-merge checks | Passed 3 | Failed 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check Warning Issue #739 requires reconnect recovery for human messages lost during a Socket Mode gap, or a missed indication. The change recovers only thread_broadcast replies found in the configured default cha… Extend reconnect recovery to the remaining required message cases, including ordinary tracked-thread replies, and add dead-connection detection or an explicit missed-message report. Add automated coverage for those cases before treating #73…
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Out of Scope Changes check Passed The source change, recovery tests, status tests, and documentation all directly support broadcast-reply recovery. The tests cover routing, deduplication, checkpointing, and dead-letter retry for that …
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: recovering Slack broadcast replies after connection gaps.

Full details: Linked Issues check

Explanation

Issue #739 requires reconnect recovery for human messages lost during a Socket Mode gap, or a missed indication. The change recovers only thread_broadcast replies found in the configured default channel history. The diff and PR description leave ordinary thread replies and dead-connection detection outside this change. Therefore the issue's broader coding requirement remains unmet.

Resolution

Extend reconnect recovery to the remaining required message cases, including ordinary tracked-thread replies, and add dead-connection detection or an explicit missed-message report. Add automated coverage for those cases before treating #739 as satisfied.


Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. (1 skipped: 1 unsupported.)


  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@openrig-review openrig-review left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at 147a58e. Slack replies sent with "Also send to channel" during a connection gap are now recovered from the configured channel's history and land once, with their thread's routing and the late-delivery marker. They go through the same sender, bot, duplicate and routing checks as live inbound; ordinary thread replies are still left to live delivery. Required CI passed at this head, on current main.

— dev60-planner@v-openrig-build

@mvschwarz
mvschwarz merged commit dbc8abe into mvschwarz:main Oct 12, 2026
10 checks passed
@mvschwarz

Copy link
Copy Markdown
Owner

Merged to main, thanks @XTLine. It will ship in the next release. Thanks for a second fix after #1156. #739 stays open for thread replies that aren't broadcast. Two optional doc notes for a later change, neither needed: the sentence saying it 'does not replay intervals already checkpointed' reads more like a changelog line than a description, and the edit dropped 'dead-connection detection' from the out-of-scope sentence without saying that the socket layer now has liveness checks.

@mvschwarz

Copy link
Copy Markdown
Owner

An update, @XTLine: OpenRig 0.6.11 is out on npm, and this change is in it (release notes: https://github.com/mvschwarz/openrig/releases/tag/v0.6.11). Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants