Skip to content

feat(desktop): proxy presets + connection test + real model list#23

Merged
hqhq1025 merged 10 commits intomainfrom
wt/proxy-presets
Apr 18, 2026
Merged

feat(desktop): proxy presets + connection test + real model list#23
hqhq1025 merged 10 commits intomainfrom
wt/proxy-presets

Conversation

@hqhq1025
Copy link
Copy Markdown
Collaborator

Summary

  • 8 built-in proxy presets — official-openai, official-anthropic, official-google, duckcoding, openrouter, siliconflow, one-api, custom — in `packages/shared/src/proxy-presets.ts` with `PROXY_PRESET_SCHEMA_VERSION=1`
  • Test connection button — calls `connection:v1:test` IPC, hits `/models` endpoint without consuming tokens, shows green check or red hint
  • Dynamic model list — `models:v1:list` IPC with 5-minute per-provider+baseUrl cache
  • Error hint messages — 401 → "API key invalid or unauthorized", 404 → "Try /v1 suffix", ECONNREFUSED → "Check domain/port/network"
  • i18n keys added to both `en.json` and `zh-CN.json` (all ASCII quotes)

Compatibility / upgradeability / no-bloat / elegance

  • Compatibility: no schema changes, new IPC channels are additive
  • Upgradeability: PROXY_PRESET_SCHEMA_VERSION=1 allows future migration
  • No-bloat: no new dependencies; fetch is built-in Node 18+
  • Elegance: presets defined once in shared, consumed by UI and tests

Test plan

  • Pick DuckCoding preset — baseUrl auto-fills as https://api.duckcoding.ai/v1, Advanced section opens
  • Enter a real API key + DuckCoding baseUrl, click Test — green "Connected"
  • Enter wrong baseUrl (without /v1), click Test — red "Base URL path wrong. Try /v1 suffix"
  • Enter wrong API key, click Test — red "API key invalid or unauthorized"
  • Select Custom preset — baseUrl field stays empty for user input
  • All 108 workspace tests pass (pnpm test)
  • Typecheck clean (pnpm -r typecheck)
  • Lint clean (pnpm lint)

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Blocker] models:v1:list silently swallows failures and returns an empty model list, which violates the project rule "errors are user-visible or thrown" and hides auth/network/config bugs as a false "no models" state. Evidence: apps/desktop/src/main/connection-ipc.ts:278, apps/desktop/src/main/connection-ipc.ts:296, apps/desktop/src/main/connection-ipc.ts:299, apps/desktop/src/main/connection-ipc.ts:305.
    Suggested fix:

    // Return a typed error (or throw) instead of silent [] fallback
    type ModelsListResponse =
      | { ok: true; models: string[] }
      | { ok: false; code: 'IPC_BAD_INPUT' | 'NETWORK' | 'HTTP' | 'PARSE'; message: string; hint: string };
    
    ipcMain.handle('models:v1:list', async (_e, raw: unknown): Promise<ModelsListResponse> => {
      const payload = parseModelsListPayload(raw); // let bad input throw
      const res = await fetch(ep.url, { method: 'GET', headers: { ...ep.headers, ...authHeaders } });
      if (!res.ok) {
        return { ok: false, code: 'HTTP', message: `HTTP ${res.status}`, hint: 'Model list request failed' };
      }
      const body = await res.json();
      return { ok: true, models: extractModelIds(body) };
    });
  • [Major] The new test file does not test production code; it re-implements helper logic inline, so regressions in connection-ipc.ts can pass CI while behavior in main process is broken. Evidence: apps/desktop/src/main/connection-ipc.test.ts:4, apps/desktop/src/main/connection-ipc.test.ts:20, apps/desktop/src/main/connection-ipc.test.ts:44.
    Suggested fix:

    // connection-ipc.helpers.ts
    export { classifyHttpError, getCachedModels, setCachedModels, _clearModelsCache };
    
    // connection-ipc.test.ts
    import {
      classifyHttpError,
      getCachedModels,
      setCachedModels,
      _clearModelsCache,
    } from './connection-ipc.helpers';
  • [Major] "Dynamic model list" is not wired to the renderer flow; the PR adds main/preload APIs but no call site in onboarding, so the feature is effectively not delivered from user path. Evidence: apps/desktop/src/preload/index.ts:119, and no renderer usage in modified onboarding path (apps/desktop/src/renderer/src/onboarding/PasteKey.tsx:174).
    Suggested fix:

    // After connection test success, fetch and surface models
    const modelsResult = await window.codesign.models.list({ provider, apiKey: trimmed, baseUrl: trimmedBaseUrl });
    if (modelsResult.models.length === 0) {
      setConnState({ kind: 'error', code: 'PARSE', hint: 'No models returned. Check endpoint and permissions.' });
      return;
    }

