Skip to content

fix(gateway/signal): poll /v1/receive instead of broken /v1/events SSE (#71636) - #71884

Open
Enough1122 wants to merge 6 commits into
NousResearch:mainfrom
Enough1122:fix/71636-signal-polling-instead-of-sse
Open

fix(gateway/signal): poll /v1/receive instead of broken /v1/events SSE (#71636)#71884
Enough1122 wants to merge 6 commits into
NousResearch:mainfrom
Enough1122:fix/71636-signal-polling-instead-of-sse

Conversation

@Enough1122

Copy link
Copy Markdown
Contributor

Summary

Replace the broken SSE listener in gateway/platforms/signal.py with a polling loop on /v1/receive/<number>, since signal-cli-rest-api v0.100+ (the API version users actually run) does not expose the SSE endpoint.

What this PR does

The current SignalAdapter listens for inbound messages via SSE on /api/v1/events (later attempts at /v1/events). Both endpoints return 404 in signal-cli-rest-api v0.100+. The actual endpoints in v0.100 are:

  • /v1/health (204) — health check
  • /v1/receive/<number> — polling for queued messages

So the adapter silently dropped every inbound message and the bot appeared connected but never replied.

This PR replaces the SSE listener with a polling loop that hits /v1/receive/<account> on a configurable interval (default 1.0s, clamped to >= 1.0s via extra.poll_interval to avoid rate-limit traps). It drops the SSE-specific knobs (SSE_RETRY_DELAY_*, HEALTH_CHECK_STALE_THRESHOLD, last-activity timestamps, _sse_response) that no longer apply, and keeps the health check on /v1/health and the JSON-RPC 2.0 outbound path.

Why opt-out of /api/v1/ vs /v1/

/v1/health is the only health endpoint. The /api/v1/ prefix in the original code was a separate prefix bug that this fix resolves as a side effect.

Tests

tests/gateway/test_signal.py::TestSignalReceivePolling:

  • test_receive_loop_polls_and_dispatches_envelope: mocks the httpx client and verifies the polling URL is http://localhost:8080/v1/receive/%2B15551234567 (with the + properly percent-encoded) and that the envelope is forwarded to _handle_envelope.
  • test_poll_interval_is_clamped_to_one_second: verifies the clamp (>= 1.0s).

Diff scope

2 files, +68/-107 (net negative because we delete SSE-specific code we no longer use).

  • gateway/platforms/signal.py: +55/-104 — drop SSE listener + retry knobs; add polling loop + poll_interval config + URL-safe account encoding
  • tests/gateway/test_signal.py: +33/-3 — TestSignalReceivePolling

Closes #71636

@Enough1122
Enough1122 force-pushed the fix/71636-signal-polling-instead-of-sse branch from 4f4631e to 992c623 Compare July 26, 2026 09:55
@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery platform/signal Signal CLI adapter P2 Medium — degraded but workaround exists needs-decision Awaiting maintainer decision before any implementation sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 26, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #71636 reports the removed SSE endpoint, and #53696 is an open WebSocket-inbound/REST-outbound migration. This PR instead uses HTTP polling for inbound delivery, so the intended Signal transport direction needs a maintainer decision.

@Enough1122

Copy link
Copy Markdown
Contributor Author

Agreeing with the triage that this needs an explicit maintainer nod on transport direction. Quick rationale for the polling direction taken here:

Why polling is the correct unblock for the issue's affected versions:

Relationship to the larger WebSocket migration (#53696):

#53696 is a bigger architectural change (WebSocket-inbound / REST-outbound). It can land independently and supersede this polling path when it does. This PR is intentionally the minimal unblock for v0.100+ users today — every day it sits open, messages sent to Hermes-managed Signal numbers are accepted by Signal's servers but never picked up.

Tuning knob:

The polling interval is the only real configuration choice. Happy to bump the default or make it configurable in config.yaml if reviewers prefer — current value was chosen to match signal-cli-rest-api's recommended --receive debounce.

Pinging for maintainer decision cc @teknium1.

@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 isolating the stale SSE premise: current main still calls /api/v1/events at gateway/platforms/signal.py:428.

Problems

  • gateway/platforms/signal.py:429 unconditionally uses an HTTP JSON GET. The upstream bbernhard/signal-cli-rest-api OpenAPI documents /v1/receive/{number} as polling only in native mode and as a WebSocket in json-rpc mode. This needs a supported-mode decision or transport selection.
  • gateway/platforms/signal.py:898 retargets every JSON-RPC action to /v1/rpc, but the upstream OpenAPI documents /v2/send for sends. The new tests at tests/gateway/test_signal.py:83-112 exercise only receive polling; outbound replies, typing, reactions, contacts, and attachments are not route-tested.
  • The transport documentation remains stale at website/docs/user-guide/messaging/signal.md:9 and its zh-Hans counterpart.

Suggested changes

  • Define supported native/json-rpc modes; use polling only where the upstream route is HTTP and retain or implement WebSocket receive for json-rpc mode.
  • Add route/payload tests for every outbound operation and update both setup guides.

Automated hermes-sweeper review.

Comment thread gateway/platforms/signal.py
Comment thread gateway/platforms/signal.py Outdated
@Enough1122
Enough1122 force-pushed the fix/71636-signal-polling-instead-of-sse branch from fd162da to 74a1928 Compare July 30, 2026 13:11
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
@Enough1122
Enough1122 force-pushed the fix/71636-signal-polling-instead-of-sse branch from 74a1928 to 7e0e30a Compare July 31, 2026 04:33
@Enough1122

Copy link
Copy Markdown
Contributor Author

cc @teknium1 — addressed both inline comments from 2026-07-30:

#1 — transport-mode awareness. Added SignalAdapter._detect_transport_mode() which probes GET /v1/about and GET /v1/receive/{number} during connect():

  • 200 on both → native mode → outbound /v2/send (REST, the docker-compose default).
  • 404 on /v1/receive/json-rpc mode → outbound /v1/rpc JSON-RPC. Inbound over WebSocket is not yet implemented — documented as a known limitation in website/docs/user-guide/messaging/signal.md rather than silently dropping messages.

Operators can lock the choice via platforms.signal.extra.transport_mode: native|json-rpc in config.yaml. Default is auto (probe); falls back to native if the probe fails.

#2 — outbound route per mode. _rpc() now POSTs to self._outbound_path (set during connect() from the probe), so MODE=native deployments land on /v2/send and MODE=json-rpc deployments land on /v1/rpc. Adapter __init__ seeds _outbound_path from the operator override too, so tests / scripts that skip connect() still hit the right URL.

Tests added in tests/gateway/test_signal.py::TestSignalTransportModeDetection:

  • test_native_mode_uses_v2_send
  • test_detect_mode_native_when_receive_endpoint_returns_200
  • test_detect_mode_jsonrpc_when_receive_endpoint_returns_404
  • test_detect_mode_explicit_native_override
  • test_detect_mode_falls_back_to_native_on_probe_failure

Existing TestSignalOutboundRpcRoutes tests (which assert /v1/rpc) keep passing because _make_signal_adapter now defaults transport_mode='json-rpc'. TestSignalTransportModeDetection::test_native_mode_uses_v2_send covers the native branch.

Local: 63 passed, 1 skipped in tests/gateway/test_signal.py. Head rebased onto current upstream main (ab158e808).

— written by Hermes Agent on behalf of @Enough1122

@Enough1122
Enough1122 force-pushed the fix/71636-signal-polling-instead-of-sse branch from 7e0e30a to 0154973 Compare August 2, 2026 15:21
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

One PR addresses issue #71636. PR #71884 replaces the nonexistent SSE and health endpoints with native-mode polling at /v1/receive/{number} and health checks at /v1/health, but it does not restore inbound delivery for the reported json-rpc mode.

Related pull requests

Suggested consolidation

Keep #71884 open with a salvage path: retain the /v1/health correction, native /v1/receive/{number} polling, transport selection, and focused receive tests, but require either a supported json-rpc WebSocket receive path or an explicitly narrower native-only scope. Before acceptance, address the contributor's keep_open review by implementing and testing mode-correct outbound route/payload mappings for sends, typing, reactions, contacts, and attachments, and update all affected transport documentation; there are no duplicate PRs to close, and #53696 is a broader migration rather than an equivalent patch.

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
    I71636(["issue #71636 (open)"])
    P71884["PR #71884 (open)"]
    P71884 -->|best fix| I71636
    class I71636 open
    class P71884 open
    class P71884 best
    class P71884 target
    click I71636 "https://github.com/NousResearch/hermes-agent/issues/71636"
    click P71884 "https://github.com/NousResearch/hermes-agent/pull/71884"
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 (state tag in the node label).

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

@Enough1122
Enough1122 force-pushed the fix/71636-signal-polling-instead-of-sse branch from e785881 to 5497c38 Compare August 7, 2026 02:24
@Enough1122
Enough1122 force-pushed the fix/71636-signal-polling-instead-of-sse branch from 5497c38 to d0bc779 Compare August 11, 2026 06:39
@Enough1122

Copy link
Copy Markdown
Contributor Author

CI note: the only failing check (Python tests slice 5/12) is test_multiplex_busy_input_mode.py::test_profile_route_and_nonmultiplexed_resolution_preserve_boundaries — a test introduced on main by #83550 (merged 2026-08-11). It fails identically on unrelated PRs in the same window (e.g. #82222, #83834), and the implementation it tests (gateway/run.py) is untouched by this PR. The failure is on main, not in this branch; a rebase onto a main that contains the fix (or a maintainer fixing #83550's test) will unblock.

@Enough1122
Enough1122 force-pushed the fix/71636-signal-polling-instead-of-sse branch from 6fe5393 to 812ea61 Compare August 14, 2026 08:32
NousResearch#71636)

signal-cli-rest-api v0.100+ does not expose the SSE endpoint
(/api/v1/events or /v1/events) that the Signal adapter was listening
on. Real endpoints are /v1/health (health) and /v1/receive/<number>
(polling). The adapter used to silently 404 and miss every inbound
message.

Replace the SSE listener with a polling loop on /v1/receive/<number>,
encode the account number for URL safety, and make the poll interval
configurable (extra.poll_interval, clamped to >= 1.0s to avoid
rate limits). Drop the SSE-specific knobs (retry delays, last-activity
timestamps, SSE response holder) since they no longer apply.

Tests:
- tests/gateway/test_signal.py: TestSignalReceivePolling covers the
  poll-and-dispatch happy path and the poll_interval clamp.
…send) vs json-rpc (/v1/rpc)

Reviewer @teknium1 inline feedback on NousResearch#71884:

1. Inbound path. The native daemon's only polling receive endpoint is
   `GET /v1/receive/{number}` (PR's existing fix). json-rpc mode's
   inbound is a WebSocket `subscribeReceive` channel that this adapter
   does not yet speak — documented as a known limitation rather than
   silently dropping inbound in json-rpc deployments.

2. Outbound path. The native daemon speaks `POST /v2/send` for sends;
   the json-rpc daemon speaks `POST /v1/rpc` JSON-RPC. The PR retargeted
   every outbound method to `/v1/rpc` even on native deployments where
   that path returns 404. Added `signal.transport_mode` config (or
   `auto` probe) and routed `_rpc()` through `self._outbound_path`.

Implementation:
- New `_detect_transport_mode()` probes `/v1/about` + `/v1/receive/{number}`;
  200 ⇒ native (outbound /v2/send), 404 ⇒ json-rpc (outbound /v1/rpc).
  Operator can lock via `extra.transport_mode: native|json-rpc`.
- Adapter `__init__` seeds `_outbound_path` from the config override so
  callers that skip `connect()` (tests, scripts) still hit the right URL.
- Documented both modes + the WebSocket gap in
  `website/docs/user-guide/messaging/signal.md`.

Tests added in `tests/gateway/test_signal.py`:
- test_native_mode_uses_v2_send (locks /v2/send under `transport_mode='native'`)
- test_detect_mode_native_when_receive_endpoint_returns_200
- test_detect_mode_jsonrpc_when_receive_endpoint_returns_404
- test_detect_mode_explicit_native_override
- test_detect_mode_falls_back_to_native_on_probe_failure
- _make_signal_adapter now defaults `transport_mode='json-rpc'` so the
  existing outbound-RPC tests (which assert /v1/rpc) keep passing.

Local: 63 passed, 1 skipped in tests/gateway/test_signal.py.
…eceive probe

The transport-mode probe performs a destructive GET /v1/receive/{number}
on native-mode daemons — anything queued at startup is returned (and
dequeued) by the probe itself. Previously those envelopes were dropped,
so a message arriving while the daemon was starting could be eaten.
Dispatch them through _handle_envelope instead (NousResearch#71884).
@Enough1122
Enough1122 force-pushed the fix/71636-signal-polling-instead-of-sse branch from 812ea61 to bbbeec8 Compare August 14, 2026 12:33
@Enough1122

Copy link
Copy Markdown
Contributor Author

Rebased onto a clean main point (16b54e2) to clear the CI failure. The slice 6/12 failure was test_model_cost_guard.py, a pre-existing main-side issue since fixed upstream (#85970) — verified 23/23 locally (cost-guard + turn-lease) on the rebased head. CI re-running.

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 needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists platform/signal Signal CLI adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

4 participants