🔒 Remove hardcoded OpenAI API key fallback - #22
Conversation
- Removed hardcoded 'demo-key' from AIService initialization. - Added runtime validation to throw errors when required API keys are missing. - Added unit tests to verify fallback logic and key validation.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRemoves the hardcoded OpenAI API key fallback and adds stricter runtime validation plus tests to ensure correct failure and fallback behavior when API keys are missing. Sequence diagram for AIService provider call with API key validation and fallbacksequenceDiagram
actor Client
participant AIService
participant Provider as PrimaryProvider
participant Fallback as FallbackProvider
Client->>AIService: initialize()
activate AIService
AIService->>Provider: callAIProvider(prompt, type, primaryConfig)
activate Provider
Provider-->>Provider: validate apiKey
alt Missing apiKey for OpenAI
Provider-->>AIService: throw Error(OpenAI API key is missing)
else Missing apiKey for HuggingFace
Provider-->>AIService: throw Error(Hugging Face API key is missing)
else Valid apiKey
Provider-->>AIService: providerResponse
end
deactivate Provider
alt Error thrown due to missing apiKey
AIService-->>AIService: catch Error
AIService->>Fallback: callAIProvider(prompt, type, fallbackConfig)
activate Fallback
Fallback-->>AIService: fallbackResponse
deactivate Fallback
AIService-->>Client: fallbackResponse
else No error
AIService-->>Client: providerResponse
end
deactivate AIService
Updated class diagram for AIService provider configuration and key validationclassDiagram
class AIServiceConfig {
<<interface>>
+string provider
+string apiKey
+string modelEndpoint
+string localModel
}
class AIService {
-AIServiceConfig primaryConfig
-AIServiceConfig[] fallbackConfigs
+AIService(primaryConfig AIServiceConfig, fallbackConfigs AIServiceConfig[])
+initialize() Promise~void~
+callAIProvider(prompt string, type string, config AIServiceConfig) Promise~string~
-callOpenAI(prompt string, type string, apiKey string) Promise~string~
-callHuggingFace(prompt string, apiKey string, modelEndpoint string) Promise~string~
-callOllama(prompt string, localModel string) Promise~string~
}
AIService o-- AIServiceConfig
class ExampleInitialization {
+createAIService() AIService
}
ExampleInitialization ..> AIService : uses
class Environment {
+string VITE_OPENAI_API_KEY
}
ExampleInitialization ..> Environment : reads
note for AIServiceConfig "For providers openai and huggingface, apiKey must be present; missing keys cause callAIProvider to throw an Error, triggering fallback logic in AIService.initialize."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughAdded comprehensive unit tests for AIService covering initialization failures and error handling when API keys are missing. Updated AIService implementation to validate API keys at runtime for OpenAI and Hugging Face providers before use, and removed the default demo-key fallback for OpenAI. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since you're now explicitly validating
apiKeyfor OpenAI and Hugging Face, consider adding similar runtime checks (with clear error messages) for other required fields likeconfig.modelEndpointandconfig.localModelinstead of relying on non-null assertions. - The new
Errormessages are quite generic; you might make them more actionable (e.g., hinting at which env var is missing) to simplify debugging misconfigurations in different environments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since you're now explicitly validating `apiKey` for OpenAI and Hugging Face, consider adding similar runtime checks (with clear error messages) for other required fields like `config.modelEndpoint` and `config.localModel` instead of relying on non-null assertions.
- The new `Error` messages are quite generic; you might make them more actionable (e.g., hinting at which env var is missing) to simplify debugging misconfigurations in different environments.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/aiService.test.ts`:
- Around line 49-50: Replace the two TypeScript suppression comments in the
aiService tests: change the `// `@ts-ignore`` directives used before the private
method access calls to `// `@ts-expect-error`` (the tests that call the private
method via `service.callAIProvider('test', 'test')` and the other suppression
around line 59); this will satisfy the `@typescript-eslint/ban-ts-comment` rule
and ensure the suppression fails if the underlying type error is resolved.
In `@src/services/aiService.ts`:
- Around line 196-200: The Hugging Face branch in AIService uses
config.modelEndpoint! without validating it; update the 'huggingface' case in
the method that dispatches to callHuggingFace to check that config.modelEndpoint
is present (non-empty string) and throw a clear Error like "Hugging Face
modelEndpoint is missing" if not, then call this.callHuggingFace(prompt,
config.apiKey, config.modelEndpoint) without the non-null assertion; reference
the 'huggingface' case, 'config.modelEndpoint', and 'callHuggingFace' when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 376b080d-665f-4411-8a44-30756867a71f
📒 Files selected for processing (2)
src/services/aiService.test.tssrc/services/aiService.ts
| // @ts-ignore - accessing private method for testing | ||
| await expect(service.callAIProvider('test', 'test')).rejects.toThrow('OpenAI API key is missing'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the test file exists and read the relevant lines
if [ -f "src/services/aiService.test.ts" ]; then
echo "=== File found, reading context around lines 49-50 and 59-60 ==="
sed -n '45,65p' "src/services/aiService.test.ts"
else
echo "File not found. Searching for it..."
find . -name "aiService.test.ts" -type f
fiRepository: ereezyy/Sol_Horse
Length of output: 840
🏁 Script executed:
# Check for ESLint configuration files
echo "=== Checking for ESLint config ==="
ls -la | grep -E "eslint|\.eslintrc|eslint\.config"
if [ -f ".eslintrc.json" ] || [ -f ".eslintrc.js" ] || [ -f ".eslintrc.yml" ]; then
echo "=== ESLint configuration found ==="
cat .eslintrc.* 2>/dev/null || echo "Config not readable"
fi
# Also check package.json for ESLint config
if [ -f "package.json" ]; then
echo "=== Checking package.json for eslintConfig ==="
jq '.eslintConfig // empty' package.json
fiRepository: ereezyy/Sol_Horse
Length of output: 210
🏁 Script executed:
# Check for TypeScript config and ESLint rules
echo "=== TypeScript configuration ==="
if [ -f "tsconfig.json" ]; then
head -50 tsconfig.json
fiRepository: ereezyy/Sol_Horse
Length of output: 212
🏁 Script executed:
# Read the ESLint configuration
echo "=== ESLint Configuration ==="
cat eslint.config.jsRepository: ereezyy/Sol_Horse
Length of output: 828
🏁 Script executed:
# Also check the tsconfig.app.json which is likely the main one
echo "=== TypeScript App Configuration ==="
cat tsconfig.app.jsonRepository: ereezyy/Sol_Horse
Length of output: 649
Replace @ts-ignore with @ts-expect-error on lines 49 and 59.
The ESLint @typescript-eslint/ban-ts-comment rule (enforced via tseslint.configs.recommended) allows only @ts-expect-error directives. @ts-ignore violates this rule and can silently become dead suppression if the underlying type error is fixed. @ts-expect-error is the correct directive for intentional type suppressions.
Proposed fix
- // `@ts-ignore` - accessing private method for testing
+ // `@ts-expect-error` - accessing private method for testing
await expect(service.callAIProvider('test', 'test')).rejects.toThrow('OpenAI API key is missing');
...
- // `@ts-ignore` - accessing private method for testing
+ // `@ts-expect-error` - accessing private method for testing
await expect(service.callAIProvider('test', 'test')).rejects.toThrow('Hugging Face API key is missing');📝 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.
| // @ts-ignore - accessing private method for testing | |
| await expect(service.callAIProvider('test', 'test')).rejects.toThrow('OpenAI API key is missing'); | |
| // `@ts-expect-error` - accessing private method for testing | |
| await expect(service.callAIProvider('test', 'test')).rejects.toThrow('OpenAI API key is missing'); |
🧰 Tools
🪛 ESLint
[error] 49-49: Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free.
(@typescript-eslint/ban-ts-comment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/aiService.test.ts` around lines 49 - 50, Replace the two
TypeScript suppression comments in the aiService tests: change the `//
`@ts-ignore`` directives used before the private method access calls to `//
`@ts-expect-error`` (the tests that call the private method via
`service.callAIProvider('test', 'test')` and the other suppression around line
59); this will satisfy the `@typescript-eslint/ban-ts-comment` rule and ensure
the suppression fails if the underlying type error is resolved.
| case 'huggingface': | ||
| return await this.callHuggingFace(prompt, config.apiKey!, config.modelEndpoint!); | ||
| if (!config.apiKey) { | ||
| throw new Error('Hugging Face API key is missing'); | ||
| } | ||
| return await this.callHuggingFace(prompt, config.apiKey, config.modelEndpoint!); |
There was a problem hiding this comment.
Validate modelEndpoint before invoking Hugging Face.
Line 200 still relies on config.modelEndpoint!. Because modelEndpoint is optional in AIServiceConfig, a Hugging Face config with an API key but no endpoint still makes it past the new guard and fails later as a generic fetch/runtime error instead of the explicit config error this PR is trying to enforce.
Proposed fix
case 'huggingface':
if (!config.apiKey) {
throw new Error('Hugging Face API key is missing');
}
- return await this.callHuggingFace(prompt, config.apiKey, config.modelEndpoint!);
+ if (!config.modelEndpoint) {
+ throw new Error('Hugging Face model endpoint is missing');
+ }
+ return await this.callHuggingFace(prompt, config.apiKey, config.modelEndpoint);📝 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.
| case 'huggingface': | |
| return await this.callHuggingFace(prompt, config.apiKey!, config.modelEndpoint!); | |
| if (!config.apiKey) { | |
| throw new Error('Hugging Face API key is missing'); | |
| } | |
| return await this.callHuggingFace(prompt, config.apiKey, config.modelEndpoint!); | |
| case 'huggingface': | |
| if (!config.apiKey) { | |
| throw new Error('Hugging Face API key is missing'); | |
| } | |
| if (!config.modelEndpoint) { | |
| throw new Error('Hugging Face model endpoint is missing'); | |
| } | |
| return await this.callHuggingFace(prompt, config.apiKey, config.modelEndpoint); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/aiService.ts` around lines 196 - 200, The Hugging Face branch in
AIService uses config.modelEndpoint! without validating it; update the
'huggingface' case in the method that dispatches to callHuggingFace to check
that config.modelEndpoint is present (non-empty string) and throw a clear Error
like "Hugging Face modelEndpoint is missing" if not, then call
this.callHuggingFace(prompt, config.apiKey, config.modelEndpoint) without the
non-null assertion; reference the 'huggingface' case, 'config.modelEndpoint',
and 'callHuggingFace' when making the change.
1 participant
🎯 What: The hardcoded 'demo-key' fallback string in
src/services/aiService.tswas being used whenVITE_OPENAI_API_KEYwas not provided.🛡️ Solution:
callAIProviderto throw anErrorifapiKeyis missing for OpenAI or Hugging Face.AIService.initialize().src/services/aiService.test.tsto ensure that initialization correctly fails or falls back when keys are missing, and that validation logic works as expected.PR created automatically by Jules for task 10333503756924995807 started by @ereezyy
Summary by Sourcery
Enforce explicit API key validation for AI providers and remove the insecure hardcoded OpenAI API key fallback.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests