Z.ai sdk integration (GLM-4.7, GLM-4.6v, GLM-4.6, GLM-4.5-air) #289
Conversation
Add detailed architecture.md covering: - Project overview and structure - Frontend/backend architecture layers - State management with Zustand - File-based data storage model - Automation system and feature lifecycle - AI provider extensibility - Configuration and settings - Testing infrastructure - Build and deployment pipeline 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Implements full support for Z.ai's GLM models as an alternative AI provider. ## Backend Changes - Add ZaiProvider class with BaseProvider interface implementation - GLM-4.7, GLM-4.6v, GLM-4.6, GLM-4.5-Air model support - Tool calling (Read, Write, Edit, Glob, Grep, Bash) - Vision support via GLM-4.6v with fallback for non-vision models - Extended thinking mode with reasoning_content streaming - Add provider-agnostic executeProviderQuery() layer - Update ProviderFactory for GLM model prefix routing - Convert all AI routes to use provider-agnostic queries - Add GET /api/models/providers and /api/models/available endpoints ## Frontend Changes - Integrate Zai models into model selector with badges - Add Zai API key management with live validation - Add provider auto-detection in profiles - Add reasoning content display with collapsible sections - Update type system with ModelProvider type and ZAI_MODEL_MAP ## Library Changes - Model resolver: provider-aware resolution with providerHint - Types: ZAI_MODEL_MAP, ModelProvider, updated Credentials ## Tests - Add comprehensive zai-provider.test.ts (560 lines) - Update provider-factory.test.ts for Zai routing ## Documentation - Add comprehensive Zai integration section to architecture.md - Add CHANGELOG.md documenting all changes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
This commit enhances the multi-provider system with improved UX for provider management and automatic model equivalence routing when a provider is disabled. **Backend Changes:** - Add resolveModelWithProviderAvailability() to model-resolver - Add MODEL_EQUIVALENCE map for cross-provider model fallback - Add getProviderForModel() helper for provider detection - Add enabledProviders option to executeProviderQuery() - Add enabledProviders to GlobalSettings type with defaults **Frontend Changes:** - Add ProviderCard component with toggle switch for enable/disable - Add ProviderSelectionStep for setup wizard provider choice - Add ZaiSetupStep for Zai API key configuration - Update setup wizard flow with provider selection - Add enabledProviders state to app-store and setup-store - Auto-enable providers when valid API key is saved **Documentation:** - Update architecture.md with onboarding wizard flow - Add model equivalence routing documentation - Update CHANGELOG.md with new features 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Improvements to the Z.ai authentication verification endpoint: - Add JSDoc documenting success/authenticated response convention - Import proper ProviderMessage and ContentBlock types (remove 'any' casts) - Extract detectZaiError() helper for DRY error pattern matching - Simplify stream loop with proper type guards - Remove redundant authenticated variable assignments - Consolidate error detection in catch block using detectZaiError() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
… streaming - Refactor ContentBlock to use discriminated unions for better type safety - Add Windows command support (dir, type, where, xcopy, robocopy, etc.) - Fix tool call completion detection to verify all data received - Improve glob pattern validation for Windows drive letters - Add Zai API key management (store, delete, verify endpoints) - Add tests for write_file, edit_file, glob_search, grep_search tools - Remove auto-advance on provider selection for better UX - Add Zai API key format validation (20+ chars) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
This commit addresses 16 issues identified during the Zai integration audit: - Server-side fixes: * Add SSE line buffering to prevent parse failures when chunks split mid-line * Add safety check for incomplete tool calls at end of stream * Add reasoning content block handling (extended_thinking -> thinking) * Add path validation to describeImages for security * Refactor available models route to use ProviderFactory * Create PROVIDER_REGISTRY for dynamic provider configuration * Add delete API key endpoint for Zai provider * Load Zai credentials in AgentService for tool use * Clarify feature naming: extendedThinking (Claude) vs thinking (Zai) - UI fixes: * Add verifyZaiAuth to SetupAPI interface * Add AlertCircle icon to Unverified badge * Add delete API key functionality to Zai setup step * Add visual feedback for API key operations - Test improvements: * Implement 3 previously skipped integration tests in provider-query * Add 5 new image/vision handling tests in zai-provider * Add proper mocks for secureFs, validatePath, and ProviderFactory - Documentation: * Clarify asymmetry in MODEL_EQUIVALENCE mapping regarding vision 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…Query - Replace Claude SDK query() calls with executeProviderQuery in describe-image, enhance-prompt, generate-title, and validate-issue routes - Add SettingsService injection to routes for API key management - Add getApiKeys() convenience method to SettingsService - Support multimodal array prompts in provider-query to enable image inputs - Add systemPrompt option to executeProviderQuery - Update Claude Sonnet equivalence to use GLM-4.6v (preserves vision capability) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
This commit addresses 13 issues identified in the ZAI SDK integration audit: **Server-side fixes:** - Fix API key logic bug in provider-query.ts (line 194) - Add enabledProviders checks to agent-service.ts and auto-mode-service.ts - Fix API key source priority in verify-zai-auth.ts (env before cache) - Fix dual provider instantiation in provider-query.ts - Add stream error handling to 4 routes (describe-file, describe-image, enhance, generate-title) - Fix ZaiProvider constructor type and remove dual API key field **UI-side fixes:** - Add ZAI delete button to settings UI (api-keys-section) - Add provider filtering to model-selector based on enabledProviders **Documentation updates:** - Update CHANGELOG: streaming support is now implemented for Zai - Add ZAI authentication setup to README - Update provider system description in README 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
This commit completes the remaining 3 tasks from the ZAI integration audit: **1. Consolidate type definitions:** - Merge complete types into libs/types/src/provider.ts as source of truth - Update apps/server/src/providers/types.ts to re-export from @automaker/types - Add ThinkingConfig, discriminated ContentBlock unions, and detailed ExecuteOptions **2. Standardize provider naming:** - Add PROVIDER_TO_API_KEY_NAME mapping constant (claude -> anthropic) - Update ModelDefinition.provider from 'anthropic' to 'claude' for consistency - Update provider-factory test to expect 'claude' instead of 'anthropic' **3. Fix ZAI setup step save/verify flow:** - Change order: verify first, then save only if verification succeeds - Prevents saving invalid API keys that fail authentication 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Update GLM model specifications with correct values: - GLM-4.7: contextWindow=200000, maxOutputTokens=128000 - GLM-4.6v: maxOutputTokens=96000 - GLM-4.6: contextWindow=200000, maxOutputTokens=128000 - GLM-4.5-Air: maxOutputTokens=96000 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…n disabled - Add autoSwitchModelIfDisabled() helper in model-utils.ts for automatic model switching - Filter model selectors to only show models from enabled providers - Auto-switch to equivalent model when provider is disabled (same badge tier) - Update agent view, settings, profile form, and profile quick select components - Fallback gracefully when no equivalent model exists Changes: - Agent view: Filter dropdown and auto-switch model on provider toggle - Settings (AI Enhancement): Filter grid and auto-switch on provider toggle - Settings (Feature Defaults): Conditional model rendering by provider - Profile form: Filter model selection and auto-switch on provider toggle - Profile quick select: Filter profiles by enabled provider
Change quote validation from blocking all quotes to requiring balanced quotes.
This allows legitimate use cases like JavaScript code snippets while still
preventing injection attacks via unclosed strings.
Before: " and ' were completely blocked
After: " and ' are allowed but must be balanced (even count)
Example now allowed:
node -e "import('./server/index').then(m => console.log('success'))"
Still blocked:
; ` $ (command separators and substitution)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Phase 1 - Critical Fixes: - Fix type import error: ModelProvider from @automaker/types - Fix test syntax: add async to beforeEach in provider-query.test.ts - Fix test assertion: update warning message in provider-factory.test.ts - Add provider name compatibility: normalize 'anthropic' -> 'claude' Phase 2 - Security & Compatibility: - Add rate limiting: 5 attempts per 15min on verify-zai-auth - Fix AbortSignal.any(): add fallback for Node.js < 20 Phase 3 - Functional Improvements: - Remove hard-coded models: use provider-aware resolveModelString() - Add Zai docs: comprehensive provider documentation - Fix feature naming: unify 'thinking' with backward compatibility Phase 4 - Robustness & Reliability: - Add retry logic: 3 attempts with exponential backoff - Fix memory leaks: add _cleanup to timeout controllers - Install express-rate-limit for rate limiting support
- Allow Docker commands by default (removed ZAI_ALLOW_DOCKER check) - Remove delete Z.ai key button from settings UI (user can delete in textbox) - Persist provider toggle state to backend settings file - Load enabledProviders from backend on app startup - Add localStorage fallback for web mode 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughIntroduces a comprehensive multi-provider system supporting Claude (Anthropic) and Z.ai (GLM) providers. Adds provider factory registration, provider-agnostic query abstraction, Z.ai provider implementation with tool and vision support, provider selection in setup flow, model equivalence mappings, and UI for managing enabled providers and API keys across the frontend and backend with type system updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Frontend<br/>(Agent/Settings)
participant Store as App Store<br/>(enabledProviders)
participant API as Backend API
participant Factory as ProviderFactory
participant Resolver as ModelResolver
participant Provider as Provider<br/>(Claude/Zai)
User->>UI: Select model or provider
UI->>Store: toggleProvider() or setEnabledProviders()
alt Electron Mode
Store->>API: POST /api/settings/global/providers
API->>API: Update credentials & settings
API-->>Store: Success
end
Note over Store: Store syncs enabledProviders<br/>locally & to backend
User->>UI: Execute query (e.g., agent message)
UI->>Resolver: getFilteredModels(enabledProviders)
Resolver-->>UI: Filter models by enabled providers
UI->>UI: Render filtered model options
User->>UI: Send message with selected model
UI->>API: POST /api/agent/execute<br/>(model, prompt, apiKeys)
API->>Resolver: resolveModelWithProviderAvailability()<br/>(model, enabledProviders)
alt Provider enabled
Resolver-->>API: Resolved model
else Provider disabled
Resolver-->>API: Fallback via MODEL_EQUIVALENCE
end
API->>API: loadCredentials from SettingsService
API->>Factory: getProviderForModel(resolvedModel)
Factory-->>API: Provider instance (Claude or Zai)
API->>Provider: executeQuery(options)<br/>+ apiKey credential
Provider->>Provider: Stream responses<br/>(text, thinking, tool_calls)
Provider-->>API: AsyncGenerator<ProviderMessage>
loop Stream processing
API->>API: Accumulate content<br/>(text, thinking blocks)
API-->>UI: Emit stream updates
UI->>UI: Update message<br/>(content, thinking)
end
API-->>UI: Final result + thinking
UI->>UI: Render with thinking block<br/>(if showReasoningByDefault)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
Summary of ChangesHello @ththeod, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the application's AI capabilities by integrating Z.ai's GLM models as an alternative to Anthropic's Claude. It introduces a flexible multi-provider system that allows users to select and manage different AI backends, enhancing the user experience through a new setup flow and refined settings. Core changes include a provider-agnostic query layer, robust security improvements for tool execution, and comprehensive updates across both backend services and frontend components to support this new extensibility. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
ththeod
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Z.ai (GLM) provider integration, adding support for GLM models with tool calling, vision, extended thinking, and structured output, alongside significant multi-provider UX enhancements. Key changes include a new provider-agnostic query layer (executeProviderQuery) that unifies AI model interactions, a refactored ProviderFactory using a registry pattern for extensibility, and redesigned API Keys settings with provider cards for managing and enabling/disabling providers. The setup wizard now includes a ProviderSelectionStep and a ZaiSetupStep for configuring Z.ai API keys. Backend routes and services are updated to leverage the new provider system, and model resolution now accounts for provider availability and model equivalence for intelligent fallbacks. Frontend components are updated to display Z.ai models and manage provider states, including a new autoSwitchModelIfDisabled utility. Documentation and changelog entries reflect these additions, including security hardening for shell commands in the Z.ai provider. Review comments highlight a critical command injection vulnerability in sanitizeCommand due to incomplete sanitization of chained commands, suggesting either disallowing chaining or sanitizing each part. Another comment points out missing common file-reading commands (less, more) in the PATH_COMMANDS set, which could lead to sensitive data exfiltration. Finally, the logic for determining API keys based on model names is identified as duplicated and should be centralized for better maintainability.
| // Extract shell pipes (|) before parsing arguments | ||
| // Pipes chain commands together: cmd1 | cmd2 | ||
| const pipes: string[] = []; | ||
| let workCommand = trimmed; | ||
|
|
||
| // Handle pipe separators first (before ampersands) | ||
| const pipeParts = workCommand.split(/\|/); | ||
| if (pipeParts.length > 1) { | ||
| pipes.push(...pipeParts.slice(1).map((p) => p.trim())); | ||
| workCommand = pipeParts[0].trim(); | ||
| } | ||
|
|
||
| // Extract Windows-style ampersand separators (&, &&) | ||
| // Windows: cmd1 & cmd2 runs async, cmd1 && cmd2 runs sequentially | ||
| // We'll treat them like pipes but they go to the pipes array | ||
| const ampersandParts = workCommand.split(/&&?/); | ||
| if (ampersandParts.length > 1) { | ||
| pipes.push(...ampersandParts.slice(1).map((p) => p.trim())); | ||
| workCommand = ampersandParts[0].trim(); | ||
| } |
There was a problem hiding this comment.
There is a critical command injection vulnerability in the sanitizeCommand function. The function splits the command by pipe (|) and ampersand (&, &&) characters but only sanitizes the first part of the command. The subsequent parts are appended back to the command string without any sanitization, allowing an attacker to bypass all security checks. For example, a command like echo "safe" | rm -rf / would be executed, as only echo "safe" is sanitized.
A safer approach would be to not support command chaining via pipes or ampersands in the execute_command tool at all, as it significantly increases the attack surface. If chaining is required, each command must be individually sanitized.
| const PATH_COMMANDS = new Set<string>([ | ||
| 'rm', | ||
| 'del', | ||
| 'erase', | ||
| 'mv', | ||
| 'move', | ||
| 'cp', | ||
| 'copy', | ||
| 'xcopy', | ||
| 'mkdir', | ||
| 'md', | ||
| 'touch', | ||
| 'ln', | ||
| 'mklink', | ||
| 'cat', | ||
| 'type', | ||
| 'head', | ||
| 'tail', | ||
| 'ls', | ||
| 'dir', | ||
| 'find', | ||
| 'findstr', | ||
| 'grep', | ||
| 'sed', | ||
| 'git', | ||
| 'npm', | ||
| 'npx', | ||
| 'pnpm', | ||
| 'yarn', | ||
| 'bun', | ||
| 'node', | ||
| 'python', | ||
| 'python3', | ||
| 'tsc', | ||
| 'vitest', | ||
| 'jest', | ||
| 'pytest', | ||
| 'mocha', | ||
| ]); |
There was a problem hiding this comment.
The PATH_COMMANDS set is a good security measure, but it's missing some common commands that can read files, such as less and more. While these are read-only, they could still be used to exfiltrate sensitive file content if the LLM is manipulated to use them with an absolute path that bypasses other checks.
const PATH_COMMANDS = new Set<string>([
'rm',
'del',
'erase',
'mv',
'move',
'cp',
'copy',
'xcopy',
'mkdir',
'md',
'touch',
'ln',
'mklink',
'cat',
'type',
'head',
'tail',
'less',
'more',
'ls',
'dir',
'find',
'findstr',
'grep',
'sed',
'git',
'npm',
'npx',
'pnpm',
'yarn',
'bun',
'node',
'python',
'python3',
'tsc',
'vitest',
'jest',
'pytest',
'mocha',
]);| // Load credentials from SettingsService for API key | ||
| let apiKey: string | undefined; | ||
| if (this.settingsService) { | ||
| try { | ||
| const credentials = await this.settingsService.getCredentials(); | ||
| const lowerModel = effectiveModel.toLowerCase(); | ||
|
|
||
| // Determine the appropriate API key for this model | ||
| if (lowerModel.startsWith('glm-') || lowerModel === 'glm') { | ||
| apiKey = credentials.apiKeys?.zai; | ||
| } else if ( | ||
| lowerModel.startsWith('claude-') || | ||
| ['haiku', 'sonnet', 'opus'].includes(lowerModel) | ||
| ) { | ||
| apiKey = credentials.apiKeys?.anthropic; | ||
| } | ||
|
|
||
| if (apiKey) { | ||
| console.log(`[AgentService] Loaded API key from settings for model: ${effectiveModel}`); | ||
| } | ||
| } catch (error) { | ||
| console.warn(`[AgentService] Failed to load credentials:`, error); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for determining which API key to use based on the model name is duplicated in agent-service.ts and auto-mode-service.ts. This can lead to inconsistencies if new providers are added or model naming conventions change. To improve maintainability, this logic should be centralized.
You can create a helper function or extend the provider factory to handle this. For example, you could use the existing getProviderForModel function from @automaker/model-resolver and a map to get the correct API key name.
ththeod
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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/ui/src/components/views/settings-view/api-keys/hooks/use-api-key-management.ts (1)
55-75: Inconsistent useEffect dependency triggers.The effect on line 56-75 checks status for all providers (Anthropic, Google, Zai) but only re-runs when
apiKeys.zaichanges (line 75). This means changes toapiKeys.anthropicorapiKeys.googlewon't trigger a status refresh.🔎 Proposed fix
}, [apiKeys.zai]); + // Consider: [apiKeys.anthropic, apiKeys.google, apiKeys.zai] or just [apiKeys]If the intent is to only refresh on Zai key changes, consider adding a comment explaining this design choice. Otherwise, expand the dependency array:
- }, [apiKeys.zai]); + }, [apiKeys]);apps/ui/src/lib/electron.ts (1)
1097-1101: Type inconsistency:SetupAPI.getApiKeysmissinghasZaiKey.The
ElectronAPI.setup.getApiKeysreturn type includeshasZaiKey(line 530), but theSetupAPIinterface on lines 1097-1101 does not. This inconsistency could cause type errors when these types are used interchangeably.🔎 Proposed fix
getApiKeys: () => Promise<{ success: boolean; hasAnthropicKey: boolean; hasGoogleKey: boolean; + hasZaiKey: boolean; }>;apps/server/src/routes/github/routes/validate-issue.ts (2)
149-154: Update stale "Claude SDK" reference in error message.Line 151 references "Claude SDK" but the code now uses the provider-agnostic
executeProviderQuery. This should be updated for consistency.🔎 Proposed fix
if (!validationResult) { - logger.error('No structured output received from Claude SDK'); + logger.error('No structured output received from provider'); logger.debug('Raw response text:', responseText); throw new Error('Validation failed: no structured output received'); }
196-204: Update stale JSDoc referencing Claude SDK.The JSDoc still describes Claude SDK usage (Read, Glob, Grep tools, JSON schema, etc.) but the implementation now uses the provider-agnostic
executeProviderQuery. Consider updating the documentation to reflect the current architecture.🔎 Proposed fix
/** * Creates the handler for validating GitHub issues against the codebase. * - * Uses Claude SDK with: - * - Read-only tools (Read, Glob, Grep) for codebase analysis + * Uses provider-agnostic query system with: + * - AI provider for codebase analysis * - JSON schema structured output for reliable parsing * - System prompt guiding the validation process * - Async execution with event emission */apps/ui/src/lib/http-api-client.ts (1)
955-976: Type mismatch ingetCredentialsresponse.The
getCredentialsresponse type (lines 955-963) doesn't includezai, butupdateCredentials(lines 965-976) expects and returnszaiin credentials. This inconsistency could cause type errors.🔎 Suggested fix
getCredentials: (): Promise<{ success: boolean; credentials?: { anthropic: { configured: boolean; masked: string }; google: { configured: boolean; masked: string }; openai: { configured: boolean; masked: string }; + zai: { configured: boolean; masked: string }; }; error?: string; }> => this.get('/api/settings/credentials'),
🧹 Nitpick comments (32)
apps/ui/src/lib/utils.ts (1)
19-23: Simplify redundant model matching logic.The
zaiModels.includes(modelStr)check on line 21 is redundant because line 22'szaiModels.some((m) => modelStr.includes(m))will also match exact strings (e.g., whenmodelStr === 'glm-4.6', the some predicate returns true for'glm-4.6'.includes('glm-4.6')).🔎 Proposed simplification
return ( claudeModels.some((m) => modelStr.includes(m)) || - zaiModels.includes(modelStr) || zaiModels.some((m) => modelStr.includes(m)) );CHANGELOG.md (1)
239-239: Consider converting the bare URL to a Markdown link.The URL on line 239 is unformatted. For consistency and better readability, wrap it in a proper link:
-1. Open Settings → API Keys -2. Enter your Z.ai API key (get from https://open.bigmodel.cn/) +1. Open Settings → API Keys +2. Enter your Z.ai API key (get from [open.bigmodel.cn](https://open.bigmodel.cn/))apps/server/tests/unit/lib/provider-query.test.ts (1)
282-296: Verify assertion completeness for provider routing.The test verifies that results are received and have a
typeproperty, but doesn't explicitly verify which provider was used. Consider adding an assertion to confirm the mock provider'sgetName()was called or that the correct provider instance was used.🔎 Suggested enhancement
it('should route to Zai provider for glm models', async () => { + const { ProviderFactory } = await import('../../../../src/providers/provider-factory.js'); const results: unknown[] = []; for await (const result of executeProviderQuery({ prompt: 'Test prompt', model: 'glm-4.7', cwd: '/test', apiKeys: { zai: 'test-key', anthropic: 'test-key' }, })) { results.push(result); } // Should receive messages from the provider expect(results.length).toBeGreaterThan(0); expect(results[0]).toHaveProperty('type'); + // Verify correct provider was selected + expect(vi.mocked(ProviderFactory.getProviderForModel)).toHaveBeenCalledWith( + expect.stringContaining('glm'), + expect.any(Object) + ); });apps/ui/src/components/views/settings-view/api-keys/hooks/use-api-key-management.ts (1)
197-199: Consider surfacing save errors to the user.The error is logged to console but not shown to the user. Users may not realize their API keys weren't saved. Consider showing a toast notification on failure.
🔎 Proposed enhancement
} catch (error) { console.error('Failed to save API keys:', error); + // Consider adding a toast notification + // toast.error('Failed to save API keys. Please try again.'); }apps/ui/src/components/views/setup-view/steps/zai-setup-step.tsx (2)
33-33: Initialize apiKey from store to support resuming setup.The
apiKeystate starts empty, but if the user has already saved a Zai key (e.g., from a previous partial setup), they would need to re-enter it. Consider initializing fromapiKeys.zai:🔎 Proposed fix
- const [apiKey, setApiKey] = useState(''); + const [apiKey, setApiKey] = useState(apiKeys.zai || '');
82-159: Consider extracting shared verification logic.The
verifyApiKey(lines 82-115) andhandleSaveAndVerify(lines 118-159) functions share similar validation and verification logic. While the current separation is functional, extracting the common verification into a helper could reduce duplication.🔎 Potential refactor approach
// Extract common verification const performVerification = async (): Promise<boolean> => { if (!apiKey || !isValidApiKey(apiKey)) { setVerificationStatus('error'); setVerificationError('Please enter a valid API key (at least 20 characters).'); return false; } setVerificationStatus('verifying'); setVerificationError(null); try { const api = getElectronAPI(); if (!api.setup?.verifyZaiAuth) { setVerificationStatus('error'); setVerificationError('Verification API not available'); return false; } const result = await api.setup.verifyZaiAuth(apiKey); if (result.success && result.authenticated) { return true; } else { setVerificationStatus('error'); setVerificationError(result.error || 'Authentication failed'); return false; } } catch (error) { setVerificationStatus('error'); setVerificationError(error instanceof Error ? error.message : 'Verification failed'); return false; } };libs/types/src/provider.ts (1)
244-256: Consider includingidandis_errorin LegacyContentBlock.The
LegacyContentBlockis missing theidfield (fromToolUseBlock, line 165) andis_errorfield (fromToolResultBlock, line 181) that exist in the new union types. If existing code relies on these fields, they should be included for full compatibility.🔎 Proposed enhancement
export interface LegacyContentBlock { type: 'text' | 'tool_use' | 'thinking' | 'tool_result'; text?: string; thinking?: string; + id?: string; name?: string; input?: unknown; tool_use_id?: string; content?: string; + is_error?: boolean; }apps/server/src/routes/setup/routes/api-keys.ts (1)
14-15: Verify the hasGoogleKey placeholder.The
hasGoogleKey: falseappears to be a placeholder for future Google provider support. Ensure this is intentional and documented, or consider removing it until Google integration is actually implemented to avoid confusion.apps/server/src/providers/base-provider.ts (1)
104-107: Consider type safety in normalizeFeatureName.The method casts any string to
ProviderFeatureon line 106, which bypasses type checking. If an invalid feature name is passed, it will silently return an invalid value. Consider validating against known feature names or using a more explicit type guard.🔎 Proposed type-safe implementation
public static normalizeFeatureName(feature: ProviderFeature | string): ProviderFeature { if (feature === 'extendedThinking') return 'thinking'; - return feature as ProviderFeature; + // Validate feature is actually a known ProviderFeature + const validFeatures: ProviderFeature[] = ['tools', 'text', 'vision', 'mcp', 'browser', 'thinking', 'structuredOutput']; + if (validFeatures.includes(feature as ProviderFeature)) { + return feature as ProviderFeature; + } + // Return as-is for unknown features (or throw error if strict validation needed) + return feature as ProviderFeature; }Alternatively, if strict validation is preferred, throw an error for unknown features.
apps/ui/src/components/views/profiles-view/utils.ts (1)
5-13: Consider refactoring the multi-condition check.The current chain of OR conditions is correct but verbose. For better maintainability, consider using a Set or array for GLM model matching:
🔎 Proposed refactor
+const ZAI_MODELS = new Set(['glm', 'glm-4.6v', 'glm-4.6', 'glm-4.7', 'glm-4.5-air']); + export function getProviderFromModel(model: AgentModel): ModelProvider { - if ( - model === 'glm' || - model === 'glm-4.6v' || - model === 'glm-4.6' || - model === 'glm-4.7' || - model === 'glm-4.5-air' - ) { + if (ZAI_MODELS.has(model)) { return 'zai'; } return 'claude'; }apps/server/src/routes/setup/routes/delete-api-key.ts (1)
61-69: Consider dynamically generating the supported providers list.The error message on line 66 hard-codes "anthropic, zai" which could become stale if more providers are added to the registry. Consider deriving this from
PROVIDER_REGISTRYfor consistency with the store-api-key route and future extensibility.🔎 Proposed fix
-import { setApiKey, getProviderEnvKey } from '../common.js'; +import { setApiKey, getProviderEnvKey, getSupportedProviderNames } from '../common.js';Then update the error message:
- error: `Unknown provider: ${provider}. Supported providers: anthropic, zai.`, + error: `Unknown provider: ${provider}. Supported providers: ${getSupportedProviderNames().join(', ')}.`,This assumes adding a helper function to
common.js:export function getSupportedProviderNames(): string[] { return Object.keys(PROVIDER_REGISTRY); }apps/server/src/routes/setup/routes/store-api-key.ts (1)
6-13: Remove unused importisSupportedProvider.The function
isSupportedProvideris imported on line 12 but never used in this file. The provider validation is done viagetProviderEnvKeyreturning null instead.🔎 Proposed fix
import { setApiKey, persistApiKeyToEnv, getErrorMessage, logError, getProviderEnvKey, - isSupportedProvider, } from '../common.js';apps/server/src/routes/setup/common.ts (1)
19-22: Consider renaming to avoid type collision withproviders/types.ts.Based on the relevant code snippets,
apps/server/src/providers/types.tsalso exports aProviderConfigtype (line 9). Having two distinctProviderConfiginterfaces in the codebase can cause confusion and potential import conflicts when both are needed in the same file.🔎 Suggested rename
-interface ProviderConfig { +interface ProviderEnvConfig { envKey: string; aliases: string[]; } -export const PROVIDER_REGISTRY: Record<string, ProviderConfig> = { +export const PROVIDER_REGISTRY: Record<string, ProviderEnvConfig> = {apps/ui/src/components/views/board-view/shared/model-constants.ts (1)
4-10: Consider importing from@automaker/typesto eliminate duplication.The
ModelOptiontype andZAI_MODELSarray are already defined inlibs/types/src/model-display.ts(lines 12-23 and 67-96 per relevant snippets). Maintaining identical definitions in two places creates a maintenance burden where changes must be synced.🔎 Suggested refactor
-export type ModelOption = { - id: AgentModel; - label: string; - description: string; - badge?: string; - provider: 'claude' | 'zai'; -}; +import type { ModelOption } from '@automaker/types'; +export type { ModelOption }; -export const ZAI_MODELS: ModelOption[] = [ - // ... duplicated definitions -]; +export { ZAI_MODELS, CLAUDE_MODELS } from '@automaker/types';Also applies to: 36-65
apps/ui/src/components/views/setup-view/steps/provider-selection-step.tsx (1)
50-50:hoveredProviderstate is declared but only used for conditional styling.The
isHoveredvariable is computed on line 75 but never actually used in the card rendering (the hover effect is handled purely by CSS classes). Consider removing the unused state or applying it for enhanced hover feedback.🔎 Option 1: Remove unused state
export function ProviderSelectionStep({ onNext, onBack, onSkip }: ProviderSelectionStepProps) { const { selectedProvider, setSelectedProvider } = useSetupStore(); - const [hoveredProvider, setHoveredProvider] = useState<SelectedProvider | null>(null); // ... return ( <Card key={provider.id} // ... onClick={() => handleProviderSelect(provider.id)} - onMouseEnter={() => setHoveredProvider(provider.id)} - onMouseLeave={() => setHoveredProvider(null)} >Also applies to: 75-75
apps/ui/src/utils/model-utils.ts (1)
11-24: Consider importing from centralized model constants.These model definitions duplicate data from
apps/ui/src/components/views/board-view/shared/model-constants.ts. Changes to model badges or additions would need to be synced across multiple files.🔎 Suggested refactor
+import { CLAUDE_MODELS, ZAI_MODELS, ALL_MODELS } from '@/components/views/board-view/shared/model-constants'; + export function autoSwitchModelIfDisabled( currentModel: AgentModel | null | undefined, enabledProviders: { claude: boolean; zai: boolean } ): AgentModel { if (!currentModel) return 'sonnet'; - const CLAUDE_MODELS: Array<{ id: AgentModel; badge?: string }> = [ - { id: 'haiku', badge: 'Speed' }, - { id: 'sonnet', badge: 'Balanced' }, - { id: 'opus', badge: 'Premium' }, - ]; - - const ZAI_MODELS: Array<{ id: AgentModel; badge?: string }> = [ - { id: 'glm-4.7', badge: 'Premium' }, - { id: 'glm-4.6v', badge: 'Vision' }, - { id: 'glm-4.6', badge: 'Balanced' }, - { id: 'glm-4.5-air', badge: 'Speed' }, - ]; - - const ALL_MODELS_INFO = [...CLAUDE_MODELS, ...ZAI_MODELS]; + const ALL_MODELS_INFO = ALL_MODELS;apps/ui/src/components/views/setup-view.tsx (1)
45-54: Add fallback handling whenselectedProvideris null.If
handleNext('provider_selection')is called whenselectedProvideris null (which shouldn't happen due to the disabled Continue button, but could occur via programmatic navigation), the function silently does nothing. Consider adding a fallback or logging.🔎 Suggested defensive handling
case 'provider_selection': // Move to the appropriate setup step based on selected provider if (selectedProvider === 'claude') { console.log('[Setup Flow] Moving to claude_setup step'); setCurrentStep('claude_setup'); } else if (selectedProvider === 'zai') { console.log('[Setup Flow] Moving to zai_setup step'); setCurrentStep('zai_setup'); + } else { + console.warn('[Setup Flow] No provider selected, cannot proceed'); } break;apps/server/src/routes/suggestions/generate-suggestions.ts (1)
166-180: Consider passingenabledProvidersto the provider query.Based on the
executeProviderQueryimplementation inapps/server/src/lib/provider-query.ts(lines 161-172), the function supports anenabledProvidersoption for model substitution when a provider is disabled. This isn't being passed here, which means the suggestions flow won't respect disabled providers.🔎 Suggested fix
// Get API keys for provider authentication const apiKeys = settingsService ? await settingsService.getApiKeys() : undefined; + + // Get enabled providers for model resolution + let enabledProviders: { claude: boolean; zai: boolean } | undefined; + if (settingsService) { + try { + const globalSettings = await settingsService.getGlobalSettings(); + enabledProviders = globalSettings.enabledProviders; + } catch (error) { + logger.warn('[Suggestions] Failed to load enabled providers:', error); + } + } const stream = executeProviderQuery({ cwd: projectPath, prompt, useCase: 'suggestions', abortController, autoLoadClaudeMd, apiKeys, + enabledProviders, outputFormat: {apps/server/src/services/agent-service.ts (1)
251-274: API key selection logic could be consolidated.The model-to-provider mapping here (checking if model starts with 'glm-' or 'claude-') duplicates logic that exists in
@automaker/model-resolver. Consider usinggetProviderForModelfrom the model-resolver package for consistency, as done inprovider-query.ts.🔎 Suggested refactor
+ import { getProviderForModel } from '@automaker/model-resolver'; + // Load credentials from SettingsService for API key let apiKey: string | undefined; if (this.settingsService) { try { const credentials = await this.settingsService.getCredentials(); - const lowerModel = effectiveModel.toLowerCase(); - - // Determine the appropriate API key for this model - if (lowerModel.startsWith('glm-') || lowerModel === 'glm') { - apiKey = credentials.apiKeys?.zai; - } else if ( - lowerModel.startsWith('claude-') || - ['haiku', 'sonnet', 'opus'].includes(lowerModel) - ) { - apiKey = credentials.apiKeys?.anthropic; - } + const providerName = getProviderForModel(effectiveModel); + const apiKeyMap: Record<string, string | undefined> = { + claude: credentials.apiKeys?.anthropic, + zai: credentials.apiKeys?.zai, + }; + apiKey = providerName ? apiKeyMap[providerName] : undefined;apps/server/src/routes/app-spec/generate-spec.ts (1)
120-134: MissingenabledProvidersin provider query call.Similar to the suggestions route, this call to
executeProviderQuerydoesn't passenabledProviders, which means spec generation won't respect disabled providers.🔎 Suggested fix
+ // Get enabled providers for model resolution + let enabledProviders: { claude: boolean; zai: boolean } | undefined; + if (settingsService) { + try { + const globalSettings = await settingsService.getGlobalSettings(); + enabledProviders = globalSettings.enabledProviders; + } catch (error) { + logger.warn(`[SpecRegeneration] Failed to load enabled providers:`, error); + } + } + // Use provider-agnostic query const stream = executeProviderQuery({ cwd: projectPath, prompt, useCase: 'spec', maxTurns: 250, allowedTools: ['Read', 'Glob', 'Grep'], abortController, autoLoadClaudeMd, apiKeys, + enabledProviders, outputFormat: {apps/server/src/routes/app-spec/generate-features-from-spec.ts (2)
127-137: MissingenabledProvidersin provider query call.Consistent with the other refactored routes, this call should also pass
enabledProvidersto respect disabled providers during feature generation.🔎 Suggested fix
+ // Get enabled providers for model resolution + let enabledProviders: { claude: boolean; zai: boolean } | undefined; + if (settingsService) { + try { + const globalSettings = await settingsService.getGlobalSettings(); + enabledProviders = globalSettings.enabledProviders; + } catch (error) { + logger.warn(`[FeatureGeneration] Failed to load enabled providers:`, error); + } + } + // Use provider-agnostic query const stream = executeProviderQuery({ cwd: projectPath, prompt, useCase: 'features', maxTurns: 50, allowedTools: ['Read', 'Glob', 'Grep'], abortController, autoLoadClaudeMd, apiKeys, + enabledProviders, });
146-159: Redundant null check on content.Line 148 checks
if (!content) continue;butcontentis already guaranteed to be truthy from the condition on line 146-147:(msg as ProviderMessage).message?.contentand the subsequent assignment.🔎 Suggested fix
if (msg.type === 'assistant' && (msg as ProviderMessage).message?.content) { const content = (msg as ProviderMessage).message!.content!; - if (!content) continue; for (const block of content) {apps/server/src/routes/context/routes/describe-file.ts (1)
14-14: Unused import:DEFAULT_MODELS
DEFAULT_MODELSis imported but never used in this file. Consider removing it to avoid confusion.Proposed fix
-import { DEFAULT_MODELS } from '@automaker/types';apps/ui/src/config/api-providers.ts (1)
64-68:ProviderConfigParamsbut not used.The
ProviderConfigParams(lines 44-52) but is not destructured inbuildProviderConfigs. This creates a type contract that requires callers to provideOption 1: Remove google from interface
export interface ProviderConfigParams { apiKeys: ApiKeys; anthropic: { value: string; setValue: Dispatch<SetStateAction<string>>; show: boolean; setShow: Dispatch<SetStateAction<boolean>>; testing: boolean; onTest: () => Promise<void>; result: { success: boolean; message: string } | null; }; - google: { - value: string; - setValue: Dispatch<SetStateAction<string>>; - show: boolean; - setShow: Dispatch<SetStateAction<boolean>>; - testing: boolean; - onTest: () => Promise<void>; - result: { success: boolean; message: string } | null; - }; zai: { ... }; }Option 2: Make google optional
- google: { + google?: {apps/server/src/routes/enhance-prompt/routes/enhance.ts (1)
11-11: Unused import:DEFAULT_MODELSSame issue as in
describe-file.ts-DEFAULT_MODELSis imported but never used.Proposed fix
-import { DEFAULT_MODELS } from '@automaker/types';apps/server/src/routes/context/routes/describe-image.ts (1)
15-15: Unused import:DEFAULT_MODELSSame pattern as other files -
DEFAULT_MODELSis imported but not used in this file.Proposed fix
-import { DEFAULT_MODELS } from '@automaker/types';apps/server/src/services/auto-mode-service.ts (1)
612-615: Clarify comment vs. behavior for auto‑mode model selection
getModelForUseCase('auto')ignores any per‑featurefeature.model, but the comment still says “env var takes precedence over feature model.” Either passfeature.modelas the explicit model to preserve per‑feature overrides, or update the comment to reflect that auto‑mode now uses a single environment‑driven model for all features.apps/server/src/providers/provider-factory.ts (1)
86-107: Consider allowing config to flow intogetAllProviders/checkAllProviders
getAllProvidersalways constructs providers with noProviderConfig, andcheckAllProvidersrelies on that. If, in the future, installation detection or “available models” needs to respect API keys coming from SettingsService (rather than only env vars), you’ll have to thread config into these methods or add overloads that accept a config. Not urgent now, but worth keeping in mind to avoid duplicating provider‑construction logic later.apps/server/src/providers/zai-provider.ts (1)
893-898: Command execution uses shell with minimal environment - consider security hardening.The command execution passes
env: { PATH: process.env.PATH }which is good for isolation. However, the command is executed through the shell which could still be vulnerable to certain attacks. Consider usingshell: falsewith an explicit array of arguments for better security.🔎 Proposed improvement: Use spawn with shell: false where possible
- const { stdout, stderr } = await execAsync(cmdString, { - cwd: resolvedCwd, - maxBuffer: 1024 * 1024, // 1MB - env: { PATH: process.env.PATH }, // Minimal environment - timeout: 30000, // 30 second timeout - }); + const { stdout, stderr } = await execAsync(cmdString, { + cwd: resolvedCwd, + maxBuffer: 1024 * 1024, // 1MB + env: { PATH: process.env.PATH, HOME: '' }, // Minimal environment, block HOME + timeout: 30000, // 30 second timeout + shell: process.platform === 'win32' ? 'cmd.exe' : '/bin/sh', + });libs/model-resolver/src/resolver.ts (2)
72-82: Duplicate pass-through logic for GLM models.Lines 78-82 check for
glm-prefix and pass through, butZAI_MODEL_MAPat line 94 would also resolveglm-4.7toglm-4.7. The explicit pass-through is redundant but harmless. Consider consolidating to reduce code duplication.
253-261: Provider fallback order is hardcoded with Claude preference.When neither the original nor equivalent provider is available, the code falls back to Claude first (line 253-255) then Zai (line 258-260). This implicit preference should be documented or made configurable.
Consider documenting this behavior or making it configurable:
// No equivalent found or equivalent's provider is also disabled - // Find any enabled provider and use its default model + // Find any enabled provider and use its default model + // Priority: Claude > Zai (for backward compatibility) if (enabledProviders.claude) {apps/server/src/lib/provider-query.ts (1)
182-205: Dynamic import of getProviderForModel is duplicated.The
getProviderForModelis dynamically imported twice (lines 184 and 202) in different branches. This could be hoisted before the conditional.🔎 Proposed fix: Hoist the dynamic import
+ // Get provider name from model + const { getProviderForModel } = await import('@automaker/model-resolver'); + const providerName = getProviderForModel(resolvedModel); + logger.info(`[ProviderQuery] Using provider: ${providerName}`); + // Determine the provider and API key let apiKey: string | undefined; - let providerName: 'claude' | 'zai' | undefined; if (apiKeys) { - // Get provider name directly from model string to avoid instantiating twice - const { getProviderForModel } = await import('@automaker/model-resolver'); - providerName = getProviderForModel(resolvedModel); - logger.info(`[ProviderQuery] Using provider: ${providerName}`); // Map provider name to API key const apiKeyMap: Record<string, string | undefined> = { claude: apiKeys.anthropic, zai: apiKeys.zai, google: apiKeys.google, openai: apiKeys.openai, }; apiKey = providerName ? apiKeyMap[providerName] : undefined; if (apiKey) { logger.info(`[ProviderQuery] Using API key from settings for ${providerName}`); } - } else { - const { getProviderForModel } = await import('@automaker/model-resolver'); - providerName = getProviderForModel(resolvedModel); - logger.info(`[ProviderQuery] Using provider: ${providerName}`); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (75)
CHANGELOG.mdREADME.mdapps/server/package.jsonapps/server/src/index.tsapps/server/src/lib/app-spec-format.tsapps/server/src/lib/provider-query.tsapps/server/src/providers/base-provider.tsapps/server/src/providers/claude-provider.tsapps/server/src/providers/provider-factory.tsapps/server/src/providers/types.tsapps/server/src/providers/zai-provider.tsapps/server/src/routes/app-spec/generate-features-from-spec.tsapps/server/src/routes/app-spec/generate-spec.tsapps/server/src/routes/app-spec/index.tsapps/server/src/routes/app-spec/parse-and-create-features.tsapps/server/src/routes/app-spec/routes/create.tsapps/server/src/routes/context/routes/describe-file.tsapps/server/src/routes/context/routes/describe-image.tsapps/server/src/routes/enhance-prompt/index.tsapps/server/src/routes/enhance-prompt/routes/enhance.tsapps/server/src/routes/features/index.tsapps/server/src/routes/features/routes/generate-title.tsapps/server/src/routes/github/routes/validate-issue.tsapps/server/src/routes/models/routes/available.tsapps/server/src/routes/models/routes/providers.tsapps/server/src/routes/setup/common.tsapps/server/src/routes/setup/index.tsapps/server/src/routes/setup/routes/api-keys.tsapps/server/src/routes/setup/routes/delete-api-key.tsapps/server/src/routes/setup/routes/store-api-key.tsapps/server/src/routes/setup/routes/verify-zai-auth.tsapps/server/src/routes/suggestions/generate-suggestions.tsapps/server/src/services/agent-service.tsapps/server/src/services/auto-mode-service.tsapps/server/src/services/settings-service.tsapps/server/tests/unit/lib/provider-query.test.tsapps/server/tests/unit/providers/provider-factory.test.tsapps/server/tests/unit/providers/zai-provider.test.tsapps/ui/src/app.tsxapps/ui/src/components/views/agent-view.tsxapps/ui/src/components/views/board-view/shared/model-constants.tsapps/ui/src/components/views/board-view/shared/model-selector.tsxapps/ui/src/components/views/board-view/shared/profile-quick-select.tsxapps/ui/src/components/views/profiles-view/components/profile-form.tsxapps/ui/src/components/views/profiles-view/constants.tsapps/ui/src/components/views/profiles-view/utils.tsapps/ui/src/components/views/settings-view.tsxapps/ui/src/components/views/settings-view/ai-enhancement/ai-enhancement-section.tsxapps/ui/src/components/views/settings-view/api-keys/api-keys-section.tsxapps/ui/src/components/views/settings-view/api-keys/authentication-status-display.tsxapps/ui/src/components/views/settings-view/api-keys/hooks/use-api-key-management.tsapps/ui/src/components/views/settings-view/api-keys/provider-card.tsxapps/ui/src/components/views/settings-view/feature-defaults/feature-defaults-section.tsxapps/ui/src/components/views/setup-view.tsxapps/ui/src/components/views/setup-view/steps/index.tsapps/ui/src/components/views/setup-view/steps/provider-selection-step.tsxapps/ui/src/components/views/setup-view/steps/zai-setup-step.tsxapps/ui/src/config/api-providers.tsapps/ui/src/hooks/use-electron-agent.tsapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/lib/electron.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/lib/utils.tsapps/ui/src/store/app-store.tsapps/ui/src/store/setup-store.tsapps/ui/src/types/electron.d.tsapps/ui/src/utils/model-utils.tsdocs/server/providers.mdlibs/model-resolver/src/index.tslibs/model-resolver/src/resolver.tslibs/types/src/index.tslibs/types/src/model-display.tslibs/types/src/model.tslibs/types/src/provider.tslibs/types/src/settings.ts
🧰 Additional context used
🧬 Code graph analysis (39)
apps/ui/src/hooks/use-settings-migration.ts (2)
apps/ui/src/lib/electron.ts (1)
isElectron(661-671)apps/ui/src/lib/http-api-client.ts (1)
getHttpApiClient(1147-1152)
apps/server/src/routes/enhance-prompt/index.ts (2)
apps/server/src/services/settings-service.ts (1)
SettingsService(104-613)apps/server/src/routes/enhance-prompt/routes/enhance.ts (1)
createEnhanceHandler(95-198)
apps/server/src/providers/base-provider.ts (1)
apps/server/src/providers/types.ts (1)
ProviderFeature(31-31)
apps/ui/src/app.tsx (1)
apps/ui/src/hooks/use-settings-migration.ts (1)
useEnabledProvidersLoader(352-361)
apps/server/src/routes/app-spec/routes/create.ts (2)
apps/server/src/lib/events.ts (1)
EventEmitter(10-13)apps/server/src/services/settings-service.ts (1)
SettingsService(104-613)
apps/ui/src/components/views/settings-view/ai-enhancement/ai-enhancement-section.tsx (2)
apps/ui/src/utils/model-utils.ts (1)
autoSwitchModelIfDisabled(3-63)apps/ui/src/components/views/board-view/shared/model-constants.ts (1)
getFilteredModels(73-82)
libs/types/src/model-display.ts (3)
libs/types/src/index.ts (2)
ZAI_MODELS(97-97)ModelOption(94-94)apps/ui/src/components/views/board-view/shared/model-constants.ts (2)
ZAI_MODELS(36-65)ModelOption(4-10)apps/ui/src/components/views/profiles-view/constants.ts (1)
ZAI_MODELS(31-36)
apps/ui/src/components/views/profiles-view/components/profile-form.tsx (4)
apps/ui/src/utils/model-utils.ts (1)
autoSwitchModelIfDisabled(3-63)apps/ui/src/components/views/profiles-view/utils.ts (1)
getProviderFromModel(4-15)apps/ui/src/components/views/board-view/shared/model-constants.ts (1)
ALL_MODELS(67-67)apps/ui/src/components/views/profiles-view/constants.ts (1)
ALL_MODELS(38-38)
apps/server/src/routes/app-spec/generate-spec.ts (4)
apps/server/src/routes/app-spec/common.ts (1)
logAuthStatus(34-47)libs/types/src/index.ts (2)
SpecOutput(59-59)specOutputSchema(60-60)apps/server/src/lib/app-spec-format.ts (2)
SpecOutput(9-9)specOutputSchema(10-10)apps/server/src/lib/provider-query.ts (1)
executeProviderQuery(138-296)
apps/server/tests/unit/lib/provider-query.test.ts (1)
apps/server/src/lib/provider-query.ts (3)
buildStructuredOutputPrompt(72-83)parseJsonFromText(302-343)executeProviderQuery(138-296)
apps/server/src/index.ts (3)
apps/server/src/routes/features/index.ts (1)
createFeaturesRoutes(17-32)apps/server/src/routes/auto-mode/index.ts (1)
createAutoModeRoutes(21-68)apps/server/src/routes/enhance-prompt/index.ts (1)
createEnhancePromptRoutes(18-24)
apps/server/src/routes/suggestions/generate-suggestions.ts (1)
apps/server/src/lib/provider-query.ts (1)
executeProviderQuery(138-296)
apps/ui/src/components/views/settings-view/api-keys/provider-card.tsx (3)
apps/ui/src/config/api-providers.ts (1)
ProviderKey(4-4)apps/ui/src/lib/utils.ts (1)
cn(5-7)apps/ui/src/components/ui/button.tsx (1)
Button(108-108)
apps/server/src/routes/setup/routes/verify-zai-auth.ts (3)
apps/server/src/providers/zai-provider.ts (1)
getApiKey(961-970)apps/server/src/routes/setup/common.ts (1)
getApiKey(64-66)apps/server/src/providers/provider-factory.ts (1)
ProviderFactory(31-141)
apps/server/src/routes/setup/routes/api-keys.ts (2)
apps/server/src/providers/zai-provider.ts (1)
getApiKey(961-970)apps/server/src/routes/setup/common.ts (1)
getApiKey(64-66)
apps/ui/src/components/views/profiles-view/utils.ts (3)
libs/types/src/index.ts (2)
AgentModel(52-52)ModelProvider(71-71)libs/types/src/model.ts (1)
AgentModel(43-43)libs/types/src/settings.ts (2)
AgentModel(12-12)ModelProvider(71-71)
apps/ui/src/hooks/use-electron-agent.ts (1)
apps/ui/src/types/electron.d.ts (1)
Message(13-21)
apps/server/src/services/auto-mode-service.ts (2)
libs/model-resolver/src/resolver.ts (3)
getModelForUseCase(154-178)resolveModelString(50-126)resolveModelWithProviderAvailability(210-269)libs/types/src/model.ts (1)
DEFAULT_MODELS(32-35)
apps/ui/src/components/views/board-view/shared/model-constants.ts (1)
libs/types/src/model-display.ts (3)
ZAI_MODELS(68-97)ModelOption(13-24)CLAUDE_MODELS(41-63)
apps/server/tests/unit/providers/provider-factory.test.ts (2)
apps/server/src/providers/provider-factory.ts (1)
ProviderFactory(31-141)apps/server/src/providers/zai-provider.ts (1)
ZaiProvider(916-1826)
apps/ui/src/components/views/profiles-view/constants.ts (4)
libs/types/src/index.ts (3)
ZAI_MODELS(97-97)AgentModel(52-52)CLAUDE_MODELS(96-96)apps/ui/src/components/views/board-view/shared/model-constants.ts (3)
ZAI_MODELS(36-65)ALL_MODELS(67-67)CLAUDE_MODELS(12-34)libs/types/src/model-display.ts (2)
ZAI_MODELS(68-97)CLAUDE_MODELS(41-63)libs/types/src/model.ts (1)
AgentModel(43-43)
apps/server/src/routes/context/routes/describe-image.ts (2)
libs/model-resolver/src/resolver.ts (1)
resolveModelString(50-126)apps/server/src/lib/provider-query.ts (1)
executeProviderQuery(138-296)
apps/server/tests/unit/providers/zai-provider.test.ts (2)
apps/server/tests/utils/helpers.ts (1)
collectAsyncGenerator(8-14)apps/server/src/lib/secure-fs.ts (1)
secureFs(8-28)
libs/types/src/settings.ts (2)
libs/types/src/index.ts (1)
ModelProvider(71-71)apps/server/src/types/settings.ts (1)
ModelProvider(14-14)
apps/ui/src/store/app-store.ts (2)
apps/ui/src/lib/http-api-client.ts (2)
get(174-178)getHttpApiClient(1147-1152)apps/ui/src/lib/electron.ts (1)
isElectron(661-671)
apps/server/src/routes/setup/common.ts (1)
apps/server/src/providers/types.ts (1)
ProviderConfig(10-10)
libs/types/src/provider.ts (2)
apps/server/src/providers/types.ts (9)
ThinkingConfig(13-13)ExecuteOptions(14-14)ConversationMessage(11-11)TextBlock(15-15)ToolUseBlock(16-16)ThinkingBlock(17-17)ToolResultBlock(18-18)ContentBlock(19-19)LegacyContentBlock(20-20)libs/types/src/index.ts (9)
ThinkingConfig(11-11)ExecuteOptions(12-12)ConversationMessage(9-9)TextBlock(13-13)ToolUseBlock(14-14)ThinkingBlock(15-15)ToolResultBlock(16-16)ContentBlock(17-17)LegacyContentBlock(18-18)
apps/server/src/routes/context/routes/describe-file.ts (1)
apps/server/src/lib/provider-query.ts (1)
executeProviderQuery(138-296)
apps/ui/src/components/views/settings-view/api-keys/api-keys-section.tsx (1)
apps/ui/src/components/views/settings-view/api-keys/provider-card.tsx (1)
ProviderCard(54-211)
apps/server/src/routes/app-spec/index.ts (1)
apps/server/src/routes/app-spec/routes/create.ts (1)
createCreateHandler(20-95)
apps/server/src/providers/claude-provider.ts (2)
apps/server/src/providers/base-provider.ts (1)
ProviderFeature(21-28)apps/server/src/providers/types.ts (1)
ProviderFeature(31-31)
apps/server/src/routes/github/routes/validate-issue.ts (1)
apps/server/src/lib/provider-query.ts (1)
executeProviderQuery(138-296)
libs/model-resolver/src/resolver.ts (3)
libs/types/src/index.ts (6)
DEFAULT_MODELS(47-47)CLAUDE_MODEL_MAP(44-44)ZAI_MODEL_MAP(45-45)ALL_MODEL_MAPS(46-46)getProviderForModel(50-50)MODEL_EQUIVALENCE(48-48)libs/types/src/model.ts (6)
DEFAULT_MODELS(32-35)CLAUDE_MODEL_MAP(4-8)ZAI_MODEL_MAP(16-27)ALL_MODEL_MAPS(48-51)getProviderForModel(97-116)MODEL_EQUIVALENCE(67-77)apps/server/src/providers/provider-factory.ts (1)
getProviderForModel(63-81)
apps/ui/src/components/views/settings-view/api-keys/hooks/use-api-key-management.ts (1)
apps/ui/src/lib/electron.ts (1)
getElectronAPI(713-722)
libs/types/src/model.ts (2)
libs/types/src/index.ts (9)
ZAI_MODEL_MAP(45-45)DEFAULT_MODELS(47-47)ModelAlias(51-51)CLAUDE_MODEL_MAP(44-44)AgentModel(52-52)ALL_MODEL_MAPS(46-46)MODEL_EQUIVALENCE(48-48)PROVIDER_TO_API_KEY_NAME(49-49)getProviderForModel(50-50)apps/server/src/providers/provider-factory.ts (1)
getProviderForModel(63-81)
apps/server/src/routes/setup/index.ts (1)
apps/server/src/routes/setup/routes/verify-zai-auth.ts (2)
zaiAuthRateLimit(24-33)createVerifyZaiAuthHandler(61-158)
apps/ui/src/components/views/agent-view.tsx (3)
apps/ui/src/utils/model-utils.ts (1)
autoSwitchModelIfDisabled(3-63)apps/ui/src/components/views/board-view/shared/model-constants.ts (2)
ALL_MODELS(67-67)getFilteredModels(73-82)apps/ui/src/components/views/profiles-view/constants.ts (1)
ALL_MODELS(38-38)
apps/server/src/routes/setup/routes/delete-api-key.ts (1)
apps/server/src/routes/setup/common.ts (1)
getProviderEnvKey(47-59)
apps/server/src/routes/features/index.ts (2)
apps/server/src/services/settings-service.ts (1)
SettingsService(104-613)apps/server/src/routes/features/routes/generate-title.ts (1)
createGenerateTitleHandler(72-149)
🪛 ast-grep (0.40.3)
apps/server/src/providers/zai-provider.ts
[warning] 660-660: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${regexStr}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 703-703: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(pattern, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
13-13: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
23-23: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
29-29: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
37-37: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
49-49: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
59-59: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
67-67: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
87-87: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
96-96: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
103-103: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
109-109: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
115-115: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
125-125: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
132-132: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
138-138: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
145-145: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
151-151: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
158-158: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
181-181: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
195-195: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
201-201: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
215-215: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
220-220: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
221-221: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
239-239: Bare URL used
(MD034, no-bare-urls)
| } catch (error) { | ||
| logger.error('[ProviderQuery] Error during execution:', error); | ||
| yield { | ||
| type: 'error', | ||
| error: (error as Error).message, | ||
| }; | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Error is yielded then re-thrown, potentially causing double handling.
The error is yielded as a message (lines 288-291) and then re-thrown (line 292). Callers iterating over the generator will receive the error message and then catch the exception, potentially handling the same error twice.
🔎 Proposed fix: Choose one error propagation method
} catch (error) {
logger.error('[ProviderQuery] Error during execution:', error);
yield {
type: 'error',
error: (error as Error).message,
- };
- throw error;
+ } as ProviderMessage;
+ // Don't re-throw - error is communicated via the yielded message
+ // Callers should check for 'error' type messages
}Alternatively, if throwing is preferred for flow control, remove the yield.
📝 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.
| } catch (error) { | |
| logger.error('[ProviderQuery] Error during execution:', error); | |
| yield { | |
| type: 'error', | |
| error: (error as Error).message, | |
| }; | |
| throw error; | |
| } | |
| } catch (error) { | |
| logger.error('[ProviderQuery] Error during execution:', error); | |
| yield { | |
| type: 'error', | |
| error: (error as Error).message, | |
| } as ProviderMessage; | |
| // Don't re-throw - error is communicated via the yielded message | |
| // Callers should check for 'error' type messages | |
| } |
🤖 Prompt for AI Agents
In apps/server/src/lib/provider-query.ts around lines 286–293 the catch block
both yields an error object and then re-throws the same error, which leads to
callers potentially handling the same error twice; pick one propagation method
and implement it consistently: either remove the yield and just throw the error
so consumers receive it via exception, or remove the throw and yield/return the
error (and allow the generator to finish) so consumers handle it as a yielded
message; update any type signatures/comments/tests to match the chosen approach.
| // Split by whitespace but respect quotes | ||
| const parts: string[] = []; | ||
| let current = ''; | ||
| let inQuote = false; | ||
| let quoteChar = ''; | ||
|
|
||
| for (let i = 0; i < commandWithoutRedirects.length; i++) { | ||
| const char = commandWithoutRedirects[i]; | ||
|
|
||
| if (inQuote) { | ||
| if (char === quoteChar) { | ||
| inQuote = false; | ||
| quoteChar = ''; | ||
| } else { | ||
| current += char; | ||
| } | ||
| } else if (char === '"' || char === "'") { | ||
| inQuote = true; | ||
| quoteChar = char; | ||
| } else if (char === ' ' || char === '\t') { | ||
| if (current) { | ||
| parts.push(current); | ||
| current = ''; | ||
| } | ||
| } else { | ||
| current += char; | ||
| } | ||
| } | ||
|
|
||
| if (current) { | ||
| parts.push(current); | ||
| } |
There was a problem hiding this comment.
Unbalanced quote handling allows command injection bypass.
The quote parsing logic doesn't validate that quotes are balanced. An attacker could craft a command like cat "file.txt (unclosed quote) which would cause inQuote to remain true until end of string, potentially allowing subsequent arguments to bypass validation.
🔎 Proposed fix: Add balanced quote validation
if (current) {
parts.push(current);
}
+ // Check for unbalanced quotes
+ if (inQuote) {
+ throw new Error('Unbalanced quotes in command');
+ }
+
if (parts.length === 0) {
throw new Error('Command cannot be empty');
}🤖 Prompt for AI Agents
In apps/server/src/providers/zai-provider.ts around lines 312 to 343, the
current split-by-whitespace logic doesn't validate that quotes are balanced
which can allow unclosed-quote input to be treated as a single argument and
bypass validation; update the parser to detect unbalanced quotes by checking
after the loop if inQuote is still true and throw or return a validation error
(include which quoteChar/unclosed position if available) instead of silently
accepting the input, and ensure any caller handles that error so
unbalanced-quote commands are rejected rather than processed.
| function globToRegex(globPattern: string, basePath: string): RegExp { | ||
| // Escape special regex characters except for glob wildcards | ||
| let regexStr = globPattern | ||
| .replace(/\./g, '\\.') // Literal dots | ||
| .replace(/\?/g, '[^/]') // ? matches any single character except / | ||
| .replace(/\*\*/g, '.*') // ** matches any number of path segments | ||
| .replace(/\*/g, '[^/]*'); // * matches any characters except / | ||
|
|
||
| return new RegExp(`^${regexStr}$`); | ||
| } |
There was a problem hiding this comment.
ReDoS vulnerability in glob pattern to regex conversion.
The globToRegex function constructs a regex from user-controlled glob patterns. As flagged by static analysis, patterns like ********** can cause catastrophic backtracking since ** becomes .* and chained wildcards create exponential complexity.
🔎 Proposed fix: Normalize consecutive wildcards
function globToRegex(globPattern: string, basePath: string): RegExp {
+ // Normalize consecutive wildcards to prevent ReDoS
+ let normalized = globPattern
+ .replace(/\*{3,}/g, '**') // Collapse 3+ stars to **
+ .replace(/(\*\*\/)+/g, '**/'); // Collapse repeated **/ patterns
+
// Escape special regex characters except for glob wildcards
- let regexStr = globPattern
+ let regexStr = normalized
.replace(/\./g, '\\.') // Literal dots
.replace(/\?/g, '[^/]') // ? matches any single character except /
- .replace(/\*\*/g, '.*') // ** matches any number of path segments
- .replace(/\*/g, '[^/]*'); // * matches any characters except /
+ .replace(/\*\*/g, '(?:[^/]*/)*') // ** matches any number of path segments (non-greedy)
+ .replace(/\*/g, '[^/]*?'); // * matches any characters except / (non-greedy)
return new RegExp(`^${regexStr}$`);
}🧰 Tools
🪛 ast-grep (0.40.3)
[warning] 660-660: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${regexStr}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
| async function grep( | ||
| pattern: string, | ||
| searchPath: string | ||
| ): Promise<Array<{ path: string; line: number; text: string }>> { | ||
| // Validate pattern for security | ||
| // Since we use native RegExp (not shell), allow most regex metacharacters | ||
| // Only block shell command separators that could enable injection | ||
| if (/[;|`]/.test(pattern)) { | ||
| throw new Error('Invalid characters in grep pattern'); | ||
| } | ||
|
|
||
| // Check for path traversal attempts in pattern | ||
| if (pattern.includes('..')) { | ||
| throw new Error('Path traversal not allowed in grep pattern'); | ||
| } | ||
|
|
||
| try { | ||
| // Find all files with matching extensions in the search path | ||
| const extensionPatterns = Array.from(GREP_EXTENSIONS).flatMap((ext) => [ | ||
| `**/*.${ext}`, | ||
| `**/*.${ext.toUpperCase()}`, | ||
| ]); | ||
|
|
||
| // Collect all matching file paths | ||
| const filePathsSet = new Set<string>(); | ||
| for (const extPattern of extensionPatterns) { | ||
| const matches = await glob(extPattern, searchPath); | ||
| for (const match of matches) { | ||
| filePathsSet.add(match); | ||
| } | ||
| } | ||
|
|
||
| const filePaths = Array.from(filePathsSet).slice(0, 1000); // Limit to 1000 files | ||
|
|
||
| // Search through each file | ||
| const results: Array<{ path: string; line: number; text: string }> = []; | ||
| const regex = new RegExp(pattern, 'i'); // Case-insensitive search |
There was a problem hiding this comment.
ReDoS vulnerability in grep pattern handling.
The user-provided pattern is passed directly to new RegExp(pattern, 'i') at line 704. Malicious patterns like (a+)+$ can cause catastrophic backtracking. Consider adding pattern complexity limits or timeout mechanisms.
🔎 Proposed fix: Add regex complexity validation
+/**
+ * Validate regex pattern complexity to prevent ReDoS
+ */
+function validateRegexComplexity(pattern: string): void {
+ // Limit pattern length
+ if (pattern.length > 200) {
+ throw new Error('Regex pattern too long');
+ }
+ // Detect common ReDoS patterns: nested quantifiers
+ if (/(\+|\*|\{)\s*\)(\+|\*|\?|\{)/.test(pattern)) {
+ throw new Error('Potentially dangerous nested quantifiers in regex');
+ }
+}
+
async function grep(
pattern: string,
searchPath: string
): Promise<Array<{ path: string; line: number; text: string }>> {
// Validate pattern for security
if (/[;|`]/.test(pattern)) {
throw new Error('Invalid characters in grep pattern');
}
+
+ // Validate regex complexity
+ validateRegexComplexity(pattern);🧰 Tools
🪛 ast-grep (0.40.3)
[warning] 703-703: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(pattern, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
In apps/server/src/providers/zai-provider.ts around lines 668 to 704, the code
constructs new RegExp(pattern, 'i') directly from user input which allows ReDoS
vectors; validate and/or constrain the pattern before creating the RegExp:
enforce a max pattern length (e.g. 200 chars), reject patterns containing known
catastrophic constructs (nested quantifiers like /(x+)+/, backtracking groups
such as (.+)+, or excessive repetition tokens), or integrate a safe-regex check
(e.g. the safe-regex npm utility) to reject unsafe patterns; alternatively run
regex matching in a cancellable worker with a short timeout per file to prevent
hangs; update the code to throw a clear error when the pattern is rejected and
only construct new RegExp after it passes these safety checks.
| try { | ||
| const stat = await fs.stat(absolutePath); | ||
| if (stat.isSymbolicLink()) { | ||
| const targetPath = path.resolve( | ||
| path.dirname(absolutePath), | ||
| await fs.readlink(absolutePath) | ||
| ); | ||
| const targetRelative = path.relative(baseCwd, targetPath); | ||
| if (targetRelative.startsWith('..')) { | ||
| throw new Error(`Symlink targets outside working directory: ${filePath}`); | ||
| } | ||
| } | ||
| } catch { | ||
| // File doesn't exist yet - allow for new file creation | ||
| // But check parent directory for symlinks | ||
| try { | ||
| const parentDir = path.dirname(absolutePath); | ||
| const parentStat = await fs.stat(parentDir); | ||
| if (parentStat.isSymbolicLink()) { | ||
| const targetPath = path.resolve(path.dirname(parentDir), await fs.readlink(parentDir)); | ||
| const targetRelative = path.relative(baseCwd, targetPath); | ||
| if (targetRelative.startsWith('..')) { | ||
| throw new Error(`Parent directory symlink targets outside working directory`); | ||
| } | ||
| } | ||
| } catch { | ||
| // Parent doesn't exist either - will be created | ||
| } | ||
| } |
There was a problem hiding this comment.
TOCTOU race condition in symlink checking.
The code checks stat.isSymbolicLink() but fs.stat follows symlinks by default and never returns true for isSymbolicLink(). You need fs.lstat to detect symlinks. Additionally, there's a race condition between the check and use.
🔎 Proposed fix: Use lstat for symlink detection
// Check for symlink escape attempts
try {
- const stat = await fs.stat(absolutePath);
- if (stat.isSymbolicLink()) {
+ const stat = await fs.lstat(absolutePath);
+ if (stat.isSymbolicLink()) {
const targetPath = path.resolve(
path.dirname(absolutePath),
await fs.readlink(absolutePath)
);
- const targetRelative = path.relative(baseCwd, targetPath);
- if (targetRelative.startsWith('..')) {
+ // Resolve the real path to handle nested symlinks
+ const realPath = await fs.realpath(absolutePath);
+ const targetRelative = path.relative(baseCwd, realPath);
+ if (targetRelative.startsWith('..') || path.isAbsolute(targetRelative)) {
throw new Error(`Symlink targets outside working directory: ${filePath}`);
}
}📝 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.
| try { | |
| const stat = await fs.stat(absolutePath); | |
| if (stat.isSymbolicLink()) { | |
| const targetPath = path.resolve( | |
| path.dirname(absolutePath), | |
| await fs.readlink(absolutePath) | |
| ); | |
| const targetRelative = path.relative(baseCwd, targetPath); | |
| if (targetRelative.startsWith('..')) { | |
| throw new Error(`Symlink targets outside working directory: ${filePath}`); | |
| } | |
| } | |
| } catch { | |
| // File doesn't exist yet - allow for new file creation | |
| // But check parent directory for symlinks | |
| try { | |
| const parentDir = path.dirname(absolutePath); | |
| const parentStat = await fs.stat(parentDir); | |
| if (parentStat.isSymbolicLink()) { | |
| const targetPath = path.resolve(path.dirname(parentDir), await fs.readlink(parentDir)); | |
| const targetRelative = path.relative(baseCwd, targetPath); | |
| if (targetRelative.startsWith('..')) { | |
| throw new Error(`Parent directory symlink targets outside working directory`); | |
| } | |
| } | |
| } catch { | |
| // Parent doesn't exist either - will be created | |
| } | |
| } | |
| try { | |
| const stat = await fs.lstat(absolutePath); | |
| if (stat.isSymbolicLink()) { | |
| const targetPath = path.resolve( | |
| path.dirname(absolutePath), | |
| await fs.readlink(absolutePath) | |
| ); | |
| // Resolve the real path to handle nested symlinks | |
| const realPath = await fs.realpath(absolutePath); | |
| const targetRelative = path.relative(baseCwd, realPath); | |
| if (targetRelative.startsWith('..') || path.isAbsolute(targetRelative)) { | |
| throw new Error(`Symlink targets outside working directory: ${filePath}`); | |
| } | |
| } | |
| } catch { | |
| // File doesn't exist yet - allow for new file creation | |
| // But check parent directory for symlinks | |
| try { | |
| const parentDir = path.dirname(absolutePath); | |
| const parentStat = await fs.stat(parentDir); | |
| if (parentStat.isSymbolicLink()) { | |
| const targetPath = path.resolve(path.dirname(parentDir), await fs.readlink(parentDir)); | |
| const targetRelative = path.relative(baseCwd, targetPath); | |
| if (targetRelative.startsWith('..')) { | |
| throw new Error(`Parent directory symlink targets outside working directory`); | |
| } | |
| } | |
| } catch { | |
| // Parent doesn't exist either - will be created | |
| } | |
| } |
| // Auto-switch model if current model's provider is disabled | ||
| useEffect(() => { | ||
| const newModel = autoSwitchModelIfDisabled(formData.model, enabledProviders); | ||
| if (newModel !== formData.model) { | ||
| setFormData({ ...formData, model: newModel }); | ||
| } | ||
| }, [enabledProviders]); |
There was a problem hiding this comment.
Potential stale closure issue with formData in useEffect.
The setFormData({ ...formData, model: newModel }) on line 46 uses the formData from the closure, which may be stale if other form fields have changed between renders. Use the functional update form to ensure you're working with the latest state.
Additionally, formData.model is read in the effect but not in the dependency array. While this may be intentional (only react to provider changes), including it or using a functional update pattern would make the intent clearer.
🔎 Proposed fix - use functional update
useEffect(() => {
- const newModel = autoSwitchModelIfDisabled(formData.model, enabledProviders);
- if (newModel !== formData.model) {
- setFormData({ ...formData, model: newModel });
- }
- }, [enabledProviders]);
+ setFormData((prev) => {
+ const newModel = autoSwitchModelIfDisabled(prev.model, enabledProviders);
+ if (newModel !== prev.model) {
+ return { ...prev, model: newModel };
+ }
+ return prev;
+ });
+ }, [enabledProviders]);📝 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.
| // Auto-switch model if current model's provider is disabled | |
| useEffect(() => { | |
| const newModel = autoSwitchModelIfDisabled(formData.model, enabledProviders); | |
| if (newModel !== formData.model) { | |
| setFormData({ ...formData, model: newModel }); | |
| } | |
| }, [enabledProviders]); | |
| // Auto-switch model if current model's provider is disabled | |
| useEffect(() => { | |
| setFormData((prev) => { | |
| const newModel = autoSwitchModelIfDisabled(prev.model, enabledProviders); | |
| if (newModel !== prev.model) { | |
| return { ...prev, model: newModel }; | |
| } | |
| return prev; | |
| }); | |
| }, [enabledProviders]); |
| // Auto-switch model if current model's provider is disabled | ||
| useEffect(() => { | ||
| const newModel = autoSwitchModelIfDisabled(enhancementModel, enabledProviders); | ||
| if (newModel !== enhancementModel) { | ||
| setEnhancementModel(newModel); | ||
| } | ||
| }, [enabledProviders]); |
There was a problem hiding this comment.
Add missing dependencies to the useEffect hook.
The useEffect hook references enhancementModel and setEnhancementModel but doesn't include them in the dependency array. This can lead to stale closures or unexpected behavior.
🔎 Proposed fix
useEffect(() => {
const newModel = autoSwitchModelIfDisabled(enhancementModel, enabledProviders);
if (newModel !== enhancementModel) {
setEnhancementModel(newModel);
}
- }, [enabledProviders]);
+ }, [enabledProviders, enhancementModel, setEnhancementModel]);🤖 Prompt for AI Agents
In
apps/ui/src/components/views/settings-view/ai-enhancement/ai-enhancement-section.tsx
around lines 12 to 18, the useEffect references enhancementModel and
setEnhancementModel (and calls autoSwitchModelIfDisabled) but only lists
enabledProviders in the dependency array; update the dependency array to include
enhancementModel and setEnhancementModel (and autoSwitchModelIfDisabled if it's
not stable) so the effect reruns when those values change, preventing stale
closures and ensuring correct behavior.
| // Handle save with auto-enable provider on successful save | ||
| const handleSaveWithAutoEnable = useCallback(async () => { | ||
| await handleSave(); | ||
|
|
||
| // Auto-enable providers that have valid API keys | ||
| setEnabledProviders({ | ||
| claude: Boolean(anthropic.value && anthropic.result?.success), | ||
| zai: Boolean(zai.value && zai.result?.success), | ||
| }); | ||
| }, [handleSave, anthropic, zai, setEnabledProviders]); | ||
|
|
||
| // Auto-enable providers when a key exists and user never disabled the switch | ||
| useEffect(() => { | ||
| const claudeValidated = anthropic.result?.success || apiKeyStatus?.hasAnthropicKey; | ||
| const zaiValidated = zai.result?.success || apiKeyStatus?.hasZaiKey; | ||
|
|
||
| if (claudeValidated && !enabledProviders.claude && !providerToggleTouched.claude) { | ||
| setEnabledProviders({ claude: true }); | ||
| } | ||
| if (zaiValidated && !enabledProviders.zai && !providerToggleTouched.zai) { | ||
| setEnabledProviders({ zai: true }); | ||
| } | ||
| }, [ |
There was a problem hiding this comment.
Respect manual provider toggles when auto‑enabling on save
handleSaveWithAutoEnable unconditionally sets enabledProviders for Claude/Zai based on key + test result. If a user has explicitly turned a provider off via the toggle (which sets providerToggleTouched[provider] = true), clicking “Save API Keys” will override that decision and re‑enable the provider, even though the useEffect below is designed not to auto‑enable once a toggle has been manually touched.
It would be better to only auto‑enable a provider here when its toggle has never been touched, mirroring the logic in the effect.
Proposed change to guard auto‑enable with providerToggleTouched
- const handleSaveWithAutoEnable = useCallback(async () => {
- await handleSave();
-
- // Auto-enable providers that have valid API keys
- setEnabledProviders({
- claude: Boolean(anthropic.value && anthropic.result?.success),
- zai: Boolean(zai.value && zai.result?.success),
- });
- }, [handleSave, anthropic, zai, setEnabledProviders]);
+ const handleSaveWithAutoEnable = useCallback(async () => {
+ await handleSave();
+
+ // Auto-enable providers that have valid API keys,
+ // but only if the user has never manually touched the toggle.
+ const updates: Partial<{ claude: boolean; zai: boolean }> = {};
+
+ if (!providerToggleTouched.claude) {
+ updates.claude = Boolean(anthropic.value && anthropic.result?.success);
+ }
+ if (!providerToggleTouched.zai) {
+ updates.zai = Boolean(zai.value && zai.result?.success);
+ }
+
+ if (Object.keys(updates).length > 0) {
+ setEnabledProviders(updates);
+ }
+ }, [handleSave, anthropic, zai, providerToggleTouched, setEnabledProviders]);🤖 Prompt for AI Agents
In apps/ui/src/components/views/settings-view/api-keys/api-keys-section.tsx
around lines 70–92, handleSaveWithAutoEnable currently unconditionally sets
enabledProviders for Claude and Zai which can override a user’s explicit toggle
choice; change it to only auto-enable a provider when its toggle has never been
touched (providerToggleTouched[provider] is false) and merge with the existing
enabledProviders state instead of replacing it so you don’t clobber the other
provider’s value.
| const equivalentModel = ALL_MODELS_INFO.find( | ||
| (m) => | ||
| // Different provider | ||
| (isClaudeModel ? ZAI_MODELS : CLAUDE_MODELS).includes(m) && | ||
| // Same badge tier | ||
| m.badge === currentModelInfo.badge && | ||
| // Provider is enabled | ||
| (isClaudeModel ? enabledProviders.zai : enabledProviders.claude) | ||
| ); |
There was a problem hiding this comment.
Fix fragile object identity comparison in .includes().
The condition (isClaudeModel ? ZAI_MODELS : CLAUDE_MODELS).includes(m) compares object references. While it works here because ALL_MODELS_INFO is created from spread copies of the same arrays, this pattern is fragile and could break if the data source changes.
🔎 Suggested fix using id comparison
const equivalentModel = ALL_MODELS_INFO.find(
(m) =>
// Different provider
- (isClaudeModel ? ZAI_MODELS : CLAUDE_MODELS).includes(m) &&
+ (isClaudeModel
+ ? ZAI_MODELS.some(zm => zm.id === m.id)
+ : CLAUDE_MODELS.some(cm => cm.id === m.id)) &&
// Same badge tier
m.badge === currentModelInfo.badge &&
// Provider is enabled
(isClaudeModel ? enabledProviders.zai : enabledProviders.claude)
);📝 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 equivalentModel = ALL_MODELS_INFO.find( | |
| (m) => | |
| // Different provider | |
| (isClaudeModel ? ZAI_MODELS : CLAUDE_MODELS).includes(m) && | |
| // Same badge tier | |
| m.badge === currentModelInfo.badge && | |
| // Provider is enabled | |
| (isClaudeModel ? enabledProviders.zai : enabledProviders.claude) | |
| ); | |
| const equivalentModel = ALL_MODELS_INFO.find( | |
| (m) => | |
| // Different provider | |
| (isClaudeModel | |
| ? ZAI_MODELS.some(zm => zm.id === m.id) | |
| : CLAUDE_MODELS.some(cm => cm.id === m.id)) && | |
| // Same badge tier | |
| m.badge === currentModelInfo.badge && | |
| // Provider is enabled | |
| (isClaudeModel ? enabledProviders.zai : enabledProviders.claude) | |
| ); |
🤖 Prompt for AI Agents
In apps/ui/src/utils/model-utils.ts around lines 40 to 48, the check using
(isClaudeModel ? ZAI_MODELS : CLAUDE_MODELS).includes(m) relies on object
identity and is fragile; change it to compare a stable identifier (e.g., m.id or
m.name) against the ids/names of the target provider array instead of using
.includes() on objects, e.g., build a Set of ids from (isClaudeModel ?
ZAI_MODELS : CLAUDE_MODELS) and test set.has(m.id); keep the existing badge and
enabledProviders checks unchanged.
| // All providers disabled - this is an error condition | ||
| console.error(`[ModelResolver] No providers are enabled! Cannot resolve model: ${resolvedModel}`); | ||
|
|
||
| // Return the defaultModel if provided, otherwise return the resolved model | ||
| // (calling code should handle this error case) | ||
| return defaultModel || resolvedModel; |
There was a problem hiding this comment.
Silent failure when all providers disabled.
When all providers are disabled (line 263-268), the function logs an error but returns a model that won't work. Consider throwing an error or returning a structured error response.
🔎 Proposed fix: Make error handling explicit
// All providers disabled - this is an error condition
console.error(`[ModelResolver] No providers are enabled! Cannot resolve model: ${resolvedModel}`);
- // Return the defaultModel if provided, otherwise return the resolved model
- // (calling code should handle this error case)
- return defaultModel || resolvedModel;
+ // Throw to make the error explicit rather than returning an unusable model
+ throw new Error(`No AI providers are enabled. Please enable at least one provider in settings.`);
}Alternatively, if throwing is not desired, consider returning a sentinel value that callers can check.
📝 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.
| // All providers disabled - this is an error condition | |
| console.error(`[ModelResolver] No providers are enabled! Cannot resolve model: ${resolvedModel}`); | |
| // Return the defaultModel if provided, otherwise return the resolved model | |
| // (calling code should handle this error case) | |
| return defaultModel || resolvedModel; | |
| // All providers disabled - this is an error condition | |
| console.error(`[ModelResolver] No providers are enabled! Cannot resolve model: ${resolvedModel}`); | |
| // Throw to make the error explicit rather than returning an unusable model | |
| throw new Error(`No AI providers are enabled. Please enable at least one provider in settings.`); |
🤖 Prompt for AI Agents
In libs/model-resolver/src/resolver.ts around lines 263–268, the code currently
logs an error when all providers are disabled but then returns a model that will
never work; change this to either throw a descriptive Error (e.g. throw new
Error(`[ModelResolver] No providers enabled for model: ${resolvedModel}`)) so
callers immediately fail, or, if the API must remain non-throwing, return a
typed sentinel error object (e.g. { error: 'NO_PROVIDERS_ENABLED', model:
resolvedModel }) and update the function's return type and all callers to detect
and handle that sentinel; ensure logging includes context and adjust callers to
handle the new error path.
The dangerous pattern /format\s/ was meant to block the Windows format command (which formats disk drives), but it was too broad and also matched the --format flag used by Docker and other tools. Changed all system command patterns to use anchors (^\s*) so they only match when the command is at the start of the line, not as a flag. Fixes: - docker ps --format "..." (previously blocked, now allowed) - docker --filter ... (no change) Still blocks: - format c: (Windows disk format) - reboot, halt, poweroff, fdisk, etc. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Previously, the provider toggle would auto-enable if there was any stored API key, even if it was never validated. Now it only enables when: 1. An API key is stored (hasKey) 2. AND the key passed the connection test (result.success) This prevents enabling providers with invalid or untested API keys. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Fix symlink bypass: use fs.lstat instead of fs.stat to properly detect symlinks - Fix ReDoS in glob pattern: add input length limit and consecutive wildcard limit - Fix ReDoS in grep pattern: add input validation for nested quantifiers - Fix insecure auth assumption: return authenticated: false on network error - Fix double error handling: return instead of throw after yielding error - Fix JSON comment removal: use state machine to preserve string contents 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
FWIW, it looks like the only change needed to allow AutoMaker to work with Z.ai is: diff --git a/apps/server/src/providers/claude-provider.ts b/apps/server/src/providers/claude-provider.ts
index 50e378be..9836983d 100644
--- a/apps/server/src/providers/claude-provider.ts
+++ b/apps/server/src/providers/claude-provider.ts
@@ -22,6 +22,8 @@ import type {
// Only these vars are passed - nothing else from process.env leaks through.
const ALLOWED_ENV_VARS = [
'ANTHROPIC_API_KEY',
+ 'ANTHROPIC_AUTH_TOKEN',
+ 'ANTHROPIC_BASE_URL',
'PATH',
'HOME',
'SHELL',and then to set the following env vars to be passed through above: This seems to work fine on their $6/mo and $30/mo plans. |
|
closing, too many conflicts |
3 participants
Summary
Implements full integration of Z.ai's GLM models (glm-4.7, glm-4.6v, glm-4.6, glm-4.5-air) as an alternative AI provider to Anthropic's Claude.
77 files changed, ~8,400 insertions, ~1,400 deletions
What's New
Provider System
executeProviderQueryto be provider-agnosticZai Provider Features
UI/UX Changes
Security Improvements
ALLOWED_ROOT_DIRECTORYTesting
Configuration
Users can now:
Checklist
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.