Skip to content

fix(bluebubbles): skip webhook server bind in standalone send path (salvage of #12439 by @Wonham) - #51763

Open
Bartok9 wants to merge 3 commits into
NousResearch:mainfrom
Bartok9:salvage-12439-bb-sendonly
Open

fix(bluebubbles): skip webhook server bind in standalone send path (salvage of #12439 by @Wonham)#51763
Bartok9 wants to merge 3 commits into
NousResearch:mainfrom
Bartok9:salvage-12439-bb-sendonly

Conversation

@Bartok9

@Bartok9 Bartok9 commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds connect(send_only=True) to the BlueBubbles adapter so the standalone send_message path no longer tries to bind the local webhook server.
  • User-facing impact: agent-initiated iMessage sends (e.g. cron delivery, the send_message tool) stop failing with OSError(EADDRINUSE) when the gateway is already running and holding the webhook port.

Motivation

Salvage of #12439 by @Wonham, re-targeted onto current main.

tools/send_message_tool._send_bluebubbles() builds its own BlueBubblesAdapter and calls adapter.connect(), which unconditionally starts an aiohttp AppRunner/TCPSite on self.webhook_port. When the gateway process already owns that port, the bind raises EADDRINUSE and the send aborts — even though an outbound-only caller never needs to receive inbound webhooks.

Fix

  • connect(*, send_only: bool = False): after the REST ping/server-info handshake, send_only=True calls _mark_connected() and returns without starting the webhook server or registering it.
  • The standalone send path passes send_only=True. Gateway startup keeps the default (send_only=False) and still binds + registers exactly as before.

Verification

  • .venv/bin/python -m pytest tests/gateway/test_bluebubbles.py58 passed

Real behavior proof

  • Environment: repo .venv (CPython 3.11).
  • With the fix (captured):
    tests/gateway/test_bluebubbles.py::TestBlueBubblesConnectSendOnly  2 passed in 0.40s
    
  • Without the source fix (source reverted, tests kept — captured):
    E   TypeError: BlueBubblesAdapter.connect() got an unexpected keyword argument 'send_only'
    FAILED ...TestBlueBubblesConnectSendOnly::test_connect_send_only_skips_webhook_bind
    
  • Regression tests: test_connect_send_only_skips_webhook_bind (stubs AppRunner/TCPSite/_register_webhook and asserts none run) and test_connect_default_still_binds_webhook (asserts the default path still binds + registers).
  • What was NOT tested: an actual live EADDRINUSE collision against a running gateway (the bind path is stubbed; the test asserts the code path is skipped).

Salvage credit: original fix by @Wonham (#12439).

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have labels Jun 24, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Salvage of #12439 by @Wonham, re-targeted onto current main — tracked as related, not a duplicate. The earlier standalone-send attempts #27542 and #31639 manually replicate adapter internals; this send_only=True flag on connect() is the cleaner canonical approach. Maintainer should close the stale predecessors in favor of this.

@Bartok9
Bartok9 force-pushed the salvage-12439-bb-sendonly branch from f91589c to 5e8f955 Compare June 24, 2026 11:39
@Bartok9
Bartok9 force-pushed the salvage-12439-bb-sendonly branch from 5e8f955 to 0a744e5 Compare July 5, 2026 11:32
@whyy9527

whyy9527 commented Jul 6, 2026

Copy link
Copy Markdown

I think this still needs one more lifecycle guard before merge.

This PR skips the local aiohttp bind/register path for connect(send_only=True), but _send_bluebubbles() still calls adapter.disconnect() in finally. On current main, disconnect() unconditionally calls _unregister_webhook() and _mark_disconnected().

That means a standalone send-only adapter can still clean up gateway-owned state: it uses the same BlueBubbles config/webhook URL as the gateway adapter, so _unregister_webhook() can find and delete the webhook that the gateway process registered, even though this send-only adapter never created the webhook listener or registered that webhook. It can also write runtime status as disconnected for a one-shot outbound sender.

I hit the original EADDRINUSE failure locally with the gateway already running. A variant that kept standalone BlueBubbles delivery REST-only end-to-end worked: connect(send_only=True) returned after ping/server-info without touching gateway runtime status, and disconnect() only unregistered the webhook / marked disconnected when this adapter instance actually had a _runner. A live hermes send --json --to bluebubbles:<phone> then succeeded while the gateway stayed connected.

Suggested shape:

  • connect(..., send_only=True) should not call _mark_connected() for the standalone sender.
  • disconnect() should call _unregister_webhook() and _mark_disconnected() only if this adapter instance owns the webhook lifecycle, e.g. self._runner is not None.
  • Add a regression test for connect(send_only=True) followed by disconnect() asserting it does not call _unregister_webhook() or runtime status writers.

That keeps outbound REST sends separate from the gateway-owned inbound webhook lifecycle.

@Bartok9

Bartok9 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Good catch — fixed in e073b6958c.

You're right that the standalone sender shared the gateway's webhook lifecycle. Two changes:

  • connect(send_only=True) now returns after the ping/server-info handshake without calling _mark_connected(). send() doesn't depend on _running, so the one-shot outbound path no longer writes gateway-owned runtime status.
  • disconnect() gates _unregister_webhook() and _mark_disconnected() on self._runner is not None. A send-only adapter never starts a runner, so it can no longer delete the webhook the gateway registered or mark the platform disconnected. The gateway path (with a real _runner) still unregisters + marks disconnected exactly as before.

Added two regression tests:

  • test_connect_send_only_skips_webhook_bind now also asserts _mark_connected() is never called and _runner stays None.
  • test_disconnect_send_only_leaves_gateway_state — send-only connect+disconnect asserts _unregister_webhook() and _mark_disconnected() are not called.

tests/gateway/test_bluebubbles.py: 63 passed. Thanks for the detailed lifecycle analysis.

@alt-glitch alt-glitch added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages comp/tools Tool registry, model_tools, toolsets labels Jul 6, 2026
@Bartok9

Bartok9 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Plate-clear 2026-07-11: connect(send_only=True) + send_message_tool wiring + disconnect gates + tests re-applied on current main.

@Bartok9
Bartok9 force-pushed the salvage-12439-bb-sendonly branch 2 times, most recently from d68ce67 to ea64957 Compare July 12, 2026 23:05
@Bartok9

Bartok9 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

@whyy9527 reconfirming after rebase onto current main.

Lifecycle guard remains in place:

  • connect(send_only=True) returns after the ping/server-info handshake and does not call _mark_connected() (_runner stays None).
  • disconnect() only calls _unregister_webhook() / runner cleanup / _mark_disconnected() when self._runner is not None, so a standalone send-only adapter cannot delete the gateway-owned webhook or overwrite gateway connected status.

Regression coverage:

  • test_connect_send_only_skips_webhook_bind — no bind/register, no _mark_connected, _runner is None
  • test_disconnect_send_only_leaves_gateway_state — connect+disconnect does not call _unregister_webhook / _mark_disconnected

tests/gateway/test_bluebubbles.py: 63 passed on ea6495723e (rebased to 7b5ba20547).

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for preserving the adapter-level design and for incorporating the lifecycle feedback from the discussion. The standalone-send premise is confirmed on current main: tools/send_message_tool.py:1805-1815 creates a separate adapter and invokes default connect(), while gateway/platforms/bluebubbles.py:281-295 binds and registers the inbound webhook. The PR keeps the default gateway path intact while introducing the narrowly scoped outbound-only path.

Problems

  • The changed call site at tools/send_message_tool.py:1806 is not directly covered. The new adapter tests validate connect(send_only=True), but no test invokes _send_bluebubbles() and asserts that the tool layer supplies the flag.

Suggested changes

  • Add a focused tests/tools/ regression test using a fake adapter to assert _send_bluebubbles() calls connect(send_only=True), then sends and disconnects. The existing adapter tests should remain the lifecycle contract coverage.

All required PR CI checks passed. This is an automated hermes-sweeper review.

@@ -1803,7 +1803,7 @@ async def _send_bluebubbles(extra, chat_id, message):
from gateway.config import PlatformConfig
pconfig = PlatformConfig(extra=extra)
adapter = BlueBubblesAdapter(pconfig)
connected = await adapter.connect()
connected = await adapter.connect(send_only=True)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add a tool-level regression test that invokes _send_bluebubbles() with a fake adapter and asserts this call receives send_only=True. The new adapter tests cover the mode itself, but not the delivery-path wiring changed here.

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 15, 2026

@GottZ GottZ left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This was generated by AI during triage.

Summary

Two PRs address the same BlueBubbles standalone-send failure: #12439 introduces an outbound-only connect path to avoid binding the gateway-owned webhook port, while #51763 salvages that design on current main and additionally prevents send-only disconnects from unregistering the gateway webhook or changing gateway runtime status. #51763 also adds adapter-level lifecycle regression coverage, but the changed tool-layer call site remains untested.

Related pull requests

  • #12439 [closed] duplicate — (+10/-3) — superseded by #51763: This closed PR identified the root cause and introduced connect(send_only=True) in the standalone sender, but its version marks the send-only adapter connected and lacks the later disconnect ownership guards, so it remains relevant as the original implementation and attribution source rather than the merge candidate.
  • #51763 related — (+128/-8) — keep open pending one focused regression test: The diff avoids the outbound-only webhook bind and correctly gates disconnect cleanup on _runner, preventing deletion of gateway-owned webhook and status state. Consistent with the keep_open review on #51763, add a tool-layer test proving _send_bluebubbles() calls connect(send_only=True), then sends and disconnects; the current tests cover only the adapter contract.

Duplicates

#12439 and #51763 implement substantially the same connect(send_only=True) fix; #51763 is the current-main salvage with the necessary lifecycle guards and expanded regression coverage.

Suggested consolidation

Keep #51763 open until the focused _send_bluebubbles() wiring regression requested by the keep_open review is added; then merge #51763 as the complete current-main implementation. #12439 is already closed and should remain closed as superseded by #51763, with its original authorship preserved through the documented co-author attribution.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup12439 ["PRs duplicating each other"]
        P12439["PR #12439 (closed)"]
        P51763["PR #51763 (open)"]
    end
    class P12439 closed
    class P51763 open
    class P51763 target
    click P12439 "https://github.com/NousResearch/hermes-agent/pull/12439"
    click P51763 "https://github.com/NousResearch/hermes-agent/pull/51763"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed or no verify verdict yet (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 10 kB of PR diffs, 4 kB of issue/PR text, 7 kB of discussion (11 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

…alvage of NousResearch#12439 by @Wonham)

Rebuilt on latest main (Bartok9 hygiene 2026-08-01).
Original: NousResearch#51763
@Bartok9

Bartok9 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Rebuilt onto latest main via patch re-apply (force-push). Please re-run CI.

— Bartok9 public PR hygiene 2026-08-01

@Bartok9
Bartok9 force-pushed the salvage-12439-bb-sendonly branch from ea64957 to cf0ca28 Compare August 1, 2026 17:35
…tok9

Per-PR attribution so check-attribution passes on this branch (Teknium).

@GottZ GottZ left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This was generated by AI during triage.

Delta since our previous triage comment

@Bartok9 rebuilt #51763 onto the latest main via patch re-apply and force-pushed a new head. The refreshed diff preserves the lifecycle fix, but it still does not add the focused tool-layer regression test requested in the visible keep_open review; no new CI result is included in the delta.

Changed pull requests

  • #51763 related — (+29/-8) — keep open: The rebuilt diff still passes send_only=True from _send_bluebubbles() and gates disconnect cleanup on runner ownership, but despite the keep_open review on #51763, it still lacks a direct test proving that the tool call site connects with that flag, sends, and disconnects.

Suggested consolidation

Recommendation unchanged: keep #51763 open pending the requested tool-layer regression test and fresh CI on the force-pushed head.

Complex graph unchanged since our previous triage comment.

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 6 kB of PR diffs, 4 kB of issue/PR text, 7 kB of discussion (12 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@Bartok9

Bartok9 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Added the focused tool-layer regression requested in the keep_open review.

tests/tools/test_send_message_bluebubbles_send_only.py uses a fake adapter and asserts _send_bluebubbles():

  • calls connect(send_only=True)
  • then send(...)
  • then disconnect()

Local: pytest tests/tools/test_send_message_bluebubbles_send_only.py — 1 passed. Adapter lifecycle tests remain the contract coverage; this covers the tool call-site wiring. CI should pick up the new head shortly.

Add focused tool-layer regression asserting connect(send_only=True),
send, and disconnect for the standalone BlueBubbles path (NousResearch#51763).
@alt-glitch alt-glitch added P2 Medium — degraded but workaround exists and removed P3 Low — cosmetic, nice to have comp/tools Tool registry, model_tools, toolsets labels Aug 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants