feat: handle didcomm msg keylist-update-response from mediator

[TheSeydiCharyyev]

Description

Fixes #391 — the SDK sent MediationKeysUpdateList to the mediator but never processed the keylist-update-response that the mediator sends back (per the coordinate-mediation 2.0 spec). Per-recipient failures reported by the mediator went unnoticed by the SDK.

Changes

• New protocol ID ProtocolIds.MediationKeysUpdateResponse + user-facing ProtocolType.DidcommMediationKeysUpdateResponse
• New handler MediationKeysUpdateResponse under plugins/internal/didcomm/connection/ — iterates body.updated and logs a warning for each entry where result is not success or no_change, including recipient_did, action, and result
• Registered the handler in didcomm/plugin.ts so the existing DIDCommConnection.receive() → RunProtocol dispatch picks it up
• Removed the [ ] handle response TODO comment in CreatePeerDID.ts — caller signature is unchanged (Promise<void>)

Verification

• Verified the mediator actually sends this response by reading MediatorCoordinationExecuter.scala — the case m: KeylistUpdate branch returns m.makeKeylistResponse(updateResponse).toPlaintextMessage with (recipient_did, action, result) tuples where result is one of success | no_change | server_error
• Covered by 5 new unit tests in tests/agent/didcomm/MediationKeysUpdateResponse.test.ts (all-success, server_error, client_error, mixed results, missing updated field)
• Full SDK test suite: 706/706 passing on this branch

Alternatives Considered

• Throw on server_error/client_error to propagate failure to the caller — would require changing DIDCommConnection.receive(), which currently swallows handler exceptions via catch { return undefined }. That affects every other handler (MediateGrant, PickupDelivery, etc.) and is out of scope for this bug fix — worth a follow-up discussion.
• Parse the response inside updateKeyListWithDID directly, bypassing the dispatch — would break the handler-registration pattern used consistently across the didcomm plugin.

Chose to mirror MediateGrant / MediateDeny / ProblemReport: handlers perform side effects (log / store / emit) and do not propagate errors up the stack. Same ergonomics for reviewers, zero behavioural change to existing flows.

Checklist

• My PR follows the contribution guidelines of this project
• My PR is free of third-party dependencies that don't comply with the Allowlist
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation (N/A — internal plugin behavior)
• I have added tests that prove my fix is effective
• I have checked the PR title to follow the conventional commit specification

[elribonazo]

I would try doing the following.

If updateMediator is activated, we will send the KeyListUpdateMessage and then wait for the reply.
If we detect an error in the task, the createPeerDID function should fail.

The way the task has been wired for this purpose is strange.
I propose to remove

  .register(ProtocolIds.MediationKeysUpdateResponse, MediationKeysUpdateResponse)

We don't want this task to trigger automatically when this message arrives, instead I will propose to run this task after

    await ctx.run(new Send({ message: keyListUpdateMessage }));

The task will not resolve until the response for that keyListUpdate is received, if the response is successful, then we resolve, or else, we throw an exception with the correct error message

Proper testing needs to be implemented as part of this task....

[elribonazo]

This is not proving much, after the changes we requested i think additional tests should be implemented, specially inside the createPeerDID function

[FabioPinheiro]

Yeah this would be a really nice to have.
This is more a new feature (in terms of Semantic Commits) so I change the title.

[coveralls]

coverage: 75.215% (+0.05%) from 75.161% — TheSeydiCharyyev:fix/mediation-keylist-update-response-handler into hyperledger-identus:main

[TheSeydiCharyyev]

@elribonazo quick architectural question before pushing the revisions — Send already returns the response message. Leaning toward using its return value (then running MediationKeysUpdateResponse explicitly) to keep the change minimal. Happy to switch to Mercury.sendMessageParseMessage if you'd prefer the lower-level path.

[FabioPinheiro]

I'm not sure how deep the test needs.

I believe @elribonazo is concerned about the life cycle. Like in createPeerDID there is logic to update the DIDs register in the mediator.

