Add gateway providers and default dev-browser off#31
Conversation
Code Review: Add Gateway Providers and Default dev-browser OffGreat work on this PR! The implementation is solid and follows the project's conventions well. Here's my detailed review: ✅ Strengths
🔍 Code Quality Issues1. Security:
|
Pull Request ReviewThis is a solid PR that expands provider coverage with four new gateway providers and improves UX defaults. The code quality is generally high with good attention to consistency and test coverage. Below are my findings: ✅ StrengthsArchitecture & Design
Code Quality
Security
|
Pull Request ReviewThis PR adds three new gateway providers (GatewayZ, Vercel AI Gateway, NanoGPT) and changes the dev-browser skill default to off. I've reviewed the code quality, architecture, and implementation details. ✅ StrengthsCode Quality & Architecture
Testing
Documentation
🔍 Areas for Improvement1. Provider URL Verification (Medium Priority)The PR description mentions these are "verified auth/base URLs", but I don't see evidence of this in the code or tests: GatewayZ (
Vercel AI Gateway (
NanoGPT (
Recommendation: Add inline comments with documentation links or verification notes for each provider's base URL. 2. Model Mapping Inconsistency (Medium Priority)In const placeholders: Record<string, Record<string, string>> = {
openrouter: { ... },
gatewayz: { ... },
vercel: { ... },
ollama: { ... },
ccrouter: { ... },
};
const providerPlaceholders = placeholders[providerKey || ''] || placeholders.ccrouter;Issues:
Are these placeholder formats verified? Different gateway providers often have different model ID conventions. Recommendation: Verify the actual model ID formats with each provider's documentation and add comments explaining the format differences. 3. Auth Mode Verification (High Priority - Vercel)In authMode: 'authToken',
requiresEmptyApiKey: true,This forces if (provider.requiresEmptyApiKey) {
env.ANTHROPIC_API_KEY = '';
}Concern: Setting Recommendation: Test that Vercel variant actually works with empty 4. Missing Dev-Browser Default Change DocumentationThe PR description says "defaults the dev-browser skill to off", but I don't see this change in the diff. Looking at the changed files list, I don't see:
Recommendation: Verify this change is actually included in the PR, or update the PR description if it was already merged separately. 5. Code Duplication in Brand Files (Low Priority)The three new brand files (
These are duplicated across all brand files. While this follows the existing pattern, it creates maintenance burden. Recommendation: Extract these color utilities to a shared module like 6. Wrapper Script Splash Art (Minor)In Minor observations:
No action needed, just noting the art looks consistent. 🔒 Security ReviewNo Security Concerns Identified
🎯 Performance ConsiderationsNo Performance Issues
📋 Recommendations SummaryBefore Merge:
Future Improvements:
Final AssessmentThis is a well-implemented PR that follows project conventions and adds meaningful functionality. The code quality is high and test coverage is comprehensive. The main concerns are around verification that the provider URLs and auth configurations actually work - I recommend manual testing of at least one new provider variant before merge. Recommendation: Approve with minor changes requested (verify Vercel auth + add documentation links). |
Pull Request Review: Gateway Providers & Dev-Browser DefaultSummaryThis PR successfully expands provider coverage by adding three gateway providers (GatewayZ, Vercel AI Gateway, NanoGPT) plus Ollama, and improves UX by defaulting dev-browser skill to opt-in. The implementation is thorough, well-tested, and follows project conventions. ✅ Strengths1. Code Quality & Architecture
2. Authentication HandlingThe PR correctly handles different auth modes:
The logic in if (authMode === 'authToken') {
if (provider.requiresEmptyApiKey) {
env.ANTHROPIC_API_KEY = '';
} else if (\!provider.authTokenAlsoSetsApiKey && Object.hasOwn(env, 'ANTHROPIC_API_KEY')) {
delete env.ANTHROPIC_API_KEY;
}
}3. UX Improvements
4. Test Coverage
5. Documentation
🔍 Issues & Concerns1. Missing Ollama Brand File (❗ BLOCKER)The PR adds Ollama to:
However, the file
Action required: Verify 2. Vercel Base URL Concern (
|
Pull Request Review: Add Gateway Providers and Default Dev-Browser OffSummaryThis PR expands provider coverage by adding GatewayZ, Vercel AI Gateway, and NanoGPT providers with full integration across core, TUI, CLI, documentation, and tests. Additionally, it defaults the dev-browser skill to off and updates maintainer context. Code Quality & Best Practices ✅Strengths
Potential Issues & Concerns1. Ollama Brand File May Be Misplaced
|
numman-ali
force-pushed
the
providers-gateways
branch
from
cd7194d to
8ec4486
Compare
Code Review: Add gateway providers and default dev-browser offThanks for this comprehensive PR! I've reviewed the changes and have the following feedback: ✅ Strengths1. Code Quality & Architecture
2. Comprehensive Coverage
3. Test Coverage
4. Documentation
|
| Guideline | Status | Notes |
|---|---|---|
| TypeScript + ESM | ✅ | All new code uses ES modules |
| 2-space indent, single quotes | ✅ | Formatting consistent |
Tests in test/ mirroring src/ |
✅ | E2E tests properly placed |
| Avoid over-engineering | authTokenAlsoSetsApiKey feels over-engineered for one provider |
🎯 Recommendations Summary
Must Fix Before Merge (Blockers)
- Resolve Ollama auth strategy: Clarify local vs cloud usage and remove hardcoded credentials from provider template
- Add
requiresEmptyApiKeyvalidation: Prevent user confusion for Vercel provider
Should Fix (Non-blockers but important)
- Extract magic number
maxVisibleto named constant - Use realistic Ollama model examples in placeholders
- Add tests for new provider template flags
Nice to Have (Future improvements)
- Extract color utilities to shared module
- Consider splitting large PRs into focused chunks
- Add viewport limiting tests
✨ Overall Assessment
Verdict: Approve with required changes ✅
This PR demonstrates strong technical execution and follows project conventions well. The main concerns are:
- The Ollama auth pattern needs clarification
- Vercel's empty API key requirement needs UX/validation support
Once these are addressed, this is a solid addition to cc-mirror's provider ecosystem.
Estimated rework: 30-60 minutes to address the two must-fix items.
💬 Questions for Discussion
- Should Ollama support cloud by default, or focus purely on local-first workflows?
- Is the
authTokenAlsoSetsApiKeypattern something we expect other providers to need? If not, consider a provider-specific workaround. - Would you like me to help draft tests for the new flags?
Great work on this comprehensive provider expansion! 🚀
PR Review: Add gateway providers and default dev-browser offSummaryThis PR successfully adds three new gateway providers (GatewayZ, Vercel AI Gateway, NanoGPT) with proper brand themes and changes the dev-browser skill default to opt-in. The implementation follows established patterns and includes comprehensive test coverage. ✅ StrengthsArchitecture & Code Quality
Feature Implementation
🔍 Observations & Suggestions1. Vercel Empty API Key Handling
|
Code Review - PR #31: Add gateway providers and default dev-browser offThank you for this comprehensive PR! This is a well-structured expansion of provider coverage. ✅ Strengths
🔍 Key ObservationsProvider Configuration (src/providers/index.ts:141-185)
Color Schemes: All three providers have distinct, professional themes with provider-specific thinking verbs Wrapper Script (src/core/wrapper.ts:256-276): Team mode logic looks correct but could use a comment explaining precedence when both teamMode and teamModifier are set 🐛 Potential Issues
🔒 Security: No concerns identified⚡ Performance: No concerns identified🧪 Test CoverageCovered: Provider creation, ASCII art, themes 🎯 VerdictAPPROVE with minor recommendations Production-ready code with excellent structure and test coverage. Suggested improvements are minor and can be addressed in follow-up PRs. Recommendations Priority:
Great work maintaining quality and consistency! 🚀 |
Code Review: PR #31 - Add Gateway Providers and Default dev-browser OffI've reviewed this PR comprehensively. Overall, this is a well-structured and thorough implementation that significantly expands provider coverage while maintaining code quality. Here's my detailed feedback: ✅ Strengths1. Excellent Code Consistency
2. Strong Provider ConfigurationThe provider definitions in
3. Comprehensive Test Coverage
4. Documentation Updates
5. Clean dev-browser Default ChangeThe implementation at const shouldInstallSkills = (_providerKey: string) => false;This correctly defaults all providers to opt-out, which aligns with the stated goal of reducing friction. 🔍 Areas for Improvement1. Code Duplication in Brand Files (Medium Priority)The four new brand files (
Recommendation: Extract these utilities to a shared module like // src/brands/themeUtils.ts
export type Rgb = { r: number; g: number; b: number };
export const clamp = (value: number) => Math.max(0, Math.min(255, Math.round(value)));
export const hexToRgb = (hex: string): Rgb => { /* ... */ };
export const rgb = (hex: string) => { /* ... */ };
export const mix = (hexA: string, hexB: string, weight: number) => { /* ... */ };
export const lighten = (hex: string, weight: number) => mix(hex, '#ffffff', weight);Then import in each brand file: import { rgb, mix, lighten } from './themeUtils.js';This would:
2. Provider Base URL Verification (High Priority - Security/Correctness)The PR adds several new base URLs but doesn't include verification that these are correct:
Recommendation: Add comments in gatewayz: {
key: 'gatewayz',
label: 'GatewayZ',
description: 'GatewayZ AI Gateway',
// Docs: https://docs.gatewayz.ai/api/anthropic
baseUrl: 'https://api.gatewayz.ai',
// ...
},3. Missing Auth Token Validation (Medium Priority)For if (authMode === 'authToken') {
const trimmed = normalizeModelValue(apiKey);
if (trimmed) {
// Optional: Add basic format validation
if (trimmed.length < 10) {
console.warn(`Warning: Auth token seems too short for ${providerKey}`);
}
env.ANTHROPIC_AUTH_TOKEN = trimmed;
// ...
}
}4. Thinking Verbs Quality (Low Priority - Polish)Some thinking verbs feel slightly off-brand or unclear: Ollama:
GatewayZ:
5. DESIGN.md Outdated Reference (Low Priority)
-dev-browser is installed into `~/.cc-mirror/<variant>/config/skills/dev-browser` by default for Z.ai and MiniMax variants (opt out with `--no-skill-install`).
+dev-browser is opt-in for all variants via TUI or `--skill-install` flag.6. Test Coverage Gap (Medium Priority)While the PR updates tests, I don't see explicit tests for:
Recommendation: Add unit tests in describe('Provider-specific auth handling', () => {
it('should set empty API key for Vercel', () => {
const env = buildEnv({
providerKey: 'vercel',
apiKey: 'test-token',
baseUrl: 'https://ai-gateway.vercel.sh',
});
expect(env.ANTHROPIC_API_KEY).toBe('');
expect(env.ANTHROPIC_AUTH_TOKEN).toBe('test-token');
});
it('should set both auth token and API key for Ollama', () => {
const env = buildEnv({
providerKey: 'ollama',
apiKey: 'ollama',
baseUrl: 'http://localhost:11434',
});
expect(env.ANTHROPIC_API_KEY).toBe('ollama');
expect(env.ANTHROPIC_AUTH_TOKEN).toBe('ollama');
});
});🛡️ Security Considerations1. API Key Exposure in Logs ✅Good: The wrapper scripts properly handle API keys from 2. Base URL Injection Risk
|
| Category | Rating | Notes |
|---|---|---|
| Code Quality | ⭐⭐⭐⭐☆ | Solid, but has duplication |
| Test Coverage | ⭐⭐⭐⭐☆ | Good, missing some edge cases |
| Documentation | ⭐⭐⭐⭐☆ | Well-updated, minor gaps |
| Security | ⭐⭐⭐⭐⭐ | No issues found |
| Maintainability | ⭐⭐⭐⭐☆ | Could improve with DRY refactor |
🚀 Recommendation
Approve with minor improvements. This PR is ready to merge with the following suggested follow-ups:
Pre-merge (recommended):
- Extract theme utilities to reduce duplication
- Update DESIGN.md to reflect new dev-browser default
- Verify provider base URLs are correct
Post-merge (can be separate PRs):
- Add unit tests for provider-specific auth handling
- Add security note to README
- Consider making thinking verbs configurable
Great work on this comprehensive provider expansion! The code is clean, well-tested, and aligns with the project's direction. 🎉
- Ollama: tan/brown #caad8d (brand accurate) + llama silhouette - OpenRouter: navy/indigo #284968 + #6467f2 (brand accurate) + routing diagram - Vercel: blue #0070F3 accent (brand accurate) + iconic triangle - Z.ai: stylized Z with orbital ring (gold colors unchanged) - MiniMax: pulse wave visualization (coral colors unchanged) - CCRouter: cloud with routing arrows - Mirror: symmetric reflection frame - GatewayZ: portal/gateway visual - NanoGPT: minimalist atom/nano design Also adds scripts/preview-splash.mjs for easy preview of all splash art. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code Review - PR #31: Add gateway providers and default dev-browser offOverviewThis PR successfully expands provider coverage with GatewayZ, Vercel AI Gateway, NanoGPT, and Ollama. The implementation follows established patterns and includes comprehensive test coverage. The changes are well-structured and consistent with the project's architecture. ✅ Strengths1. Excellent Code Organization
2. Auth Mode FlexibilityThe new // Ollama compatibility (src/providers/index.ts:138)
authTokenAlsoSetsApiKey: true, // Sets both ANTHROPIC_AUTH_TOKEN and ANTHROPIC_API_KEY
// Vercel AI Gateway (src/providers/index.ts:170)
requiresEmptyApiKey: true, // Forces empty ANTHROPIC_API_KEYThis handles gateway-specific auth requirements cleanly. 3. Brand-Accurate Theming
4. Test Coverage
🔍 Issues & Suggestions1. Color Code Mismatch in Tests
|
- Drop team mode code/docs/tests entirely - Add 'cc-mirror apply' to re-run tweakcc without reinstall - Bump defaults: tweakcc@3.4.0, claude-code@2.1.19 - Update docs/AGENTS and wrapper behavior
Pull Request Review - Fresh AnalysisThis PR adds four new gateway providers (GatewayZ, Vercel AI Gateway, NanoGPT, Ollama) and defaults the dev-browser skill to opt-in. I've conducted a fresh review focusing on areas not extensively covered in previous reviews. Summary AssessmentRecommendation: APPROVE ✅ This is a well-executed PR that follows project conventions consistently and significantly expands provider coverage. The implementation is production-ready with only minor documentation gaps. Key Findings✅ Strengths
🔍 Additional Observations1. Provider URL VerificationThe base URLs should be verified against official documentation:
Suggestion: Add inline comments with doc links in 2. Model Mapping InconsistencyNanoGPT uses Question: Does NanoGPT accept standard Claude model names without mapping? 3. Color Utility DuplicationThe Suggestion (non-blocking): Extract to 4. Dev-Browser DocumentationDESIGN.md:110 still references old dev-browser defaults. Should update to reflect opt-in behavior. Security & Performance
Test CoverageCovered:
Suggested Additions (non-blocking):
Pre-Merge ChecklistMust verify:
Nice to have (can be follow-up):
Final NotesThis PR represents significant progress on expanding provider support (#23). The code quality is high, tests are comprehensive, and the architecture is solid. The identified issues are minor and mostly relate to documentation/verification rather than code correctness. Great work! 🚀 |
- Remove npm install path; always install native Claude Code binary with SHA256 verification - Add shared native download cache under ~/.cc-mirror/.cache/claude-native - Update wrapper to block upstream claude install/update by default - Update CLI/TUI/tests/docs for native-only flow - Refresh provider splash art and NanoGPT theme
Pull Request ReviewThis PR significantly expands provider coverage and removes deprecated features. Here's my detailed feedback: ✅ StrengthsCode Quality
Features
Cleanup
|
1 participant
Description
This PR expands provider coverage and aligns defaults with the updated product direction:
Type of Change
Related Issues
What’s Included
Providers
gatewayz)https://api.gatewayz.aiauthTokenmode + requires model mappingvercel)https://ai-gateway.vercel.shauthTokenmode + requires model mappingANTHROPIC_API_KEY(token-only auth)nanogpt)https://nano-gpt.com/apiauthTokenmode (no model mapping requirement)UX / TUI
Defaults / Behavior
Docs / Tests
How Has This Been Tested?
npm test -- --test-name-pattern="E2E:"npm test(coverage) +tsc --noEmit+ lintChecklist
Notes / Follow-ups