operational hardening foundation - #104
Merged
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a WebhooksModule (controller + signature and rate-limit guards), a ShutdownService with a 30s hard-deadline, piece-level webhook configuration, unit tests for guards/controller/shutdown, and moves DB pool shutdown into an explicit closeDb()/OnModuleDestroy flow. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant TRL as TenantRateLimitGuard
participant WSG as WebhookSignatureGuard
participant Controller as WebhooksController
participant DB as Database
participant Redis as Redis
Client->>TRL: POST /webhooks/:connectionId (rawBody)
TRL->>DB: resolve connectionId -> tenantId, metadata
TRL->>Redis: EVAL rate-limit script (key: ratelimit:l1:{tenantId}:{connectionId})
Redis-->>TRL: allowed / TTL remaining
alt Rate limited
TRL-->>Client: 429 Too Many Requests + Retry-After
else Allowed
TRL->>WSG: forward request (req.webhookResolved attached)
WSG->>WSG: compute HMAC-SHA256(rawBody, secret)
WSG->>WSG: timing-safe compare with signature header
alt Invalid signature
WSG-->>Client: 403 Forbidden
else Valid
WSG->>Controller: pass request
Controller->>DB: BEGIN tx (set search_path tenant schema)
Controller->>DB: INSERT inboundGateway (traceId, body, headers, extReqId)
DB-->>Controller: Success / unique-violation (23505)
Controller-->>Client: 202 Accepted
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/connectors/src/framework/piece.ts (1)
268-323: 🛠️ Refactor suggestion | 🟠 MajorFail fast on invalid webhook config at piece creation.
createPiece()currently accepts emptysecretKeyEnv/signatureHeadervalues and defers failure to runtime request handling.🛡️ Proposed validation
export function createPiece(params: CreatePieceParams): Piece { + if (params.webhook) { + const { secretKeyEnv, signatureHeader } = params.webhook; + if (!secretKeyEnv?.trim()) { + throw new InternalServerErrorException('Invalid webhook config: secretKeyEnv is required'); + } + if (!signatureHeader?.trim()) { + throw new InternalServerErrorException('Invalid webhook config: signatureHeader is required'); + } + } + // Convert Action array to a Record for O(1) invocation lookups const actionsMap = params.actions.reduce(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/connectors/src/framework/piece.ts` around lines 268 - 323, In createPiece, validate the webhook config at construction time: if params.webhook is provided, ensure it's an object and that webhook.secretKeyEnv and webhook.signatureHeader are non-empty strings (or whatever fields are required by your webhook shape), otherwise throw an InternalServerErrorException with a clear message like "Invalid webhook config: missing secretKeyEnv/signatureHeader". Add this check near where triggers/actions are reduced (before returning the Piece) to fail fast for invalid params.webhook and reference params.webhook, secretKeyEnv, signatureHeader, and InternalServerErrorException in the change.
🤖 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/core/shutdown.service.spec.ts`:
- Around line 10-17: The current test uses vi.spyOn(process, 'on') which still
registers real signal handlers; change the spy on process.on (the processOnSpy
used in the beforeEach) to stub out the implementation so it does not append
listeners (e.g., mockImplementation(() => process) or a no-op) and keep
process.exit spied as before; update the beforeEach that constructs
ShutdownService to replace processOnSpy with a stubbed implementation and ensure
teardown restores and removes any listeners to avoid leaking handlers between
tests in ShutdownService specs.
In `@apps/api/src/core/shutdown.service.ts`:
- Around line 22-27: enableShutdownHooks is not idempotent and can register
multiple listeners; make it safe by adding a guard (e.g., a private boolean like
shutdownHooksEnabled) on the service and return early if already registered, or
alternatively register handlers with process.once and ensure you do not
re-register on subsequent calls; update enableShutdownHooks to check/set the
guard (or switch to process.once for each signal) and leave shutdown(app,
signal) and the logger usage unchanged.
In `@apps/api/src/guards/tenant-rate-limit.guard.ts`:
- Around line 73-90: The guard's canActivate currently queries Postgres for any
connectionId and only applies Redis rate-limits when a row is found; add a cheap
UUID validation for req.params['connectionId'] and immediately reject/limit
malformed IDs before performing the DB query, and add a fallback rate-limiter
path for unknown but well-formed UUIDs to avoid a DB hit on every probe: inside
canActivate (and before the db.select against appConnections) validate
connectionId with a fast UUID check, return or apply the generic limiter when
invalid/malformed, and if the UUID passes but the DB lookup yields no row, use
the fallback limiter and attach no WEBHOOK_RESOLVED_CONNECTION, otherwise
proceed as existing when a row is found.
- Around line 56-66: The Lua script in tenant-rate-limit.guard.ts (private
static readonly LUA_SCRIPT) can return TTL -1 when a key exists without expiry,
which lets canActivate() treat the bucket as allowed; modify the script to check
the TTL result (redis.call("TTL", KEYS[1])) and if it's negative (<= -1) call
redis.call("EXPIRE", KEYS[1], ARGV[2]) to reapply the expiry before returning
the TTL so exhausted buckets without an expiry are normalized and rate limiting
cannot fail open.
In `@apps/api/src/modules/webhooks/webhook-signature.guard.spec.ts`:
- Around line 252-277: Add a regression test that asserts the guard fails closed
when a connection maps to an unregistered piece: in the existing suite (use
setup, makeExecutionContext, and guard.canActivate) create a test where setup
registers a connection with appName set to an unregistered piece (e.g., appName:
'unregistered-piece'), provide appropriate headers/body (or rawBody) as needed,
then expect guard.canActivate(ctx) to reject with ForbiddenException or
NotFoundException per intended policy; ensure you reference the existing helpers
(setup, makeExecutionContext) and the guard.canActivate call so the test
explicitly verifies missing piece registration behavior.
In `@apps/api/src/modules/webhooks/webhook-signature.guard.ts`:
- Around line 48-50: The guard currently returns true when
pieceRegistry.getPiece(appName) is undefined, allowing unregistered pieces to
bypass signature checks; change the logic in webhook-signature.guard.ts so that
if pieceRegistry.getPiece(appName) returns undefined the request is rejected
(fail-closed) instead of allowed—i.e., only allow through when a piece exists
and either has no webhook config or its webhook signature verifies successfully;
update the branch that currently checks piece?.webhook to explicitly handle
piece === undefined and return false (or throw/deny) so unknown appName mappings
cannot bypass verification.
In `@apps/api/src/modules/webhooks/webhooks.controller.spec.ts`:
- Around line 100-136: Add a test that verifies controller.ingest sets no
extReqId when neither 'x-webhook-id' nor 'x-event-id' is present: copy the
pattern used in the existing tests that mocks db._tx.insert and captures
inserted values (the capturedValues variable and the vi.fn on values), call
controller.ingest with an empty headers object (or no headers), and assert that
capturedValues is defined and capturedValues['extReqId'] is undefined (or null
if your contract uses null); use the same mocking setup as the other tests to
locate the behavior in controller.ingest and db._tx.insert.
In `@apps/api/src/modules/webhooks/webhooks.controller.ts`:
- Around line 77-83: The current tx.insert(inboundGateway).values call is
persisting the entire headers object, which can leak sensitive values; before
calling tx.insert (in the code path that assembles traceId, connectionId,
payload, headers, extReqId), filter the incoming headers into an explicit
allowlist (e.g., only store safe keys like content-type, user-agent,
x-request-id) and pass that filteredHeaders object instead of headers to the
values map; update the insertion to use filteredHeaders and ensure the allowlist
is defined near the handler (or as a constant) so future reviewers can see which
headers are persisted.
- Around line 89-107: The current isPgUniqueViolation(err) suppresses all
Postgres 23505 errors; change this to only suppress idempotency-related
unique-constraint collisions by inspecting the error's constraint or detail for
the idempotency key(s) before returning true. Add a new predicate (e.g.,
isPgIdempotencyViolation or extend isPgUniqueViolation) that checks err.code ===
PG_UNIQUE_VIOLATION AND (err.constraint matches the idempotency constraint
name(s) OR err.detail/text includes the idempotency column names like
ext_req_id/trace_id), then use that predicate in the webhook handler where
isPgUniqueViolation(err) is currently called so only true idempotency collisions
are ignored.
In `@packages/pieces/quickbooks/src/index.ts`:
- Around line 271-275: The piece currently declares webhook support via the
webhook object (properties secretKeyEnv, signatureHeader, signatureEncoding) in
packages/pieces/quickbooks/src/index.ts, but QuickBooks sends all company events
to a single app endpoint and identifies companies with payload.realmId, not a
path :connectionId; remove or disable the exported webhook configuration (the
webhook property) from the QuickBooks piece until the framework/controller is
updated to resolve connections from payload.realmId instead of the URL path;
ensure any references to webhook.secretKeyEnv, webhook.signatureHeader or
webhook.signatureEncoding in this file or tests are removed or adjusted
accordingly.
---
Outside diff comments:
In `@packages/connectors/src/framework/piece.ts`:
- Around line 268-323: In createPiece, validate the webhook config at
construction time: if params.webhook is provided, ensure it's an object and that
webhook.secretKeyEnv and webhook.signatureHeader are non-empty strings (or
whatever fields are required by your webhook shape), otherwise throw an
InternalServerErrorException with a clear message like "Invalid webhook config:
missing secretKeyEnv/signatureHeader". Add this check near where
triggers/actions are reduced (before returning the Piece) to fail fast for
invalid params.webhook and reference params.webhook, secretKeyEnv,
signatureHeader, and InternalServerErrorException in the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 151de672-c1fc-4069-ac3d-e3b3df2c28ff
📒 Files selected for processing (15)
apps/api/src/app/app.module.tsapps/api/src/core/shutdown.service.spec.tsapps/api/src/core/shutdown.service.tsapps/api/src/guards/tenant-rate-limit.guard.spec.tsapps/api/src/guards/tenant-rate-limit.guard.tsapps/api/src/main.tsapps/api/src/modules/webhooks/webhook-signature.guard.spec.tsapps/api/src/modules/webhooks/webhook-signature.guard.tsapps/api/src/modules/webhooks/webhooks.controller.spec.tsapps/api/src/modules/webhooks/webhooks.controller.tsapps/api/src/modules/webhooks/webhooks.module.tsdocs/architecture/master/tasks.mdpackages/connectors/src/framework/piece.tspackages/pieces/quickbooks/src/index.tspackages/pieces/salesforce/src/index.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/core/shutdown.service.spec.ts`:
- Around line 54-58: The test currently only verifies idempotency for SIGTERM by
filtering processOnceSpy.mock.calls into sigtermCalls and asserting length 1;
add an equivalent assertion for SIGINT by creating sigintCalls =
processOnceSpy.mock.calls.filter((c) => c[0] === 'SIGINT') and
expect(sigintCalls).toHaveLength(1) so both signals are asserted idempotent;
keep the existing sigterm assertion and add the new sigint assertion using the
same processOnceSpy reference.
- Around line 26-29: The test teardown currently calls
process.removeAllListeners('SIGTERM') and process.removeAllListeners('SIGINT'),
which is unsafe and unnecessary because process.once is mocked; remove those two
process.removeAllListeners(...) calls from the afterEach block so the test no
longer mutates process-global listeners—keep the existing mock of process.once
and any restoration/cleanup of the spy but delete the two removeAllListeners
lines (references: process.removeAllListeners, process.once, afterEach).
In `@apps/api/src/core/shutdown.service.ts`:
- Around line 34-36: ShutdownService registers signal handlers that call
app.close(), but the database client also calls process.once(...) to
pool!.end(), causing race conditions; remove the direct signal handlers from the
database client (packages/database/src/client.ts) and instead implement
OnModuleDestroy in the database module (e.g., add a class implementing
OnModuleDestroy with a method that calls pool!.end()) so draining the pool runs
as part of the coordinated app.close() shutdown path; also update the
ShutdownService logger call that passes an Error object to Logger.error (the
call where it logs "Error during graceful shutdown -- forcing exit") to pass
err.stack or a stringified error (err.stack || String(err)) so the trace is
preserved.
- Around line 70-73: The catch block in shutdown.service.ts logs the caught
error object directly to this.logger.error, which expects a stack string; update
the catch handler (the block that clears deadline and calls
this.logger.error(...) then process.exit(1)) to normalize the caught err by
passing err.stack when available or falling back to err.message or String(err)
as the second argument to this.logger.error so the stacktrace is captured
correctly in logs before calling process.exit(1).
In `@apps/api/src/guards/tenant-rate-limit.guard.ts`:
- Around line 215-225: The resolveLimit function currently accepts any positive
number for rateLimitPerMin; add an upper bound check to avoid extreme values by
introducing a MAX_RATE_LIMIT constant (e.g., 10_000 or a project-appropriate
value) and if the extracted custom value exceeds MAX_RATE_LIMIT either clamp it
to MAX_RATE_LIMIT (recommended) or return DEFAULT_LIMIT, and emit a warning via
the module's logger (or console.warn if no logger present). Update resolveLimit
to validate that custom is a finite number, compare against MAX_RATE_LIMIT, log
when the incoming rateLimitPerMin is unusually high, and return the
clamped/validated value; reference the symbols resolveLimit, rateLimitPerMin,
DEFAULT_LIMIT and the new MAX_RATE_LIMIT constant in your changes.
In `@packages/connectors/src/framework/piece.ts`:
- Around line 302-318: Extend the existing runtime validation inside the
params.webhook check to also validate params.webhook.signatureEncoding when
present: ensure it's a string equal to either 'base64' or 'hex' (reject other
values), and if invalid throw the same InternalServerErrorException used in this
block with a message referencing createPiece and the piece name (params.name)
and the allowed values; place this check alongside the existing checks for
secretKeyEnv and signatureHeader so dynamically loaded pieces fail at startup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3fc8dd50-7718-4c5c-aace-6e181185f6bb
📒 Files selected for processing (10)
apps/api/src/core/shutdown.service.spec.tsapps/api/src/core/shutdown.service.tsapps/api/src/guards/tenant-rate-limit.guard.spec.tsapps/api/src/guards/tenant-rate-limit.guard.tsapps/api/src/modules/webhooks/webhook-signature.guard.spec.tsapps/api/src/modules/webhooks/webhook-signature.guard.tsapps/api/src/modules/webhooks/webhooks.controller.spec.tsapps/api/src/modules/webhooks/webhooks.controller.tspackages/connectors/src/framework/piece.tspackages/pieces/quickbooks/src/index.ts
Comment on lines
+34
to
+36
| for (const signal of ['SIGTERM', 'SIGINT'] as const) { | ||
| process.once(signal, () => void this.shutdown(app, signal)); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all process-level signal handlers and confirm whether shutdown is centralized.
rg -n -C2 "process\.(once|on)\('SIGTERM'|process\.(once|on)\('SIGINT'"Repository: pramodnarayana/nexiom
Length of output: 540
🏁 Script executed:
#!/bin/bash
# Get full context of database client initialization and signal handler registration
sed -n '39,51p' packages/database/src/client.tsRepository: pramodnarayana/nexiom
Length of output: 479
🏁 Script executed:
#!/bin/bash
# Get ShutdownService implementation to see how it coordinates shutdown
cat -n apps/api/src/core/shutdown.service.ts | head -80Repository: pramodnarayana/nexiom
Length of output: 3342
🏁 Script executed:
#!/bin/bash
# Check if there are any other competing signal handlers in the codebase
rg -n "process\.(once|on)\(" --type ts --type js | grep -E "(SIGTERM|SIGINT|SIGHUP|SIGKILL)" | head -20Repository: pramodnarayana/nexiom
Length of output: 229
Unify SIGTERM/SIGINT ownership to avoid shutdown races.
ShutdownService registers signal handlers that call app.close() (lines 34-36), but packages/database/src/client.ts:48-49 independently registers the same signals with process.once() to call pool!.end() directly. Since process.once() fires handlers in registration order with no guaranteed sequencing, the pool may close before app.close() completes its OnModuleDestroy chain, violating the "in-flight queries finish cleanly" invariant. Remove the direct signal handlers from the database client; instead, register an OnModuleDestroy hook in the database module to drain the pool as part of the coordinated app.close() flow.
Additionally, at line 72, Logger.error() receives an Error object as the second argument, but the signature expects a string trace. Call err.stack or use a proper error formatter to preserve stack diagnostics:
this.logger.error('Error during graceful shutdown -- forcing exit', err.stack || String(err));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/core/shutdown.service.ts` around lines 34 - 36, ShutdownService
registers signal handlers that call app.close(), but the database client also
calls process.once(...) to pool!.end(), causing race conditions; remove the
direct signal handlers from the database client
(packages/database/src/client.ts) and instead implement OnModuleDestroy in the
database module (e.g., add a class implementing OnModuleDestroy with a method
that calls pool!.end()) so draining the pool runs as part of the coordinated
app.close() shutdown path; also update the ShutdownService logger call that
passes an Error object to Logger.error (the call where it logs "Error during
graceful shutdown -- forcing exit") to pass err.stack or a stringified error
(err.stack || String(err)) so the trace is preserved.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/api/src/guards/tenant-rate-limit.guard.ts (1)
114-130:⚠️ Potential issue | 🟠 MajorHigh-cardinality misses still keep Postgres on the hot path.
applyFallbackRateLimit()only runs after theappConnectionslookup, and the fallback key is unique perconnectionId. A spray of random UUIDs therefore causes one DB miss and one fresh Redis key per request, so this public route can still hammer both stores without ever tripping the fallback bucket. Add a shared pre-DB limiter or a short-lived negative cache for UUID-shaped misses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/guards/tenant-rate-limit.guard.ts` around lines 114 - 130, The current flow does a DB lookup against appConnections by connectionId then calls applyFallbackRateLimit on misses, which still lets high-cardinality UUID probes hammer Postgres and create many fresh Redis keys; before querying appConnections (in tenant-rate-limit.guard.ts) add a shared pre-DB limiter or short-lived negative cache for UUID-shaped misses: detect UUID-shaped connectionId early, consult/atomically increment a shared Redis probe key (e.g., ratelimit:l1:probe:pre:{bucket}) and reject when that bucket is over threshold, and if you still perform the DB lookup, store a short TTL negative cache entry (e.g., ratelimit:l1:neg:{connectionId}) on misses so repeated invalid UUIDs hit Redis only and skip Postgres; update applyFallbackRateLimit usage to run only after these pre-checks.
🤖 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/core/shutdown.service.spec.ts`:
- Around line 83-84: Add an explicit existence assertion for the signal handler
before invoking it: locate the places where you call handler!() (the local
variable created by const handler = getHandler('SIGTERM') and the other similar
calls) and insert expect(handler).toBeDefined() immediately before each
invocation (e.g., before handler!() in the tests using getHandler). This ensures
the test fails with a clear message if the handler registration regresses.
In `@apps/api/src/guards/tenant-rate-limit.guard.ts`:
- Around line 141-144: resolveLimit reads rateLimitPerMin from connection
metadata but the Redis bucket key (`ratelimit:l1:${conn.tenantId}`) is
tenant-scoped causing cross-connection interference; update the key used by
checkRateLimit to include a unique connection identifier (e.g., `conn.id` or
`conn.connectionId`) or the app name when the limit is intended per-connection
(for example `ratelimit:l1:${conn.tenantId}:${conn.id}`), or alternatively
persist the resolved limit to a tenant-owned record if the limit must be
tenant-scoped; apply the same key change to every call that builds
`ratelimit:l1:${conn.tenantId}` (including the other occurrences around the
checkRateLimit calls) so the bucket scope matches resolveLimit(conn.metadata).
- Around line 226-234: The code currently accepts fractional rateLimitPerMin
values which Redis treats as integers, so update the validation to require an
integer: replace the Number.isFinite(custom) check with Number.isInteger(custom)
(or explicitly Math.floor/Math.trunc the value) so fractional values are
rejected or normalized before use; preserve the existing MAX_RATE_LIMIT clamp
and use resolveLimitLogger.warn to report when a fractional value is supplied
for rateLimitPerMin (referencing metadata['rateLimitPerMin'], MAX_RATE_LIMIT,
and resolveLimitLogger in tenant-rate-limit.guard.ts).
In `@packages/database/src/client.ts`:
- Around line 55-60: The closeDb function currently awaits pool.end() but leaves
pool and dbInstance set if pool.end() throws; change closeDb so that it checks
for pool, then calls await pool.end() inside a try block and always clears the
cached handles in a finally block (i.e., set pool = undefined and dbInstance =
undefined in the finally) to ensure stale connections are not retained even on
shutdown errors; reference the closeDb function and the pool.end(), pool and
dbInstance identifiers when making the change.
---
Duplicate comments:
In `@apps/api/src/guards/tenant-rate-limit.guard.ts`:
- Around line 114-130: The current flow does a DB lookup against appConnections
by connectionId then calls applyFallbackRateLimit on misses, which still lets
high-cardinality UUID probes hammer Postgres and create many fresh Redis keys;
before querying appConnections (in tenant-rate-limit.guard.ts) add a shared
pre-DB limiter or short-lived negative cache for UUID-shaped misses: detect
UUID-shaped connectionId early, consult/atomically increment a shared Redis
probe key (e.g., ratelimit:l1:probe:pre:{bucket}) and reject when that bucket is
over threshold, and if you still perform the DB lookup, store a short TTL
negative cache entry (e.g., ratelimit:l1:neg:{connectionId}) on misses so
repeated invalid UUIDs hit Redis only and skip Postgres; update
applyFallbackRateLimit usage to run only after these pre-checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef4e710e-4645-4a2d-acd6-720885e3ec60
📒 Files selected for processing (6)
apps/api/src/core/shutdown.service.spec.tsapps/api/src/core/shutdown.service.tsapps/api/src/guards/tenant-rate-limit.guard.tspackages/connectors/src/framework/piece.tspackages/database/src/client.tspackages/database/src/database.module.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/guards/tenant-rate-limit.guard.ts`:
- Around line 124-132: Replace the truthiness check on the numeric result of
redis.exists() with an explicit comparison to 1: when calling redis.exists(...)
assign to isCachedMiss and check isCachedMiss === 1 before calling
applyFallbackRateLimit and throwing NotFoundException; update the conditional
around redis.exists in tenant-rate-limit.guard.ts (the block that calls
applyFallbackRateLimit and throws NotFoundException for connectionId) to use
this explicit comparison to make the intent clear and guard against non-boolean
return values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54d06ac2-9579-4375-94c3-5d74840c09ba
📒 Files selected for processing (4)
apps/api/src/core/shutdown.service.spec.tsapps/api/src/guards/tenant-rate-limit.guard.spec.tsapps/api/src/guards/tenant-rate-limit.guard.tspackages/database/src/client.ts
This was referenced Mar 25, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Documentation