feat: decouple platform and application logic via GitOps shards - #136
Merged
Merged
Conversation
- Replaced legacy ApplicationExecutorService with dynamic ApplicationLoaderService - Introduced PipelineHookBrokerService as the sole interface for platform-to-shard communication - Replaced static registries in piece-framework with dynamic shard loads - Implemented webhook-triggered GitOps synchronization (GitopsSyncController, Worker) - Updated TargetBuilder, Replica, and Normalization services to use the new broker pattern - Migrated TenantDatabaseManager CLI to load domain provisioners dynamically - Added tests for GitopsWebhookGuard and updated existing pipeline specs
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces static in-memory app hooks with dynamic shard loading (ApplicationLoaderService + PipelineHookBrokerService), adds GitOps webhook + guarded endpoint and queue-driven syncs, migrates pipeline workers to broker hooks, implements tenant-per-database provisioning and tenant provisioner service, updates migrations/snapshots, and bumps Node to 20.18.1. ChangesPlatform sharding, GitOps, and multi-tenant database DAG
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as API Server
participant Guard as GitopsWebhookGuard
participant Ctrl as GitopsSyncController
participant Queue as QueueService
Client->>API: POST /internal/gitops/sync (webhook)
API->>Guard: canActivate(req)
Guard->>Guard: validate HMAC / X-Gitlab-Token / Authorization Bearer
alt auth fails
Guard-->>API: throw UnauthorizedException
API-->>Client: 401
else auth succeeds
Guard-->>API: allow
API->>Ctrl: triggerSync()
Ctrl->>Queue: send(GitopsQueue, { source: "webhook", triggeredAt })
Queue-->>Ctrl: enqueued
Ctrl-->>API: { accepted: true } (202)
API-->>Client: 202
end
sequenceDiagram
participant Queue as QueueService
participant Worker as GitopsSyncWorker
participant AppLoader as ApplicationLoaderService
participant Shard as Shard Module
Queue->>Worker: message { source, triggeredAt }
Worker->>Worker: syncShard(shardName)
Worker->>Worker: canonicalize path, check .git, validate branch
Worker->>Shard: git pull origin <branch> --ff-only
alt new commits pulled
Shard-->>Worker: pull indicates changes
Worker->>AppLoader: invalidateCache(shardName)
AppLoader-->>AppLoader: remove cached module
end
alt error
Worker-->>Queue: log & rethrow (retry)
else success
Worker-->>Queue: ack processed
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/init-localstack.sh (1)
13-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the missing
ai-copilot-queueto keep queue definitions in sync.At Line 18,
gitops-queuewas added, but the list still omitsai-copilot-queueeven thoughQueueName.AiCopilotQueueexists inpackages/queue/src/constants.ts. This breaks the documented “must match” contract and can cause missing-queue failures in local/dev workers.Suggested patch
QUEUES=( "inbound-queue" "replica-queue" "normalized-queue" "delivery-queue" + "ai-copilot-queue" "gitops-queue" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/init-localstack.sh` around lines 13 - 19, The QUEUES array is missing the ai-copilot-queue entry and must match the QueueName.AiCopilotQueue constant; add the string "ai-copilot-queue" to the QUEUES list (alongside "gitops-queue", "inbound-queue", etc.) so the QUEUES array and QueueName.AiCopilotQueue remain in sync and local/dev workers don’t fail due to a missing queue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/gitops/gitops-sync.controller.ts`:
- Around line 20-28: The webhook auth mismatch: update GitopsWebhookGuard so it
accepts native provider headers instead of only "Authorization: Bearer <secret>"
— implement checking for X-Hub-Signature-256 by computing the HMAC SHA256 of the
raw request body using the same GITOPS_WEBHOOK_SECRET and comparing to the
header for GitHub, and accept X-Gitlab-Token exact-match to
GITOPS_WEBHOOK_SECRET for GitLab; ensure the guard reads the raw request body
(used for HMAC) and preserves existing Authorization: Bearer behavior as a
fallback so canActivate (or the guard's validate/authorize method) supports all
three validation paths.
In `@apps/api/src/modules/gitops/gitops-webhook.guard.ts`:
- Around line 29-32: The constructor in GitopsWebhookGuard uses
config.getOrThrow('GITOPS_WEBHOOK_SECRET') but doesn't reject empty or
whitespace-only secrets; after retrieving the secret in the constructor, trim
and validate it (e.g., ensure secret.trim().length > 0 and optionally a minimum
byte/character length) and throw a clear error if invalid before computing
expectedDigest; likewise, apply the same non-empty/whitespace validation to any
bearer token retrieval/validation code referenced around lines 44-49 (e.g.,
where bearer tokens are read or compared), failing fast with a thrown error so
blank credentials cannot degrade authentication.
In `@apps/worker/src/db/database-manager.ts`:
- Around line 701-713: The domainProvisionerResolver currently swallows errors
from loaderInstance.load and from shard.provisionDomain which can hide failed
provisioning; update domainProvisionerResolver to surface failures by capturing
errors from loaderInstance.load and from shard.provisionDomain (or at minimum
log them with context including appName, schemaName) and then rethrow (or return
a rejected promise) so plan execution fails visibly; specifically update the
async inner function that calls loaderInstance.load(`${appName}-${appName}`) and
the conditional shard.provisionDomain(tenantDb, schemaName) to handle exceptions
(log via your process/logger and throw) rather than silently returning
null/no-op.
In `@apps/worker/src/modules/pipeline/gitops-sync.worker.ts`:
- Around line 93-98: syncShard builds repoPath from shardName without
path-traversal guards; normalize and validate shardName before filesystem ops:
resolve repoPath with path.resolve(this.SHARD_BASE_PATH, shardName), ensure the
resolved repoPath is contained within this.SHARD_BASE_PATH (e.g., check
path.relative(this.SHARD_BASE_PATH, repoPath) does not start with '..' or that
repoPath.startsWith(path.resolve(this.SHARD_BASE_PATH) + path.sep)), and reject
or throw for absolute/escaped inputs; update the syncShard implementation
(references: syncShard, repoPath, SHARD_BASE_PATH) to perform this validation
before calling fs.stat.
In `@apps/worker/src/modules/pipeline/normalization.service.spec.ts`:
- Around line 91-99: Update the tests in normalization.service.spec.ts to
actually assert that NormalizationService calls the broker methods: keep the
provided PipelineHookBrokerService mock but add expectations that the
mock.normalize(...) was called in the existing happy-path test (when
mockResolvedValue(null) is used ensure
vi.mocked(normalize).toHaveBeenCalledWith(...) or equivalent), and add a new
test case where the broker mock.normalize is set to resolve a non-null
normalized payload and then assert that broker.writeNormalized(...) was called
(and that the piece.normalize path was not used if applicable). Reference the
PipelineHookBrokerService mock, the NormalizationService method under test, and
the mock methods normalize and writeNormalized when adding these assertions.
In `@apps/worker/src/modules/pipeline/normalization.service.ts`:
- Around line 162-167: The catch that swallows errors when calling
this.hookBroker.normalize (assigning normalizedFromShard) should log the error
instead of being silent; update the promise catch to capture the error and call
a warning logger (e.g., this.logger.warn or processLogger.warn) with context
including connectionAppName, replica.entityType and the error message/stack,
then return null to preserve the fallback to piece.normalize, ensuring
operational visibility while keeping existing behavior.
In `@engine/sync/platform/core/src/sharding/application-executor.service.ts`:
- Around line 144-149: The code currently returns JSON.parse(resultStr) directly
(after reading resultStr via vm.getProp/vm.getString and disposing resultHandle)
which can yield null, arrays, or primitives; add a runtime guard after parsing
resultStr to ensure the parsed value is a non-null object and not an array
(e.g., const parsed = JSON.parse(resultStr); if (typeof parsed !== 'object' ||
parsed === null || Array.isArray(parsed)) throw new Error('Invalid shard mapping
return: expected Record<string, unknown>'); return parsed as Record<string,
unknown>), so that the method in application-executor.service.ts enforces the
expected return shape and fails fast on invalid shard outputs.
- Around line 92-133: The guest execution currently sets only a memory limit but
no interrupt/deadline, so before calling vm.evalCode(scriptCode) set an
interrupt handler via
vm.runtime.setInterruptHandler(shouldInterruptAfterDeadline(deadline)) with a
computed deadline, and change the async wrapper invoked in scriptCode to resolve
a guest Promise that the host can race against a timeout (use
Promise.race([guestPromise, timeoutPromise])) so the host can detect async
timeouts; after vm.evalCode and before vm.runtime.executePendingJobs() check
whether the deadline has elapsed (or the interrupt fired) and only call
executePendingJobs() when within the deadline, throwing/cleaning up otherwise.
Ensure you reference and update vm.runtime.setInterruptHandler,
shouldInterruptAfterDeadline(deadline), vm.evalCode(scriptCode), the async IIFE
in scriptCode (make it produce a guestPromise), Promise.race, and
vm.runtime.executePendingJobs() in your changes.
In `@engine/sync/platform/core/src/sharding/application-loader.service.ts`:
- Around line 49-57: The current string-based boundary check using resolvedBase
and shardPath can be bypassed via symlinks; replace it with real filesystem
canonicalization: call fs.realpathSync or await fs.promises.realpath on both
this.SHARD_BASE_PATH (or resolvedBase) and the computed shard path (shardPath)
to get baseReal and shardReal, then verify shardReal === baseReal or
shardReal.startsWith(baseReal + path.sep) (and throw the same Security violation
error if it fails). Update the checks around resolvedBase/shardPath in
application-loader.service.ts (the path validation block) and do the same for
the similar guard later (lines ~60-67).
- Around line 74-80: The code imports a shard module and caches it without
validating its exported shape, which can cause downstream failures; update the
load logic that imports `${shardPath}?v=${Date.now()}` into the variable `mod`
(typed as `ApplicationShardModule`) to perform explicit runtime checks on
required exports (e.g., ensure expected functions/classes/properties exist and
have correct types) before calling `this.cache.set(shardName, mod)` and logging;
if validation fails, throw or log a descriptive error including `shardName` and
`shardPath` so the module is not cached and failures surface immediately.
In `@engine/sync/platform/core/src/sharding/pipeline-hook-broker.service.ts`:
- Around line 138-143: The current try/catch around loader.load and
shard.getWebhookResponse in getWebhookResponse swallows all errors; change it so
only the "shard missing" case returns null. Call
this.loader.load(this.shardName(appName, appProfile)) and if it throws, inspect
the error to determine "not found" (return null); for any other loader errors
rethrow or log and rethrow. After successfully getting shard, invoke
shard.getWebhookResponse?.(body, headers) without catching its exceptions here
(or catch them to log via process logger but then rethrow) so runtime hook
failures are surfaced; reference methods loader.load, shardName, and
getWebhookResponse to locate and update the logic accordingly.
In `@packages/pieces/platform/framework/src/app-hooks.ts`:
- Around line 4-13: Add JSDoc deprecation tags to each exported API in this
module (e.g., registerNormalizedWriter, getTargetBuilder, registerTargetBuilder,
registerReplicaService, getReplicaService, registerNormalizationService,
getNormalizationService and any other exported register*/get* functions) by
placing a /** `@deprecated` {message} */ comment immediately above each exported
function or constant; use a concise message such as "Deprecated — superseded by
the dynamic shard loading architecture (ApplicationLoaderService +
PipelineHookBrokerService); will be removed in the next release" so editors/TS
tooling surface the deprecation during the one-release migration window.
---
Outside diff comments:
In `@scripts/init-localstack.sh`:
- Around line 13-19: The QUEUES array is missing the ai-copilot-queue entry and
must match the QueueName.AiCopilotQueue constant; add the string
"ai-copilot-queue" to the QUEUES list (alongside "gitops-queue",
"inbound-queue", etc.) so the QUEUES array and QueueName.AiCopilotQueue remain
in sync and local/dev workers don’t fail due to a missing queue.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c437a907-7fbb-4204-90e3-e0f2a34a08cf
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
.nvmrcapps/api/src/app/app.module.tsapps/api/src/modules/gitops/gitops-sync.controller.spec.tsapps/api/src/modules/gitops/gitops-sync.controller.tsapps/api/src/modules/gitops/gitops-webhook.guard.spec.tsapps/api/src/modules/gitops/gitops-webhook.guard.tsapps/api/src/modules/gitops/gitops.module.tsapps/worker/src/db/database-manager.tsapps/worker/src/main.tsapps/worker/src/modules/pipeline/gitops-sync.worker.spec.tsapps/worker/src/modules/pipeline/gitops-sync.worker.tsapps/worker/src/modules/pipeline/normalization.service.spec.tsapps/worker/src/modules/pipeline/normalization.service.tsapps/worker/src/modules/pipeline/pipeline.module.tsapps/worker/src/modules/pipeline/replica.service.spec.tsapps/worker/src/modules/pipeline/replica.service.tsapps/worker/src/modules/pipeline/target-builder.service.spec.tsapps/worker/src/modules/pipeline/target-builder.service.tsengine/sync/platform/core/package.jsonengine/sync/platform/core/src/index.tsengine/sync/platform/core/src/sharding/application-executor.module.tsengine/sync/platform/core/src/sharding/application-executor.service.spec.tsengine/sync/platform/core/src/sharding/application-executor.service.tsengine/sync/platform/core/src/sharding/application-loader.module.tsengine/sync/platform/core/src/sharding/application-loader.service.tsengine/sync/platform/core/src/sharding/application-shard.types.tsengine/sync/platform/core/src/sharding/logic-resolver.module.tsengine/sync/platform/core/src/sharding/logic-resolver.service.tsengine/sync/platform/core/src/sharding/pipeline-hook-broker.service.tsengine/sync/platform/core/vitest.config.tspackage.jsonpackages/pieces/platform/framework/src/app-hooks.tspackages/queue/src/constants.tsscripts/init-localstack.sh
💤 Files with no reviewable changes (3)
- engine/sync/platform/core/src/sharding/logic-resolver.module.ts
- apps/worker/src/main.ts
- engine/sync/platform/core/src/sharding/logic-resolver.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 9 file(s) based on 12 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 9 file(s) based on 12 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
apps/worker/src/modules/pipeline/gitops-sync.worker.ts (1)
93-115:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRealpath the repo before touching
.gitor runninggit pull.This guard is still string-based. A symlinked shard directory under
SHARD_BASE_PATHwill satisfy the current containment check, and bothfs.stat()andexecFile(..., { cwd: repoPath })will then follow it outside the trusted tree.Suggested hardening
async syncShard(shardName: string): Promise<void> { - const resolvedBasePath = path.resolve(this.SHARD_BASE_PATH); - const repoPath = path.resolve(this.SHARD_BASE_PATH, shardName); + const resolvedBasePath = await fs.realpath(path.resolve(this.SHARD_BASE_PATH)); + const requestedRepoPath = path.resolve(this.SHARD_BASE_PATH, shardName); + const repoPath = await fs.realpath(requestedRepoPath); - const relativePath = path.relative(resolvedBasePath, repoPath); + const relativePath = path.relative(resolvedBasePath, repoPath); if ( - relativePath.startsWith('..') || - path.isAbsolute(relativePath) || - !repoPath.startsWith(resolvedBasePath + path.sep) + relativePath.startsWith('..') || + path.isAbsolute(relativePath) ) { this.logger.warn( `Path traversal attempt detected: shardName="${shardName}" escapes SHARD_BASE_PATH. Rejecting sync.`, ); throw new Error( `Security violation: shardName escapes trusted boundary — shardName="${shardName}"`, ); } try { await fs.stat(path.join(repoPath, ".git"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/modules/pipeline/gitops-sync.worker.ts` around lines 93 - 115, The current containment check in syncShard is string-based and can be bypassed by symlinks; resolve this by calling fs.realpath on the repoPath (and ideally on path.join(repoPath, ".git")) and use the resolved realRepoPath for the containment check against the resolvedBasePath and as the cwd for any git operations (e.g., execFile). Update syncShard to realpath repoPath, verify path.relative(resolvedBasePath, realRepoPath) does not escape, and use realRepoPath when touching .git or running git pull to prevent symlink escape from SHARD_BASE_PATH.engine/sync/platform/core/src/sharding/application-loader.service.ts (1)
73-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winImport the canonical path you just validated.
load()verifiesshardReal, butimport()still executesshardPath. If the file or symlink target changes betweenrealpath()andimport(), this can validate one path and execute another. ImportshardReal/its file URL instead so the boundary check and the loaded module refer to the same file.Suggested hardening
+import { pathToFileURL } from 'node:url'; ... - const mod = (await import( - `${shardPath}?v=${Date.now()}` - )) as ApplicationShardModule; + const mod = (await import( + `${pathToFileURL(shardReal).href}?v=${Date.now()}` + )) as ApplicationShardModule;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/sync/platform/core/src/sharding/application-loader.service.ts` around lines 73 - 95, The code validates the canonical path in load() (shardReal) but then imports the original shardPath, allowing a TOCTOU where the target can change between realpath() and import(); update the import to use the canonical path (shardReal) converted to a file URL (e.g., via pathToFileURL) and preserve the timestamp cache-buster query so the same validated file is what gets executed; change the import expression that currently uses `${shardPath}?v=${Date.now()}` to use the file-URL built from shardReal with the same ?v= timestamp, and keep the existing boundary check that compares shardReal to baseReal.engine/sync/platform/core/src/sharding/pipeline-hook-broker.service.ts (1)
139-147:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t classify loader failures by substring matching.
errMsg.includes('not found')can also match broken imports like missing transitive modules, and those should surface as shard regressions, not silently returnnull. HaveApplicationLoaderService.load()throw a dedicated not-found error/code and only suppress that exact case here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/sync/platform/core/src/sharding/pipeline-hook-broker.service.ts` around lines 139 - 147, The catch block in PipelineHookBrokerService where you call this.loader.load(this.shardName(...)) must not rely on substring matching of loadErr.message; instead have ApplicationLoaderService.load() throw a specific NotFound error/class or set a distinct error.code (e.g., 'SHARD_NOT_FOUND'), then change the catch to check for that exact signal (error instanceof ShardNotFoundError or error.code === 'SHARD_NOT_FOUND') and return null only in that case, otherwise rethrow the error so genuine loader failures surface; update/reuse the loader's public API (ApplicationLoaderService.load) and the callsite in the loader.load(...) catch accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/gitops/gitops-webhook.guard.ts`:
- Around line 33-44: The code trims the env value into trimmedSecret but then
uses the original secret for this.secret and expectedDigest, causing mismatch;
change assignments to use the normalized value by setting this.secret =
trimmedSecret and computing this.expectedDigest =
createHash('sha256').update(trimmedSecret).digest() (the lookup via
config.getOrThrow can remain as-is) so the guard and digest both use the trimmed
secret consistently.
In `@apps/worker/src/db/database-manager.ts`:
- Around line 693-729: The current domainProvisionerResolver manually loads
shards via ApplicationLoaderService and calls shard.provisionDomain; instead
instantiate PipelineHookBrokerService with the ApplicationLoaderService (or pass
loaderInstance into its constructor) and have domainProvisionerResolver delegate
to broker.provisionDomain(appName, tenantDb, schemaName), catching and
rethrowing errors and logging the same contextual messages; replace direct calls
to loaderInstance.load(...) and shard.provisionDomain(...) with a single call to
broker.provisionDomain(...) and keep the existing error handling around that
call.
In `@apps/worker/src/modules/pipeline/gitops-sync.worker.ts`:
- Around line 32-38: The queue consumer callback in onModuleInit (the callback
passed to queueService.consume for QueueName.GitopsQueue) currently always
resolves even when syncShardRepositories is skipped or fails, which ACKs
messages prematurely; change the handler so it throws (or returns a rejected
Promise) when sync is skipped or when an error occurs instead of swallowing it.
Concretely: update the consume callback to inspect the result of
syncShardRepositories (and any early-return “already running” path) and throw a
specific Error when nothing was performed, and in your top-level try/catch
around the call log the error but rethrow it (do not swallow) so the queue can
NACK/retry; apply the same pattern for the other handlers referenced around
queue consumer code (the other consume callbacks at the same module).
In `@apps/worker/src/modules/pipeline/normalization.service.spec.ts`:
- Around line 114-145: The test is asserting against a PipelineHookBrokerService
instance that wasn’t the one injected into the NormalizationService handler;
fetch the broker from the same TestingModule used to build the service/handler
created in beforeEach (the module that produced handler from
NormalizationService.onModuleInit) instead of a second module, or register a spy
object for PipelineHookBrokerService in that original module’s providers so the
injected broker is the spy; update the expect to call mockBroker from that same
module (reference NormalizationService, handler, PipelineHookBrokerService, and
module.get) so the assertion validates the actual broker used by the service.
- Around line 304-355: The test's shared transaction stub is missing a
transaction method so NormalizationService's call to tx.transaction(...) falls
back to the warning path and never reaches broker.writeNormalized; update the
test's stub for the DB/tx object used by the module (the same object referenced
by the handler invocation) to include a transaction method that accepts a
callback, executes that callback with a tx-like object (or the stub itself) and
returns the callback's result (or a resolved promise), ensuring
NormalizationService.transaction(...) runs and the broker.writeNormalized path
is exercised when using mockBroker in the spec.
In `@apps/worker/src/modules/pipeline/normalization.service.ts`:
- Around line 253-266: The call to this.hookBroker.writeNormalized inside
tx.transaction passes the savepoint handle sp but still passes this.db (the root
connection), allowing shard hooks to perform writes outside the current
transaction; update the call so the transactional handle sp is passed in place
of this.db (i.e., pass sp for both connection arguments) or alternatively change
the hook contract to accept only a single transactional handle; locate the
invocation of this.hookBroker.writeNormalized in the tx.transaction callback and
replace the this.db argument with sp (or refactor writeNormalized to only accept
the transactional handle).
In `@engine/sync/platform/core/src/sharding/application-executor.service.ts`:
- Around line 65-66: Replace the error messages that expose internal paths and
raw execution details by throwing a sanitized InternalServerErrorException
(e.g., "Failed to load shard module" or "Internal server error") while logging
the detailed diagnostic (including modulePath and the original error/stack) to
the service logger; update the throw at the location using modulePath and the
similar throw around lines 181-184 to use the generic client-facing message and
ensure the real error and modulePath are recorded via this.logger.error (or
processLogger.error) before throwing.
- Around line 53-59: Resolve real filesystem paths before performing the
trusted-boundary check to prevent symlink traversal: call fs.realpathSync (or
async fs.promises.realpath) on this.SHARD_BASE_PATH to get resolvedBasePath and
on the candidate module path (built from this.SHARD_BASE_PATH and shardName into
modulePath) to get realModulePath, then enforce that realModulePath startsWith
resolvedBasePath + path.sep or equals resolvedBasePath; throw the same
InternalServerErrorException if it escapes. Apply the same realpath-based check
to the other validation that uses resolved paths (the second occurrence
involving module resolution) so both checks use real paths rather than
path.resolve.
---
Duplicate comments:
In `@apps/worker/src/modules/pipeline/gitops-sync.worker.ts`:
- Around line 93-115: The current containment check in syncShard is string-based
and can be bypassed by symlinks; resolve this by calling fs.realpath on the
repoPath (and ideally on path.join(repoPath, ".git")) and use the resolved
realRepoPath for the containment check against the resolvedBasePath and as the
cwd for any git operations (e.g., execFile). Update syncShard to realpath
repoPath, verify path.relative(resolvedBasePath, realRepoPath) does not escape,
and use realRepoPath when touching .git or running git pull to prevent symlink
escape from SHARD_BASE_PATH.
In `@engine/sync/platform/core/src/sharding/application-loader.service.ts`:
- Around line 73-95: The code validates the canonical path in load() (shardReal)
but then imports the original shardPath, allowing a TOCTOU where the target can
change between realpath() and import(); update the import to use the canonical
path (shardReal) converted to a file URL (e.g., via pathToFileURL) and preserve
the timestamp cache-buster query so the same validated file is what gets
executed; change the import expression that currently uses
`${shardPath}?v=${Date.now()}` to use the file-URL built from shardReal with the
same ?v= timestamp, and keep the existing boundary check that compares shardReal
to baseReal.
In `@engine/sync/platform/core/src/sharding/pipeline-hook-broker.service.ts`:
- Around line 139-147: The catch block in PipelineHookBrokerService where you
call this.loader.load(this.shardName(...)) must not rely on substring matching
of loadErr.message; instead have ApplicationLoaderService.load() throw a
specific NotFound error/class or set a distinct error.code (e.g.,
'SHARD_NOT_FOUND'), then change the catch to check for that exact signal (error
instanceof ShardNotFoundError or error.code === 'SHARD_NOT_FOUND') and return
null only in that case, otherwise rethrow the error so genuine loader failures
surface; update/reuse the loader's public API (ApplicationLoaderService.load)
and the callsite in the loader.load(...) catch accordingly.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d16755aa-e409-41d6-85d5-97f012970d13
📒 Files selected for processing (9)
apps/api/src/modules/gitops/gitops-webhook.guard.tsapps/worker/src/db/database-manager.tsapps/worker/src/modules/pipeline/gitops-sync.worker.tsapps/worker/src/modules/pipeline/normalization.service.spec.tsapps/worker/src/modules/pipeline/normalization.service.tsengine/sync/platform/core/src/sharding/application-executor.service.tsengine/sync/platform/core/src/sharding/application-loader.service.tsengine/sync/platform/core/src/sharding/pipeline-hook-broker.service.tspackages/pieces/platform/framework/src/app-hooks.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 6 file(s) based on 8 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 6 file(s) based on 8 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
pramodnarayana
marked this pull request as draft
April 30, 2026 15:46
pramodnarayana
marked this pull request as ready for review
April 30, 2026 15:46
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
apps/worker/src/modules/pipeline/gitops-sync.worker.ts (2)
78-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't swallow sync failures before they reach the queue consumer.
onModuleInit()now rethrows queue-handler errors, but these two catches still absorb them. A failedreaddir, timeout, orgit pullwill therefore ACK the webhook message and skip retry, which pushes recovery back to the 5-minute cron path. If non-repo directories should be ignored, keep that as an explicit early return and rethrow actual sync failures.Suggested split between “skip” and “fail” paths
try { // Ensure the base sync directory exists await fs.mkdir(this.SHARD_BASE_PATH, { recursive: true }); @@ for (const shard of shardDirs) { await this.syncShard(shard.name); } } catch (error) { this.logger.error("GitOps shard synchronization failed", error); + throw error; } finally { this.isSyncRunning = false; } } @@ async syncShard(shardName: string): Promise<void> { @@ - try { - await fs.stat(path.join(repoPath, ".git")); + try { + await fs.stat(path.join(repoPath, ".git")); + } catch { + this.logger.debug(`[${shardName}] Skipping non-git directory.`); + return; + } + try { // Validate branch name to prevent command injection const branchName = this.DEFAULT_BRANCH; @@ } else { this.logger.log( `[${shardName}] New commits pulled — invalidating module cache:\n${stdout}`, ); this.applicationLoaderService.invalidateCache(shardName); } } catch (err) { - this.logger.warn( - `[${shardName}] Not a git repository or git pull failed — skipping.`, + this.logger.warn( + `[${shardName}] GitOps sync failed.`, err instanceof Error ? err.message : String(err), ); + throw err; } }Also applies to: 164-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/modules/pipeline/gitops-sync.worker.ts` around lines 78 - 101, The catch around reading/iterating shards is currently swallowing all errors (fs.readdir, syncShard, git pull, timeouts) which causes queue messages to be ACKed; change the handler in gitops-sync.worker.ts so that only the expected "no shard dirs" case returns early (keep the existing if (shardDirs.length === 0) return) but in the catch block log the error with this.logger.error and then rethrow it so queue consumers can retry; apply the same change to the other analogous catch (the one around lines referenced 164-168) so both the method containing SHARD_BASE_PATH/shardDirs and the other failing block forward real failures instead of absorbing them, while still ignoring non-repo directories explicitly before the try/catch if needed.
112-128:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCanonicalize shard paths before enforcing the boundary.
This check is only lexical. A symlink inside
SHARD_BASE_PATHcan still passresolve/relativewhilefs.stat()andgit pulloperate on a repository outside the trusted tree.engine/sync/platform/core/src/sharding/application-loader.service.ts:45-86already usesfs.realpath()to close this gap; the worker should do the same.Suggested hardening
async syncShard(shardName: string): Promise<void> { - const resolvedBasePath = path.resolve(this.SHARD_BASE_PATH); - const repoPath = path.resolve(this.SHARD_BASE_PATH, shardName); + const resolvedBasePath = path.resolve(this.SHARD_BASE_PATH); + const repoPath = path.resolve(resolvedBasePath, shardName); + const [baseReal, repoReal] = await Promise.all([ + fs.realpath(resolvedBasePath), + fs.realpath(repoPath), + ]); - const relativePath = path.relative(resolvedBasePath, repoPath); + const relativePath = path.relative(baseReal, repoReal); if ( relativePath.startsWith("..") || path.isAbsolute(relativePath) || - !repoPath.startsWith(resolvedBasePath + path.sep) + (repoReal !== baseReal && !repoReal.startsWith(baseReal + path.sep)) ) { this.logger.warn( `Path traversal attempt detected: shardName="${shardName}" escapes SHARD_BASE_PATH. Rejecting sync.`, ); throw new Error(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/modules/pipeline/gitops-sync.worker.ts` around lines 112 - 128, The path containment check is only lexical and can be bypassed via symlinks; canonicalize both the SHARD_BASE_PATH and the resolved repo path using fs.realpath (or fs.promises.realpath) before performing the containment check. In the gitops-sync.worker.ts block that computes resolvedBasePath, repoPath and relativePath for shardName, replace the current path.resolve usage with realpath results for the base and the candidate repo (handle and propagate realpath errors), then enforce that the canonical repo realpath startsWith the canonical base realpath + path.sep (and reject/throw with the same warning message if not).apps/worker/src/modules/pipeline/normalization.service.ts (1)
170-178:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSanitize shard hook errors before logging.
Line 176–177 logs raw
message/stack. That bypasses the existingsanitizeErrorpattern used in this service and can leak sensitive context from shard exceptions.Suggested fix
this.logger.warn( { event: "l3.shard_normalize_failed", connectionAppName, appProfile, entityType: replica.entityType, - err: err instanceof Error ? err.message : String(err), - stack: err instanceof Error ? err.stack : undefined, + err: sanitizeError(err), }, "Shard normalize failed — falling back to piece.normalize", );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/modules/pipeline/normalization.service.ts` around lines 170 - 178, The log in the normalization service is currently emitting raw err.message and err.stack in the this.logger.warn call (event "l3.shard_normalize_failed"); instead call the existing sanitizeError utility used elsewhere in this class to produce a sanitized error object and log its safe fields instead of raw message/stack—i.e., obtain const safe = sanitizeError(err) and replace err instanceof Error ? err.message : String(err) and err instanceof Error ? err.stack : undefined with safe.message and safe.stack (or the sanitized fields your sanitizeError returns) while keeping connectionAppName, appProfile, and replica.entityType intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/gitops/gitops-webhook.guard.ts`:
- Around line 131-133: The code recomputes secretDigest for every GitLab token
check; instead reuse the precomputed digest stored on the instance
(this.expectedDigest) like validateBearerToken does: compute tokenDigest with
createHash('sha256').update(token).digest() and compare it against
this.expectedDigest rather than recomputing secretDigest from
this.secret/trimmedSecret; update the logic in the GitLab token validation path
(the tokenDigest/secretDigest block) to reference this.expectedDigest.
In `@apps/api/src/scripts/admin-bootstrap.ts`:
- Around line 57-58: The call to seedSystemRbac currently casts db to any which
hides a real type mismatch; locate the seedSystemRbac declaration and its
expected DB type, then remove the "as any" cast on the db argument by either (A)
updating seedSystemRbac's signature to accept NodePgDatabase<typeof schema> (or
the concrete DB type used here) or (B) creating a narrow typed adapter/bridge
variable that converts/annotates the local db (the db identifier in this file)
to the exact type seedSystemRbac expects; update imports/usages accordingly and
remove the eslint-disable comment so the compiler enforces the correct contract.
In `@apps/worker/src/modules/pipeline/normalization.service.spec.ts`:
- Around line 148-149: Remove the leftover console.log debug statements from the
test file: delete the console.log("SPY CALLS:",
JSON.stringify(mockBroker.normalize.mock.calls)); lines (both occurrences around
the mockBroker.normalize assertion) so the test only contains the expectation
calls (e.g., expect(mockBroker.normalize).toHaveBeenCalledWith(...)); keep the
mockBroker.normalize spy and assertions intact and do not add any other logging.
---
Duplicate comments:
In `@apps/worker/src/modules/pipeline/gitops-sync.worker.ts`:
- Around line 78-101: The catch around reading/iterating shards is currently
swallowing all errors (fs.readdir, syncShard, git pull, timeouts) which causes
queue messages to be ACKed; change the handler in gitops-sync.worker.ts so that
only the expected "no shard dirs" case returns early (keep the existing if
(shardDirs.length === 0) return) but in the catch block log the error with
this.logger.error and then rethrow it so queue consumers can retry; apply the
same change to the other analogous catch (the one around lines referenced
164-168) so both the method containing SHARD_BASE_PATH/shardDirs and the other
failing block forward real failures instead of absorbing them, while still
ignoring non-repo directories explicitly before the try/catch if needed.
- Around line 112-128: The path containment check is only lexical and can be
bypassed via symlinks; canonicalize both the SHARD_BASE_PATH and the resolved
repo path using fs.realpath (or fs.promises.realpath) before performing the
containment check. In the gitops-sync.worker.ts block that computes
resolvedBasePath, repoPath and relativePath for shardName, replace the current
path.resolve usage with realpath results for the base and the candidate repo
(handle and propagate realpath errors), then enforce that the canonical repo
realpath startsWith the canonical base realpath + path.sep (and reject/throw
with the same warning message if not).
In `@apps/worker/src/modules/pipeline/normalization.service.ts`:
- Around line 170-178: The log in the normalization service is currently
emitting raw err.message and err.stack in the this.logger.warn call (event
"l3.shard_normalize_failed"); instead call the existing sanitizeError utility
used elsewhere in this class to produce a sanitized error object and log its
safe fields instead of raw message/stack—i.e., obtain const safe =
sanitizeError(err) and replace err instanceof Error ? err.message : String(err)
and err instanceof Error ? err.stack : undefined with safe.message and
safe.stack (or the sanitized fields your sanitizeError returns) while keeping
connectionAppName, appProfile, and replica.entityType intact.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84244287-f696-47d1-bf90-32a55a319446
📒 Files selected for processing (7)
apps/api/src/modules/gitops/gitops-webhook.guard.tsapps/api/src/modules/webhooks/webhooks.controller.spec.tsapps/api/src/scripts/admin-bootstrap.tsapps/worker/src/db/database-manager.tsapps/worker/src/modules/pipeline/gitops-sync.worker.tsapps/worker/src/modules/pipeline/normalization.service.spec.tsapps/worker/src/modules/pipeline/normalization.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 3 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 3 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/gitops/gitops-webhook.guard.ts`:
- Around line 124-132: The code trims the incoming token into trimmedToken but
still hashes the original token variable; update the digest calculation to use
trimmedToken (e.g., replace createHash('sha256').update(token).digest() with
createHash('sha256').update(trimmedToken).digest()) and ensure any other similar
blocks (the repeated logic that creates tokenDigest) also use trimmedToken for
normalization before comparison so whitespace is consistently removed prior to
hashing; keep the UnauthorizedException check for empty/whitespace as-is.
- Around line 52-63: The guard currently casts request header values to string,
which breaks if headers are arrays; add a helper (e.g., getSingleHeader) that
accepts string | string[] | undefined and returns a single string or throws
UnauthorizedException when Array.isArray(value) to reject duplicate headers, and
use it when reading 'x-hub-signature-256' before calling validateGitHubSignature
and when reading 'x-gitlab-token' before validateGitLabToken; also normalize the
GitLab token by trimming it before any validation and before computing its
SHA-256 digest in validateGitLabToken so the digest is derived from the trimmed
token and matches the expectedDigest created in the constructor.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2adc7786-de9a-4d3d-959d-e2c26803ebd9
📒 Files selected for processing (3)
apps/api/src/modules/gitops/gitops-webhook.guard.tsapps/api/src/scripts/admin-bootstrap.tsapps/worker/src/modules/pipeline/normalization.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 1 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
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/modules/dbmanager/dbmanager.module.ts (1)
19-31:⚠️ Potential issue | 🟠 MajorDbManagerModule must implement
OnModuleDestroyto close cached tenant connection pools on graceful shutdown.The review comment incorrectly states pools are created per request. However,
TenantDatabaseManagercaches pools by tenantId (line 21 inpackages/dbmanager/src/impl/tenant-database-manager.ts), so only one pool per tenant is created.The actual issue:
DbManagerModulelacks anOnModuleDestroyhook to invokeTenantDatabaseManager.closeAll()during shutdown. ThecloseAll()method exists (lines 151–170) and properly closes all cached connections, but it's never called. When the app terminates, for N active tenants, N × 20 connections remain open, risking connection exhaustion on subsequent deployments or restart cycles. ImplementOnModuleDestroyon the provider or module to ensure graceful cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/dbmanager/dbmanager.module.ts` around lines 19 - 31, DbManagerModule currently never calls TenantDatabaseManager.closeAll(), so cached per-tenant Pool connections leak on shutdown; implement Nest's OnModuleDestroy on the provider or DbManagerModule and in the onModuleDestroy lifecycle method call the created TenantDatabaseManager.closeAll() (ensure you reference the instance returned by the factory that constructs TenantDatabaseManager) so all pooled connections are closed during graceful shutdown.apps/api/src/modules/workspaces/workspaces.service.ts (1)
34-83:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPotential WARM slot leak if workspace insert fails after claiming.
If the WARM slot claim succeeds (lines 47-57) but the subsequent workspace insert (lines 71-78) fails with a non-unique-violation error (e.g., connection error, timeout), the org will have claimed an ACTIVE database slot but no workspace record exists. The CapacityManager won't replenish this slot since it's marked ACTIVE.
Consider wrapping the claim and insert in a transaction so the claim is rolled back if the insert fails.
💡 Suggested approach
async create(orgId: string, body: CreateWorkspace) { - try { + return this.db.transaction(async (tx) => { + try { // ── Deferred Provisioning: Claim a WARM database slot ────────────── - const hasDb = await this.db.execute<{ tenant_id: string }>( + const hasDb = await tx.execute<{ tenant_id: string }>( sql`SELECT tenant_id FROM tenant_storage_registry ... ); if ((hasDb.rowCount ?? 0) === 0) { - const claimed = await this.db.execute<{ tenant_id: string }>( + const claimed = await tx.execute<{ tenant_id: string }>( sql`UPDATE tenant_storage_registry ... ); ... } - const [workspace] = await this.db + const [workspace] = await tx .insert(uiWorkspaces) ... + } catch (err) { + if (isUniqueViolation(err)) { + throw new ConflictException(...); + } + throw err; + } + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/workspaces/workspaces.service.ts` around lines 34 - 83, The WARM slot can leak if the UPDATE that claims tenant_storage_registry succeeds but the subsequent insert into uiWorkspaces fails; wrap the claim+insert in a single DB transaction so the UPDATE (claimed) is rolled back on any insert error: begin a transaction via this.db.transaction (or your DB client's transaction API), perform the SELECT/UPDATE claim logic (the same statements referencing tenant_storage_registry, hasDb, claimed) and then perform the insert into uiWorkspaces and .returning(); only commit if the insert succeeds, otherwise roll back so the tenant_storage_registry UPDATE is undone and the slot remains WARM. Ensure the FOR UPDATE SKIP LOCKED behavior remains inside the same transaction scope.apps/api/src/modules/mappings/mappings.service.ts (1)
45-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve tenant-resolution HTTP errors in
create().
getTenantDb(tenantId)now runs inside thistry, so anyHttpExceptionit throws gets rewritten asBadRequestException('Failed to create Mapping'). That changes the API contract for missing or unavailable tenants.update()already preservesHttpException;create()should do the same here.Suggested fix
} catch (error: unknown) { + if (error instanceof HttpException) { + throw error; + } const safePayload = { appName: payload.appName, category: payload.category, entity: payload.entity, viewMode: payload.viewMode,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/mappings/mappings.service.ts` around lines 45 - 73, The create() method currently wraps getTenantDb(tenantId) in a try/catch and unconditionally throws a BadRequestException, which masks HttpExceptions from tenant resolution; change the catch in create() to detect and rethrow existing HttpException instances (same behavior as update()), e.g., if error is an instance of HttpException rethrow it, otherwise log the safePayload and throw the BadRequestException as before; refer to the create() method and getTenantDb() call to locate where to add the HttpException-preserving logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/db/database-manager.ts`:
- Around line 884-886: The catch block that sets hostUrl =
'postgresql://user:password@localhost:5432' silently hides parse failures for
DATABASE_URL; update the error handling in the catch inside the database-manager
logic to log a warning (using the same logger used elsewhere in this file) that
includes the original DATABASE_URL value (or a redacted version) and the caught
error, then keep the fallback assignment if you want to preserve behavior;
reference the catch surrounding the DATABASE_URL parsing and the hostUrl
variable so you can add a logger.warn/processLogger.warn call that includes the
error and the env value before assigning the fallback.
- Around line 1052-1055: The finally currently calls await tenantPool.end() and
await globalClient.end() but if tenantPool creation throws earlier, globalClient
may never be closed; modify the flow so globalClient is guaranteed closed:
either move globalClient setup and its own try/finally to an outer scope (create
globalClient, then try { create tenantPool ... } finally { await
globalClient.end() }) or ensure you check for existence before closing (e.g., if
(tenantPool) await tenantPool.end()) and wrap tenantPool lifecycle in its own
try/finally so both tenantPool and globalClient are closed on all error paths;
update the code referencing tenantPool and globalClient in database-manager.ts
accordingly.
In `@apps/api/src/modules/gitops/gitops-webhook.guard.ts`:
- Around line 94-123: The HMAC verification currently decodes providedDigest
without validating it, which allows inputs like "sha256=<valid>zz" to be
truncated and pass timingSafeEqual; before Buffer.from(providedDigest, 'hex') in
the Gitops webhook guard (the code using providedDigest, timingSafeEqual,
createHmac and throwing UnauthorizedException), validate that providedDigest
matches /^[a-fA-F0-9]{64}$/ (exactly 64 hex characters) and if not, throw an
UnauthorizedException with an appropriate message; only then proceed to
Buffer.from(...) and the constant-time comparison.
In `@apps/api/src/modules/workspaces/capacity-manager.service.ts`:
- Around line 57-83: The loop in CapacityManager (capacity-manager.service.ts)
performs this.db.execute(INSERT ...) then this.queueService.send(...), causing
orphaned INITIALIZING rows if queue send fails; change the order to call
this.queueService.send(QueueName.TenantProvisionQueue, event) first and only
INSERT the INITIALIZING row after the send succeeds so a failed send never
leaves a committed INITIALIZING entry, and ensure the worker (provision handler)
tolerates a missing registry row (or creates it) on start; additionally add a
periodic cleanup routine (e.g., cleanupStaleInitializing or within existing
maintenance cron) to remove/reset INITIALIZING rows older than a configurable
TTL to guard against any remaining edge cases.
- Around line 101-112: deriveHostUrl() currently reconstructs and returns
DATABASE_URL including credentials and is used to populate hostUrl in
ProvisionDatabaseEvent; instead, strip any username/password and return a
credentials-free base URL (protocol, hostname, port) so credentials are not sent
in the queue. Update deriveHostUrl() to parse process.env.DATABASE_URL but omit
parsed.username and parsed.password from the returned string, and adjust the
consumer code (e.g., runMigrations or the handler that receives
ProvisionDatabaseEvent) to recompose the full connection string by reading
credentials from its own process.env.DATABASE_URL at runtime before connecting.
Ensure identifiers: deriveHostUrl(), ProvisionDatabaseEvent, and runMigrations
are updated accordingly.
- Around line 109-111: The catch block that currently returns the literal
placeholder 'postgresql://user:password@localhost:5432' must be removed so
misconfigured/invalid DATABASE_URL errors are not silently masked; instead,
rethrow the original error (or throw a new Error that includes the original
error message) from that try/catch in the host URL parsing function so the
failure surfaces to the caller (e.g., replenishPool) and is logged/handled
there. Ensure you reference and update the try/catch around the DATABASE_URL
parsing in capacity-manager.service.ts to throw the error rather than returning
the placeholder.
In `@apps/api/src/modules/workspaces/capacity-manager.types.ts`:
- Around line 1-13: ProvisionDatabaseEvent is duplicated across services causing
contract drift; extract the interface into a single shared workspace package
(e.g., create `@nexiom/contracts` or `@nexiom/queue`) and export
ProvisionDatabaseEvent from there, then update capacity-manager.service.ts and
tenant-provision.worker.ts to import the type from the new shared package
instead of redefining it so both sides use the exact same contract.
In `@apps/api/src/scripts/admin-bootstrap.ts`:
- Around line 58-62: Replace the unsafe cast by creating a new Drizzle instance
scoped to the identity schema and pass that to seedSystemRbac: instead of
casting db as NodePgDatabase<typeof identitySchema>, call drizzle(client, {
schema: identitySchema }) (matching the pattern in database-manager.ts) to
produce an identityDb and invoke seedSystemRbac(identityDb, config, console);
update references to use identityDb and remove the cast so seedSystemRbac
receives a correctly typed NodePgDatabase<typeof identitySchema>.
In `@apps/api/src/shared/db.utils.spec.ts`:
- Around line 39-42: Add a symmetric unit test for extractPgError that ensures a
non-string direct code returns null: create a new spec mirroring the existing
"returns null if cause is present but has no code string" test but pass { code:
123 } (no cause) and assert extractPgError(error) is null; place it next to the
current test in apps/api/src/shared/db.utils.spec.ts so both branches (direct
code and cause.code) are covered and the function's type-guard on code being a
string is exercised.
- Around line 9-13: Update the test case in apps/api/src/shared/db.utils.spec.ts
for the function extractPgError to include undefined among the non-object
inputs; specifically, modify the 'returns null for non-objects' test (the it
block that calls extractPgError) to also assert
expect(extractPgError(undefined)).toBeNull(), ensuring the utility safely
returns null for undefined values as well as null, strings, and numbers.
In `@apps/tenant-provisioner/src/modules/provisioner/tenant-provision.worker.ts`:
- Around line 30-40: The consumer callback registered in onModuleInit via
this.queueService.consume(QueueName.TenantProvisionQueue, ...) lacks error
handling: wrap the call to await this.provision(event) inside a try/catch, log
the error with this.logger.error including event.poolSlotId and error details,
and rethrow the error from the catch so the queue consumer
(QueueService.consume) can nack/trigger retries instead of letting an unhandled
rejection crash or silently drop the message.
- Around line 121-134: In registerWarmSlot, the UPDATE call to
tenant_storage_registry that sets status='WARM' for tenant_id
`WARM-${poolSlotId}` ignores the query result; change the code to capture the
returned result from adminClient.query and check result.rowCount — if rowCount
=== 0, emit a warning via this.logger.warn (include the poolSlotId and the
attempted tenant id) or throw an error depending on desired behavior so missing
rows are surfaced; keep the existing finally block that calls adminClient.end().
- Around line 96-103: The hard-coded rootDir calculation using path.resolve(...,
'../../../../') is fragile; change tenant-provision.worker.ts to derive the
migrationsFolder from a configurable source (e.g., process.env.MIGRATIONS_DIR or
an injected ConfigService value) and only fall back to the current
fileURLToPath-based resolution if that config is absent. Update the code that
sets migrationsFolder (and any use of rootDir) to first read the env/config
value, validate it exists, and use path.join(configValue,
'packages/database/drizzle/tenant') or the provided absolute path directly;
ensure the migrate() caller uses this migrationsFolder variable so deployments
can override the location without relying on the '../../../../' relative path.
- Around line 78-87: The SELECT-existence check uses existing.rowCount which the
pg docs discourage; change the check to inspect existing.rows.length (e.g., if
(existing.rows.length === 0) ...) when determining whether to CREATE DATABASE
for dbName in the tenant provisioning logic that uses adminClient and existing;
keep the CREATE DATABASE and logging behavior (this.logger.log and
this.logger.warn) unchanged but replace the rowCount check with rows.length for
reliability.
In `@apps/tenant-provisioner/tsconfig.json`:
- Line 3: Remove "vitest/globals" from the root tsconfig.json to stop test
globals leaking into production and create a new test-specific config named
tsconfig.spec.json that extends the base and adds "types": ["vitest/globals"]
and includes only spec files (e.g., "src/**/*.spec.ts"); finally update your
Vitest configuration (vitest.config.ts) to point to the new tsconfig.spec.json
so tests compile with Vitest globals while the main build no longer exposes
describe/it/vi/expect.
In `@apps/worker/src/modules/pipeline/normalized-outbox.worker.spec.ts`:
- Around line 51-58: The "schema query errors" test is no longer exercising
production code because connection discovery is mocked via mockTenants.where;
update the failing path to reject from the actual call the worker now uses:
change the test's failure mock to make globalDb.where(...) (or alternatively
tenantDb.transaction(...) for the other case at lines 191-194) return a rejected
promise (e.g., mockRejectedValue) instead of failing tenantDb.from(), so the
worker's error handling for connection discovery/transaction is exercised;
locate the mocks referencing mockTenants.where, globalDb, and
tenantDb.transaction and swap the resolved/error behavior accordingly.
In `@apps/worker/src/modules/pipeline/normalized-outbox.worker.ts`:
- Around line 50-59: Query only "drainable" app connections from this.globalDb
before calling getTenantDb(): change the select over appConnections to include
the schema/plan predicate that proves normalized_outbox is provisioned (so only
namespace connections with normalized_outbox are returned), e.g., add the
condition checking the schema-plan field on appConnections that guarantees
normalized_outbox exists; then short-circuit (return/continue) when the
resulting connections array is empty to avoid calling getTenantDb() and running
drainWorkspaceOutbox() unnecessarily for tenants with no eligible connections.
In `@engine/ai/core/src/categories/mapping.service.ts`:
- Around line 29-43: The code currently only queries the tenant-local DB (using
this.dbManager.getTenantDb and canonicalMappings) and returns null if nothing is
found; restore the documented "tenant override, then global fallback" behavior
by, after the tenantDb select (mappingRecord), checking if no row was returned
and then performing the identical select against the shared/global canonical
mappings DB (e.g., obtain the global DB via this.dbManager.getGlobalDb() or the
known canonical tenant DB and re-run the same
.select(...).from(canonicalMappings).where(...).limit(1)); return the global
result when present so callers get the shared default when the tenant has no
override.
In `@package.json`:
- Around line 62-67: Move the pnpm only-built-dependencies setting out of the
package.json "pnpm.onlyBuiltDependencies" block and into the workspace config by
adding an onlyBuiltDependencies entry in pnpm-workspace.yaml (the canonical
location since pnpm 10.7+); keep the same values ("@nestjs/core", "@swc/core",
"esbuild", "sharp"), remove the onlyBuiltDependencies key from package.json, and
ensure the workspace YAML uses the same allowlist so lifecycle scripts for
native-binary deps continue to run correctly.
In `@packages/database/drizzle.config.ts`:
- Around line 45-49: The global Drizzle config (export default object)
incorrectly includes tenant schema globs in the schema array; remove
'./dist/schema/tenant/**/*.js' from the schema array in
packages/database/drizzle.config.ts so only ['./dist/schema/global/**/*.js'] is
present and tenant DDL is no longer emitted into the './drizzle/global' output
(tenant tables should be managed by drizzle.config.tenant.ts). Ensure the out
setting remains './drizzle/global' and run the global migrator to confirm only
global migrations are generated.
---
Outside diff comments:
In `@apps/api/src/modules/dbmanager/dbmanager.module.ts`:
- Around line 19-31: DbManagerModule currently never calls
TenantDatabaseManager.closeAll(), so cached per-tenant Pool connections leak on
shutdown; implement Nest's OnModuleDestroy on the provider or DbManagerModule
and in the onModuleDestroy lifecycle method call the created
TenantDatabaseManager.closeAll() (ensure you reference the instance returned by
the factory that constructs TenantDatabaseManager) so all pooled connections are
closed during graceful shutdown.
In `@apps/api/src/modules/mappings/mappings.service.ts`:
- Around line 45-73: The create() method currently wraps getTenantDb(tenantId)
in a try/catch and unconditionally throws a BadRequestException, which masks
HttpExceptions from tenant resolution; change the catch in create() to detect
and rethrow existing HttpException instances (same behavior as update()), e.g.,
if error is an instance of HttpException rethrow it, otherwise log the
safePayload and throw the BadRequestException as before; refer to the create()
method and getTenantDb() call to locate where to add the
HttpException-preserving logic.
In `@apps/api/src/modules/workspaces/workspaces.service.ts`:
- Around line 34-83: The WARM slot can leak if the UPDATE that claims
tenant_storage_registry succeeds but the subsequent insert into uiWorkspaces
fails; wrap the claim+insert in a single DB transaction so the UPDATE (claimed)
is rolled back on any insert error: begin a transaction via this.db.transaction
(or your DB client's transaction API), perform the SELECT/UPDATE claim logic
(the same statements referencing tenant_storage_registry, hasDb, claimed) and
then perform the insert into uiWorkspaces and .returning(); only commit if the
insert succeeds, otherwise roll back so the tenant_storage_registry UPDATE is
undone and the slot remains WARM. Ensure the FOR UPDATE SKIP LOCKED behavior
remains inside the same transaction scope.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e277c165-9ace-45b2-884e-c0d567864731
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (78)
apps/api/src/db/database-manager.tsapps/api/src/modules/connections/connectors.service.spec.tsapps/api/src/modules/connections/connectors.service.tsapps/api/src/modules/dbmanager/dbmanager.module.tsapps/api/src/modules/gitops/gitops-webhook.guard.tsapps/api/src/modules/mappings/mappings.controller.spec.tsapps/api/src/modules/mappings/mappings.controller.tsapps/api/src/modules/mappings/mappings.service.spec.tsapps/api/src/modules/mappings/mappings.service.tsapps/api/src/modules/mappings/mappings.validation.tsapps/api/src/modules/pipeline/inbound-outbox.service.spec.tsapps/api/src/modules/pipeline/inbound-outbox.service.tsapps/api/src/modules/pipeline/replica-outbox.service.spec.tsapps/api/src/modules/pipeline/replica-outbox.service.tsapps/api/src/modules/trigger/trigger-executor.service.tsapps/api/src/modules/trigger/trigger.module.tsapps/api/src/modules/workspaces/capacity-manager.service.spec.tsapps/api/src/modules/workspaces/capacity-manager.service.tsapps/api/src/modules/workspaces/capacity-manager.types.tsapps/api/src/modules/workspaces/workspaces.module.tsapps/api/src/modules/workspaces/workspaces.service.spec.tsapps/api/src/modules/workspaces/workspaces.service.tsapps/api/src/scripts/admin-bootstrap.tsapps/api/src/shared/db.utils.spec.tsapps/tenant-provisioner/eslint.config.mjsapps/tenant-provisioner/nest-cli.jsonapps/tenant-provisioner/package.jsonapps/tenant-provisioner/src/app.module.tsapps/tenant-provisioner/src/main.tsapps/tenant-provisioner/src/modules/provisioner/provision-database.event.tsapps/tenant-provisioner/src/modules/provisioner/provisioner.module.tsapps/tenant-provisioner/src/modules/provisioner/tenant-provision.worker.spec.tsapps/tenant-provisioner/src/modules/provisioner/tenant-provision.worker.tsapps/tenant-provisioner/tsconfig.jsonapps/tenant-provisioner/vitest.config.tsapps/worker/src/modules/dbmanager/dbmanager.module.tsapps/worker/src/modules/pipeline/normalized-outbox.worker.spec.tsapps/worker/src/modules/pipeline/normalized-outbox.worker.tsengine/ai/core/package.jsonengine/ai/core/src/categories/mapping.service.tsengine/ai/core/src/services/chat-persistence.service.tspackage.jsonpackages/database/drizzle.config.tenant.tspackages/database/drizzle.config.tspackages/database/drizzle/0002_add_status_enum_values.sqlpackages/database/drizzle/0003_add_gem_indexes.sqlpackages/database/drizzle/0004_verify_gem_indexes.sqlpackages/database/drizzle/0005_greedy_eddie_brock.sqlpackages/database/drizzle/0006_quick_puck.sqlpackages/database/drizzle/0007_clammy_aaron_stack.sqlpackages/database/drizzle/MIGRATION_NOTES.mdpackages/database/drizzle/global/0000_mixed_sleepwalker.sqlpackages/database/drizzle/global/0001_shocking_microbe.sqlpackages/database/drizzle/global/0003_peaceful_beast.sqlpackages/database/drizzle/global/meta/0000_snapshot.jsonpackages/database/drizzle/global/meta/0001_snapshot.jsonpackages/database/drizzle/global/meta/0003_snapshot.jsonpackages/database/drizzle/global/meta/_journal.jsonpackages/database/drizzle/meta/0001_snapshot.jsonpackages/database/drizzle/meta/0002_snapshot.jsonpackages/database/drizzle/meta/0005_snapshot.jsonpackages/database/drizzle/meta/0007_snapshot.jsonpackages/database/drizzle/meta/_journal.jsonpackages/database/drizzle/tenant/0000_oval_calypso.sqlpackages/database/drizzle/tenant/meta/0000_snapshot.jsonpackages/database/drizzle/tenant/meta/_journal.jsonpackages/database/package.jsonpackages/database/src/index.tspackages/database/src/schema/global/canonical_mappings.tspackages/database/src/schema/global/storage_registry.tspackages/database/src/schema/tenant/canonical_mappings.tspackages/database/src/schema/tenant/chat.tspackages/database/src/schema/tenant/tenant.tspackages/database/src/schema/tenant/workspace.tspackages/dbmanager/src/impl/sql-database-manager.tspackages/dbmanager/src/interfaces.tspackages/queue/src/constants.tsscripts/init-localstack.sh
💤 Files with no reviewable changes (11)
- packages/database/drizzle/0007_clammy_aaron_stack.sql
- packages/database/drizzle/0002_add_status_enum_values.sql
- packages/database/drizzle/meta/0001_snapshot.json
- packages/database/drizzle/0003_add_gem_indexes.sql
- packages/database/drizzle/meta/0002_snapshot.json
- packages/database/drizzle/MIGRATION_NOTES.md
- packages/database/drizzle/0006_quick_puck.sql
- packages/database/drizzle/meta/0005_snapshot.json
- packages/database/drizzle/0005_greedy_eddie_brock.sql
- packages/database/drizzle/0004_verify_gem_indexes.sql
- apps/api/src/modules/mappings/mappings.validation.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 15 file(s) based on 20 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 15 file(s) based on 20 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
pramodnarayana
marked this pull request as draft
May 4, 2026 08:59
pramodnarayana
marked this pull request as ready for review
May 4, 2026 08:59
pramodnarayana
marked this pull request as draft
May 4, 2026 09:05
pramodnarayana
marked this pull request as ready for review
May 4, 2026 09:05
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
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
Improvements
Deprecations
Tests