feat(node-message-broker): resolve analysis participants via server-core#12
Merged
Conversation
Slice S2 of the node message-broker build-out (Plan 013 Track B): resolve which node clients participate in an analysis (and their public keys) from server-core, so the send/deliver flows can address and encrypt to peers. - core/analysis: IAnalysisNodeProvider port + CoreNode; AnalysisParticipant now carries the node's ECDH publicKey. - adapters/core: ParticipantResolver maps analysis-node entries to participants, skips ones missing a client id / public key, and caches per analysis behind a short-TTL LRU (off the Hub hot path); resolveSelf matches this node's client id. - app/modules/core-client: CoreClientModule wires a @privateaim/core-http-kit client (node-client credentials, mirroring the Hub link) behind the provider port and registers the resolver. Adds the core-http-kit dependency. - The resolver depends on the narrow IAnalysisNodeProvider (not the whole core client) so it stays unit-testable with a fake; the rapiq getMany query lives at the wiring site. Closes #5
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR extends analysis participant types, adds a cached participant resolver, introduces a core-client module that wires the HTTP client into the container, and updates application bootstrap and tests. ChangesServer-core participant resolution and wiring
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ParticipantResolver
participant IAnalysisNodeProvider
participant CoreHttpKitClient
Caller->>ParticipantResolver: resolve(analysisId)
alt cache hit
ParticipantResolver-->>Caller: cached AnalysisParticipant[]
else cache miss
ParticipantResolver->>IAnalysisNodeProvider: list(analysisId)
IAnalysisNodeProvider->>CoreHttpKitClient: analysisNode.getMany({ analysis_id })
CoreHttpKitClient-->>IAnalysisNodeProvider: CoreNode[]
IAnalysisNodeProvider-->>ParticipantResolver: CoreNode[]
ParticipantResolver-->>Caller: AnalysisParticipant[]
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/node-message-broker/src/app/modules/core-client/module.ts (1)
27-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVerify module ordering guarantees the logger is set up before
coreClient.
dependenciesonly declares'config', yetsetupalso readsLoggerInjectionKey. ThetryResolvemakes the logger optional, so this won't crash, but if orkos reorders modules by declared dependencies (rather than builder insertion order),coreClientcould initialize before the logger and silently lose participant-skip warnings. Confirm ordering or add'logger'todependenciesif those warnings matter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/node-message-broker/src/app/modules/core-client/module.ts` around lines 27 - 45, The CoreClientModule setup reads LoggerInjectionKey but only declares config as a dependency, so module ordering may allow coreClient to initialize before the logger and drop warnings. Update CoreClientModule.dependencies to include logger if those participant-skip warnings must always be available, or otherwise ensure the module ordering guarantees logger is registered before CoreClientModule.setup runs; keep the logger lookup via tryResolve in sync with that ordering choice.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/node-message-broker/src/app/modules/core-client/module.ts`:
- Around line 56-64: The provider.list implementation in module.ts currently
returns entry.node directly, which can let undefined nodes flow into
ParticipantResolver.fetch and crash on node.client_id. Update the mapping in
provider.list to guard at the boundary by filtering out any entries without a
node before returning the list, so only valid node objects are passed
downstream.
---
Nitpick comments:
In `@apps/node-message-broker/src/app/modules/core-client/module.ts`:
- Around line 27-45: The CoreClientModule setup reads LoggerInjectionKey but
only declares config as a dependency, so module ordering may allow coreClient to
initialize before the logger and drop warnings. Update
CoreClientModule.dependencies to include logger if those participant-skip
warnings must always be available, or otherwise ensure the module ordering
guarantees logger is registered before CoreClientModule.setup runs; keep the
logger lookup via tryResolve in sync with that ordering choice.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56f90383-c967-4c40-8bbb-8ed501285a8e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
apps/node-message-broker/package.jsonapps/node-message-broker/src/adapters/core/index.tsapps/node-message-broker/src/adapters/core/participant-resolver.tsapps/node-message-broker/src/app/builder.tsapps/node-message-broker/src/app/factory.tsapps/node-message-broker/src/app/modules/core-client/constants.tsapps/node-message-broker/src/app/modules/core-client/index.tsapps/node-message-broker/src/app/modules/core-client/module.tsapps/node-message-broker/src/core/analysis/types.tsapps/node-message-broker/test/unit/adapters/core/fake-analysis-node-provider.tsapps/node-message-broker/test/unit/adapters/core/participant-resolver.spec.ts
Contributor
Author
|
@coderabbitai pause |
✅ Action performedReviews paused. |
…elation server-core types AnalysisNode.node as non-null, but may omit the relation when `include` is not honored or data is incomplete; filter those out at the provider boundary so ParticipantResolver never dereferences an undefined node.
This was referenced Jun 25, 2026
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.
Slice S2 of the node message-broker build-out (Plan 013 Track B). Resolves which node clients participate in an analysis — and their ECDH public keys — from server-core, so the send (S5) and inbound-delivery (S6) flows can address and encrypt to peers.
What's here
core/analysis—IAnalysisNodeProviderport (list(analysisId) → CoreNode[]) +CoreNode.AnalysisParticipantnow carries the node'spublicKey(consumed by S1'sCryptoService).adapters/core—ParticipantResolver: maps server-core analysis-node entries →AnalysisParticipant, skips any participant missing aclient_idorpublic_key(can't be addressed/encrypted to), and caches per analysis behind a short-TTL LRU to keep resolution off the Hub hot path.resolveSelfreturns the entry matching this node's own client id.app/modules/core-client—CoreClientModulewires a@privateaim/core-http-kitclient (node-client credentials, mirroring the Hub link) behind the provider port and registers the resolver. Adds the@privateaim/core-http-kitdependency.Design note
The resolver depends on the narrow
IAnalysisNodeProvider, not the whole core client. The real client'sanalysisNode.getManysignature uses rapiq'sBuildInputgenerics, which don't mimic cleanly as a structural fake-able interface — so the rapiq query (getMany({ filter: { analysis_id }, include: ['node'] })) lives at the module wiring site, and the resolver's logic stays unit-testable with a trivial fake provider. This also keeps rapiq's query shapes out of the domain.Tests (8 resolver specs; 41 project-wide, all green)
Field mapping; skipping incomplete participants;
resolveSelfhit/miss; unknown analysis → empty; per-analysis cache hit; failed resolution not cached; caching disabled.Scope boundaries
Closes #5
Summary by CodeRabbit
New Features
Bug Fixes