[Lxr-max]

Summary

Connection tests (connection:v1:test-active / connection:v1:test-provider) now validate the same effective provider contract that generate uses, instead of treating a GET /models 200 as Connection OK.

Fixes #216 (slice of epic #214). Does not implement sibling #213.

Shared contract

resolveEffectiveInvokeContract() in apps/desktop/src/main/effective-provider-contract.ts is the single resolver for:

• test-active: same resolveActiveModel() snap-to-config as generate (stale renderer provider is ignored when cfg.activeProvider is set)
• test-provider: named-provider resolution (resolveProviderModel) with no snap to the active slot
• Auth: resolveCredentialForProvider() — ChatGPT Codex uses getValidAccessToken(), other providers share the same API-key / env / keyless rules as generate
• Headers: Authorization: Bearer vs x-api-key from authHeaderNameForProvider()
• baseUrl: canonicalInvokeBaseUrl() (trailing slash + /v1 for OpenAI-compatible wires)
• Wire: invokeUrlForWire() — Codex Responses vs Anthropic messages vs OpenAI chat completions
• Keyless: empty Bearer is still sent when allowKeyless is true

Generate and memory IPC now pass that canonical baseUrl into complete(), and complete() canonicalizes once more so runtime cannot silently diverge.

/models vs real wire

modelsProbeRelationForWire() in packages/shared:

• optional-discovery: GET /models is not the generate contract. Generate never calls it. A 200 on /models is not Connection OK.
• unavailable: ChatGPT Codex — no /models probe; OAuth token resolution is the test.

Probe both discovery and inference when discovery is optional:

/models	invoke	result
fail	pass	ok: true, compatibility: 'degraded', invokeParity: 'degraded-discovery'
pass	fail	ok: false, compatibility: 'diverges', code: 'INVOKE_DIVERGED'
pass	pass	ok: true, compatibility: 'compatible'
fail	fail	ok: false with invoke error

Renderer toasts distinguish success / degraded discovery / reachable but diverges from real invoke / hard failure.

Tests

• Active snap-to-config vs named-provider no-snap
• Codex OAuth uses refreshed token, not tokenStore.read()
• Keyless proxy empty Bearer
• OpenAI-compatible gateway uses /chat/completions, not /responses
• Regression: /models 200 + invoke 404 → diverges (test must not pass when runtime would fail)
• Regression: /models 404 + invoke 200 → degraded pass (test must not fail when runtime would succeed)
• Responses wire must not false-positive via /chat/completions

Principles 5b

• Compatible: existing IPC channels; extra structured fields are additive
• Upgradeable: schemaVersion unchanged; new result fields are optional for older renderers
• No bloat: no new deps; shared URL helpers instead of a second HTTP client
• Elegant: one contract object consumed by test-active, test-provider, and generate

Constraints

• BYOK / local-first unchanged; credentials stay in local config
• No provider SDKs; probes stay as fetch against the same URLs complete() uses

[Lxr-max]

Hi, a gentle ping on this one when you have time.

Status as of today:

• A rebase onto current main (5bc24f0) has no conflicts. On that merge, pnpm typecheck, biome check, and the shared/providers/desktop connection tests pass locally.
• The only red check is pr-review. The DeepSeek review bot returned an empty message on all 3 attempts (DeepSeek API returned no message content), so it was not a code failure. I think fix(ci): recover incomplete DeepSeek review output #431 has since fixed that on main, so a rebase should clear it.

This builds on #220/#223. It adds one shared invoke-contract resolver for test-active, test-provider and generate, and the /models-vs-invoke matrix from #216.

If the size (~1.5k lines incl. tests) is a blocker, I can split it into:

1. canonicalInvokeBaseUrl / inferenceEndpointUrl helpers in packages/shared + using them in complete(), generate and memory IPC
2. effective-provider-contract.ts (active snap vs named provider, auth/header/keyless) + tests
3. connection-ipc dual probe (degraded-discovery / diverges) + renderer toast + i18n

It overlaps with #422 in connection-ipc.ts. I'm happy to land #422 first and rebase this on top so the connection test reads modelDiscoveryMode instead of its own /models relation.

[Lxr-max]

Rebased onto the updated #422 head (2cdaa41) rather than bare main, so this PR carries the formalized modelDiscoveryMode work plus invoke-contract parity.

Conflict resolution

