Chore/evault pitstop refactor #395
Conversation
|
Warning Rate limit exceeded@coodos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (23)
WalkthroughThread multi-tenant eName through DB, GraphQL, and access guard; add provisioning and verification services, tests, migrations, Docker dev compose/entrypoint, and test utilities; remove legacy bootstrap, SecretsStore, provisioner package, Kubernetes integration, and various legacy tests/utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQL as GraphQL Resolver
participant Context as VaultContext
participant Db as DbService
participant Neo4j
Client->>GraphQL: GraphQL request (with X-ENAME header)
GraphQL->>Context: extract eName from headers
Context-->>GraphQL: { eName }
GraphQL->>Db: call method(params, eName)
Db->>Neo4j: Cypher query scoped by m.eName = $eName
Neo4j-->>Db: tenant-scoped results
Db-->>GraphQL: return results
GraphQL-->>Client: response
sequenceDiagram
participant Client
participant ProvisionAPI as ProvisioningService
participant Registry as Public Registry
participant VerificationSvc
participant W3ID
Client->>ProvisionAPI: POST /provision (registryEntropy, namespace, verificationId, publicKey)
ProvisionAPI->>Registry: GET /.well-known/jwks.json
Registry-->>ProvisionAPI: JWKS
ProvisionAPI->>ProvisionAPI: verify registryEntropy JWT
ProvisionAPI->>W3ID: derive eName from entropy + namespace
alt verificationId != DEMO_CODE
ProvisionAPI->>VerificationSvc: findById(verificationId)
VerificationSvc-->>ProvisionAPI: verification (approved?)
ProvisionAPI->>VerificationSvc: update linkedEName (consume)
end
ProvisionAPI->>Registry: POST /register (ename, uri, evault)
Registry-->>ProvisionAPI: 201
ProvisionAPI-->>Client: { success, w3id, uri }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
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 |
infrastructure/evault-core/src/core/provisioning/config/database.ts
Outdated
Show resolved
Hide resolved
infrastructure/evault-core/src/core/provisioning/config/database.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
package.json (1)
1-50: Rename duplicate workspace to unblock CI.The workspace name "rest-express" is defined in both
platforms/dreamSync/package.jsonandplatforms/eReputation/package.json. Monorepo workspaces must have unique names. Rename one of these packages to resolve the lint check failure.infrastructure/evault-core/package.json (1)
1-52: Upgrade @testcontainers/postgresql from beta to stable version to resolve segmentation fault.The segmentation fault (exit 139) is caused by using @testcontainers/postgresql@^10.0.0-beta.6, which is an outdated beta version known to cause segfaults. Upgrade @testcontainers/postgresql to the latest stable release (currently 11.7.2).
Update
infrastructure/evault-core/package.json:
- Change
"@testcontainers/postgresql": "^10.0.0-beta.6"to"@testcontainers/postgresql": "^11.7.2"infrastructure/evault-core/src/core/db/db.service.ts (2)
469-480: Restrict envelope updates to tenant scope
MATCH (e:Envelope { id: $envelopeId })no longer enforces tenant isolation. A crafted request that knows another tenant’s envelope ID could update it because the Cypher query no longer checks the parentMetaEnvelope’seName. Please scope the update through the meta node (id + eName) so envelope writes remain tenant-bound.- MATCH (e:Envelope { id: $envelopeId }) - SET e.value = $newValue, e.valueType = $valueType + MATCH (m:MetaEnvelope { id: $metaId, eName: $eName })-[:LINKS_TO]->(e:Envelope { id: $envelopeId }) + SET e.value = $newValue, e.valueType = $valueType `, { + metaId: id, + eName, envelopeId: existingEnvelope.id, newValue: storedValue, valueType, }
537-543: Apply the same eName guard when deleting envelopesDeletion still does
MATCH (e:Envelope { id: $envelopeId }), so a leaked ID could delete another tenant’s envelope. The delete must also traverse the owningMetaEnvelopefiltered by{ id: $metaId, eName: $eName }to maintain the multi-tenant boundary.- MATCH (e:Envelope { id: $envelopeId }) - DETACH DELETE e + MATCH (m:MetaEnvelope { id: $metaId, eName: $eName })-[:LINKS_TO]->(e:Envelope { id: $envelopeId }) + DETACH DELETE e `, - { envelopeId: envelope.id } + { metaId: id, eName, envelopeId: envelope.id } );
🧹 Nitpick comments (10)
.gitignore (1)
47-48: Minor: Redundant PNPM store ignore patterns.Both
.pnpm-storeand.pnpm-store/are present. Git ignores directories matching.pnpm-storeregardless of the trailing slash, so one entry is sufficient. Consider keeping only.pnpm-storefor clarity.-.pnpm-store -.pnpm-store/ +.pnpm-storeinfrastructure/evault-core/README.md (2)
18-18: Fix markdown linting issues.The following linting issues should be addressed for consistency:
- Line 18: Wrap bare URL in angle brackets:
<https://orbstack.dev/>- Line 90: Specify language for fenced code block (should be blank line for HTTP method, or use appropriate language)
- Line 98: Specify language for fenced code block (should be blank line for HTTP method, or use appropriate language)
- Line 143: Wrap bare URL in angle brackets if it's a reference to a URL
Apply these diffs to fix the linting issues:
-1. Install OrbStack: https://orbstack.dev/ +1. Install OrbStack: <https://orbstack.dev/>-``` +```bash GET /health -``` +```-``` +```bash POST /provision -``` +```If line 143 contains a bare URL, wrap it similarly:
-`NOMAD_ADDR` - Nomad API address (default: http://localhost:4646) +`NOMAD_ADDR` - Nomad API address (default: `http://localhost:4646`)Also applies to: 90-90, 98-98, 143-143
1-10: Document multi-tenant architecture and eName parameter.According to the PR objectives, this refactor introduces multi-tenant eVault architecture and adds an
eNameparameter to metadata. However, the README focuses entirely on provisioning setup without documenting:
- How multi-tenancy is represented (e.g., one user per PORT mapping)
- The role of
eNamein provisioning or API workflows- How the stateless nature of eVault is achieved or maintained
- How persistent volume configuration is managed at the environment level
Consider adding an "Architecture & Multi-Tenancy" section that explains these design decisions and how they manifest in the provisioning API.
platforms/registry/src/services/UriResolutionService.ts (1)
1-16: LGTM - Simplification aligns with multi-tenant architecture.The passthrough implementation is correct for the new architecture where all eVaults share infrastructure. The comments clearly explain the rationale.
Consider making this method synchronous since it no longer performs async operations:
- async resolveUri(originalUri: string): Promise<string> { + resolveUri(originalUri: string): string { // In multi-tenant architecture, URIs are stored as IP:PORT and don't need resolution // Just return the stored URI directly return originalUri; }This would require updating callers to remove
await, but improves API clarity.platforms/registry/src/test-utils/mock-registry-server.ts (2)
8-10: Consider using a mock JWK function for test isolation.The evault-core version uses
mockGetJWK()while this uses the realgetJWK(). Using the real function may require environment setup (REGISTRY_ENTROPY_KEY_JWK env var) and make tests less isolated.Consider creating a
mockGetJWK()function for consistent test isolation:// Add a mock JWK function async function mockGetJWK() { return { keys: [{ kty: "oct", kid: "test-key-id", k: "test-key-value", alg: "HS256" }] }; } // Use it in the endpoint server.get("/.well-known/jwks.json", async () => { return await mockGetJWK(); });
12-18: Add authorization check for more complete test coverage.Unlike the evault-core version, this mock doesn't validate the
Authorizationheader. Adding auth validation would make tests more realistic and help catch auth-related issues.Consider adding the authorization check:
server.post("/register", async (request, reply) => { + const authHeader = request.headers.authorization; + if (!authHeader || !authHeader.startsWith("Bearer ")) { + return reply.status(401).send({ error: "Unauthorized" }); + } + const { ename, uri, evault } = request.body as any; if (!ename || !uri || !evault) { return reply.status(400).send({ error: "Missing required fields" }); } return reply.status(201).send({ ename, uri, evault }); });infrastructure/evault-core/src/core/db/migrations/add-ename-index.ts (1)
1-34: Migration looks safe but consider automation.The migration logic is correct with proper error handling and session cleanup. The
IF NOT EXISTSclause ensures idempotency.Consider adding automated migration tracking to ensure this runs on deployment. Currently, the documentation indicates manual execution is required, which could lead to missed migrations in production environments.
You could:
- Call this migration during application startup
- Track applied migrations in a Neo4j node (similar to TypeORM's migrations table)
- Integrate with a migration runner that tracks execution history
dev-docker-compose.yaml (1)
16-42: Consider potential node_modules cache conflicts.The shared
node_modules_cachevolume is mounted across all services. While this saves disk space, it could cause issues if different services have incompatible dependencies or different Node.js versions.Consider either:
- Using per-service node_modules volumes (e.g.,
registry_node_modules,evault_node_modules)- Documenting potential caveats if services have conflicting dependencies
- Relying on pnpm's workspace behavior which should handle this correctly
Since this is using pnpm workspaces, it should generally work fine, but worth noting for future troubleshooting.
infrastructure/evault-core/src/index.ts (1)
162-245: ReuseProvisioningServiceto avoid diverging provisioning flowsThis endpoint re-implements the provisioning workflow even though we now instantiate
ProvisioningServiceabove (and hand it to Fastify). Maintaining two separate code paths has already started to drift—Fastify benefits from the new validation/error handling while Express still uses the older sequence. Please delegate to the shared service so both surfaces stay consistent (and the demo-code / namespace handling only lives in one place). For example:expressApp.post( "/provision", async ( req: Request<{}, {}, ProvisionRequest>, res: Response<ProvisionResponse> ) => { - try { - ... - res.json({ - success: true, - w3id, - uri, - }); - } catch (error) { - ... - } + if (!provisioningService) { + return res.status(503).json({ + success: false, + message: "Provisioning service unavailable", + error: "Service not initialized", + }); + } + + const result = await provisioningService.provisionEVault(req.body); + if (!result.success) { + const status = result.message?.includes("verification") ? 400 : 500; + return res.status(status).json(result); + } + res.json(result); } );infrastructure/evault-core/src/core/provisioning/services/ProvisioningService.ts (1)
1-124: Unify the twoProvisioningServiceimplementationsWe now ship two different
ProvisioningServiceclasses (this one undercore/provisioning/...and another underservices/...). They already diverge in behaviour (namespace handling, error messages, demo-code updates), which will be painful to keep in sync. Please collapse them into a single implementation that both Fastify and any provisioning callers import, so we have one source of truth for provisioning logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (60)
.gitignore(1 hunks)Dockerfile.dev(1 hunks)dev-docker-compose.README.md(1 hunks)dev-docker-compose.yaml(1 hunks)docker-entrypoint.sh(1 hunks)infrastructure/evault-core/README.md(1 hunks)infrastructure/evault-core/docs/w3id-integration.md(0 hunks)infrastructure/evault-core/package.json(2 hunks)infrastructure/evault-core/src/config/database.ts(1 hunks)infrastructure/evault-core/src/core/db/db.service.spec.ts(1 hunks)infrastructure/evault-core/src/core/db/db.service.ts(12 hunks)infrastructure/evault-core/src/core/db/migrations/add-ename-index.ts(1 hunks)infrastructure/evault-core/src/core/db/types.ts(2 hunks)infrastructure/evault-core/src/core/http/server.ts(4 hunks)infrastructure/evault-core/src/core/protocol/graphql-server.ts(7 hunks)infrastructure/evault-core/src/core/protocol/vault-access-guard.spec.ts(1 hunks)infrastructure/evault-core/src/core/protocol/vault-access-guard.ts(2 hunks)infrastructure/evault-core/src/core/provisioning/config/database.ts(1 hunks)infrastructure/evault-core/src/core/provisioning/entities/Verification.ts(1 hunks)infrastructure/evault-core/src/core/provisioning/services/ProvisioningService.ts(1 hunks)infrastructure/evault-core/src/core/provisioning/services/VerificationService.ts(1 hunks)infrastructure/evault-core/src/db/db.service.spec.ts(0 hunks)infrastructure/evault-core/src/entities/Notification.ts(1 hunks)infrastructure/evault-core/src/entities/Verification.ts(1 hunks)infrastructure/evault-core/src/evault.ts(0 hunks)infrastructure/evault-core/src/index.ts(1 hunks)infrastructure/evault-core/src/secrets/secrets-store.ts(0 hunks)infrastructure/evault-core/src/services/NotificationService.spec.ts(1 hunks)infrastructure/evault-core/src/services/NotificationService.ts(1 hunks)infrastructure/evault-core/src/services/ProvisioningService.spec.ts(1 hunks)infrastructure/evault-core/src/services/ProvisioningService.ts(1 hunks)infrastructure/evault-core/src/services/VerificationService.spec.ts(1 hunks)infrastructure/evault-core/src/test-utils/mock-registry-server.ts(1 hunks)infrastructure/evault-core/src/test-utils/neo4j-setup.ts(1 hunks)infrastructure/evault-core/src/test-utils/postgres-setup.ts(1 hunks)infrastructure/evault-core/src/test-utils/test-setup.ts(1 hunks)infrastructure/evault-core/tests/log-storage.spec.ts(0 hunks)infrastructure/evault-core/tests/utils/mock-signer.ts(0 hunks)infrastructure/evault-core/tests/utils/mock-storage.ts(0 hunks)infrastructure/evault-core/tsconfig.json(1 hunks)infrastructure/evault-core/vitest.config.ts(1 hunks)infrastructure/evault-provisioner/README.md(0 hunks)infrastructure/evault-provisioner/package.json(0 hunks)infrastructure/evault-provisioner/src/index.ts(0 hunks)infrastructure/evault-provisioner/src/templates/evault.nomad.ts(0 hunks)infrastructure/evault-provisioner/test-notification.js(0 hunks)infrastructure/evault-provisioner/test-real-notifications.js(0 hunks)infrastructure/evault-provisioner/tsconfig.json(0 hunks)package.json(2 hunks)platforms/registry/jest.config.js(1 hunks)platforms/registry/package.json(2 hunks)platforms/registry/src/config/database.ts(2 hunks)platforms/registry/src/index.ts(1 hunks)platforms/registry/src/jwt.spec.ts(1 hunks)platforms/registry/src/services/KubernetesService.ts(0 hunks)platforms/registry/src/services/UriResolutionService.spec.ts(1 hunks)platforms/registry/src/services/UriResolutionService.ts(1 hunks)platforms/registry/src/services/VaultService.spec.ts(1 hunks)platforms/registry/src/test-utils/mock-registry-server.ts(1 hunks)platforms/registry/src/test-utils/postgres-setup.ts(1 hunks)
💤 Files with no reviewable changes (15)
- infrastructure/evault-provisioner/tsconfig.json
- infrastructure/evault-core/src/db/db.service.spec.ts
- infrastructure/evault-provisioner/src/templates/evault.nomad.ts
- infrastructure/evault-core/src/evault.ts
- platforms/registry/src/services/KubernetesService.ts
- infrastructure/evault-core/src/secrets/secrets-store.ts
- infrastructure/evault-core/tests/log-storage.spec.ts
- infrastructure/evault-core/tests/utils/mock-signer.ts
- infrastructure/evault-core/tests/utils/mock-storage.ts
- infrastructure/evault-provisioner/test-real-notifications.js
- infrastructure/evault-provisioner/src/index.ts
- infrastructure/evault-provisioner/README.md
- infrastructure/evault-provisioner/package.json
- infrastructure/evault-core/docs/w3id-integration.md
- infrastructure/evault-provisioner/test-notification.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-16T18:04:28.983Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 99
File: infrastructure/w3id/tests/w3id.test.ts:48-57
Timestamp: 2025-04-16T18:04:28.983Z
Learning: In test files, suggestions to add defensive coding patterns (like extra null checks) are often unnecessary, as tests are designed to fail explicitly when expectations aren't met. The test in `infrastructure/w3id/tests/w3id.test.ts` validates that `id.logs` is an instance of `IDLogManager` before accessing its repository property.
Applied to files:
infrastructure/evault-core/src/services/VerificationService.spec.tsplatforms/registry/src/services/VaultService.spec.tsinfrastructure/evault-core/src/test-utils/test-setup.tsinfrastructure/evault-core/src/core/db/db.service.spec.tsinfrastructure/evault-core/src/services/ProvisioningService.spec.tsinfrastructure/evault-core/src/core/protocol/vault-access-guard.spec.ts
🧬 Code graph analysis (20)
platforms/registry/src/services/UriResolutionService.spec.ts (1)
platforms/registry/src/services/UriResolutionService.ts (1)
UriResolutionService(6-16)
infrastructure/evault-core/src/test-utils/postgres-setup.ts (1)
platforms/registry/src/test-utils/postgres-setup.ts (2)
setupTestDatabase(8-32)teardownTestDatabase(34-43)
infrastructure/evault-core/src/index.ts (9)
infrastructure/evault-core/src/config/database.ts (1)
AppDataSource(10-18)infrastructure/evault-core/src/services/VerificationService.ts (1)
VerificationService(4-50)infrastructure/evault-core/src/controllers/NotificationController.ts (1)
NotificationController(6-184)infrastructure/evault-core/src/core/protocol/graphql-server.ts (1)
GraphQLServer(12-349)infrastructure/evault-core/src/core/provisioning/services/ProvisioningService.ts (4)
ProvisioningService(23-124)ProvisionRequest(8-13)ProvisionResponse(15-21)DEMO_CODE_W3DS(6-6)infrastructure/evault-core/src/core/db/retry-neo4j.ts (1)
connectWithRetry(13-40)infrastructure/evault-core/src/core/db/migrations/add-ename-index.ts (1)
createENameIndex(15-33)infrastructure/evault-core/src/core/db/db.service.ts (1)
DbService(18-599)infrastructure/evault-core/src/core/http/server.ts (1)
registerHttpRoutes(17-301)
infrastructure/evault-core/src/services/VerificationService.spec.ts (2)
infrastructure/evault-core/src/services/VerificationService.ts (1)
VerificationService(4-50)infrastructure/evault-core/src/test-utils/postgres-setup.ts (2)
setupTestDatabase(10-34)teardownTestDatabase(36-45)
infrastructure/evault-core/src/entities/Verification.ts (2)
infrastructure/evault-core/src/core/provisioning/entities/Verification.ts (1)
Entity(9-52)infrastructure/evault-provisioner/src/entities/Verification.ts (1)
Verification(10-52)
platforms/registry/src/services/VaultService.spec.ts (2)
platforms/registry/src/services/VaultService.ts (1)
VaultService(4-32)platforms/registry/src/test-utils/postgres-setup.ts (2)
setupTestDatabase(8-32)teardownTestDatabase(34-43)
platforms/registry/src/jwt.spec.ts (1)
platforms/registry/src/jwt.ts (3)
generateEntropy(48-57)getJWK(70-80)generatePlatformToken(59-67)
infrastructure/evault-core/src/core/http/server.ts (3)
infrastructure/evault-core/src/core/provisioning/services/ProvisioningService.ts (2)
ProvisioningService(23-124)ProvisionRequest(8-13)infrastructure/evault-core/src/services/ProvisioningService.ts (2)
ProvisioningService(23-173)ProvisionRequest(8-13)infrastructure/evault-core/src/core/http/types.ts (2)
TypedRequest(14-16)TypedReply(18-18)
infrastructure/evault-core/src/services/ProvisioningService.ts (4)
infrastructure/evault-core/src/core/provisioning/services/ProvisioningService.ts (4)
DEMO_CODE_W3DS(6-6)ProvisionRequest(8-13)ProvisionResponse(15-21)ProvisioningService(23-124)infrastructure/evault-core/src/core/provisioning/services/VerificationService.ts (1)
VerificationService(4-51)infrastructure/web3-adapter/src/index.js (2)
registryEntropy(62-62)namespace(63-63)infrastructure/w3id/src/w3id.ts (1)
W3IDBuilder(32-158)
infrastructure/evault-core/src/core/provisioning/services/ProvisioningService.ts (3)
infrastructure/evault-core/src/core/provisioning/services/VerificationService.ts (1)
VerificationService(4-51)infrastructure/web3-adapter/src/index.js (2)
registryEntropy(62-62)namespace(63-63)infrastructure/w3id/src/w3id.ts (1)
W3IDBuilder(32-158)
infrastructure/evault-core/src/core/provisioning/services/VerificationService.ts (2)
infrastructure/evault-core/src/services/VerificationService.ts (1)
VerificationService(4-50)infrastructure/evault-provisioner/src/services/VerificationService.ts (1)
VerificationService(4-50)
infrastructure/evault-core/src/services/NotificationService.spec.ts (2)
infrastructure/evault-core/src/services/NotificationService.ts (1)
NotificationService(25-140)infrastructure/evault-core/src/test-utils/postgres-setup.ts (2)
setupTestDatabase(10-34)teardownTestDatabase(36-45)
platforms/registry/src/test-utils/postgres-setup.ts (1)
infrastructure/evault-core/src/test-utils/postgres-setup.ts (2)
setupTestDatabase(10-34)teardownTestDatabase(36-45)
infrastructure/evault-core/src/test-utils/mock-registry-server.ts (1)
platforms/registry/src/test-utils/mock-registry-server.ts (2)
createMockRegistryServer(4-30)stopMockRegistryServer(32-34)
platforms/registry/src/test-utils/mock-registry-server.ts (2)
infrastructure/evault-core/src/test-utils/mock-registry-server.ts (1)
createMockRegistryServer(16-47)platforms/registry/src/jwt.ts (1)
getJWK(70-80)
infrastructure/evault-core/src/core/db/db.service.spec.ts (2)
infrastructure/evault-core/src/core/db/types.ts (1)
Envelope(14-19)infrastructure/evault-core/src/core/db/db.service.ts (1)
DbService(18-599)
infrastructure/evault-core/src/core/db/db.service.ts (3)
infrastructure/evault-core/src/core/db/types.ts (4)
MetaEnvelope(4-9)StoreMetaEnvelopeResult(41-50)MetaEnvelopeResult(26-36)GetAllEnvelopesResult(62-62)infrastructure/w3id/src/w3id.ts (1)
W3IDBuilder(32-158)infrastructure/evault-core/src/core/db/schema.ts (1)
serializeValue(58-74)
infrastructure/evault-core/src/core/protocol/graphql-server.ts (2)
infrastructure/evault-core/src/core/protocol/vault-access-guard.ts (1)
VaultContext(7-11)infrastructure/w3id/src/utils/jwt.ts (1)
getJWTHeader(86-89)
infrastructure/evault-core/src/services/ProvisioningService.spec.ts (4)
infrastructure/evault-core/src/services/VerificationService.ts (1)
VerificationService(4-50)infrastructure/evault-core/src/services/ProvisioningService.ts (3)
ProvisioningService(23-173)ProvisionRequest(8-13)DEMO_CODE_W3DS(6-6)infrastructure/evault-core/src/test-utils/postgres-setup.ts (2)
setupTestDatabase(10-34)teardownTestDatabase(36-45)infrastructure/evault-core/src/test-utils/mock-registry-server.ts (1)
createMockRegistryServer(16-47)
infrastructure/evault-core/src/core/protocol/vault-access-guard.spec.ts (3)
infrastructure/evault-core/src/core/db/db.service.ts (1)
DbService(18-599)infrastructure/evault-core/src/core/protocol/vault-access-guard.ts (4)
VaultAccessGuard(13-176)VaultContext(7-11)validateToken(21-51)checkAccess(59-102)infrastructure/evault-core/src/test-utils/neo4j-setup.ts (2)
setupTestNeo4j(7-22)teardownTestNeo4j(24-33)
🪛 Checkov (3.2.334)
dev-docker-compose.yaml
[medium] 13-14: Basic Auth Credentials
(CKV_SECRET_4)
🪛 GitHub Actions: Check Lint
package.json
[error] 1-1: Command 'turbo run check-lint' failed with exit code 1. Failed to add workspace 'rest-express' from 'platforms/eReputation/package.json'; it already exists at 'platforms/dreamSync/package.json'.
🪛 GitHub Actions: Tests [evault-core]
infrastructure/evault-core/src/core/db/types.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/core/db/migrations/add-ename-index.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/entities/Notification.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/services/NotificationService.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/test-utils/postgres-setup.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/index.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/services/VerificationService.spec.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/core/protocol/vault-access-guard.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/core/provisioning/config/database.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/entities/Verification.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/tsconfig.json
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/core/http/server.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/services/ProvisioningService.ts
[error] 63-63: Provisioning error: JWT verification failed: Invalid token
[error] 91-91: Provisioning error: verification doesn't exist
[error] 97-97: Provisioning error: verification not approved
[error] 100-100: Provisioning error: already been used
infrastructure/evault-core/src/core/provisioning/services/ProvisioningService.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/core/provisioning/services/VerificationService.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/services/NotificationService.spec.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/test-utils/mock-registry-server.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/test-utils/test-setup.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/core/db/db.service.spec.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/core/db/db.service.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/core/protocol/graphql-server.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/README.md
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/vitest.config.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/core/provisioning/entities/Verification.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/test-utils/neo4j-setup.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/services/ProvisioningService.spec.ts
[error] 201-201: Provisioning error: JWT verification failed: Invalid token
[error] 211-211: Provisioning error: verification doesn't exist
[error] 227-227: Provisioning error: verification not approved
[error] 243-243: Provisioning error: already been used
[error] 251-251: Provisioning error: { response: { status: 500, data: { error: 'Internal server error' } } }
infrastructure/evault-core/src/config/database.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/package.json
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
infrastructure/evault-core/src/core/protocol/vault-access-guard.spec.ts
[error] 1-1: Segmentation fault (core dumped) during test run. Exit status 139
🪛 markdownlint-cli2 (0.18.1)
infrastructure/evault-core/README.md
18-18: Bare URL used
(MD034, no-bare-urls)
90-90: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
98-98: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
143-143: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
🔇 Additional comments (32)
.gitignore (1)
44-48: LGTM—appropriate infrastructure updates.The additions align with the multi-tenant eVault refactor and modern build tooling setup (PNPM). The
evault-cache.jsonretention is consistent with the broader architecture changes.infrastructure/evault-core/README.md (1)
1-186: README documentation is valid; the review comment is misattached or unsubstantiated.This is a documentation-only change and cannot cause segmentation faults or pipeline failures. Changes to README content do not execute during tests and have no impact on test infrastructure or runtime behavior.
If the test pipeline is actually failing:
- Provide specific evidence: workflow run ID, error logs from GitHub Actions, and the exact failing test
- Verify the failure exists before raising it as a blocker to a documentation PR
- Segmentation faults typically stem from runtime/environment issues (Docker, memory, native dependencies), not README content
This review comment should not block a documentation change without concrete evidence and proper correlation to the PR's actual code changes.
Likely an incorrect or invalid review comment.
infrastructure/evault-core/vitest.config.ts (5)
1-4: LGTM: Configuration structure is sound.The vitest configuration structure follows best practices with proper imports and config definition.
5-7: LGTM: Test discovery and setup configuration is appropriate.The explicit glob pattern, node environment, and setupFiles integration support the expanded test utilities for database integration testing.
8-9: LGTM: Timeout increases justified for testcontainers.The 120-second timeouts are appropriate for PostgreSQL and Neo4j testcontainer lifecycle operations (startup, initialization, teardown).
10-19: LGTM: Coverage configuration follows best practices.The v8 provider with multiple reporters and appropriate exclusions (node_modules, dist, type declarations, migrations) provides good coverage visibility without false positives.
1-21: Config is correctly configured; investigate segfault root cause separately from code review.The vitest configuration is appropriately set up for testcontainer-based tests with suitable timeouts (120s for both tests and hooks). Resource cleanup in postgres-setup.ts and neo4j-setup.ts appears sound.
The segmentation fault is likely caused by:
- Beta version of @testcontainers/postgresql (^10.0.0-beta.6) — consider upgrading or pinning to stable
- Native module conflicts or incompatibilities
- Runtime environment constraints (memory, resource limits)
These are infrastructure/environment concerns, not code review issues for the configuration file itself. Verify and test in the actual execution environment rather than through static code analysis.
infrastructure/evault-core/src/core/provisioning/entities/Verification.ts (1)
53-53: LGTM - Formatting improvement.The trailing newline follows standard file formatting conventions.
package.json (1)
13-15: LGTM - Docker development scripts are well-structured.The three scripts provide a good developer experience:
dev:dockerfor core services with hot reloaddev:docker:downfor cleanupdev:docker:allfor optional servicesinfrastructure/evault-core/src/entities/Notification.ts (1)
8-24: LGTM - Explicit column types improve schema clarity.Adding explicit column types is a best practice that:
- Ensures consistent behavior across PostgreSQL versions
- Makes the intended schema clear
- Prevents TypeORM from using potentially inconsistent default mappings
The type choices are appropriate:
varcharfor identifiers/short text,textfor body content,booleanfor flags, andtimestampfor dates.platforms/registry/src/test-utils/postgres-setup.ts (2)
8-32: LGTM - Well-structured test database setup.The singleton pattern and container management are implemented correctly:
- Reuses existing container/DataSource if already initialized
- Uses
synchronize: truefor automatic schema creation in tests- Properly initializes DataSource before returning
34-43: LGTM - Proper cleanup sequence.The teardown follows the correct order:
- Destroy DataSource (closes connections)
- Stop container
- Reset variables for next test run
platforms/registry/src/test-utils/mock-registry-server.ts (1)
20-34: LGTM - Remaining endpoints and lifecycle management are correct.The
/platformsendpoint returns suitable mock data, and the server lifecycle functions (start/stop) are implemented correctly.platforms/registry/package.json (1)
30-30: Upgrade @testcontainers/postgresql to a stable version.The latest stable version is 11.7.2, whereas this package is pinned to
^10.0.0-beta.6. Using a stable version eliminates the risk of undocumented behavior or breaking changes in beta releases. Note that upgrading from 10.0.0-beta to 11.7.2 may include breaking changes; verify your test setup remains compatible after the upgrade.infrastructure/evault-core/src/test-utils/test-setup.ts (1)
1-2: ****The
reflect-metadataimport in lines 1–2 is correct and necessary. It is properly configured as a Vitest setupFile and runs before all tests, which is exactly how TypeORM requires it to work. Web search and code inspection show no evidence of segmentation faults caused by this import. If test failures occur, they stem from testcontainers' PostgreSQL image version or environment architecture mismatches—not from this import statement. The original comment speculated without concrete logs or reproduction steps.Likely an incorrect or invalid review comment.
infrastructure/evault-core/package.json (2)
3-3: Version bump to 1.0.0 is appropriate for breaking changes.The major version bump correctly signals breaking changes in this refactor (eName parameter addition, Kubernetes service removal, multi-tenant architecture).
40-41: Good: Testcontainers correctly moved to devDependencies.Moving
@testcontainers/neo4jfrom dependencies to devDependencies and adding@testcontainers/postgresqlas a devDependency is the correct placement for test infrastructure.platforms/registry/src/config/database.ts (1)
3-3: Documentation comments clarify multi-tenant responsibility.The added comments appropriately document that the Verification entity is managed by evault-core's provisioning service, clarifying the separation of concerns in the multi-tenant architecture.
Also applies to: 16-16
platforms/registry/src/index.ts (1)
52-53: Simplified URI resolution aligns with multi-tenant architecture.The removal of Kubernetes integration and the simplified UriResolutionService initialization correctly reflect the transition to a multi-tenant architecture where URIs are stored as IP:PORT and don't require dynamic resolution.
infrastructure/evault-core/src/services/NotificationService.ts (1)
32-46: Verification lookup correctly narrowed to eName and deviceId.The updated query properly identifies verifications by both
linkedENameanddeviceId, ensuring device-specific registration. The removal of the redundantdeviceIdassignment in the update path is correct since the record already has the correctdeviceId(it's part of the lookup criteria).infrastructure/evault-core/src/core/protocol/vault-access-guard.ts (3)
10-10: eName field added to VaultContext for multi-tenant isolation.The addition of
eNametoVaultContextcorrectly supports the multi-tenant architecture, allowing tenant-specific data access control.
75-78: Good: eName presence enforced for access control.The validation ensures
eNameis present before processing access control logic, properly enforcing multi-tenant data isolation requirements. This prevents data leakage between tenants.
82-85: All DB queries correctly propagate eName for tenant isolation.The consistent passing of
context.eNametofindMetaEnvelopeByIdensures tenant-scoped queries throughout the access control flow.Also applies to: 90-90
platforms/registry/src/services/UriResolutionService.spec.ts (1)
1-36: Test coverage appropriate for simplified passthrough behavior.The test suite correctly validates that
resolveUrinow implements passthrough behavior for the multi-tenant architecture, covering common URI formats (localhost, IP addresses, HTTPS). This aligns with the removal of Kubernetes-based resolution logic.Dockerfile.dev (1)
1-21: Development image strategy is sound.The approach of keeping the image small by installing dependencies on first container run (via the entrypoint script) and mounting source code via volumes is appropriate for a development workflow. Pinning pnpm to version 10.13.1 ensures reproducible builds.
docker-entrypoint.sh (1)
7-46: Good: Coordination mechanism handles multi-container dependency installation.The lock file approach with timeout handling and cleanup on failure provides reasonable coordination for ensuring only one container installs dependencies at a time. The 5-minute timeout and re-checks during waiting are appropriate safeguards.
dev-docker-compose.README.md (1)
1-89: LGTM! Excellent developer documentation.The documentation is clear, comprehensive, and well-structured. It covers all essential aspects of the Docker Compose setup including core services, optional profiles, usage examples, environment variables, and important notes about volumes and hot-reload.
infrastructure/evault-core/src/core/db/types.ts (1)
24-36: LGTM! Clean type addition for multi-tenancy.The optional
eNamefield is appropriately added to support the multi-tenant architecture. The inline documentation clearly indicates it's for internal use only and not exposed via GraphQL responses, which is good for API stability.infrastructure/evault-core/src/config/database.ts (1)
12-12: Breaking change: URL resolution order updated.The new precedence
REGISTRY_DATABASE_URL → PROVISIONER_DATABASE_URL → registry defaultaligns with consolidating the provisioner package into evault-core. This is appropriate for the multi-tenant refactor.This is a breaking change if environments have
PROVISIONER_DATABASE_URLset withoutREGISTRY_DATABASE_URL. Verify that:
- All deployment configurations are updated
- The migration guide documents this environment variable change
- CI/CD pipelines use the correct variable name
dev-docker-compose.yaml (1)
13-14: Static analysis warning is a false positive.The static analysis tool flagged these environment variables as containing basic auth credentials. However, this is acceptable for a development Docker Compose file:
- Uses environment variable substitution with dev-friendly defaults
- Not production configuration (defaults are clearly marked "dev-secret-change-me")
- Standard practice for local development setups
infrastructure/evault-core/src/services/VerificationService.spec.ts (1)
1-171: Well-structured tests, but they're likely triggering the segfault.The test structure and logic are solid with proper setup/teardown, clear test cases, and good coverage of the VerificationService methods. The commented-out tests for
findAndCountare appropriately noted as not yet implemented.However, these tests (or the test utilities they use) are likely causing the segmentation fault (exit 139) in CI. The issue is probably in the testcontainers PostgreSQL setup, not the test logic itself.
Common causes:
- Testcontainers native module incompatibility with the CI environment's Node.js version
- Resource cleanup issues between test runs causing memory corruption
- Docker socket access issues in CI environment
To diagnose, try:
- Run tests in isolation:
pnpm test VerificationService.spec.ts- Check if segfault occurs during setup, test execution, or teardown
- Verify Docker is available and accessible in CI environment
- Consider using an in-memory database for unit tests instead of spinning up containers
⛔ Skipped due to learnings
Learnt from: coodos Repo: MetaState-Prototype-Project/prototype PR: 99 File: infrastructure/w3id/tests/w3id.test.ts:48-57 Timestamp: 2025-04-16T18:04:28.983Z Learning: In test files, suggestions to add defensive coding patterns (like extra null checks) are often unnecessary, as tests are designed to fail explicitly when expectations aren't met. The test in `infrastructure/w3id/tests/w3id.test.ts` validates that `id.logs` is an instance of `IDLogManager` before accessing its repository property.infrastructure/evault-core/tsconfig.json (1)
2-16: Investigate segfault in test execution—likely neo4j-driver 5.28.1 version regression.The segmentation fault (exit 139) during test execution is likely caused by neo4j-driver 5.28.1 having known regressions. Additionally,
engines.nodeis not pinned in package.json, which could cause Node.js version mismatches between dev and CI environments.Immediate actions:
- Upgrade neo4j-driver:
npm update neo4j-driver@latest(or downgrade to 5.27.x if latest fails)- Pin Node.js version in package.json: add
"engines": {"node": "18.x"}or"20.x"(match your CI environment)- Run diagnostics:
node --version npm ls neo4j-driver npm rebuild npm test -- --reporter=verboseIf the segfault persists, verify Node and native libs (protobuf, OpenSSL) are compatible by capturing npm ls neo4j-driver and npm ls protobuf, and try a different Node LTS version to rule out runtime incompatibility.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
infrastructure/evault-core/src/services/ProvisioningService.ts (1)
66-82: Restore deterministic W3ID generation by validating and using namespaceThe namespace parameter is required and extracted but never validated as a UUID or passed to
W3IDBuilder.withNamespace(). Without it, the builder generates a random namespace internally (per w3id.ts line 105), so identical entropy produces different W3IDs on each call. This breaks deterministic ID generation, tenant isolation, and idempotency—issues flagged in the previous review.As you confirmed, namespace must ALWAYS be a UUID. Validate it upfront and pass it to the builder:
import axios, { AxiosError } from "axios"; import { W3IDBuilder } from "w3id"; import * as jose from "jose"; +import { validate as uuidValidate } from "uuid"; import { VerificationService } from "./VerificationService";const { registryEntropy, namespace, verificationId, publicKey } = request; if (!registryEntropy || !namespace || !verificationId || !publicKey) { return { success: false, error: "Missing required fields", message: "Missing required fields: registryEntropy, namespace, verificationId, publicKey", }; } + + // Validate namespace is a valid UUID + if (!uuidValidate(namespace)) { + return { + success: false, + error: "Invalid namespace", + message: "Namespace must be a valid UUID", + }; + } // Verify the registry entropy token- // Generate eName (W3ID) from entropy - // namespace is a string, but W3IDBuilder.withNamespace expects a UUID - // We'll let W3IDBuilder generate a random namespace if namespace is not a valid UUID let w3id: string; try { - // Try to use namespace as-is (if it's already a UUID), otherwise W3IDBuilder will generate one - // W3IDBuilder internally generates a UUID v4 namespace if not provided const userId = await new W3IDBuilder() + .withNamespace(namespace) .withEntropy(payload.entropy as string) .withGlobal(true) .build(); w3id = userId.id; } catch (w3idError) { - // If W3ID generation fails, it's likely an entropy format issue - // Re-throw with clearer message, but let verification errors take precedence throw new Error(`Failed to generate W3ID from entropy: ${w3idError instanceof Error ? w3idError.message : String(w3idError)}`); }
🧹 Nitpick comments (2)
infrastructure/evault-core/src/services/ProvisioningService.ts (2)
48-48: Type the JWT payload properlyUsing
anybypasses TypeScript's type safety. The jose library exportsJWTPayload, which you can extend if the registry entropy includes custom claims:- let payload: any; + let payload: jose.JWTPayload & { entropy?: string };
104-115: Consider preserving the actual update errorThe verification was already validated to exist (lines 93–94), so an update failure here likely indicates a database issue rather than a missing record. Swallowing the real error with a generic message might complicate debugging.
Either remove the try-catch (letting DB errors propagate naturally) or preserve the actual error:
if (verificationId !== DEMO_CODE_W3DS) { try { await this.verificationService.findByIdAndUpdate(verificationId, { linkedEName: w3id, consumed: true }); } catch (updateError) { - // If update fails, it means verification doesn't exist (should have been caught above, but handle gracefully) - throw new Error("verification doesn't exist"); + throw new Error(`Failed to update verification: ${updateError instanceof Error ? updateError.message : String(updateError)}`); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrastructure/evault-core/src/services/ProvisioningService.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-16T18:04:28.983Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 99
File: infrastructure/w3id/tests/w3id.test.ts:48-57
Timestamp: 2025-04-16T18:04:28.983Z
Learning: In test files, suggestions to add defensive coding patterns (like extra null checks) are often unnecessary, as tests are designed to fail explicitly when expectations aren't met. The test in `infrastructure/w3id/tests/w3id.test.ts` validates that `id.logs` is an instance of `IDLogManager` before accessing its repository property.
Applied to files:
infrastructure/evault-core/src/services/ProvisioningService.ts
🧬 Code graph analysis (1)
infrastructure/evault-core/src/services/ProvisioningService.ts (3)
infrastructure/evault-core/src/core/provisioning/services/ProvisioningService.ts (4)
DEMO_CODE_W3DS(6-6)ProvisionRequest(8-13)ProvisionResponse(15-21)ProvisioningService(23-124)infrastructure/evault-core/src/services/VerificationService.ts (1)
VerificationService(4-50)infrastructure/w3id/src/w3id.ts (1)
W3IDBuilder(32-158)
🪛 GitHub Actions: Tests [evault-core]
infrastructure/evault-core/src/services/ProvisioningService.ts
[error] 63-63: Provisioning failed: JWT verification failed: Invalid token
[error] 1-1: Test run exited with code 139 (Segmentation fault).
🔇 Additional comments (1)
infrastructure/evault-core/src/services/ProvisioningService.ts (1)
153-174: Well-structured error handlingThe error handling properly distinguishes verification-related errors from generic provisioning failures, preserves meaningful messages, and returns a consistent response structure. The logging and AxiosError handling are appropriate.
… generation deterministic again.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
infrastructure/evault-core/package.json (1)
21-23: Clarify the addition of Express as a production dependency.Both
express(line 23) andfastify(line 24) are now in the production dependency list. The project appears to use Fastify as the primary framework. Confirm whether Express is intentionally required for the new provisioning service or if it should be removed.Please clarify:
- Is Express used by the provisioning service or any other component?
- If not needed, remove it to reduce the dependency footprint.
- If both frameworks are required, document why both are necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
infrastructure/evault-core/package.json(2 hunks)infrastructure/evault-core/src/services/ProvisioningService.spec.ts(1 hunks)infrastructure/evault-core/src/services/ProvisioningService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- infrastructure/evault-core/src/services/ProvisioningService.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-16T18:04:28.983Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 99
File: infrastructure/w3id/tests/w3id.test.ts:48-57
Timestamp: 2025-04-16T18:04:28.983Z
Learning: In test files, suggestions to add defensive coding patterns (like extra null checks) are often unnecessary, as tests are designed to fail explicitly when expectations aren't met. The test in `infrastructure/w3id/tests/w3id.test.ts` validates that `id.logs` is an instance of `IDLogManager` before accessing its repository property.
Applied to files:
infrastructure/evault-core/src/services/ProvisioningService.spec.ts
🧬 Code graph analysis (1)
infrastructure/evault-core/src/services/ProvisioningService.spec.ts (4)
infrastructure/evault-core/src/services/VerificationService.ts (1)
VerificationService(4-50)infrastructure/evault-core/src/services/ProvisioningService.ts (3)
ProvisioningService(24-222)ProvisionRequest(9-14)DEMO_CODE_W3DS(7-7)infrastructure/evault-core/src/test-utils/postgres-setup.ts (2)
setupTestDatabase(10-34)teardownTestDatabase(36-45)infrastructure/evault-core/src/test-utils/mock-registry-server.ts (2)
createMockRegistryServer(16-47)stopMockRegistryServer(49-53)
🪛 GitHub Actions: Tests [evault-core]
infrastructure/evault-core/src/services/ProvisioningService.spec.ts
[error] 1-1: Segmentation fault (core dumped) during tests. Exit status 139.
infrastructure/evault-core/package.json
[error] 1-1: Segmentation fault (core dumped) during tests. Exit status 139.
🔇 Additional comments (4)
infrastructure/evault-core/package.json (3)
3-5: Version bump and entry point update are appropriate.The major version bump to 1.0.0 correctly signals breaking changes aligned with the multi-tenant architecture migration. The main entry point change to
dist/index.jsis consistent with the TypeScript compilation workflow.
10-14: Request manual verification of segmentation fault and test environment configuration.The review comment references a pipeline segmentation fault (exit code 139), but this cannot be reproduced without installing dependencies and running the full testcontainer environment. Partial findings:
Confirmed:
- Database config file (
src/config/database.ts) exists and is properly configured- Test script is correctly defined in package.json
- Beta version of
@testcontainers/postgresql(^10.0.0-beta.6) is present—this is a legitimate risk factor for infrastructure testsUnable to verify:
- Root cause of segmentation fault—could stem from testcontainers version incompatibility, Docker/container nesting issues, memory configuration, or environment setup
- Interaction between TypeORM (0.3.24), pg driver (8.11.3), and testcontainers versions
Action items:
- Run tests locally with:
npm install && npm run test -- --reporter=verboseand share the full error output- Check if the beta testcontainers version can be upgraded to stable (^10.0.1 or later) to rule out version-related crashes
- Clarify the architectural decision for having both Express (^4.18.2) and Fastify (^4.26.2) in dependencies—typically only one primary framework is needed
12-14: Database configuration path is correct; Neo4j concern is inapplicable.The migration scripts correctly reference
src/config/database.ts, which exists and is properly configured with PostgreSQL connection parameters (viaREGISTRY_DATABASE_URL,PROVISIONER_DATABASE_URL, with fallback topostgresql://postgres:postgres@localhost:5432/registry).However, Neo4j is not managed by TypeORM migrations. It uses the native
neo4j-driverand is configured separately insrc/index.tswith dedicated environment variables (NEO4J_URI,NEO4J_USER,NEO4J_PASSWORD). The application architecture intentionally keeps PostgreSQL (TypeORM) and Neo4j (direct driver) as separate database systems—migrations only handle PostgreSQL.Likely an incorrect or invalid review comment.
infrastructure/evault-core/src/services/ProvisioningService.spec.ts (1)
182-191: Great regression coverage on verification consumption. Ensuring the spec asserts bothlinkedENameandconsumedprotects the one-shot guarantee we fixed earlier and will flag any future regressions immediately.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
infrastructure/evault-core/package.json (1)
41-41: Resolve the unaddressed beta version issue that is causing pipeline failure.The
@testcontainers/postgresqldependency remains pinned to a beta version (^10.0.0-beta.6), which was flagged in a previous review. This is now causing the test pipeline to fail with exit code 133 (segmentation fault). This must be fixed before merging.Update to the stable release and verify the pipeline passes:
- "@testcontainers/postgresql": "^10.0.0-beta.6", + "@testcontainers/postgresql": "^10.0.1",After updating, rerun the test suite to confirm the segmentation fault is resolved.
🧹 Nitpick comments (3)
infrastructure/evault-core/src/services/ProvisioningService.spec.ts (2)
35-35: Optional: Improve type safety for mocked axios.The
anytype reduces type safety. Consider usingtypeof axiosor a more specific type.-const mockedAxios = axios as any; +const mockedAxios = axios as jest.Mocked<typeof axios>;
194-293: Add test coverage for invalid namespace UUID.The error test cases are comprehensive, but one validation path is missing: the ProvisioningService checks if namespace is a valid UUID (see ProvisioningService.ts around line 77). Consider adding a test case that provides an invalid UUID namespace to verify the error handling.
Add this test case to the "error cases" describe block:
it("should fail with invalid namespace UUID", async () => { const request = await createValidRequest(); request.namespace = "not-a-valid-uuid"; const result = await provisioningService.provisionEVault(request); expect(result.success).toBe(false); expect(result.error).toContain("Invalid namespace"); });infrastructure/evault-core/src/core/protocol/vault-access-guard.spec.ts (1)
331-428: Consider adding test coverage for array filtering in middleware.The middleware implementation includes logic to filter array results (bulk queries) based on access control, but there's no test case covering this scenario. Consider adding a test where:
- The resolver returns an array of meta-envelopes
- The middleware filters the array based on each envelope's ACL
- The filtered result only includes envelopes the user can access
This would provide more complete coverage of the
filterEnvelopesByAccessmethod.Example test structure:
it("should filter array results based on access", async () => { const eName = "[email protected]"; // Create multiple meta-envelopes with different ACLs const publicEnvelope = await dbService.storeMetaEnvelope( { ontology: "Public", payload: { data: "public" }, acl: ["*"] }, ["*"], eName ); const restrictedEnvelope = await dbService.storeMetaEnvelope( { ontology: "Restricted", payload: { data: "restricted" }, acl: ["other-user"] }, ["other-user"], eName ); const context = createMockContext({ eName, currentUser: "user-123", }); const mockResolver = vi.fn(async () => { // Return array of both envelopes return [ await dbService.findMetaEnvelopeById(publicEnvelope.metaEnvelope.id, eName), await dbService.findMetaEnvelopeById(restrictedEnvelope.metaEnvelope.id, eName), ]; }); const wrappedResolver = guard.middleware(mockResolver); const result = await wrappedResolver(null, {}, context); // No id/envelopeId triggers array filtering expect(result).toHaveLength(1); expect(result[0].id).toBe(publicEnvelope.metaEnvelope.id); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
infrastructure/evault-core/package.json(2 hunks)infrastructure/evault-core/src/core/db/db.service.spec.ts(1 hunks)infrastructure/evault-core/src/core/http/server.ts(4 hunks)infrastructure/evault-core/src/core/protocol/vault-access-guard.spec.ts(1 hunks)infrastructure/evault-core/src/services/NotificationService.ts(1 hunks)infrastructure/evault-core/src/services/ProvisioningService.spec.ts(1 hunks)infrastructure/evault-core/src/services/VerificationService.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- infrastructure/evault-core/src/services/VerificationService.spec.ts
- infrastructure/evault-core/src/services/NotificationService.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-16T18:04:28.983Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 99
File: infrastructure/w3id/tests/w3id.test.ts:48-57
Timestamp: 2025-04-16T18:04:28.983Z
Learning: In test files, suggestions to add defensive coding patterns (like extra null checks) are often unnecessary, as tests are designed to fail explicitly when expectations aren't met. The test in `infrastructure/w3id/tests/w3id.test.ts` validates that `id.logs` is an instance of `IDLogManager` before accessing its repository property.
Applied to files:
infrastructure/evault-core/src/services/ProvisioningService.spec.tsinfrastructure/evault-core/src/core/db/db.service.spec.tsinfrastructure/evault-core/src/core/protocol/vault-access-guard.spec.ts
🧬 Code graph analysis (4)
infrastructure/evault-core/src/services/ProvisioningService.spec.ts (5)
infrastructure/evault-core/src/services/VerificationService.ts (1)
VerificationService(4-50)infrastructure/evault-core/src/services/ProvisioningService.ts (3)
ProvisioningService(24-222)ProvisionRequest(9-14)DEMO_CODE_W3DS(7-7)infrastructure/evault-core/src/test-utils/postgres-setup.ts (2)
setupTestDatabase(10-34)teardownTestDatabase(36-45)infrastructure/evault-core/src/test-utils/mock-registry-server.ts (2)
createMockRegistryServer(16-47)stopMockRegistryServer(49-53)infrastructure/evault-core/src/index.ts (1)
DEMO_CODE_W3DS(88-88)
infrastructure/evault-core/src/core/db/db.service.spec.ts (2)
infrastructure/evault-core/src/core/db/types.ts (1)
Envelope(14-19)infrastructure/evault-core/src/core/db/db.service.ts (1)
DbService(18-599)
infrastructure/evault-core/src/core/http/server.ts (4)
infrastructure/web3-adapter/src/index.js (1)
evault(98-98)infrastructure/evault-core/src/services/ProvisioningService.ts (2)
ProvisioningService(24-222)ProvisionRequest(9-14)infrastructure/evault-core/src/core/provisioning/services/ProvisioningService.ts (2)
ProvisioningService(23-124)ProvisionRequest(8-13)infrastructure/evault-core/src/core/http/types.ts (2)
TypedRequest(14-16)TypedReply(18-18)
infrastructure/evault-core/src/core/protocol/vault-access-guard.spec.ts (3)
infrastructure/evault-core/src/core/db/db.service.ts (1)
DbService(18-599)infrastructure/evault-core/src/core/protocol/vault-access-guard.ts (4)
VaultAccessGuard(13-176)VaultContext(7-11)validateToken(21-51)checkAccess(59-102)infrastructure/evault-core/src/test-utils/neo4j-setup.ts (2)
setupTestNeo4j(7-22)teardownTestNeo4j(24-33)
🪛 GitHub Actions: Tests [evault-core]
infrastructure/evault-core/src/services/ProvisioningService.spec.ts
[error] 230-230: Provisioning error: Error: JWT verification failed: Invalid token
[error] 240-240: Provisioning error: verification doesn't exist
[error] 256-256: Provisioning error: verification not approved
[error] 272-272: Provisioning error: already been used
[error] 280-280: Provisioning error: { response: { status: 500, data: { error: 'Internal server error' } } }
infrastructure/evault-core/package.json
[error] 1-1: pnpm test run terminated due to fatal exit in vitest process (exit code 133).
🔇 Additional comments (7)
infrastructure/evault-core/package.json (3)
3-5: Package metadata and scripts updates align well with the new architecture.Version bump to 1.0.0, main entry point relocation to
dist/index.js, and the addition of TypeORM migration scripts all appropriately support the transition to multi-tenant eVault with provisioning.Also applies to: 7-14
16-38: Dependency additions support the multi-tenant architecture effectively.The introduction of TypeORM (
^0.3.24), PostgreSQL driver (pg),reflect-metadata,cors,dotenv, andexpressare well-suited for the provisioning service, database migrations, and multi-tenant GraphQL API described in the PR objectives.
39-52: DevDependencies appropriately support the new test and development tooling.The shift to
vitest, addition of test container support for Neo4j and PostgreSQL, and tooling likets-node-dev,tsx, andnodemonare suitable for the expanded test coverage and development flow required by the multi-tenant architecture.infrastructure/evault-core/src/services/ProvisioningService.spec.ts (2)
76-96: LGTM!Excellent test lifecycle management with proper setup and teardown of database, mock registry server, and environment variables.
165-191: LGTM!This test correctly verifies that the verification record is consumed after provisioning (line 190), confirming the fix for the critical replay attack vulnerability raised in the previous review.
infrastructure/evault-core/src/core/protocol/vault-access-guard.spec.ts (2)
1-56: Excellent test infrastructure setup.The test setup properly handles Neo4j testcontainer integration, JWT key generation for token validation testing, and appropriate mocking of the JWKS endpoint. The cleanup in
afterAllensures test isolation.
225-305: Critical tenant isolation tests - excellent coverage.These tests are essential for validating the multi-tenant architecture introduced in this PR. They verify that
eNameproperly isolates tenant data, preventing cross-tenant access even with public ACLs. This directly addresses the security requirements of the new stateless eVault architecture.
infrastructure/evault-core/src/core/provisioning/config/database.ts
Outdated
Show resolved
Hide resolved
infrastructure/evault-core/src/core/provisioning/config/database.ts
Outdated
Show resolved
Hide resolved
infrastructure/evault-core/src/core/provisioning/services/ProvisioningService.ts
Outdated
Show resolved
Hide resolved
infrastructure/evault-core/src/services/VerificationService.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
infrastructure/evault-core/src/services/VerificationService.spec.ts (1)
9-125: Consider adding tests forfindManyAndCount.The
VerificationServiceincludes afindManyAndCountmethod (visible in the implementation) that handles pagination, ordering, and relations, but it's not covered by tests. Adding tests for this method would improve coverage, especially for scenarios involving multiple records and pagination.infrastructure/evault-core/src/core/provisioning/services/ProvisioningService.ts (6)
45-54: Add try-catch for clearer JWT verification error messages.The reference implementation at
infrastructure/evault-core/src/services/ProvisioningService.tswraps JWT verification in a try-catch block to provide clearer error messages like "JWT verification failed: {reason}". Without this, JWT errors will have generic messages that are harder to debug.Apply this diff to add error handling:
- // Verify the registry entropy token - const jwksResponse = await axios.get( - new URL( - `/.well-known/jwks.json`, - process.env.PUBLIC_REGISTRY_URL - ).toString() - ); - - const JWKS = jose.createLocalJWKSet(jwksResponse.data); - const { payload } = await jose.jwtVerify(registryEntropy, JWKS); + // Verify the registry entropy token + let payload: any; + try { + const jwksResponse = await axios.get( + new URL( + `/.well-known/jwks.json`, + process.env.PUBLIC_REGISTRY_URL + ).toString() + ); + + const JWKS = jose.createLocalJWKSet(jwksResponse.data); + const verified = await jose.jwtVerify(registryEntropy, JWKS); + payload = verified.payload; + } catch (jwtError) { + throw new Error( + `JWT verification failed: ${ + jwtError instanceof Error + ? jwtError.message + : String(jwtError) + }` + ); + }
57-63: Add try-catch for clearer W3ID generation error messages.The reference implementation wraps W3ID generation in a try-catch block to provide clearer error messages. This helps distinguish W3ID generation failures from other errors.
Apply this diff:
- // Generate eName (W3ID) from entropy - const userId = await new W3IDBuilder() - .withNamespace(namespace) - .withEntropy(payload.entropy as string) - .withGlobal(true) - .build(); - - const w3id = userId.id; + // Generate eName (W3ID) from entropy + let w3id: string; + try { + const userId = await new W3IDBuilder() + .withNamespace(namespace) + .withEntropy(payload.entropy as string) + .withGlobal(true) + .build(); + w3id = userId.id; + } catch (w3idError) { + throw new Error( + `Failed to generate W3ID from entropy: ${ + w3idError instanceof Error + ? w3idError.message + : String(w3idError) + }` + ); + }
68-78: Add try-catch around findById to handle invalid UUID format.The reference implementation wraps the findById call in a try-catch to handle database errors (e.g., invalid UUID format) gracefully by treating them as "verification doesn't exist". Without this, invalid UUIDs would produce confusing database error messages.
Apply this diff:
if (verificationId !== demoCode) { - const verification = await this.verificationService.findById(verificationId); + let verification; + try { + verification = await this.verificationService.findById(verificationId); + } catch (dbError) { + throw new Error("verification doesn't exist"); + } if (!verification) {
84-84: Add try-catch for clearer evaultId generation error messages.The reference implementation wraps evaultId generation in a try-catch block to provide clearer error messages.
Apply this diff:
- // Generate evault ID - const evaultId = await new W3IDBuilder().withGlobal(true).build(); + // Generate evault ID + let evaultId: { id: string }; + try { + evaultId = await new W3IDBuilder().withGlobal(true).build(); + } catch (evaultIdError) { + throw new Error( + `Failed to generate evault ID: ${ + evaultIdError instanceof Error + ? evaultIdError.message + : String(evaultIdError) + }` + ); + }
87-87: Consider checking FASTIFY_PORT before PORT.The reference implementation checks
FASTIFY_PORTbeforePORTto ensure the URI matches the actual service port. Without this, there could be a port mismatch if the service runs onFASTIFY_PORT.Apply this diff:
- const baseUri = process.env.EVAULT_BASE_URI || `http://${process.env.EVAULT_HOST || "localhost"}:${process.env.PORT || 4000}`; + const fastifyPort = process.env.FASTIFY_PORT || process.env.PORT || 4000; + const baseUri = process.env.EVAULT_BASE_URI || `http://${process.env.EVAULT_HOST || "localhost"}:${fastifyPort}`;
113-121: Preserve specific error messages for better debugging.The current error handling always returns the generic message "Failed to provision evault instance". The reference implementation preserves specific verification-related error messages (e.g., "verification doesn't exist", "verification not approved") which are more helpful for debugging.
Apply this diff:
} catch (error) { const axiosError = error as AxiosError; + const errorMessage = error instanceof Error ? error.message : String(error); console.error("Provisioning error:", error); + + // Preserve specific verification-related error messages, otherwise use generic message + const verificationErrors = [ + "verification doesn't exist", + "verification not approved", + "already been used", + "PUBLIC_REGISTRY_URL", + ]; + + const isVerificationError = verificationErrors.some((err) => + errorMessage.includes(err) + ); + const message = isVerificationError + ? errorMessage + : "Failed to provision evault instance"; + return { success: false, - error: axiosError.response?.data || axiosError.message, - message: "Failed to provision evault instance", + error: axiosError.response?.data || errorMessage, + message, }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.github/workflows/tests-registry.yml(1 hunks)infrastructure/evault-core/package.json(2 hunks)infrastructure/evault-core/src/controllers/ProvisioningController.ts(1 hunks)infrastructure/evault-core/src/core/provisioning/config/database.ts(1 hunks)infrastructure/evault-core/src/core/provisioning/services/ProvisioningService.ts(1 hunks)infrastructure/evault-core/src/index.ts(1 hunks)infrastructure/evault-core/src/services/ProvisioningService.spec.ts(1 hunks)infrastructure/evault-core/src/services/ProvisioningService.ts(1 hunks)infrastructure/evault-core/src/services/VerificationService.spec.ts(1 hunks)platforms/dreamSync/package.json(1 hunks)platforms/eReputation/package.json(1 hunks)platforms/registry/src/config/database.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- platforms/eReputation/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- platforms/registry/src/config/database.ts
- infrastructure/evault-core/src/services/ProvisioningService.spec.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-16T18:04:28.983Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 99
File: infrastructure/w3id/tests/w3id.test.ts:48-57
Timestamp: 2025-04-16T18:04:28.983Z
Learning: In test files, suggestions to add defensive coding patterns (like extra null checks) are often unnecessary, as tests are designed to fail explicitly when expectations aren't met. The test in `infrastructure/w3id/tests/w3id.test.ts` validates that `id.logs` is an instance of `IDLogManager` before accessing its repository property.
Applied to files:
infrastructure/evault-core/src/services/VerificationService.spec.tsinfrastructure/evault-core/src/services/ProvisioningService.ts
🧬 Code graph analysis (5)
infrastructure/evault-core/src/services/VerificationService.spec.ts (2)
infrastructure/evault-core/src/services/VerificationService.ts (1)
VerificationService(4-50)infrastructure/evault-core/src/test-utils/postgres-setup.ts (2)
setupTestDatabase(10-34)teardownTestDatabase(36-45)
infrastructure/evault-core/src/controllers/ProvisioningController.ts (1)
infrastructure/evault-core/src/services/ProvisioningService.ts (3)
ProvisioningService(22-221)ProvisionRequest(7-12)ProvisionResponse(14-20)
infrastructure/evault-core/src/core/provisioning/services/ProvisioningService.ts (4)
infrastructure/evault-core/src/services/ProvisioningService.ts (3)
ProvisionRequest(7-12)ProvisionResponse(14-20)ProvisioningService(22-221)infrastructure/evault-core/src/core/provisioning/services/VerificationService.ts (1)
VerificationService(4-51)infrastructure/web3-adapter/src/index.js (2)
registryEntropy(62-62)namespace(63-63)infrastructure/w3id/src/w3id.ts (1)
W3IDBuilder(32-158)
infrastructure/evault-core/src/services/ProvisioningService.ts (4)
infrastructure/evault-core/src/core/provisioning/services/VerificationService.ts (1)
VerificationService(4-51)infrastructure/evault-core/src/services/VerificationService.ts (1)
VerificationService(4-50)infrastructure/web3-adapter/src/index.js (2)
registryEntropy(62-62)namespace(63-63)infrastructure/w3id/src/w3id.ts (1)
W3IDBuilder(32-158)
infrastructure/evault-core/src/index.ts (7)
infrastructure/evault-core/src/config/database.ts (1)
AppDataSource(10-18)infrastructure/evault-core/src/core/protocol/graphql-server.ts (1)
GraphQLServer(12-349)infrastructure/evault-core/src/services/ProvisioningService.ts (1)
ProvisioningService(22-221)infrastructure/evault-core/src/core/db/retry-neo4j.ts (1)
connectWithRetry(13-40)infrastructure/evault-core/src/core/db/migrations/add-ename-index.ts (1)
createENameIndex(15-33)infrastructure/evault-core/src/core/db/db.service.ts (1)
DbService(18-599)infrastructure/evault-core/src/core/http/server.ts (1)
registerHttpRoutes(17-301)
🪛 actionlint (1.7.8)
.github/workflows/tests-registry.yml
19-19: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (11)
.github/workflows/tests-registry.yml (2)
37-45: Well-designed native module rebuild strategy.The ssh2 rebuild logic properly addresses platform-specific compatibility by removing pre-built binaries and rebuilding for the current environment. Error handling with
2>/dev/null || trueis appropriate and ensures the workflow proceeds despite missing artifacts.
48-55: Verify Docker and TestContainers configuration.The environment variables are appropriately configured for running tests with TestContainers and Docker. However, confirm that:
TESTCONTAINERS_HOST_OVERRIDE: localhostis correct for GitHub Actions runners- The
DOCKER_HOSTsocket path aligns with the GitHub Actions Docker daemon configurationIf TestContainers tests use PostgreSQL or other containers, verify they work correctly with this configuration.
platforms/dreamSync/package.json (1)
2-2: Package name rename is complete and correctly applied.Verification confirms the dreamSync package.json has been successfully renamed from "rest-express" to "dreamsync" (line 2). No source code files reference the old package name, and no code changes are affected by this metadata-only update. The package-lock.json files still contain the old name as they are generated artifacts; these will reflect the package.json changes when regenerated during the next pnpm install.
infrastructure/evault-core/src/services/VerificationService.spec.ts (4)
1-27: LGTM! Test setup follows best practices.The test infrastructure is well-structured with proper setup/teardown and per-test cleanup for isolation.
29-46: LGTM! Comprehensive test for verification creation.The test validates all provided fields and confirms ID generation.
48-67: LGTM! Good coverage of both positive and negative cases.Tests verify both successful retrieval and null return for non-existent IDs.
102-124: LGTM! Complete coverage of findOne scenarios.Tests verify both successful lookup and null return for non-existent records.
infrastructure/evault-core/src/core/provisioning/services/ProvisioningService.ts (4)
6-19: LGTM!The interface definitions are well-structured and appropriate for the provisioning flow.
21-22: LGTM!The constructor properly injects the VerificationService dependency.
91-106: LGTM!Registry registration is correctly implemented with proper authorization using the shared secret.
108-112: LGTM!The success response properly returns the provisioned w3id and uri.
infrastructure/evault-core/src/core/provisioning/services/ProvisioningService.ts
Show resolved
Hide resolved
Description of change
Issue Number
closes #389
Type of change
How the change has been tested
Unit and Integration tests
Change checklist
Summary by CodeRabbit
New Features
Documentation
Tests
Chores