fix: deliver subscription notifications to the legacy session - #89
Merged
Conversation
resources/subscribe acked and recorded interest that nothing ever delivered
on, and subscribe! callbacks were never fired. notify_resource_updated and
notify_list_changed now reach the connected legacy session alongside modern
subscriptions/listen streams:
- start! exposes the loop's session state as server.legacy_state, giving the
exported notify_* helpers (which receive only `server`) access to the
session's wire_subscriptions
- notify_resource_updated delivers notifications/resources/updated to the
session when it subscribed to the URI, and invokes subscribe! callbacks as
callback(uri) with errors logged and swallowed
- notify_list_changed delivers notifications/{kind}/list_changed to the
session when the matching listChanged capability is declared (listen
streams stay gated per-stream at ack time)
- the subscribe/unsubscribe handlers update wire_subscriptions copy-on-write
so off-loop announcers read a consistent snapshot without locking
- return value now counts every client reached (streams + legacy session)
Docs (resources.md, api_overview.md) updated to the delivered behavior;
CHANGELOG Fixed entry under 0.7.0. Suite 1986/1986 (21 new tests: wire
delivery, unsubscribe, capability gating, callback containment, combined
modern+legacy counting).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…snapshotting, callback lock, handshake gate All 3 BLOCK / 4 WARN / 1 NIT from the PR #89 review: - legacy_session_notification now takes the state+transport snapshot from its caller (one fetch per announcement — a concurrent restart cannot authorize against one session and deliver through another) and bypasses the ambient request route around the send (restored after), so an announcement made inside a modern request handler can never leak the legacy notification onto that request's response stream - legacy delivery requires a real handshake: initialized AND a negotiated protocol_version (a bare notifications/initialized never arms it) - run loop retires server.legacy_state in its finally on every exit — notify_* after shutdown sees no session instead of a stale one - subscribe!/unsubscribe! and the notify-time callback snapshot share a new server.subscriptions_lock; callbacks run from the snapshot after release, so a callback may subscribe!/unsubscribe! (itself included) safely - list_changed_declared uses findlast — duplicate capability declarations gate on what initialize actually advertised (last-wins serialization) - notify_* docstrings state enqueue semantics (not client receipt); export comment and conformance fixture wording updated from "streams" to targets 18 new hardening tests (wire envelope, no-handshake gating, reentrant callbacks, duplicate-capability last-wins, route bypass+restore, loop-exit retirement). Suite 2004/2004. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…andoff-attempt count semantics, real HTTP route-bypass test - the run loop's finally retires server.legacy_state only when it still owns it (=== the generation it installed) — an overlapping restart's fresh state is no longer erased by the old loop's teardown (regression Codex reproduced with two blocking transports) - notify_*/legacy_session_notification docstrings and CHANGELOG state the count as attempted transport handoffs — not enqueue (a disconnected HTTP transport or a queue at its soft cap drops silently), not receipt - the route-bypass test now exercises a REAL HttpTransport response route: registered ambient channel receives zero messages, the out-of-band GET notification queue receives exactly the one legacy notification, and the ambient route is restored after (the stdio variant kept for restore-only coverage) Suite 2010/2010. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
The docs audit behind #88 surfaced a functional gap:
resources/subscribeacked and recorded the client's interest, but nothing ever delivered on it, andsubscribe!callbacks were never fired. This PR wires actual legacy delivery so 0.7.0 never ships with an advertised capability that silently does nothing.start!exposes the loop's session state asserver.legacy_state— the architectural root cause was that the exportednotify_*helpers (which receive onlyserver) had no path to the session'swire_subscriptionsnotify_resource_updatednow deliversnotifications/resources/updatedto the legacy session when it subscribed to the URI (stdout on stdio, GET SSE stream on Streamable HTTP), and invokessubscribe!callbacks ascallback(uri)with errors logged and swallowednotify_list_changeddeliversnotifications/{kind}/list_changedto the legacy session when the matchinglistChangedcapability is declared (listen streams stay gated per-stream at ack time)resources/subscribe/unsubscribehandlers updatewire_subscriptionscopy-on-write, so off-loop announcers read a consistent snapshot without lockingDocs (
resources.md,api_overview.md) updated to the delivered behavior; CHANGELOGFixedentry under 0.7.0.Gates
subscriptions/listensuite unchanged: 192/192🤖 Generated with Claude Code