Summary

  • Review mode: initial. 3 issues found: 1 blocker and 2 major. No new dependency/license risk found in this diff; DCO sign-off is present in commit message.

Testing

  • Not run (automation)

open-codesign Bot

Comment thread apps/desktop/src/main/connection-ipc.ts Outdated
try {
payload = parseModelsListPayload(raw);
} catch {
return { models: [] };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

models:v1:list should not silently return { models: [] } on bad input. This masks real failures and violates the repo rule that errors must be user-visible or thrown. Please return a typed error (or throw CodesignError) with context instead of fallbacking to empty models.

import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';

// ---------------------------------------------------------------------------
// Unit tests for pure logic extracted from connection-ipc.ts
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test suite mirrors logic instead of importing production helpers, so regressions in connection-ipc.ts may not be caught. Please extract pure helpers into a shared module and test that module directly.

return () => window.clearTimeout(handle);
}, [trimmed, trimmedBaseUrl]);

async function handleConnectionTest() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The onboarding flow only calls connection.test here; models.list (added in preload/main) is not consumed in this user path. That leaves the dynamic model list feature effectively unimplemented from the UI path. Please wire window.codesign.models.list(...) after a successful connection test and handle empty/error states explicitly.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Blocker] New UI literals violate the token-only UI constraint (§8 / §5b Elegant), which is CI-enforced and will accumulate inconsistent sizing/typography. Evidence: apps/desktop/src/renderer/src/onboarding/PasteKey.tsx:234, apps/desktop/src/renderer/src/onboarding/PasteKey.tsx:244.
    Suggested fix:

    <select className="w-full h-[var(--size-control-md)] px-[var(--space-3)] rounded-[var(--radius-md)] text-[var(--font-size-body-sm)] ..." />
    <span className="text-[var(--font-size-caption)] leading-[var(--line-height-body)] ...">...</span>
  • [Major] The renderer flow still does not consume models:v1:list after connection succeeds, so the “real model list” path remains unimplemented in onboarding. Evidence: apps/desktop/src/renderer/src/onboarding/PasteKey.tsx:174 and apps/desktop/src/renderer/src/onboarding/PasteKey.tsx:184 (only connection.test is called).
    Suggested fix:

    const conn = await window.codesign.connection.test({ provider, apiKey: trimmed, baseUrl: trimmedBaseUrl });
    if (!conn.ok) {
      setConnState({ kind: 'error', code: conn.code, hint: getErrorHint(conn.code) });
      return;
    }
    const list = await window.codesign.models.list({ provider, apiKey: trimmed, baseUrl: trimmedBaseUrl });
    if (!list.ok || list.models.length === 0) {
      setConnState({ kind: 'error', code: 'PARSE', hint: 'Could not load model list.' });
      return;
    }
    setConnState({ kind: 'ok' });
  • [Major] connection-ipc.test.ts still re-implements handler/cache logic inline instead of testing production helpers, so regressions in connection-ipc.ts can pass undetected. Evidence: apps/desktop/src/main/connection-ipc.test.ts:69.
    Suggested fix:

    // connection-ipc.helpers.ts
    export { classifyHttpError, extractModelIds, getCachedModels, setCachedModels, _clearModelsCache };
    
    // connection-ipc.test.ts
    import { classifyHttpError, getCachedModels, setCachedModels } from './connection-ipc.helpers';

Summary

  • Review mode: follow-up after new commits. 3 issues found (1 blocker, 2 major). Prior blocker about silent models:v1:list fallback appears fixed; dependency/license and direct-provider-SDK issues were not found in this diff.

Testing

  • Not run (automation)

open-codesign Bot

