Skip to content

fix(oauth): honor connectionId on token refresh so email-less providers don't duplicate - #8062

Merged
diegosouzapw merged 1 commit into
diegosouzapw:release/v3.8.49from
insoln:fix/oauth-refresh-connection-dedup
Jul 22, 2026
Merged

diegosouzapw merged 1 commit into
diegosouzapw:release/v3.8.49from
insoln:fix/oauth-refresh-connection-dedup

Conversation

@insoln

@insoln insoln commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Fixes #8059

Problem

Pressing Refresh token on a GitHub Copilot connection creates a second connection instead of updating the existing one.

Root cause

persistOAuthConnection (src/lib/oauth/connectionPersistence.ts) gates its entire dedup step behind if (tokenData.email). The matcher it calls (findExistingOAuthConnectionMatch) already matches by explicit connectionId first, but it's never reached when the payload has no top-level email. GitHub Copilot's device-code flow keeps identity under providerSpecificData.githubEmail, so tokenData.email is undefined — the refresh (which does pass the existing connectionId) skips the whole match and falls through to createProviderConnection, producing a duplicate.

Fix

Two small changes in connectionPersistence.ts:

  1. Widen the gate to if (connectionId || tokenData.email) — an explicit connectionId (refresh / re-auth of a known connection) is honored regardless of whether the payload carries a top-level email.
  2. In findExistingOAuthConnectionMatch, guard the email branch with if (!tokenData.email) return false; — without it safeEqual(undefined, undefined) is true, so once the gate is widened an email-less payload could false-match another email-less connection. Email dedup now requires a non-empty email.

The connectionId-first and email/Codex-workspace matching semantics are otherwise unchanged.

Testing

New tests/unit/oauth-refresh-connection-dedup-8059.test.ts (4 cases, real SQLite):

  • A Copilot-style refresh (no top-level email) with the existing connectionId updates the connection — one connection, token updated. Verified red on the unfixed gate (asserts the duplicate) and green after the fix.
  • findExistingOAuthConnectionMatch: connectionId wins without an email; an email-less payload does not false-match an email-less connection; email dedup still works when an email is present.
  • npm run typecheck:core and ESLint clean.

Note: this is preventive — a duplicate created before the fix must still be removed manually.

…rs don't duplicate

persistOAuthConnection gated its whole dedup step behind if(tokenData.email).
The matcher (findExistingOAuthConnectionMatch) already matches by explicit
connectionId first, but it was never reached when the payload had no top-level
email. GitHub Copilot's device-code flow keeps identity under
providerSpecificData.githubEmail, so tokenData.email is undefined — a refresh
(which passes the existing connectionId) skipped the match and fell through to
createProviderConnection, producing a duplicate connection.

- Widen the gate to if(connectionId || tokenData.email) so an explicit
  connectionId is honored regardless of email.
- Guard the matcher's email branch with if(!tokenData.email) return false, so a
  widened gate can't false-match an email-less connection via
  safeEqual(undefined, undefined).

Fixes diegosouzapw#8059.
@insoln
insoln requested a review from diegosouzapw as a code owner July 21, 2026 22:34
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@diegosouzapw

Copy link
Copy Markdown
Owner

Obrigado, @insoln — mergeado na release/v3.8.49. Validado no tip combinado (merge-train: typecheck + file-size + 106 testes das áreas tocadas + vitest 271/271 verdes). O único red de CI era o base-red compartilhado de complexity (pré-existente na tip da release, não deste PR).

fenix007 pushed a commit to fenix007/OmniRoute that referenced this pull request Jul 23, 2026
…s + eslint baseline

The release branch accumulated deterministic unit-test failures (fast-path red on
every open PR). These are the ones with a clear, surgical root cause:

1. diegosouzapw#6863 combo model-lockout — the diegosouzapw#7940/diegosouzapw#7980 "cap exactCooldownMs against
   maxCooldownMs" clamp was also clamping an AUTHORITATIVE parsed upstream quota
   reset (e.g. "Resets in 92h27m28s") down to maxCooldownMs, so an exhausted model
   was retried far too early. recordModelLockoutFailure now takes
   exactCooldownIsUpstreamReset — set by the combo callers when the exact cooldown
   is a real upstream reset — which exempts it from the cap. The diegosouzapw#7980 computed
   until-midnight cap is unchanged (flag absent → still capped).