• connection-ipc.ts: kept feat(desktop): align connection tests with the generate invoke contract #421 testInvokeContract / INVOKE_DIVERGED / invokeParity path and absorbed feat(providers): formalize model discovery modes #422 listing modes (Codex getCodexModelOptions, LiteLLM hintKey, discoveryMode on list/test payloads).
• provider-settings.ts: kept feat(desktop): align connection tests with the generate invoke contract #421 providerModelFromEntry while retaining discovery-mode fields from feat(providers): formalize model discovery modes #422.
• primitives.tsx: kept ConnectionDiagnosticPanel from feat(providers): formalize model discovery modes #422 and switched connection-test toasts to feat(desktop): align connection tests with the generate invoke contract #421 connectionTestToastKind (ok / degraded / diverges / fail).
• i18n: kept feat(desktop): align connection tests with the generate invoke contract #421 connectionDegraded / connectionDiverges copy (dropped the older connectionOkDegraded wording from the other side of the conflict).
• packages/shared connection-test.ts: feat(desktop): align connection tests with the generate invoke contract #421 invoke-contract types, with optional hintKey from the feat(providers): formalize model discovery modes #422 LiteLLM path.

Head is 0e8c282. mergeable is clean again. Local pnpm install/tests not re-run in this pass (no existing node_modules in the worktree).

[github-actions]

Findings

• [Blocker] apps/desktop/src/main/connection-ipc.ts contains several corrupted/unterminated string literals in classifyHttpError and classifyNetworkError — the added lines drop closing quotes/backticks and the ${message} interpolation, which is a syntax break, not just mojibake. New lines in the diff include hint: 'API key ??????????? }; (no closing '), hint: 'baseUrl ????????penAI ?????????????/v1 ????????https://your-host/v1??, (no closing '), hint: \????????${CONNECTION_FETCH_TIMEOUT_MS / 1000}s???????baseUrl ??????????,(no closing backtick), andhint: `????????{message}...(interpolation lost). BecauseclassifyHttpError/classifyNetworkErrorfeedinvokeFailureFields, this module won't parse, so pnpm typecheck, pnpm build, and every connection-ipc test fail. The surrounding i18n JSON in the same PR renders UTF‑8 correctly, so this is file-level encoding corruption in connection-ipc.ts, not a display artifact. Suggested fix / next action: re-save apps/desktop/src/main/connection-ipc.ts` as UTF‑8 and restore the original literals, e.g.

  if (status === 401 || status === 403) {
    return { code: '401', hint: 'API key 错误或权限不足' };
  }
  if (status === 404) {
    return { code: '404', hint: 'baseUrl 路径错误。OpenAI 兼容代理通常需要 /v1 后缀（试试 https://your-host/v1）' };
  }
  // network:
  hint: `请求超时（>${CONNECTION_FETCH_TIMEOUT_MS / 1000}s），检查 baseUrl 与网络可达性`,
  hint: `网络错误：${message}。查看日志：~/Library/Logs/open-codesign/main.log`,

  Also restore the mangled // comments (the ?? sequences) while you're in the file.

• [Major] modelsProbeRelationForProvider in packages/shared/src/base-url.ts narrows modelDiscoveryMode to 'models' | 'static-hint' | 'manual' | undefined, but the new value 'infer-only' was added to ProviderModelDiscoveryMode. buildEffectiveProviderContract in apps/desktop/src/main/effective-provider-contract.ts passes resolved.modelDiscoveryMode (the full ProviderModelDiscoveryMode union), so this is a TS2345/TS2322 assignability error and pnpm typecheck fails. Semantically infer-only is already caught via supportsModelsEndpoint === false, so only the type is wrong.
  Suggested fix / next action: widen the param to accept all four modes (or reuse ProviderModelDiscoveryMode) and, for clarity, add an explicit if (caps?.modelDiscoveryMode === 'infer-only') return 'unavailable';.

• [Minor] The connection test now always fires a real inference POST (probeInferenceEndpoint) even when GET /models succeeds, on every "Test connection" click. For per-token / rate-limited providers this is a billable, throttled request that previously only ran in the degrade path. Also, a 404 from the invoke probe (e.g. the configured model id does not exist) is reported as INVOKE_DIVERGED / "reachable but generate would fail", which steers users toward switching wire when the actual fix is the model id; probeInferenceEndpoint treats only 404 and 5xx as failures. apps/desktop/src/main/connection-ipc.ts probeInferenceEndpoint / classifyInvokeAndModelsProbes.
  Suggested fix / next action: confirm the extra billing/rate-limit cost is acceptable (or gate it), and distinguish a model-not-found 4xx from a genuine contract divergence so the toast/hint points at the model id instead of the wire.

• [Minor] Production connection:v1:test-active / test-provider handlers call testInvokeContract directly (handleConnectionV1TestActive / handleConnectionV1TestProvider), while the new behavioral tests all drive runProviderTest. The two paths must stay in lockstep (e.g. LiteLLM hint application), and the actual IPC wiring isn't directly exercised. apps/desktop/src/main/connection-ipc.ts.
  Suggested fix / next action: either route the handlers through runProviderTest or add a test that calls handleConnectionV1TestActive/handleConnectionV1TestProvider so the shipped path is covered; verify runProviderTest is still referenced from production code.