<select
value={selectedPresetId}
onChange={(e) => handlePresetChange(e.target.value)}
className="w-full h-[40px] px-3 rounded-[var(--radius-md)] bg-[var(--color-surface)] border border-[var(--color-border)] text-[13px] text-[var(--color-text-primary)] focus:outline-none focus:border-[var(--color-accent)] focus:shadow-[0_0_0_3px_var(--color-focus-ring)] transition-[box-shadow,border-color] duration-150 ease-[cubic-bezier(0.16,1,0.3,1)] appearance-none cursor-pointer"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Token-only UI constraint is violated here with hardcoded sizing/typography literals (h-[40px], text-[13px], etc.). Please switch to packages/ui token variables for control height, spacing, and font size.

apiKey: trimmed,
baseUrl: trimmedBaseUrl,
});
if (result.ok) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handleConnectionTest stops after connection.test; it never calls window.codesign.models.list(...). That leaves the dynamic model-list behavior unavailable in the onboarding user path.

};

// Minimal inline handler exercising the same logic as the real ipcMain handler.
async function handleModelsList(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suite still mirrors production logic inline (handleModelsList) instead of testing exported production helpers. Please extract pure helpers from connection-ipc.ts and import them so tests fail on real regressions.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Blocker] models:v1:list still has a silent fallback to an empty model list on unexpected response shape, which violates the "no silent fallbacks" constraint and can hide provider/API regressions. Evidence: apps/desktop/src/main/connection-ipc.ts:166, apps/desktop/src/main/connection-ipc.ts:180, apps/desktop/src/main/connection-ipc.ts:335.
    Suggested fix:

    function extractModelIds(body: unknown): string[] | null {
      if (body === null || typeof body !== 'object') return null;
      const data = (body as { data?: unknown }).data;
      if (Array.isArray(data)) {
        return data
          .filter((item) => typeof item === 'object' && item !== null && 'id' in item)
          .map((item) => String((item as { id: unknown }).id));
      }
      const models = (body as { models?: unknown }).models;
      if (Array.isArray(models)) {
        return models
          .filter((item) => typeof item === 'object' && item !== null && 'id' in item)
          .map((item) => String((item as { id: unknown }).id));
      }
      return null;
    }
    
    const models = extractModelIds(body);
    if (models === null) {
      return { ok: false, code: 'PARSE', message: 'Invalid models payload', hint: 'Provider returned unexpected schema' };
    }
  • [Major] The new models:v1:list IPC is still not used in the onboarding path, so the "real model list" behavior in this PR remains unexercised from UI flow. Evidence: apps/desktop/src/renderer/src/onboarding/PasteKey.tsx:179, apps/desktop/src/renderer/src/onboarding/PasteKey.tsx:184.
    Suggested fix:

    const conn = await window.codesign.connection.test({ provider, apiKey: trimmed, baseUrl: trimmedBaseUrl });
    if (!conn.ok) {
      setConnState({ kind: 'error', code: conn.code, hint: getErrorHint(conn.code) });
      return;
    }
    
    const list = await window.codesign.models.list({ provider, apiKey: trimmed, baseUrl: trimmedBaseUrl });
    if (!list.ok) {
      setConnState({ kind: 'error', code: 'PARSE', hint: list.hint });
      return;
    }
    // use list.models (e.g. preselect/store) before marking connected
    setConnState({ kind: 'ok' });
  • [Major] The new test suite mirrors handler logic inline instead of testing production code paths, so the parse-fallback regression above can pass tests unnoticed. Evidence: apps/desktop/src/main/connection-ipc.test.ts:69, apps/desktop/src/main/connection-ipc.test.ts:152.
    Suggested fix:

    // connection-ipc.ts
    export const __testables = { extractModelIds, classifyHttpError, _clearModelsCache };
    
    // connection-ipc.test.ts
    import { __testables } from './connection-ipc';
    // add a case: malformed body => PARSE (not ok:true, models:[])

Summary

  • Review mode: follow-up after new commits. 3 issues found (1 blocker, 2 major). Not found in this diff: direct provider SDK imports or new dependency/license changes.

Testing

  • Not run (automation)
  • Suggested additional coverage: malformed models response shape should return ok:false, code:'PARSE' and must not populate cache.

open-codesign Bot

Comment thread apps/desktop/src/main/connection-ipc.ts Outdated
};
}

const models = extractModelIds(body);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extractModelIds can return [] for malformed payloads, and the handler returns ok:true anyway. This is a silent fallback; please return ok:false, code:"PARSE" when schema is unexpected.