2. diegosouzapw#5786 streaming claude←codex — stripInternalReasoningPlaceholder (diegosouzapw#8081/diegosouzapw#8162)
   unconditionally .trim()'d every value. On the per-delta streaming path this ate
   the meaningful edge spaces of each delta ("Hello, " + "world." + " Bye." glued to
   "Hello,world.Bye."). It now only collapses to "" when whitespace is all that
   remains after removing the placeholder, preserving real content verbatim.

3. SPAWN_CAPABLE_PREFIXES test — diegosouzapw#7892 added /api/vnc-session (11th spawn-capable
   prefix, spawns Docker) but the client-safe guard test still expected 10 and did
   not list it. Aligned to 11 + added the entry to the checklist.

4. ESLint baseline — diegosouzapw#8008/diegosouzapw#8062 merged new test files with no-explicit-any without
   refreshing the frozen suppressions, so "No new ESLint warnings" went red for the
   whole branch. Regenerated the two affected entries
   (combo-routing-engine.test.ts 269→271, oauth-refresh-connection-dedup-8059.test.ts +1).

Validated: the three failing tests now pass; the sibling guards they interact with
stay green (diegosouzapw#7980 exact-cooldown-cap 4/4, diegosouzapw#8162 placeholder suites 17+12+41,
account-fallback 77); typecheck:core clean; lint:json --max-warnings 0 exits 0.

NOTE: the release branch has ~20 further real base-red failures (compression-engine
catalog, handleChat fallback, provider candidate transparency, i18n, misc). Those are
tracked separately, one focused PR per root-cause cluster; this PR is the first slice.
fenix007 pushed a commit to fenix007/OmniRoute that referenced this pull request Jul 24, 2026
…s + eslint baseline

The release branch accumulated deterministic unit-test failures (fast-path red on
every open PR). These are the ones with a clear, surgical root cause:

1. diegosouzapw#6863 combo model-lockout — the diegosouzapw#7940/diegosouzapw#7980 "cap exactCooldownMs against
   maxCooldownMs" clamp was also clamping an AUTHORITATIVE parsed upstream quota
   reset (e.g. "Resets in 92h27m28s") down to maxCooldownMs, so an exhausted model
   was retried far too early. recordModelLockoutFailure now takes
   exactCooldownIsUpstreamReset — set by the combo callers when the exact cooldown
   is a real upstream reset — which exempts it from the cap. The diegosouzapw#7980 computed
   until-midnight cap is unchanged (flag absent → still capped).

2. diegosouzapw#5786 streaming claude←codex — stripInternalReasoningPlaceholder (diegosouzapw#8081/diegosouzapw#8162)
   unconditionally .trim()'d every value. On the per-delta streaming path this ate
   the meaningful edge spaces of each delta ("Hello, " + "world." + " Bye." glued to
   "Hello,world.Bye."). It now only collapses to "" when whitespace is all that
   remains after removing the placeholder, preserving real content verbatim.

3. SPAWN_CAPABLE_PREFIXES test — diegosouzapw#7892 added /api/vnc-session (11th spawn-capable
   prefix, spawns Docker) but the client-safe guard test still expected 10 and did
   not list it. Aligned to 11 + added the entry to the checklist.

4. ESLint baseline — diegosouzapw#8008/diegosouzapw#8062 merged new test files with no-explicit-any without
   refreshing the frozen suppressions, so "No new ESLint warnings" went red for the
   whole branch. Regenerated the two affected entries
   (combo-routing-engine.test.ts 269→271, oauth-refresh-connection-dedup-8059.test.ts +1).

Validated: the three failing tests now pass; the sibling guards they interact with
stay green (diegosouzapw#7980 exact-cooldown-cap 4/4, diegosouzapw#8162 placeholder suites 17+12+41,
account-fallback 77); typecheck:core clean; lint:json --max-warnings 0 exits 0.

NOTE: the release branch has ~20 further real base-red failures (compression-engine
catalog, handleChat fallback, provider candidate transparency, i18n, misc). Those are
tracked separately, one focused PR per root-cause cluster; this PR is the first slice.
HouMinXi pushed a commit to HouMinXi/OmniRoute that referenced this pull request Aug 2, 2026
…rs don't duplicate (diegosouzapw#8062)

