Skip to content

feat: implement CEP-47 server redirect client/server middleware and re-issuance - #78

Open
abhayguptas wants to merge 16 commits into
ContextVM:masterfrom
abhayguptas:feat/cep-47-redirect
Open

feat: implement CEP-47 server redirect client/server middleware and re-issuance#78
abhayguptas wants to merge 16 commits into
ContextVM:masterfrom
abhayguptas:feat/cep-47-redirect

Conversation

@abhayguptas

Copy link
Copy Markdown
Contributor

Description

This PR implements CEP-47 Server Redirect support for @contextvm/sdk, adding full end-to-end middleware and re-issuance capabilities. It faithfully follows the mentor's callback-driven middleware design without mutating the stateless initialization lifecycle, and allows the client to transparently handle complex chained redirects with a safety hop cap.

Features Included

  1. Server-Side Redirect (withServerRedirect):
    • A middleware that accepts a resolveRedirect callback.
    • It is decoupled from state configuration (e.g., stateless mode). It evaluates the incoming JSON-RPC request and if a redirect is needed, emits the standard -32044 MCP error payload with the target pubkey and relays.
  2. Client-Side Redirect (withClientRedirect):
    • Intercepts incoming messages looking for the -32044 error.
    • Transparently handles the redirect by spinning up a new NostrClientTransport to the target server.
    • Properly routes subsequent messages and replaces the old transport for a seamless caller experience.
    • Includes cycle/loop detection with a customizable maxRedirects option (defaulting to 5 hops).
  3. Bug Fixes:
    • Fixed a bug in ApplesauceRelayPool message handling. applesauce-relay emits raw events rather than typed wrapper objects in the req() subscription. Fixed the message handler so that subscriptions correctly fire on events.
    • Fixed a double prepending issue in mock-relay-server connection mapping which caused test relay timeouts.
  4. Integration & Proxy:
    • withClientRedirect is cleanly hooked up into NostrMCPProxy alongside payments.
    • Lint rules pass and TypeScript is fully typed.

Testing

  • End-to-end single redirect (Client -> Server A -> Server B -> responds)
  • Chained redirects (Client -> Server A -> Server B -> Server C -> responds)
  • Infinite Loop detection (Client -> A -> B -> A -> Error thrown via McpError -32044)

All unit tests and E2E integration tests are passing successfully.

Changeset

Included a changeset for the upcoming release.

@abhayguptas
abhayguptas force-pushed the feat/cep-47-redirect branch from 8453650 to fc04859 Compare July 29, 2026 11:34
…leaks

- Remove resubscribeAll() to prevent racing with applesauce-relay's native {resubscribe: Infinity}
- Add isDisconnected flag to prevent zombie pool generations after disconnect()
- Prevent liveness timeouts from incorrectly rebuilding an already-rebuilt pool
- Ensure ping monitor is stopped and not restarted when pool is disconnected
- Add small sleeps in E2E tests to stabilize mock relay restarts
@ContextVM-org

Copy link
Copy Markdown
Contributor

The redirect module itself is well-designed and faithfully mirrors the CEP-8 payments pattern. Do not merge as-is — there is one blocker causing the 3 test failures, plus a few spec gaps worth addressing. Details and verified fixes below.


🔴 Blocker — root cause of all 3 test failures

File: src/relay/applesauce-relay-pool.ts, createSubscription() (lines ~372–404)

The rewritten message handler checks 'id' in msgObj before type === 'EVENT' (line 401 vs 403). This misroutes every EVENT coming from relayGroup.req().

I verified at runtime what applesauce-relay@6.2.1's relayGroup.req() actually emits (the PR description claims the opposite):

{type:"OPEN",  id:"<sub-id>", filters, from}      ← wrapper has `id`
{type:"EVENT", id:"<sub-id>", event:{...}, from}  ← wrapper has `id` (subscription id, NOT event id)
{type:"EOSE",  id:"<sub-id>", from}

Because every wrapper carries an id, the 'id' in msgObj branch (intended for raw NostrEvents) catches the EVENT wrapper first and passes the wrapper to onEvent() instead of onEvent(msg.event). The else if (msgObj.type === 'EVENT' …) branch on line 403 is dead code for the req() path.

Knock-on effect: fetchServerRelayList pushes wrappers into discoveredEvents, selectOperationalRelayUrls reads .url/.marker off them (both undefined) → empty discovery → "No operational relays discovered" → falls back to the discovery relay itself. That is exactly the three diffs (["ws://127.0.0.1:…"] instead of the kind-10002 relays).