apiKey: trimmed,
baseUrl: trimmedBaseUrl,
});
if (result.ok) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After connection.test succeeds, this flow still marks connected without calling window.codesign.models.list(...), so the new model-list IPC path is not exercised in onboarding.

};

// Minimal inline handler exercising the same logic as the real ipcMain handler.
async function handleModelsList(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suite mirrors production logic in a local handleModelsList helper, so regressions in connection-ipc.ts can slip through. Please test exported production helpers and add a malformed-schema PARSE case.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Blocker] Invalid model entries are silently accepted as a successful empty model list — this hides provider/schema regressions and violates the no-silent-fallback rule. Evidence: apps/desktop/src/main/connection-ipc.ts:172, apps/desktop/src/main/connection-ipc.ts:179, apps/desktop/src/main/connection-ipc.ts:350.
    Suggested fix:

    function extractIds(items: unknown[]): string[] | null {
      const ids = items
        .map((item) =>
          item && typeof item === 'object' && typeof (item as { id?: unknown }).id === 'string'
            ? (item as { id: string }).id
            : null,
        )
        .filter((id): id is string => id !== null);
    
      // Treat non-empty arrays with zero valid ids as parse errors, not success.
      return items.length > 0 && ids.length === 0 ? null : ids;
    }
  • [Major] connection:v1:test converts payload-validation failures into NETWORK, which masks caller/input bugs and loses error semantics. Evidence: apps/desktop/src/main/connection-ipc.ts:250, apps/desktop/src/main/connection-ipc.ts:253.
    Suggested fix:

    // include IPC_BAD_INPUT in ConnectionTestError['code']
    if (err instanceof CodesignError && err.code === 'IPC_BAD_INPUT') {
      return { ok: false, code: 'IPC_BAD_INPUT', message: err.message, hint: 'Invalid connection:v1:test payload' };
    }
  • [Major] The onboarding flow still marks connection success without ever calling models:v1:list, so the “real model list” path is not exercised in UI. Evidence: apps/desktop/src/renderer/src/onboarding/PasteKey.tsx:179, apps/desktop/src/renderer/src/onboarding/PasteKey.tsx:185.
    Suggested fix:

    const conn = await window.codesign.connection.test({ provider, apiKey: trimmed, baseUrl: trimmedBaseUrl });
    if (!conn.ok) {
      setConnState({ kind: 'error', code: conn.code, hint: getErrorHint(conn.code) });
      return;
    }
    
    const models = await window.codesign.models.list({ provider, apiKey: trimmed, baseUrl: trimmedBaseUrl });
    if (!models.ok) {
      setConnState({ kind: 'error', code: 'PARSE', hint: models.hint });
      return;
    }
    setConnState({ kind: 'ok' });
  • [Major] The new unit test duplicates production logic instead of testing connection-ipc.ts, so production regressions can pass tests undetected. Evidence: apps/desktop/src/main/connection-ipc.test.ts:4, apps/desktop/src/main/connection-ipc.test.ts:69.
    Suggested fix:

    // connection-ipc.ts
    export const __testables = { extractModelIds, classifyHttpError, _clearModelsCache };
    
    // connection-ipc.test.ts
    import { __testables } from './connection-ipc';

Summary

  • Review mode: follow-up after new commits. 4 issues found (1 blocker, 3 major).
  • Constraint checks in this diff: no direct provider SDK imports added, no new prod dependency/license changes found.

Testing

  • Not run (automation)
  • Suggested: add production-level tests for models:v1:list malformed payloads ({ data: [{}] }, { models: [{}] }) to assert ok:false, code:'PARSE' and no cache write.

open-codesign Bot

Comment thread apps/desktop/src/main/connection-ipc.ts Outdated
: null,
)
.filter((id): id is string => id !== null);
return ids;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extractModelIds returns [] when an array exists but contains no valid id. That is still a silent fallback (ok:true) on malformed provider payloads. Please treat non-empty arrays with zero valid ids as PARSE error.

Comment thread apps/desktop/src/main/connection-ipc.ts Outdated
} catch (err) {
return {
ok: false,
code: 'NETWORK',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Payload validation failures from parseConnectionTestPayload are being recoded as NETWORK here. Please preserve IPC_BAD_INPUT so caller/input bugs are not masked as connectivity issues.

baseUrl: trimmedBaseUrl,
});
if (result.ok) {
setConnState({ kind: 'ok' });
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This marks connection ok immediately after connection.test, but onboarding still never calls models:v1:list. Please call window.codesign.models.list(...) before reporting success so the real model-list path is exercised.

};