persistOAuthConnection gated its whole dedup step behind if(tokenData.email).
The matcher (findExistingOAuthConnectionMatch) already matches by explicit
connectionId first, but it was never reached when the payload had no top-level
email. GitHub Copilot's device-code flow keeps identity under
providerSpecificData.githubEmail, so tokenData.email is undefined — a refresh
(which passes the existing connectionId) skipped the match and fell through to
createProviderConnection, producing a duplicate connection.

- Widen the gate to if(connectionId || tokenData.email) so an explicit
  connectionId is honored regardless of email.
- Guard the matcher's email branch with if(!tokenData.email) return false, so a
  widened gate can't false-match an email-less connection via
  safeEqual(undefined, undefined).

Fixes diegosouzapw#8059.
HouMinXi pushed a commit to HouMinXi/OmniRoute that referenced this pull request Aug 2, 2026
…gosouzapw#8209)

* chore(quality): refresh stale any-suppression count for combo-routing-engine.test.ts

Rebasing onto release/v3.8.49 pulled in upstream's diegosouzapw#8008 (prompt-cache
affinity), which added 2 more `any` usages to this test file (269 ->
271). ESLint's suppressions mechanism requires an exact count match —
any drift makes the whole file's suppression stale and reports every
violation as new. Not a violation to fix (pre-existing test-mock any
usage in an upstream commit), just an allowlist count refresh.

Co-Authored-By: Markus Hartung <markus.hartung@gmail.com>

* chore(quality): type the oauth-refresh-dedup test's connection filter instead of any

Upstream diegosouzapw#8062 introduced this test file with an untyped `any` filter
callback param, which the strict any-budget lint rule flags as a new
violation (not a pre-existing one to allowlist). Derives the element type
from getProviderConnections' own return type instead of importing/hand-
writing it.

Co-Authored-By: Markus Hartung <markus.hartream@gmail.com>

---------

Co-authored-by: Markus Hartung <markus.hartung@gmail.com>
Co-authored-by: Markus Hartung <markus.hartream@gmail.com>
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…rs don't duplicate (diegosouzapw#8062)

persistOAuthConnection gated its whole dedup step behind if(tokenData.email).
The matcher (findExistingOAuthConnectionMatch) already matches by explicit
connectionId first, but it was never reached when the payload had no top-level
email. GitHub Copilot's device-code flow keeps identity under
providerSpecificData.githubEmail, so tokenData.email is undefined — a refresh
(which passes the existing connectionId) skipped the match and fell through to
createProviderConnection, producing a duplicate connection.

- Widen the gate to if(connectionId || tokenData.email) so an explicit
  connectionId is honored regardless of email.
- Guard the matcher's email branch with if(!tokenData.email) return false, so a
  widened gate can't false-match an email-less connection via
  safeEqual(undefined, undefined).

Fixes diegosouzapw#8059.
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…gosouzapw#8209)

* chore(quality): refresh stale any-suppression count for combo-routing-engine.test.ts

Rebasing onto release/v3.8.49 pulled in upstream's diegosouzapw#8008 (prompt-cache
affinity), which added 2 more `any` usages to this test file (269 ->
271). ESLint's suppressions mechanism requires an exact count match —
any drift makes the whole file's suppression stale and reports every
violation as new. Not a violation to fix (pre-existing test-mock any
usage in an upstream commit), just an allowlist count refresh.

Co-Authored-By: Markus Hartung <markus.hartung@gmail.com>

* chore(quality): type the oauth-refresh-dedup test's connection filter instead of any

Upstream diegosouzapw#8062 introduced this test file with an untyped `any` filter
callback param, which the strict any-budget lint rule flags as a new
violation (not a pre-existing one to allowlist). Derives the element type
from getProviderConnections' own return type instead of importing/hand-
writing it.

Co-Authored-By: Markus Hartung <markus.hartream@gmail.com>

---------

Co-authored-by: Markus Hartung <markus.hartung@gmail.com>
Co-authored-by: Markus Hartung <markus.hartream@gmail.com>
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.

fix(auth): token refresh creates a duplicate connection for email-less OAuth providers (GitHub Copilot)

2 participants