feat(simplex): make delivery correlated and complete the messaging lifecycle - #100680
preyevates wants to merge 21 commits into
Conversation
alt-glitch
added
type/bug
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head ee55b72534434f9ffb119ceec121a0be0f36a46a against PR base d4611ac83789a8ff6813f6ab8ecd1eefe1f3ba3c and live main 5f048000d73f8b3fc0bb9090a5d996aa67eacd38. Current main is only three commits ahead of the PR base and those commits are desktop-only, so I am not asking for a churn-only rebase.
There is a lot of good engineering here. The correlated command/result model is a meaningful improvement over “WebSocket write == delivery”; readiness is tied to the real listener; numeric contact IDs are a better addressing primitive; partial/unknown delivery is kept non-retryable; UTF-8/JSON splitting is tested; media cleanup ownership is substantially more explicit; and the edit-supersession commit preserves David Metcalfe's authorship rather than flattening the donor history. The preserved sprmn24/lambertian work is also visible in the commit train. That is the right way to salvage compatible focused work.
I do still have four runtime blockers plus two repository acceptance blockers before this can be the integrated SimpleX lifecycle.
1. Blocker: the text-batch cancellation loss window is still present
plugins/platforms/simplex/adapter.py still has the same sequence documented by #94533:
prior_task = self._pending_text_batch_tasks.get(key)
if prior_task and not prior_task.done():
prior_task.cancel()
...
async def _flush_text_batch(self, key):
await asyncio.sleep(self._text_batch_delay)
event = self._pending_text_batches.pop(key, None)
...
await self.handle_message(event)If the quiet-period sleep has completed, the event has been popped, and a new chunk arrives while handle_message(event) is awaiting, _enqueue_text_event() cancels that in-flight flush. The cancellation lands after ownership has left _pending_text_batches, so the older message is silently lost. The finally only removes the task bookkeeping; it does not re-buffer the popped event.
#94533 is not merely adjacent cleanup: it is the exact other side of this batching state machine and provides the order-preserving re-buffer contract for SimpleX. The new tests prove sender-scoped keys, but they do not exercise cancellation after pop / during dispatch.
Required fix: compose the #94533 behavior (or an equivalent proven mechanism), preserve that contributor's credit, and add a regression that deliberately blocks inside handle_message, enqueues the next chunk, delivers cancellation at that await, then proves both texts survive in order with no duplicate dispatch.
2. Blocker: multiplex profile isolation is still using the default profile's ambient SIMPLEX_* environment
This head still reads process-global env in the adapter constructor and enablement path. In particular:
env_auto = os.getenv("SIMPLEX_AUTO_ACCEPT")SIMPLEX_GROUP_ALLOWEDis read fromos.getenv(...)beforePlatformConfig.extra_env_enablement()readsSIMPLEX_WS_URL,SIMPLEX_AUTO_ACCEPT, andSIMPLEX_GROUP_ALLOWEDdirectly
#100241 reproduces the concrete multiplex consequence: a secondary profile can inherit the default profile's daemon URL, group wildcard, and auto-accept policy; standalone/cron delivery can therefore connect to the wrong SimpleX daemon. That PR changes the same adapter/test surface, so this is also a real merge-order collision, not a theoretical follow-up.
This matters even more in this PR because the new correlated standalone path and approval/media lifecycle make the adapter more authoritative: a correctly correlated command sent to the wrong profile's daemon is still a misroute.
Required fix: either absorb #100241 with its contributor credit intact or land it first and rebase this head over it. The composed object needs the secondary-profile matrix: own value wins, missing value fails closed, default profile retains legacy behavior, and standalone send resolves the scoped daemon rather than ambient default-profile env.
3. Blocker: listener readiness was fixed, but listener ownership is still unbounded
connect() now correctly waits on _ws_ready, but it immediately sets _running, starts _ws_listener(), and returns connected without acquiring the existing scoped platform lock. disconnect() likewise has no matching release.
#35523 documents the observable result: two gateway instances pointed at the same simplex-chat daemon can each own a persistent listener and independently dispatch the same inbound event, yielding duplicate agent turns. Readiness and reconnect correctness do not solve that ownership class.
Required fix: compose the existing scoped-lock contract around the full listener lifetime (including every connect-failure/reconnect/disconnect edge), preserve the donor credit, and prove a second owner cannot enter while the first one holds the daemon URL.
4. Blocker: an allowed SimpleX group is still denied again at the gateway boundary
The adapter correctly gates group intake on SIMPLEX_GROUP_ALLOWED, but after that it builds the MessageSource without carrying an authorization decision. There is no role_authorized=True on the source. The gateway then re-checks the sending member against the DM principal allowlist.
That is the exact defect reproduced by #52241: group-level authorization succeeds in the adapter and is then discarded at the adapter→gateway projection boundary. This head's test_group_sender_prefers_member_contact_id explicitly installs adapter.set_authorization_check(lambda *_args: True), so it proves stable sender identity but bypasses the failing real gateway authorization path.
Required fix: compose #52241's role-authorized signal (or the current generic equivalent) and add the real end-to-end pair: opted-in group + unlisted member is accepted; the same source without the group authorization signal is denied. DMs must remain unchanged.
5. Blocker: this PR grows three already-oversized ownership surfaces instead of extracting the new lifecycle
The changed SimpleX adapter is still roughly 3k lines; this head has live adapter code well past line 2,900. The diff also adds behavior inside gateway/platforms/base.py at existing multi-thousand-line regions (including the retry/sidecar/cleanup path past 5k/7k) and adds edit-supersession ownership in gateway/run.py around the ~9.7k region.
For this repository, the 2k ownership gate is a hard structural invariant. This is exactly the kind of integrated lifecycle where the state machines now have enough independent responsibilities to deserve bounded owners: correlation, batching, transfer/media state, reaction approvals, and streaming/edit delivery should not all accumulate in one adapter module, and generic gateway support should sit behind a bounded seam rather than extending the godfiles further.
Required fix: shard the new SimpleX lifecycle into sub-2k plugin-owned modules and extract the generic gateway edit/sidecar seam into bounded owners while preserving public/plugin identities and existing monkeypatch/import surfaces.
6. Blocker: there is no exact-head hosted acceptance object yet
For exact head ee55b725..., all three repository workflows completed as action_required with zero jobs:
- CI: https://github.com/NousResearch/hermes-agent/actions/runs/33556577428
- Docker Build, Test, and Publish: https://github.com/NousResearch/hermes-agent/actions/runs/33556575702
- Nix flake check: https://github.com/NousResearch/hermes-agent/actions/runs/33556575701
The disclosed local receipts are useful, and I appreciate that the 20 full-suite failures were not hidden. They do not replace exact-object CI, though. The PR currently has 15 surviving commits, so the acceptance gate is not just “eventually make the tip green”: every surviving commit needs a green receipt, and the final composed head must be green after the interlocks above are resolved.
Interlock / supersession notes
- #99550 is a narrower disconnected-send fix for #98949. This PR functionally subsumes that slice, but #99550 itself preserves a #99091 attribution chain. If this PR becomes the surviving implementation, mark #99550 as superseded/duplicate explicitly without erasing that lineage.
- #97317 is genuinely integrated here, and its author survives in the commit history. That part of the attribution story is clean.
- #100241, #35523, #94533, and #52241 are complementary missing lifecycle edges, not duplicates to close. They collide in the same SimpleX surface and need an explicit merge/absorb order plus composed tests.
- The three commits by which live main has advanced are path-disjoint desktop work; they are not a substantive blocker by themselves.
Disposition: not mergeable yet. The central correlated-delivery work is strong, but an “integrated lifecycle” cannot still lose a popped batch on cancellation, inherit another profile's daemon/policy, allow duplicate listener owners, or discard group authorization at the gateway boundary. Close those four runtime classes, put the new ownership behind bounded modules, and prove the resulting exact object (and every surviving commit) green. That would turn this from a very ambitious consolidation into a genuinely safe one.
|
Thank you for the exact-head review. I reproduced each material finding and corrected the branch
The correction series is rebased onto upstream The sole integration failure is the unchanged Home Assistant no-filter expectation and reproduces Fable's final read-only adversarial review of pre-rebase head The sixth finding is now maintainer-gated: GitHub created exact-head CI, Docker, and Nix workflow |
ee55b72 to
c01402f
Compare
Singular newChatItem events (and some newChatItems array elements) nest
the AChatItem one level down ({type: newChatItem, chatItem: {chatInfo,
chatItem}}), but _handle_chat_item only reads chatInfo/chatItem at the
top level, so those messages were silently dropped.
Port only the nested-wrapper normalization from the original branch into
the current dispatch, per review: _normalize_chat_item_wrapper unwraps
the nested AChatItem form, maps the {chatInfo, item} field-name variant,
and passes already-normalized wrappers through untouched. Applied in
_handle_event for both event shapes and idempotently at the top of
_handle_chat_item so the deferred rcvFileComplete replay path is covered
too.
Group routing is unchanged: numeric group IDs with the structured
/_send #<id> json form (display-name commands can resolve ambiguously
or silently drop, and plain commands truncate multiline content).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LJcz6L6JJWpQv5FrTBz6J1
_send_ws() previously returned None on all error paths, causing send() to always return SendResult(success=True) even when the WebSocket was closed or unavailable. This masked delivery failures from callers and the gateway. Changes: - _send_ws() return type: None -> bool (True on success, False on error) - Return False on ConnectionClosed, generic Exception, and missing WS - send() now checks the bool and returns SendResult(success=False) with a descriptive error message when the underlying send fails - test: expect disconnected send to return failure
send() advertised max_message_length but passed full content straight through in a single daemon command, unlike sibling text adapters which self-chunk via the base truncate_message helper. Split long content into <= MAX_MESSAGE_LENGTH chunks via truncate_message and send each in order with the contact/group prefix, in both send() and the out-of-process _standalone_send cron path. Short messages still produce a single send. In _standalone_send the per-chunk corrId now enumerates the loop (hermes-snd-<ms>-<i>) so same-millisecond chunks get distinct correlation ids. Add focused tests: short content -> one send; long content -> multiple ordered sends, each within the limit, with the part-bodies reassembling word-for-word to the original (no content dropped).
Integrate and strengthen the four independently reported SimpleX fixes: lossless cancellation recovery, multiplex profile isolation, daemon listener locking, and allowed-group authorization propagation. Split the adapter into bounded plugin-owned modules while preserving its public surface. Integrated-from: b32aba0 Integrated-from: 32b78ef Integrated-from: 3a3ddf5 Integrated-from: 2d3081f Co-authored-by: Sahil Vishnalya <222165401+Sahilvishnaliya@users.noreply.github.com> Co-authored-by: nftpoetrist <264138787+nftpoetrist@users.noreply.github.com> Co-authored-by: lambertian <288878343+lambertian@users.noreply.github.com> Co-authored-by: Que0x <byquenox@gmail.com>
c01402f to
19ea8d7
Compare
6 participants
What does this PR do?
This makes the bundled SimpleX adapter delivery-safe and completes its supported messaging lifecycle without changing the existing process boundary:
The previous adapter treated WebSocket writes as successful delivery, used mutable display names as direct addresses, handled the wrong contact-request event/command, could silently lose oversized replies, and had incomplete streaming, media, reaction, diagnostic, and reconnect behavior.
Related Issue
Relates to #98949.
Type of Change
Changes Made
chatCmdError, partial results, and exhausted retries now fail explicitly.receivedContactRequestwith/_accept <contactReqId>while preserving Hermes pairing. Contact acceptance does not authorize Hermes access.contactId; use stable namespaced IDs for group members; retain display names only as labels.SIMPLEX_FILES_FOLDERcontracts.No new runtime dependency or SimpleX identity database migration is introduced. Existing identities and messages are not rewritten.
This consolidates and preserves authorship from focused SimpleX fixes in #26480, #27628, #35558, and the edit-supersession work in #97317. I also reviewed the other open SimpleX PRs before submission; this PR supplies the integrated lifecycle and end-to-end test surface rather than silently duplicating them.
How to Test
scripts/run_tests.sh.uv run pytest -q tests/gateway/test_simplex_plugin.py.uv run pytest -q tests/gateway/test_edit_supersede.py tests/gateway/test_session_race_guard.py tests/gateway/test_unauthorized_dm_behavior.py.git diff --check upstream/main...HEAD.Current-upstream results on Linux x86_64, Python 3.11:
All SimpleX tests passed. The 20 full-suite failures are outside the diff and confined to ten files: environment-sensitive AF_UNIX path length, locally available credential/provider state, model picker expectations, and updater tests detecting an already-running gateway. They are disclosed here because the PR-template statement that every repository test passes is not true in this local environment.
Disposable two-identity runtime staging additionally proved contact acceptance and pairing; inbound/outbound messaging; reaction approval; streaming finalization; exact 13,000-character Unicode reassembly over four items; standalone outbound media; inbound image completion and cleanup; explicit outage failure; reconnect; gateway restart; and preserved state.
The reviewed pre-rebase implementation received an independent adversarial PASS after its material findings were fixed. The final branch was then cleanly replayed without conflicts onto current upstream
375ce8eee51b9d76714cb6fd1f200c4c9ef83c4aand the focused/current repository gates above were rerun.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — canonical suite ran; 20 unrelated/environment failures are disclosed aboveDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — N/A; plugin configuration remains environment-basedCONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A; the existing adapter architecture is retainedpathlib; WebSocket and command behavior remain platform-neutralScreenshots / Logs
Not applicable. This is a headless messaging adapter; test and runtime evidence is summarized above.
ws://remains appropriate only on loopback. Remote daemon access requires a separately authenticated transport. Unknown direct contacts remain pairing-gated, and groups remain allowlist-gated.