// Minimal inline handler exercising the same logic as the real ipcMain handler.
async function handleModelsList(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test path uses an inline handleModelsList mirror instead of importing production helpers from connection-ipc.ts, so regressions in the real handler can still pass. Please test exported production helpers directly.

- Add PROXY_PRESETS array (8 presets: official-openai, official-anthropic,
  official-google, duckcoding, openrouter, siliconflow, one-api, custom)
  in packages/shared/src/proxy-presets.ts with PROXY_PRESET_SCHEMA_VERSION=1
- Register connection:v1:test and models:v1:list IPC channels via
  apps/desktop/src/main/connection-ipc.ts (no token consumed, 5-min model cache)
- Add preset dropdown + Test button to PasteKey onboarding step;
  selecting a preset auto-fills baseUrl, Test button shows green/red feedback
- Wire registerConnectionIpc() in main/index.ts app startup
- Expose window.codesign.connection.test and window.codesign.models.list
  in the preload bridge
- Add i18n keys: onboarding.preset.*, onboarding.connectionTest.*
  in both en.json and zh-CN.json (all ASCII quotes)
- All tests pass (108 total across workspace), typecheck clean, lint clean

Signed-off-by: hqhq1025 <1506751656@qq.com>
…back

Replace the `ModelsListResult` type with a typed `ModelsListResponse` union
({ ok: true; models } | { ok: false; code; message; hint }) so callers can
distinguish a real empty list from a network / HTTP / parse failure.

- connection-ipc.ts: four silent `return { models: [] }` paths each return
  an ok=false variant with code IPC_BAD_INPUT / NETWORK / HTTP / PARSE
- preload/index.ts: export `ModelsListResponse` type; update `models.list`
  return annotation to `Promise<ModelsListResponse>`
- i18n en+zh-CN: add `errors.modelListFailed` key for renderer toast use
- connection-ipc.test.ts: add 4 new cases covering the error-union paths
  (bad payload, HTTP 500, network throw, successful fetch)

Signed-off-by: hqhq1025 <1506751656@qq.com>
Replace all hardcoded size/spacing/font-size/leading/tracking literals
in PasteKey.tsx with CSS custom property references.
Add missing tokens to packages/ui: --text-2xs (10px),
--size-control-{sm,md,lg}, --tracking-label (0.08em).

Signed-off-by: hqhq1025 <1506751656@qq.com>
…ected

extractModelIds now returns string[] | null instead of string[]; null
signals an unrecognised response shape so the models:v1:list handler
returns ok=false / code=PARSE rather than silently swallowing the
anomaly as an empty model list.  Empty data/models arrays still return
ok=true with models=[].

Signed-off-by: hqhq1025 <1506751656@qq.com>
Replace .map().filter() in extractModelIds with a strict extractIds
helper that returns null on the first item lacking a string id field,
so malformed entries are never silently dropped.

Update test: mixed-data array now expects ok=false code=PARSE.
Add test: numeric id field also yields ok=false code=PARSE.

Signed-off-by: hqhq1025 <1506751656@qq.com>
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Model cache key ignores API key, causing cross-key stale model lists for 5 minutes and potentially leaking a previous account's model IDs in-session. Evidence apps/desktop/src/main/connection-ipc.ts:205.
    Suggested fix:

    import { createHash } from 'node:crypto';
    
    function getCacheKey(provider: string, baseUrl: string, apiKey: string): string {
      const keyHash = createHash('sha256').update(apiKey).digest('hex').slice(0, 16);
      return `${provider}::${baseUrl}::${keyHash}`;
    }
  • [Major] Missing preload API path returns silently on connection test, violating the no-silent-fallback constraint and giving no actionable error in UI. Evidence apps/desktop/src/renderer/src/onboarding/PasteKey.tsx:176.
    Suggested fix:

    if (!window.codesign?.connection) {
      setConnState({
        kind: 'error',
        code: 'NETWORK',
        hint: 'Renderer is not connected to the main process.',
      });
      return;
    }

Summary

  • Review mode: follow-up after new commits
  • 2 issues found in added/modified lines.
  • docs/VISION.md and docs/PRINCIPLES.md: Not found in repo/docs.

Testing

  • Not run (automation)

open-codesign Bot

Comment thread apps/desktop/src/main/connection-ipc.ts Outdated
const modelsCache = new Map<string, CacheEntry>();
const CACHE_TTL_MS = 5 * 60 * 1000;

function getCacheKey(provider: string, baseUrl: string): string {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] Cache key currently uses only provider::baseUrl, so a key switch within 5 minutes reuses stale models from the previous credential set. This can leak model IDs across accounts in one session and produce incorrect model choices.

Suggested fix:

import { createHash } from 'node:crypto';

function getCacheKey(provider: string, baseUrl: string, apiKey: string): string {
  const keyHash = createHash('sha256').update(apiKey).digest('hex').slice(0, 16);
  return `${provider}::${baseUrl}::${keyHash}`;
}


async function handleConnectionTest() {
if (!provider || trimmed.length === 0 || trimmedBaseUrl.length === 0) return;
if (!window.codesign?.connection) return;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Major] This returns with no UI signal when the preload bridge is unavailable, which is a silent fallback path. The UX should surface a concrete error state.

Suggested fix:

if (!window.codesign?.connection) {
  setConnState({
    kind: 'error',
    code: 'NETWORK',
    hint: 'Renderer is not connected to the main process.',
  });
  return;
}

- Include SHA-256(apiKey).slice(0,16) in models cache key so different
  API keys for the same provider+baseUrl never share cached model lists
- Replace silent no-op guard with NETWORK error state when
  window.codesign.connection is undefined in PasteKey handleConnectionTest
- Add 2 new cache tests (cross-key isolation) and new PasteKey.test.ts
  (6 tests covering the bridge guard and happy/error paths)

Signed-off-by: hqhq1025 <1506751656@qq.com>
Comment thread apps/desktop/src/main/connection-ipc.test.ts Fixed
Comment thread apps/desktop/src/main/connection-ipc.ts Dismissed
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Minor] Tests duplicate production logic instead of exercising the real implementation, so behavior drift in connection-ipc.ts can pass CI unnoticed. Evidence apps/desktop/src/main/connection-ipc.test.ts:6, apps/desktop/src/main/connection-ipc.test.ts:21, apps/desktop/src/main/connection-ipc.test.ts:75, apps/desktop/src/renderer/src/onboarding/PasteKey.test.ts:6.
    Suggested fix:
    vi.mock('./electron-runtime', () => ({
      ipcMain: { handle: vi.fn() },
    }));
    
    const { registerConnectionIpc } = await import('./connection-ipc');
    registerConnectionIpc();
    // capture registered handlers from ipcMain.handle and assert real handler behavior

