refactor: codebase cleanup, standardization & performance improvements#89
Merged
Conversation
❌ PR Review FailedError DetailsThe PR review system encountered an error while analyzing this pull request: What to do next
|
📝 WalkthroughWalkthroughIntroduces a centralized Env utility, normalizes status codes in ChangesEnvironment Management and HTTP Status Normalization Refactor
Stellar SDK Integration Migration
Sequence Diagram(s)sequenceDiagram
participant Client
participant Route as Controller/Route
participant Env as Env Utility
participant StellarSvc as StellarService
participant Horizon as Horizon Server
Client->>Route: HTTP request (withdraw/transfer/swap)
activate Route
Route->>Env: Env.stellarMasterSecretKey(true)
Env-->>Route: master secret
Route->>StellarSvc: transferAsset(sourceSecret, sendAsset, destAsset, amount, memo)
activate StellarSvc
StellarSvc->>Env: Env.* (stellar server config/assets)
Env-->>StellarSvc: horizonUrl, networkPassphrase, Asset instances
StellarSvc->>Horizon: loadAccount / fetchBaseFee
Horizon-->>StellarSvc: account + fee
StellarSvc->>StellarSvc: build Transaction (Operation.payment/pathPayment)
StellarSvc->>StellarSvc: sign tx (Keypair)
StellarSvc->>Horizon: submitTransaction(tx)
Horizon-->>StellarSvc: txResult (hash)
StellarSvc-->>Route: txHash / result
Route-->>Client: HTTP 200 (STATUS_CODES.OK) + payload
deactivate Route
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ Created PR with unit tests: #90 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
api/services/github-webhook/issue_comment-webhook.service.ts (1)
511-533:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMisleading user-facing error message in the outer async catch.
The message
"Bounty was created but there was an issue while posting the bounty comment or adding the bounty label on issue"is only accurate when the outercatchfires after task creation and escrow commitment. However, several earlier unguarded call sites can also propagate to this outer handler — for example,stellarService.getAccountInfo(line 340),KMSService.decryptWallet(line 358), andprisma.task.create(line 379) are not wrapped in their own try/catch blocks. If any of those throws, the outer catch fires while no bounty was ever created, and the message tells the user the opposite.The generic fallback that was used before this PR —
"Bounty creation failed due to an internal error. Please try again on the DevAsign dashboard."— is accurate for any failure stage and should be kept (or passed explicitly) in the outer catch. Move the specific "Bounty was created but..." message to only the inner catch where the GitHub comment/label mutation fails (lines 458–469), where it is already correct.🛠️ Proposed fix
- await this.bountyFailureComment( - installationId, - repository.full_name, - issue.number, - "Bounty was created but there was an issue while posting the bounty comment or adding the bounty label on issue" - ); + // No explicit message: defaults to the generic internal-error text, + // which is accurate regardless of which step failed. + await this.bountyFailureComment( + installationId, + repository.full_name, + issue.number + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/services/github-webhook/issue_comment-webhook.service.ts` around lines 511 - 533, The outer catch currently posts a user-facing message ("Bounty was created but...") that is only correct when the inner GitHub comment/label mutation fails; change the outer catch to use the generic fallback message ("Bounty creation failed due to an internal error. Please try again on the DevAsign dashboard.") and ensure statsigService.logEvent and dataLogger.error remain unchanged; move the specific "Bounty was created but there was an issue while posting the bounty comment or adding the bounty label on issue" message so it is only used in the inner catch that wraps the GitHub comment/label operations (the block that calls bountyFailureComment), and verify that earlier calls like stellarService.getAccountInfo, KMSService.decryptWallet, and prisma.task.create are allowed to bubble to the outer handler which now uses the generic message.api/controllers/installation/github.controller.ts (1)
122-129:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHardcoded
status: 200missed in the migration.All other
responseWrappercalls in this file useSTATUS_CODES.OK, but the early-return branch for the existing bounty label still uses a literal200, leaving one raw magic number the PR explicitly set out to remove.🛠️ Proposed fix
- if (bountyLabel) { - return responseWrapper({ - res, - status: 200, - data: { bountyLabel }, - message: "Bounty label already exists" - }); - } + if (bountyLabel) { + return responseWrapper({ + res, + status: STATUS_CODES.OK, + data: { bountyLabel }, + message: "Bounty label already exists" + }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/controllers/installation/github.controller.ts` around lines 122 - 129, The early-return branch that returns when bountyLabel exists is using a hardcoded literal 200; update that responseWrapper call to use STATUS_CODES.OK (consistent with other calls in this controller) and ensure STATUS_CODES is imported/available in the module so the call becomes status: STATUS_CODES.OK instead of status: 200.tests/integration/api/task/{taskId}.api.test.ts (1)
972-1003:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse AuthorizationError for authorization failures instead of ValidationError.
Lines 665–668 in
requestTimelineExtensionand lines 731–734 inreplyTimelineExtensionRequestthrowValidationErrorwhen authorization fails (user is not the contributor/creator), causing the controllers to return HTTP 400. Other task endpoints correctly useAuthorizationErrorfor authorization checks, which returns HTTP 403 (seeaddBountyCommentIdline 59 andupdateTaskBountyline 144).The authorization check should be separated from the status validation check: throw
AuthorizationErrorfor "user is not contributor/creator" andValidationErrorfor "task is not in progress". This aligns with RFC 9110 semantics where 403 Forbidden indicates the request is understood but denied due to authorization, while 400 Bad Request should indicate malformed syntax or invalid data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/api/task/`{taskId}.api.test.ts around lines 972 - 1003, In requestTimelineExtension and replyTimelineExtensionRequest, replace the ValidationError thrown for authorization failures (the checks that verify "user is contributor" or "user is creator") with AuthorizationError while keeping ValidationError for status/state validation (e.g., "task is not in progress"); locate the checks in the requestTimelineExtension and replyTimelineExtensionRequest functions and swap the thrown error type to AuthorizationError (import/ensure AuthorizationError is available) so authorization failures return 403 while other validation issues still throw ValidationError and return 400.tests/integration/api/webhook/github.api.test.ts (1)
537-570:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWebhook validation errors should return 4xx, not 500.
The webhook validation errors in lines 551, 566, 1615, 1640, and 1662 return
STATUS_CODES.INTERNAL_SERVER_ERROR(500) for:
- Missing webhook signature
- Invalid webhook signature
- Webhook secret not configured
- Invalid request body format
These are all client/configuration errors that should return 4xx status codes (e.g.,
400 BAD_REQUESTfor malformed body/signature, or401 UNAUTHORIZEDfor missing credentials). Using 500 misrepresents the issue as a server fault when the actual problem is with the request or client configuration.The root cause is in the
GitHubWebhookErrorclass (error.model.ts, lines 243-252) which hardcodesSTATUS_CODES.INTERNAL_SERVER_ERRORas the default status. Update it to accept a configurable status parameter in the constructor, then pass the appropriate 4xx code when throwing these validation errors from the webhook middleware.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/api/webhook/github.api.test.ts` around lines 537 - 570, Update the GitHubWebhookError class (error.model.ts / GitHubWebhookError) to accept an HTTP status code parameter in its constructor (instead of hardcoding STATUS_CODES.INTERNAL_SERVER_ERROR) and store it as the error's status; then update the webhook middleware/validation sites that throw this error (the places handling missing signature, invalid signature, missing webhook secret, and invalid body format) to pass appropriate 4xx codes (e.g., STATUS_CODES.BAD_REQUEST for malformed body/signature, STATUS_CODES.UNAUTHORIZED for missing/invalid signature or secret). Also update the tests expecting STATUS_CODES.INTERNAL_SERVER_ERROR to assert the new 4xx codes so behavior and test expectations align.api/services/socket.service.ts (1)
38-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExtract CORS origins outside the connection callback to avoid uncaught exceptions
Env.corsOrigins(true)throws whenCORS_ORIGINSis unset. Since it's called inside theorigincallback — which thecorspackage invokes synchronously without atry/catchwrapper — any exception propagates unhandled. Additionally, calling it per-connection defeats the caching intent; the origins list should be captured once at initialization.🛡️ Proposed fix — pre-compute origins at initialization
static initialize(httpServer: HttpServer) { + const allowedOrigins = Env.corsOrigins(true)!; this.io = new SocketIOServer(httpServer, { cors: { origin(origin, callback) { - if (!origin || Env.corsOrigins(true)!.includes(origin)) { + if (!origin || allowedOrigins.includes(origin)) { callback(null, true); } else { callback(new Error("Not allowed by CORS")); } }, methods: ["GET", "POST"] } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/services/socket.service.ts` around lines 38 - 50, Pre-compute and cache the allowed origins before constructing the SocketIOServer to avoid uncaught exceptions and per-connection calls: call Env.corsOrigins(true) once inside SocketService.initialize (store result in a const like allowedOrigins), guard that call with try/catch to handle the unset CORS_ORIGINS case (log or fallback to an empty array), then update the cors.origin callback to reference allowedOrigins (e.g., if (!origin || allowedOrigins.includes(origin)) callback(null,true) else callback(new Error("Not allowed by CORS"))), removing any further calls to Env.corsOrigins from inside the callback.api/services/cloud-tasks.service.ts (1)
76-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winField initializer throws before the constructor's soft-fail aggregation can run.
Public instance fields are added to the instance at construction time in the base class — before the constructor body runs. Because
Env.gcpProjectId(true),Env.gcpLocationId(true),Env.cloudRunServiceUrl(true),Env.cloudRunPrivateServiceUrl(true), andEnv.cloudTasksServiceAccountEmail(true)all callgetOrThrowError(which throws anErrorClasswhen the variable is absent), any single missing top-level env var causes the field initializer to throw and the constructor body at lines 107–125 to never execute. The carefully-builtmissing[]array and the consolidated log message are completely bypassed.Two secondary problems follow from this:
- Dead
|| ""on line 81.getOrThrowErroreither throws (var absent) or returns a non-empty string (var present); the|| ""fallback is unreachable in both cases.- Inconsistent error surface. Queue vars (lines 83–89) use
throwError=falseand flow through the constructor's aggregation, while the top-level vars throw rawErrorClassexceptions before aggregation can run.Fix — pick one:
Option A: Remove
throwError=truefrom field initializers so all validation flows through the constructor's aggregation block.- project: Env.gcpProjectId(true)!, - location: Env.gcpLocationId(true)!, - cloudRunServiceUrl: Env.cloudRunServiceUrl(true)!, - cloudRunPrivateServiceUrl: Env.cloudRunPrivateServiceUrl(true)!, - cloudTasksServiceAccountEmail: (Env.cloudTasksServiceAccountEmail(true) || ""), + project: Env.gcpProjectId() || "", + location: Env.gcpLocationId() || "", + cloudRunServiceUrl: Env.cloudRunServiceUrl() || "", + cloudRunPrivateServiceUrl: Env.cloudRunPrivateServiceUrl() || "", + cloudTasksServiceAccountEmail: Env.cloudTasksServiceAccountEmail() || "",Option B: Remove the constructor's soft-fail block and throw consistently (rely on field initializer throws).
constructor() { - const missing: string[] = []; - if (!Env.gcpProjectId()) missing.push("GCP_PROJECT_ID"); - if (!Env.gcpLocationId()) missing.push("GCP_LOCATION_ID"); - if (!Env.cloudRunServiceUrl()) missing.push("CLOUD_RUN_SERVICE_URL"); - if (!Env.cloudRunPrivateServiceUrl()) missing.push("CLOUD_RUN_PRIVATE_SERVICE_URL"); - if (!((Env.cloudTasksServiceAccountEmail() || "") || "")) missing.push("CLOUD_TASKS_SERVICE_ACCOUNT_EMAIL"); - if (!Env.cloudTasksPrAnalysisQueue()) missing.push("CLOUD_TASKS_PR_ANALYSIS_QUEUE"); - if (!Env.cloudTasksManualPrAnalysisQueue()) missing.push("CLOUD_TASKS_MANUAL_PR_ANALYSIS_QUEUE"); - if (!Env.cloudTasksRepoIndexingQueue()) missing.push("CLOUD_TASKS_REPO_INDEXING_QUEUE"); - if (!Env.cloudTasksIncrementalIndexingQueue()) missing.push("CLOUD_TASKS_INCREMENTAL_INDEXING_QUEUE"); - if (!Env.cloudTasksBountyPayoutQueue()) missing.push("CLOUD_TASKS_BOUNTY_PAYOUT_QUEUE"); - if (!Env.cloudTasksClearInstallationQueue()) missing.push("CLOUD_TASKS_CLEAR_INSTALLATION_QUEUE"); - if (!Env.cloudTasksClearRepoQueue()) missing.push("CLOUD_TASKS_CLEAR_REPO_QUEUE"); - if (missing.length > 0) { - dataLogger.error(`Missing required environment variables for CloudTasksService: ${missing.join(", ")}`); - } this.client = new CloudTasksClient(); }And change all queue entries to also use
throwError=true:- "pr-analysis": Env.cloudTasksPrAnalysisQueue() || "", + "pr-analysis": Env.cloudTasksPrAnalysisQueue(true)!, // ...Also applies to: 107-125
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/services/cloud-tasks.service.ts` around lines 76 - 100, The config field initializer calls Env.gcpProjectId(true), Env.gcpLocationId(true), Env.cloudRunServiceUrl(true), Env.cloudRunPrivateServiceUrl(true) and Env.cloudTasksServiceAccountEmail(true) which throw before the constructor's missing[] aggregation runs; move the entire config construction out of the public instance field and into the class constructor after the existing validation/aggregation block (i.e., build this.config inside the constructor using the validated Env values or Env.*(..., throwError=false) so all missing envs are collected consistently), and make the queue entries consistent with the chosen approach (either use throwError=false in those Env calls or ensure they also throw inside the constructor) so no field initializer can throw before aggregation.api/controllers/user/index.controller.ts (1)
155-163:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
meta: { error }passes a rawErrorobject — useerror.messagefor consistency.The rest of the catch block (lines 150–152) already extracts
error instanceof Error ? error.message : "Unknown Error"for the Statsig event. Themetafield here diverges from that pattern. A rawErrorobject serializes to{}in JSON (all its properties are non-enumerable), so the field is both useless to the client and inconsistent.✏️ Suggested fix
responseWrapper({ res, status: STATUS_CODES.OK, data: user, message: "User created successfully", warning: "Failed to add USDC trustline to wallet", - meta: { error } + meta: { error: error instanceof Error ? error.message : "Unknown error" } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/controllers/user/index.controller.ts` around lines 155 - 163, The meta field is passing a raw Error object (meta: { error }) which JSON-serializes to {} and is inconsistent with the Statsig extraction; update the catch block to pass the error message string instead (e.g., meta: { error: error instanceof Error ? error.message : String(error) }) when calling responseWrapper so clients receive a useful message; locate the responseWrapper call in the same catch handler around the user creation flow and replace meta: { error } with the message extraction expression (or an existing errorMessage variable) to keep behavior consistent.tests/integration/api/installation/team.api.test.ts (1)
234-246:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest descriptions claim "404" but assertions expect 403 FORBIDDEN.
All three
"should return 404 when installation not found"blocks assertSTATUS_CODES.FORBIDDEN(403). The description will actively mislead anyone debugging a future failure.✏️ Suggested fix (repeat for lines 330 and 441)
- it("should return 404 when installation not found", async () => { + it("should return 403 when installation not found", async () => {Also applies to: 330-342, 441-448
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/api/installation/team.api.test.ts` around lines 234 - 246, The test descriptions say "404" but the assertions use STATUS_CODES.FORBIDDEN (403); update the three failing tests' description strings (the it(...) blocks that call getEndpointWithPrefix([...]) and assert .expect(STATUS_CODES.FORBIDDEN)) to say "should return 403 when installation not found" (repeat the same change for the other two occurrences noted), so the human-readable message matches the actual expectation.api/index.ts (1)
47-65:⚠️ Potential issue | 🟠 MajorAdd
CORS_ORIGINSto startup environment validation to prevent silent crash on missing configuration.
corsOrigins(true)is called at module initialization (api/index.ts:47), and whenCORS_ORIGINSis not configured,getOrThrowError()throws an exception that crashes the process at startup with no prior validation warning.
CORS_ORIGINSis not included inrequiredVarsorwarningVarsinerror-handler.service.ts(lines 132–165), so missing configuration is not caught during the startup validation routine. This creates a silent failure mode: the app crashes with a server misconfiguration error instead of a clear, logged configuration error.Either add
CORS_ORIGINSto the validation checks or make it optional with a sensible default (e.g.,[]to deny all cross-origin requests by default).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/index.ts` around lines 47 - 65, The startup crash happens because Env.corsOrigins(true) (corsOrigins) is invoked at module init but CORS_ORIGINS isn't validated; update the startup validation to include "CORS_ORIGINS" (add it to the requiredVars or warningVars list in the error-handler service where startup envs are validated) or make corsOrigins optional by changing the call to Env.corsOrigins(false) / defaulting to [] so missing CORS_ORIGINS doesn't throw; adjust the validation arrays (requiredVars/warningVars) or the corsOrigins invocation accordingly and keep the CORS behavior consistent with the intended default (deny all or explicit allow-list).api/middlewares/auth.middleware.ts (1)
94-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove env reads inside the
tryblock to prevent unhandled promise rejection if variables are missing.In Express 4, synchronous errors thrown inside async middleware/route handlers that occur outside a
try/catchbecome unhandled promise rejections. IfCLOUD_TASKS_SERVICE_ACCOUNT_EMAILorCLOUD_RUN_SERVICE_URLare missing in production, the env calls at lines 102–104 throw synchronously, creating a rejected promise that Express 4 does not automatically catch. This causes the client to hang indefinitely waiting for a response.Move both env reads into the
tryblock so the existingcatchhandler properly callsnext(new AuthorizationError(...))on failure.Suggested fix
try { + const cloudTasksServiceAccountEmail = Env.cloudTasksServiceAccountEmail(true)!; + const cloudRunServiceUrl = Env.cloudRunServiceUrl(true)!; + // Extract the Bearer token from the Authorization header const authHeader = req.headers.authorization; if (!authHeader || !authHeader.startsWith("Bearer ")) {Remove the duplicate declarations at lines 102–104.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/middlewares/auth.middleware.ts` around lines 94 - 156, The env reads for cloudTasksServiceAccountEmail and cloudRunServiceUrl are done outside the try block in validateCloudTasksRequest which can throw synchronously and produce unhandled promise rejections; move the calls to Env.cloudTasksServiceAccountEmail(true) and Env.cloudRunServiceUrl(true) into the existing try block (remove the earlier top-level declarations) so any thrown errors are caught by the catch and routed to next(new AuthorizationError(...)); keep the rest of the verification logic unchanged (authClient.verifyIdToken, payload checks, dataLogger usage).api/controllers/installation/index.controller.ts (1)
170-201:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't downgrade missing Stellar credentials into a recoverable warning.
Line 173 can now throw a server-misconfiguration error, but this catch block turns it into a successful installation response with only a warning. That hides a deployment problem and leaves the new wallet without its required trustline. Resolve the secret before this partial-success path, or rethrow config errors here instead of treating them like transient trustline failures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/controllers/installation/index.controller.ts` around lines 170 - 201, The catch currently turns any error from resolving secrets or adding the USDC trustline into a recoverable warning; instead, ensure missing/misconfigured Stellar credentials are treated as fatal: resolve and validate Env.stellarMasterSecretKey(true) before the try (or immediately throw if it returns falsy), or in the catch rethrow errors that indicate configuration issues (e.g., missing secret or a ConfigError) rather than returning a 200 with a warning. Update the block around Env.stellarMasterSecretKey(true) and stellarService.addTrustLineViaSponsor so that only transient trustline failures are converted to a partial-success response while credential/config errors propagate as server errors.
🧹 Nitpick comments (4)
api/controllers/wallet/index.controller.ts (1)
104-119: ⚡ Quick win
usdcAssetlocal variable shadows the module-level import
const usdcAssetdeclared here (and again at line 267 inswapAsset) shadows theusdcAssetimported fromstellar.config.js. Both are correctly block-scoped so there's no runtime mixup at the current call sites, but the naming collision makes the code fragile — a future refactor that moves or hoists the variable could silently pass a balance object where a StellarAssetis expected.♻️ Suggested rename
- const usdcAsset = accountInfo.balances.find( + const usdcBalance = accountInfo.balances.find( (asset): asset is USDCBalance => "asset_code" in asset && asset.asset_code === "USDC" ); - if (!usdcAsset) { + if (!usdcBalance) { throw new ValidationError("No USDC asset found in wallet"); } - if (parseFloat(usdcAsset.balance) < Number(amount)) { + if (parseFloat(usdcBalance.balance) < Number(amount)) { throw new ValidationError( "Insufficient USDC balance", - { available: usdcAsset.balance } + { available: usdcBalance.balance } ); }Apply the same rename to the
elseblock inswapAsset(around line 267).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/controllers/wallet/index.controller.ts` around lines 104 - 119, The local constant usdcAsset in the balance-check block should be renamed to avoid shadowing the module-level usdcAsset import from stellar.config.js; change the local variable name (e.g., to usdcBalance or usdcWalletBalance) wherever it’s declared and referenced in the balance-checking logic inside the controller, and apply the same rename to the corresponding local declaration in the swapAsset function’s else block so both places stop using the imported symbol name while keeping all type guards and usages intact.api/services/kms.service.ts (1)
8-16: ⚡ Quick winRedundant fallbacks undermine the module-level guard's intent
Line 13 applies two successive
|| ""fallbacks toEnv.gcpProjectId(true), even though the guard at line 8 already throws ifgcpProjectIdis absent. At runtimePROJECT_IDwill always be a non-empty string here; the double fallback suggests distrust of the guard and is inconsistent with lines 14–16, which use the cleaner!non-null assertion pattern. Similarly, the|| ""inside the guard condition on line 8 is equivalent to just!Env.gcpProjectId()(empty string is already falsy).♻️ Proposed cleanup
-if (!(Env.gcpProjectId() || "") || !Env.gcpLocationId() || !Env.gcpKeyRingId() || !Env.gcpKeyId()) { +if (!Env.gcpProjectId() || !Env.gcpLocationId() || !Env.gcpKeyRingId() || !Env.gcpKeyId()) { throw new KmsServiceError("Missing GCP KMS credentials (GCP_PROJECT_ID, GCP_LOCATION_ID, GCP_KEY_RING_ID, GCP_KEY_ID) in environment variables"); } -const PROJECT_ID = ((Env.gcpProjectId(true) || "") || ""); +const PROJECT_ID = Env.gcpProjectId(true)!; const LOCATION_ID = Env.gcpLocationId(true)!; const KEY_RING_ID = Env.gcpKeyRingId(true)!; const KEY_ID = Env.gcpKeyId(true)!;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/services/kms.service.ts` around lines 8 - 16, The guard in the KmsService module redundantly uses `|| ""` fallbacks and an extra `|| ""` inside the initial presence check; simplify the guard and use consistent non-null assertions: update the guard that throws KmsServiceError to check presence with straightforward falsy checks (e.g., `if (!Env.gcpProjectId() || !Env.gcpLocationId() || !Env.gcpKeyRingId() || !Env.gcpKeyId()) { ... }`) and set PROJECT_ID using the same pattern as the others (e.g., `const PROJECT_ID = Env.gcpProjectId(true)!;`) removing the double `|| ""` fallbacks so PROJECT_ID, LOCATION_ID, KEY_RING_ID, and KEY_ID are handled consistently.api/services/contract.service.ts (1)
19-43: 💤 Low valueRedundant
|| ""wrappers in both the static validation block andCONFIGinitialization.Lines 22–23 use
!(Env.stellarNetwork() || "")which is logically identical to!Env.stellarNetwork()— the|| ""convertsundefined→""and""→"", but!undefinedand!""are bothtrue, so the result is the same in every case. Lines 24–26 correctly omit it, creating an unnecessary inconsistency.Lines 34–35 double the redundancy with
((Env.stellarNetwork(true) || "") || ""). WiththrowError=true,getOrThrowErroreither throws (so the fallback never runs) or returns the string value (so|| ""never applies). The expression simplifies toEnv.stellarNetwork(true)!.♻️ Suggested simplification
- if (!(Env.stellarNetwork() || "")) missing.push("STELLAR_NETWORK"); - if (!(Env.stellarRpcUrl() || "")) missing.push("STELLAR_RPC_URL"); + if (!Env.stellarNetwork()) missing.push("STELLAR_NETWORK"); + if (!Env.stellarRpcUrl()) missing.push("STELLAR_RPC_URL");- network: ((Env.stellarNetwork(true) || "") || ""), - rpcUrl: ((Env.stellarRpcUrl(true) || "") || ""), + network: Env.stellarNetwork(true)!, + rpcUrl: Env.stellarRpcUrl(true)!,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/services/contract.service.ts` around lines 19 - 43, The static initialization uses redundant "|| ''" fallbacks and inconsistent checks: remove the unnecessary "|| ''" wrappers in the static validation block (replace conditions like !(Env.stellarNetwork() || "") with !Env.stellarNetwork()) and simplify CONFIG initializers to directly use the thrown-or-present values (replace expressions like ((Env.stellarNetwork(true) || "") || "") with Env.stellarNetwork(true)!, Env.stellarRpcUrl(true)!, Env.taskEscrowContractId(true)!, Env.usdcContractId(true)!, and Env.stellarMasterPublicKey(true)!), keeping the existing EscrowContractError throw logic and the networkPassphrase/maxFee computation unchanged.api/models/error.model.ts (1)
45-53: 💤 Low value
AuthorizationErrorcode"UNAUTHORIZED"conflicts with its new HTTP status 403 FORBIDDEN.This PR changed the status to
STATUS_CODES.FORBIDDEN(403) while the internalcodefield remains"UNAUTHORIZED". 401 Unauthorized and 403 Forbidden have distinct HTTP semantics — mixing the names makes error logging and client-side error-code checks misleading.♻️ Suggested fix
export class AuthorizationError extends ErrorClass { constructor(message: string, details: unknown = null) { super( - "UNAUTHORIZED", + "FORBIDDEN", details, message, STATUS_CODES.FORBIDDEN ); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/models/error.model.ts` around lines 45 - 53, The AuthorizationError class currently sets its internal code to "UNAUTHORIZED" while using STATUS_CODES.FORBIDDEN (403); update the AuthorizationError constructor to use a matching code (e.g., "FORBIDDEN") instead of "UNAUTHORIZED" so the code string aligns with the HTTP 403 status; ensure you only change the literal passed as the first argument in the AuthorizationError constructor and leave STATUS_CODES.FORBIDDEN and other parameters intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Line 63: The CORS_ORIGINS environment variable is currently wrapped in quotes
which can cause some env loaders to treat the whole string literally; update the
CORS_ORIGINS entry so the value is an unquoted comma-separated list (e.g.,
CORS_ORIGINS=http://localhost:3000,http://localhost:4000,http://localhost:3001),
ensure there are no surrounding quotes or extra spaces, and keep the variable
name CORS_ORIGINS unchanged.
In `@api/routes/agent.route.ts`:
- Around line 14-17: The x402Config fallback uses "0" which is truthy and
defeats the guard; change the fallback for Env.x402PayeeAddress() to an empty
string so x402Config.payeeAddress is falsy when the env var is missing, ensuring
the conditional if (facilitatorUrl && x402Config.payeeAddress &&
Env.x402ApiKey()) prevents registering the X402 payment middleware (the code
path that sets payTo when registering the middleware around the x402 payment
setup).
In `@api/services/stellar.service.ts`:
- Around line 35-44: The current startup validation requires
Env.stellarMasterPublicKey() even though the service never uses it; update the
check so it only validates the master secret key (Env.stellarMasterSecretKey())
and removes the dependency on Env.stellarMasterPublicKey() so startup won't
throw StellarServiceError when only the secret is present; ensure the code path
that creates this.masterAccount via Keypair.fromSecret(...) remains unchanged
and that any references to the public key are derived from this.masterAccount
instead of Env.stellarMasterPublicKey().
- Around line 399-405: Current pathPaymentStrictSend calls use a hard-coded
destMin ("0.0000001"), which allows almost-zero fills; change the code to accept
a caller-supplied destMin (derived from the quote or explicit slippage
tolerance) and use that value in the Operation.pathPaymentStrictSend call
instead of the dust constant. Update the method signatures that build these
transactions (the function that builds txBuilder and any helpers that reference
sendAsset, destAsset, amount, destinationAddress) to accept a required destMin
parameter for cross-asset path payments, validate that destMin is present and >
0 (and reject the request if missing or below an acceptable threshold), and
apply the same change to the other two occurrences noted (the other
pathPaymentStrictSend calls).
- Around line 224-229: The Friendbot fetch call in StellarService (around the
try block that requests funding with
fetch(`https://friendbot.stellar.org?addr=...`)) needs an AbortController-based
timeout so the server thread can't hang; create an AbortController, start a
timer (e.g., setTimeout) that calls controller.abort() after the desired
timeout, pass controller.signal to fetch, and in the catch/abort branch convert
the AbortError/timeout into a StellarServiceError with a clear message before
rethrowing; ensure the timer is cleared on success to avoid leaks and reference
the fetch call and the surrounding try/catch in StellarService to make the
change.
In `@api/utils/env.ts`:
- Around line 28-32: The CORS origin list returned by static corsOrigins may
contain entries with leading/trailing spaces or empty strings; modify
corsOrigins to normalize the result from both branches (the throwError branch
using this.getOrThrowError("CORS_ORIGINS").split(",") and the fallback using
process.env.CORS_ORIGINS?.split(",")) by trimming each entry and filtering out
empty strings (e.g., .map(s => s.trim()).filter(Boolean)) before returning the
array so only valid, canonical origins are returned.
- Around line 329-334: Env.port currently coerces the PORT value with
Number(...) which can yield NaN; change Env.port (and use the string from
getOrThrowError("PORT") when throwError is true) to parse and validate the port:
parse the string (e.g., parseInt/Number), ensure it's a finite integer within
the valid TCP port range (>=1 && <=65535), and throw a clear error if validation
fails; keep the existing non-throwing branch behavior but consider returning NaN
only when throwError is false to preserve backwards compatibility.
In `@tests/integration/api/webhook/sumsub.api.test.ts`:
- Line 204: The assertion uses expect(...).toContain with a duplicate array
[STATUS_CODES.INTERNAL_SERVER_ERROR, STATUS_CODES.INTERNAL_SERVER_ERROR], which
hides the intent; update the test in sumsub.api.test.ts to either assert the
single expected status with
expect(response.status).toBe(STATUS_CODES.INTERNAL_SERVER_ERROR) or, if two
different outcomes are allowed, replace the duplicate with the correct second
constant (e.g., STATUS_CODES.UNKNOWN or STATUS_CODES.UNAUTHORIZED) so the array
passed to toContain contains two distinct meaningful values.
---
Outside diff comments:
In `@api/controllers/installation/github.controller.ts`:
- Around line 122-129: The early-return branch that returns when bountyLabel
exists is using a hardcoded literal 200; update that responseWrapper call to use
STATUS_CODES.OK (consistent with other calls in this controller) and ensure
STATUS_CODES is imported/available in the module so the call becomes status:
STATUS_CODES.OK instead of status: 200.
In `@api/controllers/installation/index.controller.ts`:
- Around line 170-201: The catch currently turns any error from resolving
secrets or adding the USDC trustline into a recoverable warning; instead, ensure
missing/misconfigured Stellar credentials are treated as fatal: resolve and
validate Env.stellarMasterSecretKey(true) before the try (or immediately throw
if it returns falsy), or in the catch rethrow errors that indicate configuration
issues (e.g., missing secret or a ConfigError) rather than returning a 200 with
a warning. Update the block around Env.stellarMasterSecretKey(true) and
stellarService.addTrustLineViaSponsor so that only transient trustline failures
are converted to a partial-success response while credential/config errors
propagate as server errors.
In `@api/controllers/user/index.controller.ts`:
- Around line 155-163: The meta field is passing a raw Error object (meta: {
error }) which JSON-serializes to {} and is inconsistent with the Statsig
extraction; update the catch block to pass the error message string instead
(e.g., meta: { error: error instanceof Error ? error.message : String(error) })
when calling responseWrapper so clients receive a useful message; locate the
responseWrapper call in the same catch handler around the user creation flow and
replace meta: { error } with the message extraction expression (or an existing
errorMessage variable) to keep behavior consistent.
In `@api/index.ts`:
- Around line 47-65: The startup crash happens because Env.corsOrigins(true)
(corsOrigins) is invoked at module init but CORS_ORIGINS isn't validated; update
the startup validation to include "CORS_ORIGINS" (add it to the requiredVars or
warningVars list in the error-handler service where startup envs are validated)
or make corsOrigins optional by changing the call to Env.corsOrigins(false) /
defaulting to [] so missing CORS_ORIGINS doesn't throw; adjust the validation
arrays (requiredVars/warningVars) or the corsOrigins invocation accordingly and
keep the CORS behavior consistent with the intended default (deny all or
explicit allow-list).
In `@api/middlewares/auth.middleware.ts`:
- Around line 94-156: The env reads for cloudTasksServiceAccountEmail and
cloudRunServiceUrl are done outside the try block in validateCloudTasksRequest
which can throw synchronously and produce unhandled promise rejections; move the
calls to Env.cloudTasksServiceAccountEmail(true) and
Env.cloudRunServiceUrl(true) into the existing try block (remove the earlier
top-level declarations) so any thrown errors are caught by the catch and routed
to next(new AuthorizationError(...)); keep the rest of the verification logic
unchanged (authClient.verifyIdToken, payload checks, dataLogger usage).
In `@api/services/cloud-tasks.service.ts`:
- Around line 76-100: The config field initializer calls Env.gcpProjectId(true),
Env.gcpLocationId(true), Env.cloudRunServiceUrl(true),
Env.cloudRunPrivateServiceUrl(true) and Env.cloudTasksServiceAccountEmail(true)
which throw before the constructor's missing[] aggregation runs; move the entire
config construction out of the public instance field and into the class
constructor after the existing validation/aggregation block (i.e., build
this.config inside the constructor using the validated Env values or Env.*(...,
throwError=false) so all missing envs are collected consistently), and make the
queue entries consistent with the chosen approach (either use throwError=false
in those Env calls or ensure they also throw inside the constructor) so no field
initializer can throw before aggregation.
In `@api/services/github-webhook/issue_comment-webhook.service.ts`:
- Around line 511-533: The outer catch currently posts a user-facing message
("Bounty was created but...") that is only correct when the inner GitHub
comment/label mutation fails; change the outer catch to use the generic fallback
message ("Bounty creation failed due to an internal error. Please try again on
the DevAsign dashboard.") and ensure statsigService.logEvent and
dataLogger.error remain unchanged; move the specific "Bounty was created but
there was an issue while posting the bounty comment or adding the bounty label
on issue" message so it is only used in the inner catch that wraps the GitHub
comment/label operations (the block that calls bountyFailureComment), and verify
that earlier calls like stellarService.getAccountInfo, KMSService.decryptWallet,
and prisma.task.create are allowed to bubble to the outer handler which now uses
the generic message.
In `@api/services/socket.service.ts`:
- Around line 38-50: Pre-compute and cache the allowed origins before
constructing the SocketIOServer to avoid uncaught exceptions and per-connection
calls: call Env.corsOrigins(true) once inside SocketService.initialize (store
result in a const like allowedOrigins), guard that call with try/catch to handle
the unset CORS_ORIGINS case (log or fallback to an empty array), then update the
cors.origin callback to reference allowedOrigins (e.g., if (!origin ||
allowedOrigins.includes(origin)) callback(null,true) else callback(new
Error("Not allowed by CORS"))), removing any further calls to Env.corsOrigins
from inside the callback.
In `@tests/integration/api/installation/team.api.test.ts`:
- Around line 234-246: The test descriptions say "404" but the assertions use
STATUS_CODES.FORBIDDEN (403); update the three failing tests' description
strings (the it(...) blocks that call getEndpointWithPrefix([...]) and assert
.expect(STATUS_CODES.FORBIDDEN)) to say "should return 403 when installation not
found" (repeat the same change for the other two occurrences noted), so the
human-readable message matches the actual expectation.
In `@tests/integration/api/task/`{taskId}.api.test.ts:
- Around line 972-1003: In requestTimelineExtension and
replyTimelineExtensionRequest, replace the ValidationError thrown for
authorization failures (the checks that verify "user is contributor" or "user is
creator") with AuthorizationError while keeping ValidationError for status/state
validation (e.g., "task is not in progress"); locate the checks in the
requestTimelineExtension and replyTimelineExtensionRequest functions and swap
the thrown error type to AuthorizationError (import/ensure AuthorizationError is
available) so authorization failures return 403 while other validation issues
still throw ValidationError and return 400.
In `@tests/integration/api/webhook/github.api.test.ts`:
- Around line 537-570: Update the GitHubWebhookError class (error.model.ts /
GitHubWebhookError) to accept an HTTP status code parameter in its constructor
(instead of hardcoding STATUS_CODES.INTERNAL_SERVER_ERROR) and store it as the
error's status; then update the webhook middleware/validation sites that throw
this error (the places handling missing signature, invalid signature, missing
webhook secret, and invalid body format) to pass appropriate 4xx codes (e.g.,
STATUS_CODES.BAD_REQUEST for malformed body/signature, STATUS_CODES.UNAUTHORIZED
for missing/invalid signature or secret). Also update the tests expecting
STATUS_CODES.INTERNAL_SERVER_ERROR to assert the new 4xx codes so behavior and
test expectations align.
---
Nitpick comments:
In `@api/controllers/wallet/index.controller.ts`:
- Around line 104-119: The local constant usdcAsset in the balance-check block
should be renamed to avoid shadowing the module-level usdcAsset import from
stellar.config.js; change the local variable name (e.g., to usdcBalance or
usdcWalletBalance) wherever it’s declared and referenced in the balance-checking
logic inside the controller, and apply the same rename to the corresponding
local declaration in the swapAsset function’s else block so both places stop
using the imported symbol name while keeping all type guards and usages intact.
In `@api/models/error.model.ts`:
- Around line 45-53: The AuthorizationError class currently sets its internal
code to "UNAUTHORIZED" while using STATUS_CODES.FORBIDDEN (403); update the
AuthorizationError constructor to use a matching code (e.g., "FORBIDDEN")
instead of "UNAUTHORIZED" so the code string aligns with the HTTP 403 status;
ensure you only change the literal passed as the first argument in the
AuthorizationError constructor and leave STATUS_CODES.FORBIDDEN and other
parameters intact.
In `@api/services/contract.service.ts`:
- Around line 19-43: The static initialization uses redundant "|| ''" fallbacks
and inconsistent checks: remove the unnecessary "|| ''" wrappers in the static
validation block (replace conditions like !(Env.stellarNetwork() || "") with
!Env.stellarNetwork()) and simplify CONFIG initializers to directly use the
thrown-or-present values (replace expressions like ((Env.stellarNetwork(true) ||
"") || "") with Env.stellarNetwork(true)!, Env.stellarRpcUrl(true)!,
Env.taskEscrowContractId(true)!, Env.usdcContractId(true)!, and
Env.stellarMasterPublicKey(true)!), keeping the existing EscrowContractError
throw logic and the networkPassphrase/maxFee computation unchanged.
In `@api/services/kms.service.ts`:
- Around line 8-16: The guard in the KmsService module redundantly uses `|| ""`
fallbacks and an extra `|| ""` inside the initial presence check; simplify the
guard and use consistent non-null assertions: update the guard that throws
KmsServiceError to check presence with straightforward falsy checks (e.g., `if
(!Env.gcpProjectId() || !Env.gcpLocationId() || !Env.gcpKeyRingId() ||
!Env.gcpKeyId()) { ... }`) and set PROJECT_ID using the same pattern as the
others (e.g., `const PROJECT_ID = Env.gcpProjectId(true)!;`) removing the double
`|| ""` fallbacks so PROJECT_ID, LOCATION_ID, KEY_RING_ID, and KEY_ID are
handled consistently.
🪄 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: bde8927d-2a37-4e5d-8c93-77bb71aecdc3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (83)
.env.exampleREADME.mdapi/config/firebase.config.tsapi/config/logger.config.tsapi/config/stellar.config.tsapi/controllers/installation/github.controller.tsapi/controllers/installation/index.controller.tsapi/controllers/installation/team.controller.tsapi/controllers/internal/index.controller.tsapi/controllers/task/activities.controller.tsapi/controllers/task/contributor.controller.tsapi/controllers/task/index.controller.tsapi/controllers/task/installation.controller.tsapi/controllers/task/{taskId}.controller.tsapi/controllers/user/index.controller.tsapi/controllers/user/sumsub.controller.tsapi/controllers/wallet/index.controller.tsapi/controllers/wallet/transactions.controller.tsapi/controllers/webhook/github.controller.tsapi/controllers/webhook/sumsub.controller.tsapi/index.tsapi/middlewares/auth.middleware.tsapi/middlewares/error.middleware.tsapi/middlewares/rate-limit.middleware.tsapi/middlewares/request.middleware.tsapi/middlewares/webhook.middleware.tsapi/models/error.model.tsapi/routes/_test_/contract.test.route.tsapi/routes/_test_/general.test.route.tsapi/routes/_test_/octokit.test.route.tsapi/routes/_test_/stellar.test.route.tsapi/routes/agent.route.tsapi/routes/installation.route.tsapi/routes/internal.route.tsapi/routes/task.route.tsapi/routes/user.route.tsapi/routes/wallet.route.tsapi/routes/webhook.route.tsapi/schemas/global.schema.tsapi/schemas/installation.schema.tsapi/schemas/task.schema.tsapi/schemas/wallet.schema.tsapi/services/cloud-tasks.service.tsapi/services/contract.service.tsapi/services/error-handler.service.tsapi/services/github-webhook/installation-webhook.service.tsapi/services/github-webhook/installation_repositories-webhook.service.tsapi/services/github-webhook/issue_comment-webhook.service.tsapi/services/github-webhook/pull_request-webhook.service.tsapi/services/github-webhook/push-webhook.service.tsapi/services/kms.service.tsapi/services/octokit.service.tsapi/services/socket.service.tsapi/services/statsig.service.tsapi/services/stellar.service.tsapi/utils/data.tsapi/utils/env.tsapi/utils/helper.tspackage.jsontests/helpers/__tests__/database-test-utilities.test.tstests/helpers/database-test-helper.tstests/helpers/test-utils.tstests/integration/api/installation/github.api.test.tstests/integration/api/installation/index.api.test.tstests/integration/api/installation/team.api.test.tstests/integration/api/internal/index.api.test.tstests/integration/api/task/activities.api.test.tstests/integration/api/task/contributor.api.test.tstests/integration/api/task/index.api.test.tstests/integration/api/task/installation.api.test.tstests/integration/api/task/{taskId}.api.test.tstests/integration/api/user/index.api.test.tstests/integration/api/user/sumsub.api.test.tstests/integration/api/wallet/index.api.test.tstests/integration/api/wallet/transactions.api.test.tstests/integration/api/webhook/github.api.test.tstests/integration/api/webhook/sumsub.api.test.tstests/integration/middlewares/auth.middleware.test.tstests/integration/middlewares/error.middleware.test.tstests/integration/middlewares/rate-limit.middleware.test.tstests/integration/middlewares/request.middleware.test.tstests/integration/middlewares/webhook.middleware.test.tstests/unit/helper.test.ts
💤 Files with no reviewable changes (1)
- package.json
Comment on lines
+399
to
+405
| txBuilder = txBuilder.addOperation(Operation.pathPaymentStrictSend({ | ||
| destination: destinationAddress, | ||
| sendAsset, | ||
| destAsset, | ||
| sendAmount: amount, | ||
| destMin: "0.0000001" // For simplicity, accepting any amount of destAsset. | ||
| })); |
There was a problem hiding this comment.
These path payments currently accept effectively any fill price.
Using destMin: "0.0000001" means a thin or manipulated order book can complete the path payment for an almost-zero return. For transfer and swap flows, that is a financial correctness bug. destMin needs to come from a quote or explicit slippage tolerance, not a hard-coded dust value.
Suggested direction
- destMin: "0.0000001"
+ destMinPass destMin into these methods from a quoted amount or a caller-supplied slippage calculation, and reject requests where it is missing for cross-asset path payments.
Also applies to: 486-492, 565-570
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/services/stellar.service.ts` around lines 399 - 405, Current
pathPaymentStrictSend calls use a hard-coded destMin ("0.0000001"), which allows
almost-zero fills; change the code to accept a caller-supplied destMin (derived
from the quote or explicit slippage tolerance) and use that value in the
Operation.pathPaymentStrictSend call instead of the dust constant. Update the
method signatures that build these transactions (the function that builds
txBuilder and any helpers that reference sendAsset, destAsset, amount,
destinationAddress) to accept a required destMin parameter for cross-asset path
payments, validate that destMin is present and > 0 (and reject the request if
missing or below an acceptable threshold), and apply the same change to the
other two occurrences noted (the other pathPaymentStrictSend calls).
Merge Score: 95/100🟢 This PR introduces a comprehensive refactoring of the codebase, focusing on standardizing HTTP status codes, centralizing environment variable management via a new Code Suggestions (5)High Priority (1)
Reasoning: Suggested Code: const feeBump = TransactionBuilder.buildFeeBumpTransaction(
sponsorKeyPair.publicKey(),
baseFee,
transaction,
networkPassphrase
);Low Priority (4)
Reasoning: The expression Suggested Code: if (!Env.cloudTasksServiceAccountEmail()) missing.push("CLOUD_TASKS_SERVICE_ACCOUNT_EMAIL");
Reasoning: The expressions Suggested Code: private static CONFIG = {
network: Env.stellarNetwork(true)!,
rpcUrl: Env.stellarRpcUrl(true)!,
Reasoning: The expression Suggested Code: if (!Env.githubAppId() || !Env.githubAppPrivateKey()) {
throw new GitHubAPIError("Missing GitHub App credentials (GITHUB_APP_ID, GITHUB_APP_PRIVATE_KEY) in environment variables");
}
Reasoning: The expression Suggested Code: if (!Env.gcpProjectId() || !Env.gcpLocationId() || !Env.gcpKeyRingId() || !Env.gcpKeyId()) {
throw new KmsServiceError("Missing GCP KMS credentials (GCP_PROJECT_ID, GCP_LOCATION_ID, GCP_KEY_RING_ID, GCP_KEY_ID) in environment variables");
}
// Set Google Cloud KMS environment variables
const PROJECT_ID = Env.gcpProjectId(true)!;📋 Copy Review for AI Agent
📊 Review Metadata
|
…edundant fallbacks
❌ PR Review FailedError DetailsThe PR review system encountered an error while analyzing this pull request: What to do next
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/services/kms.service.ts`:
- Around line 8-16: The module currently validates GCP KMS env vars at import
time (the top-level check that throws KmsServiceError and the constants
PROJECT_ID, LOCATION_ID, KEY_RING_ID, KEY_ID), which can crash the app even when
KMS is unused; move this validation lazily into the KMS service methods (or a
single helper e.g., ensureKmsEnv()) so the checks run at runtime before any KMS
call. Replace top-level throws and direct constant initialization with functions
that call Env.gcpProjectId(true) etc. inside service methods (or the new
ensureKmsEnv) and throw KmsServiceError there if missing, keeping fail-fast
behavior only when KMS is actually invoked.
🪄 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: 7e0b3e39-497d-4c27-9cb7-f71f6dc90cc9
📒 Files selected for processing (10)
.env.exampleapi/routes/_test_/general.test.route.tsapi/routes/agent.route.tsapi/services/cloud-tasks.service.tsapi/services/contract.service.tsapi/services/kms.service.tsapi/services/octokit.service.tsapi/services/stellar.service.tsapi/utils/env.tstests/integration/api/webhook/sumsub.api.test.ts
✅ Files skipped from review due to trivial changes (3)
- tests/integration/api/webhook/sumsub.api.test.ts
- api/services/contract.service.ts
- api/services/cloud-tasks.service.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- api/routes/test/general.test.route.ts
- api/services/octokit.service.ts
- api/utils/env.ts
- api/routes/agent.route.ts
- api/services/stellar.service.ts
Comment on lines
+8
to
+16
| if (!Env.gcpProjectId() || !Env.gcpLocationId() || !Env.gcpKeyRingId() || !Env.gcpKeyId()) { | ||
| throw new KmsServiceError("Missing GCP KMS credentials (GCP_PROJECT_ID, GCP_LOCATION_ID, GCP_KEY_RING_ID, GCP_KEY_ID) in environment variables"); | ||
| } | ||
|
|
||
| // Set Google Cloud KMS environment variables | ||
| const PROJECT_ID = process.env.GCP_PROJECT_ID; | ||
| const LOCATION_ID = process.env.GCP_LOCATION_ID; | ||
| const KEY_RING_ID = process.env.GCP_KEY_RING_ID; | ||
| const KEY_ID = process.env.GCP_KEY_ID; | ||
| const PROJECT_ID = Env.gcpProjectId(true)!; | ||
| const LOCATION_ID = Env.gcpLocationId(true)!; | ||
| const KEY_RING_ID = Env.gcpKeyRingId(true)!; | ||
| const KEY_ID = Env.gcpKeyId(true)!; |
There was a problem hiding this comment.
Defer KMS env validation to runtime usage instead of module import.
Validating and throwing at module scope can crash app startup as soon as this file is imported, even when KMS is not used on that code path. Keep fail-fast behavior, but do it lazily inside the service methods (or a shared resolver) so unrelated flows can still boot.
Suggested refactor
-// Verify required environment variables are present
-if (!Env.gcpProjectId() || !Env.gcpLocationId() || !Env.gcpKeyRingId() || !Env.gcpKeyId()) {
- throw new KmsServiceError("Missing GCP KMS credentials (GCP_PROJECT_ID, GCP_LOCATION_ID, GCP_KEY_RING_ID, GCP_KEY_ID) in environment variables");
-}
-
-// Set Google Cloud KMS environment variables
-const PROJECT_ID = Env.gcpProjectId(true)!;
-const LOCATION_ID = Env.gcpLocationId(true)!;
-const KEY_RING_ID = Env.gcpKeyRingId(true)!;
-const KEY_ID = Env.gcpKeyId(true)!;
-
const client = new KeyManagementServiceClient();
-const keyName = client.cryptoKeyPath(PROJECT_ID, LOCATION_ID, KEY_RING_ID, KEY_ID);
+const getKeyName = () => {
+ const projectId = Env.gcpProjectId(true);
+ const locationId = Env.gcpLocationId(true);
+ const keyRingId = Env.gcpKeyRingId(true);
+ const keyId = Env.gcpKeyId(true);
+
+ if (!projectId || !locationId || !keyRingId || !keyId) {
+ throw new KmsServiceError(
+ "Missing GCP KMS credentials (GCP_PROJECT_ID, GCP_LOCATION_ID, GCP_KEY_RING_ID, GCP_KEY_ID) in environment variables"
+ );
+ }
+
+ return client.cryptoKeyPath(projectId, locationId, keyRingId, keyId);
+};
@@
- name: keyName,
+ name: getKeyName(),
@@
- name: keyName,
+ name: getKeyName(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/services/kms.service.ts` around lines 8 - 16, The module currently
validates GCP KMS env vars at import time (the top-level check that throws
KmsServiceError and the constants PROJECT_ID, LOCATION_ID, KEY_RING_ID, KEY_ID),
which can crash the app even when KMS is unused; move this validation lazily
into the KMS service methods (or a single helper e.g., ensureKmsEnv()) so the
checks run at runtime before any KMS call. Replace top-level throws and direct
constant initialization with functions that call Env.gcpProjectId(true) etc.
inside service methods (or the new ensureKmsEnv) and throw KmsServiceError there
if missing, keeping fail-fast behavior only when KMS is actually invoked.
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.
This PR covers a broad set of refactoring, chore, and performance improvements focused on cleaning up dependencies, standardizing status codes, improving environment variable handling, and optimizing runtime behavior.
Summary by CodeRabbit
Bug Fixes
Chores