fix(providers): handle optional client certificate requests

[yixinforu]

Summary

Electron 43 aborts requests with ERR_SSL_CLIENT_AUTH_CERT_NEEDED when a server requests an optional client certificate, preventing model discovery and connection tests from reaching HTTP.

Retry HTTPS metadata GET/OPTIONS requests once without a client certificate when this exact error occurs. Preserve proxy routing, server certificate and hostname verification, cancellation, and redirect restrictions.

Keep Electron 43 and existing platform compatibility, including macOS 12. Agent turns and plugin requests are unaffected.

Validation

• 57 related regression and user-path tests passed.
• Real Electron 43.6.0 E2E reproduced the original failure and verified recovery, authenticated proxy routing, cancellation, and redirect restrictions.
• Build, Desktop typecheck, lint, and architecture checks passed.

Fixes #1676

[muzimu217]

Verified against upstream/main + PR head in an isolated worktree. The approach is sound and the edge coverage is unusually thorough.

What it does: metadata probes in model-discovery that die with net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED are retried once through the existing authenticated SOCKS relay over raw Node TLS with no client identity — which is exactly what optional-certificate endpoints need (Chromium's stack aborts before HTTP even when the server would accept an empty cert reply, matching what #1676 reported).

Checked:

• TLS verification survives the tunnel: SNI + hostname check, bundled + OS roots, rejectUnauthorized (provider-https-metadata.ts), with a dedicated test for hostname validation through a supplied tunnel.
• The retry trigger is narrow: exact error string, HTTPS only, GET/OPTIONS only, throwIfAborted between attempts; HTTP responses and unrelated TLS errors never fall back (both tested).
• Redirects stay manual inside the caller's origin-guarded loop, so no credentials or URLs drift; the raw transport adds no new URL surface (same host, verified by TLS).
• Cancellation races are handled on connect, handshake and body phases; sockets are torn down in finally.

Ran the new suites locally: 15/15 pass (provider-https-metadata.test.mjs, provider-metadata-fetch.test.mjs), including required-cert-still-fails and retry-once semantics.

Two non-blocking notes:

1. The trigger matches error.message === "net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED" literally — fine for Electron 43, but an upstream message change would silently disable the fallback. It fails safe (current behavior), just invisibly; a comment or named constant would help whoever upgrades Electron next.
2. fetchHttpsMetadata buffers the whole body with no size cap — bounded in practice by the configured provider host, but a cap would make it bulletproof.

This closes the gap from #1676 (model-discovery probes on the global fetch with no select-client-certificate handling). The approach differs from an Electron-side handler but is tighter: it covers exactly the replayable metadata surface without touching agent traffic.

[vastsa]

Thanks for the focused fix and local TLS coverage. I verified the reported Electron 43 failure and reviewed the retry boundary: it is limited to HTTPS GET/OPTIONS metadata probes after the exact optional-client-certificate error; cancellation, redirects, proxy routing, hostname/certificate checks, and agent/plugin requests remain protected. On the latest-main candidate, the provider regression tests passed (15/15), the isolated Electron 43 TLS/proxy E2E passed, and Desktop typecheck passed.

I updated the PR branch to current main (head ec3ad42). I cannot merge yet: CI, Docs check, and PR-base for that head are all action_required, so GitHub has no remote check results. Please have the fork workflows approved and let them finish; I can merge once those required checks are green. Thanks for the thorough contribution.

[vastsa]

Follow-up after #1735 landed: I refreshed this PR again to latest main (e219a3e; current head 0495f59) and confirmed the PR tree matches the locally tested candidate. On that candidate the 15 provider regression tests, Desktop typecheck, and isolated Electron 43 TLS/proxy E2E all passed. GitHub still reports CI, Docs check, and PR-base as action_required; the connected integration cannot approve those runs (the approval request returns 404), so I’m leaving the PR open until the required remote checks run green.

[muzimu217]

Heads-up: this PR is now CONFLICTING against main (head 0495f59, 9 commits behind). I reproduced it locally and it is one file, one hunk — docs/spec/06-delivery/04-e2e-test-plan.md, nothing in the code.

Cause: main added the plugin-session-follow E2E entry right where your ### E2E-PROVIDER-optional-client-certificate section sits, so both sides append after the same - **Status**: … line.

Resolution that keeps both (verified locally — docs/check-docs.mjs passes, 579 pages, zero conflict markers):

1. Keep main's block verbatim, including its following apps/desktop/test/sidebar-project-runs.test.mjs covers count … paragraph — that paragraph belongs to the sidebar entry above it, not to session-follow, so it must stay where upstream put it.
2. Insert your ### E2E-PROVIDER-optional-client-certificate section (the whole block, from the heading through - **Status:** Automated fixture; …) after the plugin-session-follow entry ends at … remains a manual validation run.

That is the whole fix; no other file conflicts, and git merge-tree confirms no code overlap. After rebasing, your 15 provider tests and the TLS/proxy E2E should be unaffected — I re-ran them on the pre-rebase head and they pass.

[muzimu217]

I reproduced this PR's current state locally (merged upstream/main into head 0495f59) and ran the gates. Two things block it, and neither is the code:

1. Merge conflict — one file, one hunk. docs/spec/06-delivery/04-e2e-test-plan.md. main added the plugin-session-follow E2E entry at the same anchor. Resolution that works (verified): keep main's block verbatim — including the apps/desktop/test/sidebar-project-runs.test.mjs covers count … paragraph, which belongs to the sidebar entry above it — then insert your whole ### E2E-PROVIDER-optional-client-certificate section after the session-follow entry ends at … remains a manual validation run. No other file conflicts.

2. Your ADR fails the docs gate. node scripts/check-docs.mjs reports exactly one problem:

adr/provider-metadata-client-certificate-retry.md: H1 must start with "ADR", found
"Provider metadata recovery from optional client certificate requests"

Current heading is # Provider metadata recovery from optional client certificate requests. Every ADR in this repo uses # ADR NNNN: <title> (and docs/adr/README.md indexes them). Rename the file to the next free number (docs/adr/0332-… looks free) and make the H1 # ADR 0332: Provider metadata recovery from optional client certificate requests, then add the index row in docs/adr/README.md.

After those two, I ran the gates on the merged tree:

• apps/desktop/test/provider-https-metadata.test.mjs + provider-metadata-fetch.test.mjs: 15/15 pass.
• docs/check-docs.mjs: the only remaining problem is the ADR heading above; fixing it should take the run to zero.

The certificate logic itself is fine — this is purely rebase + ADR formatting.

[muzimu217]

Correction, and good news: after your rebase (907e0edf), the docs gate passes as-is — I ran docs/check-docs.mjs on your new head and it reports 0 problems across 580 pages.

So my earlier suggestion to renumber to # ADR 0332: … was not required. Your # ADR: Provider metadata recovery from optional client certificate requests satisfies the checker; the rule only requires the heading to start with "ADR", not to carry a number. Sorry for the extra direction there — the numbered form is the convention most ADRs here follow, but it is not what the gate enforces.

Current state of your head 907e0edf:

• Conflict resolved — MERGEABLE.
• Docs gate: clean.
• No CI run yet on this head (no checks reported on the 'fix/optional-client-cert' branch) — it needs a push that triggers the workflow, or a maintainer re-run, before it can land.

Nothing else from me; the certificate logic itself I verified earlier (15/15 provider tests pass).

[vastsa]

I confirmed the #1676 failure path: Electron 43's main-process network request fails on the optional client-certificate handshake, and this retry is narrowly scoped to replayable HTTPS metadata requests while retaining TLS verification and proxy routing.

The current head is based on latest main, but GitHub reports no CI, Docs, or PR-base runs for 907e0ed. The local Electron 43 repro/E2E is useful evidence, but the required remote checks are still missing. Please have the workflows run and pass on this exact head, then I can reconsider the merge. Thanks for the careful TLS and cancellation coverage.

[yixinforu]

Thanks for confirming!

The workflow runs for 907e0ed have been created, but all three are currently marked as action_required and appear to be awaiting maintainer approval:

• CI
• Docs check
• PR base

Could a maintainer with the required permissions approve these runs through the GitHub web UI so the checks can proceed?

Thanks for your help!

[vastsa]

我已在 #1739 合入后把分支更新到最新 main（base 17a0c30，head 5b493a0）。补丁仍保持原来的窄范围；但这个 head 目前没有任何 check runs，commit status 仍是 pending。请让 CI、Docs check 和 PR-base 在该 head 上实际运行并通过；此前的 action_required 仍需要维护者在 GitHub 上批准 workflow。通过后我会继续合入审核。

[vastsa]

I reviewed the refreshed head 58c0e3a against current main (87a48b9). The #1676 Electron 43 failure is real, and the retry remains narrowly limited to replayable HTTPS metadata requests while retaining TLS verification, proxy routing, cancellation, and redirect restrictions.

I cannot merge yet: CI, Docs check, and PR-base for this exact head are all action_required (runs 39266525367, 39266525759, 39266525726), so none has produced a result. Please have a maintainer release these runs; I’ll merge after the required checks pass. Thanks for resolving the docs conflict and refreshing the branch.