Summary

  • Review mode: follow-up after new commits
  • 1 issue found in added/modified lines.
  • docs/VISION.md and docs/PRINCIPLES.md: Not found in repo/docs.

Testing

  • Not run (automation)

open-codesign Bot


// ---------------------------------------------------------------------------
// Unit tests for pure logic extracted from connection-ipc.ts
// These tests do NOT import connection-ipc directly because that file imports
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test file re-implements production logic inline instead of importing/exercising real handlers. That can let behavior drift in connection-ipc.ts pass tests. Consider mocking ./electron-runtime and asserting behavior through registered handlers.

…logic

Export extractIds, extractModelIds, getCacheKey, and classifyHttpError from
connection-ipc.ts. Delete the inline mirror copies from the test file and
import the real functions directly, using vi.mock('./electron-runtime') so
vitest can load the module without a live Electron process.

Also adds dedicated test suites for extractIds, extractModelIds, and
getCacheKey that were previously untested.

Signed-off-by: hqhq1025 <1506751656@qq.com>
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] Anthropic baseUrl normalization appends duplicate /v1 for common custom inputs, causing false 404/network failures in connection test and model listing. Evidence apps/desktop/src/main/connection-ipc.ts:105.
    Suggested fix:

    const normalized = baseUrl.replace(/\/+$/, '');
    if (provider === 'anthropic') {
      const url = normalized.endsWith('/v1/models')
        ? normalized
        : normalized.endsWith('/v1')
          ? `${normalized}/models`
          : `${normalized}/v1/models`;
      return { url, headers: {} };
    }
  • [Minor] New tests still reimplement models:v1:list behavior in a test-only helper, so handler regressions can pass even when IPC wiring or endpoint construction changes. Evidence apps/desktop/src/main/connection-ipc.test.ts:25.
    Suggested fix:

    vi.mock('./electron-runtime', () => ({ ipcMain: { handle: vi.fn() } }));
    const { registerConnectionIpc } = await import('./connection-ipc');
    registerConnectionIpc();
    // capture the real 'models:v1:list' handler from ipcMain.handle mocks and test it directly