The first DID created register in the mediator (as a new account). But all the following DIDs that are created, calls that updatekey to add the new DID as a alias of the first one.
It would be nice to have tests for that but you also need to mock the interactions with the mediator

[TheSeydiCharyyev]

updateKeyListWithDID now awaits the response from Send, runs it through MediationKeysUpdateResponse (which throws on a non-success result or a malformed body), and bails out after 60s if nothing comes back.

MediationKeysUpdateResponse is no longer auto-registered as a message handler — it is invoked explicitly from updateKeyListWithDID after Send, so the lifecycle is deterministic.

The old unit test is replaced by tests/agent/CreatePeerDID.test.ts (9 scenarios — success, no_change, client_error, server_error, malformed body, timeout, Send-returns-undefined, updateMediator = false, no mediator connected). Tests exercise the flow through createPeerDID rather than the handler directly.

Branch is rebased on current main. 784/784 SDK tests pass locally.

[elribonazo]

This doesn't look correct, have u manually tested to debug ?¿

Sending a message to a mediator with the updateKeyList message is one task, good!

Next thing is waiting until we receive another message with the mediation response, currently, you are expecting the mediator to reply with that message as response. But the mediator can respond asynchronously, so this code is not fully correct.

I encourage you to test things manually before submitting a PR like this one, testing locally will probably help you understand what i mean

[TheSeydiCharyyev]

I had it wrong — assumed Send returns the response directly, didn't account for the mediator replying in a separate message later.

Got the local identus-docker mediator running, and mediator2.trust0.id is also responding. Going to send a real keylist-update through it and watch what comes back. Then I'll change updateKeyListWithDID to wait for the inbound response by thid, with the 60s timeout on that wait instead of on Send.

I'll push the new version after testing it against the real mediator.

[elribonazo]

I had it wrong — assumed Send returns the response directly, didn't account for the mediator replying in a separate message later.

Got the local identus-docker mediator running, and mediator2.trust0.id is also responding. Going to send a real keylist-update through it and watch what comes back. Then I'll change updateKeyListWithDID to wait for the inbound response by thid, with the 60s timeout on that wait instead of on Send.

I'll push the new version after testing it against the real mediator.

Sounds like a plan, you can use my mediator feel free, ping me if u have any issues.
And yeah, the mediator will respond you or build another message, if done correctly what should happen is:

1. You send the mediationKeyList update ( with the thread id defined in the didcomm message)
2. Mediator will then reply, hopefully with the same thread id
3. Once u receive the reply, u know that's a response by checking other message with the same thread

So basically the thread if what connects everything, same with credential issuance for example, or presentation flows

[TheSeydiCharyyev]

Found the root cause — it wasn't about strict vs lenient timeout, it was a missing header.

The mediator returns keylist-update-response inline in the same HTTP reply, but only when the request carries return_route: "all". Mercury auto-attaches that header for the piuris in ReturnRouteProtocols (packages/wasm/didcomm/src/Wrapper.ts), and keylist-update was never in that list. So every keylist-update went out without return_route, the mediator dispatched the response asynchronously to the mediated DID, and the client never observed it. That explains why Send.run always resolved to undefined and the validation in MediationKeysUpdateResponse never ran — the response wasn't reaching the client at all.

For reference:

• hyperledger-identus/mediator AgentExecutorMediator.scala splits on plaintextMessage.return_route — sync via transport.send for all/thread, async via TransportDispatcher otherwise. Behaviour was introduced in commit a306d59 ("reply asynchronous unless return_route all", test: refactor and add new scenarios #86).
• sdk-swift sets return_route: "all" on MediationKeysUpdateList explicitly. sdk-kmp and sdk-ts were the outliers.

Verified against mediator2.trust0.id: with keylist-update added to ReturnRouteProtocols, the mediator returns the response inline in ~1.8 s, thid matching the outgoing message id, body success per recipient.

Changes in the redesign:

• Wrapper.ts: add keylist-update to ReturnRouteProtocols.
• DIDCommConnection.send: return the parsed inline response instead of the registered handler's result, so Send.run surfaces the response message for callers to validate.
• updateKeyListWithDID: send, race against a 60 s timeout, assert the response is a Message with the expected piuri and a thid matching the outgoing id, validate via MediationKeysUpdateResponse (throws on any non-success / non-no_change result).
• Tests cover success, no_change, client_error, server_error, malformed body, timeout, wrong piuri, wrong thid, missing response.
• A Mercury.test.ts guard asserts keylist-update stays in ReturnRouteProtocols so the same regression doesn't recur (PR fix(mercury): Add return route with value all to async messages that are initiated by the holder. #85 narrowed the list and missed this entry).

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[elribonazo]

@TheSeydiCharyyev will it depend on what the mediator has configured?

Can we try the following approach?

A) contains the response and we are ready to validate
B) if response is empty we should wait for the update message manually

https://didcomm.org/coordinate-mediation/2.0/

Nothing says there if it should be response of the request message or another message linked to the update key list request.

I would encourage us to implement both, if we get it in response, we continue, but if we don't, we must have a way to know when the new diddcomm message appears and validate it by its thid, find the request and complete.

All this should probably happen inside the agent

[TheSeydiCharyyev]

Understand A and B ideas, but I am not sure how to do it in the inside of agent. You meant a method on Agent or separate task?

[elribonazo]

You can do it inside the current task.

What I would try to do:

1. Create a message listener each time we create a DID, agent.listen or something like that..
2. Send the updateKeyListUpdate
   2.1 If the response comes directly after in sending, and parse the message.
   2.1 if the response is null or does not exist, we wait for a period of time to see if we receive a message
   2.2 The response keylist message should have as thid, the same thid u sent, or your peerDID, if the updateKeyList matches, resolve the createPeerDID function, if timeout lasts, then exception

[TheSeydiCharyyev]

Pushed the dual-path. updateKeyListWithDID now adds a MESSAGE listener before sending: (A) if Send returns a matching response we use it directly, (B) if not, we wait for the response on the MESSAGE event, with the same 60s timeout.

About "the same thid u sent, or your peerDID" — I made it match either one: the response is accepted if its thid equals the message id we sent, or the new peer DID. Tell me if you meant only one of them.

Path (B) is covered by unit tests — async success, response threaded on the peerDID, failure result, and non-matching thid timeout. I could not test it against a real mediator though: mediator2 answers inline once return_route is set, so (B) never runs there. If there is a mediator that replies async, I can check against it.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[abhigyan1102]

Went through the full conversation here the journey from auto-registered handler → explicit invocation → discovering the missing return_route header → dual-path (sync + async fallback) is really well documented. The root cause analysis comparing sdk-swift (which already sets return_route: "all") against sdk-ts was a nice catch.

One question on the dual-path: for path B (async fallback via MESSAGE listener), is there a risk of the listener leaking if the timeout fires but the message arrives right after cleanup? Or does the removeListener in the finally/timeout block cover that?

[TheSeydiCharyyev]

On the listener leak — removeListener in finally removes the callback from EventsManager before any message can call it. If a matching message arrives right after cleanup, emit just does not find the listener in the map, so no spurious validation, asyncResponse is not set. I think there is no listener leak here.

But while rechecking I found a different leak in the poll loop: the setTimeout chain is not cancelled on timeout. The last scheduled setTimeout(poll, 50) fires after the finally cleanup, asyncResponse never gets set (listener is removed), and poll schedules new setTimeouts forever. Each timed-out call leaves an endless poll closure in memory.

@elribonazo could you take a look at my proposal — store the setTimeout handle in outer scope and clearTimeout in finally.

[abhigyan1102]

Good catch on the poll timeout leak that's exactly the kind of edge case I was wondering about. Storing the handle and clearing in finally makes sense.