Note: master's version of this method was correct — it dispatched on message.type. The commits 1eb2745/fc04859 ("resilient type fallback…", "correctly handle array message shape…") rewrote it while chasing flaky E2E timing and broke the dispatch. The changeset line "Also fixes a bug in ApplesauceRelayPool message handling" should be dropped — it introduced the regressions.

Fix — discriminate on type first (typed wrappers always have type; raw NostrEvents never do). Smallest correct patch:

next: (message: unknown) => {
  // req() emits typed wrappers {type:'EOSE'|'EVENT'|...}; raw NostrEvents
  // from subscription() have no `type`. Check `type` FIRST so the EVENT
  // wrapper's own `id` (subscription id) isn't mistaken for an event id.
  if (typeof message === 'object' && message !== null && 'type' in message) {
    const msg = message as { type: string; event?: NostrEvent };
    if (msg.type === 'EOSE') onEose?.();
    else if (msg.type === 'EVENT' && msg.event) onEvent(msg.event);
    return;
  }
  if (Array.isArray(message)) {
    if (message[0] === 'EOSE') onEose?.();
    else if (message[0] === 'EVENT' && message[2]) onEvent(message[2] as NostrEvent);
    return;
  }
  if (message === 'EOSE') onEose?.();
  else if (typeof message === 'object' && message !== null && 'id' in message)
    onEvent(message as NostrEvent);
},

I applied this locally → 505 tests, 500 pass, 0 fail (was 497/3), typecheck clean, no regressions in the relay-pool race-condition tests. Alternatively, reverting createSubscription to master's req(filters, {reconnect, resubscribe}) for both paths also works — but the split is fine once the dispatch order is fixed.


🟡 Spec gaps vs CEP-47

1. No CEP-17 fallback when provided relays are unreachable

client-redirect.ts performTransition pins relayHandler = new ApplesauceRelayPool(relays) when relays is provided. resolveOperationalRelays then early-returns on configuredRelayUrls.length > 0, so if target is unreachable on those relays there is no CEP-17 fallback. Spec: "If relays is provided and target is not reachable on those relays, the client SHOULD fall back to CEP-17 (kind 10002) discovery."

(Note: the relays-absent case is fine — NostrClientTransport defaults discoveryRelayUrls to DEFAULT_BOOTSTRAP_RELAY_URLS, so CEP-17 discovery for target works. My initial concern about NostrMCPProxy stripping discoveryRelayUrls/fallbackOperationalRelayUrls turned out not to break discovery.)

2. CEP-41 active streams are not handled on either side

  • Server: createRedirectMiddleware has no awareness of open streams — and InboundMiddlewareCtx doesn't expose stream state, so it can't check today. Spec: "A server MUST NOT emit a redirect for a request that has an active CEP-41 open-ended stream."
  • Client: performTransition closes the old transport without surfacing a stream failure to the caller. Spec: client SHOULD release local stream state and surface the failure, then follow the redirect.

Acceptable to defer, but please add a ponytail:/TODO and track it — streams + redirect is a real interoperability hole.

3. Self-redirect short-circuit deviates from the spec

performTransition line 164: if (currentServerPubkey === target) return;. CEP-47 explicitly says clients should follow redirect directives uniformly and not special-case target === current server. It's still safe here because the hop counter increments in handleInbound before this call, so the cap still bounds it — but it's a documented deviation from the spec's letter. Either remove the short-circuit or add a comment noting the deviation and why it's safe.


🟠 Robustness / minor

  • redirectCounts is an unbounded Map (client-redirect.ts:74). rawRequestCache is a bounded LruCache(1000), but redirectCounts is only cleared on a terminal non-redirect response. Requests stranded by a transport swap leak entries. Bound it or clear it on transition.
  • In-flight pending requests are not re-issued after a swap. Only origReq is resent (client-redirect.ts:307); everything else pending on the old transport is dropped on close(). This matches the spec's "subsequent requests" framing and MCP's serialized initialize, but it's surprising — worth a comment so it's not read as a bug later.
  • this.relayStates (applesauce-relay-pool.ts:96,150-156) is write-mostly: it only dedups the "Relay came online" log line. The field name implies load-bearing state; either use it for something or inline the dedup intent.