Summary

  • Review mode: follow-up after new commits
  • 2 issues found in added/modified lines.
  • docs/VISION.md and docs/PRINCIPLES.md: Not found in repo/docs.

Testing

  • Not run (automation)

open-codesign Bot

Comment thread apps/desktop/src/main/connection-ipc.ts Outdated
// Anthropic baseUrl is typically https://api.anthropic.com (no /v1 suffix)
// We append /v1/models if baseUrl doesn't already end with /models
return {
url: baseUrl.endsWith('/v1/models') ? baseUrl : `${baseUrl}/v1/models`,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

baseUrl values that already end in /v1 become /v1/v1/models here. That breaks common Anthropic custom URL inputs. Consider normalizing trailing slashes and handling both /v1 and /v1/models explicitly.


import type { ModelsListResponse } from './connection-ipc';

async function handleModelsList(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helper duplicates much of production handler logic. Regressions in the real IPC registration/handler can still pass. Prefer registering registerConnectionIpc() and testing the captured real ipcMain.handle callback for models:v1:list.

…egments

Adds `normalizeBaseUrl(baseUrl, provider)` helper that strips or ensures
the /v1 suffix before path concatenation, so users passing
https://api.anthropic.com/v1 no longer get a 404 from /v1/v1/models.

Signed-off-by: hqhq1025 <1506751656@qq.com>
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] connection:v1:test misclassifies payload validation failures as NETWORK errors, which masks deterministic client-input bugs as transient connectivity failures and drives incorrect remediation UI. Evidence apps/desktop/src/main/connection-ipc.ts:274.
    Suggested fix:

    export interface ConnectionTestError {
      ok: false;
      code: 'IPC_BAD_INPUT' | '401' | '404' | 'ECONNREFUSED' | 'NETWORK' | 'PARSE';
      message: string;
      hint: string;
    }
    
    // ...
    } catch (err) {
      return {
        ok: false,
        code: 'IPC_BAD_INPUT',
        message: err instanceof Error ? err.message : 'Invalid payload',
        hint: 'Invalid connection:v1:test payload',
      };
    }
  • [Minor] connection-ipc.test.ts still validates a test-only reimplementation (handleModelsList) instead of the registered IPC handler, so endpoint-construction/handler-wiring regressions can pass undetected. Evidence apps/desktop/src/main/connection-ipc.test.ts:26.
    Suggested fix:

    import { ipcMain } from './electron-runtime';
    import { registerConnectionIpc } from './connection-ipc';
    
    registerConnectionIpc();
    const modelsHandler = vi
      .mocked(ipcMain.handle)
      .mock.calls.find(([channel]) => channel === 'models:v1:list')?.[1];
    
    expect(modelsHandler).toBeTypeOf('function');
    const result = await modelsHandler?.({} as never, {
      provider: 'openai',
      apiKey: 'sk-test',
      baseUrl: 'https://api.openai.com/v1',
    });

Summary

  • Review mode: follow-up after new commits
  • 2 issues found in added/modified lines of the latest diff.
  • docs/VISION.md and docs/PRINCIPLES.md: Not found in repo/docs.

Testing

  • Not run (automation)

open-codesign Bot

