refactor: small improvements for cal ai#23444
Conversation
Walkthrough
Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
keithwillcode
added
core
Udit-takkar
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/lib/formatPhoneNumber.ts (1)
3-6: Harden against parse exceptions to avoid UI crashes.
parsePhoneNumberWithErrorthrows on invalid input; current code will throw. Wrap in try/catch and fall back to the original string.export const formatPhoneNumber = (phoneNumber: string) => { - const parsedPhoneNumber = parsePhoneNumberWithError(phoneNumber); - return parsedPhoneNumber?.isValid() ? parsedPhoneNumber.formatInternational() : phoneNumber; + try { + const parsed = parsePhoneNumberWithError(phoneNumber); + return parsed.isValid() ? parsed.formatInternational() : phoneNumber; + } catch { + return phoneNumber; + } };packages/lib/server/repository/PrismaApiKeyRepository.ts (1)
7-30: Exclude sensitivehashedKeyfrom Prisma queries
- In packages/lib/server/repository/PrismaApiKeyRepository.ts, change the
findManyto explicitlyselectonly non-sensitive fields (omittinghashedKey):const apiKeys = await prisma.apiKey.findMany({ where: { userId, OR: [ { NOT: { appId: "zapier" } }, { appId: null }, ], }, orderBy: { createdAt: "desc" }, + select: { + id: true, + userId: true, + teamId: true, + appId: true, + note: true, + createdAt: true, + updatedAt: true, + expiresAt: true, + lastUsedAt: true, + }, });
- Audit all other Prisma calls that currently return full API-key records (grep for
hashedKeyin.tsfiles) and apply the same pattern—especially in tRPC handlers like create.handler.ts—to ensurehashedKeyis never sent downstream.packages/features/calAIPhone/providers/retellAI/services/AgentService.ts (3)
115-121: Don’t mint API keys from caller-supplied IDs; use verified owner context.Using data.userId/teamId lets a caller forge API keys attributed to another principal. Derive userId/teamId from the authorized agent/eventType owner before calling createApiKey.
Apply within this range:
- const apiKey = - reusableKey ?? - (await this.createApiKey({ - userId: data.userId, - teamId: data.teamId || undefined, - })); + const apiKey = + reusableKey ?? + (await this.createApiKey({ + userId: verifiedOwner.userId, + teamId: verifiedOwner.teamId, + }));Add earlier in the method (outside this hunk), after validating inputs, to compute verifiedOwner:
// Example – replace with actual repo method you have: const agentRecord = await this.agentRepository.findByProviderAgentIdWithUserAccess({ providerAgentId: agentId, userId: data.userId!, // caller identity from session/context teamId: data.teamId ?? undefined, }); if (!agentRecord) { throw new HttpError({ statusCode: 403, message: "You don't have access to this agent." }); } const verifiedOwner = { userId: agentRecord.userId, teamId: agentRecord.teamId }; /** Also assert eventTypeId belongs to verifiedOwner before proceeding. **/
158-167: Redact secrets from error logs.Retell errors may echo payloads containing cal_api_key. Avoid logging raw error objects; sanitize keys like cal_api_key, authorization, x-api-key.
Apply within this range:
- this.logger.error("Failed to update agent general tools in external AI service", { + this.logger.error("Failed to update agent general tools in external AI service", { agentId, - error, + error: redactSecrets(error), });Define once in this file:
function redactSecrets(err: unknown) { try { const json = JSON.parse(JSON.stringify(err)); const redact = (o: any) => { if (!o || typeof o !== "object") return; for (const k of Object.keys(o)) { if (/(key|token|secret|authorization|cal_api_key)/i.test(k)) o[k] = "***redacted***"; else redact(o[k]); } }; redact(json); return json; } catch { return { message: err instanceof Error ? err.message : String(err) }; } }
59-75: Enforce agent and eventType ownership in updateToolsFromAgentId
Before calling the external update, load and verify access to both resources:
- Call this.agentRepository.findByProviderAgentIdWithUserAccess({ providerAgentId: agentId, userId: data.userId }) and throw a 403 if it returns null.
- Call this.eventTypeRepository.findByIdWithUserAccess({ id: data.eventTypeId!, userId: data.userId }) and throw a 403 if it returns null.
Use the returned agent’s userId/teamId for all downstream operations.
🧹 Nitpick comments (4)
packages/lib/formatPhoneNumber.ts (1)
1-1: Double-check bundle-size impact oflibphonenumber-js/max.
maxships full metadata and can noticeably increase client bundles if used in the browser. If this runs client-side, consider a lighter entry (e.g., dynamic import ofmaxonly where needed) or document the rationale.packages/trpc/server/routers/viewer/workflows/update.handler.ts (1)
806-809: Good switch to TRPCError; consider a more specific code and limiting client-facing detail.Optional: use
BAD_GATEWAY/FAILED_DEPENDENCYfor external-service failures and keep detailed errors in logs while sending a concise message to clients.- throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: externalToolErrors.join("; "), - }); + throw new TRPCError({ + code: "BAD_GATEWAY", + message: "Failed to update external agent tools", + cause: externalToolErrors, + });Confirm the TRPC version supports the suggested
codeandcausevalues in your stack.packages/features/ee/workflows/components/AgentConfigurationSheet.tsx (1)
942-944: Addrel="noopener noreferrer"for external link security.Opening a new tab should include
relto prevent tab-nabbing.- target="_blank" + target="_blank" + rel="noopener noreferrer"packages/features/calAIPhone/providers/retellAI/services/AgentService.ts (1)
29-34: Consider TTL for ephemeral API keys.For agent-tool usage, long‑lived keys increase secret sprawl. Consider passing an expiresAt (e.g., 90–180 days) or making TTL configurable via env.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/web/public/static/locales/en/common.json(1 hunks)packages/features/calAIPhone/providers/retellAI/services/AgentService.ts(2 hunks)packages/features/ee/workflows/components/AgentConfigurationSheet.tsx(1 hunks)packages/lib/formatPhoneNumber.ts(1 hunks)packages/lib/server/repository/PrismaApiKeyRepository.ts(2 hunks)packages/trpc/server/routers/viewer/workflows/update.handler.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/trpc/server/routers/viewer/workflows/update.handler.tspackages/lib/formatPhoneNumber.tspackages/lib/server/repository/PrismaApiKeyRepository.tspackages/features/calAIPhone/providers/retellAI/services/AgentService.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/trpc/server/routers/viewer/workflows/update.handler.tspackages/features/ee/workflows/components/AgentConfigurationSheet.tsxpackages/lib/formatPhoneNumber.tspackages/lib/server/repository/PrismaApiKeyRepository.tspackages/features/calAIPhone/providers/retellAI/services/AgentService.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/trpc/server/routers/viewer/workflows/update.handler.tspackages/features/ee/workflows/components/AgentConfigurationSheet.tsxpackages/lib/formatPhoneNumber.tspackages/lib/server/repository/PrismaApiKeyRepository.tspackages/features/calAIPhone/providers/retellAI/services/AgentService.ts
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/ee/workflows/components/AgentConfigurationSheet.tsx
**/*Repository.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Repository files must include
Repositorysuffix, prefix with technology if applicable (e.g.,PrismaAppRepository.ts), and use PascalCase matching the exported class
Files:
packages/lib/server/repository/PrismaApiKeyRepository.ts
**/*Service.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/features/calAIPhone/providers/retellAI/services/AgentService.ts
🧠 Learnings (8)
📓 Common learnings
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts:212-220
Timestamp: 2025-08-08T10:26:13.362Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts should include the phone number in client-facing HttpError messages (e.g., in updatePhoneNumber/getPhoneNumber catch blocks). Do not suggest redacting the phone number from these errors unless requirements change (per maintainer: Udit-takkar).
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/AgentService.ts:195-216
Timestamp: 2025-08-08T09:27:23.896Z
Learning: In PR calcom/cal.com#22919, file packages/features/calAIPhone/providers/retellAI/services/AgentService.ts, the updateAgentConfiguration method intentionally does not persist the optional `name` parameter to the repository for now, per maintainer (Udit-takkar). Future reviews should not flag this unless requirements change.
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/interfaces/AIPhoneService.interface.ts:118-143
Timestamp: 2025-08-08T09:29:11.681Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/interfaces/AIPhoneService.interface.ts (TypeScript), the AIPhoneServiceAgentListItem is required to include user.email in listAgents responses (per maintainer Udit-takkar). Future reviews should not flag this as unnecessary PII unless requirements change.
📚 Learning: 2025-08-26T20:23:28.396Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/features/calAIPhone/providers/retellAI/services/AgentService.ts:83-88
Timestamp: 2025-08-26T20:23:28.396Z
Learning: In calcom/cal.com PR #22995, the workflow update handler in packages/trpc/server/routers/viewer/workflows/update.handler.ts includes workflow-level authorization via isAuthorized(userWorkflow, ctx.user.id, "workflow.update") which validates the user can update the workflow before calling updateToolsFromAgentId (per maintainer Udit-takkar).
Applied to files:
packages/trpc/server/routers/viewer/workflows/update.handler.ts
📚 Learning: 2025-08-15T00:27:33.280Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/features/ee/workflows/components/AgentConfigurationSheet.tsx:348-371
Timestamp: 2025-08-15T00:27:33.280Z
Learning: In calcom/cal.com, the variable insertion pattern in AgentConfigurationSheet.tsx follows the same implementation as existing workflow code. The current approach of building variable tokens directly from the input variable (with toUpperCase and space replacement) is the established pattern and doesn't need to be changed for locale-agnostic concerns, per maintainer Udit-takkar.
Applied to files:
packages/features/ee/workflows/components/AgentConfigurationSheet.tsx
📚 Learning: 2025-08-15T00:27:33.280Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/features/ee/workflows/components/AgentConfigurationSheet.tsx:348-371
Timestamp: 2025-08-15T00:27:33.280Z
Learning: In calcom/cal.com workflows and AI agent components, variable insertion follows a consistent pattern of directly transforming the input variable with toUpperCase() and replace(/ /g, "_") to create tokens like {VARIABLE_NAME}. The AgentConfigurationSheet.tsx implementation correctly follows this same pattern as WorkflowStepContainer.tsx, per maintainer Udit-takkar in PR #22995.
Applied to files:
packages/features/ee/workflows/components/AgentConfigurationSheet.tsx
📚 Learning: 2025-08-08T10:26:13.362Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts:212-220
Timestamp: 2025-08-08T10:26:13.362Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts should include the phone number in client-facing HttpError messages (e.g., in updatePhoneNumber/getPhoneNumber catch blocks). Do not suggest redacting the phone number from these errors unless requirements change (per maintainer: Udit-takkar).
Applied to files:
packages/lib/formatPhoneNumber.ts
📚 Learning: 2025-08-08T09:27:23.896Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/AgentService.ts:195-216
Timestamp: 2025-08-08T09:27:23.896Z
Learning: In PR calcom/cal.com#22919, file packages/features/calAIPhone/providers/retellAI/services/AgentService.ts, the updateAgentConfiguration method intentionally does not persist the optional `name` parameter to the repository for now, per maintainer (Udit-takkar). Future reviews should not flag this unless requirements change.
Applied to files:
packages/features/calAIPhone/providers/retellAI/services/AgentService.ts
📚 Learning: 2025-08-27T12:15:43.830Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/aiVoiceAgent/testCall.handler.ts:41-44
Timestamp: 2025-08-27T12:15:43.830Z
Learning: In calcom/cal.com, the AgentService.getAgent() method in packages/features/calAIPhone/providers/retellAI/services/AgentService.ts does NOT include authorization checks - it only validates the agentId parameter and directly calls the repository without verifying user/team access. This contrasts with other methods like getAgentWithDetails() which properly use findByIdWithUserAccessAndDetails() for authorization. When reviewing updateToolsFromAgentId() calls, always verify both agent ownership and eventType ownership are checked.
Applied to files:
packages/features/calAIPhone/providers/retellAI/services/AgentService.ts
📚 Learning: 2025-08-08T09:29:11.681Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/interfaces/AIPhoneService.interface.ts:118-143
Timestamp: 2025-08-08T09:29:11.681Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/interfaces/AIPhoneService.interface.ts (TypeScript), the AIPhoneServiceAgentListItem is required to include user.email in listAgents responses (per maintainer Udit-takkar). Future reviews should not flag this as unnecessary PII unless requirements change.
Applied to files:
packages/features/calAIPhone/providers/retellAI/services/AgentService.ts
🧬 Code graph analysis (2)
packages/trpc/server/routers/viewer/workflows/update.handler.ts (1)
packages/platform/libraries/index.ts (1)
TRPCError(56-56)
packages/features/calAIPhone/providers/retellAI/services/AgentService.ts (1)
packages/lib/server/repository/PrismaApiKeyRepository.ts (1)
PrismaApiKeyRepository(6-61)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (3)
apps/web/public/static/locales/en/common.json (1)
820-821: LGTM — terminology fix.Capitalizing “URI” improves clarity and consistency.
packages/lib/server/repository/PrismaApiKeyRepository.ts (1)
32-60: No changes required:generateUniqueAPIKeyreturns a two-element array, so[hashedApiKey, apiKey] = generateHashedApiKey()is correct.Likely an incorrect or invalid review comment.
packages/features/calAIPhone/providers/retellAI/services/AgentService.ts (1)
6-6: Good abstraction: API key creation moved to repository.Centralizing key generation/persistence in PrismaApiKeyRepository reduces duplication and keeps hashing logic in one place.
E2E results are ready! |
4 participants
What does this PR do?
small follow up #22995
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?