feat(openid4vc): refactor agent to use shared public listener - #577
Draft
rmlearney-digicatapult wants to merge 5 commits into
Draft
feat(openid4vc): refactor agent to use shared public listener#577rmlearney-digicatapult wants to merge 5 commits into
rmlearney-digicatapult wants to merge 5 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Refactors DIDComm HTTP and WebSocket traffic onto a shared public listener while retaining the private TSOA/event listener. Changes are required because startup readiness does not cover WebSocket upgrades or actual listener binding.
Changes:
- Moves DIDComm to
/didcommand/didcomm-wson port 5002. - Adds public/private listener separation and startup gating.
- Updates fixtures, deployment configuration, documentation, and tests.
Blockers (Must-Fix)
src/index.ts:97: WebSocket upgrades bypass the readiness gate.src/index.ts:176: Readiness is announced before listeners successfully bind.src/agent.ts:266: Configured WebSocket ports are silently ignored.
Targeted Suggestions
- Check readiness inside the HTTP upgrade handler.
- Await listener
listeningevents and reject startup onerror. - Validate or remove the obsolete WebSocket port configuration.
Test Gap Analysis
- Add a test rejecting
/didcomm-wsbefore readiness. - Make the unknown-path test fail if the socket opens.
- Cover mismatched HTTP/WS port configuration.
Integration Risks
- Existing deployments using port 5003 will lose WebSocket connectivity.
- README and Compose configuration still expose or document obsolete WS ports.
- Rollback requires restoring separate HTTP and WS transport listeners.
Scores
| Category | Score | Reason |
|---|---|---|
| Correctness | 6/10 | Readiness has HTTP/WS gaps. |
| Security | 7/10 | WS traffic bypasses startup gating. |
| Testing | 6/10 | One WS rejection test is ineffective. |
| Maintainability | 7/10 | Transport configuration is misleading. |
| Performance | 9/10 | No material regression identified. |
| Integration/Infra | 5/10 | Port documentation and exposure remain inconsistent. |
| Overall | 6.5/10 | Core approach is sound but requires readiness and configuration fixes. |
Release Notes Draft
- Added a shared public protocol listener.
- Moved DIDComm HTTP to
/didcomm. - Moved DIDComm WebSocket traffic to
/didcomm-wson the HTTP listener. - Kept administrative APIs and event WebSockets on the private listener.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/agent.ts |
Configures shared DIDComm transports. |
src/index.ts |
Creates public/private listeners and upgrade routing. |
src/env.ts |
Updates endpoint defaults. |
src/utils/readiness.ts |
Adds startup request gating. |
tests/unit/agent.test.ts |
Tests listener separation and routing. |
tests/unit/utils/helpers.ts |
Adds shared-listener test setup. |
tests/integration/assets/dids/alice/did.json |
Updates Alice’s DIDComm endpoint. |
tests/integration/assets/dids/bob/did.json |
Updates Bob’s DIDComm endpoint. |
tests/integration/assets/dids/charlie/did.json |
Updates Charlie’s DIDComm endpoint. |
docker-compose.yml |
Updates single-agent endpoints. |
docker-compose-testnet.yml |
Updates testnet endpoints. |
README.md |
Documents the new endpoint paths. |
Suppressed comments (1)
src/index.ts:177
- This opens the gate before listener startup has actually succeeded.
privateApp.listen(...)is asynchronous, andDidWebServer.start()also returns immediately after callinglisten(), so public DIDComm traffic can be accepted while the private API or enabled DID:web server is not yet bound; a subsequent bind error also occurs after readiness was announced. Await each server'slisteningevent (and reject onerror) before callingmarkReady().
// Public and private listeners, TSOA routes and DID:web are all up: only now accept protocol traffic.
readinessGate.markReady()
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/agent.ts:263
- A WS-only configuration now creates a
noServerWebSocket server, but the upgrade handler is installed only when an HTTP transport also exists. Because the current validation only checks ports when both entries are present,setupAgentcan succeed with an unreachable WS inbound transport. Reject WS without HTTP (or retain a standalone WS listener) so this valid former configuration does not fail silently.
const httpTransportConfig = inboundTransports.find((t) => t.transport === 'http')
const wsTransportConfig = inboundTransports.find((t) => t.transport === 'ws')
if (wsTransportConfig && httpTransportConfig && wsTransportConfig.port !== httpTransportConfig.port) {
src/index.ts:166
- The readiness contract is opened before all listeners are actually ready.
privateApp.listen()returns before itslisteningcallback, andDidWebServer.start()likewise returns immediately after callinglisten, so public DIDComm traffic can be accepted while the private or DID:web bind is still pending (or about to fail). Await each server'slisteningevent and reject onerrorbefore callingmarkReady().
// Public and private listeners, TSOA routes and DID:web are all up: only now accept protocol traffic.
readinessGate.markReady()
tests/unit/agent.test.ts:209
- This new suite starts
privateServerbut never closes it, unlike the existinggetTestServerusers. The open HTTP handle can keep Mocha alive and leaks a listener into subsequent tests.
after(async () => {
await agent.shutdown()
await deleteAgentStore(agent)
})
tests/unit/agent.test.ts:243
- This test closes immediately after the WebSocket handshake, so it does not prove that the newly wired
handleUpgradepath delivers DIDComm frames toDidCommWsInboundTransport. Add a multi-agent integration case that sends a DIDComm message through the advertised/didcomm-wsendpoint and verifies the receiving agent's event or record state.
test('accepts a WebSocket upgrade at /didcomm-ws', async () => {
const { port: boundPort } = httpServer.address() as AddressInfo
const outcome = await attemptWebSocketUpgrade(`ws://127.0.0.1:${boundPort}${DIDCOMM_WS_PATH}`)
expect(outcome).to.equal('open')
package.json:52
- The PR description says the OpenID4VC dependency is installed, but this dependency hunk removes
@credo-ts/openid4vc(and its lockfile entries), while the implementation says OpenID4VC is deferred to a later PR. Please either retain the dependency and intended OpenID4VC scope or update the PR description so the delivered scope and release review are accurate.
"@credo-ts/node": "^0.7.0",
"@hyperledger/anoncreds-nodejs": "^0.4.0",
rmlearney-digicatapult
marked this pull request as draft
August 21, 2026 19:41
Signed-off-by: rmlearney-digicatapult <robert.learney@digicatapult.org.uk>
rmlearney-digicatapult
force-pushed
the
feat/openid4vc
branch
from
August 22, 2026 18:31
97557f8 to
c71c549
Compare
rmlearney-digicatapult
force-pushed
the
feat/openid4vc-shared-listener
branch
from
August 22, 2026 18:32
e8f0775 to
90d0fb4
Compare
Signed-off-by: rmlearney-digicatapult <robert.learney@digicatapult.org.uk>
Signed-off-by: rmlearney-digicatapult <robert.learney@digicatapult.org.uk>
Signed-off-by: rmlearney-digicatapult <robert.learney@digicatapult.org.uk>
rmlearney-digicatapult
force-pushed
the
feat/openid4vc-shared-listener
branch
from
August 22, 2026 18:33
90d0fb4 to
b943525
Compare
Signed-off-by: rmlearney-digicatapult <robert.learney@digicatapult.org.uk>
rmlearney-digicatapult
force-pushed
the
feat/openid4vc
branch
from
August 24, 2026 11:38
c71c549 to
22b194c
Compare
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
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.
Pull Request
Checklist
PR Type
Please delete options that are irrelevant.
Linked tickets
https://digicatapult.atlassian.net/browse/ENG-346
High level description
Refactor agent startup code to present public (DIDComm HTTP & WS) and private endpoints (TSOA & WS events)
Detailed description
PR for
feat/openid4vcfeature branchfalse/didcommand/didcomm-ws503until Cloudagent listener is readyDescribe alternatives you've considered
Operational impact
Additional context