Comment thread apps/desktop/src/main/connection-ipc.ts Outdated
} catch (err) {
return {
ok: false,
code: 'NETWORK',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseConnectionTestPayload failures are returned as code: 'NETWORK' here. That conflates invalid IPC input with transport failures and produces the wrong remediation path. Consider returning IPC_BAD_INPUT (and extending ConnectionTestError['code']) to preserve error semantics.


import type { ModelsListResponse } from './connection-ipc';

async function handleModelsList(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helper reimplements the models:v1:list logic instead of exercising the registered ipcMain.handle callback. Wiring or endpoint-construction regressions in production can pass this test. Consider capturing the real handler after registerConnectionIpc() and testing that function directly.

…k errors

Validation failures in connection:v1:test now return code IPC_BAD_INPUT
instead of NETWORK, preventing misleading "Cannot reach base URL" messages
when the caller passes a malformed payload. Adds ConnectionTestResponse
type alias and four new tests covering the bad-payload and happy paths.

Signed-off-by: hqhq1025 <1506751656@qq.com>
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] IPC response contract drift between main and preload for connection:v1:test — main now returns code: 'IPC_BAD_INPUT', but preload’s ConnectionTestError omits it. This weakens type safety across the bridge and forces renderer code into an unintended default error path for deterministic payload issues. Evidence apps/desktop/src/preload/index.ts:28, apps/desktop/src/main/connection-ipc.ts:276.
    Suggested fix:

    // apps/desktop/src/preload/index.ts
    export interface ConnectionTestError {
      ok: false;
      code: 'IPC_BAD_INPUT' | '401' | '404' | 'ECONNREFUSED' | 'NETWORK' | 'PARSE';
      message: string;
      hint: string;
    }
    
    // apps/desktop/src/renderer/src/onboarding/PasteKey.tsx
    function getErrorHint(code: ConnectionTestError['code']): string {
      switch (code) {
        case 'IPC_BAD_INPUT':
          return 'Invalid connection test payload.';
        // existing cases...
      }
    }
  • [Minor] New onboarding test reimplements production logic instead of exercising production code paths, so behavior can drift while tests still pass. PasteKey.test.ts inlines a copy of handleConnectionTest rather than testing PasteKey behavior through the actual bridge contract. Evidence apps/desktop/src/renderer/src/onboarding/PasteKey.test.ts:24.
    Suggested fix:

    import { render, screen } from '@testing-library/react';
    import userEvent from '@testing-library/user-event';
    import { PasteKey } from './PasteKey';
    
    vi.stubGlobal('codesign', {
      connection: { test: vi.fn().mockResolvedValue({ ok: false, code: 'NETWORK', hint: 'x', message: 'x' }) },
      detectProvider: vi.fn().mockResolvedValue('openai'),
      onboarding: { validateKey: vi.fn().mockResolvedValue({ ok: true, modelCount: 1 }) },
    });
    
    // drive UI and assert rendered connection status from real component logic.

Summary

  • Review mode: follow-up after new commits
  • 2 issues found in added/modified lines of the latest PR diff.
  • docs/VISION.md and docs/PRINCIPLES.md: Not found in repo/docs.

Testing

  • Not run (automation)

open-codesign Bot

Comment thread apps/desktop/src/preload/index.ts Outdated
}
export interface ConnectionTestError {
ok: false;
code: '401' | '404' | 'ECONNREFUSED' | 'NETWORK' | 'PARSE';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in main now returns , but this bridge type omits that code. Please add here to keep the IPC contract type-safe end-to-end.

trimmedBaseUrl: string,
connectionBridge: { test: (payload: unknown) => Promise<{ ok: boolean }> } | undefined,
setConnState: (s: ConnectionState) => void,
): Promise<void> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test copies component logic instead of exercising itself, so production/test behavior can diverge. Consider using RTL with a mocked bridge and asserting rendered connection states from the real component path.

Replace local ConnectionTestError/ConnectionTestResult/ModelsListResponse
type definitions in preload/index.ts with `import type` re-exports from
connection-ipc.ts so the bridge has a single source of truth. The missing
`IPC_BAD_INPUT` code is now included automatically.

Add `IPC_BAD_INPUT` case to getErrorHint switch in PasteKey.tsx so the
renderer can narrow the full union and show a helpful toast hint.

Signed-off-by: hqhq1025 <1506751656@qq.com>
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • None.

Summary

  • Review mode: follow-up after new commits
  • No blocking/major/minor issues found in added/modified lines of the latest PR diff.
  • docs/VISION.md and docs/PRINCIPLES.md: Not found in repo/docs.
  • Residual risk: onboarding changes are covered mainly by logic-level unit tests; no component-level test currently exercises the real PasteKey UI + bridge path end-to-end.

Testing

  • Not run (automation)

open-codesign Bot

@hqhq1025 hqhq1025 merged commit 66f640b into main Apr 18, 2026
5 of 6 checks passed
@hqhq1025 hqhq1025 deleted the wt/proxy-presets branch April 18, 2026 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants