Repository navigation
Refactor/tdd core services - #152
Merged
Merged
Conversation
Contributor
|
Worried about impact? Review this PR in Change Stack to explore blast radius before you approve or request changes. 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:
📝 WalkthroughWalkthroughLarge cross-cutting refactor: adds a Nest-based DB CLI and tenant tooling; implements OAuth/credential controllers, credential linking with savepoints and lifecycle orchestration; introduces scheduler and trigger abstractions and Redis-backed implementations; refactors worker pipeline into router/batch processor with delivery retry and GEM hydration; adds savepoint, lock, DLQ, KV, and event publisher contracts; updates many tests and Vitest configs. ChangesPlatform Refactor and Tooling
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Owner
Author
|
coderabbitai review |
pramodnarayana
force-pushed
the
refactor/tdd-core-services
branch
from
June 5, 2026 11:39
ac620ff to
8b71c36
Compare
![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: 36
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts (1)
352-375: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd coverage for the blocked deletion path (
success: false).This spec now tracks
deleteIfNotLastAdmin, but it doesn’t assert the “last admin cannot be deleted” branch. Add one test to lock this behavior.Suggested test case
describe('deleteUser', () => { @@ it('should delete user', async () => { @@ expect(mockUserProvider.deleteIfNotLastAdmin).toHaveBeenCalledWith( 'u1', getRequiredSystemTenantId(), ); }); + + it('should throw BadRequestException when deleting the last platform admin', async () => { + mockUserProvider.findById.mockResolvedValue({ id: 'u1' }); + mockUserProvider.deleteIfNotLastAdmin.mockResolvedValue({ + success: false, + }); + + await expect(controller.deleteUser('u1')).rejects.toThrow( + BadRequestException, + ); + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts` around lines 352 - 375, Add a test for the "blocked deletion" path in the deleteUser suite: mock mockUserProvider.findById to return a user (e.g., {id: 'u1'}) and mock mockUserProvider.deleteIfNotLastAdmin to resolve with { success: false }, then assert that controller.deleteUser('u1') rejects (e.g., toThrow ConflictException) and that mockUserProvider.deleteIfNotLastAdmin was called with 'u1' and getRequiredSystemTenantId(); this covers the "last admin cannot be deleted" branch for deleteUser.apps/api/src/modules/exceptions/exception.service.ts (1)
322-369:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDo not perform queue dispatch inside the DB transaction.
dispatchRetryis an external side effect inside the transaction scope. If dispatch succeeds but commit fails, the retry is enqueued while DB state rolls back, causing duplicate/invalid retry behavior.Prefer a transactional outbox pattern: persist retry intent in the same transaction, then dispatch asynchronously after commit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/exceptions/exception.service.ts` around lines 322 - 369, The code currently calls this.queueDispatcher.dispatchRetry inside the this.db.transaction block (using outboundGateway, outboundGatewayId, updatedRows), causing an external side effect during the DB transaction; instead, persist a retry intent (an outbox row referencing outboundGatewayId, traceId, routeId, payload, stitch.destDataSourceId, srcDataSourceId and a status like 'PENDING') inside the transaction (within the same block that updates outboundGateway) and remove the call to queueDispatcher.dispatchRetry there; after the transaction completes, asynchronously read the outbox entry and call this.queueDispatcher.dispatchRetry (catching errors and updating the outbox row status to 'SENT' or 'FAILED') so dispatch happens only after commit and can be retried safely by an outbox worker or on failure.apps/worker/src/modules/pipeline/delivery.service.ts (1)
569-592: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMake
tenantDbrequired on the public method signature.Line 592 throws when
tenantDbis omitted, sotenantDb?: DrizzleDbis lying to callers. SincewriteL6Result()is now public, keep this as a type-level requirement instead of a runtime trap.Suggested signature cleanup
public async writeL6Result( destSchemaName: string, srcSchemaName: string, outboundGatewayId: string, dataSourceId: string, traceId: string, routeId: string, resPayload: Record<string, unknown> | null, sentPayload: Record<string, unknown> | null, statusCode: number, finalStatus: "SUCCESS" | "FAIL" | "RETRY", start: number, destVendorId: string | undefined, canonicalType: string, srcAppName: string, srcTenantId: string, srcVendorId: string | undefined, targetConnectionId: string, targetAppName?: string, targetTenantId?: string, targetObject?: string, - tenantDb?: DrizzleDb, + tenantDb: DrizzleDb, ): Promise<boolean> { - if (!tenantDb) throw new Error("tenantDb is required for writeL6Result");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/worker/src/modules/pipeline/delivery.service.ts` around lines 569 - 592, The public method writeL6Result currently declares tenantDb as optional (tenantDb?: DrizzleDb) but immediately throws if it's missing; change the signature to require tenantDb (tenantDb: DrizzleDb) and remove the runtime guard (the if (!tenantDb) throw ...). Update any callers that relied on it being optional to pass a valid DrizzleDb instance; keep the function name writeL6Result and all other parameters unchanged.apps/worker/src/modules/pipeline/inbound-outbox.poller.ts (1)
92-119:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRestore a per-claim ownership token.
This change stops mutating any version/lease field when a row is claimed. Once
nextRetryAtexpires, another poller can reclaim the same row with the sameattemptsvalue, and the first worker can still match the completion updates on Line 188 and Lines 207-225 because those predicates only checkidplusstatus='PROCESSING'(or the unchangedattempts). That lets stale workers overwrite the newest attempt outcome. Keep failure-count semantics if you want, but add a separate lease token / claim version and require it in the success/failure updates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/worker/src/modules/pipeline/inbound-outbox.poller.ts` around lines 92 - 119, The claim logic must set and return a unique per-claim lease token so later completion updates require that token (preventing stale workers from overwriting newer outcomes): modify the claiming transaction around tenantDb.transaction / update(inboundOutbox) to SET a new claim_token (e.g. a generated UUID) alongside status="PROCESSING" and nextRetryAt, and include that claim_token in the returning payload; then update the completion/failure update paths (the code that currently checks id and status='PROCESSING' in the success/failure handlers referenced around the completion updates) to include AND claim_token = <returned token> in their WHERE clauses; do not remove attempts handling (keep attempts only updated on real delivery failure) but require the claim_token match for any terminal updates.packages/identity/src/adapters/drizzle-tenant.adapter.spec.ts (1)
127-160: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert the new publisher side effect in
create()tests.The adapter now depends on
IIdentityEventPublisher, but this spec doesn’t verify thatpublishTenantProvisionedis invoked. Capturing the mock and asserting one publish call will prevent silent regressions.Suggested test tweak
- const adapter = new DrizzleTenantAdapter( - db as unknown as NodePgDatabase<typeof schema>, - mkPublisher(), - ); + const publisher = mkPublisher(); + const adapter = new DrizzleTenantAdapter( + db as unknown as NodePgDatabase<typeof schema>, + publisher, + ); @@ const tenant = await adapter.create("user-1", "Acme"); @@ expect(tenant.name).toBe("Acme"); expect(tenant.slug).toMatch(/^acme-/); expect(tx.insert).toHaveBeenCalled(); + expect(publisher.publishTenantProvisioned).toHaveBeenCalledTimes(1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/identity/src/adapters/drizzle-tenant.adapter.spec.ts` around lines 127 - 160, The test for DrizzleTenantAdapter.create currently doesn't assert the publisher side effect; update the "create creates organization and admin member, handles slug collision retries" spec to capture the mock IIdentityEventPublisher returned by mkPublisher(), call the adapter.create("user-1","Acme") as before, then assert that publishTenantProvisioned (the publisher mock method) was called exactly once with the provisioned tenant (or at least with expected properties like tenant.id or slug/name) to prevent regressions; reference the DrizzleTenantAdapter instance, the create(...) call, and the publisher mock (from mkPublisher) when adding the assertion.packages/credentials/src/oauth/token-refresh.service.ts (1)
84-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t map all
TypeErrors to HTTP 400 input validation failures.
BaseOAuthRefreshClient.refresh()converts anyTypeErrorintoOAuthRefreshError(..., 400), butfetch()commonly rejects withTypeErrorfor network/transport and request-configuration problems—so transport faults get misclassified as “bad input” (and retry behavior/diagnostics can break).🔧 Suggested fix
+class RefreshInputError extends TypeError {} + private validateInputs(tenantId: string, appName: string, externalId: string, refreshToken: string): void { - if (typeof tenantId !== 'string' || !tenantId.trim()) throw new TypeError('Invalid refresh input: tenantId'); - if (typeof appName !== 'string' || !appName.trim()) throw new TypeError('Invalid refresh input: appName'); - if (typeof externalId !== 'string' || !externalId.trim()) throw new TypeError('Invalid refresh input: externalId'); - if (typeof refreshToken !== 'string' || !refreshToken.trim()) throw new TypeError('Invalid refresh input: refreshToken'); + if (typeof tenantId !== 'string' || !tenantId.trim()) throw new RefreshInputError('Invalid refresh input: tenantId'); + if (typeof appName !== 'string' || !appName.trim()) throw new RefreshInputError('Invalid refresh input: appName'); + if (typeof externalId !== 'string' || !externalId.trim()) throw new RefreshInputError('Invalid refresh input: externalId'); + if (typeof refreshToken !== 'string' || !refreshToken.trim()) throw new RefreshInputError('Invalid refresh input: refreshToken'); } @@ - if (error instanceof TypeError) { + if (error instanceof RefreshInputError) { throw new OAuthRefreshError(`Invalid refresh input: ${error.message}`, 400); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/credentials/src/oauth/token-refresh.service.ts` around lines 84 - 86, The code in BaseOAuthRefreshClient.refresh currently maps every TypeError to OAuthRefreshError(..., 400); instead, only translate TypeError instances that originate from input validation into a 400 OAuthRefreshError (e.g., those thrown by your input validation routine), and for other TypeErrors (fetch/transport/configuration) either rethrow the original error or wrap it with a non-4xx status (e.g., 502) so transport faults are not misclassified. Concretely, in BaseOAuthRefreshClient.refresh() inspect the TypeError (by origin: check error.source, error.name, or error.message or compare to the specific validation function that throws) and only create OAuthRefreshError(..., 400) for validation-related messages; otherwise rethrow the TypeError or throw an OAuthRefreshError with a 5xx code so retry/diagnostics remain correct.packages/credentials/src/oauth/token-manager.service.ts (1)
318-324:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize JSON parse failures to
AppCredentialError.Line 318 can throw raw
SyntaxErrorfromJSON.parse, bypassing your domain error path. This creates inconsistent behavior for malformed/corrupt payloads.Suggested fix
- const parsed: unknown = JSON.parse(await this.crypto.decrypt(connection.value)); - if (!isOAuthCredentialBlob(parsed)) { - throw new AppCredentialError( - 'Stored credentials are malformed and do not match OAuthCredentialBlob. Re-authorization required.', - ); - } - return parsed; + let parsed: unknown; + try { + parsed = JSON.parse(await this.crypto.decrypt(connection.value)); + } catch { + throw new AppCredentialError( + 'Stored credentials are malformed and do not match OAuthCredentialBlob. Re-authorization required.', + ); + } + if (!isOAuthCredentialBlob(parsed)) { + throw new AppCredentialError( + 'Stored credentials are malformed and do not match OAuthCredentialBlob. Re-authorization required.', + ); + } + return parsed;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/credentials/src/oauth/token-manager.service.ts` around lines 318 - 324, Wrap the JSON.parse(await this.crypto.decrypt(connection.value)) call in a try/catch so that any SyntaxError or other parsing errors are caught and rethrown as an AppCredentialError; specifically, inside the method where parsed is created (referencing parsed, this.crypto.decrypt, JSON.parse, isOAuthCredentialBlob), catch parse errors and throw new AppCredentialError with a clear message (and include the original error/message for debugging) before proceeding to the existing isOAuthCredentialBlob check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/db/cli/commands/db-debug.command.ts`:
- Around line 13-42: Change the Option flags to require values (so parseUser and
parseRole always receive strings), then in run() enforce mutual exclusivity (if
both options.user and options.role are provided, print usage/error and
process.exit(1)). Validate that options.user/options.role are strings before
calling rbacInspectorService.checkUserPermissions or
rbacInspectorService.debugPermissions and treat non-string/true values as
missing (error + process.exit(1)). Also handle service failures or "not found"
results by logging the error via console.error and calling process.exit(1)
instead of returning so the command exits with a non-zero code. Use the symbols
parseUser, parseRole, run, rbacInspectorService.checkUserPermissions, and
rbacInspectorService.debugPermissions to locate the changes.
In `@apps/api/src/db/cli/commands/db-provision.command.ts`:
- Around line 61-70: Validate the provided flags before executing any
provisioner: ensure exactly one of options.local, options.gateway, or
options.outbound is set (reject combinations like both gateway and outbound),
require options.schema when calling schemaProvisioner.provisionGateway or
schemaProvisioner.provisionOutbound, and on invalid input print a corrected
usage message (e.g., "Usage: db:provision --local | --gateway --schema <schema>
| --outbound --schema <schema>") and exit with a non-zero status; update the
branch in db-provision.command.ts to perform this validation prior to calling
sandboxProvisioner.provisionLocal, schemaProvisioner.provisionGateway, or
schemaProvisioner.provisionOutbound.
In `@apps/api/src/db/infrastructure/migration-runner.service.ts`:
- Line 41: The current urlWithDb built by concatenating hostUrl and dbName can
produce invalid or wrong DSNs when either contains URL-sensitive characters;
update the logic in migration-runner.service.ts to construct the database URL
with the URL API: parse hostUrl with new URL(...), normalize the existing
pathname (remove trailing slash if present), append the dbName as a path segment
using URL-safe encoding (e.g., encodeURIComponent on dbName or setting pathname
to the normalized path + '/' + encoded dbName), then use url.toString() as
urlWithDb so hostUrl, dbName, and any URL components are handled safely.
In `@apps/api/src/db/services/connection-schema-provisioner.service.ts`:
- Around line 137-140: The migrateAllSchemas method in
connection-schema-provisioner.service.ts starts a bulk, write-heavy migration
but is missing the environment safety check; update migrateAllSchemas to call
the existing assertSafeEnvironment() guard at the start (the same pattern used
in the other methods around the earlier calls) so the method aborts outside safe
environments before proceeding with schema migration to OUTBOUND_ACTIVE.
- Around line 151-152: The query uses WHERE schema_name LIKE 'ws_%' which treats
'_' as a single-character wildcard; update the SQL in the method that
builds/fetches schema names (look for functions like getSchemasToMigrate /
fetchSchemas in connection-schema-provisioner.service or the SQL string
variable) to match a literal underscore by escaping it, e.g. use WHERE
schema_name LIKE 'ws\_%' ESCAPE '\' (or use schema_name LIKE 'ws\_' || '%' with
an explicit escape) so only schemas starting with the literal "ws_" are
returned.
- Around line 41-50: The created per-tenant pg.Pool in
TenantDatabaseManager.dbFactory is never closed and leaks connections; fix by
ensuring the pool is closed after provisioning: either register the created pool
with TenantDatabaseManager so existing
TenantDatabaseManager.closeTenantDb()/closeAll() will call pool.end(), or
explicitly await pool.end() in the finally block that wraps withSchemaMgr (the
same finally that currently only calls client.end()); update dbFactory (where
const pool = new Pool(...) and drizzle(pool,...)) to call the registration
helper or return an object that allows the caller to close the pool so the
caller can invoke await pool.end() in its finally. Ensure you reference/modify
TenantDatabaseManager.dbFactory, the pool variable, drizzle(...),
withSchemaMgr's finally, and TenantDatabaseManager.closeTenantDb()/closeAll()
accordingly.
In `@apps/api/src/db/services/dev-sandbox-provisioner.service.ts`:
- Around line 102-107: The code that constructs hostUrl/tenantUrl using new
URL() strips credentials (username/password) so live DB connections fail; update
the logic in dev-sandbox-provisioner.service.ts (where parsedUrl, hostUrl and
tenantUrl are built) to preserve credentials when present by including
parsedUrl.username and parsedUrl.password (or the original
process.env.DATABASE_URL) in the returned connection string/origin used for
actual connections; adjust both the hostUrl construction around parsedUrl and
the analogous code at the later block (lines referenced 205-210) so that
authentication info is retained for authenticated Postgres URLs.
- Around line 216-223: The resolver passed into TenantDatabaseManager currently
creates a new Pool (using new Pool({ connectionString: tenantUrl })) on every
call which leaks connections; change it to use a cached pool/drizzle per host
identifier (e.g. maintain a Map<string, Pool|Drizzle> keyed by _hostIdentifier
inside the same module), create the Pool and drizzle instance only if absent,
return the cached drizzle instance for subsequent calls, and ensure you provide
a companion cleanup path to end and delete the Pool when a tenant is removed;
update references to Pool, drizzle, TenantDatabaseManager, tenantUrl, and
dbSchema to use this cache.
In `@apps/api/src/db/services/sync-seeding.service.ts`:
- Around line 25-31: Current code picks an arbitrary workspace with
db.select().from(schema.uiWorkspaces).limit(1) and uses its id/orgId for seeding
(workspaceId/orgId), which is unsafe in multi-tenant setups; update the logic to
deterministically resolve the target workspace: either require an explicit
workspace identifier passed into the seeding function (e.g., a workspaceId
parameter or env var) and validate it exists, or query for a canonical workspace
(e.g., WHERE is_default = true or name = 'default') and throw an error if none
or multiple matches are found; ensure you replace the use sites of workspaceId
and orgId in sync-seeding.service.ts accordingly and add clear error messages
when resolution fails.
In `@apps/api/src/db/services/system-seeder.service.ts`:
- Around line 226-243: The code finds an existing member via
db.query.member.findFirst and only inserts a new row if absent, but it doesn't
promote an existing non-owner role to the owner role; update the logic in
system-seeder.service (around the existingMember check) so that if
existingMember exists and existingMember.role !== config.ownerRoleId you perform
an update on schema.member to set role = config.ownerRoleId (and probably
updatedAt) for that member id (or use db.update with a where on userId and
organizationId); otherwise keep the current insert-with-onConflictDoNothing flow
using db.insert(schema.member).values(...).onConflictDoNothing().
- Around line 76-81: The monorepo root calculation in system-seeder.service.ts
is too shallow: update the monorepoRoot resolution (the const monorepoRoot
derived from fileURLToPath(import.meta.url) / path.resolve) to climb one more
level so it reaches the repository root (fix the '../../../../' to the correct
depth) so that piecePaths (used for packages/pieces/application and
packages/pieces/platform) resolve correctly for piece discovery.
- Around line 346-348: The ABAC condition currently compares role to the
hardcoded string 'owner' — update the code to pull the owner role id from a
configuration or constant instead of using the literal; e.g., obtain the owner
role id via your config/service (ConfigService.get('OWNER_ROLE_ID') or a defined
OWNER_ROLE_ID constant) and replace the literal in the condition where you build
AbacConditions (the object with role: { $ne: 'owner' }) so it uses role: { $ne:
ownerRoleId } (keep the AbacConditions type), add a sensible fallback if the
config is missing, and ensure any tests or seed logic that reference this
condition are updated to use the same config-driven identifier.
In `@apps/api/src/db/services/tenant-schema.service.ts`:
- Around line 72-77: The DROP DATABASE SQL builds an identifier by interpolating
dbName directly into the query (the adminClient.query call that currently uses
`DROP DATABASE IF EXISTS "${dbName}"`), which is unsafe; change this to emit a
properly escaped identifier using an identifier formatter such as pg-format
(import format from 'pg-format') and call adminClient.query(format('DROP
DATABASE IF EXISTS %I', dbName)); keep the existing parameterized
terminate-backend query as-is and only replace the DROP DATABASE string
construction where adminClient.query is called.
In `@apps/api/src/modules/connections/connection-lifecycle.service.ts`:
- Around line 226-256: The catch block in connection-lifecycle.service.ts
currently re-throws ConflictException but fails to re-throw ForbiddenException,
allowing cross-tenant ForbiddenException to be swallowed and misclassified;
update the catch in the method containing that try/catch so that if (err
instanceof ForbiddenException) throw err; is executed before the schema/pgCode
checks (similar to the existing ConflictException handling), ensuring
ForbiddenException is propagated and not treated as a "schema does not exist"
warning.
- Around line 116-142: The transaction is always started even when
workspaceProvisionInfo.createdAppConnection is false, creating an empty
transaction scope; change the code to only call this.db.transaction(...) when
workspaceProvisionInfo.createdAppConnection is true (i.e., guard around the
transaction) so the updates/inserts for dataSources, credentials, and
globalRegistryOutbox (use of dataSources, credentials, globalRegistryOutbox,
AppConnectionStatus.FAILED, and failedConn) only run inside an actual
transaction triggered when createdAppConnection is true, or alternatively move
the conditional before the transaction and return early to avoid opening a no-op
transaction.
In `@apps/api/src/modules/connections/connections/credential.controller.ts`:
- Around line 179-193: The list endpoint is currently selecting the encrypted
blob `credentials.value` (in the select mapping that includes id, appName,
authType, etc.), which reads sensitive data into memory; change the select to
omit `credentials.value` and instead project a boolean (e.g., `hasValue` or
`valuePresent`) that indicates whether a value exists by using a NULL-check
expression or ORM `isNotNull()`/raw SQL (do this for the same select usage later
around the other occurrence). Update the select mapping in the credential
controller (where `credentials.value` is referenced) to remove the blob and
return only the computed boolean flag.
In `@apps/api/src/modules/connections/connections/oauth.controller.ts`:
- Around line 43-44: The externalId generation uses randomBytes(2)
(uniqueSuffix) which only provides 65k variants and risks collisions; change the
entropy to a larger size (e.g., randomBytes(4) or randomBytes(6)) when
constructing the externalId (`uniqueSuffix` used with `providerName` and
`baseSlug`) to drastically reduce collisions, update the other occurrence noted
(the second use at the same symbol/location), and run/update any tests or
consumers that assume the suffix length/format so the return value
`${providerName}-${baseSlug}-${uniqueSuffix}` remains stable except for the
longer hex suffix.
- Around line 511-515: mergedMetadata currently lets request-derived
aliasAppProfile (from body.providerName) overwrite verified state metadata;
change the merge so verified metadata remains authoritative: use the metadata
returned by verifyState as the base and only add appProfile from aliasAppProfile
when metadata.appProfile is undefined/null (i.e., do not spread aliasAppProfile
unconditionally). Keep originalProviderName and other metadata fields, and
reference mergedMetadata, aliasAppProfile, metadata and verifyState when
locating the code to update.
In `@apps/api/src/modules/connections/connectors.service.ts`:
- Around line 424-426: Replace the plain Error thrown when connection is falsy
with a NestJS InternalServerErrorException: change throw new Error('Failed to
retrieve connection ID after insert') to throw new
InternalServerErrorException('Failed to retrieve connection ID after insert') in
the ConnectorsService where the `connection` result is checked; also add an
import for InternalServerErrorException from '`@nestjs/common`' if not already
present so the code remains consistent with other internal failure handling
(e.g., the similar "connection reference lost" case).
In `@apps/api/src/modules/connections/services/credential-linking.service.ts`:
- Around line 314-355: The recovery branch for existingFailed updates
dataSources and inserts credentials but omits the globalRegistryOutbox upsert
present in the other write paths; after the credentials insert (and before
setting connection = updated) add the same tx upsert to globalRegistryOutbox
that the other branches use (i.e., perform the identical
insert(...).onConflictDoUpdate(...) against globalRegistryOutbox via tx, using
updated.id / dataSourceId and the same payload/columns/event metadata) so
recovered rows emit the same outbox event as the non-failed paths.
In `@apps/api/src/modules/connections/services/oauth-orchestration.service.ts`:
- Around line 172-203: handleTokenExchangeError currently includes vendor
response body in exceptions sent to clients; keep the detailed sanitizedError
only in logs and throw generic client-facing messages instead. Update
handleTokenExchangeError: retain building/sanitizing errorBody and call
this.logger.error(...) with the detailed message (errorMessage), but when
throwing BadRequestException/UnauthorizedException/HttpException use a generic
message like `Failed to exchange code with ${providerName}` (or include only
providerName and status) instead of the sanitizedError; keep the original status
codes for 4xx responses, and continue to throw InternalServerErrorException with
a generic message for non-4xx errors. Ensure no vendor body or sanitizedError is
included in any thrown exception.
In `@apps/api/src/modules/scheduler/infrastructure/windmill-scheduler.client.ts`:
- Around line 265-272: The request currently pre-serializes the payload by
calling JSON.stringify(body) before passing it to this.httpClient.request (in
the method that builds the request using this.url(path), this.token and
REQUEST_TIMEOUT_MS), which causes double-encoding because
FetchHttpClient.request already serializes bodies; remove the JSON.stringify
call and pass the original body value directly (keep the conditional
'Content-Type: application/json' header as before) so the body is serialized
only once by FetchHttpClient.request.
In `@apps/api/src/modules/trigger/infrastructure/redis-trigger-dlq.service.ts`:
- Around line 115-117: Both scheduleDelayedRetry and markJobFailed currently
perform LREM then a second enqueue call separately, risking job loss if the
process dies between calls; replace each two-step sequence with a single Redis
EVAL Lua script that performs the LREM and the enqueue atomically. For
scheduleDelayedRetry update the method to call this.redis.eval with a Lua script
that LREM DLQ_PROCESSING_KEY 1 <raw> and, if the LREM removed an item, ZADD
DLQ_DELAYED_KEY <score=(Date.now()+delayMs)> <retryPayload>, returning a status;
for markJobFailed do the same but enqueue into DLQ_FAILED_KEY (e.g., RPUSH or
appropriate list op) instead of ZADD. Use the existing symbols
DLQ_PROCESSING_KEY, DLQ_DELAYED_KEY, DLQ_FAILED_KEY and the method names
scheduleDelayedRetry and markJobFailed so the transition is atomic and returns a
clear success/failure result.
In `@apps/api/src/modules/trigger/trigger-payload-transformer.ts`:
- Around line 39-60: The code around payload/ bounded in
trigger-payload-transformer.ts can throw or return undefined when calling
JSON.stringify(record); wrap the serialization of record (both the sorted object
path and the else path where JSON.stringify(record) is used) in a try/catch,
ensure the resulting payload is always a string (fallback to String(record) or
'{}' when stringify returns undefined), and only then compute bounded from
payload.length using FINGERPRINT_MAX_BYTES; update references to payload,
bounded, FINGERPRINT_MAX_BYTES, VOLATILE_KEYS, record, and sorted accordingly so
no runtime exception occurs during serialization.
In `@apps/web/src/app/providers/data-provider.ts`:
- Around line 35-54: The code currently loops over filters twice (using
filters.find to set searchFilter and filters.forEach to set other exact
matches); consolidate into a single-pass over filters: iterate once through
filters (the CrudFilter items), check for 'field' and if field is 'q' or
'search' set queryFilters.search from the value, otherwise if operator === 'eq'
set queryFilters[f.field] = f.value; keep the existing sorter handling (sorters,
sort, sortOrder) unchanged; update references to searchFilter, filters,
CrudFilter, and queryFilters accordingly.
In `@apps/worker/src/modules/pipeline/delivery-retry.service.ts`:
- Around line 127-145: Persist the destination vendor ID during the initial L6
write (store resp.entityId into the outboundGateway record alongside resp.body)
so it survives partial commits, and update retrySourceFinalization to load that
persisted field instead of reconstructing destVendorId from
outboundGateway.response.entityId/resp.body; specifically, modify the initial
write path that calls deliveryService.writeL6Result (and the code that sets
outboundGateway.response/resp.entityId) to save a destVendorId column, then
change the retry logic that reads existingResult[0].response to prefer
existingResult[0].destVendorId (or reload the persisted field) before calling
deliveryService.writeL6Result in retrySourceFinalization so replicaEntity/GEM
rows can be recreated reliably.
In `@apps/worker/src/modules/pipeline/delivery.service.spec.ts`:
- Around line 90-101: The test suite currently stubs DeliveryRetryService so the
new transactional search_path / replay-query logic isn't covered; restore
focused coverage by adding a dedicated spec for DeliveryRetryService (e.g.,
delivery-retry.service.spec.ts) or updating this file to provide the real
DeliveryRetryService instead of the stub. Specifically, write tests that
exercise DeliveryRetryService methods (handleRetryAndDelay, isSourceFinalized,
retrySourceFinalization) through their transactional flow and replay-query
interactions (or keep one integration-style test here that injects the actual
DeliveryRetryService rather than the mock) so regressions in the retry flow are
detected.
In `@apps/worker/src/modules/pipeline/fanout-batch-processor.ts`:
- Around line 44-472: processSingleStitch leaves lockRefCount.count unchanged
for successful routes because decrements are only in skip/error branches; update
processSingleStitch (and related logic in FanoutRouterService) to decrement
lockRefCount.count exactly once per stitch regardless of outcome by removing the
branch-level decrements scattered throughout the method and adding a single
decrement in a unified exit path (e.g., a finally block at the end of
processSingleStitch that checks if srcVendorId and then decrements
lockRefCount.count); ensure the rest of the logic (outbound insert/publish,
DependenciesMissingError handling, and error handling) no longer performs
individual decrements so activeSyncLocks cleanup in FanoutRouterService (which
relies on lockRefCount.count === 0) works correctly.
In `@engine/sync/application/mapping/package.json`:
- Line 23: The package.json for the mapping package added a dependency entry
"`@soopa/platform`": "workspace:*" but the root pnpm-lock.yaml is missing a
matching importer/dependency entry, causing frozen-lockfile installs to fail;
update the lockfile by regenerating it so the mapping importer includes
`@soopa/platform` (e.g., run pnpm install from the repo root or run pnpm -w
install) so pnpm-lock.yaml contains the mapping importer entry and a resolved
`@soopa/platform` entry that matches "workspace:*".
In `@engine/sync/application/salesforce/revenova/replicate.js`:
- Around line 73-77: The current code silently drops batched Salesforce
notifications by taking notifications['Notification'][0]; change this to handle
arrays explicitly: if notifications['Notification'] is an array either iterate
over each element and run the existing processing path for each notification
(use the same logic that reads notification and sObject) so all sObjects are
processed, or if the system cannot handle batches, fail fast by throwing a clear
error when notifications['Notification'].length > 1. Update the branches around
notifications['Notification'], notification, and sObject in replicate.js to
implement one of these two behaviors and ensure downstream code expects/returns
multiple results if you choose iteration.
In `@packages/credentials/src/oauth/token-manager.service.ts`:
- Around line 297-307: performTokenRefresh currently awaits
eventPublisher.publishCredentialRefreshed after persisting the credential which
allows listener errors to bubble up and fail the refresh; wrap the publish call
(the emit via eventPublisher.publishCredentialRefreshed with new
CredentialRefreshedEvent(...)) in a try/catch and log the error instead of
rethrowing (or fire-and-forget without awaiting) so failures in listeners do not
propagate back through refreshWithLock or getValidCredentials — ensure db.update
(or the persistence step in performTokenRefresh) remains awaited before
emitting.
- Line 86: The constructor parameter on TokenManagerService is incorrectly
annotated with `@Inject`('REDIS_CLIENT') while the factories in pipeline.module
and connections.module actually construct and pass a RedisDistributedLock
instance (which implements IDistributedLock) via useFactory; remove the
`@Inject`('REDIS_CLIENT') decorator from the TokenManagerService constructor
parameter (leaving its type as IDistributedLock) so the signature matches what
the factories supply, or alternatively switch to injecting a proper
IDistributedLock token and update the provider factories to bind
RedisDistributedLock to that token (references: TokenManagerService constructor,
`@Inject`('REDIS_CLIENT'), IDistributedLock, RedisDistributedLock, useFactory
providers in pipeline.module and connections.module).
In `@packages/credentials/src/oauth/token-refresh.service.ts`:
- Around line 62-69: The published event is sending externalId instead of the
actual credential ID; update the logic in token-refresh.service.ts so that
publishCredentialInvalidated is called with the real credential ID (e.g.,
credential.id) instead of externalId when creating the new
CredentialInvalidatedEvent; ensure you resolve or fetch the credential record
(or otherwise obtain credential.id) before the error branch so
CredentialInvalidatedEvent(...) receives the correct credentialId, and apply the
same change in the corresponding similar block (lines ~99-123) where externalId
is currently used.
In `@packages/identity/src/adapters/better-auth.adapter.spec.ts`:
- Around line 117-120: The test introduces mkPublisher but never asserts the new
event behavior; update the successful createInvitation test to verify that the
mocked publisher.publishUserInvited was called (e.g., calledOnce) with the
expected payload/arguments after calling createInvitation. Locate the test that
invokes createInvitation in better-auth.adapter.spec.ts, inject the mkPublisher
mock there, and add an assertion on publishUserInvited (and optionally its call
count and argument shape) to ensure the invitation publication path is covered.
In `@packages/identity/src/adapters/drizzle-tenant.adapter.ts`:
- Around line 58-67: The TenantProvisionedEvent is being published inside the DB
transaction callback (see mapTenant and eventPublisher.publishTenantProvisioned
with TenantProvisionedEvent(orgId, userId, name)); move the publish call out of
the transaction so it executes only after the transaction successfully commits:
have the transaction callback return the created tenant (mapTenant result) and
then, after the transaction completes, call
eventPublisher.publishTenantProvisioned with the TenantProvisionedEvent and
handle errors there (log on failure) so async listeners don't keep the
transaction open or receive events for rolled-back writes.
In `@packages/identity/src/identity.module.ts`:
- Around line 112-113: The module exports the IDENTITY_EVENT_PUBLISHER token but
register() never provides/binds it, causing unresolved dependency errors when
BetterAuthAdapter injects that token during synchronous registration; update the
register() implementation to bind a provider for IDENTITY_EVENT_PUBLISHER (e.g.,
a factory or value provider that returns the same event-publisher instance used
by the async path) and include that provider in the returned providers and
exports so the token is resolvable during sync initialization, ensuring the
provider name matches IDENTITY_EVENT_PUBLISHER and that BetterAuthAdapter can
inject it.
---
Outside diff comments:
In `@apps/api/src/modules/exceptions/exception.service.ts`:
- Around line 322-369: The code currently calls
this.queueDispatcher.dispatchRetry inside the this.db.transaction block (using
outboundGateway, outboundGatewayId, updatedRows), causing an external side
effect during the DB transaction; instead, persist a retry intent (an outbox row
referencing outboundGatewayId, traceId, routeId, payload,
stitch.destDataSourceId, srcDataSourceId and a status like 'PENDING') inside the
transaction (within the same block that updates outboundGateway) and remove the
call to queueDispatcher.dispatchRetry there; after the transaction completes,
asynchronously read the outbox entry and call this.queueDispatcher.dispatchRetry
(catching errors and updating the outbox row status to 'SENT' or 'FAILED') so
dispatch happens only after commit and can be retried safely by an outbox worker
or on failure.
In `@apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts`:
- Around line 352-375: Add a test for the "blocked deletion" path in the
deleteUser suite: mock mockUserProvider.findById to return a user (e.g., {id:
'u1'}) and mock mockUserProvider.deleteIfNotLastAdmin to resolve with { success:
false }, then assert that controller.deleteUser('u1') rejects (e.g., toThrow
ConflictException) and that mockUserProvider.deleteIfNotLastAdmin was called
with 'u1' and getRequiredSystemTenantId(); this covers the "last admin cannot be
deleted" branch for deleteUser.
In `@apps/worker/src/modules/pipeline/delivery.service.ts`:
- Around line 569-592: The public method writeL6Result currently declares
tenantDb as optional (tenantDb?: DrizzleDb) but immediately throws if it's
missing; change the signature to require tenantDb (tenantDb: DrizzleDb) and
remove the runtime guard (the if (!tenantDb) throw ...). Update any callers that
relied on it being optional to pass a valid DrizzleDb instance; keep the
function name writeL6Result and all other parameters unchanged.
In `@apps/worker/src/modules/pipeline/inbound-outbox.poller.ts`:
- Around line 92-119: The claim logic must set and return a unique per-claim
lease token so later completion updates require that token (preventing stale
workers from overwriting newer outcomes): modify the claiming transaction around
tenantDb.transaction / update(inboundOutbox) to SET a new claim_token (e.g. a
generated UUID) alongside status="PROCESSING" and nextRetryAt, and include that
claim_token in the returning payload; then update the completion/failure update
paths (the code that currently checks id and status='PROCESSING' in the
success/failure handlers referenced around the completion updates) to include
AND claim_token = <returned token> in their WHERE clauses; do not remove
attempts handling (keep attempts only updated on real delivery failure) but
require the claim_token match for any terminal updates.
In `@packages/credentials/src/oauth/token-manager.service.ts`:
- Around line 318-324: Wrap the JSON.parse(await
this.crypto.decrypt(connection.value)) call in a try/catch so that any
SyntaxError or other parsing errors are caught and rethrown as an
AppCredentialError; specifically, inside the method where parsed is created
(referencing parsed, this.crypto.decrypt, JSON.parse, isOAuthCredentialBlob),
catch parse errors and throw new AppCredentialError with a clear message (and
include the original error/message for debugging) before proceeding to the
existing isOAuthCredentialBlob check.
In `@packages/credentials/src/oauth/token-refresh.service.ts`:
- Around line 84-86: The code in BaseOAuthRefreshClient.refresh currently maps
every TypeError to OAuthRefreshError(..., 400); instead, only translate
TypeError instances that originate from input validation into a 400
OAuthRefreshError (e.g., those thrown by your input validation routine), and for
other TypeErrors (fetch/transport/configuration) either rethrow the original
error or wrap it with a non-4xx status (e.g., 502) so transport faults are not
misclassified. Concretely, in BaseOAuthRefreshClient.refresh() inspect the
TypeError (by origin: check error.source, error.name, or error.message or
compare to the specific validation function that throws) and only create
OAuthRefreshError(..., 400) for validation-related messages; otherwise rethrow
the TypeError or throw an OAuthRefreshError with a 5xx code so retry/diagnostics
remain correct.
In `@packages/identity/src/adapters/drizzle-tenant.adapter.spec.ts`:
- Around line 127-160: The test for DrizzleTenantAdapter.create currently
doesn't assert the publisher side effect; update the "create creates
organization and admin member, handles slug collision retries" spec to capture
the mock IIdentityEventPublisher returned by mkPublisher(), call the
adapter.create("user-1","Acme") as before, then assert that
publishTenantProvisioned (the publisher mock method) was called exactly once
with the provisioned tenant (or at least with expected properties like tenant.id
or slug/name) to prevent regressions; reference the DrizzleTenantAdapter
instance, the create(...) call, and the publisher mock (from mkPublisher) when
adding the assertion.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc1f0424-0d46-4aec-aa7f-0f8873f0794f
📒 Files selected for processing (144)
apps/api/package.jsonapps/api/src/app/app.module.tsapps/api/src/db/cli/cli.module.tsapps/api/src/db/cli/commands/db-debug.command.tsapps/api/src/db/cli/commands/db-fresh.command.tsapps/api/src/db/cli/commands/db-migrate.command.tsapps/api/src/db/cli/commands/db-provision.command.tsapps/api/src/db/cli/commands/db-reset.command.tsapps/api/src/db/cli/commands/db-seed.command.tsapps/api/src/db/cli/main.tsapps/api/src/db/data/seed-abac.tsapps/api/src/db/data/seed-mappings.tsapps/api/src/db/database-manager.spec.tsapps/api/src/db/database-manager.tsapps/api/src/db/infrastructure/migration-runner.service.tsapps/api/src/db/infrastructure/pg-connection.pool.tsapps/api/src/db/services/connection-schema-provisioner.service.tsapps/api/src/db/services/database-reset.service.tsapps/api/src/db/services/dev-sandbox-provisioner.service.tsapps/api/src/db/services/environment-guard.service.tsapps/api/src/db/services/rbac-inspector.service.tsapps/api/src/db/services/sync-seeding.service.tsapps/api/src/db/services/system-seeder.service.tsapps/api/src/db/services/tenant-schema.service.tsapps/api/src/modules/connections/connection-lifecycle.service.spec.tsapps/api/src/modules/connections/connection-lifecycle.service.tsapps/api/src/modules/connections/connections.module.tsapps/api/src/modules/connections/connections/connectors.controller.spec.tsapps/api/src/modules/connections/connections/credential.controller.tsapps/api/src/modules/connections/connections/oauth.controller.tsapps/api/src/modules/connections/connectors.service.spec.tsapps/api/src/modules/connections/connectors.service.tsapps/api/src/modules/connections/events/connection-paused.event.tsapps/api/src/modules/connections/events/connection-schema-provisioned.event.tsapps/api/src/modules/connections/oauth-url-builder.spec.tsapps/api/src/modules/connections/oauth-url-builder.tsapps/api/src/modules/connections/services/credential-linking.service.tsapps/api/src/modules/connections/services/oauth-orchestration.service.tsapps/api/src/modules/exceptions/exception.service.spec.tsapps/api/src/modules/exceptions/exception.service.tsapps/api/src/modules/exceptions/exceptions.module.tsapps/api/src/modules/exceptions/infrastructure/soopa-delivery-queue-dispatcher.service.tsapps/api/src/modules/exceptions/interfaces/delivery-queue-dispatcher.interface.tsapps/api/src/modules/identity/auth/auth.controller.tsapps/api/src/modules/identity/system-admin/system-admin.controller.spec.tsapps/api/src/modules/identity/system-admin/system-admin.controller.tsapps/api/src/modules/pipeline/pipeline-cdc.listener.tsapps/api/src/modules/pipeline/pipeline.module.tsapps/api/src/modules/scheduler/infrastructure/fetch-http.client.tsapps/api/src/modules/scheduler/infrastructure/stub-scheduler.client.spec.tsapps/api/src/modules/scheduler/infrastructure/stub-scheduler.client.tsapps/api/src/modules/scheduler/infrastructure/windmill-scheduler.client.spec.tsapps/api/src/modules/scheduler/infrastructure/windmill-scheduler.client.tsapps/api/src/modules/scheduler/interfaces/http-client.interface.tsapps/api/src/modules/scheduler/interfaces/scheduler-client.interface.tsapps/api/src/modules/scheduler/scheduler.module.tsapps/api/src/modules/scheduler/scheduler.service.spec.tsapps/api/src/modules/scheduler/scheduler.service.tsapps/api/src/modules/scheduler/windmill.client.tsapps/api/src/modules/trigger/dlq-processor.service.spec.tsapps/api/src/modules/trigger/dlq-processor.service.tsapps/api/src/modules/trigger/infrastructure/redis-distributed-lock.service.tsapps/api/src/modules/trigger/infrastructure/redis-trigger-dlq.service.tsapps/api/src/modules/trigger/interfaces/distributed-lock.interface.tsapps/api/src/modules/trigger/interfaces/trigger-dlq.interface.tsapps/api/src/modules/trigger/key-value-trigger-store.spec.tsapps/api/src/modules/trigger/key-value-trigger-store.tsapps/api/src/modules/trigger/trigger-executor.service.spec.tsapps/api/src/modules/trigger/trigger-executor.service.tsapps/api/src/modules/trigger/trigger-payload-transformer.spec.tsapps/api/src/modules/trigger/trigger-payload-transformer.tsapps/api/src/modules/trigger/trigger-retry-policy.service.spec.tsapps/api/src/modules/trigger/trigger-retry-policy.service.tsapps/api/src/modules/trigger/trigger.module.tsapps/api/src/scripts/admin-bootstrap.tsapps/api/src/scripts/test-e2e-ingestion.tsapps/api/vitest.config.mtsapps/tenant-provisioner/vitest.config.tsapps/web/src/app/providers/data-provider.tsapps/web/src/test/environments/jsdom-msw.tsapps/web/vitest.config.tsapps/worker/src/db/database-manager.spec.tsapps/worker/src/db/database-manager.tsapps/worker/src/modules/pipeline/delivery-retry.service.tsapps/worker/src/modules/pipeline/delivery.service.spec.tsapps/worker/src/modules/pipeline/delivery.service.tsapps/worker/src/modules/pipeline/fanout-batch-processor.tsapps/worker/src/modules/pipeline/fanout-router.service.tsapps/worker/src/modules/pipeline/gem-hydration.service.tsapps/worker/src/modules/pipeline/inbound-outbox.poller.tsapps/worker/src/modules/pipeline/pipeline.module.tsapps/worker/src/modules/pipeline/registry-replication.service.tsapps/worker/vitest.config.mtsengine/ai/core/vitest.config.tsengine/sync/application/mapping/package.jsonengine/sync/application/mapping/src/mapping-engine.tsengine/sync/application/salesforce/revenova/replicate.jsengine/sync/application/salesforce/revenova/replicate.tsengine/sync/platform/core/src/sharding/pipeline-hook-broker.service.spec.tsengine/sync/platform/core/vitest.config.tspackages/auth/src/services/auth.service.tspackages/cache/src/index.tspackages/cache/src/key-value-store.interface.tspackages/credentials/package.jsonpackages/credentials/src/events/credential-deleted.event.tspackages/credentials/src/events/credential-invalidated.event.tspackages/credentials/src/events/credential-refreshed.event.tspackages/credentials/src/events/index.tspackages/credentials/src/index.tspackages/credentials/src/interfaces/event-publisher.interface.tspackages/credentials/src/interfaces/index.tspackages/credentials/src/oauth/redis-lock.tspackages/credentials/src/oauth/token-manager.service.spec.tspackages/credentials/src/oauth/token-manager.service.tspackages/credentials/src/oauth/token-refresh.service.spec.tspackages/credentials/src/oauth/token-refresh.service.tspackages/credentials/src/services/credentials-event-publisher.service.tspackages/credentials/vitest.config.tspackages/database/src/database.module.tspackages/database/src/index.tspackages/database/src/savepoint/index.tspackages/database/src/savepoint/savepoint.manager.tspackages/database/src/schema/global/credentials.tspackages/database/src/schema/global/data-sources.tspackages/dbmanager/src/cli/interfaces.tspackages/dbmanager/src/impl/tenant-database-manager.tspackages/dbmanager/src/index.tspackages/identity/package.jsonpackages/identity/src/adapters/better-auth.abac.spec.tspackages/identity/src/adapters/better-auth.adapter.spec.tspackages/identity/src/adapters/better-auth.adapter.tspackages/identity/src/adapters/drizzle-tenant.adapter.spec.tspackages/identity/src/adapters/drizzle-tenant.adapter.tspackages/identity/src/constants.tspackages/identity/src/events/index.tspackages/identity/src/events/tenant-provisioned.event.tspackages/identity/src/events/user-invited.event.tspackages/identity/src/identity.module.tspackages/identity/src/interfaces/auth-provider.interface.tspackages/identity/src/interfaces/event-publisher.interface.tspackages/identity/src/interfaces/index.tspackages/identity/src/services/identity-event-publisher.service.spec.tspackages/identity/src/services/identity-event-publisher.service.tspackages/identity/src/services/permission-seeder.spec.ts
💤 Files with no reviewable changes (3)
- apps/worker/src/db/database-manager.ts
- apps/api/src/modules/scheduler/windmill.client.ts
- apps/worker/src/db/database-manager.spec.ts
Comment on lines
+13
to
+42
| @Option({ | ||
| flags: '-u, --user [identifier]', | ||
| description: 'Check permissions for a specific user (by ID or Email)', | ||
| }) | ||
| parseUser(val: string): string { | ||
| return val; | ||
| } | ||
|
|
||
| @Option({ | ||
| flags: '-r, --role [roleName]', | ||
| description: 'Debug RBAC permissions for a role', | ||
| }) | ||
| parseRole(val: string): string { | ||
| return val; | ||
| } | ||
|
|
||
| async run( | ||
| _passedParam: string[], | ||
| options?: { user?: string; role?: string }, | ||
| ): Promise<void> { | ||
| try { | ||
| if (options?.user) { | ||
| await this.rbacInspectorService.checkUserPermissions(options.user); | ||
| } else if (options?.role) { | ||
| await this.rbacInspectorService.debugPermissions(options.role); | ||
| } else { | ||
| console.error( | ||
| 'Usage: db:debug [--user <userId|email>] [--role <roleName>]', | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In commander.js (used by nest-commander), what value is assigned when an option declared as --user [identifier] is provided without a value?
💡 Result:
In commander.js, when an option is declared with an optional argument using square brackets (e.g., --user [identifier]), and the flag is provided without a value, the assigned value is true [1][2][3]. This behavior allows the option to act as a boolean flag when no value is provided, while still accepting a custom value when one is specified [1][3]. If you wish to set a different default value when the option is not provided at all, you can define one using the third parameter of the.option method [4]. Since nest-commander is a wrapper around commander.js, it follows this same behavior for option parsing [5][6]. In nest-commander, you can also explicitly define a defaultValue in the @Option decorator metadata to handle cases where the option is omitted [6][7].
Citations:
- 1: https://github.com/tj/commander.js/blob/master/Readme.md
- 2: https://www.npmjs.com/package/commander
- 3: https://github.com/tj/commander.js/
- 4: https://app.studyraid.com/en/read/11908/379346/setting-default-values-for-options
- 5: https://nest-commander.jaymcdoniel.dev/en/features/commander/
- 6: https://nest-commander.jaymcdoniel.dev/en/api/
- 7: https://www.npmjs.com/nest-commander
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the reviewed command file around the cited lines
FILE="apps/api/src/db/cli/commands/db-debug.command.ts"
if [ -f "$FILE" ]; then
echo "=== $FILE (lines 1-120) ==="
nl -ba "$FILE" | sed -n '1,140p'
else
echo "Missing file: $FILE" >&2
exit 1
fi
# Locate usages/definitions of the RBAC inspector methods called here
rg -n "rbacInspectorService\.(checkUserPermissions|debugPermissions)" apps/api/src -S || true
rg -n "checkUserPermissions\(" apps/api/src -S || true
rg -n "debugPermissions\(" apps/api/src -S || trueRepository: pramodnarayana/nexiom
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="apps/api/src/db/cli/commands/db-debug.command.ts"
if [ -f "$FILE" ]; then
echo "=== $FILE (lines 1-220) ==="
# Use sed for line-numbered output (nl may not exist)
awk 'NR>=1 && NR<=220 {printf "%d\t%s\n", NR, $0}' "$FILE"
else
echo "Missing file: $FILE" >&2
exit 1
fi
echo
echo "=== rbaci n s p e c t o r service method calls ==="
rg -n "rbacInspectorService\.(checkUserPermissions|debugPermissions)" apps/api/src -S || true
echo
echo "=== method definitions (checkUserPermissions / debugPermissions) ==="
rg -n "checkUserPermissions\s*\(" apps/api/src -S || true
rg -n "debugPermissions\s*\(" apps/api/src -S || trueRepository: pramodnarayana/nexiom
Length of output: 3624
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print relevant sections of the service implementation
FILE1="apps/api/src/db/services/rbac-inspector.service.ts"
FILE2="apps/api/src/db/database-manager.ts"
for f in "$FILE1" "$FILE2"; do
echo "=== $f (top/bordered sections around RBAC methods) ==="
if [ -f "$f" ]; then
# Show first 140 lines for context, then the method bodies by searching markers
head -n 140 "$f" | awk '{print NR"\t"$0}' || true
echo "--- grep markers ---"
rg -n "checkUserPermissions|debugPermissions" "$f" -n || true
# Print from the checkUserPermissions definition through the next ~120 lines
if rg -n "async checkUserPermissions" "$f" >/dev/null; then
start=$(rg -n "async checkUserPermissions" "$f" | head -n1 | cut -d: -f1)
end=$((start+140))
sed -n "${start},${end}p" "$f" | awk '{print (NR+0)"\t"$0}' || true
fi
if rg -n "async debugPermissions" "$f" >/dev/null; then
start=$(rg -n "async debugPermissions" "$f" | head -n1 | cut -d: -f1)
end=$((start+140))
sed -n "${start},${end}p" "$f" | awk '{print (NR+0)"\t"$0}' || true
fi
else
echo "Missing file: $f" >&2
fi
echo
doneRepository: pramodnarayana/nexiom
Length of output: 24359
Make db:debug require option values and enforce mutual exclusivity.
Commander parses --user [identifier] / --role [roleName] as a boolean flag when the value is omitted (the option value becomes true), so checkUserPermissions(identifier: string) / debugPermissions(roleName: string) can receive a non-string; additionally, when both flags are provided, run() silently prefers --user. When the user/role isn’t found, the service only console.error + return, so misuse can still finish with a zero exit code.
Suggested fix
`@Option`({
- flags: '-u, --user [identifier]',
+ flags: '-u, --user <identifier>',
description: 'Check permissions for a specific user (by ID or Email)',
})
parseUser(val: string): string {
return val;
}
`@Option`({
- flags: '-r, --role [roleName]',
+ flags: '-r, --role <roleName>',
description: 'Debug RBAC permissions for a role',
})
parseRole(val: string): string {
return val;
}
async run(
_passedParam: string[],
options?: { user?: string; role?: string },
): Promise<void> {
try {
- if (options?.user) {
+ const hasUser = Boolean(options?.user);
+ const hasRole = Boolean(options?.role);
+
+ if (hasUser === hasRole) {
+ console.error(
+ 'Usage: db:debug --user <userId|email> | --role <roleName>',
+ );
+ process.exit(1);
+ }
+
+ if (hasUser) {
await this.rbacInspectorService.checkUserPermissions(options.user);
- } else if (options?.role) {
+ } else {
await this.rbacInspectorService.debugPermissions(options.role);
- } else {
- console.error(
- 'Usage: db:debug [--user <userId|email>] [--role <roleName>]',
- );
}📝 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
| @Option({ | |
| flags: '-u, --user [identifier]', | |
| description: 'Check permissions for a specific user (by ID or Email)', | |
| }) | |
| parseUser(val: string): string { | |
| return val; | |
| } | |
| @Option({ | |
| flags: '-r, --role [roleName]', | |
| description: 'Debug RBAC permissions for a role', | |
| }) | |
| parseRole(val: string): string { | |
| return val; | |
| } | |
| async run( | |
| _passedParam: string[], | |
| options?: { user?: string; role?: string }, | |
| ): Promise<void> { | |
| try { | |
| if (options?.user) { | |
| await this.rbacInspectorService.checkUserPermissions(options.user); | |
| } else if (options?.role) { | |
| await this.rbacInspectorService.debugPermissions(options.role); | |
| } else { | |
| console.error( | |
| 'Usage: db:debug [--user <userId|email>] [--role <roleName>]', | |
| ); | |
| } | |
| `@Option`({ | |
| flags: '-u, --user <identifier>', | |
| description: 'Check permissions for a specific user (by ID or Email)', | |
| }) | |
| parseUser(val: string): string { | |
| return val; | |
| } | |
| `@Option`({ | |
| flags: '-r, --role <roleName>', | |
| description: 'Debug RBAC permissions for a role', | |
| }) | |
| parseRole(val: string): string { | |
| return val; | |
| } | |
| async run( | |
| _passedParam: string[], | |
| options?: { user?: string; role?: string }, | |
| ): Promise<void> { | |
| try { | |
| const hasUser = Boolean(options?.user); | |
| const hasRole = Boolean(options?.role); | |
| if (hasUser === hasRole) { | |
| console.error( | |
| 'Usage: db:debug --user <userId|email> | --role <roleName>', | |
| ); | |
| process.exit(1); | |
| } | |
| if (hasUser) { | |
| await this.rbacInspectorService.checkUserPermissions(options.user); | |
| } else { | |
| await this.rbacInspectorService.debugPermissions(options.role); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/db/cli/commands/db-debug.command.ts` around lines 13 - 42,
Change the Option flags to require values (so parseUser and parseRole always
receive strings), then in run() enforce mutual exclusivity (if both options.user
and options.role are provided, print usage/error and process.exit(1)). Validate
that options.user/options.role are strings before calling
rbacInspectorService.checkUserPermissions or
rbacInspectorService.debugPermissions and treat non-string/true values as
missing (error + process.exit(1)). Also handle service failures or "not found"
results by logging the error via console.error and calling process.exit(1)
instead of returning so the command exits with a non-zero code. Use the symbols
parseUser, parseRole, run, rbacInspectorService.checkUserPermissions, and
rbacInspectorService.debugPermissions to locate the changes.
Comment on lines
+61
to
+70
| if (options?.local) { | ||
| await this.sandboxProvisioner.provisionLocal(); | ||
| } else if (options?.gateway && options?.schema) { | ||
| await this.schemaProvisioner.provisionGateway(options.schema); | ||
| } else if (options?.outbound && options?.schema) { | ||
| await this.schemaProvisioner.provisionOutbound(options.schema); | ||
| } else { | ||
| console.error( | ||
| 'Usage: db:provision --type <local|gateway|outbound> [--schema <schema>]', | ||
| ); |
Contributor
There was a problem hiding this comment.
Reject ambiguous mode combinations and fix the usage contract.
The command currently accepts conflicting flags (for example, --gateway --outbound) and silently executes only the first matched branch. It also prints a --type usage that does not exist. This can trigger the wrong provisioning path.
Suggested fix
async run(
@@
): Promise<void> {
try {
- if (options?.local) {
+ const modeCount =
+ Number(!!options?.local) +
+ Number(!!options?.gateway) +
+ Number(!!options?.outbound);
+
+ if (modeCount !== 1) {
+ console.error(
+ 'Usage: db:provision (--local | --gateway --schema <schema> | --outbound --schema <schema>)',
+ );
+ process.exit(1);
+ }
+
+ if (options?.local) {
await this.sandboxProvisioner.provisionLocal();
} else if (options?.gateway && options?.schema) {
await this.schemaProvisioner.provisionGateway(options.schema);
} else if (options?.outbound && options?.schema) {
await this.schemaProvisioner.provisionOutbound(options.schema);
} else {
console.error(
- 'Usage: db:provision --type <local|gateway|outbound> [--schema <schema>]',
+ 'Usage: db:provision (--local | --gateway --schema <schema> | --outbound --schema <schema>)',
);
+ process.exit(1);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/db/cli/commands/db-provision.command.ts` around lines 61 - 70,
Validate the provided flags before executing any provisioner: ensure exactly one
of options.local, options.gateway, or options.outbound is set (reject
combinations like both gateway and outbound), require options.schema when
calling schemaProvisioner.provisionGateway or
schemaProvisioner.provisionOutbound, and on invalid input print a corrected
usage message (e.g., "Usage: db:provision --local | --gateway --schema <schema>
| --outbound --schema <schema>") and exit with a non-zero status; update the
branch in db-provision.command.ts to perform this validation prior to calling
sandboxProvisioner.provisionLocal, schemaProvisioner.provisionGateway, or
schemaProvisioner.provisionOutbound.
Comment on lines
+41
to
+50
| const pool = new Pool({ | ||
| connectionString: fullDsn, | ||
| max: 20, | ||
| idleTimeoutMillis: 30_000, | ||
| connectionTimeoutMillis: 5_000, | ||
| }); | ||
| return drizzle(pool, { | ||
| schema: dbSchema, | ||
| }) as unknown as import('@soopa/database').DrizzleDb; | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate TenantDatabaseManager implementation and inspect pool lifecycle:"
fd -i 'tenant-database-manager.ts'
rg -n -C3 'class TenantDatabaseManager|new Pool\(|pool\.end\(|close\(|dispose\(' packages/dbmanager/src/impl/tenant-database-manager.ts
echo
echo "Check any interface contracts for teardown hooks:"
rg -n -C3 'interface.*DatabaseManager|dispose|close|shutdown' packages/dbmanager/src packages/dbmanagerRepository: pramodnarayana/nexiom
Length of output: 9628
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find call sites for TenantDatabaseManager teardown:"
rg -n --hidden --glob '!**/node_modules/**' "closeAll\(\)" packages apps .
rg -n --hidden --glob '!**/node_modules/**' "closeTenantDb\(" packages apps .Repository: pramodnarayana/nexiom
Length of output: 807
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find references to TenantDatabaseManager and see how it is constructed / cleaned up:"
rg -n --hidden --glob '!**/node_modules/**' "new TenantDatabaseManager|TenantDatabaseManager\(" packages apps .
rg -n --hidden --glob '!**/node_modules/**' "dbmanager|DatabaseManager" apps/api/src packages/dbmanager/src | head -n 200Repository: pramodnarayana/nexiom
Length of output: 14728
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- apps/api/src/modules/dbmanager/dbmanager.module.ts ----"
wc -l apps/api/src/modules/dbmanager/dbmanager.module.ts
sed -n '1,220p' apps/api/src/modules/dbmanager/dbmanager.module.ts | cat -n
echo
echo "---- apps/worker/src/modules/dbmanager/dbmanager.module.ts ----"
wc -l apps/worker/src/modules/dbmanager/dbmanager.module.ts
sed -n '1,220p' apps/worker/src/modules/dbmanager/dbmanager.module.ts | cat -n
echo
echo "---- apps/api/src/modules/identity/tenant-offboarding.service.ts (if any teardown) ----"
ls apps/api/src/modules/identity/tenant-offboarding.service.ts
wc -l apps/api/src/modules/identity/tenant-offboarding.service.ts
rg -n "closeAll|closeTenantDb|end\(" apps/api/src/modules/identity/tenant-offboarding.service.ts || true
sed -n '1,260p' apps/api/src/modules/identity/tenant-offboarding.service.ts | cat -n
echo
echo "---- packages/dbmanager/src/interfaces.ts (contract) ----"
wc -l packages/dbmanager/src/interfaces.ts
sed -n '1,120p' packages/dbmanager/src/interfaces.ts | cat -n
echo
echo "---- packages/dbmanager/src/impl/tenant-database-manager.ts (closeTenantDb/closeAll impl) ----"
wc -l packages/dbmanager/src/impl/tenant-database-manager.ts
sed -n '200,320p' packages/dbmanager/src/impl/tenant-database-manager.ts | cat -nRepository: pramodnarayana/nexiom
Length of output: 20722
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# show class declaration area + any lifecycle hooks by scanning for 'onModuleDestroy' / 'shutdown' / 'beforeApplicationShutdown'
rg -n "onModuleDestroy|beforeApplicationShutdown|beforeExit|shutdown|Destroy" packages/dbmanager/src/impl/tenant-database-manager.ts
rg -n "class TenantDatabaseManager" -n packages/dbmanager/src/impl/tenant-database-manager.ts
sed -n '1,120p' packages/dbmanager/src/impl/tenant-database-manager.ts | cat -n
sed -n '120,220p' packages/dbmanager/src/impl/tenant-database-manager.ts | cat -n
sed -n '220,300p' packages/dbmanager/src/impl/tenant-database-manager.ts | cat -nRepository: pramodnarayana/nexiom
Length of output: 15646
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for shutdown hook wiring (Nest):"
rg -n "enableShutdownHooks|beforeApplicationShutdown|onModuleDestroy" apps/api apps/worker || true
echo
echo "Search for any usage of closeAll / closeTenantDb (string-based):"
rg -n "closeAll" apps packages || true
rg -n "closeTenantDb" apps packages || true
echo
echo "Check dbCache eviction points in TenantDatabaseManager:"
rg -n "dbCache\.(delete|clear)|inProgress\.(clear|delete)" packages/dbmanager/src/impl/tenant-database-manager.tsRepository: pramodnarayana/nexiom
Length of output: 3323
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- apps/api/src/core/shutdown.service.ts ----"
wc -l apps/api/src/core/shutdown.service.ts
sed -n '1,220p' apps/api/src/core/shutdown.service.ts | cat -n
echo
rg -n "closeAll|closeTenantDb|DB_MANAGER|TenantDatabaseManager" apps/api/src/core/shutdown.service.ts || true
echo
echo "---- apps/api/src/main.ts (shutdown hook wiring) ----"
wc -l apps/api/src/main.ts
sed -n '70,140p' apps/api/src/main.ts | cat -n
echo
rg -n "ShutdownService|enableShutdownHooks" apps/api/src/main.ts
echo
echo "---- apps/api/src/db/services/connection-schema-provisioner.service.ts (TenantDatabaseManager instantiation + getTenantDb usage) ----"
wc -l apps/api/src/db/services/connection-schema-provisioner.service.ts
sed -n '1,140p' apps/api/src/db/services/connection-schema-provisioner.service.ts | cat -n
echo
rg -n "new TenantDatabaseManager|getTenantDb|closeAll|closeTenantDb" apps/api/src/db/services/connection-schema-provisioner.service.ts
echo
echo "---- apps/api/src/db/database-manager.ts (other TenantDatabaseManager instantiations) ----"
# show small windows around the line numbers reported earlier
for n in 900 1120; do
echo "--- around $n ---"
sed -n "${n},$((n+80))p" apps/api/src/db/database-manager.ts | cat -n
done
rg -n "new TenantDatabaseManager" apps/api/src/db/database-manager.tsRepository: pramodnarayana/nexiom
Length of output: 19820
End the per-tenant pg.Pool created in TenantDatabaseManager’s dbFactory to prevent connection/pool resource leaks.
In apps/api/src/db/services/connection-schema-provisioner.service.ts, the dbFactory creates const pool = new Pool(...) and returns drizzle(pool, ...), but nothing ever calls pool.end(). The finally in withSchemaMgr only does await client.end() (global client), and TenantDatabaseManager.closeTenantDb()/closeAll() (which call end()) are never invoked here, so each provisioning run can leave tenant pools alive until shutdown.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/db/services/connection-schema-provisioner.service.ts` around
lines 41 - 50, The created per-tenant pg.Pool in TenantDatabaseManager.dbFactory
is never closed and leaks connections; fix by ensuring the pool is closed after
provisioning: either register the created pool with TenantDatabaseManager so
existing TenantDatabaseManager.closeTenantDb()/closeAll() will call pool.end(),
or explicitly await pool.end() in the finally block that wraps withSchemaMgr
(the same finally that currently only calls client.end()); update dbFactory
(where const pool = new Pool(...) and drizzle(pool,...)) to call the
registration helper or return an object that allows the caller to close the pool
so the caller can invoke await pool.end() in its finally. Ensure you
reference/modify TenantDatabaseManager.dbFactory, the pool variable,
drizzle(...), withSchemaMgr's finally, and
TenantDatabaseManager.closeTenantDb()/closeAll() accordingly.
Comment on lines
+297
to
+307
| // 8. Emit the domain event | ||
| if (this.eventPublisher) { | ||
| await this.eventPublisher.publishCredentialRefreshed( | ||
| new CredentialRefreshedEvent( | ||
| connection.credentialId as string, | ||
| connection.tenantId as string, | ||
| connection.id as string, | ||
| expiresAt, | ||
| ) | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant function/section around the cited lines
FILE="packages/credentials/src/oauth/token-manager.service.ts"
python3 - <<'PY'
import pathlib
path = pathlib.Path("packages/credentials/src/oauth/token-manager.service.ts")
print("EXISTS:", path.exists(), "SIZE_LINES:", sum(1 for _ in path.open()))
PY
# Show the exact region around lines 270-340 (where the snippet claims 297-307)
nl -ba "$FILE" | sed -n '260,340p'
# Find where publishCredentialRefreshed is declared/called and what it does
rg -n "publishCredentialRefreshed|CredentialRefreshedEvent|emitAsync|eventPublisher" packages/credentials/src/oauth/token-manager.service.ts
rg -n "publishCredentialRefreshed|CredentialRefreshedEvent" packages/credentials/src | head -n 200
# Try to locate the event publisher implementation/interface
rg -n "class .*EventPublisher|interface .*EventPublisher|eventPublisher" packages/credentials/src | head -n 200Repository: pramodnarayana/nexiom
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Additional: find getValidCredentials() and see how refresh errors are handled
FILE="packages/credentials/src/oauth/token-manager.service.ts"
rg -n "getValidCredentials\\(" "$FILE"
# Print a wider window around it
nl -ba "$FILE" | sed -n '1,220p'
nl -ba "$FILE" | sed -n '220,520p'
# Search for other refresh-related try/catch patterns
rg -n "refresh|token refresh|Credential refresh|try \\{|catch \\(" "$FILE"Repository: pramodnarayana/nexiom
Length of output: 295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for usage of event listener publish in other files to see established error-handling pattern
rg -n "publishCredentialRefreshed\\(" packages/credentials/src
rg -n "try \\{\\s*await .*publish.*\\}|catch \\(.*\\).*publish" packages/credentials/src | head -n 200Repository: pramodnarayana/nexiom
Length of output: 504
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/credentials/src/oauth/token-manager.service.ts"
python3 - <<'PY'
import itertools, pathlib
path = pathlib.Path("packages/credentials/src/oauth/token-manager.service.ts")
start, end = 260, 330
lines = path.read_text().splitlines()
for i in range(start-1, min(end, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
PY
echo "---- publisher service ----"
python3 - <<'PY'
import pathlib
path = pathlib.Path("packages/credentials/src/services/credentials-event-publisher.service.ts")
lines = path.read_text().splitlines()
for i in range(1, min(len(lines), 220)):
print(f"{i:4d}: {lines[i-1]}")
PY
echo "---- interface ----"
python3 - <<'PY'
import pathlib
path = pathlib.Path("packages/credentials/src/interfaces/event-publisher.interface.ts")
lines = path.read_text().splitlines()
for i in range(1, min(len(lines), 120)):
print(f"{i:4d}: {lines[i-1]}")
PY
echo "---- getValidCredentials body (approx) ----"
python3 - <<'PY'
import pathlib
path = pathlib.Path("packages/credentials/src/oauth/token-manager.service.ts")
lines = path.read_text().splitlines()
# find definition line
def_line=None
for idx,l in enumerate(lines, start=1):
if "async getValidCredentials" in l:
def_line=idx
break
print("getValidCredentials starts at", def_line)
# print a window around it
if def_line:
start=def_line-20
end=def_line+160
for i in range(max(1,start), min(end, len(lines))+1):
print(f"{i:4d}: {lines[i-1]}")
PY
echo "---- search for try/catch around refresh ----"
rg -n "refresh|token refresh|Credential refresh" packages/credentials/src/oauth/token-manager.service.ts
rg -n "try \\{|catch \\(" packages/credentials/src/oauth/token-manager.service.tsRepository: pramodnarayana/nexiom
Length of output: 17969
Isolate credential refresh event publication so listener errors don’t fail token refresh
performTokenRefresh() persists the refreshed credential (db.update(...)) and then awaits this.eventPublisher.publishCredentialRefreshed(...) (via emitAsync). If a listener/handler causes emitAsync to throw/reject, that error propagates back through refreshWithLock() and getValidCredentials(), turning a successful refresh into an avoidable request failure.
Suggested fix
- if (this.eventPublisher) {
- await this.eventPublisher.publishCredentialRefreshed(
- new CredentialRefreshedEvent(
- connection.credentialId as string,
- connection.tenantId as string,
- connection.id as string,
- expiresAt,
- )
- );
- }
+ if (this.eventPublisher) {
+ try {
+ await this.eventPublisher.publishCredentialRefreshed(
+ new CredentialRefreshedEvent(
+ connection.credentialId as string,
+ connection.tenantId as string,
+ connection.id as string,
+ expiresAt,
+ )
+ );
+ } catch (eventError) {
+ this.logger.warn(
+ `Credential refresh event publish failed for ${connection.credentialId as string}: ${
+ eventError instanceof Error ? eventError.message : String(eventError)
+ }`,
+ );
+ }
+ }📝 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
| // 8. Emit the domain event | |
| if (this.eventPublisher) { | |
| await this.eventPublisher.publishCredentialRefreshed( | |
| new CredentialRefreshedEvent( | |
| connection.credentialId as string, | |
| connection.tenantId as string, | |
| connection.id as string, | |
| expiresAt, | |
| ) | |
| ); | |
| } | |
| // 8. Emit the domain event | |
| if (this.eventPublisher) { | |
| try { | |
| await this.eventPublisher.publishCredentialRefreshed( | |
| new CredentialRefreshedEvent( | |
| connection.credentialId as string, | |
| connection.tenantId as string, | |
| connection.id as string, | |
| expiresAt, | |
| ) | |
| ); | |
| } catch (eventError) { | |
| this.logger.warn( | |
| `Credential refresh event publish failed for ${connection.credentialId as string}: ${ | |
| eventError instanceof Error ? eventError.message : String(eventError) | |
| }`, | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/credentials/src/oauth/token-manager.service.ts` around lines 297 -
307, performTokenRefresh currently awaits
eventPublisher.publishCredentialRefreshed after persisting the credential which
allows listener errors to bubble up and fail the refresh; wrap the publish call
(the emit via eventPublisher.publishCredentialRefreshed with new
CredentialRefreshedEvent(...)) in a try/catch and log the error instead of
rethrowing (or fire-and-forget without awaiting) so failures in listeners do not
propagate back through refreshWithLock or getValidCredentials — ensure db.update
(or the persistence step in performTokenRefresh) remains awaited before
emitting.
Comment on lines
+62
to
+69
| if (response.status === 400 || response.status === 401) { | ||
| if (this.eventPublisher) { | ||
| try { | ||
| // Note: we use externalId here as the proxy for credentialId, as the true credential ID is resolved internally. | ||
| // The host app can map this back. Alternatively, we could resolve credential.id from the DB. | ||
| await this.eventPublisher.publishCredentialInvalidated( | ||
| new CredentialInvalidatedEvent(externalId, `HTTP ${response.status}: ${response.statusText}`, appName) | ||
| ); |
Contributor
There was a problem hiding this comment.
Publish the real credential ID in CredentialInvalidatedEvent, not externalId.
The event contract says credentialId, but the current payload sends externalId. This can break downstream invalidation handling and cross-service joins keyed by credential ID.
🔧 Suggested fix
- const [connection] = await this.db.select({ value: credentials.value }).from(dataSources).innerJoin(credentials, eq(credentials.dataSourceId, dataSources.id)).where(
+ const [connection] = await this.db
+ .select({ credentialId: credentials.id, value: credentials.value })
+ .from(dataSources)
+ .innerJoin(credentials, eq(credentials.dataSourceId, dataSources.id))
+ .where(
withTenantGuard(dataSources.tenantId, tenantId, and(eq(dataSources.appName, appName), eq(dataSources.externalId, externalId), eq(credentials.status, AppConnectionStatus.ACTIVE)))
- ).orderBy(desc(dataSources.updatedAt), desc(dataSources.id)).limit(1);
+ )
+ .orderBy(desc(dataSources.updatedAt), desc(dataSources.id))
+ .limit(1);
@@
- return {
+ return {
+ credentialId: connection.credentialId,
clientId: valueBlob.clientId,
clientSecret: valueBlob.clientSecret,
vendorParams: {
...vendorParamsSpread,
...environmentEntry,
},
};- const { clientId, clientSecret, vendorParams } = await this.getCredentials(tenantId, appName, externalId);
+ const { credentialId, clientId, clientSecret, vendorParams } = await this.getCredentials(tenantId, appName, externalId);
@@
- new CredentialInvalidatedEvent(externalId, `HTTP ${response.status}: ${response.statusText}`, appName)
+ new CredentialInvalidatedEvent(credentialId, `HTTP ${response.status}: ${response.statusText}`, appName)Also applies to: 99-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/credentials/src/oauth/token-refresh.service.ts` around lines 62 -
69, The published event is sending externalId instead of the actual credential
ID; update the logic in token-refresh.service.ts so that
publishCredentialInvalidated is called with the real credential ID (e.g.,
credential.id) instead of externalId when creating the new
CredentialInvalidatedEvent; ensure you resolve or fetch the credential record
(or otherwise obtain credential.id) before the error branch so
CredentialInvalidatedEvent(...) receives the correct credentialId, and apply the
same change in the corresponding similar block (lines ~99-123) where externalId
is currently used.
Comment on lines
58
to
67
| const tenant = this.mapTenant(org); | ||
| try { | ||
| await this.eventPublisher.publishTenantProvisioned( | ||
| new TenantProvisionedEvent(orgId, userId, name), | ||
| ); | ||
| } catch (err) { | ||
| console.error("Failed to publish TenantProvisionedEvent", err); | ||
| } | ||
| return tenant; | ||
| }); |
Contributor
There was a problem hiding this comment.
Publish TenantProvisionedEvent only after the DB transaction commits.
Emitting inside the transaction callback risks sending events for uncommitted/rolled-back writes and keeps the transaction open while async listeners run.
Proposed fix
- return await this.db.transaction(async (tx) => {
+ const tenant = await this.db.transaction(async (tx) => {
@@
- const tenant = this.mapTenant(org);
- try {
- await this.eventPublisher.publishTenantProvisioned(
- new TenantProvisionedEvent(orgId, userId, name),
- );
- } catch (err) {
- console.error("Failed to publish TenantProvisionedEvent", err);
- }
- return tenant;
+ return this.mapTenant(org);
});
+
+ try {
+ await this.eventPublisher.publishTenantProvisioned(
+ new TenantProvisionedEvent(orgId, userId, name),
+ );
+ } catch (err) {
+ console.error("Failed to publish TenantProvisionedEvent", err);
+ }
+ return tenant;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/identity/src/adapters/drizzle-tenant.adapter.ts` around lines 58 -
67, The TenantProvisionedEvent is being published inside the DB transaction
callback (see mapTenant and eventPublisher.publishTenantProvisioned with
TenantProvisionedEvent(orgId, userId, name)); move the publish call out of the
transaction so it executes only after the transaction successfully commits: have
the transaction callback return the created tenant (mapTenant result) and then,
after the transaction completes, call eventPublisher.publishTenantProvisioned
with the TenantProvisionedEvent and handle errors there (log on failure) so
async listeners don't keep the transaction open or receive events for
rolled-back writes.
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 29 file(s) based on 36 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 29 file(s) based on 36 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 31
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/api/src/modules/trigger/dlq-processor.service.ts (1)
121-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't silently drop DLQ jobs when registry lookup fails.
getTrigger()returningundefinedcan be transient during deploy drift or registry mismatches.acknowledgeJob(raw)deletes the only queued copy, so those events are unrecoverable once the trigger becomes available again. Move the job to the failed bucket, or leave it in DLQ for operator recovery, instead of discarding it.Suggested fix
if (!trigger) { this.logger.warn('DLQ job references unknown trigger — discarding', { appName: job.appName, triggerName: job.triggerName, }); - await this.dlqService.acknowledgeJob(raw); // discard it + const failedPayload = JSON.stringify({ + ...job, + exhaustedAt: new Date().toISOString(), + error: 'trigger_not_found', + }); + await this.dlqService.markJobFailed(raw, failedPayload); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/trigger/dlq-processor.service.ts` around lines 121 - 127, When getTrigger() returns undefined, do not call this.dlqService.acknowledgeJob(raw) (which permanently deletes the message); instead move the message to the failed bucket or leave it unacknowledged so operators can recover it later — e.g., replace the acknowledgeJob(raw) call with a call to a failure-path method such as this.dlqService.moveToFailed(raw, { reason: 'unknown trigger', appName: job.appName, triggerName: job.triggerName }) or, if no moveToFailed exists, omit acknowledgement so the job remains in the DLQ and add a clear warn log via this.logger.warn(...) with the same metadata.apps/worker/src/modules/pipeline/inbound-outbox.poller.ts (1)
92-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve a claim token when reclaiming timed-out
PROCESSINGrows.Removing the claim-time version bump means a reclaimed row keeps the same
attemptsvalue as the stale owner. After a lease timeout, the old worker can still hit theseUPDATE ... WHERE id AND status = 'PROCESSING'predicates and flip the row toSUCCESS,RETRY, orFAILafter a newer claimant has already taken over.Keep a monotonic claim token/version per claim, or at least include the claimed
nextRetryAtlease value in every compare-and-swap update so only the current owner can complete the row.🔧 Suggested fix
private async processOutboxRow( tenantDb: DrizzleDb, schemaName: string, row: { id: string; traceId: string; dataSourceId: string; attempts: number; + nextRetryAt: Date | null; }, ): Promise<void> { @@ await tenantDb .update(inboundOutbox) .set({ status: "SUCCESS", errorMessage: null }) .where( - sql`${inboundOutbox.id} = ${row.id} AND ${inboundOutbox.status} = 'PROCESSING' AND ${inboundOutbox.attempts} = ${row.attempts}`, + sql`${inboundOutbox.id} = ${row.id} + AND ${inboundOutbox.status} = 'PROCESSING' + AND ${inboundOutbox.nextRetryAt} = ${row.nextRetryAt}`, ); @@ await tenantDb .update(inboundOutbox) .set({ status: "RETRY", nextRetryAt, errorMessage: errorMessage, attempts: incrementedAttempts, }) .where( - sql`${inboundOutbox.id} = ${row.id} AND ${inboundOutbox.status} = 'PROCESSING'`, + sql`${inboundOutbox.id} = ${row.id} + AND ${inboundOutbox.status} = 'PROCESSING' + AND ${inboundOutbox.nextRetryAt} = ${row.nextRetryAt}`, );Apply the same ownership predicate to the
FAILpath as well.Also applies to: 183-225
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/worker/src/modules/pipeline/inbound-outbox.poller.ts` around lines 92 - 119, The claim is not protected against mid-air reclamation: when you transition rows to PROCESSING in the tenantDb.transaction (the UPDATE that sets status="PROCESSING" and nextRetryAt), record a monotonic claim token (or keep the claimed nextRetryAt timestamp) on that row and use that token/timestamp in all subsequent compare-and-swap updates (the code paths that set status => SUCCESS, RETRY, FAIL — e.g. the FAIL path mentioned and the updates in the ~183-225 section, and code around processOutboxRow) so only the current owner can complete the row; i.e., set claimToken (or preserve nextRetryAt) when claiming and add WHERE claim_token = <claimedToken> (or WHERE next_retry_at = <claimedLeaseValue>) to all status-updating queries so reclaimed rows by stale owners cannot overwrite a new claimant.apps/worker/src/modules/pipeline/delivery.service.ts (1)
658-709:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't mark source finalization complete before the GEM write succeeds.
sync_logis inserted andactiveSyncLocksare released beforegemService.writeGemMapping()runs. If the GEM upsert fails,writeL6Result()returnsfalse, but the next delivery will callisSourceFinalized()and see the existing L6sync_logrow, so it skips the replay path and never repairs the missing GEM linkage.Please make the finalized marker match the full source-side commit boundary here—either move the L6 completion marker/lock release after GEM persistence, or change
isSourceFinalized()/ the replay path so GEM completion is part of the finalization check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/worker/src/modules/pipeline/delivery.service.ts` around lines 658 - 709, The L6 sync_log insert and activeSyncLocks deletion are happening before gemService.writeGemMapping(), which can leave an L6 marker without GEM linkage; move the L6 completion and lock release to occur only after writeGemMapping() succeeds (or include the GEM upsert inside the same transactional boundary) so the finalization marker (the tx.insert(syncLog)...onConflictDoNothing and the tx.delete(activeSyncLocks) against activeSyncLocks.lockedByTraceId) is only applied when gemService.writeGemMapping(...) completes successfully; then set sourceCommitted = true after that step. Ensure isSourceFinalized()/replay logic now sees L6 only when GEM persistence has completed.apps/api/src/modules/exceptions/exception.service.ts (1)
322-359:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMove retry dispatch out of the DB transaction.
dispatchRetry()is an external side effect. If it succeeds and the transaction later aborts, the worker can process a retry while the row stays in its old status; if the dispatcher stalls, this transaction stays open longer than necessary. This needs an outbox or a post-commit dispatch path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/exceptions/exception.service.ts` around lines 322 - 359, The dispatch logic (this.queueDispatcher.dispatchRetry) must run after the DB transaction commits: inside the transaction (this.db.transaction) only perform assertValidSchemaName, SET LOCAL search_path, the tx.update(...) call that sets status to 'PENDING' and its .returning(...) and throw the ConflictException if updatedRows.length === 0, then capture the returned row data (traceId, routeId, payload) and any needed ids (outboundGatewayRow.srcDataSourceId, stitch.destDataSourceId) into a local variable and let the transaction complete; once the transaction resolves successfully, call this.queueDispatcher.dispatchRetry using those captured values; also add a clear error-handling path for dispatch failures (retry/outbox or mark status) outside the transaction so the DB update and external dispatch are not executed inside the same transaction.apps/api/src/modules/connections/connections/connectors.controller.spec.ts (1)
110-128: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy liftThis suite is no longer exercising the controllers that production mounts.
ConnectionsModulenow exposesOAuthControllerandCredentialController, but this spec still instantiatesConnectorsControllerdirectly. That leaves the real route wiring/DTO/guard behavior untested after the refactor. Please move the critical cases to the new controller specs or add a thin module-level integration suite around the live controller list.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/connections/connections/connectors.controller.spec.ts` around lines 110 - 128, The test suite is instantiating ConnectorsController directly but ConnectionsModule now exposes OAuthController and CredentialController; update tests to exercise the real controllers and route wiring by either moving the critical specs into the new OAuthController and CredentialController tests or replace the Test.createTestingModule invocation to import the live ConnectionsModule (or explicitly list OAuthController and CredentialController in controllers) so the actual DTOs/guards/pipes are exercised; update references to ConnectorsController, ConnectionsModule, OAuthController, CredentialController and the Test.createTestingModule setup accordingly and remove the direct instantiation of ConnectorsController used in controller = module.get<ConnectorsController>(ConnectorsController).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/app/app.module.ts`:
- Line 17: The code is suppressing type checking by casting a
NodePgDatabase<typeof schema> to DrizzleDb using "as any"; replace the unsafe
cast by making the provider and injection types consistent: import and use the
correct DrizzleDb type where DATABASE_CONNECTION is provided and ensure the
factory that creates NodePgDatabase<typeof schema> is typed to return DrizzleDb
(or update DATABASE_CONNECTION's declared type to NodePgDatabase<typeof schema>
if that is the canonical type), remove "as any" in the provider (refer to the
provider/factory that constructs NodePgDatabase<typeof schema> and the
DATABASE_CONNECTION token) and update any consuming constructor/injection types
to match the chosen type so TypeScript can verify compatibility.
In `@apps/api/src/db/cli/commands/db-provision.command.ts`:
- Around line 42-49: Change the Option decorator on parseSchemaName to require a
value by using '<name>' instead of '[name]' so Commander enforces a schema value
(update flags string '-s, --schema <name>'). Then remove the manual typeof
options.schema !== 'string' validation and related branching in the command
validation logic (the code around options.schema checks and error throw), since
Commander will guarantee a string; keep parseSchemaName as the converter. Also
update any usage/help text that shows '<schema>' to remain consistent with the
new required argument.
In `@apps/api/src/db/services/dev-sandbox-provisioner.service.ts`:
- Around line 9-13: The module-level Map tenantPoolCache is accumulating Pool
instances and causing connection leaks; implement a cleanup that iterates
tenantPoolCache, calls .end() (or the Pool shutdown method) on each cached pool
and clears the Map in the service's onModuleDestroy (or equivalent teardown)
lifecycle method, and also ensure any code paths that replace or overwrite
entries (the resolver code that creates caches around the existing tenant pool
creation logic) close the old pool before replacing its entry so no Pool remains
unclosed; reference tenantPoolCache, onModuleDestroy, and the tenant pool
creation/resolver functions to locate where to add the close-and-clear logic.
- Around line 30-44: The encryptFixture function builds the key buffer without
an explicit encoding which can yield unexpected byte lengths; change
Buffer.from(encryptionKey) to Buffer.from(encryptionKey, 'utf8') (or another
explicit encoding your app expects) in encryptFixture, keep the existing 32-byte
length check, and update the error message guidance if you switch to a different
encoding so the validation matches the expected encoding for the ENCRYPTION_KEY.
- Around line 342-348: Add a concise inline comment above the CASE expression
used in the ON CONFLICT update (the sql`CASE WHEN ${dbSchema.credentials.status}
IN ('ACTIVE', 'REVOKED') THEN ${dbSchema.credentials.status} ELSE 'INACTIVE'
END`) explaining the business rule: we intentionally preserve statuses "ACTIVE"
and "REVOKED" on conflict and normalize any other state to "INACTIVE" to enforce
credential lifecycle semantics (e.g., only explicit activation or revocation
should persist); mention why this choice is made and any related downstream
expectations (credential lifecycle handling) so future maintainers understand
the rationale.
In `@apps/api/src/db/services/rbac-inspector.service.ts`:
- Around line 98-112: When enforcing the Single-Tenant Rule in
rbac-inspector.service.ts (inside the block that handles user.members and uses
normalizeRole), add a warning log when user.members.length > 1 so extra
memberships aren’t silently ignored; include identifying info such as the user
id (or principal), the members count and optionally the skipped organizationIds,
and emit it via the module logger (or console.warn) before selecting
user.members[0] so maintainers can spot multi-membership edge cases.
In `@apps/api/src/db/services/sync-seeding.service.ts`:
- Around line 66-115: The integrationStitches lookup and insert currently ignore
mapping.sourceDataSourceId, so different source connections can collide; update
the query in the stitches selection (the where clause on
schema.integrationStitches used when populating stitches) to include
eq(schema.integrationStitches.sourceDataSourceId, mapping.sourceDataSourceId)
and also add sourceDataSourceId: mapping.sourceDataSourceId to the values passed
to db.insert(schema.integrationStitches) (the block that creates newStitch),
ensuring stitch identity includes the sourceDataSourceId and prevents
overwriting fieldMappings across different sources.
In `@apps/api/src/db/services/system-seeder.service.ts`:
- Around line 225-258: When a pre-existing user is found (variable user) the
seeder currently only ensures System Owner membership and never creates/updates
the login credential; modify the branch that handles existing users to upsert
the bootstrap credential: compute hashedPassword (reuse bcrypt.hash(password,
10)), then insert or update schema.credential for that user (use uuidv4() for id
on insert) so the credential passwordHash and primary flag (and updatedAt) are
set to the bootstrap values if a credential already exists; use
db.insert(schema.credential).values(...).onConflictDoUpdate(...) or an
equivalent update path via db.update(schema.credential).where(...) to ensure
rerunning the seeder with BOOTSTRAP_ADMIN_PASSWORD reliably restores the admin
sign-in.
- Around line 407-445: The current unfiltered query using
db.select().from(schema.uiWorkspaces).limit(1) can pick any workspace; change
the workspace lookup to a deterministic query that targets the seeded fixture
used by seedMapping() (e.g., filter by a known fixture field such as slug,
orgId, or a system flag) instead of LIMIT 1; update the code that sets
workspaces/workspaceId (the workspaces variable and subsequent usage in the
integrationStitches insert/where) to use that filtered result so stitch
creation/lookup (schema.integrationStitches) always binds to the intended seeded
workspace.
In `@apps/api/src/db/services/tenant-schema.service.ts`:
- Around line 188-191: The catch around the "Shard Truncate" operation in
TenantSchemaService currently swallows all errors; change it so it only ignores
the specific "database does not exist" failure and rethrows any other errors.
Locate the try/catch that prints `⚠️ Shard Truncate failed (database might not
exist yet)` (the shard-truncate logic in the TenantSchemaService method) and
update the catch to inspect the error (e.g., check error.code === '3D000' or
error.message/includes('does not exist') / includes('invalid_catalog_name') or
similar DB-specific indicator) and only suppress the log when that check passes;
for all other errors, log and rethrow so permission, connectivity, and truncate
failures are not silently ignored.
- Around line 85-110: createTenantDatabase currently opens the admin PgClient
using process.env.DATABASE_URL, ignoring the provided hostUrl so CREATE DATABASE
may run on a different server than migrationRunner.migrateTenant; fix by
constructing/connecting the admin PgClient using hostUrl when present (e.g. new
PgClient({ connectionString: hostUrl || process.env.DATABASE_URL })) so the
CREATE DATABASE runs on the same host as migrationRunner.migrateTenant(dbName,
hostUrl), keep the dbName validation and the try/finally adminClient.end() logic
intact.
In `@apps/api/src/modules/connections/connection-lifecycle.service.ts`:
- Around line 293-329: The handler currently updates dataSources by matching
only externalId + appName (variables: dataSources, event.providerId,
event.credentialId) which can affect multiple tenants; change the logic to use a
tenant-scoped identity (include tenantId from the event payload or require
event.tenantId) or iterate over all returned rows: call
tx.update(...).where(...) .returning() and treat the result as an array (e.g.,
updatedConns), then for each updatedConn update credentials
(credentials.dataSourceId), insert a globalRegistryOutbox row with that
updatedConn, and emit a ConnectionPausedEvent per updatedConn (use
updatedConn.id and updatedConn.tenantId) so every affected connection gets its
own outbox entry and event; if event currently lacks tenantId, modify the event
producer to include tenantId + externalId (or include tenantId in the where
clause) so matches are tenant-scoped.
- Around line 175-281: The current flow takes a FOR UPDATE lock in the first
db.transaction then releases it before checking GEM and deleting, which creates
a race; fix by obtaining a cross-request mutex (Postgres advisory lock) around
the entire check-and-delete sequence so the lock is held while calling
storageResolver.resolveStorageProfile, tenantDb queries (dbManager.getTenantDb +
globalEntityMap check), and the final db.transaction that deletes from
dataSources and inserts into globalRegistryOutbox; specifically, acquire a
pg_advisory_xact_lock (or pg_advisory_lock) keyed on dataSourceId/tenantId at
the start of the operation, perform storageResolver.resolveStorageProfile and
the tenantDb mapping lookup, then run the delete/update inside the same
protected context (the existing db.transaction that deletes dataSources and
writes globalRegistryOutbox), and release the advisory lock at the end to ensure
the precondition cannot be invalidated between check and delete.
- Around line 75-109: The eventEmitter.emit('connection.provisioned', new
ConnectionSchemaProvisionedEvent(...)) is fired before the db.transaction that
marks the connection ACTIVE and inserts the globalRegistryOutbox; move the emit
so it only runs after the transaction successfully commits. Remove the current
pre-transaction emit, change the db.transaction block (db.transaction(...)) to
return the created/updated activeConn (or re-query the dataSource by
workspaceProvisionInfo.dataSourceId after the transaction), and then call
eventEmitter.emit with new ConnectionSchemaProvisionedEvent(tenantId,
activeConn.id or workspaceProvisionInfo.dataSourceId,
workspaceProvisionInfo.schemaName) after the await db.transaction completes so
listeners only see the event for a committed ACTIVE connection.
In `@apps/api/src/modules/connections/services/credential-linking.service.ts`:
- Around line 80-96: The update branch that performs tx.update(dataSources) must
also persist the deterministic schemaName like the insert and recovery paths do;
add schemaName to the .set({...}) payload (using the same computed schemaName
variable) and ensure it is returned by the .returning() call so pre-migration
rows get their routing column populated; update the block that sets displayName,
externalId, metadata, updatedAt and envType in credential-linking.service.ts
accordingly.
In `@apps/api/src/modules/connections/services/oauth-orchestration.service.ts`:
- Around line 255-261: The token request currently always includes client_secret
even when secretless; update the code that builds the request body (the
URLSearchParams creation in oauth-orchestration.service.ts) to only include
client_secret when clientSecret is non-empty/defined (e.g., build params with
grant_type, code, redirect_uri, client_id then conditionally append
client_secret if clientSecret truthy, or construct the params object dynamically
before passing to new URLSearchParams). Ensure the resulting body string omits
the client_secret key entirely for public/secretless clients.
In `@apps/api/src/modules/scheduler/infrastructure/fetch-http.client.ts`:
- Around line 20-22: When serializing options.body to JSON inside the fetch
helper, ensure you also set the Content-Type header to application/json: after
the block that does fetchOptions.body = JSON.stringify(options.body)
(referencing fetchOptions and options.body), add logic to initialize/merge
fetchOptions.headers and set fetchOptions.headers['Content-Type'] =
'application/json' only if a Content-Type header is not already provided
(preserve existing headers/casing). This ensures JSON bodies are sent with the
correct Content-Type without overwriting user-specified headers.
In `@apps/api/src/modules/scheduler/scheduler.service.ts`:
- Around line 108-114: The handleConnectionPaused event handler currently calls
this.schedulerClient.deleteSchedule(event.connectionId) without protection; wrap
that call in a try-catch inside the handleConnectionPaused method and, on
failure, call this.logger.error with a descriptive message that includes
event.connectionId, event.reason and the caught error (error.message / stack) so
you retain context (mirror the pattern used in deleteOrgSchedules); do not
swallow the error silently—log details and allow the handler to complete
gracefully.
In `@apps/api/src/modules/trigger/trigger-executor.service.ts`:
- Around line 345-349: When a record is inserted, the code currently
unconditionally calls payloadTransformer.extractRecordCursor(record) and awaits
store.put('last_cursor', sourceCursor), but extractRecordCursor can return
undefined which causes KeyValueTriggerStore.put to fail; modify the block inside
the didInsert branch (the code around didInsert/inserted, sourceCursor, and the
store.put('last_cursor', ...) call) to first call
this.payloadTransformer.extractRecordCursor(record), check if the returned
sourceCursor is defined (not undefined/null), and only then await
store.put('last_cursor', sourceCursor); skip the store.put when no cursor is
present to avoid turning successful inserts into store-write failures.
In `@apps/web/src/app/providers/data-provider.ts`:
- Line 26: Remove the unused type import CrudFilter from the import list
alongside GetListParams in the module where data provider types are defined;
update the import statement to only import GetListParams (or any other actually
used symbols) so linting errors stop, since CrudFilter is not referenced
anywhere and its type is already inferred from GetListParams.
In `@apps/worker/src/modules/pipeline/fanout-batch-processor.ts`:
- Around line 277-325: The PENDING row in outbound_gateway (inserted inside
tenantDb.transaction via destTx) is committed before queueService.send and if
sending fails the row remains PENDING and won't be retried; after catching
sendErr, run an explicit DB update to mark that specific row (identify by
traceId and routeId/stitch.id and dest_schema via the same tenantDb connection)
to a retryable failed state (e.g., set status='FAILED' or increment attempts and
set updated_at = NOW()) so the ON CONFLICT retry logic can pick it up; implement
this in the catch block for queueService.send (referencing queueService.send,
stitch.id, traceId, outbound_gateway, and destTx/tenantDb) and ensure the update
is executed against the same destination schema used for the INSERT.
In `@engine/sync/platform/core/src/sharding/pipeline-hook-broker.service.spec.ts`:
- Around line 96-100: The test currently asserts fulfillment with await
expect(service.provisionDomain(...)).resolves.not.toThrow(), which is valid but
less explicit for a Promise<void> return; change the assertion to await
expect(service.provisionDomain('salesforce', 'standard', {} as any,
'public')).resolves.toBeUndefined() so the test clearly documents that
provisionDomain (when loader.load returns incompleteShard with
extractReplica/normalize) resolves with no value; locate the test block using
the spec name and references to service.provisionDomain, (loader.load as
any).mockResolvedValue(incompleteShard), and the incompleteShard mock to update
the expectation.
In `@packages/cache/src/key-value-store.interface.ts`:
- Around line 1-11: The interface IKeyValueStore currently mixes generic KV
methods (get/set/del) with Redis-specific operations (eval, lpush, rpop, hget,
hset, hdel); either rename the interface to IRedisClient to accurately reflect
these Redis-only methods, or refactor by creating a minimal IKeyValueStore with
only set/get/del and move the Redis methods into a separate IRedisClient (or
IRedisExtensions) interface; update any implementations and imports to use the
new name(s) (IKeyValueStore or IRedisClient/IRedisExtensions) so callers and
classes referencing IKeyValueStore are updated accordingly.
- Line 2: The current set signature `set(key: string, value: string | Buffer |
number, mode?: string, duration?: number, flag?: string): Promise<'OK' | null>`
is ambiguous and fragile; change it to accept an options object and/or add
convenience methods: replace the method on the KeyValueStore interface with
`set(key: string, value: string|Buffer|number, options?: { ex?: number; px?:
number; nx?: boolean; xx?: boolean; keepTtl?: boolean; get?: boolean }):
Promise<'OK' | null>` (or similar) and optionally add `setEx`, `setNx` helpers
to the interface to mirror Redis semantics; update any implementors of `set` to
read named options instead of positional `mode/duration/flag`.
In `@packages/credentials/src/oauth/token-refresh.service.ts`:
- Line 48: Destructure and retain credentialId when first calling getCredentials
in token-refresh.service.ts (const { clientId, clientSecret, vendorParams,
credentialId } = await this.getCredentials(tenantId, appName, externalId)) and
then use that credentialId inside the error handler instead of calling
this.getCredentials a second time; remove the redundant getCredentials call in
the 400/401 error branch and reference the previously captured credentialId for
the event emission.
In `@packages/database/src/savepoint/savepoint.manager.ts`:
- Around line 16-26: The savepoint methods createSavepoint, releaseSavepoint,
and rollbackToSavepoint do not validate the name parameter and can generate
malformed SQL for empty/null/undefined values; update each method in
savepoint.manager.ts to first validate that name is a non-empty string (e.g.,
typeof name === 'string' && name.trim().length > 0) and throw a clear error
(TypeError or custom) if invalid, and only then call tx.execute(sql`...
${sql.identifier(name)}`) so invalid names are rejected before hitting the
database.
In `@packages/identity/src/adapters/better-auth.adapter.ts`:
- Around line 403-414: The current try/catch around
eventPublisher.publishUserInvited swallows failures (in better-auth.adapter.ts)
so add robust handling: replace the single try/catch with a small retry loop
(e.g., 3 attempts with exponential backoff) that calls
eventPublisher.publishUserInvited(new UserInvitedEvent(...)); on final failure,
log the full error via processLogger/error and either rethrow or call an
alerting/metrics hook (e.g., emitEventFailure or increment a counter) so the
caller can observe the failure; keep the UserInvitedEvent construction unchanged
and extract the publish call into a helper (publishWithRetry) for clarity.
In `@packages/identity/src/adapters/drizzle-tenant.adapter.ts`:
- Around line 62-68: The current try/catch around
eventPublisher.publishTenantProvisioned in the create flow
(publishTenantProvisioned, TenantProvisionedEvent) swallows failures and risks
lost events; implement an outbox: inside the same DB transaction used by the
create method in DrizzleTenantAdapter persist the TenantProvisionedEvent payload
to an outbox/events table instead of calling eventPublisher directly, remove or
convert the current publish call to an async dispatcher, and add a separate
background processor that reads the outbox, calls
eventPublisher.publishTenantProvisioned, retries on failure, and marks rows as
sent (or deletes them) on success; ensure the insert to the outbox happens
within the transaction so event persistence is atomic with tenant creation.
In `@packages/identity/src/identity.module.ts`:
- Around line 102-105: The provider for IDENTITY_EVENT_PUBLISHER currently uses
useExisting with a fallback to IDENTITY_EVENT_PUBLISHER, creating a circular
unresolved dependency; update the provider to choose between
useExisting(options.eventPublisherToken) when options.eventPublisherToken is
supplied and useClass(IdentityEventPublisher) as the default otherwise (mirror
the registerAsync() pattern), so the provider either proxies an externally
supplied token or registers IdentityEventPublisher as the concrete
implementation for IDENTITY_EVENT_PUBLISHER.
In `@packages/identity/src/interfaces/event-publisher.interface.ts`:
- Around line 5-6: Change the two methods on the EventPublisher interface to be
async-only by replacing the union return types with Promise<void>; specifically
update publishTenantProvisioned and publishUserInvited signatures to return
Promise<void> (not Promise<void> | void) and adjust any concrete implementations
or mocks (e.g., classes implementing EventPublisher and test doubles) to
return/respect promises so callers can uniformly await them.
---
Outside diff comments:
In `@apps/api/src/modules/connections/connections/connectors.controller.spec.ts`:
- Around line 110-128: The test suite is instantiating ConnectorsController
directly but ConnectionsModule now exposes OAuthController and
CredentialController; update tests to exercise the real controllers and route
wiring by either moving the critical specs into the new OAuthController and
CredentialController tests or replace the Test.createTestingModule invocation to
import the live ConnectionsModule (or explicitly list OAuthController and
CredentialController in controllers) so the actual DTOs/guards/pipes are
exercised; update references to ConnectorsController, ConnectionsModule,
OAuthController, CredentialController and the Test.createTestingModule setup
accordingly and remove the direct instantiation of ConnectorsController used in
controller = module.get<ConnectorsController>(ConnectorsController).
In `@apps/api/src/modules/exceptions/exception.service.ts`:
- Around line 322-359: The dispatch logic (this.queueDispatcher.dispatchRetry)
must run after the DB transaction commits: inside the transaction
(this.db.transaction) only perform assertValidSchemaName, SET LOCAL search_path,
the tx.update(...) call that sets status to 'PENDING' and its .returning(...)
and throw the ConflictException if updatedRows.length === 0, then capture the
returned row data (traceId, routeId, payload) and any needed ids
(outboundGatewayRow.srcDataSourceId, stitch.destDataSourceId) into a local
variable and let the transaction complete; once the transaction resolves
successfully, call this.queueDispatcher.dispatchRetry using those captured
values; also add a clear error-handling path for dispatch failures (retry/outbox
or mark status) outside the transaction so the DB update and external dispatch
are not executed inside the same transaction.
In `@apps/api/src/modules/trigger/dlq-processor.service.ts`:
- Around line 121-127: When getTrigger() returns undefined, do not call
this.dlqService.acknowledgeJob(raw) (which permanently deletes the message);
instead move the message to the failed bucket or leave it unacknowledged so
operators can recover it later — e.g., replace the acknowledgeJob(raw) call with
a call to a failure-path method such as this.dlqService.moveToFailed(raw, {
reason: 'unknown trigger', appName: job.appName, triggerName: job.triggerName })
or, if no moveToFailed exists, omit acknowledgement so the job remains in the
DLQ and add a clear warn log via this.logger.warn(...) with the same metadata.
In `@apps/worker/src/modules/pipeline/delivery.service.ts`:
- Around line 658-709: The L6 sync_log insert and activeSyncLocks deletion are
happening before gemService.writeGemMapping(), which can leave an L6 marker
without GEM linkage; move the L6 completion and lock release to occur only after
writeGemMapping() succeeds (or include the GEM upsert inside the same
transactional boundary) so the finalization marker (the
tx.insert(syncLog)...onConflictDoNothing and the tx.delete(activeSyncLocks)
against activeSyncLocks.lockedByTraceId) is only applied when
gemService.writeGemMapping(...) completes successfully; then set sourceCommitted
= true after that step. Ensure isSourceFinalized()/replay logic now sees L6 only
when GEM persistence has completed.
In `@apps/worker/src/modules/pipeline/inbound-outbox.poller.ts`:
- Around line 92-119: The claim is not protected against mid-air reclamation:
when you transition rows to PROCESSING in the tenantDb.transaction (the UPDATE
that sets status="PROCESSING" and nextRetryAt), record a monotonic claim token
(or keep the claimed nextRetryAt timestamp) on that row and use that
token/timestamp in all subsequent compare-and-swap updates (the code paths that
set status => SUCCESS, RETRY, FAIL — e.g. the FAIL path mentioned and the
updates in the ~183-225 section, and code around processOutboxRow) so only the
current owner can complete the row; i.e., set claimToken (or preserve
nextRetryAt) when claiming and add WHERE claim_token = <claimedToken> (or WHERE
next_retry_at = <claimedLeaseValue>) to all status-updating queries so reclaimed
rows by stale owners cannot overwrite a new claimant.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 65cb6ecb-4f8b-4d64-b18e-6d85d7c822d8
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (144)
apps/api/package.jsonapps/api/src/app/app.module.tsapps/api/src/db/cli/cli.module.tsapps/api/src/db/cli/commands/db-debug.command.tsapps/api/src/db/cli/commands/db-fresh.command.tsapps/api/src/db/cli/commands/db-migrate.command.tsapps/api/src/db/cli/commands/db-provision.command.tsapps/api/src/db/cli/commands/db-reset.command.tsapps/api/src/db/cli/commands/db-seed.command.tsapps/api/src/db/cli/main.tsapps/api/src/db/data/seed-abac.tsapps/api/src/db/data/seed-mappings.tsapps/api/src/db/database-manager.spec.tsapps/api/src/db/database-manager.tsapps/api/src/db/infrastructure/migration-runner.service.tsapps/api/src/db/infrastructure/pg-connection.pool.tsapps/api/src/db/services/connection-schema-provisioner.service.tsapps/api/src/db/services/database-reset.service.tsapps/api/src/db/services/dev-sandbox-provisioner.service.tsapps/api/src/db/services/environment-guard.service.tsapps/api/src/db/services/rbac-inspector.service.tsapps/api/src/db/services/sync-seeding.service.tsapps/api/src/db/services/system-seeder.service.tsapps/api/src/db/services/tenant-schema.service.tsapps/api/src/modules/connections/connection-lifecycle.service.spec.tsapps/api/src/modules/connections/connection-lifecycle.service.tsapps/api/src/modules/connections/connections.module.tsapps/api/src/modules/connections/connections/connectors.controller.spec.tsapps/api/src/modules/connections/connections/credential.controller.tsapps/api/src/modules/connections/connections/oauth.controller.tsapps/api/src/modules/connections/connectors.service.spec.tsapps/api/src/modules/connections/connectors.service.tsapps/api/src/modules/connections/events/connection-paused.event.tsapps/api/src/modules/connections/events/connection-schema-provisioned.event.tsapps/api/src/modules/connections/oauth-url-builder.spec.tsapps/api/src/modules/connections/oauth-url-builder.tsapps/api/src/modules/connections/services/credential-linking.service.tsapps/api/src/modules/connections/services/oauth-orchestration.service.tsapps/api/src/modules/exceptions/exception.service.spec.tsapps/api/src/modules/exceptions/exception.service.tsapps/api/src/modules/exceptions/exceptions.module.tsapps/api/src/modules/exceptions/infrastructure/soopa-delivery-queue-dispatcher.service.tsapps/api/src/modules/exceptions/interfaces/delivery-queue-dispatcher.interface.tsapps/api/src/modules/identity/auth/auth.controller.tsapps/api/src/modules/identity/system-admin/system-admin.controller.spec.tsapps/api/src/modules/identity/system-admin/system-admin.controller.tsapps/api/src/modules/pipeline/pipeline-cdc.listener.tsapps/api/src/modules/pipeline/pipeline.module.tsapps/api/src/modules/scheduler/infrastructure/fetch-http.client.tsapps/api/src/modules/scheduler/infrastructure/stub-scheduler.client.spec.tsapps/api/src/modules/scheduler/infrastructure/stub-scheduler.client.tsapps/api/src/modules/scheduler/infrastructure/windmill-scheduler.client.spec.tsapps/api/src/modules/scheduler/infrastructure/windmill-scheduler.client.tsapps/api/src/modules/scheduler/interfaces/http-client.interface.tsapps/api/src/modules/scheduler/interfaces/scheduler-client.interface.tsapps/api/src/modules/scheduler/scheduler.module.tsapps/api/src/modules/scheduler/scheduler.service.spec.tsapps/api/src/modules/scheduler/scheduler.service.tsapps/api/src/modules/scheduler/windmill.client.tsapps/api/src/modules/trigger/dlq-processor.service.spec.tsapps/api/src/modules/trigger/dlq-processor.service.tsapps/api/src/modules/trigger/infrastructure/redis-distributed-lock.service.tsapps/api/src/modules/trigger/infrastructure/redis-trigger-dlq.service.tsapps/api/src/modules/trigger/interfaces/distributed-lock.interface.tsapps/api/src/modules/trigger/interfaces/trigger-dlq.interface.tsapps/api/src/modules/trigger/key-value-trigger-store.spec.tsapps/api/src/modules/trigger/key-value-trigger-store.tsapps/api/src/modules/trigger/trigger-executor.service.spec.tsapps/api/src/modules/trigger/trigger-executor.service.tsapps/api/src/modules/trigger/trigger-payload-transformer.spec.tsapps/api/src/modules/trigger/trigger-payload-transformer.tsapps/api/src/modules/trigger/trigger-retry-policy.service.spec.tsapps/api/src/modules/trigger/trigger-retry-policy.service.tsapps/api/src/modules/trigger/trigger.module.tsapps/api/src/scripts/admin-bootstrap.tsapps/api/src/scripts/test-e2e-ingestion.tsapps/api/vitest.config.mtsapps/tenant-provisioner/vitest.config.tsapps/web/src/app/providers/data-provider.tsapps/web/src/test/environments/jsdom-msw.tsapps/web/vitest.config.tsapps/worker/src/db/database-manager.spec.tsapps/worker/src/db/database-manager.tsapps/worker/src/modules/pipeline/delivery-retry.service.tsapps/worker/src/modules/pipeline/delivery.service.spec.tsapps/worker/src/modules/pipeline/delivery.service.tsapps/worker/src/modules/pipeline/fanout-batch-processor.tsapps/worker/src/modules/pipeline/fanout-router.service.tsapps/worker/src/modules/pipeline/gem-hydration.service.tsapps/worker/src/modules/pipeline/inbound-outbox.poller.tsapps/worker/src/modules/pipeline/pipeline.module.tsapps/worker/src/modules/pipeline/registry-replication.service.tsapps/worker/vitest.config.mtsengine/ai/core/vitest.config.tsengine/sync/application/mapping/package.jsonengine/sync/application/mapping/src/mapping-engine.tsengine/sync/application/salesforce/revenova/replicate.jsengine/sync/application/salesforce/revenova/replicate.tsengine/sync/platform/core/src/sharding/pipeline-hook-broker.service.spec.tsengine/sync/platform/core/vitest.config.tspackages/auth/src/services/auth.service.tspackages/cache/src/index.tspackages/cache/src/key-value-store.interface.tspackages/credentials/package.jsonpackages/credentials/src/events/credential-deleted.event.tspackages/credentials/src/events/credential-invalidated.event.tspackages/credentials/src/events/credential-refreshed.event.tspackages/credentials/src/events/index.tspackages/credentials/src/index.tspackages/credentials/src/interfaces/event-publisher.interface.tspackages/credentials/src/interfaces/index.tspackages/credentials/src/oauth/redis-lock.tspackages/credentials/src/oauth/token-manager.service.spec.tspackages/credentials/src/oauth/token-manager.service.tspackages/credentials/src/oauth/token-refresh.service.spec.tspackages/credentials/src/oauth/token-refresh.service.tspackages/credentials/src/services/credentials-event-publisher.service.tspackages/credentials/vitest.config.tspackages/database/src/database.module.tspackages/database/src/index.tspackages/database/src/savepoint/index.tspackages/database/src/savepoint/savepoint.manager.tspackages/database/src/schema/global/credentials.tspackages/database/src/schema/global/data-sources.tspackages/dbmanager/src/cli/interfaces.tspackages/dbmanager/src/impl/tenant-database-manager.tspackages/dbmanager/src/index.tspackages/identity/package.jsonpackages/identity/src/adapters/better-auth.abac.spec.tspackages/identity/src/adapters/better-auth.adapter.spec.tspackages/identity/src/adapters/better-auth.adapter.tspackages/identity/src/adapters/drizzle-tenant.adapter.spec.tspackages/identity/src/adapters/drizzle-tenant.adapter.tspackages/identity/src/constants.tspackages/identity/src/events/index.tspackages/identity/src/events/tenant-provisioned.event.tspackages/identity/src/events/user-invited.event.tspackages/identity/src/identity.module.tspackages/identity/src/interfaces/auth-provider.interface.tspackages/identity/src/interfaces/event-publisher.interface.tspackages/identity/src/interfaces/index.tspackages/identity/src/services/identity-event-publisher.service.spec.tspackages/identity/src/services/identity-event-publisher.service.tspackages/identity/src/services/permission-seeder.spec.ts
💤 Files with no reviewable changes (3)
- apps/api/src/modules/scheduler/windmill.client.ts
- apps/worker/src/db/database-manager.spec.ts
- apps/worker/src/db/database-manager.ts
| import { DATABASE_CONNECTION } from '@soopa/database'; | ||
| import { NodePgDatabase } from 'drizzle-orm/node-postgres'; | ||
| import * as schema from '../db/schema.js'; | ||
| import { DATABASE_CONNECTION, type DrizzleDb } from '@soopa/database'; |
Contributor
There was a problem hiding this comment.
Verify type compatibility and eliminate the unsafe cast.
The migration from NodePgDatabase<typeof schema> to DrizzleDb is followed by an unsafe as any cast at line 92, which suppresses type checking. This defeats the purpose of TypeScript's type safety and may hide compatibility issues between the two type definitions.
Run the following script to verify the type definitions are compatible:
#!/bin/bash
# Check if DrizzleDb is properly exported and used consistently
rg -n "DrizzleDb|NodePgDatabase" --type=ts -C3 -g '!node_modules' -g '!dist'Also applies to: 70-70, 92-92
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/app/app.module.ts` at line 17, The code is suppressing type
checking by casting a NodePgDatabase<typeof schema> to DrizzleDb using "as any";
replace the unsafe cast by making the provider and injection types consistent:
import and use the correct DrizzleDb type where DATABASE_CONNECTION is provided
and ensure the factory that creates NodePgDatabase<typeof schema> is typed to
return DrizzleDb (or update DATABASE_CONNECTION's declared type to
NodePgDatabase<typeof schema> if that is the canonical type), remove "as any" in
the provider (refer to the provider/factory that constructs
NodePgDatabase<typeof schema> and the DATABASE_CONNECTION token) and update any
consuming constructor/injection types to match the chosen type so TypeScript can
verify compatibility.
Comment on lines
+42
to
+49
| @Option({ | ||
| flags: '-s, --schema [name]', | ||
| description: | ||
| 'The physical schema name (e.g. ws_salesforce_abc123) for gateway/outbound commands', | ||
| }) | ||
| parseSchemaName(val: string): string { | ||
| return val; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use required argument <name> to match usage and simplify validation.
The flag is defined with optional argument [name] (line 43), but the usage message consistently shows <schema> (required) at lines 74, 84, and 96. Using [name] allows Commander to accept --schema without a value (setting options.schema = true), which then requires the manual typeof options.schema !== 'string' check at line 91.
Changing to <name> would make Commander enforce the value requirement immediately, provide better error messages, simplify the validation logic (no need for the typeof check), and align with the documented usage.
♻️ Proposed fix
`@Option`({
- flags: '-s, --schema [name]',
+ flags: '-s, --schema <name>',
description:
'The physical schema name (e.g. ws_salesforce_abc123) for gateway/outbound commands',
})
parseSchemaName(val: string): string {
return val;
}Then simplify the validation at lines 90-100:
// Validate schema is required for gateway and outbound
if (options?.gateway || options?.outbound) {
- if (!options?.schema || typeof options.schema !== 'string') {
+ if (!options?.schema) {
console.error(
'Error: --schema <schema> is required when using --gateway or --outbound',
);📝 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
| @Option({ | |
| flags: '-s, --schema [name]', | |
| description: | |
| 'The physical schema name (e.g. ws_salesforce_abc123) for gateway/outbound commands', | |
| }) | |
| parseSchemaName(val: string): string { | |
| return val; | |
| } | |
| `@Option`({ | |
| flags: '-s, --schema <name>', | |
| description: | |
| 'The physical schema name (e.g. ws_salesforce_abc123) for gateway/outbound commands', | |
| }) | |
| parseSchemaName(val: string): string { | |
| return val; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/db/cli/commands/db-provision.command.ts` around lines 42 - 49,
Change the Option decorator on parseSchemaName to require a value by using
'<name>' instead of '[name]' so Commander enforces a schema value (update flags
string '-s, --schema <name>'). Then remove the manual typeof options.schema !==
'string' validation and related branching in the command validation logic (the
code around options.schema checks and error throw), since Commander will
guarantee a string; keep parseSchemaName as the converter. Also update any
usage/help text that shows '<schema>' to remain consistent with the new required
argument.
Comment on lines
+403
to
+414
| try { | ||
| await this.eventPublisher.publishUserInvited( | ||
| new UserInvitedEvent( | ||
| invitation.id, | ||
| invitation.email, | ||
| invitation.organizationId || "", | ||
| invitation.role || "member", | ||
| ), | ||
| ); | ||
| } catch (err) { | ||
| console.error("Failed to publish UserInvitedEvent", err); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Event publishing errors are suppressed.
Event publishing failures are caught, logged, and do not prevent the invitation from being returned. While this defensive pattern prevents cascading failures, it could lead to silent event loss if the publisher has transient issues. Consider whether events are critical enough to warrant retry logic or alerting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/identity/src/adapters/better-auth.adapter.ts` around lines 403 -
414, The current try/catch around eventPublisher.publishUserInvited swallows
failures (in better-auth.adapter.ts) so add robust handling: replace the single
try/catch with a small retry loop (e.g., 3 attempts with exponential backoff)
that calls eventPublisher.publishUserInvited(new UserInvitedEvent(...)); on
final failure, log the full error via processLogger/error and either rethrow or
call an alerting/metrics hook (e.g., emitEventFailure or increment a counter) so
the caller can observe the failure; keep the UserInvitedEvent construction
unchanged and extract the publish call into a helper (publishWithRetry) for
clarity.
Comment on lines
+62
to
+68
| try { | ||
| await this.eventPublisher.publishTenantProvisioned( | ||
| new TenantProvisionedEvent(orgId, userId, name), | ||
| ); | ||
| } catch (err) { | ||
| console.error("Failed to publish TenantProvisionedEvent", err); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Verify event publisher error handling meets reliability requirements.
Event publication errors are logged but don't fail the create operation. This "best-effort" pattern is acceptable for non-critical notifications but means lost events if the publisher fails.
For workflows requiring guaranteed event delivery, consider an outbox pattern: write events to a DB table within the transaction and process them asynchronously.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/identity/src/adapters/drizzle-tenant.adapter.ts` around lines 62 -
68, The current try/catch around eventPublisher.publishTenantProvisioned in the
create flow (publishTenantProvisioned, TenantProvisionedEvent) swallows failures
and risks lost events; implement an outbox: inside the same DB transaction used
by the create method in DrizzleTenantAdapter persist the TenantProvisionedEvent
payload to an outbox/events table instead of calling eventPublisher directly,
remove or convert the current publish call to an async dispatcher, and add a
separate background processor that reads the outbox, calls
eventPublisher.publishTenantProvisioned, retries on failure, and marks rows as
sent (or deletes them) on success; ensure the insert to the outbox happens
within the transaction so event persistence is atomic with tenant creation.
Comment on lines
+5
to
+6
| publishTenantProvisioned(event: TenantProvisionedEvent): Promise<void> | void; | ||
| publishUserInvited(event: UserInvitedEvent): Promise<void> | void; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider enforcing async-only methods for clarity.
Methods return Promise<void> | void, allowing synchronous implementations. While this provides testing flexibility, event publishing typically involves I/O and should be asynchronous. Enforcing Promise<void> signals this expectation and prevents accidental blocking in critical paths.
The current design works—calling code already uses await—but async-only would be clearer.
♻️ Proposed refinement
export interface IIdentityEventPublisher {
- publishTenantProvisioned(event: TenantProvisionedEvent): Promise<void> | void;
- publishUserInvited(event: UserInvitedEvent): Promise<void> | void;
+ publishTenantProvisioned(event: TenantProvisionedEvent): Promise<void>;
+ publishUserInvited(event: UserInvitedEvent): Promise<void>;
}📝 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
| publishTenantProvisioned(event: TenantProvisionedEvent): Promise<void> | void; | |
| publishUserInvited(event: UserInvitedEvent): Promise<void> | void; | |
| export interface IIdentityEventPublisher { | |
| publishTenantProvisioned(event: TenantProvisionedEvent): Promise<void>; | |
| publishUserInvited(event: UserInvitedEvent): Promise<void>; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/identity/src/interfaces/event-publisher.interface.ts` around lines 5
- 6, Change the two methods on the EventPublisher interface to be async-only by
replacing the union return types with Promise<void>; specifically update
publishTenantProvisioned and publishUserInvited signatures to return
Promise<void> (not Promise<void> | void) and adjust any concrete implementations
or mocks (e.g., classes implementing EventPublisher and test doubles) to
return/respect promises so callers can uniformly await them.
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 21 file(s) based on 30 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 21 file(s) based on 30 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 29
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/api/package.json (1)
21-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winComplete the CLI migration for all database scripts.
Lines 22-27 were migrated to the new
src/db/cli/main.tsentrypoint, but lines 21 and 28-31 still reference the oldsrc/db/db-cli.tspath. This inconsistency will cause these commands to fail if the old CLI file is removed or refactored.🔧 Proposed fix
Migrate the remaining scripts to the new CLI or document why they remain separate:
- "db:drop": "tsx src/db/db-cli.ts drop", + "db:drop": "tsx src/db/cli/main.ts db:drop", "db:seed": "tsx src/db/cli/main.ts db:seed", "db:fresh": "tsx src/db/cli/main.ts db:fresh", "db:reset": "tsx src/db/cli/main.ts db:reset", "db:migrate": "tsx src/db/cli/main.ts db:migrate", "db:provision": "tsx src/db/cli/main.ts db:provision", "db:debug": "tsx src/db/cli/main.ts db:debug", - "db:provision:local": "tsx src/db/db-cli.ts provision:local", - "db:provision:gateway": "tsx src/db/db-cli.ts provision:gateway", - "db:migrate:schemas": "tsx src/db/db-cli.ts migrate:schemas", - "db:check-role": "tsx src/db/db-cli.ts check-role", + "db:provision:local": "tsx src/db/cli/main.ts db:provision:local", + "db:provision:gateway": "tsx src/db/cli/main.ts db:provision:gateway", + "db:migrate:schemas": "tsx src/db/cli/main.ts db:migrate:schemas", + "db:check-role": "tsx src/db/cli/main.ts db:check-role",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/package.json` around lines 21 - 31, Update the inconsistent npm scripts so they all use the new CLI entrypoint: change the "db:drop", "db:provision:local", "db:provision:gateway", "db:migrate:schemas", and "db:check-role" scripts to invoke "src/db/cli/main.ts" (same as "db:seed","db:fresh","db:reset","db:migrate","db:provision","db:debug") or, if any must remain on "src/db/db-cli.ts", add a comment explaining why and keep a shim that forwards to the new main CLI; ensure you update the script targets for those exact script names so they won't break when src/db/db-cli.ts is removed.apps/worker/src/modules/pipeline/inbound-outbox.poller.ts (1)
194-230:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the log message to use the incremented attempt count.
Line 228 logs
attempt ${row.attempts}but should logattempt ${incrementedAttempts}to reflect the actual attempt count after incrementing.🐛 Proposed fix
this.logger.warn( - `[${schemaName}] InboundOutbox delivery delayed for traceId=${row.traceId} (attempt ${row.attempts}): ${errorMessage}`, + `[${schemaName}] InboundOutbox delivery delayed for traceId=${row.traceId} (attempt ${incrementedAttempts}): ${errorMessage}`, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/worker/src/modules/pipeline/inbound-outbox.poller.ts` around lines 194 - 230, The warning log uses the old attempt count (row.attempts) instead of the incremented value; update the logger.warn call in inbound-outbox.poller.ts to interpolate incrementedAttempts (the variable set to row.attempts + 1) so the message shows the correct attempt number for traceId=row.traceId and keep the surrounding condition and DB update that set attempts to incrementedAttempts unchanged.apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts (1)
361-375: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd test coverage for the last-admin deletion prevention.
The test suite only covers the success path where
deleteIfNotLastAdminreturns{ success: true }. Add a test case that verifies aBadRequestExceptionis thrown when attempting to delete the last Platform Administrator (i.e., whendeleteIfNotLastAdminreturns{ success: false }).🧪 Suggested test case
}); }); + + describe('deleteUser - last admin protection', () => { + it('should throw BadRequestException when attempting to delete the last admin', async () => { + mockUserProvider.findById.mockResolvedValue({ + id: 'u1', + }); + mockUserProvider.deleteIfNotLastAdmin.mockResolvedValue({ + success: false, + }); + + await expect(controller.deleteUser('u1')).rejects.toThrow( + BadRequestException, + ); + await expect(controller.deleteUser('u1')).rejects.toThrow( + 'Cannot delete the last Platform Administrator', + ); + }); + }); describe('inviteUser', () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts` around lines 361 - 375, Add a test to cover the failure path in system-admin.controller.spec.ts: mock mockUserProvider.findById to return the user id ('u1') and mock mockUserProvider.deleteIfNotLastAdmin to resolve to { success: false }, then call controller.deleteUser('u1') and assert it rejects/throws a BadRequestException (use expect(...).rejects.toThrow(BadRequestException) or equivalent). Ensure the expectation also verifies deleteIfNotLastAdmin was called with 'u1' and getRequiredSystemTenantId() to match the success test pattern.apps/api/src/modules/trigger/trigger-executor.service.spec.ts (1)
237-240:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the typo property
run反Pollfrom the mock.Line 238 defines a property named
run反Poll(containing a Chinese character) that is never used. Line 239 correctly definesrunPoll. Remove the typo property.🧹 Proposed fix
const failingExecutor = { - run反Poll: vi.fn().mockRejectedValue(new Error('permanent fail')), runPoll: vi.fn().mockRejectedValue(new Error('permanent fail')), } as unknown as TriggerExecutorService;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/trigger/trigger-executor.service.spec.ts` around lines 237 - 240, In the test setup inside trigger-executor.service.spec.ts, remove the stray/typo property named "run反Poll" from the mock object (the block that also contains auth, propsValue, workspaceId, dataSourceId) because only "runPoll" is used; ensure the mock only defines "runPoll" and not the unintended "run反Poll" so tests reference the correct property.apps/api/src/modules/trigger/key-value-trigger-store.ts (1)
4-20: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUpdate the doc comment to reflect the generic key-value abstraction.
The comment still describes Redis-specific implementation details (Redis Hash, Redis keys, etc.) but the class now uses a generic
IKeyValueStore. Update the documentation to reflect the abstraction rather than a specific backing store.📝 Suggested documentation update
/** - * Redis-backed TriggerStore. + * Key-value-backed TriggerStore. * - * All cursor state is stored as a Redis Hash under a fully-qualified key: + * All cursor state is stored as a hash under a fully-qualified key: * cursor:{workspaceId}:{appName}:{objectType}:{triggerName} * * Using all four dimensions prevents cross-object collisions when the same * trigger runs for different object types (e.g., Account vs Contact) within * the same workspace. * * Key namespace separation: * - Cursor keys: cursor:... * - Rate-limiter keys: rl:... * - Token cache keys: token:... * - DLQ list: dlq:triggers * - Poll locks: lock:poll:... */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/trigger/key-value-trigger-store.ts` around lines 4 - 20, The doc comment for the Redis-backed TriggerStore is out of date now that the implementation uses the generic IKeyValueStore; update the header in key-value-trigger-store.ts to describe a generic key-value backed TriggerStore (mentioning IKeyValueStore) and remove Redis-specific details (Redis Hash, Redis key prefixes). Keep the explanation of the fully-qualified cursor key shape and namespace separation but phrase them as logical key namespaces used by the key-value store (e.g., cursor:{workspaceId}:{appName}:{objectType}:{triggerName}, rl:..., token:..., dlq:..., lock:...) and note that the underlying storage is abstracted by IKeyValueStore rather than tied to Redis.packages/credentials/src/oauth/token-manager.service.ts (1)
151-158:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap lock release in try/catch to prevent masking the original error.
If
lock.release()throws at line 157, it will override any error being rethrown at line 155, masking the root cause of the refresh failure. In async finally blocks, exceptions from the finally always win.🔒 Proposed fix to prevent error masking
} finally { - await this.releaseLock(lockKey, lockValue); + try { + await this.releaseLock(lockKey, lockValue); + } catch (releaseErr) { + this.logger.warn( + `Failed to release lock ${lockKey}: ${ + releaseErr instanceof Error ? releaseErr.message : String(releaseErr) + }`, + ); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/credentials/src/oauth/token-manager.service.ts` around lines 151 - 158, The finally block in refresh handling should not let releaseLock errors mask the original refresh error: modify the block around performTokenRefresh/handleRefreshError to call releaseLock(lockKey, lockValue) inside its own try/catch (or try/catch/finally) and log or swallow releaseLock errors while preserving and rethrowing the original error thrown by performTokenRefresh/handleRefreshError; reference the existing methods performTokenRefresh, handleRefreshError and releaseLock and ensure the original error is rethrown after releaseLock is attempted.
♻️ Duplicate comments (4)
apps/worker/src/modules/pipeline/delivery.service.spec.ts (1)
90-101: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy liftTest coverage gap remains for retry service logic.
The retry service is still mocked, so the new transactional
search_path/ replay-query logic introduced inDeliveryRetryServiceis not covered by tests. Consider adding a dedicateddelivery-retry.service.spec.tsfile to test the retry logic directly, or replace the mock with the real service in at least one integration-style test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/worker/src/modules/pipeline/delivery.service.spec.ts` around lines 90 - 101, The test currently mocks DeliveryRetryService (handleRetryAndDelay, isSourceFinalized, retrySourceFinalization) so the new transactional search_path and replay-query logic in DeliveryRetryService isn't exercised; add a new unit test file delivery-retry.service.spec.ts that imports the real DeliveryRetryService and its dependencies (or alternatively replace the mocked provider in one existing integration-style test with the real DeliveryRetryService) and write tests targeting handleRetryAndDelay and retrySourceFinalization flows to cover the transactional search_path/replay behavior, stubbing only external DB or network calls as needed so the transactional logic runs in tests.packages/identity/src/identity.module.ts (1)
102-105:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCircular dependency when
eventPublisherTokenis undefined.Line 104 creates a circular reference: when
options.eventPublisherTokenisundefined, the provider resolves to{ provide: IDENTITY_EVENT_PUBLISHER, useExisting: IDENTITY_EVENT_PUBLISHER }. This will fail at runtime with an unresolved dependency error becauseIDENTITY_EVENT_PUBLISHERreferences itself without a concrete implementation.The
registerAsync()path (lines 201-204) correctly usesuseClass: IdentityEventPublisheras the default. Apply the same pattern here.🔧 Proposed fix to provide a default implementation
{ provide: IDENTITY_EVENT_PUBLISHER, - useExisting: options.eventPublisherToken || IDENTITY_EVENT_PUBLISHER, + ...(options.eventPublisherToken + ? { useExisting: options.eventPublisherToken } + : { useClass: IdentityEventPublisher }), },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/identity/src/identity.module.ts` around lines 102 - 105, The provider for IDENTITY_EVENT_PUBLISHER creates a self-referential circular dependency when options.eventPublisherToken is undefined; change the provider so that if options.eventPublisherToken is present it uses useExisting: options.eventPublisherToken, otherwise it provides a concrete default by using useClass: IdentityEventPublisher (same pattern as registerAsync()). Update the provider entry that references IDENTITY_EVENT_PUBLISHER and options.eventPublisherToken to conditionally choose useExisting vs useClass IdentityEventPublisher so the token is never bound to itself.apps/api/src/db/services/tenant-schema.service.ts (1)
102-102:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
pg-formatfor CREATE DATABASE identifier.Line 102 interpolates
dbNamedirectly into SQL despite the regex validation. This is inconsistent with line 78, which correctly usespg-formatfor DROP DATABASE. While the regex provides some protection, using the same escaping mechanism throughout reduces risk and improves maintainability.🔒 Proposed fix to use pg-format consistently
+ const format = (await import('pg-format')).default; if (!/^[a-zA-Z0-9_]+$/.test(dbName)) { throw new Error(`Invalid tenant database name: ${dbName}`); } - await adminClient.query(`CREATE DATABASE "${dbName}"`); + await adminClient.query(format('CREATE DATABASE %I', dbName)); console.log(` ✓ Created tenant database: ${dbName}`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/db/services/tenant-schema.service.ts` at line 102, Replace the direct string interpolation in the adminClient.query call that runs CREATE DATABASE by using pg-format to safely escape the identifier (e.g. use format('CREATE DATABASE %I', dbName)); import format from 'pg-format' if not already present and mirror the approach used for DROP DATABASE so adminClient.query uses format(...) instead of `"${dbName}"`, referencing the existing adminClient.query call and the dbName variable.apps/worker/src/modules/pipeline/delivery-retry.service.ts (1)
94-101:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd
destVendorIdto the SELECT clause.Lines 132-135 read
existingResult[0].destVendorId, but the query here does not select that field—onlyresponseandstatusCodeare retrieved.destVendorIdwill always beundefined, breaking the replay logic that depends on it.🐛 Proposed fix
return await tx .select({ response: outboundGateway.response, statusCode: outboundGateway.statusCode, + destVendorId: outboundGateway.destVendorId, }) .from(outboundGateway)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/worker/src/modules/pipeline/delivery-retry.service.ts` around lines 94 - 101, The SELECT that builds the existingResult uses tx.select({... response, statusCode ...}) from outboundGateway but omits destVendorId, so existingResult[0].destVendorId is always undefined; update the select object used in the transaction (the call that references outboundGateway.response and outboundGateway.statusCode) to also include destVendorId (e.g., add destVendorId: outboundGateway.destVendorId) so the replay logic can read existingResult[0].destVendorId correctly and adjust any related types/aliases if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/db/cli/commands/db-fresh.command.ts`:
- Around line 13-20: The run() method currently catches errors from
resetService.fresh() and calls process.exit(1); remove the abrupt exit and let
Nest/Aggregator handle shutdown by either rethrowing the caught error (throw
err) or removing the try/catch so the exception bubbles up; ensure you still log
the failure (e.g., console.error or this.logger.error) with the error details
before rethrowing so resetService.fresh() failures are visible but allow NestJS
to perform graceful cleanup.
In `@apps/api/src/db/cli/commands/db-reset.command.ts`:
- Around line 13-20: The catch block in async run() swallows the error and calls
process.exit(1); instead replace that abrupt exit with a graceful shutdown by
removing process.exit and rethrowing the error (or awaiting the app close) so
NestJS can clean up; specifically, in the catch for this.resetService.reset()
(the run() method), log the error with console.error('Failed to reset
database:', err) and then rethrow err (or invoke the application's close method)
instead of calling process.exit(1).
In `@apps/api/src/db/cli/commands/db-seed.command.ts`:
- Around line 19-28: Add a success log at the end of the run() method so
operators get confirmation when seeding completes: after the awaited calls to
this.seederService.seed(), this.seederService.seedAbac(ABAC_PERMISSIONS), and
this.syncSeederService.seedMappings(LOCAL_SEED_MAPPINGS) succeed, call a clear
success logger (e.g., console.log or the class logger) like "Database seed
completed successfully" (optionally include which seeds ran) before returning;
keep the existing error catch intact.
In `@apps/api/src/db/infrastructure/migration-runner.service.ts`:
- Line 21: Add a timeout option to the execAsync invocations so the process
can't hang indefinitely: update the execAsync call that runs 'npx drizzle-kit
migrate' and the execAsync inside migrateTenant to include a timeout (e.g.,
timeout in milliseconds) in the options object passed to execAsync, keeping the
other options intact; reference the execAsync calls and the migrateTenant
function to locate and apply the same timeout value to both places.
- Line 12: The constructor of MigrationRunnerService injects an unused
PgConnectionPool (constructor(private readonly connectionPool: PgConnectionPool)
{}) — remove the unused dependency and its import to eliminate unnecessary
coupling: delete the PgConnectionPool parameter from the constructor signature
(and remove any corresponding private property), and remove the PgConnectionPool
import statement so the class no longer references that symbol.
In `@apps/api/src/db/services/connection-schema-provisioner.service.ts`:
- Around line 153-203: migrateAllSchemas is acquiring a second Postgres client
via connectionPool.getPgClient() inside the withSchemaMgr callback (creating the
local client variable) which duplicates connections and risks leaks on early
returns; modify migrateAllSchemas to reuse the client/connection that
withSchemaMgr already provides (or change withSchemaMgr to expose the underlying
pg client) instead of calling connectionPool.getPgClient(), remove the nested
client.end() cleanup (only close the client if this function actually created
it), and update references to use the already-open client/DB handle (symbols:
migrateAllSchemas, withSchemaMgr, connectionPool.getPgClient, client) so only a
single connection is used and properly closed by the owner.
- Line 30: The variable createdPools is incorrectly typed as typeof
Pool.prototype[] (which becomes Function[]); change its annotation to an array
of Pool instances (e.g., Pool[] or Array<Pool>) so calls like
createdPools.forEach(p => p.end()) are type-safe; ensure the Pool type is
imported from 'pg' (or the correct PG client) and update the declaration of
createdPools in connection-schema-provisioner.service.ts accordingly.
In `@apps/api/src/db/services/dev-sandbox-provisioner.service.ts`:
- Around line 244-268: The cache logic in the resolver is recreating pools
instead of reusing them; inside the lambda that checks tenantPoolCache for
cacheKey, when a cached entry exists (cached variable), stop closing and
recreating pools and simply return cached.drizzle; otherwise create a new Pool
and drizzle instance, set tenantPoolCache with that object. In other words,
remove the branch that calls old.pool.end() and constructs a new Pool/Drizzle
when cached is truthy and ensure tenantPoolCache only gets updated on creation
(use Pool and drizzle from new Pool only when cached is falsy).
In `@apps/api/src/db/services/rbac-inspector.service.ts`:
- Around line 15-18: The constructor of RbacInspectorService injects an unused
dependency environmentGuard of type EnvironmentGuardService; remove the
environmentGuard parameter from the constructor signature, delete the
corresponding EnvironmentGuardService import, and update any constructor
calls/instantiations (if present) so the service only takes PgConnectionPool;
ensure there are no references to environmentGuard elsewhere in the file and
run/adjust tests or DI registration if needed.
In `@apps/api/src/modules/connections/connections/credential.controller.ts`:
- Line 178: Remove the dynamic import of drizzle-orm inside the request handler:
eliminate the "const { isNotNull, sql } = await import('drizzle-orm');" line,
move any needed static imports to the module top (note that sql is already
statically imported elsewhere) and drop the unused isNotNull symbol entirely;
update the handler to use the existing top-level sql import so no per-request
dynamic import occurs.
In `@apps/api/src/modules/scheduler/infrastructure/fetch-http.client.ts`:
- Around line 15-27: The code mutates the caller's headers by assigning const
headers = options?.headers and then setting headers['Content-Type']; instead
make a shallow copy of the incoming headers before modification (e.g., create a
new Record from options?.headers or use a new Headers instance) and use that
copy when building fetchOptions; ensure the Content-Type check remains
case-insensitive (check both 'Content-Type' and 'content-type' keys) and assign
the header to the copied headers object so options.headers is never modified
(affects the headers variable and fetchOptions in this file).
In `@apps/api/src/modules/trigger/dlq-processor.service.spec.ts`:
- Around line 237-239: Remove the stray typo property on the failingExecutor
mock: delete the unused property named run反Poll and keep the correct runPoll
mock; update the failingExecutor object (the mock used in
dlq-processor.service.spec.ts) so only runPoll is defined (mockRejectedValue(new
Error('permanent fail'))), ensuring no other tests rely on the removed run反Poll
key.
In `@apps/api/src/modules/trigger/dlq-processor.service.ts`:
- Line 13: The inline comment "// Replaced by RedisTriggerDlqService
implementation" in the dlq-processor.service.ts file is now orphaned after
refactoring; either remove the comment entirely or replace it with a clear doc
comment indicating the relationship to RedisTriggerDlqService (e.g., note that
DlqProcessorService was deprecated and functionality moved to
RedisTriggerDlqService), and ensure any references to
DlqProcessorService/DlqProcessorService class name in the file/header match that
explanation so readers know why the file remains or can be safely deleted.
In `@apps/api/src/modules/trigger/infrastructure/redis-trigger-dlq.service.ts`:
- Around line 46-78: The reclaimStaleJobs method performs up to batchSize
separate redis.eval calls (using LUA_RECLAIM, DLQ_PROCESSING_KEY, DLQ_KEY) which
is inefficient; replace the per-iteration calls with a single EVAL that accepts
batchSize (e.g. ARGV[2]) and loops inside the Lua script to atomically reclaim
up to N stale items, returning the number reclaimed, then call this.redis.eval
once from reclaimStaleJobs and use the returned count instead of the reclaimed++
loop.
In `@apps/api/src/modules/trigger/trigger-executor.service.ts`:
- Line 63: The injection token 'REDIS_CLIENT' is misleading for the
IKeyValueStore dependency; update the injection to use an abstraction-aligned
token (e.g., 'KEY_VALUE_STORE' or a Symbol like IKeyValueStore) and update all
providers and consumers to match. Locate the constructor parameter in
TriggerExecutorService (the `@Inject`('REDIS_CLIENT') private readonly kvStore:
IKeyValueStore) and change the token to the chosen new token, then update the
provider registration that currently binds REDIS_CLIENT to the Redis
implementation (and any other files referencing 'REDIS_CLIENT') so they provide
the new token and still return the Redis-backed IKeyValueStore used by
KeyValueTriggerStore. Ensure imports and any tests referencing the token are
updated accordingly.
In `@apps/api/src/modules/trigger/trigger-payload-transformer.ts`:
- Around line 9-17: The return value in extractRecordCursor uses r[key] which
TypeScript still types as unknown despite the runtime typeof check; update the
return to assert the narrowed type (e.g., return r[key] as string) so the
function signature string is satisfied, and ensure this change is applied inside
the for-loop where r and key are used.
In `@apps/web/vitest.config.ts`:
- Around line 27-32: Update the coverage thresholds in the thresholds object
(keys: statements, branches, functions, lines) from 10 to 80 and replace the
inline TODO with a clear reference to a created tracker ticket (e.g., add the
issue ID or short URL in the comment) so the plan to raise thresholds is
tracked; after changing the values in vitest.config.ts, run the test suite to
ensure the new 80% thresholds are satisfied or add missing tests to meet them.
In `@apps/worker/src/modules/pipeline/delivery.service.ts`:
- Around line 47-49: There is a circular dependency between DeliveryService and
DeliveryRetryService (the forwardRef on DeliveryRetryService). Fix it by
extracting the shared callback logic (e.g., writeL6Result) into a new injectable
service (suggest name: L6ResultWriterService) and have both DeliveryService and
DeliveryRetryService depend on that instead; move the implementation of
writeL6Result from DeliveryService into L6ResultWriterService, update all calls
in DeliveryService and DeliveryRetryService to use the new service, inject
L6ResultWriterService into both constructors, and then remove the forwardRef and
the DeliveryRetryService dependency cycle.
In `@apps/worker/src/modules/pipeline/fanout-batch-processor.ts`:
- Around line 228-233: The catch block that catches errors loading destState
from the replica entity currently logs a misleading message about publishing;
update the this.logger.error call (the one passing sanitizeErrorObject(err),
traceId, routeId: stitch.id and sanitizeError(err)) to reflect the actual
failure (e.g., "failed to load destState from replica entity" or similar) so it
correctly identifies that the error occurred while loading destState from the
replica rather than publishing the stitch routing envelope.
- Around line 314-342: Move the initial schema resolution for destSchemaName out
of the try block so the same resolved value is reused in the catch path instead
of calling storageResolver.resolveSchemaName again; ensure destSchemaName is
declared in the outer scope (near where stitch and traceId are available) and
used both inside the try and in the catch's tenantDb.transaction block that
updates outbound_gateway, removing the duplicate resolve call in the catch and
leaving the error logging and transaction/update logic intact.
In `@apps/worker/src/modules/pipeline/gem-hydration.service.ts`:
- Around line 10-23: The writeGemMapping function signature has too many
positional parameters (12) which harms readability and maintainability; refactor
it to accept a single parameter object: create an interface (e.g.,
GemMappingParams) containing traceId, routeId, srcAppName, dataSourceId,
srcTenantId, canonicalType, srcVendorId, targetAppName, targetConnectionId,
targetTenantId, destVendorId and change writeGemMapping(tenantDb: DrizzleDb,
...params) to writeGemMapping(tenantDb: DrizzleDb, params: GemMappingParams);
update all internal uses inside writeGemMapping to read from
params.<propertyName> and adjust all call sites to pass an object instead of
positional args. Ensure types and imports are updated accordingly.
In `@apps/worker/vitest.config.mts`:
- Around line 58-63: The lowered coverage thresholds in the thresholds object
(statements/branches/functions/lines set to 50) were temporary; create a tracked
task to restore them to 80% after the TDD refactor completes and update the TODO
to reference that task (e.g., replace the inline TODO with the issue/epic ID and
brief target: "Restore thresholds to 80% when refactor done"). Ensure the task
references the vitest thresholds change and the thresholds symbol so reviewers
can close the loop when the refactor is finished.
In `@engine/sync/application/salesforce/revenova/replicate.js`:
- Around line 107-108: The current implementation builds a results array but
returns only the first element; update the function in replicate.js to preserve
batched results: either change the final return to return the full results array
(return results) to surface all processed notifications, or if you must keep
backward compatibility, add a clear warning log when results.length > 1 (e.g.,
processLogger.warn or logger.warn) to indicate data was dropped and then
continue returning results[0]; modify the single return statement accordingly
and ensure any calling code/docs are updated to reflect the chosen behavior.
In `@engine/sync/application/salesforce/revenova/replicate.ts`:
- Around line 74-76: The code currently only takes the first item when
notifications['Notification'] is an array (const notification =
Array.isArray(notifications['Notification']) ? notifications['Notification'][0]
: notifications['Notification'];), which drops additional notifications; either
(A) update the pipeline to support batches by changing ReplicateRevenovaObject
to return an array of replicas and modify the extractor/normalizer/write code
paths to accept and iterate over multiple replicas (i.e., return Notification[]
-> map each notification to { entityType, entityId, data } and propagate the
array), or (B) explicitly enforce the single-notification assumption by adding a
clear guard/validation and comment (e.g., throw or log an error when
Array.isArray(...) && length > 1) so callers know multiple notifications are not
supported; pick one approach and make corresponding changes to the functions
that consume ReplicateRevenovaObject to keep types and behavior consistent.
In `@packages/credentials/src/oauth/redis-lock.ts`:
- Line 2: Extract the IDistributedLock interface out of token-manager.service.js
into a new dedicated interface file (e.g., distributed-lock.interface.ts) and
update references: create the interface file exporting IDistributedLock, import
IDistributedLock in redis-lock.ts (replacing the import from
token-manager.service.js), and update token-manager.service.js (and any other
files) to import IDistributedLock from the new interface file so the lock
abstraction is decoupled from the service layer.
In `@packages/database/src/savepoint/savepoint.manager.ts`:
- Around line 31-39: The validateSavepointName function currently allows names
starting with digits; update its validation to enforce SQL identifier
conventions by requiring the first character be a letter (a-z or A-Z) or
underscore and subsequent characters be alphanumeric or underscores (adjust the
regex used in validateSavepointName and update the thrown error text accordingly
to reflect the stricter rule).
In `@packages/dbmanager/src/cli/interfaces.ts`:
- Around line 8-14: Update the IMigrationRunner interface to document that
migrate() and migrateGlobal() are synchronous/blocking and throw on failure: add
a short JSDoc comment above the IMigrationRunner declaration and above the
migrate() and migrateGlobal() method signatures clarifying they return void
because they perform synchronous/blocking work (e.g., call execSync in
database-manager.ts) and will throw synchronously on error; reference the
existing implementations (migrate in apps/api/src/db/database-manager.ts and
calls in apps/api/src/db/db-cli.ts) in the comment for clarity.
In `@packages/identity/src/adapters/better-auth.abac.spec.ts`:
- Line 67: Replace the unsafe "as any" cast on the mock event publisher with a
proper IIdentityEventPublisher-typed mock: create or use a typed mock helper
that implements IIdentityEventPublisher and set publishTenantProvisioned and
publishUserInvited to vi.fn() on that typed object (referencing the mock object
containing publishTenantProvisioned and publishUserInvited and the
IIdentityEventPublisher interface) so the spec uses real types instead of "as
any".
In `@packages/identity/src/adapters/better-auth.adapter.spec.ts`:
- Around line 117-120: The mkPublisher helper currently returns an untyped
object; change its signature to return IIdentityEventPublisher and ensure its
returned object implements publishTenantProvisioned and publishUserInvited as
vi.fn().mockResolvedValue(undefined) so callers no longer need to cast to any;
update the mkPublisher declaration to have the explicit return type
IIdentityEventPublisher and keep the two mocked methods
(publishTenantProvisioned, publishUserInvited) so all `as any` uses can be
removed.
---
Outside diff comments:
In `@apps/api/package.json`:
- Around line 21-31: Update the inconsistent npm scripts so they all use the new
CLI entrypoint: change the "db:drop", "db:provision:local",
"db:provision:gateway", "db:migrate:schemas", and "db:check-role" scripts to
invoke "src/db/cli/main.ts" (same as
"db:seed","db:fresh","db:reset","db:migrate","db:provision","db:debug") or, if
any must remain on "src/db/db-cli.ts", add a comment explaining why and keep a
shim that forwards to the new main CLI; ensure you update the script targets for
those exact script names so they won't break when src/db/db-cli.ts is removed.
In `@apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts`:
- Around line 361-375: Add a test to cover the failure path in
system-admin.controller.spec.ts: mock mockUserProvider.findById to return the
user id ('u1') and mock mockUserProvider.deleteIfNotLastAdmin to resolve to {
success: false }, then call controller.deleteUser('u1') and assert it
rejects/throws a BadRequestException (use
expect(...).rejects.toThrow(BadRequestException) or equivalent). Ensure the
expectation also verifies deleteIfNotLastAdmin was called with 'u1' and
getRequiredSystemTenantId() to match the success test pattern.
In `@apps/api/src/modules/trigger/key-value-trigger-store.ts`:
- Around line 4-20: The doc comment for the Redis-backed TriggerStore is out of
date now that the implementation uses the generic IKeyValueStore; update the
header in key-value-trigger-store.ts to describe a generic key-value backed
TriggerStore (mentioning IKeyValueStore) and remove Redis-specific details
(Redis Hash, Redis key prefixes). Keep the explanation of the fully-qualified
cursor key shape and namespace separation but phrase them as logical key
namespaces used by the key-value store (e.g.,
cursor:{workspaceId}:{appName}:{objectType}:{triggerName}, rl:..., token:...,
dlq:..., lock:...) and note that the underlying storage is abstracted by
IKeyValueStore rather than tied to Redis.
In `@apps/api/src/modules/trigger/trigger-executor.service.spec.ts`:
- Around line 237-240: In the test setup inside
trigger-executor.service.spec.ts, remove the stray/typo property named
"run反Poll" from the mock object (the block that also contains auth, propsValue,
workspaceId, dataSourceId) because only "runPoll" is used; ensure the mock only
defines "runPoll" and not the unintended "run反Poll" so tests reference the
correct property.
In `@apps/worker/src/modules/pipeline/inbound-outbox.poller.ts`:
- Around line 194-230: The warning log uses the old attempt count (row.attempts)
instead of the incremented value; update the logger.warn call in
inbound-outbox.poller.ts to interpolate incrementedAttempts (the variable set to
row.attempts + 1) so the message shows the correct attempt number for
traceId=row.traceId and keep the surrounding condition and DB update that set
attempts to incrementedAttempts unchanged.
In `@packages/credentials/src/oauth/token-manager.service.ts`:
- Around line 151-158: The finally block in refresh handling should not let
releaseLock errors mask the original refresh error: modify the block around
performTokenRefresh/handleRefreshError to call releaseLock(lockKey, lockValue)
inside its own try/catch (or try/catch/finally) and log or swallow releaseLock
errors while preserving and rethrowing the original error thrown by
performTokenRefresh/handleRefreshError; reference the existing methods
performTokenRefresh, handleRefreshError and releaseLock and ensure the original
error is rethrown after releaseLock is attempted.
---
Duplicate comments:
In `@apps/api/src/db/services/tenant-schema.service.ts`:
- Line 102: Replace the direct string interpolation in the adminClient.query
call that runs CREATE DATABASE by using pg-format to safely escape the
identifier (e.g. use format('CREATE DATABASE %I', dbName)); import format from
'pg-format' if not already present and mirror the approach used for DROP
DATABASE so adminClient.query uses format(...) instead of `"${dbName}"`,
referencing the existing adminClient.query call and the dbName variable.
In `@apps/worker/src/modules/pipeline/delivery-retry.service.ts`:
- Around line 94-101: The SELECT that builds the existingResult uses
tx.select({... response, statusCode ...}) from outboundGateway but omits
destVendorId, so existingResult[0].destVendorId is always undefined; update the
select object used in the transaction (the call that references
outboundGateway.response and outboundGateway.statusCode) to also include
destVendorId (e.g., add destVendorId: outboundGateway.destVendorId) so the
replay logic can read existingResult[0].destVendorId correctly and adjust any
related types/aliases if necessary.
In `@apps/worker/src/modules/pipeline/delivery.service.spec.ts`:
- Around line 90-101: The test currently mocks DeliveryRetryService
(handleRetryAndDelay, isSourceFinalized, retrySourceFinalization) so the new
transactional search_path and replay-query logic in DeliveryRetryService isn't
exercised; add a new unit test file delivery-retry.service.spec.ts that imports
the real DeliveryRetryService and its dependencies (or alternatively replace the
mocked provider in one existing integration-style test with the real
DeliveryRetryService) and write tests targeting handleRetryAndDelay and
retrySourceFinalization flows to cover the transactional search_path/replay
behavior, stubbing only external DB or network calls as needed so the
transactional logic runs in tests.
In `@packages/identity/src/identity.module.ts`:
- Around line 102-105: The provider for IDENTITY_EVENT_PUBLISHER creates a
self-referential circular dependency when options.eventPublisherToken is
undefined; change the provider so that if options.eventPublisherToken is present
it uses useExisting: options.eventPublisherToken, otherwise it provides a
concrete default by using useClass: IdentityEventPublisher (same pattern as
registerAsync()). Update the provider entry that references
IDENTITY_EVENT_PUBLISHER and options.eventPublisherToken to conditionally choose
useExisting vs useClass IdentityEventPublisher so the token is never bound to
itself.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c55619e-aed1-484b-bd9f-98924919f22f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (144)
apps/api/package.jsonapps/api/src/app/app.module.tsapps/api/src/db/cli/cli.module.tsapps/api/src/db/cli/commands/db-debug.command.tsapps/api/src/db/cli/commands/db-fresh.command.tsapps/api/src/db/cli/commands/db-migrate.command.tsapps/api/src/db/cli/commands/db-provision.command.tsapps/api/src/db/cli/commands/db-reset.command.tsapps/api/src/db/cli/commands/db-seed.command.tsapps/api/src/db/cli/main.tsapps/api/src/db/data/seed-abac.tsapps/api/src/db/data/seed-mappings.tsapps/api/src/db/database-manager.spec.tsapps/api/src/db/database-manager.tsapps/api/src/db/infrastructure/migration-runner.service.tsapps/api/src/db/infrastructure/pg-connection.pool.tsapps/api/src/db/services/connection-schema-provisioner.service.tsapps/api/src/db/services/database-reset.service.tsapps/api/src/db/services/dev-sandbox-provisioner.service.tsapps/api/src/db/services/environment-guard.service.tsapps/api/src/db/services/rbac-inspector.service.tsapps/api/src/db/services/sync-seeding.service.tsapps/api/src/db/services/system-seeder.service.tsapps/api/src/db/services/tenant-schema.service.tsapps/api/src/modules/connections/connection-lifecycle.service.spec.tsapps/api/src/modules/connections/connection-lifecycle.service.tsapps/api/src/modules/connections/connections.module.tsapps/api/src/modules/connections/connections/connectors.controller.spec.tsapps/api/src/modules/connections/connections/credential.controller.tsapps/api/src/modules/connections/connections/oauth.controller.tsapps/api/src/modules/connections/connectors.service.spec.tsapps/api/src/modules/connections/connectors.service.tsapps/api/src/modules/connections/events/connection-paused.event.tsapps/api/src/modules/connections/events/connection-schema-provisioned.event.tsapps/api/src/modules/connections/oauth-url-builder.spec.tsapps/api/src/modules/connections/oauth-url-builder.tsapps/api/src/modules/connections/services/credential-linking.service.tsapps/api/src/modules/connections/services/oauth-orchestration.service.tsapps/api/src/modules/exceptions/exception.service.spec.tsapps/api/src/modules/exceptions/exception.service.tsapps/api/src/modules/exceptions/exceptions.module.tsapps/api/src/modules/exceptions/infrastructure/soopa-delivery-queue-dispatcher.service.tsapps/api/src/modules/exceptions/interfaces/delivery-queue-dispatcher.interface.tsapps/api/src/modules/identity/auth/auth.controller.tsapps/api/src/modules/identity/system-admin/system-admin.controller.spec.tsapps/api/src/modules/identity/system-admin/system-admin.controller.tsapps/api/src/modules/pipeline/pipeline-cdc.listener.tsapps/api/src/modules/pipeline/pipeline.module.tsapps/api/src/modules/scheduler/infrastructure/fetch-http.client.tsapps/api/src/modules/scheduler/infrastructure/stub-scheduler.client.spec.tsapps/api/src/modules/scheduler/infrastructure/stub-scheduler.client.tsapps/api/src/modules/scheduler/infrastructure/windmill-scheduler.client.spec.tsapps/api/src/modules/scheduler/infrastructure/windmill-scheduler.client.tsapps/api/src/modules/scheduler/interfaces/http-client.interface.tsapps/api/src/modules/scheduler/interfaces/scheduler-client.interface.tsapps/api/src/modules/scheduler/scheduler.module.tsapps/api/src/modules/scheduler/scheduler.service.spec.tsapps/api/src/modules/scheduler/scheduler.service.tsapps/api/src/modules/scheduler/windmill.client.tsapps/api/src/modules/trigger/dlq-processor.service.spec.tsapps/api/src/modules/trigger/dlq-processor.service.tsapps/api/src/modules/trigger/infrastructure/redis-distributed-lock.service.tsapps/api/src/modules/trigger/infrastructure/redis-trigger-dlq.service.tsapps/api/src/modules/trigger/interfaces/distributed-lock.interface.tsapps/api/src/modules/trigger/interfaces/trigger-dlq.interface.tsapps/api/src/modules/trigger/key-value-trigger-store.spec.tsapps/api/src/modules/trigger/key-value-trigger-store.tsapps/api/src/modules/trigger/trigger-executor.service.spec.tsapps/api/src/modules/trigger/trigger-executor.service.tsapps/api/src/modules/trigger/trigger-payload-transformer.spec.tsapps/api/src/modules/trigger/trigger-payload-transformer.tsapps/api/src/modules/trigger/trigger-retry-policy.service.spec.tsapps/api/src/modules/trigger/trigger-retry-policy.service.tsapps/api/src/modules/trigger/trigger.module.tsapps/api/src/scripts/admin-bootstrap.tsapps/api/src/scripts/test-e2e-ingestion.tsapps/api/vitest.config.mtsapps/tenant-provisioner/vitest.config.tsapps/web/src/app/providers/data-provider.tsapps/web/src/test/environments/jsdom-msw.tsapps/web/vitest.config.tsapps/worker/src/db/database-manager.spec.tsapps/worker/src/db/database-manager.tsapps/worker/src/modules/pipeline/delivery-retry.service.tsapps/worker/src/modules/pipeline/delivery.service.spec.tsapps/worker/src/modules/pipeline/delivery.service.tsapps/worker/src/modules/pipeline/fanout-batch-processor.tsapps/worker/src/modules/pipeline/fanout-router.service.tsapps/worker/src/modules/pipeline/gem-hydration.service.tsapps/worker/src/modules/pipeline/inbound-outbox.poller.tsapps/worker/src/modules/pipeline/pipeline.module.tsapps/worker/src/modules/pipeline/registry-replication.service.tsapps/worker/vitest.config.mtsengine/ai/core/vitest.config.tsengine/sync/application/mapping/package.jsonengine/sync/application/mapping/src/mapping-engine.tsengine/sync/application/salesforce/revenova/replicate.jsengine/sync/application/salesforce/revenova/replicate.tsengine/sync/platform/core/src/sharding/pipeline-hook-broker.service.spec.tsengine/sync/platform/core/vitest.config.tspackages/auth/src/services/auth.service.tspackages/cache/src/index.tspackages/cache/src/key-value-store.interface.tspackages/credentials/package.jsonpackages/credentials/src/events/credential-deleted.event.tspackages/credentials/src/events/credential-invalidated.event.tspackages/credentials/src/events/credential-refreshed.event.tspackages/credentials/src/events/index.tspackages/credentials/src/index.tspackages/credentials/src/interfaces/event-publisher.interface.tspackages/credentials/src/interfaces/index.tspackages/credentials/src/oauth/redis-lock.tspackages/credentials/src/oauth/token-manager.service.spec.tspackages/credentials/src/oauth/token-manager.service.tspackages/credentials/src/oauth/token-refresh.service.spec.tspackages/credentials/src/oauth/token-refresh.service.tspackages/credentials/src/services/credentials-event-publisher.service.tspackages/credentials/vitest.config.tspackages/database/src/database.module.tspackages/database/src/index.tspackages/database/src/savepoint/index.tspackages/database/src/savepoint/savepoint.manager.tspackages/database/src/schema/global/credentials.tspackages/database/src/schema/global/data-sources.tspackages/dbmanager/src/cli/interfaces.tspackages/dbmanager/src/impl/tenant-database-manager.tspackages/dbmanager/src/index.tspackages/identity/package.jsonpackages/identity/src/adapters/better-auth.abac.spec.tspackages/identity/src/adapters/better-auth.adapter.spec.tspackages/identity/src/adapters/better-auth.adapter.tspackages/identity/src/adapters/drizzle-tenant.adapter.spec.tspackages/identity/src/adapters/drizzle-tenant.adapter.tspackages/identity/src/constants.tspackages/identity/src/events/index.tspackages/identity/src/events/tenant-provisioned.event.tspackages/identity/src/events/user-invited.event.tspackages/identity/src/identity.module.tspackages/identity/src/interfaces/auth-provider.interface.tspackages/identity/src/interfaces/event-publisher.interface.tspackages/identity/src/interfaces/index.tspackages/identity/src/services/identity-event-publisher.service.spec.tspackages/identity/src/services/identity-event-publisher.service.tspackages/identity/src/services/permission-seeder.spec.ts
💤 Files with no reviewable changes (3)
- apps/worker/src/db/database-manager.spec.ts
- apps/worker/src/db/database-manager.ts
- apps/api/src/modules/scheduler/windmill.client.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 31 file(s) based on 29 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 31 file(s) based on 29 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/api/src/db/cli/commands/db-seed.command.ts (1)
25-28:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid hard process termination in command error handling.
On Line 27,
process.exit(1)short-circuits command teardown and is inconsistent withDbFreshCommand/DbResetCommand, which rethrow after logging. Keep the error log and rethrow so bootstrap handles termination.Suggested fix
} catch (err) { console.error('Failed to seed database:', err); - process.exit(1); + throw err; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/db/cli/commands/db-seed.command.ts` around lines 25 - 28, In the catch block of the DbSeedCommand (the error handler inside the command's run/execute method), remove the hard process.exit(1) and instead log the error as currently done and then rethrow the error so bootstrap or the caller can handle termination consistently with DbFreshCommand/DbResetCommand; keep the console.error('Failed to seed database:', err) call and follow it with a throw err to propagate the failure.apps/api/src/db/services/dev-sandbox-provisioner.service.ts (1)
15-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClass should implement
OnModuleDestroyinterface for lifecycle hook to be called.The
onModuleDestroy()method exists but the class doesn't implement theOnModuleDestroyinterface. While NestJS may still call the hook via duck typing, explicitly implementing the interface ensures type safety and makes the contract clear.🔧 Proposed fix
-import { Injectable } from '`@nestjs/common`'; +import { Injectable, OnModuleDestroy } from '`@nestjs/common`'; `@Injectable`() -export class DevSandboxProvisionerService { +export class DevSandboxProvisionerService implements OnModuleDestroy {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/db/services/dev-sandbox-provisioner.service.ts` around lines 15 - 35, Add the Nest lifecycle interface to the class declaration and import it: update DevSandboxProvisionerService to implement OnModuleDestroy and add the corresponding import from '`@nestjs/common`', keeping the existing onModuleDestroy() implementation as-is so the lifecycle hook is explicitly typed and enforced.packages/identity/src/adapters/better-auth.adapter.spec.ts (1)
415-419: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRemove unnecessary
as anycast onpublisher.Line 419 casts
mkPublisher() as anybutmkPublisher()already returnsIIdentityEventPublisher. This is inconsistent with other call sites in the file (lines 140, 151, 159, etc.) that usemkPublisher()without casting.♻️ Proposed fix
- const adapter = new BetterAuthAdapter(db, email as any, cfg(), mkTenantProvider() as any, mkOptions(), publisher as any); + const adapter = new BetterAuthAdapter(db, email as any, cfg(), mkTenantProvider() as any, mkOptions(), publisher);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/identity/src/adapters/better-auth.adapter.spec.ts` around lines 415 - 419, In the test that constructs BetterAuthAdapter, remove the unnecessary "as any" cast on the publisher argument: replace mkPublisher() as any with mkPublisher() so the call to the BetterAuthAdapter constructor uses the actual IIdentityEventPublisher type; this matches other usages (e.g., earlier tests) and keeps the constructor invocation (BetterAuthAdapter(..., mkPublisher(), ...)) consistent and type-safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/credentials/src/oauth/redis-lock.ts`:
- Line 2: The code imports IDistributedLock from ./distributed-lock.interface.js
but no such module exists; create a new file
packages/credentials/src/oauth/distributed-lock.interface.ts that exports the
IDistributedLock interface (matching the shape used by redis-lock.ts and
token-manager.service.ts), or alternatively update the imports in redis-lock.ts
and token-manager.service.ts to point to the existing interface (e.g., the API's
IDistributedLockService) and rename/adjust references accordingly; ensure the
symbol IDistributedLock is exported and its methods match usages in RedisLock
(class/function names: RedisLock, TokenManagerService) so TypeScript resolution
error TS2307 is resolved.
In `@packages/credentials/src/oauth/token-manager.service.ts`:
- Line 7: Create a new local interface file exporting IDistributedLock that
matches the method signatures used by RedisDistributedLock (the acquire/release
shape) and update the imports in TokenManagerService and RedisDistributedLock to
import from this new distributed-lock.interface instead of the missing
./distributed-lock.interface.js; specifically define and export IDistributedLock
with the exact acquire(...) and release(...) async signatures implemented by
class RedisDistributedLock so TokenManagerService and RedisDistributedLock
compile against the same type.
---
Outside diff comments:
In `@apps/api/src/db/cli/commands/db-seed.command.ts`:
- Around line 25-28: In the catch block of the DbSeedCommand (the error handler
inside the command's run/execute method), remove the hard process.exit(1) and
instead log the error as currently done and then rethrow the error so bootstrap
or the caller can handle termination consistently with
DbFreshCommand/DbResetCommand; keep the console.error('Failed to seed
database:', err) call and follow it with a throw err to propagate the failure.
In `@apps/api/src/db/services/dev-sandbox-provisioner.service.ts`:
- Around line 15-35: Add the Nest lifecycle interface to the class declaration
and import it: update DevSandboxProvisionerService to implement OnModuleDestroy
and add the corresponding import from '`@nestjs/common`', keeping the existing
onModuleDestroy() implementation as-is so the lifecycle hook is explicitly typed
and enforced.
In `@packages/identity/src/adapters/better-auth.adapter.spec.ts`:
- Around line 415-419: In the test that constructs BetterAuthAdapter, remove the
unnecessary "as any" cast on the publisher argument: replace mkPublisher() as
any with mkPublisher() so the call to the BetterAuthAdapter constructor uses the
actual IIdentityEventPublisher type; this matches other usages (e.g., earlier
tests) and keeps the constructor invocation (BetterAuthAdapter(...,
mkPublisher(), ...)) consistent and type-safe.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e60cd2c6-a6ef-41cd-b083-f6cc37fa38ec
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
apps/api/src/db/cli/commands/db-fresh.command.tsapps/api/src/db/cli/commands/db-reset.command.tsapps/api/src/db/cli/commands/db-seed.command.tsapps/api/src/db/infrastructure/migration-runner.service.tsapps/api/src/db/services/connection-schema-provisioner.service.tsapps/api/src/db/services/dev-sandbox-provisioner.service.tsapps/api/src/db/services/rbac-inspector.service.tsapps/api/src/modules/connections/connections/credential.controller.tsapps/api/src/modules/scheduler/infrastructure/fetch-http.client.tsapps/api/src/modules/trigger/dlq-processor.service.spec.tsapps/api/src/modules/trigger/dlq-processor.service.tsapps/api/src/modules/trigger/infrastructure/redis-trigger-dlq.service.tsapps/api/src/modules/trigger/trigger-executor.service.tsapps/api/src/modules/trigger/trigger-payload-transformer.tsapps/web/vitest.config.tsapps/worker/src/modules/pipeline/delivery.service.tsapps/worker/src/modules/pipeline/fanout-batch-processor.tsapps/worker/src/modules/pipeline/gem-hydration.service.tsapps/worker/vitest.config.mtsengine/sync/application/salesforce/revenova/replicate.jsengine/sync/application/salesforce/revenova/replicate.tspackages/cache/package.jsonpackages/cache/src/cache.module.tspackages/cache/tsconfig.jsonpackages/credentials/src/oauth/redis-lock.tspackages/credentials/src/oauth/token-manager.service.tspackages/database/src/savepoint/savepoint.manager.tspackages/dbmanager/src/cli/interfaces.tspackages/identity/src/adapters/better-auth.abac.spec.tspackages/identity/src/adapters/better-auth.adapter.spec.ts
💤 Files with no reviewable changes (3)
- apps/api/src/db/services/rbac-inspector.service.ts
- apps/api/src/modules/trigger/dlq-processor.service.ts
- apps/api/src/modules/trigger/dlq-processor.service.spec.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The agent ran but didn't make any changes. The issues may already be fixed or require manual intervention. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/src/db/services/system-seeder.service.ts (1)
271-275:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore
updatedAtwhen promoting an existing member.Line 272 updates role only;
updatedAtis no longer refreshed. This regresses membership change auditability.Suggested fix
await db .update(schema.member) .set({ role: config.ownerRoleId, + updatedAt: now, }) .where(eq(schema.member.id, existingMember.id));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/db/services/system-seeder.service.ts` around lines 271 - 275, When promoting an existing member the current update call (.update(schema.member).set({ role: config.ownerRoleId }).where(eq(schema.member.id, existingMember.id))) only changes role and does not refresh updatedAt; modify that .set(...) to also set updatedAt (e.g. updatedAt: new Date()) so membership change audit timestamps are preserved when promoting existingMember.packages/identity/src/identity.module.ts (1)
205-209:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
eventPublisherTokeninregisterAsync(currently ignored)
IdentityModule.registerrespectseventPublisherTokenforIDENTITY_EVENT_PUBLISHER(viauseExisting), butIdentityModule.registerAsyncalways bindsIDENTITY_EVENT_PUBLISHERtoIdentityEventPublisher, ignoringidentityOptions.eventPublisherToken. This breaks the configuration contract and can route events to the wrong publisher.Suggested fix
+import { ModuleRef } from "`@nestjs/core`"; ... providers: [ + IdentityEventPublisher, ... - { - provide: IDENTITY_EVENT_PUBLISHER, - useClass: IdentityEventPublisher, - }, + { + provide: IDENTITY_EVENT_PUBLISHER, + useFactory: ( + identityOptions: IdentityModuleOptions, + moduleRef: ModuleRef, + defaultPublisher: IdentityEventPublisher, + ) => { + if (!identityOptions.eventPublisherToken) return defaultPublisher; + return moduleRef.get(identityOptions.eventPublisherToken, { + strict: false, + }); + }, + inject: [IDENTITY_OPTIONS, ModuleRef, IdentityEventPublisher], + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/identity/src/identity.module.ts` around lines 205 - 209, IdentityModule.registerAsync currently ignores identityOptions.eventPublisherToken and always binds IDENTITY_EVENT_PUBLISHER to IdentityEventPublisher; update the provider logic in IdentityModule.registerAsync so that the provider for IDENTITY_EVENT_PUBLISHER checks identityOptions.eventPublisherToken and, if present, uses useExisting: identityOptions.eventPublisherToken (so the module re-uses the externally provided publisher), otherwise fall back to useClass: IdentityEventPublisher; adjust the provider creation near where IDENTITY_EVENT_PUBLISHER is declared so it mirrors the behavior in IdentityModule.register (and ensure the token referenced is the same identityOptions.eventPublisherToken symbol).
♻️ Duplicate comments (4)
apps/api/src/db/services/rbac-inspector.service.ts (1)
26-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate “not found” as command failure.
Line 26 and Line 81 currently log and return, so
db:debugcan exit successfully even when the target role/user does not exist. Throw here so the command path exits non-zero consistently.Suggested fix
if (!role) { - console.error(`❌ Role '${roleName}' not found`); - return; + throw new Error(`Role '${roleName}' not found`); } @@ if (!user) { - console.error(`❌ User '${identifier}' not found`); - return; + throw new Error(`User '${identifier}' not found`); }Also applies to: 81-84
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/db/services/rbac-inspector.service.ts` around lines 26 - 29, Replace the current console.error + return behavior when an entity isn't found with throwing an Error so the CLI exits non‑zero: where the code checks for role (using role and roleName) throw new Error(`Role '${roleName}' not found`) instead of console.error/return, and do the same for the user not‑found branch (referencing userName in the user lookup at the 81-84 block); ensure the thrown Error message matches the existing text for clarity.apps/api/src/db/services/sync-seeding.service.ts (1)
83-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude source connection in stitch identity to prevent mapping collisions.
Line 87 and Line 105 still omit
mapping.sourceDataSourceIdfrom stitch lookup/creation. Different sources can reuse one stitch and overwritefieldMappings.Suggested fix
const stitches = await db .select() .from(schema.integrationStitches) .where( and( + eq( + schema.integrationStitches.sourceDataSourceId, + mapping.sourceDataSourceId, + ), eq( schema.integrationStitches.canonicalObject, mapping.canonicalObject, ), eq( @@ const [newStitch] = await db .insert(schema.integrationStitches) .values({ name: mapping.name, orgId: orgId, workspaceId: workspaceId, + sourceDataSourceId: mapping.sourceDataSourceId, destDataSourceId: mapping.destDataSourceId, canonicalObject: mapping.canonicalObject, targetObject: mapping.targetObject, })Also applies to: 103-112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/db/services/sync-seeding.service.ts` around lines 83 - 99, The stitch lookup and creation must include the source data source to avoid collisions: add mapping.sourceDataSourceId to the query predicates against schema.integrationStitches (alongside canonicalObject, destDataSourceId, workspaceId) and also include sourceDataSourceId when constructing or upserting a stitch (so fieldMappings aren't shared across different sources); update the select .where(...) and the code that creates/updates integrationStitches (references: mapping.sourceDataSourceId, schema.integrationStitches, fieldMappings, workspaceId) to include this additional identity key.apps/web/vitest.config.ts (1)
27-35: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftRe-raising: 10–15% thresholds are too low to serve as an effective gate.
Line 31–34 substantially weaken test-safety for this refactor stream. Please ratchet these back to a meaningful minimum (or gate on changed files) before merge.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/vitest.config.ts` around lines 27 - 35, The coverage thresholds in the vitest configuration (the thresholds object containing statements, branches, functions, lines) are set too low; update those keys (statements, branches, functions, lines) inside the exported Vitest config (where thresholds is defined) to a reasonable minimum (for example 70 or higher) or implement a per-change gating strategy; specifically change the numeric values for statements, branches, functions, and lines in the thresholds object to the new minimums (or replace the flat thresholds with a changed-files gating mechanism) so the PR enforces meaningful coverage checks before merge.apps/api/src/db/services/dev-sandbox-provisioner.service.ts (1)
127-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve DB username even when password is empty.
On Line 128, auth is only included when both username and password exist. For URLs like
postgresql://user@host:5432/db, this dropsuser@and can break connection auth.Suggested fix
- const auth = - parsedUrl.username && parsedUrl.password - ? `${parsedUrl.username}:${parsedUrl.password}@` - : ''; + const auth = parsedUrl.username + ? `${parsedUrl.username}${parsedUrl.password ? `:${parsedUrl.password}` : ''}@` + : '';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/db/services/dev-sandbox-provisioner.service.ts` around lines 127 - 131, The auth construction drops the username when password is empty; update the logic that builds auth (the parsedUrl/hostUrl block) to include parsedUrl.username whenever present (as "username@" if no password, or "username:password@" when both present) instead of requiring both username and password; locate the auth variable assignment and change its condition to check parsedUrl.username first and append parsedUrl.password only when it exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/db/services/tenant-schema.service.ts`:
- Around line 68-69: The dynamic import of "pg-format" in
tenant-schema.service.ts (the const format = (await
import('pg-format')).default;) is suppressed with `@ts-expect-error` but the
package is not declared, which will fail at runtime; add "pg-format" to the
package's dependencies (apps/api workspace/package), run pnpm install to update
pnpm-lock.yaml, and remove the `@ts-expect-error` comments around the import and
the later usage (the two suppression blocks around the format import and its
subsequent usage) so the import is real and type-checked; optionally add/install
typings if needed.
In `@apps/api/src/modules/exceptions/exception.service.ts`:
- Around line 358-406: The code currently starts an unawaited async IIFE that
calls this.queueDispatcher.dispatchRetry (using outboundGatewayId, schemaName,
txResult.*) which causes a fire-and-forget dispatch after you already mark
queued: true; change the flow so dispatchRetry is awaited (or its result
checked) before returning/marking the row PENDING/queued, i.e. remove the
unawaited IIFE and call await this.queueDispatcher.dispatchRetry(...) inside the
same transaction/logic path (or only set status to PENDING after a successful
await), and preserve the existing error handling that sets status to FAIL using
outboundGatewayId and schemaName if dispatchRetry throws.
In `@apps/worker/src/modules/pipeline/delivery.service.ts`:
- Around line 649-670: The GEM mapping is persisted via
gemService.writeGemMapping before the source schema transaction commits, which
can leave dangling GEM records if the source commit fails; either move the
gemService.writeGemMapping call to after the source schema transaction's
successful commit (so it runs only on success), or keep it before but add
compensating logic in the source commit failure path to delete/mark the GEM
record (use tenantDb and traceId/routeId to identify it). Update the code paths
around writeGemMapping and the source commit/rollback handlers so the two
operations are ordered or have a clear compensating rollback using the same
identifiers (traceId, routeId, tenantDb).
In `@packages/database/src/schema/tenant/pipeline.ts`:
- Around line 315-335: Add a missing nextRetryAt timestamp column to the
outbound_outbox table definition (same properties as in other outbox tables:
timestamp with timezone, nullable/default behavior matching peers) and replace
the existing idx_outbound_outbox_claim index with a composite index on (status,
nextRetryAt) that includes the same WHERE clause used by
inbound_outbox/replica_outbox/normalized_outbox for claimable statuses; update
the table schema symbol outboundOutbox and index symbol
idx_outbound_outbox_claim accordingly so the outbox poller can order by
nextRetryAt when claiming.
In `@packages/identity/src/utils/rbac-seeding.spec.ts`:
- Around line 53-67: getExistingRolePermissions currently ignores its _roleIds
argument and always returns all entries from this.existingRolePermissions;
update the function to parse each stored mapping (split on "|") and filter so
only mappings whose roleId exists in the provided _roleIds array are returned,
preserving the existing organizationId "__NULL__" => null conversion; keep the
return shape ({ roleId, permissionId, organizationId }) and Promise.resolve
behavior.
In `@packages/identity/src/utils/rbac-seeding.ts`:
- Around line 113-123: Insert of role-permission rows is not idempotent and can
race; change insertRolePermissions to perform a safe upsert/ignore-on-conflict
instead of a plain insert by using the query builder's "on conflict do nothing"
/ "ignore duplicates" option when inserting into schema.rolePermission (so
duplicate-key errors are suppressed) and apply the same change to the other
insertion block mentioned in the review (the similar role-permission insert
around the later block). Ensure you use the
db.insert(...).values(...).onConflictDoNothing / .onDuplicateKeyIgnore
equivalent for your SQL dialect so concurrent runs become safe.
In `@packages/pieces/platform/registry/src/pieces/migration-worker.service.ts`:
- Around line 130-133: The getActiveTenants function currently declares an
unused parameter pieceName; remove the pieceName parameter from the
getActiveTenants signature and all call sites (update any invocation that passes
a pieceName) to avoid unused-parameter warnings, or if the parameter is intended
for future filtering, add a clear TODO comment inside getActiveTenants
referencing pieceName and use it in a WHERE/filter clause later; update the call
site that currently passes pieceName so it matches the new signature (or keep
passing it if you opt for the TODO approach) — target symbols: getActiveTenants
and its callers.
---
Outside diff comments:
In `@apps/api/src/db/services/system-seeder.service.ts`:
- Around line 271-275: When promoting an existing member the current update call
(.update(schema.member).set({ role: config.ownerRoleId
}).where(eq(schema.member.id, existingMember.id))) only changes role and does
not refresh updatedAt; modify that .set(...) to also set updatedAt (e.g.
updatedAt: new Date()) so membership change audit timestamps are preserved when
promoting existingMember.
In `@packages/identity/src/identity.module.ts`:
- Around line 205-209: IdentityModule.registerAsync currently ignores
identityOptions.eventPublisherToken and always binds IDENTITY_EVENT_PUBLISHER to
IdentityEventPublisher; update the provider logic in
IdentityModule.registerAsync so that the provider for IDENTITY_EVENT_PUBLISHER
checks identityOptions.eventPublisherToken and, if present, uses useExisting:
identityOptions.eventPublisherToken (so the module re-uses the externally
provided publisher), otherwise fall back to useClass: IdentityEventPublisher;
adjust the provider creation near where IDENTITY_EVENT_PUBLISHER is declared so
it mirrors the behavior in IdentityModule.register (and ensure the token
referenced is the same identityOptions.eventPublisherToken symbol).
---
Duplicate comments:
In `@apps/api/src/db/services/dev-sandbox-provisioner.service.ts`:
- Around line 127-131: The auth construction drops the username when password is
empty; update the logic that builds auth (the parsedUrl/hostUrl block) to
include parsedUrl.username whenever present (as "username@" if no password, or
"username:password@" when both present) instead of requiring both username and
password; locate the auth variable assignment and change its condition to check
parsedUrl.username first and append parsedUrl.password only when it exists.
In `@apps/api/src/db/services/rbac-inspector.service.ts`:
- Around line 26-29: Replace the current console.error + return behavior when an
entity isn't found with throwing an Error so the CLI exits non‑zero: where the
code checks for role (using role and roleName) throw new Error(`Role
'${roleName}' not found`) instead of console.error/return, and do the same for
the user not‑found branch (referencing userName in the user lookup at the 81-84
block); ensure the thrown Error message matches the existing text for clarity.
In `@apps/api/src/db/services/sync-seeding.service.ts`:
- Around line 83-99: The stitch lookup and creation must include the source data
source to avoid collisions: add mapping.sourceDataSourceId to the query
predicates against schema.integrationStitches (alongside canonicalObject,
destDataSourceId, workspaceId) and also include sourceDataSourceId when
constructing or upserting a stitch (so fieldMappings aren't shared across
different sources); update the select .where(...) and the code that
creates/updates integrationStitches (references: mapping.sourceDataSourceId,
schema.integrationStitches, fieldMappings, workspaceId) to include this
additional identity key.
In `@apps/web/vitest.config.ts`:
- Around line 27-35: The coverage thresholds in the vitest configuration (the
thresholds object containing statements, branches, functions, lines) are set too
low; update those keys (statements, branches, functions, lines) inside the
exported Vitest config (where thresholds is defined) to a reasonable minimum
(for example 70 or higher) or implement a per-change gating strategy;
specifically change the numeric values for statements, branches, functions, and
lines in the thresholds object to the new minimums (or replace the flat
thresholds with a changed-files gating mechanism) so the PR enforces meaningful
coverage checks before merge.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ced53e83-8471-4c1e-9bec-2bcd38b25dcb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (60)
apps/api/package.jsonapps/api/src/app/app.module.tsapps/api/src/db/cli/commands/db-debug.command.tsapps/api/src/db/cli/commands/db-seed.command.tsapps/api/src/db/services/connection-schema-provisioner.service.tsapps/api/src/db/services/dev-sandbox-provisioner.service.tsapps/api/src/db/services/rbac-inspector.service.tsapps/api/src/db/services/sync-seeding.service.tsapps/api/src/db/services/system-seeder.service.tsapps/api/src/db/services/tenant-schema.service.tsapps/api/src/modules/connections/connection-lifecycle.service.spec.tsapps/api/src/modules/connections/connection-lifecycle.service.tsapps/api/src/modules/connections/connections/connectors.controller.spec.tsapps/api/src/modules/connections/services/credential-linking.service.tsapps/api/src/modules/exceptions/exception.service.spec.tsapps/api/src/modules/exceptions/exception.service.tsapps/api/src/modules/identity/system-admin/system-admin.controller.spec.tsapps/api/src/modules/scheduler/infrastructure/windmill-scheduler.client.spec.tsapps/api/src/modules/trigger/dlq-processor.service.spec.tsapps/api/src/modules/trigger/dlq-processor.service.tsapps/api/src/modules/trigger/key-value-trigger-store.tsapps/api/src/modules/trigger/trigger-payload-transformer.tsapps/api/vitest.config.mtsapps/web/vitest.config.tsapps/worker/src/modules/pipeline/delivery-retry.service.spec.tsapps/worker/src/modules/pipeline/delivery-retry.service.tsapps/worker/src/modules/pipeline/delivery.service.tsapps/worker/src/modules/pipeline/inbound-outbox.poller.tsengine/sync/platform/core/vitest.config.tspackages/credentials/src/oauth/distributed-lock.interface.tspackages/credentials/src/oauth/redis-lock.tspackages/credentials/src/oauth/token-manager.service.tspackages/credentials/src/oauth/token-refresh.service.tspackages/database/src/schema/tenant/pipeline.tspackages/identity/src/adapters/better-auth.abac.spec.tspackages/identity/src/adapters/better-auth.adapter.spec.tspackages/identity/src/adapters/drizzle-tenant.adapter.spec.tspackages/identity/src/identity.module.tspackages/identity/src/services/permission-seeder.spec.tspackages/identity/src/services/permission-seeder.tspackages/identity/src/utils/rbac-seeding.spec.tspackages/identity/src/utils/rbac-seeding.tspackages/identity/src/utils/role-normalization.spec.tspackages/identity/src/utils/role-visibility.spec.tspackages/identity/src/utils/url.util.spec.tspackages/identity/vitest.config.tspackages/infra/src/aws-kms.adapter.spec.tspackages/infra/src/encryption.module.tspackages/infra/src/local-crypto.adapter.spec.tspackages/infra/vitest.config.tspackages/pieces/platform/framework/vitest.config.tspackages/pieces/platform/registry/package.jsonpackages/pieces/platform/registry/src/pieces/migration-worker.service.spec.tspackages/pieces/platform/registry/src/pieces/migration-worker.service.tspackages/pieces/platform/registry/vitest.config.mtspackages/platform/package.jsonpackages/platform/src/index.tspackages/platform/src/path-utils.tspackages/platform/tsconfig.jsonpackages/queue/vitest.config.ts
💤 Files with no reviewable changes (1)
- apps/api/src/modules/exceptions/exception.service.spec.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 9 file(s) based on 7 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 9 file(s) based on 7 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/worker/src/modules/pipeline/delivery.service.ts (1)
657-708:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftA GEM write failure becomes permanently unrecoverable here.
Lines 657-682 commit the source-side
sync_log/lock release before Lines 684-705 attemptwriteGemMapping(). If GEM persistence fails, this method returnsfalseand requeues once, butDeliveryRetryService.isSourceFinalized()only checks for the L6sync_logrow. On redelivery, the duplicate path now treats the source as finalized and skipswriteL6Result(), so the missing GEM linkage is never retried.Either move the L6 finalization marker after a successful GEM write, or extend the retry/finalization path to detect and backfill missing GEM state instead of keying only on
sync_log.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/worker/src/modules/pipeline/delivery.service.ts` around lines 657 - 708, The L6 finalization (inserting into sync_log and deleting activeSyncLocks) currently runs before gemService.writeGemMapping, making GEM failures permanently unrecoverable; fix by moving the sync_log insert and lock-release (the tx.insert(syncLog)... and tx.delete(activeSyncLocks)... that reference syncLog and activeSyncLocks) so they occur only after gemService.writeGemMapping completes successfully, or alternatively update DeliveryRetryService.isSourceFinalized()/writeL6Result logic to detect/backfill missing GEM state on retry (i.e., check for the GEM mapping for traceId/routeId and retry writeGemMapping when absent) so a failed writeGemMapping does not mark the source as finalized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/modules/exceptions/exception.service.ts`:
- Around line 323-405: The outer try currently wraps both the transactional
status update (the tx that sets OUTBOUND_GATEWAY to PENDING) and the dispatch
call, so a ConflictException from the transaction can be caught and treated like
a dispatch failure; to fix, split the logic so the transaction block that
updates outboundGateway (the code using this.db.transaction that returns
txResult and can throw ConflictException) runs without the dispatchRetry() in
its try-catch, then call queueDispatcher.dispatchRetry(...) inside its own
try/catch that only handles dispatch failures: on dispatch failure log the error
and run the rollback transaction to set status = 'FAIL' and errorMessage (as
currently done), rethrow the dispatch error; keep ConflictException unhandled by
that dispatch catch so it propagates normally.
---
Outside diff comments:
In `@apps/worker/src/modules/pipeline/delivery.service.ts`:
- Around line 657-708: The L6 finalization (inserting into sync_log and deleting
activeSyncLocks) currently runs before gemService.writeGemMapping, making GEM
failures permanently unrecoverable; fix by moving the sync_log insert and
lock-release (the tx.insert(syncLog)... and tx.delete(activeSyncLocks)... that
reference syncLog and activeSyncLocks) so they occur only after
gemService.writeGemMapping completes successfully, or alternatively update
DeliveryRetryService.isSourceFinalized()/writeL6Result logic to detect/backfill
missing GEM state on retry (i.e., check for the GEM mapping for traceId/routeId
and retry writeGemMapping when absent) so a failed writeGemMapping does not mark
the source as finalized.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 907be262-d1e1-47b6-bebe-7fcc8ee621f5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/api/package.jsonapps/api/src/db/services/tenant-schema.service.tsapps/api/src/modules/exceptions/exception.service.tsapps/worker/src/modules/pipeline/delivery.service.tspackages/database/src/schema/tenant/pipeline.tspackages/identity/src/utils/rbac-seeding.spec.tspackages/identity/src/utils/rbac-seeding.tspackages/pieces/platform/registry/src/pieces/migration-worker.service.ts
💤 Files with no reviewable changes (1)
- apps/api/src/db/services/tenant-schema.service.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/api/src/db/services/system-seeder.service.ts (1)
466-475:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
sourceDataSourceIdinseedMapping()stitch lookup.Line 487 persists source ID, but the lookup at Lines 466-475 does not filter by it. This can bind mappings to a stitch from a different source connection.
Proposed fix
const stitches = await db .select() .from(schema.integrationStitches) .where( and( eq(schema.integrationStitches.canonicalObject, 'TMS_CARRIER'), + eq( + schema.integrationStitches.sourceDataSourceId, + salesforceConn[0].id, + ), eq(schema.integrationStitches.destDataSourceId, qbConn[0].id), eq(schema.integrationStitches.workspaceId, workspaceId), ), ) .limit(1);Also applies to: 487-487
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/db/services/system-seeder.service.ts` around lines 466 - 475, The stitch lookup in seedMapping() is missing a filter on sourceDataSourceId, so add an equality check for schema.integrationStitches.sourceDataSourceId against the source connection id used earlier (the same value persisted at the later insert) in the .where(...) alongside canonicalObject, destDataSourceId and workspaceId; also ensure the insert/update path that writes sourceDataSourceId uses the same identifier variable so the lookup and persistence match the same source connection for integrationStitches.packages/database/src/schema/global/stitches.ts (1)
86-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce stitch identity uniqueness at DB level.
Current code paths match stitches by workspace/source/destination/canonical object, but there is no corresponding unique constraint. Concurrent inserts can create duplicates and split field mappings.
Proposed fix
uniqueIndex('stitch_name_workspace_unique_idx').on(table.workspaceId, sql`lower(${table.name})`), + uniqueIndex('stitch_identity_unique_idx').on( + table.workspaceId, + table.canonicalObject, + table.sourceDataSourceId, + table.destDataSourceId, + ), index('stitch_workspace_idx').on(table.workspaceId),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/database/src/schema/global/stitches.ts` around lines 86 - 92, The schema lacks a DB-level unique constraint enforcing stitch identity (workspace + source + destination + canonical object), so add a unique index such as uniqueIndex('stitch_identity_unique_idx') on the columns that define identity (table.workspaceId, table.sourceDataSourceId, table.destDataSourceId, table.canonicalObjectId) in packages/database/src/schema/global/stitches.ts (replace or augment the current uniqueIndex('stitch_name_workspace_unique_idx') as appropriate), and create a corresponding migration to apply that unique constraint to prevent concurrent duplicate inserts; ensure the index name and column list match the logic used in stitch matching code paths.packages/pieces/platform/registry/src/pieces/migration-worker.service.spec.ts (1)
16-16: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueType
dbManagerproperly instead of usingany.Using
anybypasses TypeScript's type checking. Consider using a proper mock type or at minimum a partial type.- let dbManager: any; + let dbManager: { getTenantDb: ReturnType<typeof vi.fn> };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pieces/platform/registry/src/pieces/migration-worker.service.spec.ts` at line 16, Replace the loose any on the test variable dbManager with a proper typed mock (e.g., use Partial<DatabaseManager> or jest.Mocked<DatabaseManager>) so TypeScript can check mocked method signatures; update the declaration of dbManager (and any places where you assign mocks) to use that type and adjust mock implementations to satisfy the DatabaseManager interface used by MigrationWorkerService tests (referencing dbManager and MigrationWorkerService in the spec to locate usages).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/db/services/dev-sandbox-provisioner.service.ts`:
- Around line 127-130: The hostUrl being persisted contains credentials because
it's built using parsedUrl.username/password; update the code that constructs
hostUrl in dev-sandbox-provisioner.service.ts so that any persisted registry
entries (tenant_storage_registry, shard_registry) use a sanitized host URL
without auth (omit `${parsedUrl.username}${...}@`), while keeping a separate
credentialed connectionUrl used only for live DB connections; ensure every other
construction site noted (the other occurrences around the same method/flow)
follows the same pattern by referencing the parsedUrl -> hostUrl variable and
replacing it with a sanitizedHost or connectionUrl pair accordingly.
In `@apps/api/src/modules/stitches/stitches.service.ts`:
- Line 127: The patch is missing ownership validation for sourceDataSourceId;
add the same org membership check you used for destDataSourceId in
stitches.service.ts: fetch or look up the data source identified by
sourceDataSourceId and confirm its orgId matches the request organization (or
reuse the existing helper that validates destDataSourceId), and if it does not
belong to the org throw/return the same authorization/validation error before
inserting sourceDataSourceId into the stitch record. Ensure you perform this
check in the same scope where destDataSourceId is validated so both IDs are
verified consistently.
In `@engine/ai/core/vitest.config.ts`:
- Around line 10-15: The coverage thresholds object (thresholds with statements,
branches, functions, lines) is set to 0 which disables coverage gating; update
the vitest coverage configuration to use non-zero baseline thresholds (e.g., set
statements/branches/functions/lines to your project's minimums or derive them
from an env var like COVERAGE_THRESHOLD) so CI fails on regressions, and ensure
the thresholds object in the config (thresholds ->
statements/branches/functions/lines) is enforced rather than left at 0.
In `@packages/database/src/schema/global/stitches.ts`:
- Around line 61-62: The schema defines sourceDataSourceId without the matching
foreign key and index parity that destDataSourceId has; update the stitches
schema to add the same FK constraint and index for sourceDataSourceId as used
for destDataSourceId (i.e., reference the data_sources primary key and create
the same index), and apply the same change in the related block around lines
80-91 where sourceDataSourceId is also declared so both occurrences have
identical FK + index treatment to prevent orphaned references and improve query
performance.
---
Outside diff comments:
In `@apps/api/src/db/services/system-seeder.service.ts`:
- Around line 466-475: The stitch lookup in seedMapping() is missing a filter on
sourceDataSourceId, so add an equality check for
schema.integrationStitches.sourceDataSourceId against the source connection id
used earlier (the same value persisted at the later insert) in the .where(...)
alongside canonicalObject, destDataSourceId and workspaceId; also ensure the
insert/update path that writes sourceDataSourceId uses the same identifier
variable so the lookup and persistence match the same source connection for
integrationStitches.
In `@packages/database/src/schema/global/stitches.ts`:
- Around line 86-92: The schema lacks a DB-level unique constraint enforcing
stitch identity (workspace + source + destination + canonical object), so add a
unique index such as uniqueIndex('stitch_identity_unique_idx') on the columns
that define identity (table.workspaceId, table.sourceDataSourceId,
table.destDataSourceId, table.canonicalObjectId) in
packages/database/src/schema/global/stitches.ts (replace or augment the current
uniqueIndex('stitch_name_workspace_unique_idx') as appropriate), and create a
corresponding migration to apply that unique constraint to prevent concurrent
duplicate inserts; ensure the index name and column list match the logic used in
stitch matching code paths.
In
`@packages/pieces/platform/registry/src/pieces/migration-worker.service.spec.ts`:
- Line 16: Replace the loose any on the test variable dbManager with a proper
typed mock (e.g., use Partial<DatabaseManager> or jest.Mocked<DatabaseManager>)
so TypeScript can check mocked method signatures; update the declaration of
dbManager (and any places where you assign mocks) to use that type and adjust
mock implementations to satisfy the DatabaseManager interface used by
MigrationWorkerService tests (referencing dbManager and MigrationWorkerService
in the spec to locate usages).
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6cc35fe1-cc8b-45f1-b30a-a67c975de318
📒 Files selected for processing (22)
apps/api/src/db/database-manager.tsapps/api/src/db/services/dev-sandbox-provisioner.service.tsapps/api/src/db/services/rbac-inspector.service.tsapps/api/src/db/services/sync-seeding.service.tsapps/api/src/db/services/system-seeder.service.tsapps/api/src/db/services/tenant-schema.service.tsapps/api/src/modules/exceptions/exception.service.tsapps/api/src/modules/stitches/stitches.service.spec.tsapps/api/src/modules/stitches/stitches.service.tsapps/api/src/modules/stitches/stitches.validation.spec.tsapps/api/src/modules/stitches/stitches.validation.tsapps/web/vitest.config.tsapps/worker/src/modules/pipeline/delivery.service.tsapps/worker/src/modules/pipeline/fanout-router.service.tsapps/worker/src/modules/pipeline/fanout.service.tsengine/ai/core/vitest.config.tspackages/database/src/schema/global/identity.tspackages/database/src/schema/global/stitches.tspackages/identity/src/identity.module.tspackages/identity/src/services/permission-seeder.spec.tspackages/identity/src/utils/rbac-seeding.spec.tspackages/pieces/platform/registry/src/pieces/migration-worker.service.spec.ts
| name: body.name, | ||
| orgId, | ||
| workspaceId: body.workspaceId, | ||
| sourceDataSourceId: body.sourceDataSourceId, |
Contributor
There was a problem hiding this comment.
Validate sourceDataSourceId belongs to the organization.
The code validates that destDataSourceId belongs to the org (lines 99-116), but sourceDataSourceId is inserted without similar ownership verification. A malicious or buggy client could reference a data source from another organization.
// Verify destination connection belongs to org
const destConn = await this.db
.select()
.from(dataSources)
.where(
and(
eq(dataSources.id, body.destDataSourceId),
eq(dataSources.tenantId, orgId),
),
)
.limit(1)
.then((rows) => rows[0]);
if (!destConn) {
throw new NotFoundException(
`Data source ${body.destDataSourceId} not found.`,
);
}
+ // Verify source connection belongs to org
+ const srcConn = await this.db
+ .select()
+ .from(dataSources)
+ .where(
+ and(
+ eq(dataSources.id, body.sourceDataSourceId),
+ eq(dataSources.tenantId, orgId),
+ ),
+ )
+ .limit(1)
+ .then((rows) => rows[0]);
+
+ if (!srcConn) {
+ throw new NotFoundException(
+ `Source data source ${body.sourceDataSourceId} not found.`,
+ );
+ }📝 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
| sourceDataSourceId: body.sourceDataSourceId, | |
| // Verify destination connection belongs to org | |
| const destConn = await this.db | |
| .select() | |
| .from(dataSources) | |
| .where( | |
| and( | |
| eq(dataSources.id, body.destDataSourceId), | |
| eq(dataSources.tenantId, orgId), | |
| ), | |
| ) | |
| .limit(1) | |
| .then((rows) => rows[0]); | |
| if (!destConn) { | |
| throw new NotFoundException( | |
| `Data source ${body.destDataSourceId} not found.`, | |
| ); | |
| } | |
| // Verify source connection belongs to org | |
| const srcConn = await this.db | |
| .select() | |
| .from(dataSources) | |
| .where( | |
| and( | |
| eq(dataSources.id, body.sourceDataSourceId), | |
| eq(dataSources.tenantId, orgId), | |
| ), | |
| ) | |
| .limit(1) | |
| .then((rows) => rows[0]); | |
| if (!srcConn) { | |
| throw new NotFoundException( | |
| `Source data source ${body.sourceDataSourceId} not found.`, | |
| ); | |
| } | |
| sourceDataSourceId: body.sourceDataSourceId, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/modules/stitches/stitches.service.ts` at line 127, The patch is
missing ownership validation for sourceDataSourceId; add the same org membership
check you used for destDataSourceId in stitches.service.ts: fetch or look up the
data source identified by sourceDataSourceId and confirm its orgId matches the
request organization (or reuse the existing helper that validates
destDataSourceId), and if it does not belong to the org throw/return the same
authorization/validation error before inserting sourceDataSourceId into the
stitch record. Ensure you perform this check in the same scope where
destDataSourceId is validated so both IDs are verified consistently.
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 4 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 3 file(s) based on 4 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
…nd coverage threshold
This was referenced Jun 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation