feat(engine): initialize core packages and generic provider framework - #64
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds new engine and database workspace packages, provider and app_connection Drizzle schemas, a TokenManagerService with Redis-backed refresh locking, an OAuth callback controller and tests, QuickBooks/Salesforce integration starters, shared ESLint/tsconfig tooling, workspace updates, and connector-scaling documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant OAuthProvider as OAuth Provider
participant CallbackCtrl as Callback Controller
participant Session as Session Store
participant Encryption as EncryptionService
participant DB as Database
OAuthProvider->>CallbackCtrl: GET /connect/:provider/callback?code&state
CallbackCtrl->>Session: retrieve grant response
CallbackCtrl->>CallbackCtrl: validate provider & grantResponse
CallbackCtrl->>Encryption: decrypt state -> tenantId/context
CallbackCtrl->>Encryption: encrypt credentials payload
CallbackCtrl->>DB: upsert app_connection (encryptedCredentials, expiresAt, metadata)
DB-->>CallbackCtrl: success/failure
CallbackCtrl-->>OAuthProvider: redirect to success/error page
sequenceDiagram
participant Client as Service
participant TokenMgr as TokenManagerService
participant DB as Database
participant Redis as Redis Lock
participant OAuthVendor as OAuth Vendor
participant Encryption as EncryptionService
Client->>TokenMgr: getValidCredentials(connectionId)
TokenMgr->>DB: fetch app_connection
DB-->>TokenMgr: record (encryptedCredentials, expiresAt)
TokenMgr->>TokenMgr: isExpired? (buffer)
alt expired
TokenMgr->>Redis: try acquire lock (10s TTL)
alt lock acquired
TokenMgr->>Encryption: decrypt payload
TokenMgr->>OAuthVendor: refresh tokens using refresh_token
OAuthVendor-->>TokenMgr: new tokens / error
TokenMgr->>Encryption: encrypt updated payload
TokenMgr->>DB: update app_connection (encryptedCredentials, expiresAt, status)
TokenMgr->>Redis: release lock
TokenMgr-->>Client: decrypted credentials
else lock not acquired
TokenMgr->>TokenMgr: wait/poll DB until refreshed or timeout
TokenMgr-->>Client: decrypted credentials or error
end
else not expired
TokenMgr->>Encryption: decrypt payload
TokenMgr-->>Client: credentials
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 28
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/engine/connections/callback.controller.ts`:
- Around line 54-60: The state parameter (rawState -> tenantId) must be verified
against a server-stored, signed short-lived state token instead of just checking
for non-empty; update the OAuth flow to store a cryptographically signed/nonce
state in the user's session when initiating auth, then in callback.controller.ts
(where grantResponse.raw?.state, rawState and tenantId are read) retrieve and
validate the session-stored signed state (or decrypt/verify the signed token)
and only accept the grant if the incoming rawState matches the verified/stored
value and is not expired—otherwise throw BadRequestException; ensure the
verification logic uses the same signing/verification utilities used when
creating the state and remove the temporary "mocking decryption" behavior.
- Around line 41-42: request.params.provider is being stored directly into
appName (unvalidated) and can contain arbitrary user input; validate provider
against a whitelist of known provider identifiers (e.g., a providers array like
['quickbooks','salesforce']) before assigning to the provider/appName variable
and reject/return an error for anything not in the registry; additionally ensure
you don’t create duplicate tenant+provider rows by converting the insert into an
upsert using Drizzle’s .onConflictDoUpdate(...) on the tenant+provider unique
key (or perform a prior DELETE for that tenant+provider) so re-authentication
updates the existing row rather than inserting a duplicate.
- Around line 80-98: The insert into appConnections in the callback controller
is missing tenant isolation: ensure the app_connection schema (e.g., in
tenant.ts / appConnections definition) contains a tenantId column (or implement
schema-per-tenant/row-level security per architecture), then include tenantId in
the db.insert(appConnections).values({...}) payload and run the insert inside
TenantContext.run({ tenantId }, async () => { ... }) so the row is stored and
scoped to the correct tenant; update any related model types/migrations and
ensure queries filter by tenantId or use the tenant schema context accordingly.
- Around line 57-59: The tenantId null-check currently throws a Nest
BadRequestException which breaks the browser redirect UX; instead, change the
branch to perform the same redirect-flow used by other error paths: build the
redirect URL with the same query-string error code pattern and call
res.redirect(...) (or return a RedirectResponse) so the user's browser is sent
back with the error in the query string rather than a JSON 400. Update the
branch that checks tenantId (in CallbackController's callback handler where the
tenantId variable is validated) to mirror the existing redirect logic used
elsewhere in this method so it uses the same error query parameter and state
handling.
- Around line 69-71: The code is currently using Buffer.toString('base64') which
is only encoding, not encryption, leaving OAuth tokens in plain text; replace
the mock encoding in callback.controller.ts (the const encryptedPayload
assignment) with a real symmetric encryption call via the existing
EncryptionService (use its encrypt method backed by AES-256-GCM with
KMS/HSM-managed keys) and persist the returned ciphertext into the
encryptedCredentials column; also update any corresponding read/decrypt paths to
call EncryptionService.decrypt before using credentials so tokens are never
stored or read in plaintext.
In `@docs/architecture/connector_scaling_architecture.md`:
- Around line 136-187: Update the incorrect package namespaces in the example:
replace imports of EncryptionService and TenantContext from
"@fluxnex/core-kernel", db from "@fluxnex/database", and appConnections from
"@fluxnex/database-schema/tenant" with the correct "@nexiom/*" equivalents so
the OAuthCallbackController example (class OAuthCallbackController,
handleCallback method) imports from "@nexiom/core-kernel", "@nexiom/database",
and "@nexiom/database-schema/tenant" respectively; keep the same named symbols
(EncryptionService, TenantContext, db, appConnections) and behavior intact.
- Around line 279-289: refreshWithLock currently uses recursion by calling
this.getValidCredentials(connection.id) when the lock is held, risking stack
overflow and retry storms under high concurrency; change this behavior to an
iterative, bounded retry loop inside refreshWithLock (or the caller) instead of
recursive calls: detect lock contention (lockKey), sleep/backoff (e.g.,
incremental or jittered delay), limit max attempts and return an error or fresh
credentials after the loop, and ensure you call the non-recursive path to
refresh credentials (referencing refreshWithLock and getValidCredentials) so no
new stack frames are added on each retry.
In `@docs/architecture/connector_scaling_strategy.md`:
- Around line 55-57: The document ends mid-sentence and omits remaining Phase 2
tasks and Phases 3–4; finish the truncated line by completing the sentence about
the "Token Expiration Checker" in "Layer 5 (Delivery Worker)", then expand Phase
2 to enumerate remaining tasks (e.g., implement retry/backoff, monitoring hooks,
token refresh integration, tests), and add clear Phase 3 and Phase 4 sections
with goals, deliverables, and timelines (e.g., scaling validation, chaos
testing, production rollout, observability improvements). Reference the existing
phrase "Token Expiration Checker" and "Layer 5 (Delivery Worker)" when placing
the completed sentence so it reads naturally, and ensure each phase has a short
bullet list of tasks and expected outcomes.
In `@integrations/quickbooks/src/auth/config.ts`:
- Line 1: Both config files import GenericCredentialType via a deep path
('@nexiom/engine/src/connectivity/types') which breaks consumers of the packaged
library; update the import to use the engine's public API (import
GenericCredentialType from '@nexiom/engine' or the public re-export) in the auth
config files that reference GenericCredentialType so they no longer rely on
internal src/ paths.
In `@integrations/salesforce/package.json`:
- Around line 1-15: Add "private": true to package.json to prevent accidental
npm publication and update the devDependencies.typescript entry to a current
stable TypeScript release (replace "typescript": "^5.3.3" with a newer pinned
range, e.g., "^5.5.0" or the project's chosen supported version) so the package
uses up-to-date type-checking features; modify the package.json top-level fields
"private" and "devDependencies.typescript" accordingly.
In `@integrations/salesforce/src/auth/config.ts`:
- Line 1: The code is using deep imports from package internals; update the
import specifiers to the public package entry points: replace any import of
GenericCredentialType from '@nexiom/engine/src/connectivity/types' with an
import from '@nexiom/engine' (e.g., the GenericCredentialType import in the
salesforce and quickbooks auth config files), and replace the import from
'@nexiom/identity/src/schema' with '@nexiom/identity' (the schema import in the
API schema file); after changing the specifiers, run the TypeScript
build/typecheck to ensure the exported symbols resolve correctly.
In `@packages/database/package.json`:
- Around line 1-18: Add a "private": true field to the package.json for
`@nexiom/database` and update the devDependencies.typescript version to a later
release (replace the pinned "^5.3.3" with a current workspace-aligned version)
so the package is non-publishable and uses an upgraded TypeScript; modify the
top-level JSON to include "private": true and change the "typescript" value
under "devDependencies" (the package name "@nexiom/database" and the fields
"private" and "devDependencies.typescript" identify where to make the edits).
In `@packages/database/src/client.ts`:
- Around line 5-7: The Pool is created without sizing/timeouts which can cause
queued requests; update the Pool construction (the pool constant created from
new Pool) to pass explicit options for max, idleTimeoutMillis, and
connectionTimeoutMillis (use environment variables like DATABASE_POOL_MAX,
DATABASE_IDLE_TIMEOUT_MS, DATABASE_CONN_TIMEOUT_MS with sensible defaults) so
the pool has a bounded connection count and timeouts in production; ensure you
update any export or initialization around Pool/pool to use these new options.
- Around line 5-7: The code currently falls back to hardcoded DB credentials and
omits the schema when initializing drizzle; change the Pool creation to require
process.env.DATABASE_URL (use Pool({ connectionString: process.env.DATABASE_URL
})) and if DATABASE_URL is missing throw a clear error (e.g. throw new
Error("DATABASE_URL is required")), and update the drizzle initialization to
pass the schema option (e.g. drizzle(pool, { schema })) importing or referencing
the exported schema object so db.query.appConnections.findFirst(...) works at
runtime; ensure you only remove the insecure fallback and add the schema
reference where drizzle(...) is called.
- Around line 9-10: Replace the `@ts-ignore` usage by importing the schema from
./schema/tenant and pass it as the second argument to drizzle so the call
becomes drizzle(pool, { schema }); specifically, remove the // `@ts-ignore` before
export const db = drizzle(pool), add an import for the schema from
"./schema/tenant", and update the export line to export const db = drizzle(pool,
{ schema }) so it matches other usages (db, drizzle, pool, schema).
In `@packages/database/src/schema/tenant.ts`:
- Line 19: The updatedAt column uses
timestamp('updated_at').defaultNow().notNull() which only sets the value on
INSERT; update operations won't refresh it—add the Drizzle on-update hook by
chaining .$onUpdate(() => new Date()) to the updatedAt column definition (i.e.,
change updatedAt: timestamp('updated_at').defaultNow().notNull() to include
.$onUpdate(() => new Date())) so the timestamp auto-updates on every UPDATE;
ensure your environment uses Drizzle v0.30.5+ which provides .$onUpdate.
- Around line 18-19: The createdAt and updatedAt column definitions are missing
the { withTimezone: true } option (unlike expiresAt) causing them to be stored
without time zone; update the timestamp calls for createdAt and updatedAt to use
timestamp('created_at', { withTimezone: true }) and timestamp('updated_at', {
withTimezone: true }) (keeping defaultNow() and notNull()) so all audit
timestamps (createdAt, updatedAt, expiresAt) consistently include time zone
handling.
- Around line 3-23: The app_connection table (pgTable declared as
appConnections) is missing a tenant identifier, so add a tenantId column (e.g.,
uuid('tenant_id').notNull()) to the pgTable definition and make it part of
relevant indexes (create index('tenant_app_idx').on(table.tenantId,
table.appName) and index on tenantId for efficient tenant-scoped queries);
update any default values/constraints and ensure code that inserts/queries
appConnections supplies tenantId so rows are tenant-isolated (this will also
enable applying row-level security later).
- Around line 20-23: The schema uses the deprecated object-return index callback
form — update the tenant table index callback to the array-return form used
elsewhere: replace the (table) => ({ appNameIdx: ..., statusIdx: ... }) pattern
with (table) => [ index(...).on(table.appName), index(...).on(table.status) ] so
that the index creators (index) are returned as an array instead of named
properties (appNameIdx, statusIdx); update the callback around the tenant table
definition in tenant.ts accordingly to remove the TypeScript deprecation
warnings.
In `@packages/engine/package.json`:
- Around line 9-13: The package.json is missing the `@nestjs/common` dependency
required by TokenManagerService (which imports Injectable and Logger); update
this package's package.json to include "@nestjs/common" either under
"dependencies" if this package should bundle NestJS or under "peerDependencies"
(and optionally "devDependencies") if consumers will provide NestJS, so
TokenManagerService's imports (Injectable, Logger) resolve reliably during
build/runtime.
In `@packages/engine/src/connectivity/token-manager.service.ts`:
- Around line 96-103: The revocation branch in the catch block of
TokenManagerService currently assumes an axios-style error
(error.response.status); update the error handling so revocation is detected
reliably by normalizing errors from OAuthRefreshClient: modify
OAuthRefreshClient to throw a typed/normalized error object (e.g., include a
numeric status or code property) on HTTP failures, and then change the catch in
token-manager.service.ts (the block that calls db.update(appConnections) and
logs "Token refresh rejected. Marked connection as REVOKED.") to check that
normalized property (e.g., error.status) instead of error.response.status so
revoked tokens are correctly detected regardless of underlying HTTP lib.
- Around line 24-29: The constructor in TokenManagerService directly
instantiates dependencies (new Redis(...), new EncryptionService(), new
OAuthRefreshClient()) which bypasses NestJS DI and lifecycle; change
TokenManagerService to accept these as constructor-injected providers (e.g.,
inject a Redis client/token store, EncryptionService, and OAuthRefreshClient)
using Nest decorators or typed constructor params instead of new, update the
module to register/provide those providers (and configure Redis URL via config),
and implement/ensure onModuleDestroy or a close method on TokenManagerService
(or the injected Redis provider) is used to call disconnect/quit to avoid
leaking TCP connections.
- Around line 84-86: The code computes expiresAt using newTokens.expires_in
which may be undefined and yield NaN; update the logic around
newTokens/expires_in in the token creation flow so you normalize and defend
against missing values (e.g. compute a local expiresIn = typeof
newTokens.expires_in === 'number' ? newTokens.expires_in : null and then set
expiresAt = expiresIn ? new Date(Date.now() + expiresIn * 1000) : null) and
ensure updatedPayload (and any DB write that uses expiresAt) receives a safe
value (null or omitted) rather than NaN; adjust any downstream handling that
expects a Date accordingly.
- Around line 7-10: The mocked EncryptionService (class EncryptionService with
methods encrypt and decrypt) uses base64 which is reversible and unsafe for
storing credentials; update it to prevent accidental use in non-development
environments by adding a runtime guard that throws when NODE_ENV is "production"
(or when a config flag like USE_MOCK_ENCRYPTION is false), and add a prominent
TODO(SECURITY) comment above the class and/or method declarations referencing
encryptedCredentials to make CI greppable; ensure the guard triggers before
encrypt/decrypt run so credentials cannot be encoded/decoded in production.
- Around line 104-107: The finally block unconditionally calls
this.redis.del(lockKey) which can delete a lock another process acquired after
TTL expiry; instead store a unique lock value when acquiring the lock (e.g.,
lockValue) and replace the unconditional delete with a value-guarded delete
using a Redis EVAL/LUA script that compares the current value at lockKey to
lockValue and only deletes if they match; update the code paths around lock
acquisition (where lockKey is set) to generate and save the unique value and
call the compare-and-del Lua via this.redis.eval / this.redis.evalsha so only
the owner releases the lock.
- Around line 61-65: getValidCredentials currently does an unbounded recursive
retry when lockAcquired is false (calling itself after a setTimeout), which
risks stack overflow and retry storms; replace the recursive pattern in
getValidCredentials/refreshWithLock with an iterative loop that attempts to
acquire the lock up to a configurable maxAttempts (e.g., MAX_LOCK_RETRIES) with
an await sleep/backoff between attempts, and if still unable to acquire the lock
return a clear error or rejected Promise (or propagate a specific exception)
instead of recursing; update related log lines (this.logger.debug) to include
attempt counts and ensure refreshWithLock respects the same iterative
retry/timeout behavior.
In `@packages/engine/src/connectivity/types.ts`:
- Around line 13-21: GenericCredentialType currently allows oauth to be optional
regardless of authType, so enforce the invariant by replacing
GenericCredentialType with a discriminated union: define one interface (e.g.,
OAuthCredential) with authType: 'OAUTH2' and a required oauth field
(authorizeUrl, tokenUrl, optional scope) and another interface (e.g.,
ApiKeyCredential) with authType: 'API_KEY' and no oauth, then export type
GenericCredentialType = OAuthCredential | ApiKeyCredential; update any
references to ConnectorAuthSchema or uiSchema to remain optional on both
variants and adjust code that constructs or narrows GenericCredentialType to use
the authType discriminator when accessing oauth.
In `@packages/engine/tsconfig.json`:
- Around line 1-15: The package tsconfig.json files are duplicated and missing
project references support; create a root shared tsconfig.base.json with the
common compilerOptions (target, module, declaration, outDir, strict,
esModuleInterop, skipLibCheck, forceConsistentCasingInFileNames, include/src
pattern) and then update each package tsconfig.json (e.g.,
packages/engine/tsconfig.json, packages/database, integrations/salesforce,
integrations/quickbooks) to extend that base and add "compilerOptions": {
"composite": true } so TypeScript project references work for Turborepo; ensure
each package keeps any package-specific settings but otherwise inherits from
tsconfig.base.json and remove the duplicated options from the package files.
---
Duplicate comments:
In `@integrations/quickbooks/package.json`:
- Around line 1-15: The package.json for `@nexiom/quickbooks` is missing
"private": true and uses a TypeScript devDependency that may be inconsistent
with the rest of the monorepo; update the package.json to add "private": true at
the root of the JSON and align the "typescript" version in devDependencies with
the project standard (match the version used by `@nexiom/salesforce/root`
workspace), ensuring "scripts.build" and the "main"/"types" fields remain
unchanged.
In `@integrations/quickbooks/tsconfig.json`:
- Around line 1-15: This tsconfig.json duplicates settings already in
packages/engine/tsconfig.json; extract the shared compilerOptions into a single
base tsconfig (e.g., base-tsconfig.json) and have this file use "extends" to
reuse those settings, and enable "composite": true under "compilerOptions" for
incremental builds; specifically update this file to remove the duplicated
options and reference the base via "extends", add "composite": true in
compilerOptions, and ensure the project uses proper "references" if part of a
composite build.
In `@integrations/salesforce/tsconfig.json`:
- Around line 1-15: This tsconfig.json duplicates settings from packages/engine;
extract shared options into a base config and have this file extend it: replace
the duplicated "compilerOptions" entries with an "extends":
"../../tsconfig.base.json" and keep only any package-specific overrides (e.g.,
"include": ["src/**/*"]) here; also add "composite": true to the package-local
compilerOptions if this project is part of a project-reference build so ensure
the tsconfig in integrations/salesforce includes "compilerOptions": {
"composite": true } when needed.
In `@packages/database/tsconfig.json`:
- Around line 1-15: The packages/database tsconfig.json duplicates
packages/engine config; extract shared settings into a common base (e.g., create
a root tsconfig.base.json with the compilerOptions currently duplicated) and
have packages/database/tsconfig.json extend that base, then enable "composite":
true in the package tsconfig (and in the base if preferred) and add proper
"references" to support project references; update the "include"/outDir as
needed but remove duplicated options from packages/database/tsconfig.json so it
simply extends the shared config and sets only package-specific overrides.
Comment on lines
+54
to
+60
| // 2. Extract and Validate State (Tenant Context) | ||
| const rawState = grantResponse.raw?.state; | ||
| const tenantId = rawState; // Mocking decryption for now | ||
|
|
||
| if (!tenantId) { | ||
| throw new BadRequestException('Invalid State/Tenant Context'); | ||
| } |
Contributor
There was a problem hiding this comment.
OAuth state parameter is not authenticated — this is an OAuth CSRF attack surface.
The state parameter is passed round-trip through the user's browser. An attacker who can influence the state value (CSRF, open-redirect, or state fixation) can associate a victim's grant with an arbitrary tenant context. RFC 6749 §10.12 requires the state value to be unguessable and verified against a value stored server-side before any token exchange is accepted.
The current code only checks that the value is non-empty:
const tenantId = rawState; // Mocking decryption for now
if (!tenantId) { throw ... }The required fix is to (a) store a signed, short-lived state token in the session when initiating the OAuth flow and (b) verify the incoming state against that stored value here before proceeding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/modules/engine/connections/callback.controller.ts` around lines
54 - 60, The state parameter (rawState -> tenantId) must be verified against a
server-stored, signed short-lived state token instead of just checking for
non-empty; update the OAuth flow to store a cryptographically signed/nonce state
in the user's session when initiating auth, then in callback.controller.ts
(where grantResponse.raw?.state, rawState and tenantId are read) retrieve and
validate the session-stored signed state (or decrypt/verify the signed token)
and only accept the grant if the incoming rawState matches the verified/stored
value and is not expired—otherwise throw BadRequestException; ensure the
verification logic uses the same signing/verification utilities used when
creating the state and remove the temporary "mocking decryption" behavior.
Comment on lines
+80
to
+98
| // 5. Save to Database (Strictly within Tenant Context) | ||
| // await TenantContext.run({ tenantId }, async () => { | ||
| try { | ||
| await db.insert(appConnections).values({ | ||
| appName: provider, | ||
| authType: 'OAUTH2', | ||
| encryptedCredentials: encryptedPayload, | ||
| expiresAt: expiresAt, | ||
| metadata: { realmId: grantResponse.raw?.realmId }, // App-specific metadata extraction | ||
| }); | ||
| this.logger.log( | ||
| `Successfully stored credentials for ${provider}, tenant: ${tenantId}`, | ||
| ); | ||
| } catch (error) { | ||
| this.logger.error(`Failed to store credentials for ${provider}`, error); | ||
| res.redirect(`/app/connections?error=internal_error`); | ||
| return; | ||
| } | ||
| // }); |
Contributor
There was a problem hiding this comment.
tenantId is extracted but never stored — credentials have zero tenant isolation.
After validation on Line 56-60 the tenantId is simply dropped. The db.insert(appConnections).values({...}) call (Line 83) contains no tenant identifier, and the app_connection table schema has no tenant column. This means all tenant credentials are mixed in a single global table with no way to scope reads by tenant. Any service that queries appConnections without a tenant filter will see credentials belonging to other tenants.
The architecture document mandates tenant_{id} schema isolation via TenantContext.run(...). Neither the schema nor the controller implements this — the tenantId column must be added to the table (or the row-level security / schema-per-tenant approach from the architecture doc must be applied) and the insert must include it.
🔧 Minimum fix (add tenantId to schema and insert)
In packages/database/src/schema/tenant.ts:
export const appConnections = pgTable('app_connection', {
id: uuid('id').defaultRandom().primaryKey(),
+ tenantId: varchar('tenant_id', { length: 100 }).notNull(),
appName: varchar('app_name', { length: 100 }).notNull(),In the controller insert:
await db.insert(appConnections).values({
+ tenantId,
appName: provider,
authType: 'OAUTH2',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/modules/engine/connections/callback.controller.ts` around lines
80 - 98, The insert into appConnections in the callback controller is missing
tenant isolation: ensure the app_connection schema (e.g., in tenant.ts /
appConnections definition) contains a tenantId column (or implement
schema-per-tenant/row-level security per architecture), then include tenantId in
the db.insert(appConnections).values({...}) payload and run the insert inside
TenantContext.run({ tenantId }, async () => { ... }) so the row is stored and
scoped to the correct tenant; update any related model types/migrations and
ensure queries filter by tenantId or use the tenant schema context accordingly.
Comment on lines
+1
to
+15
| { | ||
| "compilerOptions": { | ||
| "target": "ES2022", | ||
| "module": "CommonJS", | ||
| "declaration": true, | ||
| "outDir": "./dist", | ||
| "strict": true, | ||
| "esModuleInterop": true, | ||
| "skipLibCheck": true, | ||
| "forceConsistentCasingInFileNames": true | ||
| }, | ||
| "include": [ | ||
| "src/**/*" | ||
| ] | ||
| } No newline at end of file |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract a shared base tsconfig.json and add composite: true for project references.
All four package tsconfigs (packages/engine, packages/database, integrations/salesforce, integrations/quickbooks) are identical. This is a DRY violation and creates a maintenance burden — any future change (e.g., adding paths, changing moduleResolution) must be replicated across all four files.
The standard monorepo pattern is a root tsconfig.base.json that each package extends. Additionally, composite: true is required for TypeScript project references, which Turborepo relies on for accurate dependency graph traversal and incremental builds. The PR description states "Ensures Turborepo layer compliance" — this is currently incomplete without it.
♻️ Proposed approach
Root tsconfig.base.json (new file at repo root):
+{
+ "compilerOptions": {
+ "target": "ES2022",
+ "module": "CommonJS",
+ "moduleResolution": "node",
+ "declaration": true,
+ "declarationMap": true,
+ "outDir": "./dist",
+ "rootDir": "./src",
+ "composite": true,
+ "strict": true,
+ "esModuleInterop": true,
+ "skipLibCheck": true,
+ "forceConsistentCasingInFileNames": true
+ }
+}Each package tsconfig.json (e.g., packages/engine/tsconfig.json):
-{
- "compilerOptions": {
- "target": "ES2022",
- "module": "CommonJS",
- "declaration": true,
- "outDir": "./dist",
- "strict": true,
- "esModuleInterop": true,
- "skipLibCheck": true,
- "forceConsistentCasingInFileNames": true
- },
- "include": [
- "src/**/*"
- ]
-}
+{
+ "extends": "../../tsconfig.base.json",
+ "compilerOptions": {
+ "outDir": "./dist",
+ "rootDir": "./src"
+ },
+ "include": ["src/**/*"]
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/engine/tsconfig.json` around lines 1 - 15, The package tsconfig.json
files are duplicated and missing project references support; create a root
shared tsconfig.base.json with the common compilerOptions (target, module,
declaration, outDir, strict, esModuleInterop, skipLibCheck,
forceConsistentCasingInFileNames, include/src pattern) and then update each
package tsconfig.json (e.g., packages/engine/tsconfig.json, packages/database,
integrations/salesforce, integrations/quickbooks) to extend that base and add
"compilerOptions": { "composite": true } so TypeScript project references work
for Turborepo; ensure each package keeps any package-specific settings but
otherwise inherits from tsconfig.base.json and remove the duplicated options
from the package files.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/engine/connections/callback.controller.spec.ts`:
- Around line 170-191: The controller's handleCallback currently encrypts and
persists credentials without validating presence of an access_token; add a guard
in the handleCallback method to check the OAuth payload (e.g.,
request.grant.response.raw or the object passed into encryptionService.encrypt)
for a defined access_token and, if missing, immediately redirect to
'/app/connections?error=invalid_credentials' (or consistent error name) instead
of proceeding to encrypt/persist; update or add a unit test in
callback.controller.spec.ts to assert that when access_token is absent the
controller redirects with the chosen error and does not call
encryptionService.encrypt or the DB persistence path (references:
handleCallback, encryptionService.encrypt, mockOnConflictDoUpdate).
In `@apps/api/src/modules/engine/connections/callback.controller.ts`:
- Line 119: Remove the stale trailing comment "// });" from the
CallbackController file — locate the leftover comment token "// });" in
callback.controller.ts (near the callback handling / TenantContext wrapper area)
and delete it so there are no orphaned closing-comment artifacts left in the
method or controller block.
- Around line 70-75: The credentials object currently includes grantResponse.raw
which duplicates tokens and may contain extra provider fields; update the
credentials creation in the callback controller to omit rawResponse (or extract
only the minimal provider fields you actually need) so that credentials only
contain accessToken: grantResponse.access_token, refreshToken:
grantResponse.refresh_token (and any explicitly selected raw fields if required)
instead of rawResponse: grantResponse.raw.
- Around line 100-109: The upsert using onConflictDoUpdate(...) targeting
appConnections.tenantId and appConnections.appName will fail because the
composite index tenant_app_name_idx is a non-unique index; change the schema to
create a unique constraint: in the table definition that declares
tenant_app_name_idx replace index(...) with uniqueIndex(...) (and add/import
uniqueIndex from the pg-core/drizzle import list) so the composite columns
become a unique index usable by onConflictDoUpdate; keep the same column list
(tenantId, appName) and ensure the index name remains tenant_app_name_idx.
In `@apps/api/vitest.config.mts`:
- Line 50: Replace the `// NOTE: Restore to 90% after PBAC refactor stabilizes
(Technical Debt: ISSUE-123)` comment with a proper `// TODO:` entry so automated
tools pick it up; update the comment to read `// TODO: Restore to 90% after PBAC
refactor stabilizes (ISSUE-123)` and ensure the task/issue id is included
exactly as `ISSUE-123` so linters/IDE task trackers and CI hooks will surface
the technical debt for follow-up (file: vitest.config.mts, comment near the
coverage-threshold note).
In `@integrations/quickbooks/eslint.config.mjs`:
- Around line 1-48: The eslint.config.mjs duplicates configuration across
integration packages; extract the shared settings (plugins, extends:
eslint.configs.recommended, tseslint.configs.recommendedTypeChecked,
eslintPluginPrettierRecommended, shared languageOptions.globals and rules) into
a new reusable preset (e.g. `@nexiom/eslint-config`) and have integrations call
tseslint.config with that preset plus only per-package overrides; specifically
move the common rules and plugins out of
integrations/quickbooks/eslint.config.mjs and keep only the package-specific
parserOptions.tsconfigRootDir (using __dirname) and any unique ignores or
overrides so integrations import/extend the shared preset and pass their
tsconfigRootDir into tseslint.config.
In `@packages/database/src/client.ts`:
- Line 1: Update the stale inline comment "// Placeholder for Drizzle Client" in
client.ts to reflect that this module is a fully implemented Drizzle client (or
remove it entirely); locate the placeholder comment in the top of the module and
either replace it with a brief, accurate description of the module’s
responsibilities (e.g., "Drizzle database client and helpers") or delete the
comment so the file header matches the implemented functionality.
In `@packages/database/src/schema/tenant.ts`:
- Line 5: The tenantId column currently only has a comment; update its column
declaration (tenantId: uuid('tenant_id').notNull()) to include a DB-level
foreign key by adding .references(() => tenants.id, { onDelete: "cascade" }) so
the schema enforces referential integrity for tenants.id; if the tenants table
is defined in a different module, instead add the FK in a migration that
references the tenants table.
In `@packages/engine/package.json`:
- Around line 1-18: Add a top-level "private": true flag to the package.json for
the `@nexiom/engine` package to prevent accidental npm publication; open the
package.json (the object containing "name": "@nexiom/engine", "version":
"1.0.0", etc.) and insert "private": true alongside the existing fields so the
package is treated as private by npm.
In `@packages/engine/src/connectivity/token-manager.service.ts`:
- Line 2: TokenManagerService currently imports the Drizzle DB as a module
singleton (db) which bypasses NestJS DI and causes test/lifecycle/typing issues;
change it to accept the Drizzle DB via dependency injection (mirror the Redis
pattern) by adding an injected provider (e.g., use `@Inject`('DRIZZLE_DB') private
readonly db: DrizzleClient or similar) into the TokenManagerService constructor
and update usages (db.query.*, db.update(...)) to reference the injected this.db
so you can remove the module import and the `@ts-ignore` annotations and enable
proper typing, lifecycle control, and easier mocking in tests.
- Around line 6-28: The file currently exports concrete stub classes
(EncryptionService, OAuthRefreshClient, OAuthRefreshError) alongside the real
TokenManagerService which creates confusion for consumers; change these exported
stubs so they no longer appear in the public API by either moving the stub
implementations into a separate test-only barrel (e.g., __mocks__ or testing)
and removing their exports from the package public index, or convert the stubs
into abstract classes or interfaces (e.g., make EncryptionService and
OAuthRefreshClient abstract or replace them with interfaces and keep
OAuthRefreshError as a typed export if needed) so callers are forced to provide
real DI implementations; also update any references in TokenManagerService to
use the abstract/interface types and update the package export barrel to stop
re-exporting the concrete test stubs.
- Around line 75-86: The retry loop that runs when lockAcquired is false should,
after successfully acquiring the lock (via redis.set using lockKey/lockValue),
re-fetch the latest connection record from the DB (the same source used earlier
for connection) and check the stored token/refresh timestamp on that connection
before proceeding; if the tokens were already updated by the other worker, skip
the refresh and release the lock instead of performing another rotation. Update
the logic around lockKey/lockValue, lock acquisition (redis.set), and the code
that currently calls the refresh routine (the token refresh path in
TokenManagerService / the method handling connection refresh) to re-read the
connection by id (connection.id), compare token/updatedAt (or equivalent
fields), and only execute the refresh when the stored credentials are still
stale. Ensure lock release happens in both branches.
In `@tsconfig.base.json`:
- Around line 1-14: Add "declarationMap": true to the compilerOptions in the
tsconfig base so project references produce .d.ts.map files and editors can "Go
to Definition" into the original TypeScript source of referenced packages;
update the compilerOptions block (which currently contains "target", "module",
"declaration", "composite", "experimentalDecorators", "emitDecoratorMetadata",
etc.) to include the "declarationMap" flag alongside "declaration": true.
---
Duplicate comments:
In `@packages/database/package.json`:
- Around line 1-18: This package.json for the workspace package
"@nexiom/database" is missing the top-level "private" field; add "private": true
to the package.json root to prevent accidental npm publication (ensure the value
is the boolean true, not a string), placing it alongside existing top-level
fields like "name" and "version".
In `@packages/engine/package.json`:
- Around line 9-14: The package now lists `@nestjs/common` as a dependency but
should explicitly declare the NestJS runtime contract via peerDependencies so
the host app provides `@nestjs/core` and reflect-metadata; update packages/engine
package.json to add a "peerDependencies" entry that includes "@nestjs/core" and
"reflect-metadata" (with appropriate version ranges compatible with
`@nestjs/common`) and remove or keep `@nestjs/common` as needed, ensuring the
library expects the host application to satisfy those runtime packages.
In `@packages/engine/tsconfig.json`:
- Around line 1-16: tsconfig.json correctly extends "../../tsconfig.base.json"
and declares the project reference to "../database"; ensure the shared base
(referenced by "extends") contains "composite": true so incremental builds work,
verify "compilerOptions" keys rootDir/outDir/tsBuildInfoFile are correct for
build output, and if you add more workspaces add them to the "references" array
(e.g., add another entry with "path": "../your-package") so TypeScript project
references remain accurate.
Comment on lines
+170
to
+191
| it('should redirect with internal_error if database insert fails', async () => { | ||
| mockEncryptionService.decrypt.mockResolvedValue('tenant-123'); | ||
| mockEncryptionService.encrypt.mockResolvedValue('encrypted-credentials'); | ||
|
|
||
| // Override the mock to simulate failure | ||
| mockOnConflictDoUpdate.mockRejectedValueOnce(new Error('DB Error')); | ||
|
|
||
| const req = mockRequest('salesforce', { | ||
| grant: { | ||
| response: { | ||
| raw: { state: 'encrypted-state', expires_in: 3600 }, | ||
| }, | ||
| }, | ||
| }); | ||
| const res = mockResponse(); | ||
|
|
||
| await controller.handleCallback(req as Request, res as Response); | ||
|
|
||
| expect(res.redirect).toHaveBeenCalledWith( | ||
| '/app/connections?error=internal_error', | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
Test exposes a controller gap: no validation that access_token exists before encryption.
This test's mock request omits access_token and refresh_token, yet the controller happily encrypts { accessToken: undefined, refreshToken: undefined, ... } and persists it. The test passes because the controller never validates token presence. Consider adding a guard in the controller to redirect with an error when access_token is missing — and adding a corresponding test case here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/modules/engine/connections/callback.controller.spec.ts` around
lines 170 - 191, The controller's handleCallback currently encrypts and persists
credentials without validating presence of an access_token; add a guard in the
handleCallback method to check the OAuth payload (e.g.,
request.grant.response.raw or the object passed into encryptionService.encrypt)
for a defined access_token and, if missing, immediately redirect to
'/app/connections?error=invalid_credentials' (or consistent error name) instead
of proceeding to encrypt/persist; update or add a unit test in
callback.controller.spec.ts to assert that when access_token is absent the
controller redirects with the chosen error and does not call
encryptionService.encrypt or the DB persistence path (references:
handleCallback, encryptionService.encrypt, mockOnConflictDoUpdate).
Comment on lines
+100
to
+109
| .onConflictDoUpdate({ | ||
| target: [appConnections.tenantId, appConnections.appName], | ||
| set: { | ||
| encryptedCredentials: encryptedPayload, | ||
| expiresAt: expiresAt, | ||
| metadata: { realmId: grantResponse.raw?.realmId }, | ||
| status: 'ACTIVE', | ||
| updatedAt: new Date(), | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the tenant schema file
echo "=== Looking for schema files ==="
find . -name "*.ts" -path "*/schema/*" | grep -i tenant
# Examine the tenant schema for the index definition
echo ""
echo "=== Checking tenant schema for index definition ==="
if [ -f "packages/database/src/schema/tenant.ts" ]; then
cat -n "packages/database/src/schema/tenant.ts" | grep -A5 -B5 "tenant_app_name_idx"
fi
# Check imports in the schema file
echo ""
echo "=== Checking imports in tenant schema ==="
if [ -f "packages/database/src/schema/tenant.ts" ]; then
head -20 "packages/database/src/schema/tenant.ts"
fiRepository: pramodnarayana/nexiom
Length of output: 1783
🏁 Script executed:
#!/bin/bash
# Check the callback controller to confirm onConflictDoUpdate usage
echo "=== Checking callback controller for onConflictDoUpdate ==="
if [ -f "apps/api/src/modules/engine/connections/callback.controller.ts" ]; then
cat -n "apps/api/src/modules/engine/connections/callback.controller.ts" | grep -A10 -B5 "onConflictDoUpdate"
fiRepository: pramodnarayana/nexiom
Length of output: 812
onConflictDoUpdate requires a unique constraint, but the schema only defines a regular index.
The upsert targets [appConnections.tenantId, appConnections.appName] (line 101), but the composite index tenant_app_name_idx in packages/database/src/schema/tenant.ts is created with index() — not uniqueIndex(). Drizzle's onConflictDoUpdate() with a target parameter maps to PostgreSQL's ON CONFLICT (columns) DO UPDATE, which requires a unique index or constraint. This will throw a runtime error when the upsert executes.
Fix: change to a unique index in the schema
In packages/database/src/schema/tenant.ts, update the index and imports:
-import { pgTable, uuid, varchar, text, timestamp, jsonb, index } from 'drizzle-orm/pg-core';
+import { pgTable, uuid, varchar, text, timestamp, jsonb, index, uniqueIndex } from 'drizzle-orm/pg-core';- index('tenant_app_name_idx').on(table.tenantId, table.appName),
+ uniqueIndex('tenant_app_name_idx').on(table.tenantId, table.appName),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/modules/engine/connections/callback.controller.ts` around lines
100 - 109, The upsert using onConflictDoUpdate(...) targeting
appConnections.tenantId and appConnections.appName will fail because the
composite index tenant_app_name_idx is a non-unique index; change the schema to
create a unique constraint: in the table definition that declares
tenant_app_name_idx replace index(...) with uniqueIndex(...) (and add/import
uniqueIndex from the pg-core/drizzle import list) so the composite columns
become a unique index usable by onConflictDoUpdate; keep the same column list
(tenantId, appName) and ensure the index name remains tenant_app_name_idx.
| res.redirect(`/app/connections?error=internal_error`); | ||
| return; | ||
| } | ||
| // }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove stale comment.
// }); appears to be a leftover from previously removed code wrapping (e.g., a TenantContext.run block).
🧹 Remove
- // });
-
res.redirect(`/app/connections?success=true`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // }); | |
| res.redirect(`/app/connections?success=true`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/modules/engine/connections/callback.controller.ts` at line 119,
Remove the stale trailing comment "// });" from the CallbackController file —
locate the leftover comment token "// });" in callback.controller.ts (near the
callback handling / TenantContext wrapper area) and delete it so there are no
orphaned closing-comment artifacts left in the method or controller block.
| reporter: ['text', 'json', 'html'], | ||
| thresholds: { | ||
| // TODO: Restore to 90% after PBAC refactor stabilizes (Technical Debt: ISSUE-123) | ||
| // NOTE: Restore to 90% after PBAC refactor stabilizes (Technical Debt: ISSUE-123) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
TODO → NOTE reduces automated tracking of this technical debt item.
// TODO: comments are picked up automatically by IDEs (VS Code, IntelliJ), ESLint's no-warning-comments rule, and many CI lint hooks. Relabeling it // NOTE: keeps the text but silently drops that visibility, making it easy for the 80%-threshold debt (tied to ISSUE-123) to be forgotten. Consider keeping the TODO prefix, or linking the issue directly in the CI system so the threshold restoration is enforced externally.
✏️ Suggested fix
- // NOTE: Restore to 90% after PBAC refactor stabilizes (Technical Debt: ISSUE-123)
+ // TODO: Restore to 90% after PBAC refactor stabilizes (Technical Debt: ISSUE-123)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // NOTE: Restore to 90% after PBAC refactor stabilizes (Technical Debt: ISSUE-123) | |
| // TODO: Restore to 90% after PBAC refactor stabilizes (Technical Debt: ISSUE-123) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/vitest.config.mts` at line 50, Replace the `// NOTE: Restore to 90%
after PBAC refactor stabilizes (Technical Debt: ISSUE-123)` comment with a
proper `// TODO:` entry so automated tools pick it up; update the comment to
read `// TODO: Restore to 90% after PBAC refactor stabilizes (ISSUE-123)` and
ensure the task/issue id is included exactly as `ISSUE-123` so linters/IDE task
trackers and CI hooks will surface the technical debt for follow-up (file:
vitest.config.mts, comment near the coverage-threshold note).
Initializes the database, engine, and integration packages. Connects TokenManagerService and OAuthCallbackController. Ensures Turborepo layer compliance.
… resolve type-check errors
pramodnarayana
force-pushed
the
feat/connector-engine-backend-core
branch
from
February 20, 2026 10:55
a326068 to
805ce52
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 21
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/engine/connections/callback.controller.spec.ts`:
- Around line 160-191: The test uses a non-UUID string for tenantId via
mockEncryptionService.decrypt which mismatches the DB schema; update the mock in
the test (callback.controller.spec.ts) so
mockEncryptionService.decrypt.mockResolvedValue(...) returns a valid UUID (e.g.,
"550e8400-e29b-41d4-a716-446655440000") before calling
controller.handleCallback, leaving the rest of the assertions
(mockEncryptionService.encrypt, mockInsert, res.redirect) unchanged to keep test
fidelity to the tenant_id uuid column type.
In `@apps/api/src/modules/engine/connections/callback.controller.ts`:
- Around line 55-68: After decrypting rawState into tenantId (using
this.crypto.decrypt), validate that tenantId is a well-formed UUID before
proceeding; if the decrypted value is missing or not a valid UUID, log a warning
(include the provider and the invalid tenantId value) and redirect with
res.redirect(`/app/connections?error=invalid_state`) just like the existing
error path. Update the try/catch block around this.crypto.decrypt to add the
UUID check (e.g., using a UUID regex or a validator) and treat non-UUID
decrypted values the same as other decryption failures so downstream DB calls
using tenantId cannot receive an invalid value.
- Around line 1-5: The controller currently imports the module-level singleton
"db" which bypasses NestJS DI; change the controller to accept the database via
injection like TokenManagerService does: remove the top-level import of "db"
from `@nexiom/database`, add an `@Inject`('DRIZZLE_DB') private readonly db
parameter to the controller's constructor (matching TokenManagerService
pattern), update all uses of the module-level db to use this.db, and ensure the
DRIZZLE_DB provider token is configured in the module so the controller receives
the DI-managed instance for testability and lifecycle management.
In `@docs/architecture/connector_scaling_architecture.md`:
- Around line 319-325: The catch block currently treats any 400 or 401 from the
token refresh as a permanent revocation and updates appConnections to 'REVOKED';
instead, change the logic in the catch block (where
db.update(appConnections).set(...) and this.logger.error are called) to inspect
the error response body for an OAuth2 error code of "invalid_grant" (or
equivalent provider-specific revocation indicator) before marking the connection
revoked; if the response is 400/401 but the body does not contain
"invalid_grant", do not update appConnections.status to 'REVOKED'—instead
surface/throw the error (or retry/backoff) and log the full response/error
details for diagnosis via this.logger.error without silently destroying the
integration. Ensure references to connection.id remain when updating only after
confirming "invalid_grant".
- Line 4: Replace the outdated brand name "FluxNex" with the current name
"Nexiom" throughout the document "connector_scaling_architecture.md" (e.g.,
change "500+ FluxNex integrations" to "500+ Nexiom integrations" and "FluxNex
must maintain the `tenantId`" to "Nexiom must maintain the `tenantId`"); search
the file for any other instances of "FluxNex" (case-sensitive and variants) and
update them to "Nexiom" while preserving surrounding punctuation,
capitalization, and code formatting such as backticks.
In `@integrations/quickbooks/package.json`:
- Around line 1-19: The lint script currently runs eslint with --fix which
auto-corrects issues; add a separate CI-friendly script named lint:check that
runs the same glob ("src/**/*.ts") without --fix so CI can fail on lint errors,
leaving the existing "lint" script for local auto-fixing; update the "scripts"
section to include "lint:check" (and adjust any CI workflow to call lint:check).
In `@integrations/salesforce/src/auth/config.ts`:
- Around line 6-9: The oauth config currently hardcodes authorizeUrl and
tokenUrl to login.salesforce.com which blocks sandbox/custom domains; make the
login domain runtime-configurable by adding a loginUrl field to the
integration's uiSchema and use that runtime value when building
oauth.authorizeUrl and oauth.tokenUrl (falling back to
"https://login.salesforce.com" if unset). Update the code that reads oauth (look
for the oauth object/authorizeUrl/tokenUrl in config.ts) to construct URLs using
the provided loginUrl (e.g., `${loginUrl}/services/oauth2/authorize` and
`${loginUrl}/services/oauth2/token`) so sandbox (test.salesforce.com) and custom
instance domains are supported.
In `@packages/database/package.json`:
- Around line 6-8: The package.json currently only has a "build" script and the
devDependency "drizzle-kit" is unused; add standard migration scripts to the
"scripts" object (e.g., "db:generate", "db:migrate", "db:push") that invoke the
drizzle-kit CLI (via npx or local node_modules binary) so developers can
generate and run migrations from the workspace root; update the "scripts" key in
package.json to include those entries and ensure any required config flags/env
variables are passed through to the drizzle-kit commands.
In `@packages/database/src/schema/tenant.ts`:
- Line 24: The uniqueIndex('tenant_app_name_unique_idx') on
app_connection(tenantId, appName) prevents multi-realm providers and
reconnection flows; either extend the uniqueness to include a stable
per-connection identifier (e.g., add a connectionKey column such as realmId in
the app_connection table and include it in the unique index) or change the index
to a partial unique index that only applies to ACTIVE rows (so revoked rows
don’t block new inserts). Also update the callback controller insert logic (the
controller that creates app_connection rows) to perform an upsert (use
onConflictDoUpdate targeting the same unique key you choose) so reconnects
update status/credentials when appropriate. Ensure metadata still stores realmId
if you add connectionKey and keep consistent column names (realmId,
connectionKey, app_connections, uniqueIndex('tenant_app_name_unique_idx'), and
the callback controller insert/upsert).
In `@packages/engine/package.json`:
- Around line 10-15: package.json currently lists "@nestjs/common" under
"dependencies" which can produce duplicate NestJS instances; remove
"@nestjs/common" from the "dependencies" block and add it to "peerDependencies"
with the same version string ("^11.0.1"); optionally add it to "devDependencies"
if local tests/examples need it, and run your package manager to update the
lockfile (so the package exposes NestJS as a peer rather than bundling its own
copy).
In `@packages/engine/src/connectivity/provider-registry.ts`:
- Around line 1-9: The hardcoded Set ALLOWED_PROVIDERS in provider-registry.ts
won't scale to 500+ providers; replace the static in-code registry with a
pluggable source (e.g., a database table "providers" with an enabled flag or a
config/service endpoint) so providers can be added without code changes;
refactor usages of ALLOWED_PROVIDERS to call a new ProviderRegistry class or
async function getAllowedProviders() that reads from the DB/config and caches
results (with a short TTL and a refresh mechanism) and ensure validation paths
(where ALLOWED_PROVIDERS was referenced) now await and use the dynamic source.
- Around line 6-9: ALLOWED_PROVIDERS as Set<string> loses compile-time safety
for provider names; replace it with a const tuple of literals (e.g. const
ALLOWED_PROVIDER_LIST = ['salesforce','quickbooks'] as const), derive a union
type (type Provider = typeof ALLOWED_PROVIDER_LIST[number]) and then build the
runtime Set from that array (const ALLOWED_PROVIDERS = new
Set<Provider>(ALLOWED_PROVIDER_LIST)). Update any function signatures or
variables that accept provider names to use the Provider type and adjust
switch/case or exhaustive checks to rely on Provider for compile-time
validation.
In `@packages/engine/src/connectivity/token-manager.service.ts`:
- Around line 27-32: The constructor currently types the injected DB as
Record<string, any>, which removes Drizzle type safety; replace that broad type
with the proper Drizzle client type or a minimal typed interface so queries via
this.db are compile-time checked. Update the constructor parameter decorated
with `@Inject`('DRIZZLE_DB') to use the imported Drizzle client type (or a small
interface covering the tables/methods your TokenManager uses), add the
corresponding import from `@nexiom/database` (or define the interface near
token-manager.service.ts), and ensure all usages of this.db in methods of the
TokenManager service compile against the new type.
- Around line 42-59: In getValidCredentials, connection.expiresAt being null
short-circuits the expiry check so OAUTH2 tokens are never refreshed; update the
logic in getValidCredentials to treat a null or missing connection.expiresAt as
expired for authType === 'OAUTH2' by either (a) changing the isExpired
calculation to consider !connection.expiresAt as true for OAUTH2, or (b) adding
an explicit branch before the existing expiry check that calls
refreshWithLock(connection) when connection.authType === 'OAUTH2' and
!connection.expiresAt; optionally emit a warning log before calling
refreshWithLock to aid debugging. Ensure you reference getValidCredentials,
connection.expiresAt, authType === 'OAUTH2', and refreshWithLock in your change.
- Around line 61-88: The refreshWithLock function can use a fresh DB read after
successfully acquiring the retry lock to avoid using a stale connection; after
the retryLock is set (the branch where retryLock is truthy inside the retry
loop), re-query appConnections for the current connection (same where:
eq(appConnections.id, connection.id)) and replace the local connection variable
with that fresh record before proceeding to decrypt encryptedCredentials and
perform the refresh; ensure subsequent logic that reads expiresAt or
encryptedCredentials uses the reloaded record.
- Around line 6-10: The mock used in callback.controller.spec.ts includes unused
properties hash and verifyHash that don't exist on the EncryptionService
abstract class; remove those two properties from the test mock so the mock only
implements decrypt and encrypt per the EncryptionService contract, ensuring the
mock's shape matches the abstract class (EncryptionService) and avoiding unused
or misleading members in the test fixture.
In `@packages/engine/src/index.ts`:
- Around line 1-3: ALLOWED_PROVIDERS (from provider-registry) is being
re-exported as a mutable Set<string>, exposing brittle public surface; change
the module that defines ALLOWED_PROVIDERS to export a strongly typed, immutable
list and a derived type instead: replace the loose Set<string> with an "as
const" tuple (or readonly array) of provider keys and export a type like
Provider = typeof ALLOWED_PROVIDERS[number], and if a runtime collection is
needed keep an internal ReadonlySet<Provider> or export a getter (e.g.,
getAllowedProviders()) to prevent callers from depending on raw strings — update
exports in provider-registry and any references to use the new Provider type and
accessor instead of Set<string>.
In `@packages/eslint-config/package.json`:
- Around line 8-9: The dependency entries for "@eslint/js" and "eslint" in
package.json are intentionally pinned to v9 (e.g., "@eslint/js": "^9.20.0",
"eslint": "^9.20.1") and require no change now; leave these version ranges as-is
but add a short TODO comment in the repo tracking doc or an internal tracking
ticket to evaluate upgrading to ESLint v10 when CI/node images move to Node.js
20+ (since ESLint v10 requires Node >= 20.19.0). Reference the package.json
dependency keys "@eslint/js" and "eslint" when adding the ticket or note.
- Around line 7-14: Move shared tooling from "dependencies" into
"peerDependencies" in package.json so consumers control versions; specifically
remove "eslint", "@eslint/js" (or "@eslint/js"), "typescript-eslint" (the
package named "typescript-eslint" in the diff) and related ESLint tool packages
from the dependencies object and add them under "peerDependencies" with the same
version ranges, and keep config-only packages (like "eslint-config-prettier" and
"eslint-plugin-prettier") as devDependencies if they’re only used for
testing/own linting; update package.json accordingly so published consumers will
resolve their own ESLint and typescript-eslint versions.
In `@tsconfig.base.json`:
- Around line 12-13: The shared tsconfig currently sets "experimentalDecorators"
and "emitDecoratorMetadata" globally which forces non-Nest packages (e.g.,
packages/database, integrations/salesforce, integrations/quickbooks) to inherit
NestJS-specific compiler flags; remove these two flags from tsconfig.base.json
and instead add them only to the tsconfig(s) used by Nest apps that require DI
metadata (e.g., the API app tsconfig — add "experimentalDecorators": true and
"emitDecoratorMetadata": true to apps/api's tsconfig or a Nest-specific base
tsconfig that apps/api extends), and ensure other package tsconfigs continue to
extend tsconfig.base.json without inheriting those flags.
- Around line 2-14: Add an explicit moduleResolution option under
compilerOptions to avoid relying on TypeScript defaults: update the
"compilerOptions" block (where "module": "CommonJS" is set) to include
"moduleResolution": "node" so tooling and future TypeScript upgrades use a
deterministic Node resolution algorithm.
---
Duplicate comments:
In `@apps/api/src/modules/engine/connections/callback.controller.ts`:
- Around line 52-68: The decrypted OAuth state (rawState → tenantId via
this.crypto.decrypt in callback.controller) is not being validated against a
server-side session value, leaving CSRF/replay risk; update the handler to fetch
the session-stored state (e.g., req.session.oauthState or similar session key
used during the authorization request), compare it to the decrypted value
(tenantId or the original nonce/payload), and reject/redirect if they differ;
ensure you reference grantResponse.raw?.state and the decrypted tenantId from
this.crypto.decrypt, clear the session state after successful validation, and
keep the existing error redirect
(res.redirect(`/app/connections?error=invalid_state`)) on mismatch or missing
session.
In `@docs/architecture/connector_scaling_architecture.md`:
- Around line 136-141: The example uses non-existent package namespaces; update
the import statements in the callback controller example (the file shown with
Controller/Get/Req/Res imports) to import db and appConnections from the actual
package `@nexiom/database` (they are re-exported from its index), and replace the
`@fluxnex/core-kernel/`@fluxnex/database-schema references accordingly; also apply
the same fix in the TokenManagerService example where `@fluxnex/`* is used so that
TokenManagerService imports the db and appConnections (and any other re-exported
symbols) from `@nexiom/database` instead.
- Around line 279-289: refreshWithLock currently performs unbounded recursion by
calling this.getValidCredentials(connection.id) when the lock isn't acquired;
replace that recursion with a bounded iterative retry loop inside
getValidCredentials/refreshWithLock to avoid growing the call stack.
Specifically, in refreshWithLock (and the caller getValidCredentials), change
the "if (!lockAcquired) { ... return this.getValidCredentials(...); }" behavior
to a loop that retries lock acquisition with an exponential or fixed backoff and
a hard deadline or max attempts based on the lock TTL (10000 ms) and a safe
margin; ensure you reference the same lockKey (`lock:refresh:${connection.id}`)
and this.redis.set parameters ('PX', 10000, 'NX') so retries stop after the TTL
or reach the max attempts, and return or throw a clear error when retries are
exhausted instead of recursing.
In `@docs/architecture/connector_scaling_strategy.md`:
- Around line 55-57: The document is truncated mid-sentence at "Write the
**Token Expiration Checker** in your Layer 5 (Delivery Worker", so complete the
broken sentence by closing the parenthesis and finish the Task list for Phase 1,
then add the missing Phase 2, Phase 3 and Phase 4 sections with their respective
tasks and descriptions; specifically restore the full mention of "Token
Expiration Checker" in Layer 5 (Delivery Worker) and add subsequent phase
headings and task bullet points consistent with the document's style so the
architecture/connector_scaling_strategy.md contains a full, continuous
description of all phases.
In `@integrations/quickbooks/eslint.config.mjs`:
- Around line 1-8: The integration ESLint config is correctly using the shared
factory createIntegrationConfig and computing __dirname via
dirname(fileURLToPath(import.meta.url)); no code changes are required — keep the
file using import { createIntegrationConfig } from '@nexiom/eslint-config' and
exporting default createIntegrationConfig(__dirname) as-is to maintain the
uniform minimal wrapper behavior across integrations.
In `@integrations/salesforce/src/auth/config.ts`:
- Line 1: Import of GenericCredentialType now correctly uses the public entry
point "@nexiom/engine"; no code change needed—leave the import of
GenericCredentialType as-is in integrations/salesforce/src/auth/config.ts.
In `@packages/database/package.json`:
- Around line 1-18: The package.json for the internal package "@nexiom/database"
is missing "private": true which risks accidental publishes; update the
top-level package.json (the object containing "name": "@nexiom/database" and
"version": "1.0.0") by adding "private": true alongside the existing fields so
npm will refuse to publish this workspace package; ensure the key is a boolean
at root level of the JSON.
In `@packages/database/src/schema/tenant.ts`:
- Line 5: The tenantId column in the schema (tenantId:
uuid('tenant_id').notNull()) lacks a DB-level foreign key causing orphaned
app_connection rows; update the column definition to include a foreign-key
reference (e.g., tenantId:
uuid('tenant_id').notNull().references('identity.tenants.id') or the equivalent
API in your schema builder) and/or add a migration that creates the FK
constraint on app_connection. Locate tenantId in
packages/database/src/schema/tenant.ts and the app_connection table definition,
add the .references(...) call (or create SQL migration to ALTER TABLE
app_connection ADD CONSTRAINT ... FOREIGN KEY (tenant_id) REFERENCES
identity.tenants(id)), and run/generate the corresponding migration so the
constraint is enforced at the DB level.
| # Detailed Design: Connector Auth Lifecycle | ||
|
|
||
| **Target Audience:** Senior Backend Engineers | ||
| **Scope:** This document defines the exact data models, sequence flows, and service implementations required to securely authenticate, store, and refresh OAuth2/API credentials for 500+ FluxNex integrations. |
Contributor
There was a problem hiding this comment.
Branding inconsistency — "FluxNex" should be "Nexiom" throughout this document.
Line 4 ("500+ FluxNex integrations") and Line 106 ("FluxNex must maintain the tenantId") reference an old project name.
🐛 Proposed fix
-**Scope:** This document defines the exact data models, sequence flows, and service implementations required to securely authenticate, store, and refresh OAuth2/API credentials for 500+ FluxNex integrations.
+**Scope:** This document defines the exact data models, sequence flows, and service implementations required to securely authenticate, store, and refresh OAuth2/API credentials for 500+ Nexiom integrations.And at Line 106:
-When a user initiates an OAuth connection, FluxNex must maintain the `tenantId` across the redirect boundary.
+When a user initiates an OAuth connection, Nexiom must maintain the `tenantId` across the redirect boundary.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **Scope:** This document defines the exact data models, sequence flows, and service implementations required to securely authenticate, store, and refresh OAuth2/API credentials for 500+ FluxNex integrations. | |
| **Scope:** This document defines the exact data models, sequence flows, and service implementations required to securely authenticate, store, and refresh OAuth2/API credentials for 500+ Nexiom integrations. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/architecture/connector_scaling_architecture.md` at line 4, Replace the
outdated brand name "FluxNex" with the current name "Nexiom" throughout the
document "connector_scaling_architecture.md" (e.g., change "500+ FluxNex
integrations" to "500+ Nexiom integrations" and "FluxNex must maintain the
`tenantId`" to "Nexiom must maintain the `tenantId`"); search the file for any
other instances of "FluxNex" (case-sensitive and variants) and update them to
"Nexiom" while preserving surrounding punctuation, capitalization, and code
formatting such as backticks.
Comment on lines
+319
to
+325
| } catch (error) { | ||
| // Handle cases where the user revoked access in the external app | ||
| if (error?.response?.status === 400 || error?.response?.status === 401) { | ||
| await db.update(appConnections).set({ status: 'REVOKED' }).where(eq(appConnections.id, connection.id)); | ||
| this.logger.error(`Token refresh rejected. Marked connection as REVOKED.`); | ||
| } | ||
| throw error; |
Contributor
There was a problem hiding this comment.
HTTP 400 is too broad to infer token revocation — valid connections will be permanently broken.
The catch block marks the connection REVOKED on any 400 or 401 response. An OAuth2 400 can arise from many non-revocation causes: malformed request bodies (wrong Content-Type), provider-side input validation errors, network proxies, or transient rate-limiting responses. Treating all of them as permanent revocation silently destroys users' integrations.
Only an OAuth2 invalid_grant error in the response body reliably signals that the refresh token has been revoked or expired. A bare 401 alone may also be a transient auth failure.
🛡️ Proposed fix
- if (error?.response?.status === 400 || error?.response?.status === 401) {
+ const oauthError = error?.response?.data?.error ?? error?.response?.data;
+ const isRevoked =
+ (error?.response?.status === 400 && oauthError === 'invalid_grant') ||
+ (error?.response?.status === 401 && oauthError === 'invalid_grant');
+ if (isRevoked) {
await db.update(appConnections).set({ status: 'REVOKED' }).where(eq(appConnections.id, connection.id));
this.logger.error(`Token refresh rejected. Marked connection as REVOKED.`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (error) { | |
| // Handle cases where the user revoked access in the external app | |
| if (error?.response?.status === 400 || error?.response?.status === 401) { | |
| await db.update(appConnections).set({ status: 'REVOKED' }).where(eq(appConnections.id, connection.id)); | |
| this.logger.error(`Token refresh rejected. Marked connection as REVOKED.`); | |
| } | |
| throw error; | |
| } catch (error) { | |
| // Handle cases where the user revoked access in the external app | |
| const oauthError = error?.response?.data?.error ?? error?.response?.data; | |
| const isRevoked = | |
| (error?.response?.status === 400 && oauthError === 'invalid_grant') || | |
| (error?.response?.status === 401 && oauthError === 'invalid_grant'); | |
| if (isRevoked) { | |
| await db.update(appConnections).set({ status: 'REVOKED' }).where(eq(appConnections.id, connection.id)); | |
| this.logger.error(`Token refresh rejected. Marked connection as REVOKED.`); | |
| } | |
| throw error; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/architecture/connector_scaling_architecture.md` around lines 319 - 325,
The catch block currently treats any 400 or 401 from the token refresh as a
permanent revocation and updates appConnections to 'REVOKED'; instead, change
the logic in the catch block (where db.update(appConnections).set(...) and
this.logger.error are called) to inspect the error response body for an OAuth2
error code of "invalid_grant" (or equivalent provider-specific revocation
indicator) before marking the connection revoked; if the response is 400/401 but
the body does not contain "invalid_grant", do not update appConnections.status
to 'REVOKED'—instead surface/throw the error (or retry/backoff) and log the full
response/error details for diagnosis via this.logger.error without silently
destroying the integration. Ensure references to connection.id remain when
updating only after confirming "invalid_grant".
Comment on lines
+1
to
+3
| export * from './connectivity/types'; | ||
| export * from './connectivity/token-manager.service'; | ||
| export * from './connectivity/provider-registry'; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
rg -n "ALLOWED_PROVIDERS" --type=ts -A 3 -B 1Repository: pramodnarayana/nexiom
Length of output: 2014
ALLOWED_PROVIDERS is now part of the public API surface — consider stricter typing for versioning safety.
Re-exporting ./connectivity/provider-registry makes ALLOWED_PROVIDERS (currently a Set<string>) accessible to every consumer of @nexiom/engine. Adding or removing a provider key becomes a semver-breaking change. While the current Set<string> implementation is functional, consider using as const with a typed accessor or a discriminated union type to enforce allowed values at compile time and improve type safety for consumers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/engine/src/index.ts` around lines 1 - 3, ALLOWED_PROVIDERS (from
provider-registry) is being re-exported as a mutable Set<string>, exposing
brittle public surface; change the module that defines ALLOWED_PROVIDERS to
export a strongly typed, immutable list and a derived type instead: replace the
loose Set<string> with an "as const" tuple (or readonly array) of provider keys
and export a type like Provider = typeof ALLOWED_PROVIDERS[number], and if a
runtime collection is needed keep an internal ReadonlySet<Provider> or export a
getter (e.g., getAllowedProviders()) to prevent callers from depending on raw
strings — update exports in provider-registry and any references to use the new
Provider type and accessor instead of Set<string>.
Comment on lines
+8
to
+9
| "@eslint/js": "^9.20.0", | ||
| "eslint": "^9.20.1", |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
ESLint v10 is now released — ^9.20.1 correctly stays on v9, no action required today.
The latest ESLint version is 10.0.0, and it requires Node.js ^20.19.0, ^22.13.0, or >=24. Because ^ never crosses a major boundary, ^9.20.1 will not auto-upgrade. The current pin is intentional and safe, but track the v10 migration when CI upgrades to Node.js 20+.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/eslint-config/package.json` around lines 8 - 9, The dependency
entries for "@eslint/js" and "eslint" in package.json are intentionally pinned
to v9 (e.g., "@eslint/js": "^9.20.0", "eslint": "^9.20.1") and require no change
now; leave these version ranges as-is but add a short TODO comment in the repo
tracking doc or an internal tracking ticket to evaluate upgrading to ESLint v10
when CI/node images move to Node.js 20+ (since ESLint v10 requires Node >=
20.19.0). Reference the package.json dependency keys "@eslint/js" and "eslint"
when adding the ticket or note.
…ts cognitive complexity
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/db/schema.ts`:
- Around line 42-44: Consolidate the two separate re-exports into a single
statement: replace the two export lines that export providers and appConnections
from `@nexiom/database` with one combined export that exports both symbols
(providers, appConnections) from '@nexiom/database' so there is a single
import/export statement for those named exports.
In `@apps/api/src/modules/engine/connections/callback.controller.spec.ts`:
- Around line 179-210: The test currently only checks that mockInsert was
called; update the success test for controller.handleCallback to also assert the
upsert payload passed to mockInsert is correct: retrieve the captured values
(from the hoisted values mock or the last call of mockInsert) and assert it
contains the expected tenantId (VALID_TENANT_ID), connectionKey/domain-specific
fields (e.g., realmId), and encrypted credential blob (string containing
"accessToken":"acc-123"), using expect.objectContaining or stringContaining
matchers to validate those fields rather than just existence of the call.
- Around line 9-12: The current vi.hoisted() callback mutates
process.env.DATABASE_URL (inside the block that returns mockOnConflictDoUpdate,
mockInsert, mockDb), which creates a persistent side effect across the Vitest
worker; move the DATABASE_URL setup out of the vi.hoisted() hoisted block and
instead set and restore process.env.DATABASE_URL inside the test file's
lifecycle hooks (beforeEach/afterEach or beforeAll/afterAll if import-time
initialization forces it) so the change is scoped to this test; if import-time
initialization requires the hoisted placement, add a clear explanatory comment
above the vi.hoisted() usage mentioning the unavoidable global mutation and why
it is necessary.
In `@apps/api/src/modules/engine/connections/callback.controller.ts`:
- Line 44: The awaits for providerRegistry.isAllowed(provider) and
crypto.encrypt(...) must be wrapped in try/catch like the DB insert block to
prevent unhandled rejections from escaping the controller; locate the calls to
providerRegistry.isAllowed in the callback controller and the crypto.encrypt
usage (lines referenced around 44 and 96-98), wrap each await in its own
try/catch, log the error (using the controller logger) and return/throw the same
error-handling/redirect behavior used by the existing DB insert catch block
(e.g., produce the same redirect response or throw the same BadRequestException)
so failures in the registry or encryption path follow the same UX as DB errors.
- Around line 101-105: The current assignment to expiresIn allows zero or
negative numbers (grantResponse.raw.expires_in) which yields an already-expired
expiresAt; change the guard in the expiresIn calculation to only accept positive
numbers (e.g., typeof grantResponse.raw?.expires_in === 'number' &&
grantResponse.raw.expires_in > 0) and otherwise fall back to the default (3600)
or clamp via Math.max(min, value) before computing expiresAt so that expiresAt
is always in the future; update the expiresIn constant and the expiresAt
computation accordingly (referencing grantResponse.raw.expires_in, expiresIn,
and expiresAt).
- Around line 89-110: Comments duplicate step numbers around the
payload/encryption block and there's an unnecessary type cast; rename/resequence
the inline step comments so they read 4→5→6→7 (e.g., change the second "4." to
"5." and the second "5." to "6." or appropriate sequential numbers) to reflect
the actual flow of preparing credentials, encrypting, computing expiry, deriving
connectionKey, and saving; and remove the redundant "as string" cast from the
connectionKey assignment so it becomes connectionKey =
grantResponse.raw?.realmId ?? 'default' (reference symbols: credentials,
encryptedPayload, expiresIn, expiresAt, connectionKey,
grantResponse.raw?.realmId).
In `@packages/database/package.json`:
- Around line 8-10: The db scripts ("db:generate", "db:migrate", "db:studio") in
packages/database package.json will fail because there is no drizzle.config.ts
in that package; add a new drizzle.config.ts in packages/database that mirrors
the required config (schema sources, out directory, and dialect) similar to
apps/api/drizzle.config.ts, ensure it exports the DrizzleConfig (or default) and
points to the correct local schema files and database URL used by
packages/database so drizzle-kit can run for the package.
In `@packages/database/src/client.ts`:
- Around line 6-25: The getDb function creates a Postgres Pool (variable pool)
but never calls pool.end(), so DB connections linger on shutdown; add a graceful
shutdown hook that calls pool.end() (and awaits it) when the process is
terminating (e.g. on SIGTERM, SIGINT, and beforeExit/exit) and guard for
undefined pool; update getDb (and export/initialize code near drizzle/dbInstance
and Pool) to register an idempotent process.on handler that logs/errors
appropriately and calls pool.end(). Ensure the handler clears itself or is safe
to register only once and that closing handles pending queries (await
pool.end()) and does not throw if pool is already closed.
- Around line 7-33: The current use of ReturnType<typeof drizzle> for dbInstance
and the db proxy erases the schema generic causing query types to be unknown;
fix by extracting the merged schema into a concrete constant (e.g., schemaBundle
= { ...tenantSchema, ...providerSchema }) and use NodePgDatabase<typeof
schemaBundle> (or drizzle's generic with typeof schemaBundle) as the typed
return for dbInstance, getDb, and the db proxy so that drizzle({... , schema:
schemaBundle}) preserves the schema types for callers like
db.query.providers.findFirst and db.query.appConnections.findMany.
In `@packages/database/src/schema/provider.ts`:
- Around line 16-19: The jsonb columns are currently untyped and return unknown;
update the column definitions for scopes and uiSchema to include .$type<...>()
so TypeScript infers proper types—e.g. add .$type<string[]>() to the scopes
column (jsonb('scopes')) and .$type<Record<string, unknown>>() or a specific
UiSchema interface to the uiSchema column (jsonb('ui_schema')); ensure any
referenced UiSchema type is imported or declared and keep the existing
.default([]) and .default({}) calls.
- Line 11: The authType column on the Provider schema (authType in provider.ts)
currently uses varchar and lacks a DB-level constraint; replace it with a proper
enum or add a CHECK constraint: preferred fix — define a pgEnum (e.g., const
AuthType = pgEnum('AuthType', ['OAUTH2','API_KEY','BASIC']) and use authType:
AuthType('auth_type').notNull()) so PostgreSQL enforces allowed values and
TypeScript infers the type; alternative — keep varchar but add a table-level
.check(sql`auth_type IN ('OAUTH2','API_KEY','BASIC')`) on the provider table to
prevent invalid inserts.
In `@packages/engine/src/connectivity/provider-registry.ts`:
- Around line 27-34: The getProvider and getAllProviders functions currently
return untyped promises; change their return types to use the provider table's
inferred type (e.g. Promise<InferSelectModel<typeof providers> | null> for
getProvider and Promise<InferSelectModel<typeof providers>[]> for
getAllProviders), import InferSelectModel from 'drizzle-orm', and ensure any
nullability is reflected (getProvider returns null when not found); this gives
callers compile-time access to fields like authorizeUrl, tokenUrl, and scopes
while keeping the existing implementations (refer to getProvider,
getAllProviders and the providers symbol).
In `@packages/engine/src/connectivity/token-manager.service.ts`:
- Around line 146-184: performTokenRefresh currently merges vendor snake_case
token fields into a camelCase persisted payload implicitly (via spread), which
can leave future snake_case fields unnormalized; update performTokenRefresh to
explicitly map known token fields from newTokens (e.g., map access_token ->
accessToken, refresh_token -> refreshToken, expires_in -> expiresIn, id_token ->
idToken, token_type -> tokenType) and then merge any remaining unknown fields
from oldPayload (or newTokens) into updatedPayload so new known fields are
normalized while unknowns are preserved; also add a short comment in
performTokenRefresh describing the expected normalized payload shape for future
maintainers.
- Around line 54-74: The current getValidCredentials method assumes
connection.expiresAt is a Date and calls .getTime(), which can throw if
expiresAt is an ISO string; update getValidCredentials to defensively
coerce/validate expiresAt before using getTime (e.g., if typeof
connection.expiresAt !== 'object' or not instance of Date, create a Date from it
and treat invalid/NaN dates as missing/expired), keep the same OAUTH2 expiry
logic and the fallback to refreshWithLock(connection) when expired, and ensure
the later return still decrypts connection.encryptedCredentials via
this.crypto.decrypt as before.
- Around line 23-33: The DrizzleDb interface is duplicated and too restrictive:
move the exported DrizzleDb interface out of token-manager.service.ts into a
shared types file (e.g., create types.ts in the same directory) and update
TokenManagerService, ProviderRegistryService, and OAuthCallbackController to
import and use that single DrizzleDb type; also relax the .select() chain
signature so it allows terminating at .from().where() or .from().where().limit()
(make .where() return a union that either returns the array Promise or an object
with limit(...):Promise) or replace the custom interface with Drizzle's
NodePgDatabase<typeof schema> if the schema is available in the engine package,
ensuring getAllProviders() (which calls select().from().where()) compiles
against the shared type.
In `@packages/eslint-config/package.json`:
- Around line 4-6: Add an "exports" field to package.json so Node and bundlers
can resolve the ESM entry explicitly: under the existing "type": "module" and
"main": "index.mjs" entries add an exports map that at minimum maps "." to
"./index.mjs" (and optionally provides "./package.json" or other subpath exports
if needed) so consumers get stable ESM subpath resolution instead of relying on
the legacy "main" fallback.
- Around line 7-11: Add an explicit peerDependency for prettier in package.json:
update the package's "peerDependencies" object to include "prettier": ">=3.0.0"
so consumers see the requirement up-front (this complements the existing
dependency on "eslint-plugin-prettier" and enforces the peer contract declared
by that package).
---
Duplicate comments:
In `@apps/api/src/modules/engine/connections/callback.controller.ts`:
- Around line 123-128: The upsert using onConflictDoUpdate targeting
appConnections.tenantId, appConnections.appName and appConnections.connectionKey
will fail unless the schema defines a composite unique constraint on those three
columns; open packages/database/src/schema/tenant.ts and ensure there is a
uniqueIndex (not index) for the composite key (tenant_app_name or equivalent)
that includes tenantId, appName and connectionKey, or add a migration to create
a unique constraint covering those three columns so the upsert in
callback.controller.ts can succeed.
- Around line 61-78: The decrypted state currently assumes a plaintext tenantId;
change handling in the callback so this.crypto.decrypt(rawState) returns a JSON
payload (e.g., { tenantId, nonce, iat }), parse that payload instead of treating
it as a raw tenantId, validate tenantId with uuidValidate, and verify iat is
within a short configurable window (e.g., Date.now() - payload.iat <= 5min); if
any of these checks fail, log and redirect as before. Optionally, if you have a
store for one-time nonces, also check/mark payload.nonce as used to enforce
single-use; update the error handling around rawState/tenantId parsing and
validation in the same block (referencing rawState, this.crypto.decrypt,
tenantId, uuidValidate).
In `@packages/database/package.json`:
- Around line 1-4: Add the missing "private": true field to the package.json
object to prevent accidental publishing; edit the top-level JSON in packages'
package.json (the object containing "name": "@nexiom/database", "version":
"1.0.0", "main": "dist/index.js") and insert "private": true alongside those
keys so the package is marked private.
In `@packages/eslint-config/package.json`:
- Around line 12-16: The peerDependencies currently pin "eslint" to "^9.20.1"
which will block ESLint v10; update the "peerDependencies" entry for "eslint" in
package.json (the "peerDependencies" block containing "@eslint/js", "eslint",
"typescript-eslint") to allow v10 as well — e.g. change the range to include
both v9 and v10 (for example "^9.20.1 || ^10.0.0" or ">=9.20.1 <11") so future
ESLint v10 installs are accepted, and ensure CI is monitored for Node.js/ESLint
compatibility once CI moves to Node 20+.
Comment on lines
+23
to
+33
| /** Minimal typed interface for the injected Drizzle DB client. */ | ||
| export interface DrizzleDb { | ||
| query: { | ||
| appConnections: { | ||
| findFirst(args: Record<string, unknown>): Promise<Record<string, any> | undefined>; | ||
| }; | ||
| }; | ||
| insert(table: unknown): { values(data: Record<string, unknown>): { onConflictDoUpdate(args: Record<string, unknown>): Promise<unknown> } }; | ||
| update(table: unknown): { set(data: Record<string, unknown>): { where(condition: unknown): Promise<unknown> } }; | ||
| select(fields?: unknown): { from(table: unknown): { where(condition: unknown): { limit(n: number): Promise<Record<string, unknown>[]> } } }; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, find provider-registry.ts and check DRIZZLE_DB usage
find packages/engine/src -name "provider-registry.ts" -o -name "*provider-registry*"Repository: pramodnarayana/nexiom
Length of output: 118
🏁 Script executed:
# Check DRIZZLE_DB usage across the codebase
rg -n 'DRIZZLE_DB' --type=ts -g '!node_modules' -C 3Repository: pramodnarayana/nexiom
Length of output: 7093
🏁 Script executed:
# Check for DrizzleDb interface usage and definitions
rg -n 'DrizzleDb|interface DrizzleDb' --type=ts -g '!node_modules'Repository: pramodnarayana/nexiom
Length of output: 479
🏁 Script executed:
# Look at the actual select chain usage in the codebase
rg -n '\.select\(' --type=ts -g '!node_modules' -C 3 | head -60Repository: pramodnarayana/nexiom
Length of output: 3871
🏁 Script executed:
# Check db.provider.ts to see what type is actually returned
cat -n packages/api/src/db/db.provider.tsRepository: pramodnarayana/nexiom
Length of output: 131
🏁 Script executed:
# Check what's imported from drizzle-orm
rg -n 'from.*drizzle' --type=ts -g '!node_modules' | head -20Repository: pramodnarayana/nexiom
Length of output: 2115
🏁 Script executed:
# Check if schema is accessible from the engine package
find packages/engine -name "schema*" -type fRepository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
# Look at the actual select usage in provider-registry to see what methods are called
cat -n packages/engine/src/connectivity/provider-registry.ts | head -50Repository: pramodnarayana/nexiom
Length of output: 1710
🏁 Script executed:
# Check callback.controller.ts for actual select() usage patterns
cat -n apps/api/src/modules/engine/connections/callback.controller.tsRepository: pramodnarayana/nexiom
Length of output: 5732
🏁 Script executed:
# Check token-manager.service.ts for actual select() usage patterns
cat -n packages/engine/src/connectivity/token-manager.service.ts | head -100Repository: pramodnarayana/nexiom
Length of output: 4970
Share DrizzleDb interface between TokenManagerService and ProviderRegistryService via a common types file.
ProviderRegistryService currently types the injected DB as Record<string, any> (line 13) while TokenManagerService and OAuthCallbackController both use the DrizzleDb interface. Move DrizzleDb to a shared location (e.g., types.ts in this directory) so both services reference the same contract.
Additionally, the .select() chain signature is overly constrained. The interface requires .where().limit() to complete, but getAllProviders() (line 38–41 in provider-registry.ts) calls .select().from().where() without .limit(). Drizzle's actual query builder makes both .where() and .limit() optional termination points. Either broaden the interface to support flexible chain endings, or prefer Drizzle's native NodePgDatabase<typeof schema> type if the schema is accessible to the engine package.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/engine/src/connectivity/token-manager.service.ts` around lines 23 -
33, The DrizzleDb interface is duplicated and too restrictive: move the exported
DrizzleDb interface out of token-manager.service.ts into a shared types file
(e.g., create types.ts in the same directory) and update TokenManagerService,
ProviderRegistryService, and OAuthCallbackController to import and use that
single DrizzleDb type; also relax the .select() chain signature so it allows
terminating at .from().where() or .from().where().limit() (make .where() return
a union that either returns the array Promise or an object with
limit(...):Promise) or replace the custom interface with Drizzle's
NodePgDatabase<typeof schema> if the schema is available in the engine package,
ensuring getAllProviders() (which calls select().from().where()) compiles
against the shared type.
pramodnarayana
marked this pull request as draft
February 20, 2026 14:54
pramodnarayana
marked this pull request as ready for review
February 20, 2026 14:54
pramodnarayana
marked this pull request as draft
February 20, 2026 14:59
pramodnarayana
marked this pull request as ready for review
February 20, 2026 14:59
pramodnarayana
marked this pull request as draft
February 20, 2026 15:02
pramodnarayana
marked this pull request as ready for review
February 20, 2026 15:02
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/database/drizzle.config.ts`:
- Around line 7-10: The dbCredentials.url fallback currently embeds hardcoded
credentials (the DATABASE_URL fallback string) in drizzle.config.ts; remove the
literal 'postgres://admin:password123@...' and either (a) throw/exit with a
clear error when process.env.DATABASE_URL is missing, or (b) configure
dotenv/envFilePath for drizzle-kit and read a non-committed local .env variable
instead; update the dbCredentials reference and any bootstrap code that expects
DATABASE_URL so it fails loudly or uses process.env loaded from the .env file
rather than an in-repo password.
- Line 4: The schema glob in the drizzle config (the schema property) is too
broad ('./src/schema/*') and doesn't match .ts files; update the schema value to
include the file extension (change to './src/schema/*.ts') or use './src/schema'
to let drizzle-kit load the directory recursively so files like provider.ts and
tenant.ts are matched; modify the schema property in the exported config
accordingly.
In `@packages/database/src/client.ts`:
- Around line 1-2: Consolidate the two separate imports from
'drizzle-orm/node-postgres' into a single import statement: replace the separate
"import { drizzle } from 'drizzle-orm/node-postgres';" and "import { type
NodePgDatabase } from 'drizzle-orm/node-postgres';" with one combined import
that brings in both drizzle and the NodePgDatabase type (use the symbol names
drizzle and NodePgDatabase) to reduce redundancy and follow module import best
practices.
In `@packages/engine/src/connectivity/provider-registry.ts`:
- Line 34: The cast using InferSelectModel<typeof providers> masks potential
schema mismatches because DrizzleDb (the loose DB type) returns Record<string,
unknown>[]; replace the DrizzleDb usage with a strongly typed
NodePgDatabase<typeof schemaBundle> (from the database package) where the DB
instance is typed, then remove the manual casts (the return line that currently
does return (rows[0] as InferSelectModel<typeof providers>) ?? null and the
similar cast at the other occurrence). Update any function signatures or the
registry constructor that accept the DB to use NodePgDatabase<typeof
schemaBundle> so the providers table type is inferred directly and the
InferSelectModel cast is no longer needed.
In `@packages/engine/src/connectivity/token-manager.service.ts`:
- Around line 161-172: The current merge `{ ...oldPayload, ...newTokens }`
copies vendor snake_case keys (e.g., access_token, refresh_token) alongside the
camelCase mappings, causing duplicated fields to accumulate; change the creation
of updatedPayload to either (a) build it explicitly from oldPayload plus only
the desired normalized fields from newTokens (map
newTokens.access_token→accessToken, access_token→omit,
refresh_token→refreshToken, etc.) and preserve any other vendor custom fields by
filtering newTokens to exclude the known snake_case keys before spreading, or
(b) keep the current spread but immediately delete the snake_case keys
(access_token, refresh_token, expires_in, id_token, token_type) from
updatedPayload after you set the camelCase properties; reference updatedPayload,
oldPayload, newTokens and the snake_case keys (access_token, refresh_token,
expires_in, id_token, token_type) when applying the change.
- Around line 111-122: The retry path in waitForRefreshOrAcquireLock() uses
freshConnection.expiresAt.getTime() directly which can throw if expiresAt isn't
a Date; create a small helper (e.g., parseExpiresAt(value): Date | null) that
mirrors the defensive logic used in getValidCredentials() — it should return a
Date if value is a valid date instance or parseable string/number and return
null for invalid/NaN inputs — then use that helper in both
waitForRefreshOrAcquireLock() and getValidCredentials() to replace direct
instanceof/getTime usage and guard the comparison that checks whether the token
was refreshed by another worker.
- Line 23: The DrizzleDb import is placed after the abstract class declarations;
move the line "import { DrizzleDb } from './types.js';" up into the main imports
block so it sits with the other top-of-file imports (grouped with the existing
import statements) to restore conventional module ordering and avoid
interleaving imports with class definitions.
In `@packages/engine/src/connectivity/types.ts`:
- Line 35: The findFirst declaration currently returns Promise<Record<string,
any> | undefined>, which leaks any; change its return type to
Promise<Record<string, unknown> | undefined> in the types.ts declaration for
findFirst and update any direct usages that assume concrete properties to
narrow/cast the result (e.g., via type guards or explicit casts) so callers
handle unknown-safe access; keep the function name findFirst and the args type
Record<string, unknown> unchanged.
- Line 38: The types declare insert(table).values(...) as returning only an
object with onConflictDoUpdate, which prevents plain await usage; change the
type of values(...) so it is thenable/awaitable and still exposes chainable
methods (e.g. make it return PromiseLike<unknown> & { onConflictDoUpdate(args:
Record<string, unknown>): Promise<unknown>; returning(...args: unknown[]):
Promise<unknown> } or a similar interface) so callers can do await
db.insert(...).values(...) as well as chain .onConflictDoUpdate(...) and
.returning(...); update the signature for insert(...) -> { values(data:
Record<string, unknown>): PromiseLike<unknown> & { onConflictDoUpdate(args:
Record<string, unknown>): Promise<unknown>; returning(...args: unknown[]):
Promise<unknown> } } to match Drizzle's lazy executor behavior.
- Around line 31-44: The current hand-written DrizzleDb interface is brittle;
replace it with the real inferred type by importing the configured db from
`@nexiom/database` and exporting its typeof as the DrizzleDb type. Specifically,
remove the manual interface declaration named DrizzleDb in types.ts, add an
import for the exported db from '@nexiom/database', and export a type alias like
DrizzleDb = typeof db so token-manager.service.ts and provider-registry.ts (and
any other consumers) get the exact Drizzle client type.
- Around line 40-44: The current select() declaration on DrizzleDb is too
generic and hides the lazy query builder behavior; update the select signature
to return a typed PromiseLike/thenable builder that models the full chain
(select(...) -> from(...) -> where(...) -> limit(...)) and preserves the real
result type T instead of unknown. Replace the nested anonymous object return
types for select/from/where/limit with a generic builder interface (e.g.,
QueryBuilder<T> or PromiseLike<T[]>) that implements then/ catch/ finally and
exposes from(table), where(condition) and limit(n) methods returning the same
builder typed as PromiseLike<Record<string, unknown>[]> (or generic T[]), and
update the DrizzleDb.select definition to return that builder so callers get
correct typings and IDE autocomplete.
In `@packages/eslint-config/package.json`:
- Line 18: Update the peer dependency floor for Prettier in package.json to be
less restrictive: replace the current "prettier": "^3.7.4" entry with a broader
range such as "prettier": ">=3.0.0" (or an equivalent semver range you prefer)
so consumers running any 3.x Prettier can use this config; locate the "prettier"
peer dependency entry in packages/eslint-config/package.json and adjust that
value accordingly.
---
Duplicate comments:
In `@packages/eslint-config/package.json`:
- Around line 15-17: Update the peerDependencies in package.json to allow ESLint
v10 consumers by expanding the "eslint" semver range (currently "eslint":
"^9.20.1") to include v10 (e.g., a range that permits 9.x and 10.x); ensure the
change is applied to the "peerDependencies" object and keep "@eslint/js" range
consistent or adjust similarly if needed so the package's flat config in
index.mjs remains compatible with ESLint v10.
| import type { Config } from 'drizzle-kit'; | ||
|
|
||
| export default { | ||
| schema: './src/schema/*', |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "package.json" | head -5Repository: pramodnarayana/nexiom
Length of output: 195
🌐 Web query:
drizzle-kit schema glob pattern configuration
💡 Result:
In drizzle-kit, the schema option in drizzle.config.ts can point to:
- a single file (e.g.
./src/db/schema.ts) (orm.drizzle.team) - a folder (drizzle-kit will load schema files from that folder recursively) (e.g.
./src/db/schema) (orm.drizzle.team) - glob patterns (and also an array mixing files/globs) (orm.drizzle.team)
Examples:
// drizzle.config.ts
import { defineConfig } from "drizzle-kit";
export default defineConfig({
dialect: "postgresql",
// single file
// schema: "./src/db/schema.ts",
// folder (recursive)
// schema: "./src/db/schema",
// glob
schema: "./src/db/schema/**/*.ts",
// or multiple inputs
// schema: ["./src/db/schema/**/*.ts", "./src/db/extra-schema.ts"],
});Note: schema (schema files path/glob) is different from schemaFilter (Postgres database schema names filter like "public" / "tenant_*"). (orm.drizzle.team)
Citations:
- 1: https://orm.drizzle.team/docs/sql-schema-declaration?utm_source=openai
- 2: https://orm.drizzle.team/docs/sql-schema-declaration?utm_source=openai
- 3: https://orm.drizzle.team/docs/drizzle-kit-push?utm_source=openai
- 4: https://orm.drizzle.team/docs/drizzle-config-file?utm_source=openai
🏁 Script executed:
fd -t f "drizzle.config" | head -10Repository: pramodnarayana/nexiom
Length of output: 163
🏁 Script executed:
cat -n packages/database/drizzle.config.tsRepository: pramodnarayana/nexiom
Length of output: 462
🏁 Script executed:
cat package.json | grep -A 50 '"dependencies"' | grep drizzleRepository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
fd -t f "package-lock.json\|yarn.lock\|pnpm-lock.yaml" | head -1Repository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
cat package.json | jq '.dependencies, .devDependencies' 2>/dev/null | grep -i drizzleRepository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
cat packages/database/package.json 2>/dev/null | jq '.dependencies, .devDependencies' 2>/dev/null | grep -i drizzleRepository: pramodnarayana/nexiom
Length of output: 120
🏁 Script executed:
cd packages/database && ls -laRepository: pramodnarayana/nexiom
Length of output: 415
🏁 Script executed:
find packages/database/src/schema -type f -name "*.ts" 2>/dev/null | head -20Repository: pramodnarayana/nexiom
Length of output: 144
🏁 Script executed:
cd packages/database && ls -la src/schema/Repository: pramodnarayana/nexiom
Length of output: 295
Fix glob pattern to include file extension: change './src/schema/*' to './src/schema/*.ts'
The glob pattern './src/schema/*' will not match the actual schema files (provider.ts, tenant.ts). Drizzle-kit requires glob patterns to include the file extension. Use './src/schema/*.ts' instead, or alternatively use './src/schema' to let drizzle-kit recursively load schema files from that directory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/database/drizzle.config.ts` at line 4, The schema glob in the
drizzle config (the schema property) is too broad ('./src/schema/*') and doesn't
match .ts files; update the schema value to include the file extension (change
to './src/schema/*.ts') or use './src/schema' to let drizzle-kit load the
directory recursively so files like provider.ts and tenant.ts are matched;
modify the schema property in the exported config accordingly.
Comment on lines
+7
to
+10
| dbCredentials: { | ||
| url: | ||
| process.env.DATABASE_URL || | ||
| 'postgres://admin:password123@localhost:5432/nexiom_master', |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hardcoded fallback credentials in source control.
This is a CLI-only config (migrations/studio), so the blast radius is smaller than runtime code, but password123 in version control is still a poor practice. Consider failing explicitly or using a .env-loaded default restricted to local development.
♻️ Proposed fix
dbCredentials: {
- url:
- process.env.DATABASE_URL ||
- 'postgres://admin:password123@localhost:5432/nexiom_master',
+ url: process.env.DATABASE_URL!,
},If a local fallback is desired, load it from a .env file (drizzle-kit supports dotenv integration via envFilePath) rather than embedding credentials in source.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/database/drizzle.config.ts` around lines 7 - 10, The
dbCredentials.url fallback currently embeds hardcoded credentials (the
DATABASE_URL fallback string) in drizzle.config.ts; remove the literal
'postgres://admin:password123@...' and either (a) throw/exit with a clear error
when process.env.DATABASE_URL is missing, or (b) configure dotenv/envFilePath
for drizzle-kit and read a non-committed local .env variable instead; update the
dbCredentials reference and any bootstrap code that expects DATABASE_URL so it
fails loudly or uses process.env loaded from the .env file rather than an
in-repo password.
Comment on lines
+1
to
+2
| import { drizzle } from 'drizzle-orm/node-postgres'; | ||
| import { type NodePgDatabase } from 'drizzle-orm/node-postgres'; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consolidate imports from the same module.
♻️ Proposed fix
-import { drizzle } from 'drizzle-orm/node-postgres';
-import { type NodePgDatabase } from 'drizzle-orm/node-postgres';
+import { drizzle, type NodePgDatabase } from 'drizzle-orm/node-postgres';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { drizzle } from 'drizzle-orm/node-postgres'; | |
| import { type NodePgDatabase } from 'drizzle-orm/node-postgres'; | |
| import { drizzle, type NodePgDatabase } from 'drizzle-orm/node-postgres'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/database/src/client.ts` around lines 1 - 2, Consolidate the two
separate imports from 'drizzle-orm/node-postgres' into a single import
statement: replace the separate "import { drizzle } from
'drizzle-orm/node-postgres';" and "import { type NodePgDatabase } from
'drizzle-orm/node-postgres';" with one combined import that brings in both
drizzle and the NodePgDatabase type (use the symbol names drizzle and
NodePgDatabase) to reduce redundancy and follow module import best practices.
| .from(providers) | ||
| .where(eq(providers.name, name)) | ||
| .limit(1); | ||
| return (rows[0] as InferSelectModel<typeof providers>) ?? null; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
as InferSelectModel<...> casts paper over the loose DrizzleDb interface.
These casts are needed because DrizzleDb (in types.ts) returns Record<string, unknown>[], erasing the schema-derived types. This is functional, but if the actual column set ever drifts from the InferSelectModel, the cast will silently hide the mismatch at compile time. Consider replacing DrizzleDb with NodePgDatabase<typeof schemaBundle> from the database package if cross-package type sharing is feasible, which would eliminate the need for these casts entirely.
Also applies to: 42-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/engine/src/connectivity/provider-registry.ts` at line 34, The cast
using InferSelectModel<typeof providers> masks potential schema mismatches
because DrizzleDb (the loose DB type) returns Record<string, unknown>[]; replace
the DrizzleDb usage with a strongly typed NodePgDatabase<typeof schemaBundle>
(from the database package) where the DB instance is typed, then remove the
manual casts (the return line that currently does return (rows[0] as
InferSelectModel<typeof providers>) ?? null and the similar cast at the other
occurrence). Update any function signatures or the registry constructor that
accept the DB to use NodePgDatabase<typeof schemaBundle> so the providers table
type is inferred directly and the InferSelectModel cast is no longer needed.
| abstract refresh(appName: string, refreshToken: string): Promise<Record<string, unknown>>; | ||
| } | ||
|
|
||
| import { DrizzleDb } from './types.js'; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Import placed after class declarations.
The DrizzleDb import on line 23 is separated from the other imports (lines 1-4) by the abstract class definitions. Move it to the top with the other imports for conventional module layout.
♻️ Proposed fix
import { Injectable, Logger, Inject, OnModuleDestroy } from '@nestjs/common';
import { appConnections } from '@nexiom/database';
import { eq } from 'drizzle-orm';
import Redis from 'ioredis';
+import { DrizzleDb } from './types.js';
// Abstract contracts — consumers must provide real implementations via DI
export abstract class EncryptionService {
@@ ...
export abstract class OAuthRefreshClient {
abstract refresh(appName: string, refreshToken: string): Promise<Record<string, unknown>>;
}
-import { DrizzleDb } from './types.js';
`@Injectable`()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/engine/src/connectivity/token-manager.service.ts` at line 23, The
DrizzleDb import is placed after the abstract class declarations; move the line
"import { DrizzleDb } from './types.js';" up into the main imports block so it
sits with the other top-of-file imports (grouped with the existing import
statements) to restore conventional module ordering and avoid interleaving
imports with class definitions.
Comment on lines
+31
to
+44
| /** Minimal typed interface for the injected Drizzle DB client. */ | ||
| export interface DrizzleDb { | ||
| query: { | ||
| appConnections: { | ||
| findFirst(args: Record<string, unknown>): Promise<Record<string, any> | undefined>; | ||
| }; | ||
| }; | ||
| insert(table: unknown): { values(data: Record<string, unknown>): { onConflictDoUpdate(args: Record<string, unknown>): Promise<unknown> } }; | ||
| update(table: unknown): { set(data: Record<string, unknown>): { where(condition: unknown): Promise<unknown> } }; | ||
| select(fields?: unknown): { | ||
| from(table: unknown): Promise<Record<string, unknown>[]> & { | ||
| where(condition: unknown): Promise<Record<string, unknown>[]> & { limit(n: number): Promise<Record<string, unknown>[]> }; | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check `@nexiom/database` exports and DrizzleDb usage
echo "=== `@nexiom/database` structure ==="
find packages/database -type f -name "*.ts" | head -20
echo ""
echo "=== `@nexiom/database/src/index.ts` ==="
if [ -f "packages/database/src/index.ts" ]; then
cat packages/database/src/index.ts
elif [ -f "packages/database/index.ts" ]; then
cat packages/database/index.ts
fi
echo ""
echo "=== Search for 'db' export ==="
rg -n "export.*\bdb\b" --type ts packages/database/ -A 3
echo ""
echo "=== DrizzleDb usage in services ==="
rg -n "DrizzleDb" --type ts packages/engine/ -B 2 -A 2
echo ""
echo "=== DI patterns with Drizzle ==="
rg -n "@InjectDrizzle|@Inject.*[Dd]rizzle|DRIZZLE" --type ts packages/ -B 1 -A 1Repository: pramodnarayana/nexiom
Length of output: 3538
Replace the hand-rolled DrizzleDb interface with typeof db from @nexiom/database.
Maintaining a manual structural approximation of Drizzle's API surface is fragile — every new operation added to token-manager.service.ts or provider-registry.ts requires updating this interface, and subtle mismatches go undetected until runtime. The @nexiom/database package already exports a configured db instance; its type can be inferred directly:
♻️ Proposed refactor
+import { db } from '@nexiom/database';
+
-/** Minimal typed interface for the injected Drizzle DB client. */
-export interface DrizzleDb {
- query: {
- appConnections: {
- findFirst(args: Record<string, unknown>): Promise<Record<string, any> | undefined>;
- };
- };
- insert(table: unknown): { values(data: Record<string, unknown>): { onConflictDoUpdate(args: Record<string, unknown>): Promise<unknown> } };
- update(table: unknown): { set(data: Record<string, unknown>): { where(condition: unknown): Promise<unknown> } };
- select(fields?: unknown): {
- from(table: unknown): Promise<Record<string, unknown>[]> & {
- where(condition: unknown): Promise<Record<string, unknown>[]> & { limit(n: number): Promise<Record<string, unknown>[]> };
- }
- };
-}
+export type DrizzleDb = typeof db;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** Minimal typed interface for the injected Drizzle DB client. */ | |
| export interface DrizzleDb { | |
| query: { | |
| appConnections: { | |
| findFirst(args: Record<string, unknown>): Promise<Record<string, any> | undefined>; | |
| }; | |
| }; | |
| insert(table: unknown): { values(data: Record<string, unknown>): { onConflictDoUpdate(args: Record<string, unknown>): Promise<unknown> } }; | |
| update(table: unknown): { set(data: Record<string, unknown>): { where(condition: unknown): Promise<unknown> } }; | |
| select(fields?: unknown): { | |
| from(table: unknown): Promise<Record<string, unknown>[]> & { | |
| where(condition: unknown): Promise<Record<string, unknown>[]> & { limit(n: number): Promise<Record<string, unknown>[]> }; | |
| } | |
| }; | |
| import { db } from '@nexiom/database'; | |
| export type DrizzleDb = typeof db; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/engine/src/connectivity/types.ts` around lines 31 - 44, The current
hand-written DrizzleDb interface is brittle; replace it with the real inferred
type by importing the configured db from `@nexiom/database` and exporting its
typeof as the DrizzleDb type. Specifically, remove the manual interface
declaration named DrizzleDb in types.ts, add an import for the exported db from
'@nexiom/database', and export a type alias like DrizzleDb = typeof db so
token-manager.service.ts and provider-registry.ts (and any other consumers) get
the exact Drizzle client type.
| export interface DrizzleDb { | ||
| query: { | ||
| appConnections: { | ||
| findFirst(args: Record<string, unknown>): Promise<Record<string, any> | undefined>; |
Contributor
There was a problem hiding this comment.
Record<string, any> leaks any into findFirst return type — use unknown.
The args parameter correctly uses unknown, but the return type uses any, which silently disables type checking on all properties accessed from the result. Make it consistent:
🛡️ Proposed fix
- findFirst(args: Record<string, unknown>): Promise<Record<string, any> | undefined>;
+ findFirst(args: Record<string, unknown>): Promise<Record<string, unknown> | undefined>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| findFirst(args: Record<string, unknown>): Promise<Record<string, any> | undefined>; | |
| findFirst(args: Record<string, unknown>): Promise<Record<string, unknown> | undefined>; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/engine/src/connectivity/types.ts` at line 35, The findFirst
declaration currently returns Promise<Record<string, any> | undefined>, which
leaks any; change its return type to Promise<Record<string, unknown> |
undefined> in the types.ts declaration for findFirst and update any direct
usages that assume concrete properties to narrow/cast the result (e.g., via type
guards or explicit casts) so callers handle unknown-safe access; keep the
function name findFirst and the args type Record<string, unknown> unchanged.
| findFirst(args: Record<string, unknown>): Promise<Record<string, any> | undefined>; | ||
| }; | ||
| }; | ||
| insert(table: unknown): { values(data: Record<string, unknown>): { onConflictDoUpdate(args: Record<string, unknown>): Promise<unknown> } }; |
Contributor
There was a problem hiding this comment.
insert().values() is not typed as awaitable — blocks plain await db.insert(...).values(...) at all call sites.
The current typing makes .values() return only { onConflictDoUpdate }, which is not a Promise or thenable. Any consumer calling await db.insert(table).values(data) without .onConflictDoUpdate() will get a TypeScript compile error, even though this is perfectly valid Drizzle usage. Drizzle's actual insert builder is a lazy executor that is directly awaitable and also chains to .returning() and .onConflictDoUpdate().
🔧 Proposed fix
- insert(table: unknown): { values(data: Record<string, unknown>): { onConflictDoUpdate(args: Record<string, unknown>): Promise<unknown> } };
+ insert(table: unknown): {
+ values(data: Record<string, unknown> | Record<string, unknown>[]): {
+ then: Promise<unknown>['then'];
+ returning(): Promise<Record<string, unknown>[]>;
+ onConflictDoUpdate(args: Record<string, unknown>): {
+ then: Promise<unknown>['then'];
+ returning(): Promise<Record<string, unknown>[]>;
+ };
+ };
+ };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/engine/src/connectivity/types.ts` at line 38, The types declare
insert(table).values(...) as returning only an object with onConflictDoUpdate,
which prevents plain await usage; change the type of values(...) so it is
thenable/awaitable and still exposes chainable methods (e.g. make it return
PromiseLike<unknown> & { onConflictDoUpdate(args: Record<string, unknown>):
Promise<unknown>; returning(...args: unknown[]): Promise<unknown> } or a similar
interface) so callers can do await db.insert(...).values(...) as well as chain
.onConflictDoUpdate(...) and .returning(...); update the signature for
insert(...) -> { values(data: Record<string, unknown>): PromiseLike<unknown> & {
onConflictDoUpdate(args: Record<string, unknown>): Promise<unknown>;
returning(...args: unknown[]): Promise<unknown> } } to match Drizzle's lazy
executor behavior.
Comment on lines
+40
to
+44
| select(fields?: unknown): { | ||
| from(table: unknown): Promise<Record<string, unknown>[]> & { | ||
| where(condition: unknown): Promise<Record<string, unknown>[]> & { limit(n: number): Promise<Record<string, unknown>[]> }; | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find and read the types.ts file
fd -g "types.ts" -path "*connectivity*" | head -5Repository: pramodnarayana/nexiom
Length of output: 233
🏁 Script executed:
#!/bin/bash
# Also locate the service files mentioned
fd -g "token-manager.service.ts" -o -g "provider-registry.ts"Repository: pramodnarayana/nexiom
Length of output: 181
🏁 Script executed:
#!/bin/bash
# Check the structure of the connectivity directory
find . -type f -name "*.ts" -path "*connectivity*" | head -20Repository: pramodnarayana/nexiom
Length of output: 224
🏁 Script executed:
#!/bin/bash
# Read the types.ts file
cat -n ./packages/engine/src/connectivity/types.ts | head -60Repository: pramodnarayana/nexiom
Length of output: 2003
🏁 Script executed:
#!/bin/bash
# Check token-manager.service.ts for DrizzleDb usage
cat -n ./packages/engine/src/connectivity/token-manager.service.tsRepository: pramodnarayana/nexiom
Length of output: 10691
🏁 Script executed:
#!/bin/bash
# Check provider-registry.ts for DrizzleDb usage
cat -n ./packages/engine/src/connectivity/provider-registry.tsRepository: pramodnarayana/nexiom
Length of output: 1952
🏁 Script executed:
#!/bin/bash
# Check package.json for drizzle-orm dependency
find . -name "package.json" -exec grep -l "drizzle-orm" {} \;Repository: pramodnarayana/nexiom
Length of output: 185
🏁 Script executed:
#!/bin/bash
# Look for any TypeScript errors or build config
ls -la ./packages/engine/ | head -20Repository: pramodnarayana/nexiom
Length of output: 348
🏁 Script executed:
#!/bin/bash
# Check if tsconfig exists and look for any error files
find ./packages/engine -name "tsconfig.json" -o -name "*.ts.error"Repository: pramodnarayana/nexiom
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Search for actual Drizzle ORM type definitions in node_modules or package references
rg "export.*SelectBuilder|export.*PgSelect|lazy.*query.*builder" --max-count=10Repository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check drizzle-orm in database package.json
cat ./packages/database/package.jsonRepository: pramodnarayana/nexiom
Length of output: 582
🏁 Script executed:
#!/bin/bash
# Check engine package.json
cat ./packages/engine/package.jsonRepository: pramodnarayana/nexiom
Length of output: 553
🏁 Script executed:
#!/bin/bash
# Check if there's a node_modules or import from actual database package
rg "from.*@nexiom/database" ./packages/engine/src --type ts -A 2 | head -30Repository: pramodnarayana/nexiom
Length of output: 672
🏁 Script executed:
#!/bin/bash
# Look at how DrizzleDb is actually instantiated/passed in
rg "DRIZZLE_DB|drizzle(" ./packages -A 3 -B 3 --type ts | head -50Repository: pramodnarayana/nexiom
Length of output: 153
🏁 Script executed:
#!/bin/bash
# Search for DRIZZLE_DB and drizzle instantiation
rg "DRIZZLE_DB|drizzle\(" ./packages -A 3 -B 3 --type ts | head -80Repository: pramodnarayana/nexiom
Length of output: 1933
🏁 Script executed:
#!/bin/bash
# Look for actual Drizzle type definitions - search the database package
find ./packages/database -name "*.ts" -type f | head -20Repository: pramodnarayana/nexiom
Length of output: 253
🏁 Script executed:
#!/bin/bash
# Check the main database exports
cat ./packages/database/dist/index.d.ts 2>/dev/null || cat ./packages/database/src/index.ts 2>/dev/null || find ./packages/database/src -name "*.ts" | head -5Repository: pramodnarayana/nexiom
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Check the actual client.ts to see the type of the drizzle instance
cat -n ./packages/database/src/client.tsRepository: pramodnarayana/nexiom
Length of output: 1725
🏁 Script executed:
#!/bin/bash
# Look for type definitions or exports from drizzle-orm in node_modules or type stubs
rg "class.*Select|interface.*Select" ./packages/database/src --type tsRepository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check if there's a way to see the actual type of what DrizzleDb should be
# Let's see what methods NodePgDatabase has by looking at what Drizzle exports
rg "NodePgDatabase" ./packages -A 5 -B 5 --type tsRepository: pramodnarayana/nexiom
Length of output: 45529
🏁 Script executed:
#!/bin/bash
# Search for any existing TypeScript error reports or build logs
find ./packages/engine -name "*.log" -o -name "*.error" -o -name ".tsc-errors"Repository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Try to understand the actual structural typing issue
# Check if the DrizzleDb type is actually used elsewhere or if there are casting issues
rg "as DrizzleDb|: DrizzleDb" ./packages --type ts -A 2 -B 2Repository: pramodnarayana/nexiom
Length of output: 971
🏁 Script executed:
#!/bin/bash
# Let's check if drizzle-orm v0.45.1 has specific SelectBase types
# by looking at what's available in the imports
rg "from 'drizzle-orm" ./packages -h --type ts | sort -uRepository: pramodnarayana/nexiom
Length of output: 9340
🏁 Script executed:
#!/bin/bash
# Let me check if there's an actual DI provider that assigns a real Drizzle instance to DRIZZLE_DB
rg "DRIZZLE_DB|provide.*DRIZZLE" ./packages -A 5 -B 5 --type ts | head -100Repository: pramodnarayana/nexiom
Length of output: 2105
🏁 Script executed:
#!/bin/bash
# Look for module setup or providers configuration
find ./packages -name "*.module.ts" -type f | xargs grep -l "DRIZZLE_DB" 2>/dev/nullRepository: pramodnarayana/nexiom
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Search for where providers are defined for the engine package
find ./packages/engine/src -name "*.module.ts" -o -name "*.ts" | xargs grep -l "provide\|Provider" | head -10Repository: pramodnarayana/nexiom
Length of output: 255
🏁 Script executed:
#!/bin/bash
# Look for connectivity module or any configuration that sets up DRIZZLE_DB
rg "Module|provide|Provider" ./packages/engine/src/connectivity --type ts -B 2 -A 2Repository: pramodnarayana/nexiom
Length of output: 7193
🏁 Script executed:
#!/bin/bash
# Look at the app-level module to see if DRIZZLE_DB is provided there
find ./packages -name "*.module.ts" -type f | head -10Repository: pramodnarayana/nexiom
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Check the engine's index.ts to see what's exported
cat -n ./packages/engine/src/index.tsRepository: pramodnarayana/nexiom
Length of output: 227
🏁 Script executed:
#!/bin/bash
# Search for where DRIZZLE_DB or any provider configuration happens
rg "provide.*DRIZZLE|DRIZZLE.*provide|\{.*provide" ./packages -A 3 --type ts | head -60Repository: pramodnarayana/nexiom
Length of output: 3234
🏁 Script executed:
#!/bin/bash
# Check if there's a main app module that provides DRIZZLE_DB
find ./apps -name "*.module.ts" -type f | xargs grep -l "DRIZZLE_DB\|engine" 2>/dev/null | head -5Repository: pramodnarayana/nexiom
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Check the app module to see the DRIZZLE_DB provider
cat -n ./apps/api/src/app/app.module.ts | head -100Repository: pramodnarayana/nexiom
Length of output: 3320
🏁 Script executed:
#!/bin/bash
# Also search for where the actual Drizzle instance is assigned to DRIZZLE_DB token
rg "DRIZZLE_DB.*useValue|useFactory.*DRIZZLE" ./apps -A 5 -B 5 --type tsRepository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check the DbModule to see how DRIZZLE_DB is provided
find ./apps/api/src -name "db.module.ts" -o -name "database.module.ts" | xargs catRepository: pramodnarayana/nexiom
Length of output: 276
🏁 Script executed:
#!/bin/bash
# Search for the actual provider definition
rg "provide.*DRIZZLE|DRIZZLE_DB" ./apps/api/src -A 5 -B 2 --type tsRepository: pramodnarayana/nexiom
Length of output: 2792
🏁 Script executed:
#!/bin/bash
# Check the full db.provider.ts file
cat -n ./apps/api/src/db/db.provider.tsRepository: pramodnarayana/nexiom
Length of output: 716
🏁 Script executed:
#!/bin/bash
# Now I understand the setup. The DRIZZLE_DB token is provided with a real NodePgDatabase instance
# Let me verify what the actual Drizzle builder type looks like at runtime
# by checking if there are any compilation errors in the test files
rg "db\.select|db\.insert|db\.update" ./apps/api/src/modules/engine --type ts -A 2 | head -60Repository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Let me verify if there are any actual TypeScript compilation errors by checking if the codebase builds
# Look for any reported type errors or check if there's a tsconfig that enforces strict typing
find ./packages/engine -name "tsconfig.json" -exec cat {} \;Repository: pramodnarayana/nexiom
Length of output: 458
🏁 Script executed:
#!/bin/bash
# Check if ProviderRegistryService methods compile without errors - look for return type assertions
rg "as.*\[|as.*unknown|as InferSelectModel" ./packages/engine/src/connectivity/provider-registry.tsRepository: pramodnarayana/nexiom
Length of output: 303
🏁 Script executed:
#!/bin/bash
# Let me do a final check - see if there are actual type errors by looking at what
# the actual Drizzle select return type is vs what DrizzleDb claims
# Check if rows are being used as Promise directly anywhere
rg "await.*db\.select|await.*db\.insert" ./packages/engine --type ts -B 2 -A 2Repository: pramodnarayana/nexiom
Length of output: 47
Interface is incomplete, not broken—but improve typing for better maintainability.
The DrizzleDb.select() interface is imprecise rather than structurally incompatible. While Drizzle's lazy builders are thenable and the current usage patterns work, the type signature is incomplete and masks the actual return type. Update to use PromiseLike<T> and model the full builder chain to improve type safety and prevent future misuse:
Suggested improvement — better model the lazy query builder
- select(fields?: unknown): {
- from(table: unknown): Promise<Record<string, unknown>[]> & {
- where(condition: unknown): Promise<Record<string, unknown>[]> & { limit(n: number): Promise<Record<string, unknown>[]> };
- }
- };
+ select(fields?: unknown): {
+ from(table: unknown): PromiseLike<Record<string, unknown>[]> & {
+ where(condition: unknown): PromiseLike<Record<string, unknown>[]> & {
+ limit(n: number): PromiseLike<Record<string, unknown>[]>;
+ orderBy(...args: unknown[]): PromiseLike<Record<string, unknown>[]>;
+ };
+ limit(n: number): PromiseLike<Record<string, unknown>[]>;
+ orderBy(...args: unknown[]): PromiseLike<Record<string, unknown>[]>;
+ };
+ };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/engine/src/connectivity/types.ts` around lines 40 - 44, The current
select() declaration on DrizzleDb is too generic and hides the lazy query
builder behavior; update the select signature to return a typed
PromiseLike/thenable builder that models the full chain (select(...) ->
from(...) -> where(...) -> limit(...)) and preserves the real result type T
instead of unknown. Replace the nested anonymous object return types for
select/from/where/limit with a generic builder interface (e.g., QueryBuilder<T>
or PromiseLike<T[]>) that implements then/ catch/ finally and exposes
from(table), where(condition) and limit(n) methods returning the same builder
typed as PromiseLike<Record<string, unknown>[]> (or generic T[]), and update the
DrizzleDb.select definition to return that builder so callers get correct
typings and IDE autocomplete.
| "peerDependencies": { | ||
| "@eslint/js": "^9.20.0", | ||
| "eslint": "^9.20.1", | ||
| "prettier": "^3.7.4", |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prettier peer floor ^3.7.4 is more restrictive than necessary.
3.7.4 was released on Dec 3, 2025, while the current latest is 3.8.1. The ^3.7.4 specifier will correctly resolve to 3.8.1 in the workspace, but it would reject consumers on any 3.0.0–3.7.3 release without a clear justification. The prior suggestion was >=3.0.0; since this is a formatting-only config with no API surface tied to a specific prettier version, a broader floor reduces friction:
♻️ Proposed floor relaxation
- "prettier": "^3.7.4",
+ "prettier": ">=3.0.0",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "prettier": "^3.7.4", | |
| "prettier": ">=3.0.0", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/eslint-config/package.json` at line 18, Update the peer dependency
floor for Prettier in package.json to be less restrictive: replace the current
"prettier": "^3.7.4" entry with a broader range such as "prettier": ">=3.0.0"
(or an equivalent semver range you prefer) so consumers running any 3.x Prettier
can use this config; locate the "prettier" peer dependency entry in
packages/eslint-config/package.json and adjust that value accordingly.
pramodnarayana
marked this pull request as draft
February 20, 2026 15:03
…t typing analysis and drizzle schema validation
pramodnarayana
marked this pull request as ready for review
February 20, 2026 15:24
pramodnarayana
marked this pull request as draft
February 20, 2026 15:24
pramodnarayana
marked this pull request as ready for review
February 20, 2026 15:30
This was referenced Feb 21, 2026
This was referenced Feb 22, 2026
This was referenced Mar 4, 2026
Merged
This was referenced Mar 17, 2026
Merged
Merged
This was referenced Mar 25, 2026
Merged
Merged
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.
Initializes the database, engine, and integration packages. Connects TokenManagerService and OAuthCallbackController. Ensures Turborepo layer compliance.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores