chore: improve local testing retell #23577
Conversation
WalkthroughAdds test-mode overrides for Retell AI. .env.example introduces RETELL_AI_TEST_MODE, RETELL_AI_TEST_EVENT_TYPE_MAP, and RETELL_AI_TEST_CAL_API_KEY. turbo.json registers these in globalEnv. packages/lib/constants.ts exports RETELL_AI_TEST_MODE (boolean) and RETELL_AI_TEST_EVENT_TYPE_MAP (JSON-parsed map with warning on parse failure). AgentService.ts consumes these: when test mode is enabled, maps event type IDs for tool existence checks, creation, assignments, and cleanup/removal; logs mapped IDs; and optionally uses RETELL_AI_TEST_CAL_API_KEY for API access. Non-test-mode behavior remains unchanged. No exported/public API signatures are altered. Possibly related PRs
✨ 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
![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 (1)
packages/features/calAIPhone/providers/retellAI/services/AgentService.ts (1)
60-76: Add agent & event type ownership checks inupdateToolsFromAgentId
packages/trpc/server/routers/viewer/workflows/update.handler.tsalready enforcesisAuthorized(userWorkflow, ctx.user.id, "workflow.update")before calling the service.packages/trpc/server/routers/viewer/aiVoiceAgent/testCall.handler.tsinvokesupdateToolsFromAgentIdwith no ownership guard.- Move ownership assertions into
AgentService.updateToolsFromAgentIdso all call sites are protected:async updateToolsFromAgentId( agentId: string, data: { eventTypeId: number | null; timeZone: string; userId: number | null; teamId?: number | null } ) { if (!agentId?.trim()) { throw new HttpError({ statusCode: 400, message: "Agent ID is required and cannot be empty" }); } if (!data.eventTypeId || !data.userId) { throw new HttpError({ statusCode: 400, message: "Event type ID and user ID are required" }); } + // Verify agent ownership/admin access + const hasAgentAccess = await this.agentRepository.findByIdWithAdminAccess({ + id: /* resolve internal agent DB id for providerAgentId */, + userId: data.userId, + teamId: data.teamId ?? undefined, + }); + if (!hasAgentAccess) { + throw new HttpError({ statusCode: 403, message: "No permission to modify this agent." }); + } + + // Verify event type ownership + await this.agentRepository.assertCanManageEventType({ + userId: data.userId, + teamId: data.teamId ?? undefined, + eventTypeId: data.eventTypeId, + });
🧹 Nitpick comments (6)
.env.example (2)
391-393: Fix dotenv-linter ordering and clarify JSON guidanceReorder and tighten the comment so users don’t paste invalid JSON. This also resolves the UnorderedKey warnings.
Apply:
-# JSON mapping of local to production event type IDs (e.g. {"50": 747280} without any string) -RETELL_AI_TEST_EVENT_TYPE_MAP= -RETELL_AI_TEST_CAL_API_KEY= +# JSON mapping from local → production event type IDs. +# Keys must be strings (JSON requirement); values must be numbers. Example: {"50": 747280} +RETELL_AI_TEST_CAL_API_KEY= +RETELL_AI_TEST_EVENT_TYPE_MAP=
390-394: Optional: keep keys sorted for consistencyIf you prefer alpha order for this block: CAL_API_KEY, EVENT_TYPE_MAP, MODE.
-RETELL_AI_TEST_MODE=false -RETELL_AI_TEST_CAL_API_KEY= -RETELL_AI_TEST_EVENT_TYPE_MAP= +RETELL_AI_TEST_CAL_API_KEY= +RETELL_AI_TEST_EVENT_TYPE_MAP= +RETELL_AI_TEST_MODE=falsepackages/lib/constants.ts (1)
245-254: Type and validate RETELL_AI_TEST_EVENT_TYPE_MAP to avoid “any” and bad inputsStrong-typing prevents accidental non-numeric values; also trims junk.
-// Retell AI test mode configuration -export const RETELL_AI_TEST_MODE = process.env.RETELL_AI_TEST_MODE === "true"; -export const RETELL_AI_TEST_EVENT_TYPE_MAP = (() => { - if (!process.env.RETELL_AI_TEST_EVENT_TYPE_MAP) return null; - try { - return JSON.parse(process.env.RETELL_AI_TEST_EVENT_TYPE_MAP); - } catch (e) { - console.warn("Failed to parse RETELL_AI_TEST_EVENT_TYPE_MAP", e); - return null; - } -})(); +// Retell AI test mode configuration +export const RETELL_AI_TEST_MODE = process.env.RETELL_AI_TEST_MODE === "true"; +export const RETELL_AI_TEST_EVENT_TYPE_MAP: Record<string, number> | null = (() => { + const raw = process.env.RETELL_AI_TEST_EVENT_TYPE_MAP; + if (!raw) return null; + try { + const parsed = JSON.parse(raw) as Record<string, unknown>; + const out: Record<string, number> = {}; + for (const [k, v] of Object.entries(parsed)) { + const n = typeof v === "string" ? Number(v.trim()) : (v as number); + if (Number.isFinite(n)) out[String(k)] = Math.trunc(n); + } + return Object.keys(out).length ? Object.freeze(out) : null; + } catch (e) { + console.warn("Failed to parse RETELL_AI_TEST_EVENT_TYPE_MAP", e); + return null; + } +})();packages/features/calAIPhone/providers/retellAI/services/AgentService.ts (3)
85-91: DRY the eventTypeId mapping into a helper to reduce mistakesMapping logic is duplicated in three places; centralize to avoid divergence.
export class AgentService { private logger = logger.getSubLogger({ prefix: ["AgentService"] }); + // Map local → production eventTypeId in test mode + private mapEventTypeId = (id: number): number => { + if (RETELL_AI_TEST_MODE && RETELL_AI_TEST_EVENT_TYPE_MAP) { + const mapped = RETELL_AI_TEST_EVENT_TYPE_MAP[String(id)]; + return mapped ? Number(mapped) : id; + } + return id; + }; + constructor(- let eventTypeId = data.eventTypeId; - - if (RETELL_AI_TEST_MODE && RETELL_AI_TEST_EVENT_TYPE_MAP) { - const mappedId = RETELL_AI_TEST_EVENT_TYPE_MAP[String(data.eventTypeId)]; - eventTypeId = mappedId ? Number(mappedId) : data.eventTypeId; - } + const eventTypeId = this.mapEventTypeId(data.eventTypeId);- let mappedEventTypeIds = eventTypeIds; - - if (RETELL_AI_TEST_MODE && RETELL_AI_TEST_EVENT_TYPE_MAP) { - mappedEventTypeIds = eventTypeIds.map((id) => { - const mappedId = RETELL_AI_TEST_EVENT_TYPE_MAP[String(id)]; - return mappedId ? Number(mappedId) : id; - }); - } + const mappedEventTypeIds = eventTypeIds.map((id) => this.mapEventTypeId(id));- let mappedActiveEventTypeIds = activeEventTypeIds; - - if (RETELL_AI_TEST_MODE && RETELL_AI_TEST_EVENT_TYPE_MAP) { - mappedActiveEventTypeIds = activeEventTypeIds.map((id) => { - const mappedId = RETELL_AI_TEST_EVENT_TYPE_MAP[String(id)]; - return mappedId ? Number(mappedId) : id; - }); - } + const mappedActiveEventTypeIds = activeEventTypeIds.map((id) => this.mapEventTypeId(id));Also applies to: 193-201, 263-271
124-131: API key selection path: safe precedence; add tiny guard to avoid empty stringsIf RETELL_AI_TEST_CAL_API_KEY is set to an empty string, it will be truthy in some shells but falsy here; explicitly trim.
- RETELL_AI_TEST_MODE && process.env.RETELL_AI_TEST_CAL_API_KEY - ? process.env.RETELL_AI_TEST_CAL_API_KEY + RETELL_AI_TEST_MODE && (process.env.RETELL_AI_TEST_CAL_API_KEY?.trim() ?? "") !== "" + ? process.env.RETELL_AI_TEST_CAL_API_KEY!.trim() : reusableKey ??
221-227: Minor perf/readability: build a Set for membership checksO(n*m) includes() is fine for small arrays, but Set clarifies intent and is linear.
- const toolNamesToRemove = mappedEventTypeIds.flatMap((eventTypeId) => [ - `check_availability_${eventTypeId}`, - `book_appointment_${eventTypeId}`, - ]); - - const filteredTools = existing.filter((tool) => !toolNamesToRemove.includes(tool.name)); + const toolNamesToRemove = new Set( + mappedEventTypeIds.flatMap((id) => [`check_availability_${id}`, `book_appointment_${id}`]) + ); + const filteredTools = existing.filter((tool) => !toolNamesToRemove.has(tool.name));
📜 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 (4)
.env.example(1 hunks)packages/features/calAIPhone/providers/retellAI/services/AgentService.ts(10 hunks)packages/lib/constants.ts(1 hunks)turbo.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/lib/constants.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/lib/constants.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/lib/constants.tspackages/features/calAIPhone/providers/retellAI/services/AgentService.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 (4)
📚 Learning: 2025-08-21T16:30:19.309Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/aiVoiceAgent/get.handler.ts:16-20
Timestamp: 2025-08-21T16:30:19.309Z
Learning: In calcom/cal.com Cal AI voice agent endpoints, it's acceptable to return cal_api_key values in the general_tools array when they belong to the requesting user themselves. This is an exception to the general rule about not returning credential.key fields, as users need to see their own API keys for configuration purposes (per maintainer Udit-takkar in PR #22995).
Applied to files:
.env.examplepackages/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: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-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 (1)
packages/features/calAIPhone/providers/retellAI/services/AgentService.ts (2)
packages/lib/constants.ts (2)
RETELL_AI_TEST_MODE(245-245)RETELL_AI_TEST_EVENT_TYPE_MAP(246-254)packages/features/calAIPhone/interfaces/AIPhoneService.interface.ts (1)
AIPhoneServiceTools(81-82)
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 392-392: [UnorderedKey] The RETELL_AI_TEST_EVENT_TYPE_MAP key should go before the RETELL_AI_TEST_MODE key
(UnorderedKey)
[warning] 393-393: [UnorderedKey] The RETELL_AI_TEST_CAL_API_KEY key should go before the RETELL_AI_TEST_EVENT_TYPE_MAP key
(UnorderedKey)
🔇 Additional comments (1)
turbo.json (1)
152-154: Confirm server-only usage of RETELL test env vars
• Found references in packages/lib/constants.ts (lines 245, 247, 249) and services/AgentService.ts (lines 124–125); ensure neither file is imported into any client-side bundle to avoid exposing secrets.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
E2E results are ready! |
What does this PR do?
Added environment variable-based configuration to enable local testing of Retell AI services using production event type IDs and API keys.
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?