Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughType declarations were centralized and standardized across CLI and provider modules. Local interfaces were removed in favor of shared types. A new dynamic Google GenAI client creation path and environment key normalization were added. Provider registry utilities were introduced. Documentation for the refactors was updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Env as Env Vars
participant Module as googleAiStudio.ts
participant Loader as Dynamic Import (@google/genai)
participant Google as GoogleGenAI Client
participant Live as Live Session
User->>Module: start audio streaming
Module->>Env: check GOOGLE_GENERATIVE_AI_API_KEY
Env-->>Module: not set
Module->>Env: read GOOGLE_AI_API_KEY
Env-->>Module: value (if present)
Module->>Module: normalize env key (propagate if needed)
Module->>Loader: import("@google/genai")
alt import succeeds
Loader-->>Module: { GoogleGenAI }
Module->>Google: new GoogleGenAI({ apiKey })
Google-->>Module: client
Module->>Google: client.live.connect(config)
Google-->>Live: session
Live-->>Module: session established
Module-->>User: streaming proceeds
else import fails
Loader--x Module: error
Module-->>User: AuthenticationError (install @google/genai)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
src/lib/types/cli.ts (1)
334-356: Duplicate GenerateResult type conflicts with core generateTypesThis file redefines GenerateResult, which already exists in ./generateTypes.ts (different shape). This will create ambiguity across modules and fragile imports.
Apply one of:
- Preferred: reuse core type and extend for CLI needs.
+import type { GenerateResult as CoreGenerateResult } from "./generateTypes.js"; -export type GenerateResult = CommandResult & { +export type CLIGenerateResult = CommandResult & CoreGenerateResult & { content: string; provider?: string; model?: string; usage?: TokenUsage; responseTime?: number; toolCalls?: ToolCall[]; toolResults?: ToolResult[]; analytics?: AnalyticsData; evaluation?: EvaluationData; toolsUsed?: string[]; toolExecutions?: Array<{ toolName: string; args: UnknownRecord; result: unknown; executionTime: number; }>; enhancedWithTools?: boolean; availableTools?: Array<{ name: string; description: string; }>; };
- Or: rename this type to CLIGenerateResult (without importing Core type) to avoid name clash.
-export type GenerateResult = CommandResult & { +export type CLIGenerateResult = CommandResult & { ... };And update the type guard below accordingly:
-export function isGenerateResult(value: unknown): value is GenerateResult { +export function isGenerateResult(value: unknown): value is CLIGenerateResult {src/cli/factories/sagemakerCommandFactory.ts (3)
221-259: “Validate” does not validate credentialsConstructing a SageMakerClient doesn't verify creds; current checks only test presence. Call STS GetCallerIdentity (fast, no permissions beyond STS) with a short timeout.
private static validateSecureConfiguration( secureConfig: SecureConfiguration, ): void { - // Create temporary AWS SDK client with secure credentials - const tempClient = new SageMakerClient({ - region: secureConfig.region, - credentials: { - accessKeyId: secureConfig.accessKeyId, - secretAccessKey: secureConfig.secretAccessKey, - }, - }); - - // Test basic connectivity (this will throw if credentials are invalid) - // Note: We're not actually making a call here, just validating the client can be created - if ( - !tempClient || - !secureConfig.accessKeyId || - !secureConfig.secretAccessKey - ) { - throw new Error("Invalid AWS credentials provided"); - } + // Validate by calling STS.GetCallerIdentity with a short timeout + const { STSClient, GetCallerIdentityCommand } = await import("@aws-sdk/client-sts"); + const controller = new AbortController(); + const abortId = setTimeout(() => controller.abort(), 3000); + try { + const sts = new STSClient({ + region: secureConfig.region, + credentials: { + accessKeyId: secureConfig.accessKeyId, + secretAccessKey: secureConfig.secretAccessKey, + }, + }); + await sts.send(new GetCallerIdentityCommand({}), { abortSignal: controller.signal as AbortSignal }); + } catch (e) { + throw new Error("Invalid AWS credentials or STS not accessible"); + } finally { + clearTimeout(abortId); + } // Basic validation of configuration values if (!secureConfig.region || secureConfig.region.length < 3) { throw new Error("Invalid AWS region provided"); }Note: Node 18+ provides AbortController globally; if targeting lower, import it.
296-316: Avoid printing sensitive identifiersMask Access Key when displaying config status.
- logger.always(` Access Key: ${aws.accessKeyId}`); + const ak = String(aws.accessKeyId ?? ""); + const maskedAk = ak.length > 6 ? `${ak.slice(0,4)}…${ak.slice(-2)}` : "set"; + logger.always(` Access Key: ${maskedAk}`);
511-519: Do not print secrets in config outputSecret Access Key and Session Token must never be printed.
- logger.always(` Access Key: ${aws.accessKeyId}`); - logger.always(` Secret Key: ${aws.secretAccessKey}`); - logger.always(` Session Token: ${aws.sessionToken}`); + const ak = String(aws.accessKeyId ?? ""); + const maskedAk = ak.length > 6 ? `${ak.slice(0,4)}…${ak.slice(-2)}` : "set"; + logger.always(` Access Key: ${maskedAk}`); + logger.always(` Secret Key: **redacted**`); + logger.always(` Session Token: **redacted**`);src/lib/types/providers.ts (4)
139-140: ProviderName should be enum values, not keyskeyof typeof AIProviderName yields "BEDROCK"|"OPENAI"|..., but runtime uses "bedrock"|"openai"|... Fix the type.
-export type ProviderName = keyof typeof AIProviderName; +export type ProviderName = (typeof AIProviderName)[keyof typeof AIProviderName];
337-345: Name collision with ProviderConfig from providers/types.tsThis ProviderConfig (catalog of supported models) collides with the new union ProviderConfig in src/lib/providers/types.ts used for runtime configuration. Rename to avoid import confusion.
-export type ProviderConfig = { +export type ProviderCatalogEntry = { provider: AIProviderName; models: SupportedModelName[]; };And update usages below.
690-703: Follow-up to rename: update DEFAULT_PROVIDER_CONFIGSAlign with ProviderCatalogEntry rename.
-export const DEFAULT_PROVIDER_CONFIGS: ProviderConfig[] = [ +export const DEFAULT_PROVIDER_CONFIGS: ProviderCatalogEntry[] = [ { provider: AIProviderName.BEDROCK, models: [BedrockModels.CLAUDE_3_7_SONNET, BedrockModels.CLAUDE_3_5_SONNET], },
46-51: Replace all hard-coded Bedrock inference-profile ARNs with generic model IDs
- src/lib/types/providers.ts: change
CLAUDE_3_7_SONNETto"anthropic.claude-3-7-sonnet-20250219-v1:0"- src/cli/commands/setup-bedrock.ts (line 533)
- src/lib/utils/providerSetupMessages.ts (line 64)
- src/cli/commands/config.ts (lines 52, 510)
- src/lib/constants/tokens.ts (line 135)
- src/lib/core/types.ts (line 71)
Use the non-ARN form (
"anthropic.claude-<model>-<date>-v<n>:0") everywhere to avoid embedding account/region specifics.src/lib/providers/googleAiStudio.ts (1)
34-42: Improve type safety in dynamic import handlingThe dynamic import handling uses multiple type assertions which could be unsafe. Consider creating a proper type guard to validate the imported module structure.
-async function createGoogleGenAIClient(apiKey: string): Promise<GenAIClient> { - const mod: unknown = await import("@google/genai"); - const ctor = (mod as Record<string, unknown>).GoogleGenAI as unknown; - if (!ctor) { - throw new Error("@google/genai does not export GoogleGenAI"); - } - const Ctor = ctor as GoogleGenAIClass; - return new Ctor({ apiKey }); -} +async function createGoogleGenAIClient(apiKey: string): Promise<GenAIClient> { + const mod = await import("@google/genai"); + if (!mod || typeof mod !== "object" || !("GoogleGenAI" in mod)) { + throw new Error("@google/genai does not export GoogleGenAI"); + } + const Ctor = mod.GoogleGenAI as GoogleGenAIClass; + if (typeof Ctor !== "function") { + throw new Error("GoogleGenAI is not a constructor"); + } + return new Ctor({ apiKey }); +}src/lib/providers/amazonBedrock.ts (1)
78-79: Async operation in constructorThe constructor calls
performInitialHealthCheck()(line 78) which is an async method, but the call is not awaited. This means errors from the health check won't prevent object creation and the health check might not complete before the object is used.Consider one of these approaches:
- Make the health check synchronous in a factory method
- Add an
initialize()method that must be called after construction- Store the health check promise and await it in the first actual API call
Example factory pattern:
static async create(modelName?: string, neurolink?: NeuroLink): Promise<AmazonBedrockProvider> { const provider = new AmazonBedrockProvider(modelName, neurolink); await provider.performInitialHealthCheck(); return provider; }src/lib/providers/openaiCompatible.ts (1)
287-301: Bug: new URL('/v1/models', baseURL) drops base path; also clearTimeout on all pathsIf OPENAI_COMPATIBLE_BASE_URL includes a path (e.g., https://api.openrouter.ai/api/v1), using a leading slash resets to domain root, yielding https://api.openrouter.ai/v1/models. Also, clearTimeout is skipped on exceptions.
Apply:
- const modelsUrl = new URL("/v1/models", this.config.baseURL).toString(); + // Preserve any path suffix in baseURL and append "models" + const base = this.config.baseURL.endsWith("/") + ? this.config.baseURL + : this.config.baseURL + "/"; + const modelsUrl = new URL("models", base).toString(); logger.debug(`Fetching available models from: ${modelsUrl}`); - const proxyFetch = createProxyFetch(); - const controller = new AbortController(); - const t = setTimeout(() => controller.abort(), 5000); - const response = await proxyFetch(modelsUrl, { - headers: { - Authorization: `Bearer ${this.config.apiKey}`, - "Content-Type": "application/json", - }, - signal: controller.signal, - }); - clearTimeout(t); + const proxyFetch = createProxyFetch(); + const controller = new AbortController(); + const t = setTimeout(() => controller.abort(), 5000); + let response: Response; + try { + response = await proxyFetch(modelsUrl, { + headers: { + Authorization: `Bearer ${this.config.apiKey}`, + "Content-Type": "application/json", + }, + signal: controller.signal, + }); + } finally { + clearTimeout(t); + }Optionally, consider using createTimeoutController for consistency with other calls.
🧹 Nitpick comments (14)
todos/refactor/03-providers-module.md (2)
58-59: Doc path mismatch: providerSpecific.ts vs providers.tsThe PR centralizes provider-specific types in src/lib/types/providers.ts, not src/lib/types/providerSpecific.ts. Update the doc to avoid confusion for future contributors.
Apply:
-- `src/lib/types/providerSpecific.ts` - Create if needed for provider-specific types +- `src/lib/types/providers.ts` - Central place for provider-specific types
140-151: Snippet is partial and could misleadThe PROVIDERS mapping excerpt is truncated (only tail of the object shown). Either show the full mapping or remove the mid-object fragment to prevent copy-paste errors.
src/lib/types/cli.ts (2)
450-452: ConsoleOverride function signature is too restrictiveConsole methods accept arguments. Loosen signature for drop-in replacement in quiet mode.
export type ConsoleOverride = { - [method: string]: (() => void) | undefined; + [method: string]: ((...args: unknown[]) => void) | undefined; };
457-476: Update type guard if you rename CLIGenerateResultIf you adopt CLIGenerateResult, ensure the type guard reflects the new type and still only depends on “content” presence to avoid cross-module coupling.
-export function isGenerateResult(value: unknown): value is GenerateResult { +export function isGenerateResult(value: unknown): value is CLIGenerateResult { return ( typeof value === "object" && value !== null && "content" in value && - typeof (value as GenerateResult).content === "string" + typeof (value as { content?: unknown }).content === "string" ); }src/lib/types/providers.ts (2)
556-560: Constrain BedrockToolResult.statusUse a narrow union for stronger type safety.
export type BedrockToolResult = { toolUseId: string; content: Array<{ text: string }>; - status: string; + status: "success" | "error"; };
200-210: ModelCapability naming alignmentYou also have ProviderCapability elsewhere. Consider a single canonical capability union reused across model/provider contexts to reduce drift.
src/lib/providers/googleAiStudio.ts (2)
31-31: Comment references incorrect module pathThe comment states that types are imported from
../types/providerSpecific.js, but the actual import on line 16 is from../types/providers.js. Update the comment to reflect the correct module path.-// Google AI Live API types now imported from ../types/providerSpecific.js +// Google AI Live API types now imported from ../types/providers.js
45-50: Consider moving environment variable setup to a more appropriate locationSetting environment variables at module load time (lines 45-50) is a side effect that could cause unexpected behavior. This setup should ideally be done in an initialization function or configuration module.
Consider moving this environment variable normalization logic to:
- A dedicated environment configuration module
- The provider's constructor
- A static initialization method
This would make the side effects more explicit and controllable.
todos/refactor/04-cli-module.md (1)
288-299: Incorrect type name in commentThe comment on line 290 refers to
YargsArgumentsbut the actual type defined isMiddlewareFunction. This appears to be a documentation error.-// Type the middleware function +// Define middleware and argument typessrc/lib/providers/amazonBedrock.ts (2)
39-39: Comment references incorrect module pathSimilar to the googleAiStudio.ts file, the comment states types are imported from
../types/providerSpecific.js, but the actual import is from../types/providers.js.-// Bedrock-specific types now imported from ../types/providerSpecific.js +// Bedrock-specific types now imported from ../types/providers.js
875-876: Unprotected iteration counterThe
iterationvariable is initialized to 0 outside the try block (line 875) but incremented inside the while loop (line 915). If an error occurs and the loop retries, this could lead to premature termination.Consider resetting the iteration counter if implementing retry logic, or document that iterations are cumulative across retries.
src/lib/providers/index.ts (2)
45-49: ProviderClassName is fine; name may be misleadingValues are exported symbol names (aliases), not always class identifiers (e.g., "OpenAICompatible"). Consider a more accurate name like ProviderExportName. Optional only.
-export type ProviderClassName = (typeof PROVIDERS)[ProviderName]; +export type ProviderClassName = (typeof PROVIDERS)[ProviderName]; +// Optional rename: +// export type ProviderExportName = (typeof PROVIDERS)[ProviderName];
64-69: Use O(1) membership check for the type guardAvoid linear includes over an array; check property presence on PROVIDERS.
export function isValidProviderName(name: string): name is ProviderName { - return AVAILABLE_PROVIDERS.includes(name as ProviderName); + return Object.prototype.hasOwnProperty.call(PROVIDERS, name); }src/lib/providers/openaiCompatible.ts (1)
53-53: Stale comment pathUpdate comment to reference ../types/providers.js, not providerSpecific.js.
-// ModelsResponse type now imported from ../types/providerSpecific.js +// ModelsResponse type is imported from ../types/providers.js
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/cli/factories/sagemakerCommandFactory.ts(1 hunks)src/lib/providers/amazonBedrock.ts(1 hunks)src/lib/providers/googleAiStudio.ts(2 hunks)src/lib/providers/index.ts(1 hunks)src/lib/providers/openaiCompatible.ts(2 hunks)src/lib/types/cli.ts(14 hunks)src/lib/types/providers.ts(1 hunks)todos/refactor/03-providers-module.md(7 hunks)todos/refactor/04-cli-module.md(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: YasmeenOgo
PR: juspay/neurolink#145
File: src/lib/core/types.ts:0-0
Timestamp: 2025-09-02T13:50:42.770Z
Learning: The APIVersions enum in src/lib/core/types.ts now contains comprehensive API version constants for all major AI providers: Azure OpenAI (latest, stable, legacy), OpenAI (current, beta), Google AI (current, beta), and Anthropic (current). This centralization helps avoid API version drift across the codebase.
📚 Learning: 2025-09-02T13:50:42.770Z
Learnt from: YasmeenOgo
PR: juspay/neurolink#145
File: src/lib/core/types.ts:0-0
Timestamp: 2025-09-02T13:50:42.770Z
Learning: The APIVersions enum in src/lib/core/types.ts now contains comprehensive API version constants for all major AI providers: Azure OpenAI (latest, stable, legacy), OpenAI (current, beta), Google AI (current, beta), and Anthropic (current). This centralization helps avoid API version drift across the codebase.
Applied to files:
src/lib/types/providers.tssrc/lib/providers/googleAiStudio.tssrc/lib/providers/openaiCompatible.ts
📚 Learning: 2025-09-01T22:58:39.149Z
Learnt from: sudharsan-juspay
PR: juspay/neurolink#140
File: src/lib/core/types.ts:198-203
Timestamp: 2025-09-01T22:58:39.149Z
Learning: In src/lib/core/types.ts, StreamOptions (imported from streamTypes.js) and StreamingOptions are intentionally different types with different use cases. StreamingOptions is for unified AI requests with multiple provider configurations, while StreamOptions is for individual streaming operations.
Applied to files:
src/lib/types/cli.tssrc/lib/providers/openaiCompatible.ts
🧬 Code graph analysis (2)
src/lib/providers/index.ts (1)
src/lib/types/providers.ts (1)
ProviderName(139-139)
src/lib/types/cli.ts (2)
src/lib/types/generateTypes.ts (1)
GenerateResult(71-127)src/lib/types/common.ts (1)
UnknownRecord(13-13)
🪛 GitHub Check: build-check
src/cli/factories/sagemakerCommandFactory.ts
[failure] 23-23:
Cannot find module '$lib/types/index.js' or its corresponding type declarations.
🪛 GitHub Check: 🛡️ Code Quality & Security Gate
src/cli/factories/sagemakerCommandFactory.ts
[failure] 23-23:
'SecureConfiguration' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
🪛 GitHub Check: test (20)
src/cli/factories/sagemakerCommandFactory.ts
[failure] 23-23:
Cannot find module '$lib/types/index.js' or its corresponding type declarations.
🪛 GitHub Actions: CI
src/cli/factories/sagemakerCommandFactory.ts
[error] 23-23: TypeScript error TS2307: Cannot find module '$lib/types/index.js' or its corresponding type declarations. (Command: tsc --project tsconfig.cli.json)
🔇 Additional comments (10)
todos/refactor/04-cli-module.md (2)
3-17: Documentation accurately reflects completion statusThe completion verification checklist properly documents all the type consolidation work completed in this refactor.
206-283: Comprehensive error handling implementationThe error handling types and mapper function provide excellent structure for CLI error reporting with helpful suggestions for common issues.
src/lib/providers/amazonBedrock.ts (2)
64-71: Clean AWS SDK configuration approachGood practice letting the AWS SDK handle credentials natively through its standard credential chain (IAM roles, environment variables, config files, instance metadata).
244-256: Good diagnostic logging for AWS operationsThe extensive logging before API calls (lines 244-256) will be helpful for debugging AWS credential and configuration issues in production.
src/lib/providers/index.ts (4)
56-63: LGTM: Derived union list is correctThe PROVIDER_CLASS_NAMES derivation from PROVIDERS is sound.
71-78: LGTM: Direct lookup helper is correctThe mapping read is type-safe and aligns with the registry.
1-38: No action needed: import paths foramazonSagemaker.jsmatch the file names exactly—casing is consistent acrosssrc/lib/providersandsrc/lib/providers/sagemaker.
25-38: Enforce PROVIDERS ≡ AIProviderName: add a compile-time type assertion or test to ensurekeyof typeof PROVIDERSexactly matcheskeyof typeof AIProviderName, preventing any drift between the two.src/lib/providers/openaiCompatible.ts (2)
7-7: Good: centralizes ModelsResponse typeType-only import aligns with consolidation; usage at Line 309 is correct.
316-319: LGTM: response shape guardMapping model ids with defensive checks is correct.
5e9cbce to
78dec00
Compare
- Consolidate all provider-specific types from providerSpecific.ts to providers.ts - Convert 20+ CLI interfaces to type aliases using modern syntax - Remove unused type imports from Amazon Bedrock and Google AI Studio providers - Delete redundant mcp.d.ts declaration file with unused function declarations - Update import paths to use centralized provider types - Maintain backward compatibility while improving type organization - Achieve zero linting errors with clean separation of concerns
78dec00 to
8205485
Compare
ghost
mentioned this pull request
2 participants
Pull Request
Description
Type of Change
Related Issues
Changes Made
AI Provider Impact
Component Impact
Testing
Test Environment
Performance Impact
Breaking Changes
Screenshots/Demo
Checklist
Additional Notes
Summary by CodeRabbit
New Features
Refactor
Documentation