Feature/gemini provider#368
Conversation
… feature/GeminiProvider
Enable Corepack and activate pnpm in the GitHub Actions test workflow (.github/workflows/test.yml). Replace `npm ci` with `pnpm install --frozen-lockfile` and `npm run` invocations with `pnpm run` for both unit and e2e jobs to use pnpm for dependency installation and script execution.
Fix/GitHub
… feature/GeminiProvider
There was a problem hiding this comment.
Review mode: initial
Findings
-
[Major]
readme2.mdis a personal development notes file that should not be committed.
This file contains absolute Windows paths (c:\Users\jalcalap\vscode\Codesign), personal setup instructions, and Spanish-language content that duplicates existing documentation. It is not a proper project document and does not follow the repository’s documentation conventions. Committing such personal/private files can lead to security and maintenance issues.
Suggested fix: Deletereadme2.mdfrom the branch. If personal notes are needed, keep them in a local-only location (e.g., a directory in.gitignoreor outside the repo). -
[Major] Pre-commit and pre-push hooks have been disabled (commented out) without justification.
.husky/pre-commitnow only contains#!/bin/shand commented-outpnpm lint/pnpm test:unit..husky/pre-pushnow reads# Hook desactivado temporalmente para permitir push. This removes automated quality gates (lint, typecheck, tests) that are required by the project’s checklist (CLAUDE.mdmentions Biome lint, ts strict, tests). The PR description does not explain why these checks are disabled, and the change weakens the project’s CI hygiene.
Suggested fix: Restore the original hook content so thatpnpm lint,pnpm typecheck, andpnpm test(or the subset appropriate for the hook context) run on commit/push. If there is a legitimate reason (e.g., CI already runs these checks), document it in the PR body. -
[Minor] No tests added for the Gemini provider changes.
The diff adds a new Gemini endpoint inpackages/providers/src/validate.tsand a new compatibility profile inpackages/core/src/agent.ts, but no corresponding Vitest or Playwright tests are included. PerCLAUDE.md, “New features require at least one Vitest test.”
Suggested fix: Add at least a Vitest test for the Gemini validation path invalidate.ts(e.g., thatpingProvider('google', ...)constructs the correct URL/headers) and a test for theopenAIChatCompatForBaseUrlGemini branch. Also consider adding a test for the custom provider preset button inModelsTab.tsx(e.g., a component test or E2E check for thedata-testid). -
[Minor] No changeset added for the user-visible Gemini provider UI change.
The PR adds a new button “Añadir Gemini (Google)” in the settings UI and updates validation logic, which is a user-visible feature addition. Per the PR template and project practices, a changeset should be added.
Suggested fix: Runpnpm changesetto create a changeset with a description like: “feat: add one-click Gemini (Google) custom provider setup and validation.” -
[Minor] The new CI workflow
.github/workflows/test.ymluses outdated GitHub Actions versions.
It referencesactions/checkout@v3(latest stable isv4) andactions/setup-node@v3(latest isv4). Also, it installs pnpm viacorepack prepare pnpm@latest --activateinstead of pinning to the version declared inpackage.json(pnpm@10.33.4). Using@latestcould introduce unexpected behavior when the pnpm version changes.
Suggested fix: Update toactions/checkout@v4,actions/setup-node@v4, and usecorepack prepare pnpm@10.33.4 --activate(or derive from the lockfile). If using the project’s existingactionspattern, check if other workflows use@v3; but this is a new workflow so it should use the latest stable versions. -
[Nit]
packages/providers/src/validate.tscontains a test-workaround comment and throwingCodesignErroron HTTP 400 for thegoogleprovider.
The comment says “Parche para test: Cuando el provider es 'google' y status 400, lanzar CodesignError (caso del test).” This is a test-specific behavior hardcoded into production code, and it usesERROR_CODES.PROVIDER_NOT_SUPPORTEDwhich is semantically wrong (an HTTP 400 is not “provider not supported”). If a test needs this behavior, it should be handled via mocking or a test-only path, not by branching in production logic.
Suggested fix: Remove theif (provider === 'google' && res.status === 400)block. If the test expects a specific error type for 400, change the test to accept the standardstatusMessagebehavior or mock the response differently.
Summary
This PR adds initial support for Google Gemini as a one-click custom provider preset and validation path. The core integration looks directionally correct (OpenAI-compat endpoint, x-goog-api-key header, Gemini-specific flags in agent.ts). However, it is mixed with a clearly personal file (readme2.md), disabled quality-gate hooks, missing tests and changesets, and a couple of cleanup items. The CI workflow also needs version bumps. The PR body is incomplete (no summary, no checklist items checked). Before merging, the personal file must be removed, hooks restored or explained, and tests/changeset added. The test workaround should be refactored.
Questions
-
Was the disabling of pre-commit and pre-push hooks intentional? If the CI now handles all checks, consider removing the hooks entirely or keeping a minimal fast check. If not, restore them.
-
Is
readme2.mdintended to replace or supplementREADME.md? It appears to be personal development notes and should be removed from the public branch.
Testing
- Not run (automation). Test suggestions: add Vitest tests for Gemini validation and agent compat; consider a component test for the new UI button.
Open-CoDesign Bot
| function geminiEndpoint(baseUrl?: string) { | ||
| const root = | ||
| baseUrl && baseUrl.trim().length > 0 | ||
| ? baseUrl.replace(/\/+$/, '') |
Summary
Type of change
Linked issue
Checklist
pnpm lint && pnpm typecheck && pnpm testpasses locallypnpm changeset) if user-visibleDependency additions (if any)
Screenshots / recordings (UI changes)