🔵 Test debt / diff noise

  • src/transport/nostr-transport-reconnection.test.ts: 4× await sleep(1000) added after relay restarts. This masks timing flakiness rather than fixing the underlying race. If rebuild races restart, synchronize on the rebuild event.
  • src/relay/applesauce-relay-pool.test.ts: the rejection setTimeouts on the subscription promises were removed and replaced with sleep(100) before publish. Those timeouts were catching real hangs — removing them turns a hang into an infinite test wait. Keep a timeout, just make it generous.
  • src/__mocks__/mock-relay-server.ts: the matchFilters → temp-variable change is a no-op refactor; unrelated to CEP-47, consider dropping to keep the diff focused.

✅ What's good

  • Middleware shape (createRedirectMiddleware, withServerRedirect, withClientRedirect) cleanly mirrors CEP-8 and the ResolvePriceFn callback model.
  • Hop cap is correctly scoped per original request id and cleared on terminal response — matches the spec exactly.
  • Loop detection surfaces the final -32044 to the caller rather than silently looping.
  • Gateway wiring order is correct: withServerRedirect registers before withServerPayments on the same transport, so redirected requests short-circuit before payment gating.
  • wrapTransport composition with withClientPayments works end-to-end (payments wrapper exposes onmessageWithContext, so the redirect's context path is preserved across hops).
  • E2E coverage (single hop, chain A→B→C, loop A↔B) exercises the core flows.

Suggested merge sequence

  1. Apply the blocker fix above (required — suite is red without it).
  2. Drop/rewrite the changeset line about the relay-pool "bug fix".
  3. Address gap feat: add nostr proxy server with bidirectional message routing #1 (CEP-17 fallback when provided relays unreachable) — highest priority of the spec gaps.
  4. File gaps Feat/gateway2 #2/refactor: better subs, security, announcements #3 and the test-debt items as follow-ups.

Want me to also draft the precise fix for gap #1, or leave that for the contributor?

…47 spec updates

- Fix createSubscription message handler to check 'type' in wrapper before checking 'id' property on raw NostrEvent
- Fall back to CEP-17 (kind 10002) discovery when provided configuredRelayUrls are unreachable
- Convert redirectCounts Map to bounded LruCache(1000)
- Add CEP-41 stream and spec deviation comments
@ContextVM-org

Copy link
Copy Markdown
Contributor

looks good, two cheap tests away from merge

✅ Fixes landed correctly

  • Relay dispatch blocker — fixed right (type discriminator first). Full suite 500/500, the 3 previously-failing tests pass, typecheck clean.
  • redirectCounts → bounded LruCache(1000) — all call sites consistently keyed by String(reqId).
  • CEP-41 + self-redirect-deviation — TODO/comment annotations added on both sides.

🔴 Two tests I'd ask for before merge

Both are cheap and directly back claims in the PR description that are currently unexercised:

  1. Proxy/Gateway integration. Every E2E test wires withServerRedirect/withClientRedirect directly and bypasses NostrMCPProxy/NostrMCPGateway. So the redirectOptions/redirectConfig options — the ones real consumers pass — have zero coverage. There's a proxy-payments.test.ts/gateway-payments.test.ts analogue but no redirect equivalent. One test per side through the high-level API would close this.

  2. Redirect × payments composition. The E2E tests use wrapTransport: (t) => t (no-op) and never enable paymentOptions. The composition the PR advertises ("cleanly hooked up alongside payments") — withClientRedirectwithClientPayments on the client and the withServerRedirect-before-withServerPayments ordering on the server — is never exercised end-to-end. Two transport/middleware layers composing is exactly where things silently break.

🟡 Minor / follow-ups (non-blocking)

  • CEP-17 fallback when relays unreachable (relay-resolution.ts): the new branch reuses connectFallbackOperationalRelays, whose ApplesauceRelayPool.connect() I verified is a reachability no-op (resolves in 0ms for a dead port). So the "Configured operational relays are unreachable; falling back to CEP-17" warning can never fire for valid URLs. This is pre-existing — master already used the same no-op helper for the fallback-relay path — so it's out of scope here. Suggest either reverting just this hunk to avoid shipping dead code, or tracking a follow-up to probe via Relay.connected$/status$ with a timeout (fixes both the master bug and the CEP-47 SHOULD at once).
  • requestEventId (the new InboundMiddlewareCtx field) is never asserted — server tests use a dummyCtx without it and the sendResponse mock drops the 3rd arg.
  • Changeset still says "Also fixes a bug in ApplesauceRelayPool message handling" — that "fix" introduced the regressions; consider dropping the line.

Verdict

Functionally mergeable today. Adding the two integration tests above would de-risk the headline claims; everything else is fine as tracked follow-ups.

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.

2 participants