Questions

• Was the mojibake in connection-ipc.ts introduced by an editor/tool round-trip on non-UTF‑8? If so, please confirm the file is clean (a biome check / throwaway tsc on the file) before re-requesting review.

Summary

Review mode: initial

The design (single resolveEffectiveInvokeContract, /models treated as optional discovery, invoke-parity classification, discovery modes) is directionally sound and well covered by unit tests, and the changesets are present. However, the PR as pushed cannot build: connection-ipc.ts has syntax-breaking corrupted literals (Blocker), and the modelsProbeRelationForProvider signature rejection of infer-only will fail pnpm typecheck (Major). The remaining two items are verification/UX nits. The PR closes #216 (a slice of epic #214); the diff does touch the expected connection-test paths, so the Fixes #216 claim looks consistent — but it should not merge until the build-break issues are fixed and CI is green.

Testing

Not run (automation). The added Vitest coverage is broad (active snap vs named provider, Codex OAuth path, keyless, INVOKE_DIVERGED, degraded discovery), but it will not execute until the connection-ipc.ts syntax errors and the base-url.ts type error are fixed.

Open-CoDesign Bot

[github-actions]

Findings

• [Blocker] apps/desktop/src/main/connection-ipc.ts is still syntactically broken by the same UTF-8 corruption flagged in the prior review. The added lines include hint: 'API key ??????????? }; (no closing quote, :314), hint: 'baseUrl ... ??, (no closing quote, :318), and hint: `????????${CONNECTION_FETCH_TIMEOUT_MS / 1000}s???????baseUrl ??????????, (no closing backtick, :339). The same hunk also truncates TestEndpointResponse to | { ok: false; (:1409) and drops the ${message} interpolation in classifyNetworkError (:352). This makes the module fail to parse, so pnpm typecheck, pnpm build, and the connection-ipc tests cannot pass.
  Suggested fix / next action: restore apps/desktop/src/main/connection-ipc.ts from a clean UTF-8 source, fix the TestEndpointResponse union to | { ok: false; error: string; message: string; hintKey?: string }, and run pnpm lint && pnpm typecheck before re-requesting review.

• [Major] modelsProbeRelationForProvider still excludes the new infer-only value from its parameter type. packages/shared/src/base-url.ts:180 accepts modelDiscoveryMode?: 'models' | 'static-hint' | 'manual' | undefined, but apps/desktop/src/main/effective-provider-contract.ts:83 passes resolved.modelDiscoveryMode typed as the full ProviderModelDiscoveryMode union. This is a TS2345 assignability error.
  Suggested fix / next action: type the parameter with ProviderModelDiscoveryMode (or add 'infer-only') and add an explicit if (caps?.modelDiscoveryMode === 'infer-only') return 'unavailable'; branch.

• [Major] 'INVOKE_DIVERGED' is added to the connection-test error union but not to ErrorCode in packages/shared/src/diagnostics.ts:3. ProviderCard assigns res.code into a state typed ErrorCode at apps/desktop/src/renderer/src/components/settings/primitives.tsx:408, and the new test calls diagnose('INVOKE_DIVERGED', baseCtx) at packages/shared/src/diagnostics.test.ts:150. Both fail typecheck because the literal is not assignable to ErrorCode.
  Suggested fix / next action: add 'INVOKE_DIVERGED' to the ErrorCode union and keep the diagnose branch/test aligned.

• [Minor] connectionErrorFromUnknown picks the ChatGPT-login hint by substring matching 'chatgpt' or 'codex' in any error message (apps/desktop/src/main/effective-provider-contract.ts:150-158). A regular provider such as codex-deepseek with a missing API key will therefore show the ChatGPT subscription hint instead of the API-key/import hint.
  Suggested fix / next action: branch on ERROR_CODES.CODEX_TOKEN_NOT_LOGGED_IN (or isCodexOAuthProvider(providerId, wire)) rather than message text.

Questions

• Was connection-ipc.ts round-tripped through a non-UTF-8 editor/tool? If so, can you re-save the original file and confirm pnpm typecheck locally before the next CI run?

Summary

Review mode: follow-up after new commits. The previous Blocker (corrupted connection-ipc.ts literals) and Major (modelsProbeRelationForProvider type) remain unresolved, and the new ErrorCode mismatch is another typecheck blocker. The design is directionally sound and the added tests cover the intended parity cases, but the PR cannot build in its current state. I could not independently re-fetch linked issue #216 in this run; the diff still targets the connection-test/invoke-contract paths.

Testing

Not run (automation). The added Vitest coverage is broad, but it cannot execute until the parse and type errors above are fixed. Re-run pnpm lint && pnpm typecheck && pnpm test and include the result in the next update.

Open-CoDesign Bot