Merged
Conversation
📝 WalkthroughWalkthroughAdds configurable NATS authentication (nkey, creds, usernamePassword, none), a creds-file option, and a toggle for NATS-based notifications. Migrates pending ACK storage from in-memory Map to JetStream KV, updates many getNatsOptions callsites to accept a creds file, and gates notification NATS wiring behind a feature flag. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Subscriber
participant PendingAckStore
participant NatsService
participant JetStreamKV
participant NATS
Subscriber->>PendingAckStore: save(stream, consumer, msg)
activate PendingAckStore
PendingAckStore->>NatsService: ensure connected / getKV("PENDING_ACK")
activate NatsService
NatsService->>JetStreamKV: js.views.kv("PENDING_ACK")
deactivate NatsService
PendingAckStore->>JetStreamKV: put(key, JSON(metadata))
JetStreamKV-->>PendingAckStore: ack
deactivate PendingAckStore
Subscriber->>NATS: publish ack/acknowledge (via JsMsg.ack)
NATS-->>Subscriber: ack received
Subscriber->>PendingAckStore: delete(key)
PendingAckStore->>JetStreamKV: delete(key)
JetStreamKV-->>PendingAckStore: ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
…older Signed-off-by: RinkalBhojani <rinkal.bhojani@ayanworks.com>
Signed-off-by: RinkalBhojani <rinkal.bhojani@ayanworks.com>
Signed-off-by: RinkalBhojani <rinkal.bhojani@ayanworks.com>
RinkalBhojani
force-pushed
the
feat/nats-credfile-based-auth
branch
from
60c5fc4 to
c7e9d24
Compare
shitrerohit
approved these changes
Mar 28, 2026
Signed-off-by: RinkalBhojani <rinkal.bhojani@ayanworks.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
libs/common/src/common.utils.ts (1)
153-156: Consider case-insensitive comparison for consistency.The existing
shouldLoadOidcModulesuses.toLowerCase()for case-insensitive comparison, butshouldLoadNatsNotificationuses strict equality. This inconsistency meansENABLE_NATS_NOTIFICATION=TRUEorTruewould returnfalse.♻️ Proposed fix for consistency
export function shouldLoadNatsNotification(): boolean { const raw = process.env.ENABLE_NATS_NOTIFICATION; - return 'true' === raw; + return 'true' === raw?.toLowerCase(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/common/src/common.utils.ts` around lines 153 - 156, The shouldLoadNatsNotification function currently compares process.env.ENABLE_NATS_NOTIFICATION with 'true' using strict equality, which is case-sensitive; update shouldLoadNatsNotification to perform a case-insensitive check (e.g., normalize with .toLowerCase() and optional .trim()) and guard against undefined/null before calling string methods so values like 'TRUE' or ' True ' return true, mirroring the behavior of shouldLoadOidcModules.libs/common/src/nats.config.ts (1)
53-61: Consider adding a warning or error for missing nkeySeed in default case.When
NATS_AUTH_TYPE=nkey(or default) butnkeySeedis not provided, the function silently falls back to unauthenticated mode. While this might be intentional for backward compatibility, it could mask configuration issues.At minimum, consider logging a warning when falling back to unauthenticated mode for the default
nkeyauth type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/common/src/nats.config.ts` around lines 53 - 61, The switch default/'nkey' branch currently returns baseOptions silently when nkeySeed is missing; update that branch to emit a warning (e.g., via the existing logger or console.warn) when NATS auth type is 'nkey' (or default) but nkeySeed is falsy, then continue to return baseOptions; reference the nkeySeed variable, the nkeyAuthenticator usage, and baseOptions in the change so the warning clearly states that nkey authentication was requested but seed is missing and the client will connect unauthenticated..env.sample (1)
279-283: Fix formatting issues flagged by linter.Static analysis detected several formatting problems:
- Line 279: Trailing whitespace and unordered key (NATS_AUTH_TYPE should come before NATS_CREDS_FILE)
- Line 281: Spaces around equal sign and trailing whitespace
- Line 283: Missing blank line at end of file
🔧 Proposed fix
PULL_CONSUMER=hub-pull-consumer +# 'nkey' | 'creds' | 'usernamePassword' | 'none' +NATS_AUTH_TYPE=nkey NATS_CREDS_FILE=/platform/app_user.creds -# 'nkey' | 'creds' | 'usernamePassword' | 'none' -NATS_AUTH_TYPE=nkey # 'nkey' | 'creds' | 'usernamePassword' | 'none' -NOTIFICATION_NATS_AUTH_TYPE= +NOTIFICATION_NATS_AUTH_TYPE= ENABLE_NATS_NOTIFICATION=false +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.sample around lines 279 - 283, Remove trailing whitespace and normalize spacing around equals for the env keys shown: ensure lines use KEY=VALUE with no extra spaces (fix NOTIFICATION_NATS_AUTH_TYPE and NATS_AUTH_TYPE). Reorder the block so NATS_AUTH_TYPE appears before NATS_CREDS_FILE if that key exists elsewhere (preserve logical ordering of NATS_* keys). Ensure there's a single blank newline at end of file. Target the keys NATS_AUTH_TYPE, NATS_CREDS_FILE, NOTIFICATION_NATS_AUTH_TYPE, and ENABLE_NATS_NOTIFICATION when making these edits.apps/notification/src/nats/nats.service.ts (1)
16-16: Consider importingNatsAuthTypefrom the shared config.This type is already defined in
libs/common/src/nats.config.ts. Importing it would avoid duplication and ensure consistency if the allowed auth types change.♻️ Import from shared config
-type NatsAuthType = 'nkey' | 'creds' | 'usernamePassword' | 'none'; +import { NatsAuthType } from '@credebl/common/nats.config';Note: You may need to export the type from
nats.config.tsif it isn't already exported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/notification/src/nats/nats.service.ts` at line 16, Replace the locally declared NatsAuthType in nats.service.ts with the shared definition by importing NatsAuthType from the common nats config (e.g., from libs/common/src/nats.config.ts); if NatsAuthType is not exported from that module, export it there first, then update nats.service.ts to remove the duplicate type and add the import so the service uses the single shared NatsAuthType definition.apps/organization/src/organization.module.ts (1)
57-58: Duplicate provider entry.
UserActivityRepositoryis listed twice in theprovidersarray. While NestJS handles this gracefully (no runtime error), it's dead code that should be cleaned up.♻️ Remove duplicate entry
UserActivityRepository, - UserActivityRepository, UserOrgRolesRepository,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/organization/src/organization.module.ts` around lines 57 - 58, Remove the duplicate provider entry for UserActivityRepository in the providers array of the OrganizationModule (organization.module.ts); locate the providers list where UserActivityRepository appears twice and delete the redundant occurrence so only a single UserActivityRepository provider remains, and while there confirm there are no other duplicate provider entries in the same array.apps/notification/src/nats/pendingAckStore.ts (2)
75-75: Refactor nested template literal for readability.SonarCloud flags the nested template literal. Extract the delay suffix to improve clarity.
♻️ Suggested refactor
nak: async (delay?: number) => { const nc = await this.natsService.connect(); - nc.publish(metadata.reply, Buffer.from(`-NAK${delay ? ` ${delay}` : ''}`)); + const delaySuffix = delay ? ` ${delay}` : ''; + nc.publish(metadata.reply, Buffer.from(`-NAK${delaySuffix}`)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/notification/src/nats/pendingAckStore.ts` at line 75, Refactor the nested template literal in the nc.publish call by extracting the delay suffix into a separate variable (e.g., in pendingAckStore code compute a delaySuffix from the delay value) and then use that variable in the Buffer.from call so the publish becomes Buffer.from(`-NAK${delaySuffix}`); update the statement that currently calls nc.publish(metadata.reply, Buffer.from(`-NAK${delay ? ` ${delay}` : ''}`)) to use the new delaySuffix variable for clarity while keeping the same semantics with nc.publish and metadata.reply.
85-88: Consider adding error handling for delete operation.Unlike
get(), thedelete()method has no error handling. If the key doesn't exist or there's a KV connection issue, the error propagates to the caller. For consistency withget(), consider whether this should fail silently or log errors.♻️ Optional: Add resilient error handling
async delete(key: PendingKey): Promise<void> { - const kv = await this.getKV(); - await kv.delete(key); + try { + const kv = await this.getKV(); + await kv.delete(key); + } catch (error) { + console.error(`Failed to delete pending ACK for key ${key}:`, error); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/notification/src/nats/pendingAckStore.ts` around lines 85 - 88, The delete method in pendingAckStore (async delete(key: PendingKey)) lacks error handling and will propagate KV errors; wrap the await kv.delete(key) call in a try-catch, call the same logger used in get() (or the class logger) to log a debug/warn message on failure and optionally swallow the error to match get()'s resilient behavior, or rethrow if your design requires failing; ensure you acquire the KV via getKV() as before and reference the delete method and getKV() so the change is easy to locate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.demo:
- Around line 249-255: Fix formatting in the .env demo: remove trailing
whitespace after the value of NATS_AUTH_TYPE (currently "nkey "), remove the
extra space after the equals sign for NOTIFICATION_NATS_AUTH_TYPE so it reads
NOTIFICATION_NATS_AUTH_TYPE= (no spaces), and ensure the file ends with a
newline character so ENABLE_NATS_NOTIFICATION=false is followed by an EOF
newline.
In `@apps/notification/src/nats/nats-subscriber.ts`:
- Around line 68-70: The code calls msg.ack() without awaiting it, so
pendingAckStore.delete(data.ackKey) may run before the async ACK completes;
update the handler to await the ACK promise (await msg.ack()) and only then call
await this.pendingAckStore.delete(data.ackKey), handling errors from msg.ack()
(try/catch or propagated) so the pending key is removed only after successful
ACK; locate the logic around msg.ack() and this.pendingAckStore.delete in the
NATS subscriber handler to make this change.
In `@apps/notification/src/nats/nats.service.ts`:
- Around line 110-118: The getKV method currently caches a single KV in this.kv
and ignores the bucketName parameter; update it so caching is per-bucket by
replacing the single this.kv with a Map keyed by bucketName (e.g., this.kvMap)
and return or set entries via this.js.views.kv(bucketName), or if only one
bucket is intended, remove the bucketName parameter and hardcode the bucket
(eliminate this.kv ambiguity). Ensure you update references to getKV and any
initialization of this.kv to use the new Map (or remove usages if you hardcode)
and preserve the connected check and error behavior.
In `@apps/notification/src/nats/pendingAckStore.ts`:
- Around line 80-82: The catch block in pendingAckStore.ts currently swallows
all errors and returns undefined; update the catch to at minimum log the caught
error (using the project's logger if available, otherwise console.error) so
KV/network/data errors are visible, e.g., in the try/catch around the get/save
logic for the pending ACK store (the function containing this catch), and
preserve the existing return behavior only after logging the error.
In `@apps/webhook/src/main.ts`:
- Around line 15-19: Replace the incorrect usage of
process.env.ISSUANCE_NKEY_SEED with process.env.WEBHOOK_NKEY_SEED in the NATS
options calls: update the getNatsOptions invocation that supplies the NKEY seed
(the call using CommonConstants.WEBHOOK_SERVICE) and any similar call in the
Webhook module (the place creating NATS options in webhook.module.ts) so both
pass process.env.WEBHOOK_NKEY_SEED instead of process.env.ISSUANCE_NKEY_SEED.
In `@apps/webhook/src/webhook.module.ts`:
- Around line 23-27: The NATS options in the WebhookModule are using the
issuance NKEY seed (process.env.ISSUANCE_NKEY_SEED) which can authenticate this
service with the wrong key; update the call to getNatsOptions inside
WebhookModule (where CommonConstants.WEBHOOK_SERVICE is passed) to use a
webhook-specific environment variable (e.g. process.env.WEBHOOK_NKEY_SEED)
instead of ISSUANCE_NKEY_SEED, and ensure the new env var is documented/loaded
where other service envs are handled.
In `@libs/common/src/nats.config.ts`:
- Around line 39-49: The usernamePassword branch in the NATS config silently
falls back to unauthenticated mode when NATS_USER or NATS_PASSWORD are missing;
update the case 'usernamePassword' handling to validate process.env.NATS_USER
and process.env.NATS_PASSWORD and, if either is absent, throw a clear Error (or
log and throw) instead of returning baseOptions so the misconfiguration is
surfaced; ensure the error message names the missing env var(s) and keep using
usernamePasswordAuthenticator(user, pass) when both are present.
- Around line 29-37: The 'creds' branch in the switch silently falls back to
unauthenticated baseOptions when creds is missing or unreadable; modify the case
'creds' block in nats.config.ts (the code that currently calls readFileSync and
returns {...baseOptions, authenticator: credsAuthenticator(utf8)}) to fail fast:
if creds is falsy or the file cannot be read, throw a clear Error identifying
the missing/invalid creds (include the creds path and the selected auth type),
and only call credsAuthenticator(readFileSync(...)) and return the auth options
when the file exists and is successfully read; ensure the thrown message gives
enough context for debugging.
---
Nitpick comments:
In @.env.sample:
- Around line 279-283: Remove trailing whitespace and normalize spacing around
equals for the env keys shown: ensure lines use KEY=VALUE with no extra spaces
(fix NOTIFICATION_NATS_AUTH_TYPE and NATS_AUTH_TYPE). Reorder the block so
NATS_AUTH_TYPE appears before NATS_CREDS_FILE if that key exists elsewhere
(preserve logical ordering of NATS_* keys). Ensure there's a single blank
newline at end of file. Target the keys NATS_AUTH_TYPE, NATS_CREDS_FILE,
NOTIFICATION_NATS_AUTH_TYPE, and ENABLE_NATS_NOTIFICATION when making these
edits.
In `@apps/notification/src/nats/nats.service.ts`:
- Line 16: Replace the locally declared NatsAuthType in nats.service.ts with the
shared definition by importing NatsAuthType from the common nats config (e.g.,
from libs/common/src/nats.config.ts); if NatsAuthType is not exported from that
module, export it there first, then update nats.service.ts to remove the
duplicate type and add the import so the service uses the single shared
NatsAuthType definition.
In `@apps/notification/src/nats/pendingAckStore.ts`:
- Line 75: Refactor the nested template literal in the nc.publish call by
extracting the delay suffix into a separate variable (e.g., in pendingAckStore
code compute a delaySuffix from the delay value) and then use that variable in
the Buffer.from call so the publish becomes Buffer.from(`-NAK${delaySuffix}`);
update the statement that currently calls nc.publish(metadata.reply,
Buffer.from(`-NAK${delay ? ` ${delay}` : ''}`)) to use the new delaySuffix
variable for clarity while keeping the same semantics with nc.publish and
metadata.reply.
- Around line 85-88: The delete method in pendingAckStore (async delete(key:
PendingKey)) lacks error handling and will propagate KV errors; wrap the await
kv.delete(key) call in a try-catch, call the same logger used in get() (or the
class logger) to log a debug/warn message on failure and optionally swallow the
error to match get()'s resilient behavior, or rethrow if your design requires
failing; ensure you acquire the KV via getKV() as before and reference the
delete method and getKV() so the change is easy to locate.
In `@apps/organization/src/organization.module.ts`:
- Around line 57-58: Remove the duplicate provider entry for
UserActivityRepository in the providers array of the OrganizationModule
(organization.module.ts); locate the providers list where UserActivityRepository
appears twice and delete the redundant occurrence so only a single
UserActivityRepository provider remains, and while there confirm there are no
other duplicate provider entries in the same array.
In `@libs/common/src/common.utils.ts`:
- Around line 153-156: The shouldLoadNatsNotification function currently
compares process.env.ENABLE_NATS_NOTIFICATION with 'true' using strict equality,
which is case-sensitive; update shouldLoadNatsNotification to perform a
case-insensitive check (e.g., normalize with .toLowerCase() and optional
.trim()) and guard against undefined/null before calling string methods so
values like 'TRUE' or ' True ' return true, mirroring the behavior of
shouldLoadOidcModules.
In `@libs/common/src/nats.config.ts`:
- Around line 53-61: The switch default/'nkey' branch currently returns
baseOptions silently when nkeySeed is missing; update that branch to emit a
warning (e.g., via the existing logger or console.warn) when NATS auth type is
'nkey' (or default) but nkeySeed is falsy, then continue to return baseOptions;
reference the nkeySeed variable, the nkeyAuthenticator usage, and baseOptions in
the change so the warning clearly states that nkey authentication was requested
but seed is missing and the client will connect unauthenticated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd0f9bf3-1cad-48e4-acfa-ad7d4281b3bf
📒 Files selected for processing (65)
.env.demo.env.sampleapps/agent-provisioning/src/agent-provisioning.module.tsapps/agent-provisioning/src/main.tsapps/agent-service/src/agent-service.module.tsapps/agent-service/src/main.tsapps/api-gateway/src/agent-service/agent-service.module.tsapps/api-gateway/src/app.module.tsapps/api-gateway/src/authz/authz.module.tsapps/api-gateway/src/cloud-wallet/cloud-wallet.module.tsapps/api-gateway/src/connection/connection.module.tsapps/api-gateway/src/credential-definition/credential-definition.module.tsapps/api-gateway/src/ecosystem/ecosystem.module.tsapps/api-gateway/src/fido/fido.module.tsapps/api-gateway/src/geo-location/geo-location.module.tsapps/api-gateway/src/issuance/issuance.module.tsapps/api-gateway/src/main.tsapps/api-gateway/src/notification/notification.module.tsapps/api-gateway/src/oid4vc-issuance/oid4vc-issuance.module.tsapps/api-gateway/src/oid4vc-verification/oid4vc-verification.module.tsapps/api-gateway/src/organization/organization.module.tsapps/api-gateway/src/platform/platform.module.tsapps/api-gateway/src/schema/schema.module.tsapps/api-gateway/src/user/user.module.tsapps/api-gateway/src/utilities/utilities.module.tsapps/api-gateway/src/verification/verification.module.tsapps/api-gateway/src/webhook/webhook.module.tsapps/api-gateway/src/x509/x509.module.tsapps/cloud-wallet/src/cloud-wallet.module.tsapps/cloud-wallet/src/main.tsapps/connection/src/connection.module.tsapps/connection/src/main.tsapps/ecosystem/src/ecosystem.module.tsapps/ecosystem/src/main.tsapps/geo-location/src/geo-location.module.tsapps/geo-location/src/main.tsapps/issuance/src/issuance.module.tsapps/issuance/src/main.tsapps/ledger/src/credential-definition/credential-definition.module.tsapps/ledger/src/ledger.module.tsapps/ledger/src/main.tsapps/ledger/src/schema/schema.module.tsapps/notification/src/main.tsapps/notification/src/nats/nats-subscriber.tsapps/notification/src/nats/nats.service.tsapps/notification/src/nats/pendingAckStore.tsapps/notification/src/notification.module.tsapps/oid4vc-issuance/src/main.tsapps/oid4vc-issuance/src/oid4vc-issuance.module.tsapps/oid4vc-verification/src/main.tsapps/oid4vc-verification/src/oid4vc-verification.module.tsapps/organization/src/main.tsapps/organization/src/organization.module.tsapps/user/src/main.tsapps/user/src/user.module.tsapps/utility/src/main.tsapps/utility/src/utilities.module.tsapps/verification/src/main.tsapps/verification/src/verification.module.tsapps/webhook/src/main.tsapps/webhook/src/webhook.module.tsapps/x509/src/main.tsapps/x509/src/x509.module.tslibs/common/src/common.utils.tslibs/common/src/nats.config.ts
Signed-off-by: RinkalBhojani <rinkal.bhojani@ayanworks.com>
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.env.demo (1)
249-255:⚠️ Potential issue | 🟡 MinorResolve remaining dotenv-linter warnings (key order + EOF newline).
NATS_AUTH_TYPEshould be placed beforeNATS_CREDS_FILE, and the file should end with a trailing newline to clear current lint warnings.🔧 Proposed fix
-NATS_CREDS_FILE= -# 'nkey' | 'creds' | 'usernamePassword' | 'none' NATS_AUTH_TYPE=nkey # 'nkey' | 'creds' | 'usernamePassword' | 'none' +NATS_CREDS_FILE= +# 'nkey' | 'creds' | 'usernamePassword' | 'none' NOTIFICATION_NATS_AUTH_TYPE= ENABLE_NATS_NOTIFICATION=false +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.demo around lines 249 - 255, The dotenv file has lint warnings due to key order and missing EOF newline: move the NATS_AUTH_TYPE entry above NATS_CREDS_FILE (so NATS_AUTH_TYPE appears before NATS_CREDS_FILE), ensure NOTIFICATION_NATS_AUTH_TYPE remains in its intended position, and add a trailing newline at the end of the file so the file ends with a single EOF newline to satisfy dotenv-linter.
🧹 Nitpick comments (2)
.env.sample (1)
277-283: Address dotenv-linter warnings in the new NATS env block.Please reorder
NATS_AUTH_TYPEbeforeNATS_CREDS_FILEand add a trailing blank line at EOF to keep.env.samplelint-clean.Proposed diff
-PULL_CONSUMER=hub-pull-consumer - -NATS_CREDS_FILE=/platform/app_user.creds -# 'nkey' | 'creds' | 'usernamePassword' | 'none' -NATS_AUTH_TYPE=nkey +PULL_CONSUMER=hub-pull-consumer + +# 'nkey' | 'creds' | 'usernamePassword' | 'none' +NATS_AUTH_TYPE=nkey +NATS_CREDS_FILE=/platform/app_user.creds # 'nkey' | 'creds' | 'usernamePassword' | 'none' NOTIFICATION_NATS_AUTH_TYPE= ENABLE_NATS_NOTIFICATION=false +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.sample around lines 277 - 283, Move the NATS env variables so NATS_AUTH_TYPE appears before NATS_CREDS_FILE (i.e., reorder the block so NATS_AUTH_TYPE, then NATS_CREDS_FILE) and ensure the file ends with a single trailing newline; update the lines containing NATS_AUTH_TYPE, NATS_CREDS_FILE, NOTIFICATION_NATS_AUTH_TYPE and ENABLE_NATS_NOTIFICATION accordingly to satisfy dotenv-linter.apps/notification/src/nats/pendingAckStore.ts (1)
34-37: Remove the impossiblemsg.infonull path.
save()already accepts aJsMsg, soinfois part of the contract here. Keeping this branch makes the real precondition (reply) harder to see. Based on learnings: In NATS.js (nats npm package), the JsMsg interface has an info property typed as DeliveryInfo (not optional), meaning msg.info is always present on JetStream messages and doesn't require null checking or non-null assertions. (nats-io.github.io)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/notification/src/nats/pendingAckStore.ts` around lines 34 - 37, Remove the impossible null-check for msg.info in save(): since save() accepts a JsMsg and JsMsg.info is non-optional, delete the `if (!info) { throw ... }` branch so the real precondition (msg.reply) is obvious; ensure any downstream uses of `info` use it directly (e.g., in save() and any callers) and add a brief comment noting that JsMsg.info is always present per the nats JsMsg contract.
🤖 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/notification/src/nats/pendingAckStore.ts`:
- Around line 25-27: getKV() assumes the KV bucket exists (KV_BUCKET) and calls
NatsService.getKV() which only binds to this.js.views.kv(bucketName); to fix,
add an explicit provisioning or startup validation: either create the KV bucket
during NATS bootstrap (mirror ensureStream() behavior) or add a startup check
that calls NatsService.createOrEnsureKV(KV_BUCKET) (or similar) and fail fast if
creation/binding fails; update pendingAckStore.getKV(), and ensure NatsService
exposes a method to create/ensure a KV bucket so save(), delete(), and get()
won't throw or silently fail.
- Around line 59-80: The get(key: PendingKey) function is incorrectly casting
mockMsg to JsMsg; define and export a new PendingAckHandle interface that
explicitly lists the properties you intend to provide (e.g., reply?: string,
ack(): Promise<void>, nak?(delay?: number): Promise<void>) and change get() to
return Promise<PendingAckHandle | undefined>; remove the JsMsg cast on mockMsg,
ensure mockMsg matches PendingAckHandle, and update the consumer in
nats-subscriber.ts to import/use PendingAckHandle instead of JsMsg so the
contract is explicit and safe.
---
Duplicate comments:
In @.env.demo:
- Around line 249-255: The dotenv file has lint warnings due to key order and
missing EOF newline: move the NATS_AUTH_TYPE entry above NATS_CREDS_FILE (so
NATS_AUTH_TYPE appears before NATS_CREDS_FILE), ensure
NOTIFICATION_NATS_AUTH_TYPE remains in its intended position, and add a trailing
newline at the end of the file so the file ends with a single EOF newline to
satisfy dotenv-linter.
---
Nitpick comments:
In @.env.sample:
- Around line 277-283: Move the NATS env variables so NATS_AUTH_TYPE appears
before NATS_CREDS_FILE (i.e., reorder the block so NATS_AUTH_TYPE, then
NATS_CREDS_FILE) and ensure the file ends with a single trailing newline; update
the lines containing NATS_AUTH_TYPE, NATS_CREDS_FILE,
NOTIFICATION_NATS_AUTH_TYPE and ENABLE_NATS_NOTIFICATION accordingly to satisfy
dotenv-linter.
In `@apps/notification/src/nats/pendingAckStore.ts`:
- Around line 34-37: Remove the impossible null-check for msg.info in save():
since save() accepts a JsMsg and JsMsg.info is non-optional, delete the `if
(!info) { throw ... }` branch so the real precondition (msg.reply) is obvious;
ensure any downstream uses of `info` use it directly (e.g., in save() and any
callers) and add a brief comment noting that JsMsg.info is always present per
the nats JsMsg contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d80624c0-e600-4e6a-835c-0ece7df3b209
📒 Files selected for processing (6)
.env.demo.env.sampleapps/notification/src/nats/nats-subscriber.tsapps/notification/src/nats/pendingAckStore.tsapps/oid4vc-verification/src/oid4vc-verification.service.tslibs/common/src/nats.config.ts
✅ Files skipped from review due to trivial changes (1)
- apps/oid4vc-verification/src/oid4vc-verification.service.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/notification/src/nats/nats-subscriber.ts
- libs/common/src/nats.config.ts
Comment on lines
+25
to
+27
| private async getKV(): Promise<KV> { | ||
| return this.natsService.getKV(this.KV_BUCKET); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "References to the bucket name:"
rg -n -C2 '\bPENDING_ACK\b'
echo
echo "JetStream/KV initialization paths:"
rg -n -C3 'views\.kv\s*\(|getKV\s*\(|jetstreamManager\s*\('Repository: credebl/platform
Length of output: 6265
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Finding ensureStream and related functions ==="
fd -t f '\.ts$' -x grep -l 'ensureStream\|ensureBucket\|ensureConsumer' {} \;
echo
echo "=== Examining ensureStream implementation ==="
rg -n 'export.*ensureStream|function ensureStream|const ensureStream' -A 15
echo
echo "=== Checking nats.module.ts around initialization ==="
rg -n 'ensureStream|ensureConsumer|ensureBucket' apps/notification/src/nats/nats.module.ts -B 3 -A 3Repository: credebl/platform
Length of output: 253
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== jetstream.setup.ts full content ==="
cat -n apps/notification/src/nats/jetstream.setup.ts
echo
echo "=== Checking for bucket-related functions across NATS folder ==="
rg -n 'kv\(|bucket|PENDING_ACK|createBucket' apps/notification/src/nats/ --type tsRepository: credebl/platform
Length of output: 5523
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full nats.module.ts initialization ==="
cat -n apps/notification/src/nats/nats.module.ts | head -80
echo
echo "=== Check if there are any bucket-related initialization patterns elsewhere ==="
rg -n 'buckets\.|views\.kv|createBucket|bucket\.create' apps/notification/ --type tsRepository: credebl/platform
Length of output: 2218
Verify PENDING_ACK is provisioned before first use.
getKV() currently assumes the bucket already exists. NatsService.getKV() only binds via this.js.views.kv(bucketName), which will fail if the bucket is missing. The codebase provisions JetStream streams via ensureStream() during startup, but there is no equivalent bucket provisioning. Missing the bucket causes save() and delete() to throw, and get() to return undefined after logging. Either create the bucket during NATS bootstrap or fail fast with an explicit startup check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/notification/src/nats/pendingAckStore.ts` around lines 25 - 27, getKV()
assumes the KV bucket exists (KV_BUCKET) and calls NatsService.getKV() which
only binds to this.js.views.kv(bucketName); to fix, add an explicit provisioning
or startup validation: either create the KV bucket during NATS bootstrap (mirror
ensureStream() behavior) or add a startup check that calls
NatsService.createOrEnsureKV(KV_BUCKET) (or similar) and fail fast if
creation/binding fails; update pendingAckStore.getKV(), and ensure NatsService
exposes a method to create/ensure a KV bucket so save(), delete(), and get()
won't throw or silently fail.
Comment on lines
+59
to
+80
| async get(key: PendingKey): Promise<JsMsg | undefined> { | ||
| try { | ||
| const kv = await this.getKV(); | ||
| const entry: KvEntry | null = await kv.get(key); | ||
|
|
||
| if (!entry) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const metadata: PendingMessageMetadata = JSON.parse(new TextDecoder().decode(entry.value)); | ||
|
|
||
| const mockMsg = { | ||
| reply: metadata.reply, | ||
| ack: async () => { | ||
| const nc = await this.natsService.connect(); | ||
| nc.publish(metadata.reply); | ||
| }, | ||
| nak: async (delay?: number) => { | ||
| const nc = await this.natsService.connect(); | ||
| nc.publish(metadata.reply, Buffer.from(`-NAK${delay ? ` ${delay}` : ''}`)); | ||
| } | ||
| } as unknown as JsMsg; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the file and understand the full context
find . -type f -name "pendingAckStore.ts" | head -5Repository: credebl/platform
Length of output: 107
🏁 Script executed:
# Look for call sites that use the result of pendingAckStore.get()
rg "\.get\(" --type typescript -A 5 | grep -A 5 "pendingAckStore"Repository: credebl/platform
Length of output: 94
🏁 Script executed:
# Search for accesses to msg.subject, msg.info, msg.seq, msg.redelivered in the codebase
rg "msg\.(subject|info|seq|redelivered)" --type typescript -B 2 -A 2Repository: credebl/platform
Length of output: 94
🏁 Script executed:
# Check the PendingMessageMetadata interface definition
rg "interface PendingMessageMetadata|type PendingMessageMetadata" --type typescript -A 10Repository: credebl/platform
Length of output: 94
🏁 Script executed:
# View the complete pendingAckStore.ts file to understand all usages
wc -l apps/notification/src/nats/pendingAckStore.tsRepository: credebl/platform
Length of output: 108
🏁 Script executed:
# Search for usages of pendingAckStore.get() without file type constraint
rg "pendingAckStore" -A 10 | grep -E "(get\(|\.get\(|pendingAckStore)"Repository: credebl/platform
Length of output: 911
🏁 Script executed:
# Look for any pattern where get() return value is used
rg "\.get\(" -A 5 --glob="*.ts" | head -50Repository: credebl/platform
Length of output: 2853
🏁 Script executed:
# View the complete pendingAckStore.ts file
cat -n apps/notification/src/nats/pendingAckStore.tsRepository: credebl/platform
Length of output: 3203
🏁 Script executed:
# Search for accesses to msg properties in the entire codebase
rg "msg\.(subject|info|seq|redelivered|reply|ack|nak)" --glob="*.ts" -B 2 -A 2Repository: credebl/platform
Length of output: 3933
🏁 Script executed:
# Look for where the returned msg from pendingAckStore.get() is actually used
rg "await this\.pendingAckStore\.get" -B 2 -A 10 --glob="*.ts"Repository: credebl/platform
Length of output: 1070
🏁 Script executed:
# Check the nats-subscriber.ts file to see full context of msg usage
cat -n apps/notification/src/nats/nats-subscriber.ts | grep -A 30 "pendingAckStore.get"Repository: credebl/platform
Length of output: 630
🏁 Script executed:
# Verify if there are any other accesses to msg properties after get() call
rg "pendingAckStore.get" -A 20 --glob="*.ts"Repository: credebl/platform
Length of output: 1254
Remove the JsMsg cast and create a proper PendingAckHandle interface.
The mockMsg object is cast to JsMsg but lacks required properties: info (which is always present on actual JsMsg), seq, and redelivered. While the current call site in nats-subscriber.ts only uses msg.ack() (which is provided), the cast is fundamentally incorrect and masks a contract mismatch. If future code accesses msg.subject or msg.info, it will fail at runtime. Create a dedicated PendingAckHandle interface that honestly declares what this object provides, and update the return type accordingly.
Suggested fix
+interface PendingAckHandle {
+ reply: string;
+ subject: string;
+ streamSequence: number;
+ ack(): Promise<void>;
+ nak(delay?: number): Promise<void>;
+}
+
- async get(key: PendingKey): Promise<JsMsg | undefined> {
+ async get(key: PendingKey): Promise<PendingAckHandle | undefined> {
try {
const kv = await this.getKV();
const entry: KvEntry | null = await kv.get(key);
if (!entry) {
return undefined;
}
const metadata: PendingMessageMetadata = JSON.parse(new TextDecoder().decode(entry.value));
- const mockMsg = {
+ return {
reply: metadata.reply,
+ subject: metadata.subject,
+ streamSequence: metadata.streamSequence,
ack: async () => {
const nc = await this.natsService.connect();
nc.publish(metadata.reply);
},
nak: async (delay?: number) => {
const nc = await this.natsService.connect();
nc.publish(metadata.reply, Buffer.from(`-NAK${delay ? ` ${delay}` : ''}`));
}
- } as unknown as JsMsg;
-
- return mockMsg;
+ };
} catch (error) {
this.logger.warn(`Failed to retrieve pending ACK for key ${key}`, error);
return undefined;
}
}Export PendingAckHandle and update the call site in nats-subscriber.ts to depend on it explicitly.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 78-78: Refactor this code to not use nested template literals.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/notification/src/nats/pendingAckStore.ts` around lines 59 - 80, The
get(key: PendingKey) function is incorrectly casting mockMsg to JsMsg; define
and export a new PendingAckHandle interface that explicitly lists the properties
you intend to provide (e.g., reply?: string, ack(): Promise<void>, nak?(delay?:
number): Promise<void>) and change get() to return Promise<PendingAckHandle |
undefined>; remove the JsMsg cast on mockMsg, ensure mockMsg matches
PendingAckHandle, and update the consumer in nats-subscriber.ts to import/use
PendingAckHandle instead of JsMsg so the contract is explicit and safe.
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
2 participants
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
Documentation