feat: manual pr review via x402 integration#88
Conversation
Merge Score: 65/100🔴 The PR introduces a new manual PR review route integrated with x402 for payments. However, the change to Code Suggestions (3)High Priority (1)
Reasoning: TypeScript's Suggested Code: {
"compilerOptions": {
"target": "ES2020",
"module": "CommonJS",
"lib": [
"ES2020"
],
"outDir": "./dist",
"rootDir": "./",
"strict": true,
"esModuleInterop": true,
"skipLibCheck": true,
"forceConsistentCasingInFileNames": true,
"moduleResolution": "node",
"resolveJsonModule": true,
"types": [
"node",
"jest"
]
}
}Medium Priority (1)
Reasoning: In environments where this feature is not configured (e.g., local development, CI/CD, or specific deployments), the entire application will fail to start. Graceful degradation improves maintainability and deployment stability. Suggested Code: export const agentRoutes = Router();
if (!facilitatorUrl || !payTo || !facilitatorApiKey) {
console.warn("Missing required x402 environment variables. Agent routes are disabled.");
} else {
const facilitatorClient = new HTTPFacilitatorClient({
url: facilitatorUrl,
createAuthHeaders: async () => {
const headers = { Authorization: `Bearer ${facilitatorApiKey}` };
return { verify: headers, settle: headers, supported: headers };
}
});
agentRoutes.use(
paymentMiddlewareFromConfig(
{
[`POST ${ENDPOINTS.AGENT.REVIEW}`]: {
accepts: {
scheme: "exact",
price: "$0.50",
network,
payTo
},
description: "Pull request review",
mimeType: "application/json"
}
} as RoutesConfig,
facilitatorClient,
[{ network, server: new ExactStellarScheme() }]
)
);
agentRoutes.post(
ENDPOINTS.AGENT.REVIEW,
(async (req: Request, res: Response, next: NextFunction) => {
try {
const { prUrl } = req.body;
const githubPrRegex = /^https?:\/\/(www\.)?github\.com\/[a-zA-Z0-9_.-]+\/[a-zA-Z0-9_.-]+\/pull\/\d+\/?.*$/;
if (!prUrl || !githubPrRegex.test(prUrl)) {
throw new ValidationError("Please provide a valid GitHub Pull Request URL.");
}
const taskId = await cloudTasksService.addManualPRAnalysisJob(prUrl);
responseWrapper({
res,
status: STATUS_CODES.SUCCESS,
data: { success: true, taskId },
message: "Review in progress. A comment will be posted by our bot."
});
} catch (error) {
next(error);
}
}) as unknown as RequestHandler
);
}Low Priority (1)
Reasoning: The current regex allows any trailing characters ( Suggested Code: const match = prUrl.match(/^https?:\/\/(?:www\.)?github\.com\/([a-zA-Z0-9_.-]+)\/([a-zA-Z0-9_.-]+)\/pull\/(\d+)/);
if (!match) {
throw new ValidationError("Please provide a valid GitHub Pull Request URL.");
}
// Optionally reconstruct a clean URL to pass to the job
const cleanPrUrl = match[0];
const taskId = await cloudTasksService.addManualPRAnalysisJob(cleanPrUrl);📊 Review Metadata
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a paid PR-review endpoint (/agent/review) with x402/Stellar payment middleware, enqueues a manual PR analysis Cloud Task, extends Cloud Tasks job types/endpoints, adjusts CORS exposure, migrates many local imports to explicit ESM Changes
Sequence DiagramsequenceDiagram
participant Client
participant AgentRoute as Agent Route Handler
participant X402MW as x402 Payment Middleware
participant Facilitator as x402 Facilitator
participant Stellar as Stellar Network
participant CloudTasks as Cloud Tasks Service
participant Queue as Cloud Tasks Queue
Client->>AgentRoute: POST /agent/review { prUrl }
AgentRoute->>X402MW: pass request through payment middleware
X402MW->>Facilitator: initiate payment ($0.50, exact)
Facilitator->>Stellar: submit Stellar transaction
Stellar-->>Facilitator: confirmation
Facilitator-->>X402MW: payment verified
X402MW-->>AgentRoute: middleware clears
AgentRoute->>AgentRoute: validate prUrl
AgentRoute->>CloudTasks: addManualPRAnalysisJob(prUrl)
CloudTasks->>Queue: enqueue "manual-pr-analysis" task
Queue-->>CloudTasks: returns taskId
CloudTasks-->>AgentRoute: taskId
AgentRoute-->>Client: 200 { taskId, status: "success" }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
api/routes/agent.route.ts (1)
50-73: Consider extracting the async handler wrapper pattern.The
as unknown as RequestHandlercast is a common workaround for Express's lack of native async handler support. This pattern appears to be manually applied here. If this pattern is used elsewhere in the codebase, consider extracting it into a reusable wrapper.♻️ Optional: Create a reusable async handler wrapper
// In a utilities file type AsyncRequestHandler = (req: Request, res: Response, next: NextFunction) => Promise<void>; export const asyncHandler = (fn: AsyncRequestHandler): RequestHandler => (req, res, next) => Promise.resolve(fn(req, res, next)).catch(next);Then usage becomes cleaner:
agentRoutes.post( ENDPOINTS.AGENT.REVIEW, asyncHandler(async (req, res, next) => { // ... handler code }) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/routes/agent.route.ts` around lines 50 - 73, The route handler in agentRoutes.post uses the repetitive "as unknown as RequestHandler" cast to support async handlers; extract a reusable async wrapper (e.g., asyncHandler) that accepts an AsyncRequestHandler ((req, res, next) => Promise<void>) and returns a RequestHandler which calls the async fn and catches errors to call next(err). Replace the cast in agentRoutes.post by passing the async handler through asyncHandler (instead of the "as unknown as RequestHandler" pattern) so the code is cleaner and reusable across other routes that use async handlers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/routes/agent.route.ts`:
- Around line 56-59: Replace the vulnerable githubPrRegex in
api/routes/agent.route.ts (the githubPrRegex constant) with a safer pattern that
enforces HTTPS, removes the unnecessary (www\.) and the trailing `.*$`
backtracking part, and allows an optional trailing slash; e.g. use a pattern
like ^https://github\.com/[\w.-]+/[\w.-]+/pull/\d+/?$ to validate PR URLs and
keep the existing ValidationError logic unchanged.
In `@api/services/cloud-tasks.service.ts`:
- Line 83: The new config key "manual-pr-analysis" uses
CLOUD_TASKS_MANUAL_PR_ANALYSIS_QUEUE but that env var isn't validated in the
requiredVars list; update the environment validation (the requiredVars array
used by the init/config function that currently checks other CLOUD_TASKS_* vars)
to include "CLOUD_TASKS_MANUAL_PR_ANALYSIS_QUEUE" so startup will error or warn
if it's missing and prevent empty-string runtime failures when enqueuing manual
PR analysis tasks.
In `@tests/integration/api/wallet/index.api.test.ts`:
- Line 68: The test's Jest mock and the dynamic import refer to different module
specifiers so the mock is bypassed; update them to match by either changing the
mock registration for the Stellar module to include the .js extension (i.e.,
mock "../../../../api/services/stellar.service.js") or change the dynamic import
to import "../../../../api/services/stellar.service" so the module specifier
matches; ensure the mocked export name stellarService is what the test expects
and that jest.mock targets the exact same module string used by the dynamic
import.
In `@tsconfig.json`:
- Line 4: tsconfig.json changed "module" to "NodeNext" but the repo is missing
the surrounding ESM config; add "type": "module" to package.json and update Jest
config so ESM test files are handled: set extensionsToTreatAsEsm to include
".ts" and configure ts-jest with useESM: true (and ensure jest.config.js uses
those options) so dynamic imports like
import("../../../api/config/firebase.config.js") resolve correctly under
NodeNext.
---
Nitpick comments:
In `@api/routes/agent.route.ts`:
- Around line 50-73: The route handler in agentRoutes.post uses the repetitive
"as unknown as RequestHandler" cast to support async handlers; extract a
reusable async wrapper (e.g., asyncHandler) that accepts an AsyncRequestHandler
((req, res, next) => Promise<void>) and returns a RequestHandler which calls the
async fn and catches errors to call next(err). Replace the cast in
agentRoutes.post by passing the async handler through asyncHandler (instead of
the "as unknown as RequestHandler" pattern) so the code is cleaner and reusable
across other routes that use async handlers.
🪄 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: 67907463-5219-4ae7-a819-695901be7b7f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
api/index.tsapi/models/github.model.tsapi/routes/agent.route.tsapi/routes/index.tsapi/services/cloud-tasks.service.tsapi/utilities/data.tspackage.jsontests/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/wallet/index.api.test.tstests/integration/api/wallet/transactions.api.test.tstests/integration/api/webhook/github.api.test.tstests/integration/middlewares/auth.middleware.test.tstsconfig.json
Updated Merge Score: 85/100🟢 The author successfully addressed the Code Suggestions (2)High Priority (1)
Reasoning: Adding the x402 environment variables to the required variables list in Suggested Code: const warningVars = [
{ keys: ["X402_FACILITATOR_URL", "X402_PAYEE_ADDRESS", "X402_API_KEY"], msg: "x402 misconfiguration - manual PR review agent will be disabled" },
// ... existing warningVars
];Medium Priority (1)
Reasoning: Because Suggested Code: export const agentRoutes = Router();
if (facilitatorUrl && payTo && facilitatorApiKey) {
const facilitatorClient = new HTTPFacilitatorClient({
url: facilitatorUrl,
createAuthHeaders: async () => {
const headers = { Authorization: `Bearer ${facilitatorApiKey}` };
return { verify: headers, settle: headers, supported: headers };
}
});
agentRoutes.use(
paymentMiddlewareFromConfig(
{
[`POST ${ENDPOINTS.AGENT.REVIEW}`]: {
accepts: {
scheme: "exact",
price: "$0.50",
network,
payTo
},
description: "Pull request review",
mimeType: "application/json"
}
} as RoutesConfig,
facilitatorClient,
[{ network, server: new ExactStellarScheme() }]
)
);
agentRoutes.post(
ENDPOINTS.AGENT.REVIEW,
(async (req: Request, res: Response, next: NextFunction) => {
try {
const { prUrl } = req.body;
const match = prUrl.match(/^https?:\/\/(?:www\.)?github\.com\/([a-zA-Z0-9_.-]+)\/([a-zA-Z0-9_.-]+)\/pull\/(\d+)/);
if (!match) {
throw new ValidationError("Please provide a valid GitHub Pull Request URL.");
}
const cleanPrUrl = match[0];
const taskId = await cloudTasksService.addManualPRAnalysisJob(cleanPrUrl);
responseWrapper({
res,
status: STATUS_CODES.SUCCESS,
data: { success: true, taskId },
message: "Review in progress. A comment will be posted by our bot."
});
} catch (error) {
next(error);
}
}) as unknown as RequestHandler
);
} else {
agentRoutes.post(ENDPOINTS.AGENT.REVIEW, (req: Request, res: Response) => {
responseWrapper({
res,
status: STATUS_CODES.SERVER_ERROR,
data: {},
message: "Agent features are not configured on this server."
});
});
}📊 Review Metadata
|
Updated Merge Score: 90/100🟢 The author successfully addressed the unconditional instantiation of the x402 client by wrapping the setup in Code Suggestions (1)High Priority (1)
Reasoning: You successfully added graceful degradation in Suggested Code: const warningVars = [
{ keys: ["X402_FACILITATOR_URL", "X402_PAYEE_ADDRESS", "X402_API_KEY"], msg: "x402 misconfiguration - agent features will be disabled" },
// ... existing warning vars
];📊 Review Metadata
|
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
api/routes/agent.route.ts (1)
53-60:⚠️ Potential issue | 🔴 CriticalReplace the PR URL regex with a linear-time parser or anchored check.
This pattern is still vulnerable to backtracking on attacker-controlled input, and it also accepts arbitrary junk after the PR number because
cleanPrUrluses the partial match. Parse withURL+ pathname segments, or switch to an anchored, non-backtracking validation approach before release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/routes/agent.route.ts` around lines 53 - 60, The current regex-based validation of prUrl (and the use of match[0] as cleanPrUrl) is vulnerable to backtracking and may allow trailing junk; replace it with a linear parser using the URL constructor: create a URL from prUrl, verify url.protocol is http/https and url.hostname is "github.com" (or "www.github.com"), split url.pathname by "/" and confirm segments match ["", owner, repo, "pull", number] with owner/repo matching allowed characters and number being digits only, then rebuild a canonical cleanPrUrl from those components and pass that to cloudTasksService.addManualPRAnalysisJob(cleanPrUrl). Ensure you remove the unsafe regex and thrown ValidationError remains when parsing/validation fails.
🧹 Nitpick comments (8)
api/config/stellar.config.ts (1)
11-15: Type safety reduced by casting toany.The
customClient as anycast suppresses type checking between axios'sAxiosInstanceand whatApplicationConfigurationexpects. This likely indicates a type mismatch between the SDK's expected HTTP client interface and axios's types.Consider investigating the root cause—this could be a version mismatch or an SDK type definition issue. If the cast is necessary, the inline comment explaining why would help future maintainers.
#!/bin/bash # Check `@stellar/typescript-wallet-sdk` version and axios version echo "Checking relevant package versions:" cat package.json | jq '{stellar_sdk: .dependencies["@stellar/typescript-wallet-sdk"], axios: .dependencies["axios"]}' # Check ApplicationConfiguration constructor signature if available in node_modules echo -e "\nSearching for ApplicationConfiguration type definition:" fd "ApplicationConfiguration" node_modules/@stellar --type f -x head -50 {} 2>/dev/null || echo "SDK not found in node_modules"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/config/stellar.config.ts` around lines 11 - 15, The cast to any on customClient when constructing ApplicationConfiguration (new ApplicationConfiguration(DefaultSigner, customClient as any)) hides a type mismatch between axios's AxiosInstance and the SDK's expected HTTP client; either update the code to pass a properly typed client that matches the SDK interface (adapt/custom-wrapper or use the SDK's HttpClient type) or, if the cast is unavoidable, add an explicit comment explaining why and include the SDK and axios version check; inspect ApplicationConfiguration's constructor/type in the SDK to determine the required methods/signature and implement or type-assert to that specific interface instead of using any (referencing customClient, ApplicationConfiguration, and DefaultSigner to locate the code).tests/integration/api/task/installation.api.test.ts (1)
9-9: Variable shadowing:mockFirebaseAuthis both imported and redeclared.Line 9 imports
mockFirebaseAuthfrom the mock file, but line 50 declares a local variable with the same name that shadows the import. This could lead to confusion. Consider renaming the local variable (e.g.,mockVerifyIdToken) to clarify its purpose and avoid shadowing.♻️ Suggested rename to avoid shadowing
-import { mockFirebaseAuth } from "../../../mocks/firebase.service.mock.js"; +import { mockFirebaseAuth as firebaseAuthMock } from "../../../mocks/firebase.service.mock.js";Or rename the local variable:
- let mockFirebaseAuth: any; + let mockVerifyIdToken: any; ... - mockFirebaseAuth = firebaseAdmin.auth().verifyIdToken; + mockVerifyIdToken = firebaseAdmin.auth().verifyIdToken; ... - mockFirebaseAuth.mockResolvedValue({ + mockVerifyIdToken.mockResolvedValue({Also applies to: 50-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/api/task/installation.api.test.ts` at line 9, The import mockFirebaseAuth is being shadowed by a locally declared variable named mockFirebaseAuth; rename the local variable (for example to mockVerifyIdToken or mockFirebaseVerifyIdToken) wherever it’s declared and referenced in this test to avoid shadowing and clarify intent—update references in the test setup/teardown and any calls that use the local mock so they point to the new name, leaving the imported mockFirebaseAuth untouched.tests/integration/middlewares/webhook.middleware.test.ts (2)
10-26: Keep the mocked specifiers identical to the ESM imports.These mocks still target extensionless module ids even though the migrated code imports
.jsspecifiers. Vitest keys mocks by the provided module id, so leaving the strings different makes mock application depend on resolver normalization instead of being explicit. (vitest.dev)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/middlewares/webhook.middleware.test.ts` around lines 10 - 26, The mocks use extensionless module ids so Vitest won't reliably match the ESM imports; update the vi.mock calls to use the exact ESM specifiers (include the .js extension) that the code imports, e.g. change "../../../api/services/octokit.service" to "../../../api/services/octokit.service.js" and "../../../api/config/logger.config" to "../../../api/config/logger.config.js" while keeping the mocked objects (OctokitService and dataLogger with their fn() stubs) identical.
424-532: Restore explicit coverage for the remaining webhook guards.
api/middlewares/webhook.middleware.ts:169-254still short-circuits PRs that do not target the default branch and rejectsissue_commentpayloads with noissue, but this suite no longer exercises either branch. I’d keep unit tests for both here since they gate whether manual review processing is skipped or accepted.Also applies to: 534-620
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/middlewares/webhook.middleware.test.ts` around lines 424 - 532, Add tests that explicitly cover the remaining webhook guard branches in validateGitHubWebhookEvent: (1) a pull_request payload whose pull_request.base.ref is not the repository default branch (use createValidPRPayload({ pull_request: { base: { ref: "non-default" }}})), set X-GitHub-Event to "pull_request", call validateGitHubWebhookEvent and assert mockNext was not called and mockResponse.status/json were called with the same status/message the middleware returns for non-default-target PRs; and (2) an issue_comment payload that omits the issue object (use a createIssueCommentPayload or build payload with no issue), set X-GitHub-Event to "issue_comment", call validateGitHubWebhookEvent and assert the middleware rejects it by not calling mockNext and by calling mockResponse.status/json with the same status/message the middleware returns for missing issue. Ensure you reference validateGitHubWebhookEvent, STATUS_CODES, mockRequest.get, mockResponse, mockNext, and the existing payload helpers so the tests exercise those exact guard branches.tests/mocks/firebase.service.mock.ts (1)
261-271: KeepsetupFirebaseMocks()free ofvi.mock()calls.Vitest hoists
vi.mockbefore imports regardless of where it appears in the file, so these calls are not scoped tosetupFirebaseMocks()and don’t behave like runtime setup. That makes the helper misleading and duplicates the top-level mocks already declared above. I’d let this helper only return/reset the hoisted instances. (vitest.dev)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mocks/firebase.service.mock.ts` around lines 261 - 271, The setupFirebaseMocks function currently contains vi.mock calls which are hoisted by Vitest and therefore not scoped to runtime; remove the vi.mock("firebase-admin/firestore") and vi.mock("../../api/config/firebase.config.js") calls from setupFirebaseMocks, leaving the top-level hoisted mocks in place, and change setupFirebaseMocks to only return or reset the existing mock instances (mockTimestamp, mockFirestoreDB, mockFirebaseAdmin) so it becomes a runtime helper that resets/returns those mocks rather than declaring them.tests/integration/api/webhook/github.api.test.ts (2)
15-94: Match thesevi.mock(...)ids to the.jsimport specifiers.Same concern as the other migrated suites: the app code now imports
.jspaths, but these mocks still use extensionless ids. Vitest mock resolution is module-id based, and mismatched ids are a known source of mocks silently not applying. (vitest.dev)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/api/webhook/github.api.test.ts` around lines 15 - 94, The vi.mock call paths in github.api.test.ts use extensionless module ids that don't match the app's imports (which use .js), so mocks silently fail; update each vi.mock(...) identifier to the exact .js import specifier used by the app (for the Firebase config mock, the FirebaseService mock, OctokitService mock, ContractService mock, KMSService mock, cloudTasksService mock, and stellarService mock) so the module ids match (e.g. change "../../../../api/config/firebase.config" to "../../../../api/config/firebase.config.js" and likewise for the other mocked modules).
458-468: Keep asserting the queued-response timestamp.
api/services/github-webhook/pull_request-webhook.service.ts:65-80still returnsdata.timestamp. Dropping it from the assertion weakens this API contract test and makes a response-shape regression easier to miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/api/webhook/github.api.test.ts` around lines 458 - 468, The test removed asserting data.timestamp but the pull_request-webhook.service.ts (function handling PR webhooks) still returns data.timestamp, so restore an assertion for it in tests/integration/api/webhook/github.api.test.ts: update the expect(response.body).toMatchObject (or add a separate assertion) to include data.timestamp and validate it is present and a well-formed timestamp (e.g., non-empty string or ISO datetime); reference the pull_request-webhook.service.ts timestamp field to ensure the test and implementation remain in sync.tests/integration/api/task/index.api.test.ts (1)
16-76: Use the exact.jsspecifiers in these Vitest mocks.The code under test now imports the ESM
.jspaths, but thesevi.mock(...)calls still use extensionless ids. Vitest registers mocks against the provided module id, and id/path mismatches are a known way to accidentally load the real module instead of the mock. I’d make every mocked specifier match the runtime import string exactly here. (vitest.dev)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/api/task/index.api.test.ts` around lines 16 - 76, The Vitest mocks use extensionless module ids so they don't match the runtime ESM imports; update every vi.mock call so the specifier exactly matches the ESM import string (include the ".js" extension) used by the code under test—e.g. change the vi.mock invocations that register mockStellarService, mockContractService, mockFirebaseService, mockOctokitService, and the KMSService mock to use the exact ".js" module specifiers the application imports so the mocks (getAccountInfo, transferAsset, createEscrow, refund, createTask, updateTaskStatus, addBountyLabelAndCreateBountyComment, decryptWallet, etc.) are correctly applied at test runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/config/firebase.config.ts`:
- Line 1: The import for ServiceAccount is wrong—it's not exported from the root
"firebase-admin"; update the import statements so ServiceAccount is imported
from "firebase-admin/app" (or switch to modular imports from
"firebase-admin/app" such as initializeApp and cert) and keep the default admin
import from "firebase-admin" if you still use admin.initializeApp; specifically
replace the current import of ServiceAccount from "firebase-admin" with an
import of type ServiceAccount from "firebase-admin/app" or refactor to modular
imports (initializeApp, cert, ServiceAccount) to resolve the incorrect export.
In `@api/routes/agent.route.ts`:
- Around line 10-21: The startup currently builds the HTTPFacilitatorClient with
potentially undefined envs (facilitatorUrl, payTo, facilitatorApiKey) and always
emits an Authorization header; update the initialization around
HTTPFacilitatorClient so you validate that facilitatorUrl and payTo are present
before creating the client (or skip/disable the paywall when missing), and
change createAuthHeaders to only return Authorization when facilitatorApiKey is
non-empty (otherwise return empty headers or omit Authorization). Reference the
constants network, facilitatorUrl, payTo, facilitatorApiKey and the
HTTPFacilitatorClient/createAuthHeaders function to locate and guard the logic.
- Around line 27-69: The payment middleware (paymentMiddlewareFromConfig) is
currently applied before request validation and can charge callers even if the
handler fails; move the cheap PR URL validation (the regex check for prUrl in
the POST handler for ENDPOINTS.AGENT.REVIEW) to run before attaching the payment
middleware so invalid requests are rejected without charging, and implement a
durable/compensating pattern around cloudTasksService.addManualPRAnalysisJob
(e.g., persist a pending-job record or enqueue to a reliable work-queue first
and only allow payment settlement after successful enqueue, or implement
refund/compensation logic when addManualPRAnalysisJob throws) so failures in
addManualPRAnalysisJob do not result in unrecoverable charges.
- Around line 27-43: The payment middleware configuration currently passes a
single object as the `accepts` value to paymentMiddlewareFromConfig (in
agentRoutes usage of paymentMiddlewareFromConfig with ENDPOINTS.AGENT.REVIEW and
ExactStellarScheme), but the installed x402 v2 API expects `accepts` to be an
array and a separately registered resource server parameter; update the call so
the route's config uses `accepts: [{ scheme: "exact", price: "$0.50", network,
payTo, mimeType: "application/json", description: "Pull request review" }]` and
pass the resource server instance according to the v2 signature (verify whether
paymentMiddlewareFromConfig expects `facilitatorClient, server` or a servers
list and adjust usage of facilitatorClient and ExactStellarScheme accordingly),
then run the TypeScript build to confirm the types (RoutesConfig,
paymentMiddlewareFromConfig, agentRoutes) match the package's v2 definitions.
In `@api/services/cloud-tasks.service.ts`:
- Line 92: Add a route handler for the internal manual PR analysis endpoint so
Cloud Tasks doesn't get 404s: in internal.route.ts register a POST handler for
ENDPOINTS.INTERNAL.MANUAL_PR_ANALYSIS (or the router method used by existing
internal routes, e.g., registerInternalRoutes/router.post) that validates the
incoming task payload, invokes the same processing logic used for manual PR
analysis (call the service function that performs the analysis), and returns the
appropriate 200/4xx response; ensure the handler name and exported router follow
the project's routing conventions so the endpoint matches the path configured in
cloud-tasks.service.ts.
In `@api/services/statsig.service.ts`:
- Around line 1-2: The import for statsig-node is wrong: replace the named
import of Statsig and StatsigUser with a default import for Statsig and a
type-only named import for StatsigUser (i.e., import Statsig, { type StatsigUser
} from "statsig-node") so the runtime uses the default export; update the
top-level import statement that currently reads "import { Statsig, StatsigUser }
from 'statsig-node'" to the corrected form and keep dataLogger usage unchanged.
In `@tests/mocks/firebase.service.mock.ts`:
- Around line 16-35: The messagesCol.get() mock currently uses mockResolvedValue
with a snapshot of mockMessages at construction, causing later createMessage
calls not to appear; change the messagesCol.get mock (the vi.fn assigned to get
on messagesCol) to use mockImplementation(async () => ({ docs:
mockMessages.map(msg => ({ id: msg.id, data: () => msg })) })) so each
invocation returns a live snapshot built from the current mockMessages array
(ensure it's async so callers still receive a Promise).
---
Duplicate comments:
In `@api/routes/agent.route.ts`:
- Around line 53-60: The current regex-based validation of prUrl (and the use of
match[0] as cleanPrUrl) is vulnerable to backtracking and may allow trailing
junk; replace it with a linear parser using the URL constructor: create a URL
from prUrl, verify url.protocol is http/https and url.hostname is "github.com"
(or "www.github.com"), split url.pathname by "/" and confirm segments match ["",
owner, repo, "pull", number] with owner/repo matching allowed characters and
number being digits only, then rebuild a canonical cleanPrUrl from those
components and pass that to
cloudTasksService.addManualPRAnalysisJob(cleanPrUrl). Ensure you remove the
unsafe regex and thrown ValidationError remains when parsing/validation fails.
---
Nitpick comments:
In `@api/config/stellar.config.ts`:
- Around line 11-15: The cast to any on customClient when constructing
ApplicationConfiguration (new ApplicationConfiguration(DefaultSigner,
customClient as any)) hides a type mismatch between axios's AxiosInstance and
the SDK's expected HTTP client; either update the code to pass a properly typed
client that matches the SDK interface (adapt/custom-wrapper or use the SDK's
HttpClient type) or, if the cast is unavoidable, add an explicit comment
explaining why and include the SDK and axios version check; inspect
ApplicationConfiguration's constructor/type in the SDK to determine the required
methods/signature and implement or type-assert to that specific interface
instead of using any (referencing customClient, ApplicationConfiguration, and
DefaultSigner to locate the code).
In `@tests/integration/api/task/index.api.test.ts`:
- Around line 16-76: The Vitest mocks use extensionless module ids so they don't
match the runtime ESM imports; update every vi.mock call so the specifier
exactly matches the ESM import string (include the ".js" extension) used by the
code under test—e.g. change the vi.mock invocations that register
mockStellarService, mockContractService, mockFirebaseService,
mockOctokitService, and the KMSService mock to use the exact ".js" module
specifiers the application imports so the mocks (getAccountInfo, transferAsset,
createEscrow, refund, createTask, updateTaskStatus,
addBountyLabelAndCreateBountyComment, decryptWallet, etc.) are correctly applied
at test runtime.
In `@tests/integration/api/task/installation.api.test.ts`:
- Line 9: The import mockFirebaseAuth is being shadowed by a locally declared
variable named mockFirebaseAuth; rename the local variable (for example to
mockVerifyIdToken or mockFirebaseVerifyIdToken) wherever it’s declared and
referenced in this test to avoid shadowing and clarify intent—update references
in the test setup/teardown and any calls that use the local mock so they point
to the new name, leaving the imported mockFirebaseAuth untouched.
In `@tests/integration/api/webhook/github.api.test.ts`:
- Around line 15-94: The vi.mock call paths in github.api.test.ts use
extensionless module ids that don't match the app's imports (which use .js), so
mocks silently fail; update each vi.mock(...) identifier to the exact .js import
specifier used by the app (for the Firebase config mock, the FirebaseService
mock, OctokitService mock, ContractService mock, KMSService mock,
cloudTasksService mock, and stellarService mock) so the module ids match (e.g.
change "../../../../api/config/firebase.config" to
"../../../../api/config/firebase.config.js" and likewise for the other mocked
modules).
- Around line 458-468: The test removed asserting data.timestamp but the
pull_request-webhook.service.ts (function handling PR webhooks) still returns
data.timestamp, so restore an assertion for it in
tests/integration/api/webhook/github.api.test.ts: update the
expect(response.body).toMatchObject (or add a separate assertion) to include
data.timestamp and validate it is present and a well-formed timestamp (e.g.,
non-empty string or ISO datetime); reference the pull_request-webhook.service.ts
timestamp field to ensure the test and implementation remain in sync.
In `@tests/integration/middlewares/webhook.middleware.test.ts`:
- Around line 10-26: The mocks use extensionless module ids so Vitest won't
reliably match the ESM imports; update the vi.mock calls to use the exact ESM
specifiers (include the .js extension) that the code imports, e.g. change
"../../../api/services/octokit.service" to
"../../../api/services/octokit.service.js" and
"../../../api/config/logger.config" to "../../../api/config/logger.config.js"
while keeping the mocked objects (OctokitService and dataLogger with their fn()
stubs) identical.
- Around line 424-532: Add tests that explicitly cover the remaining webhook
guard branches in validateGitHubWebhookEvent: (1) a pull_request payload whose
pull_request.base.ref is not the repository default branch (use
createValidPRPayload({ pull_request: { base: { ref: "non-default" }}})), set
X-GitHub-Event to "pull_request", call validateGitHubWebhookEvent and assert
mockNext was not called and mockResponse.status/json were called with the same
status/message the middleware returns for non-default-target PRs; and (2) an
issue_comment payload that omits the issue object (use a
createIssueCommentPayload or build payload with no issue), set X-GitHub-Event to
"issue_comment", call validateGitHubWebhookEvent and assert the middleware
rejects it by not calling mockNext and by calling mockResponse.status/json with
the same status/message the middleware returns for missing issue. Ensure you
reference validateGitHubWebhookEvent, STATUS_CODES, mockRequest.get,
mockResponse, mockNext, and the existing payload helpers so the tests exercise
those exact guard branches.
In `@tests/mocks/firebase.service.mock.ts`:
- Around line 261-271: The setupFirebaseMocks function currently contains
vi.mock calls which are hoisted by Vitest and therefore not scoped to runtime;
remove the vi.mock("firebase-admin/firestore") and
vi.mock("../../api/config/firebase.config.js") calls from setupFirebaseMocks,
leaving the top-level hoisted mocks in place, and change setupFirebaseMocks to
only return or reset the existing mock instances (mockTimestamp,
mockFirestoreDB, mockFirebaseAdmin) so it becomes a runtime helper that
resets/returns those mocks rather than declaring them.
🪄 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: 6ad1ff24-6774-4e17-8c62-d46de2f42bd9
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (98)
api/config/database.config.tsapi/config/firebase.config.tsapi/config/stellar.config.tsapi/controllers/installation/github.controller.tsapi/controllers/installation/index.controller.tsapi/controllers/installation/index.tsapi/controllers/installation/team.controller.tsapi/controllers/internal/index.controller.tsapi/controllers/internal/index.tsapi/controllers/task/activities.controller.tsapi/controllers/task/contributor.controller.tsapi/controllers/task/index.controller.tsapi/controllers/task/index.tsapi/controllers/task/installation.controller.tsapi/controllers/task/{taskId}.controller.tsapi/controllers/user/index.controller.tsapi/controllers/user/index.tsapi/controllers/user/sumsub.controller.tsapi/controllers/wallet/index.controller.tsapi/controllers/wallet/index.tsapi/controllers/wallet/transactions.controller.tsapi/controllers/webhook/github.controller.tsapi/controllers/webhook/index.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/ai-review.model.tsapi/models/error.model.tsapi/models/task.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/index.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/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/firebase.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.tsjest.config.jspackage.jsontests/helpers/__tests__/database-test-utilities.test.tstests/helpers/database-test-helper.tstests/helpers/database-test-utilities.tstests/helpers/test-data-factory.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/mocks/cuid2.mock.tstests/mocks/firebase.service.mock.tstests/setup/global-setup.tstests/setup/setup-verification.test.tstests/setup/vitest.setup.tstests/tsconfig.jsontests/unit/helper.test.tstsconfig.jsonvitest.config.ts
💤 Files with no reviewable changes (1)
- jest.config.js
✅ Files skipped from review due to trivial changes (51)
- api/schemas/wallet.schema.ts
- api/models/task.model.ts
- api/controllers/internal/index.ts
- api/controllers/installation/github.controller.ts
- api/services/github-webhook/push-webhook.service.ts
- api/services/contract.service.ts
- tests/helpers/database-test-utilities.ts
- api/models/ai-review.model.ts
- api/routes/test/general.test.route.ts
- tests/helpers/database-test-helper.ts
- api/controllers/installation/team.controller.ts
- api/controllers/task/index.ts
- api/controllers/user/sumsub.controller.ts
- api/controllers/webhook/github.controller.ts
- api/controllers/webhook/index.ts
- api/services/octokit.service.ts
- api/services/socket.service.ts
- api/config/database.config.ts
- api/services/stellar.service.ts
- api/services/github-webhook/installation_repositories-webhook.service.ts
- api/controllers/user/index.ts
- api/routes/task.route.ts
- api/routes/user.route.ts
- api/middlewares/request.middleware.ts
- api/controllers/task/installation.controller.ts
- tests/setup/global-setup.ts
- api/services/kms.service.ts
- api/routes/webhook.route.ts
- api/schemas/task.schema.ts
- api/routes/wallet.route.ts
- api/controllers/wallet/index.ts
- api/controllers/installation/index.controller.ts
- api/middlewares/auth.middleware.ts
- api/controllers/internal/index.controller.ts
- api/controllers/webhook/sumsub.controller.ts
- api/services/github-webhook/pull_request-webhook.service.ts
- api/controllers/task/activities.controller.ts
- api/controllers/user/index.controller.ts
- tests/mocks/cuid2.mock.ts
- tests/integration/api/task/contributor.api.test.ts
- api/controllers/wallet/index.controller.ts
- api/routes/installation.route.ts
- api/services/github-webhook/issue_comment-webhook.service.ts
- tests/integration/api/task/{taskId}.api.test.ts
- api/schemas/installation.schema.ts
- api/services/firebase.service.ts
- tests/integration/middlewares/rate-limit.middleware.test.ts
- api/routes/test/octokit.test.route.ts
- api/controllers/task/{taskId}.controller.ts
- api/controllers/task/index.controller.ts
- api/controllers/task/contributor.controller.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- api/index.ts
- tests/integration/api/task/activities.api.test.ts
- tests/integration/api/wallet/transactions.api.test.ts
- package.json
- tests/integration/api/internal/index.api.test.ts
- tsconfig.json
- tests/integration/middlewares/auth.middleware.test.ts
- tests/integration/api/user/index.api.test.ts
- tests/integration/api/wallet/index.api.test.ts
- tests/integration/api/installation/index.api.test.ts
- api/routes/index.ts
- tests/integration/api/installation/team.api.test.ts
- tests/integration/api/installation/github.api.test.ts
| @@ -1,5 +1,4 @@ | |||
| import * as admin from "firebase-admin"; | |||
| import { ServiceAccount } from "firebase-admin"; | |||
| import admin, { ServiceAccount } from "firebase-admin"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check firebase-admin version in package.json
echo "Checking firebase-admin version:"
cat package.json | jq '.dependencies["firebase-admin"] // .devDependencies["firebase-admin"]'
# Check if there are any other firebase-admin imports in the codebase for consistency
echo -e "\nOther firebase-admin imports in codebase:"
rg "from ['\"]firebase-admin['\"]" --type ts -nRepository: devasignhq/devasign-api
Length of output: 240
🌐 Web query:
firebase-admin 13.6.1 ESM default import ServiceAccount export structure
💡 Result:
In firebase-admin v13.6.1, the ServiceAccount type is defined (and exported) like this:
export interface ServiceAccount {
projectId?: string;
clientEmail?: string;
privateKey?: string;
}[1]
Correct ESM import (recommended)
ServiceAccount is exported from the modular entrypoint firebase-admin/app:
import { initializeApp, cert, type ServiceAccount } from "firebase-admin/app";
const sa: ServiceAccount = {
projectId: process.env.FIREBASE_PROJECT_ID,
clientEmail: process.env.FIREBASE_CLIENT_EMAIL,
privateKey: process.env.FIREBASE_PRIVATE_KEY?.replace(/\\n/g, "\n"),
};
initializeApp({ credential: cert(sa) });[2]
About import admin from "firebase-admin" (default import)
The top-level firebase-admin entrypoint is still implemented as a CommonJS export = ... module (legacy “namespaced API”), not a true ESM export default module. So a default import only works via CJS↔ESM interop (Node/TS settings), not because the package defines an ESM default export. [3]
“camelCase vs snake_case” keys
Even though the TS ServiceAccount interface is camelCase, the runtime code accepts either camelCase (projectId, clientEmail, privateKey) or snake_case (project_id, client_email, private_key) and normalizes between them. [4]
Sources:
[1] https://github.com/firebase/firebase-admin-node/blob/v13.6.1/src/app/credential.ts
[2] https://github.com/firebase/firebase-admin-node/blob/v13.6.1/src/app/index.ts
[3] https://github.com/firebase/firebase-admin-node/blob/v13.6.1/src/index.ts
[4] https://github.com/firebase/firebase-admin-node/blob/v13.6.1/src/app/credential-internal.ts
🏁 Script executed:
#!/bin/bash
# Check if there are any errors related to this import in the codebase
# Look for how admin is being used
echo "Usage of 'admin' in firebase.config.ts:"
rg "admin\." api/config/firebase.config.ts -A 2 -B 2
# Check if ServiceAccount type is being used
echo -e "\nUsage of 'ServiceAccount' in codebase:"
rg "ServiceAccount" --type ts -nRepository: devasignhq/devasign-api
Length of output: 724
🏁 Script executed:
#!/bin/bash
# Check tsconfig to see module resolution settings
echo "TypeScript Configuration:"
cat tsconfig.json | jq '.compilerOptions | {module, moduleResolution, esModuleInterop}'
# Try to find if ServiceAccount is re-exported from root firebase-admin
echo -e "\nChecking firebase-admin package types:"
npm list firebase-admin 2>/dev/null | head -5
# Check if there are type errors or if the code compiles
echo -e "\nLooking for any firebase-admin type definition files:"
fd "firebase-admin" node_modules/@types -type d 2>/dev/null | head -5Repository: devasignhq/devasign-api
Length of output: 466
🏁 Script executed:
#!/bin/bash
# Fix the jq query syntax
echo "TypeScript Configuration:"
cat tsconfig.json 2>/dev/null | head -20
# Check if firebase-admin is actually installed
echo -e "\nChecking if firebase-admin is installed:"
test -d node_modules/firebase-admin && echo "firebase-admin is installed" || echo "firebase-admin NOT found in node_modules"
# Try to check the actual firebase-admin package exports
echo -e "\nChecking firebase-admin package.json for exports:"
if [ -f "node_modules/firebase-admin/package.json" ]; then
cat node_modules/firebase-admin/package.json | grep -A 20 '"exports"'
else
echo "firebase-admin package.json not found in node_modules"
fi
# Check the main index.ts in firebase-admin to see what it exports
echo -e "\nChecking main firebase-admin entry point:"
if [ -f "node_modules/firebase-admin/lib/index.d.ts" ]; then
head -50 node_modules/firebase-admin/lib/index.d.ts | grep -E "export|ServiceAccount"
else
echo "Type definition file not found, checking alternate locations..."
find node_modules/firebase-admin -name "index.d.ts" 2>/dev/null | head -3
fiRepository: devasignhq/devasign-api
Length of output: 935
Incorrect ServiceAccount import location.
ServiceAccount is not exported from the root firebase-admin module. It must be imported from firebase-admin/app instead. Update to:
import admin from "firebase-admin";
import { type ServiceAccount } from "firebase-admin/app";Or use the modular imports directly:
import { initializeApp, cert, type ServiceAccount } from "firebase-admin/app";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/config/firebase.config.ts` at line 1, The import for ServiceAccount is
wrong—it's not exported from the root "firebase-admin"; update the import
statements so ServiceAccount is imported from "firebase-admin/app" (or switch to
modular imports from "firebase-admin/app" such as initializeApp and cert) and
keep the default admin import from "firebase-admin" if you still use
admin.initializeApp; specifically replace the current import of ServiceAccount
from "firebase-admin" with an import of type ServiceAccount from
"firebase-admin/app" or refactor to modular imports (initializeApp, cert,
ServiceAccount) to resolve the incorrect export.
api/routes/agent.route.ts
Outdated
| agentRoutes.use( | ||
| paymentMiddlewareFromConfig( | ||
| { | ||
| [`POST ${ENDPOINTS.AGENT.REVIEW}`]: { | ||
| accepts: { | ||
| scheme: "exact", | ||
| price: "$0.50", | ||
| network, | ||
| payTo | ||
| }, | ||
| description: "Pull request review", | ||
| mimeType: "application/json" | ||
| } | ||
| } as RoutesConfig, | ||
| facilitatorClient, | ||
| [{ network, server: new ExactStellarScheme() }] | ||
| ) | ||
| ); | ||
|
|
||
| // Protected endpoint handler | ||
| agentRoutes.post( | ||
| ENDPOINTS.AGENT.REVIEW, | ||
| (async (req: Request, res: Response, next: NextFunction) => { | ||
| try { | ||
| const { prUrl } = req.body; | ||
|
|
||
| // Validate PR URL | ||
| const match = prUrl.match(/^https?:\/\/(?:www\.)?github\.com\/([a-zA-Z0-9_.-]+)\/([a-zA-Z0-9_.-]+)\/pull\/(\d+)/); | ||
| if (!match) { | ||
| throw new ValidationError("Please provide a valid GitHub Pull Request URL."); | ||
| } | ||
| // Optionally reconstruct a clean URL to pass to the job | ||
| const cleanPrUrl = match[0]; | ||
| const taskId = await cloudTasksService.addManualPRAnalysisJob(cleanPrUrl); | ||
|
|
||
| responseWrapper({ | ||
| res, | ||
| status: STATUS_CODES.SUCCESS, | ||
| data: { success: true, taskId }, | ||
| message: "Review in progress. A comment will be posted by our bot." | ||
| }); | ||
| } catch (error) { | ||
| next(error); |
There was a problem hiding this comment.
Don’t settle payment before you know the request can be fulfilled.
paymentMiddlewareFromConfig(...) runs before the handler, and x402’s standard flow verifies/settles the payment before the server returns the successful resource response. With the current ordering, a missing/non-string prUrl or a Cloud Tasks enqueue failure can still charge the caller and then return an error. Move cheap body validation ahead of the paywall and add a durable/compensating path around addManualPRAnalysisJob. (github.com)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/routes/agent.route.ts` around lines 27 - 69, The payment middleware
(paymentMiddlewareFromConfig) is currently applied before request validation and
can charge callers even if the handler fails; move the cheap PR URL validation
(the regex check for prUrl in the POST handler for ENDPOINTS.AGENT.REVIEW) to
run before attaching the payment middleware so invalid requests are rejected
without charging, and implement a durable/compensating pattern around
cloudTasksService.addManualPRAnalysisJob (e.g., persist a pending-job record or
enqueue to a reliable work-queue first and only allow payment settlement after
successful enqueue, or implement refund/compensation logic when
addManualPRAnalysisJob throws) so failures in addManualPRAnalysisJob do not
result in unrecoverable charges.
api/routes/agent.route.ts
Outdated
| agentRoutes.use( | ||
| paymentMiddlewareFromConfig( | ||
| { | ||
| [`POST ${ENDPOINTS.AGENT.REVIEW}`]: { | ||
| accepts: { | ||
| scheme: "exact", | ||
| price: "$0.50", | ||
| network, | ||
| payTo | ||
| }, | ||
| description: "Pull request review", | ||
| mimeType: "application/json" | ||
| } | ||
| } as RoutesConfig, | ||
| facilitatorClient, | ||
| [{ network, server: new ExactStellarScheme() }] | ||
| ) |
There was a problem hiding this comment.
Verify this matches the current x402 v2 server API.
The code is on the v2 @x402/* packages, but Coinbase’s current TypeScript seller docs show paymentMiddleware(..., server) with accepts: [{ ... }] and an explicitly registered resource server. This route still passes a scalar accepts object through paymentMiddlewareFromConfig, so please confirm it emits valid payment requirements for the installed package version before shipping the paywall. (docs.cdp.coinbase.com)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/routes/agent.route.ts` around lines 27 - 43, The payment middleware
configuration currently passes a single object as the `accepts` value to
paymentMiddlewareFromConfig (in agentRoutes usage of paymentMiddlewareFromConfig
with ENDPOINTS.AGENT.REVIEW and ExactStellarScheme), but the installed x402 v2
API expects `accepts` to be an array and a separately registered resource server
parameter; update the call so the route's config uses `accepts: [{ scheme:
"exact", price: "$0.50", network, payTo, mimeType: "application/json",
description: "Pull request review" }]` and pass the resource server instance
according to the v2 signature (verify whether paymentMiddlewareFromConfig
expects `facilitatorClient, server` or a servers list and adjust usage of
facilitatorClient and ExactStellarScheme accordingly), then run the TypeScript
build to confirm the types (RoutesConfig, paymentMiddlewareFromConfig,
agentRoutes) match the package's v2 definitions.
| }, | ||
| endpoints: { | ||
| "pr-analysis": ENDPOINTS.INTERNAL.PREFIX + ENDPOINTS.INTERNAL.PR_ANALYSIS, | ||
| "manual-pr-analysis": ENDPOINTS.INTERNAL.PREFIX + ENDPOINTS.INTERNAL.MANUAL_PR_ANALYSIS, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for route handler registration for /internal/manual-pr-analysis
# Search for MANUAL_PR_ANALYSIS route registration in internal routes
rg -n "MANUAL_PR_ANALYSIS" --type=ts -C3
# Search for internal route definitions
ast-grep --pattern 'router.$_($_, $$$)'
# Check internal route file specifically
fd -t f "internal" --extension ts --exec cat {}Repository: devasignhq/devasign-api
Length of output: 50379
🏁 Script executed:
# Search for internal route files
find api/routes -type f -name "*.route.ts" ! -path "*/_test_/*" | head -20Repository: devasignhq/devasign-api
Length of output: 259
🏁 Script executed:
# Search for route registrations that handle manual-pr-analysis
rg -n "manual-pr-analysis|MANUAL_PR_ANALYSIS" api/routes --type=ts -lRepository: devasignhq/devasign-api
Length of output: 49
🏁 Script executed:
# Look for internal or agent route files specifically
fd -t f "\.route\.ts$" api/routesRepository: devasignhq/devasign-api
Length of output: 420
🏁 Script executed:
cat api/routes/internal.route.tsRepository: devasignhq/devasign-api
Length of output: 409
🏁 Script executed:
# Also check agent.route.ts to see the manual-pr-analysis job dispatch
rg -n "addManualPRAnalysisJob" api/routes/agent.route.ts -A 10 -B 5Repository: devasignhq/devasign-api
Length of output: 753
Add missing internal route handler for manual PR analysis.
The Cloud Tasks service is configured to dispatch tasks to /internal/manual-pr-analysis (cloud-tasks.service.ts:92), but the internal route file (api/routes/internal.route.ts) has no handler registered for this endpoint. Cloud Tasks will receive 404 errors when attempting to execute queued jobs. Add a route handler for ENDPOINTS.INTERNAL.MANUAL_PR_ANALYSIS to internal.route.ts that processes the manual PR analysis task.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/services/cloud-tasks.service.ts` at line 92, Add a route handler for the
internal manual PR analysis endpoint so Cloud Tasks doesn't get 404s: in
internal.route.ts register a POST handler for
ENDPOINTS.INTERNAL.MANUAL_PR_ANALYSIS (or the router method used by existing
internal routes, e.g., registerInternalRoutes/router.post) that validates the
incoming task payload, invokes the same processing logic used for manual PR
analysis (call the service function that performs the analysis), and returns the
appropriate 200/4xx response; ensure the handler name and exported router follow
the project's routing conventions so the endpoint matches the path configured in
cloud-tasks.service.ts.
| import { Statsig, StatsigUser } from "statsig-node"; | ||
| import { dataLogger } from "../config/logger.config.js"; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
statsig-node named exports Statsig class API
💡 Result:
statsig-node (legacy Node.js Server SDK) does not expose a named Statsig class. The module’s primary export is the global Statsig singleton (shown in docs as const Statsig = require("statsig-node");).[1]
How to import
CommonJS (documented)
const Statsig = require("statsig-node");
await Statsig.initialize("server-secret-key", options);[1]
ESM / TypeScript (typical interop)
import Statsig from "statsig-node";
await Statsig.initialize("server-secret-key", options);(ESM default import works in many setups via CommonJS interop; if your tooling doesn’t, use createRequire or import * as Statsig from "statsig-node".)
“Class-based” API (the named export you likely want)
For multiple independent SDK instances, the SDK exposes an instance-based class StatsigServer (used as new StatsigServer(secretKey, options) then initializeAsync()).[1]
import { StatsigServer } from "statsig-node";
const sdk = new StatsigServer(secretKey, options);
await sdk.initializeAsync();[1]
Main singleton API surface (what require("statsig-node") gives you)
The exported singleton (statsig/Statsig) provides methods like initialize, checkGate, getConfig, getExperiment, logEvent, shutdown, etc.[2]
Sources: Statsig Legacy Node.js Server SDK docs[1] and the SDK’s generated README/API doc in the package.[2]
🏁 Script executed:
cd api/services && head -120 statsig.service.ts | cat -nRepository: devasignhq/devasign-api
Length of output: 4538
🌐 Web query:
statsig-node StatsigUser type named export TypeScript
💡 Result:
In statsig-node, StatsigUser is a TypeScript-only type export (export type StatsigUser = ...). [1] So you should import it as a type-only named import:
import Statsig from "statsig-node";
import type { StatsigUser } from "statsig-node";
const user: StatsigUser = { userID: "123" };If you want a single import line (TS 3.8+), you can do:
import Statsig, { type StatsigUser } from "statsig-node";Type-only imports are erased from JS output, which avoids runtime “named export” issues. [2]
Sources: [1] [2]
Fix statsig-node import statement.
The import uses named exports, but statsig-node exports Statsig as a default export and StatsigUser as a type-only named export. Update the import to:
import Statsig, { type StatsigUser } from "statsig-node";The current import will cause a runtime error when trying to destructure Statsig as a named export.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/services/statsig.service.ts` around lines 1 - 2, The import for
statsig-node is wrong: replace the named import of Statsig and StatsigUser with
a default import for Statsig and a type-only named import for StatsigUser (i.e.,
import Statsig, { type StatsigUser } from "statsig-node") so the runtime uses
the default export; update the top-level import statement that currently reads
"import { Statsig, StatsigUser } from 'statsig-node'" to the corrected form and
keep dataLogger usage unchanged.
| const messagesCol = { | ||
| doc: vi.fn((id?: string) => ({ | ||
| id: id || `mock_message_${Date.now()}`, | ||
| get: vi.fn().mockResolvedValue({ | ||
| exists: true, | ||
| id: id || `mock_message_${Date.now()}`, | ||
| data: () => mockMessages.find(m => m.id === id) || {} | ||
| }), | ||
| set: vi.fn().mockResolvedValue(undefined), | ||
| update: vi.fn().mockResolvedValue(undefined) | ||
| })), | ||
| where: vi.fn().mockReturnThis(), | ||
| orderBy: vi.fn().mockReturnThis(), | ||
| get: vi.fn().mockResolvedValue({ | ||
| docs: mockMessages.map(msg => ({ | ||
| id: msg.id, | ||
| data: () => msg | ||
| })) | ||
| }) | ||
| }; |
There was a problem hiding this comment.
Return a live snapshot from messagesCol.get().
mockResolvedValue({ docs: mockMessages.map(...) }) captures the collection contents once at mock construction time. Later createMessage calls will never appear in collection("messages").get(), so tests that read through the Firestore API will see stale state. Switch this to mockImplementation(async () => ({ docs: ... })) so each call reflects the current array.
Suggested fix
- get: vi.fn().mockResolvedValue({
- docs: mockMessages.map(msg => ({
- id: msg.id,
- data: () => msg
- }))
- })
+ get: vi.fn().mockImplementation(async () => ({
+ docs: mockMessages.map(msg => ({
+ id: msg.id,
+ data: () => msg
+ }))
+ }))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const messagesCol = { | |
| doc: vi.fn((id?: string) => ({ | |
| id: id || `mock_message_${Date.now()}`, | |
| get: vi.fn().mockResolvedValue({ | |
| exists: true, | |
| id: id || `mock_message_${Date.now()}`, | |
| data: () => mockMessages.find(m => m.id === id) || {} | |
| }), | |
| set: vi.fn().mockResolvedValue(undefined), | |
| update: vi.fn().mockResolvedValue(undefined) | |
| })), | |
| where: vi.fn().mockReturnThis(), | |
| orderBy: vi.fn().mockReturnThis(), | |
| get: vi.fn().mockResolvedValue({ | |
| docs: mockMessages.map(msg => ({ | |
| id: msg.id, | |
| data: () => msg | |
| })) | |
| }) | |
| }; | |
| const messagesCol = { | |
| doc: vi.fn((id?: string) => ({ | |
| id: id || `mock_message_${Date.now()}`, | |
| get: vi.fn().mockResolvedValue({ | |
| exists: true, | |
| id: id || `mock_message_${Date.now()}`, | |
| data: () => mockMessages.find(m => m.id === id) || {} | |
| }), | |
| set: vi.fn().mockResolvedValue(undefined), | |
| update: vi.fn().mockResolvedValue(undefined) | |
| })), | |
| where: vi.fn().mockReturnThis(), | |
| orderBy: vi.fn().mockReturnThis(), | |
| get: vi.fn().mockImplementation(async () => ({ | |
| docs: mockMessages.map(msg => ({ | |
| id: msg.id, | |
| data: () => msg | |
| })) | |
| })) | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/mocks/firebase.service.mock.ts` around lines 16 - 35, The
messagesCol.get() mock currently uses mockResolvedValue with a snapshot of
mockMessages at construction, causing later createMessage calls not to appear;
change the messagesCol.get mock (the vi.fn assigned to get on messagesCol) to
use mockImplementation(async () => ({ docs: mockMessages.map(msg => ({ id:
msg.id, data: () => msg })) })) so each invocation returns a live snapshot built
from the current mockMessages array (ensure it's async so callers still receive
a Promise).
2 participants
Closes #86
Summary by CodeRabbit
New Features
Updates
Chores