Skip to content

fix(desktop): active provider sync — UI badge and generate IPC now share single source of truth - #55

Merged
hqhq1025 merged 3 commits into
mainfrom
wt/loop-fix-active-provider-sync
Apr 18, 2026
Merged

hqhq1025 merged 3 commits into
mainfrom
wt/loop-fix-active-provider-sync

Conversation

@hqhq1025

Copy link
Copy Markdown
Collaborator

Summary

  • The renderer kept its own copy of cfg.provider / cfg.modelPrimary in the Zustand store and forwarded them in GeneratePayloadV1.model. When that store drifted from cachedConfig (e.g. between Settings tabs, multiple windows, or any path that mutated cfg without firing setConfig), the generate handler silently called a different provider than the Active badge displayed.
  • User-visible bug: Settings showed OpenRouter Active, but dev log showed provider: openai, baseUrl: https://api.duckcoding.ai/v1 (a leftover per-provider baseUrl entry resolved against the stale renderer hint).
  • New resolveActiveModel(cfg, hint) in provider-settings.ts is the single resolver: it snaps the renderer hint back to cachedConfig.provider (the SAME source toProviderRows uses for the Active badge), preserves the hint's modelId when the providers match (so the renderer can still pick fast vs primary), and otherwise snaps the modelId to cfg.modelPrimary. baseUrl is sourced exclusively from the active provider's cached baseUrls entry.
  • Wired into codesign:v1:generate, the legacy codesign:generate shim, and codesign:apply-comment. Mismatches log step=resolve_active.override so the dev timeline shows when a snap happened.

Root cause / disconnect

Two implicit sources of truth:

  1. UI Active badge → runListProviders() → toProviderRows(cachedConfig) (canonical).
  2. Generate IPC credentials/baseUrl → looked up via payload.model.provider from the renderer (could be stale).

Whenever the renderer's store fell behind a Settings mutation, source (2) diverged from (1). The fix collapses both onto cachedConfig via resolveActiveModel.

Test plan

  • pnpm --filter @open-codesign/desktop test — 142 tests pass, including 4 new resolveActiveModel cases (matching hint, stale-hint snap, baseUrl threading, missing-secret guard).
  • pnpm typecheck — clean.
  • pnpm lint — clean (only pre-existing warnings).
  • Manual repro: add openai (with custom baseUrl) + openrouter, switch Active to openrouter, send a prompt — dev log now shows provider: openrouter, baseUrl: <default> and never the stale openai/duckcoding combo.

PRINCIPLES §5b

  • Compatibility: green — no schema changes, no IPC contract changes, payload still accepts model as-is.
  • Upgradeability: green — single resolver makes a future "renderer-doesn't-send-model" cleanup trivial.
  • No bloat: green — net +50 lines in main, +69 in tests, no new deps.
  • Elegance: green — collapses two sources of truth onto one canonical getter.

@github-actions github-actions Bot left a comment

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.

Findings

  • [Major] Renderer baseUrl still takes precedence after provider override, which can route the active provider request to a stale endpoint and send the active provider API key to the wrong host. This reintroduces cross-provider drift in transport even though model/provider are now canonicalized, evidence apps/desktop/src/main/index.ts:162, apps/desktop/src/main/index.ts:276.
    Suggested fix:
    // Do not trust renderer-supplied baseUrl once provider/model are resolved from cached config.
    const baseUrl = active.baseUrl ?? undefined;

Summary

  • Review mode: initial
  • 1 issue found in the latest diff. docs/VISION.md and docs/PRINCIPLES.md were not available in this checkout (Not found in repo/docs), so this review used repo-visible constraints plus changed code.

Testing

  • Not run (automation)

open-codesign Bot

Comment thread apps/desktop/src/main/index.ts Outdated
// diverge from what the user sees.
const active = resolveActiveModel(cfg, payload.model);
const apiKey = getApiKeyForProvider(active.model.provider);
const baseUrl = payload.baseUrl ?? active.baseUrl ?? undefined;

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.baseUrl is still prioritized here even when resolveActiveModel has overridden provider/model. That can send the active provider's API key to a stale host. Prefer canonical config endpoint only:

const baseUrl = active.baseUrl ?? undefined;

Comment thread apps/desktop/src/main/index.ts Outdated
}
const active = resolveActiveModel(cfg, payload.model);
const apiKey = getApiKeyForProvider(active.model.provider);
const baseUrl = payload.baseUrl ?? active.baseUrl ?? undefined;

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.

Same precedence issue in the legacy shim: payload.baseUrl can bypass canonical provider endpoint selection. Align with resolveActiveModel output:

const baseUrl = active.baseUrl ?? undefined;

hqhq1025 added a commit that referenced this pull request Apr 18, 2026
Codex Major on PR #55: even after resolveActiveModel snaps to the
canonical active provider, the IPC handlers were still preferring
payload.baseUrl over active.baseUrl. A stale renderer hint like
{provider: openai, baseUrl: duckcoding.ai/v1} would route the active
openrouter API key to duckcoding's host.

Once we've snapped, the renderer baseUrl belongs to a (possibly stale)
different provider and cannot be trusted. Always use the per-provider
baseUrl from cached config — same source the Settings UI reads.

Adds a vitest case covering the cross-provider baseUrl scenario.

Signed-off-by: hqhq1025 <1506751656@qq.com>
@hqhq1025
hqhq1025 force-pushed the wt/loop-fix-active-provider-sync branch from b44459d to b9dcd07 Compare April 18, 2026 20:20
…are single source of truth

The renderer kept its own copy of cfg.provider/modelPrimary in the Zustand
store and forwarded them in GeneratePayloadV1.model. When that copy drifted
from cachedConfig (e.g. after switching providers in Settings), the generate
handler silently called a different provider than the Active badge displayed
— users would see OpenRouter active but logs showed openai + a leftover
duckcoding baseUrl.

Add resolveActiveModel() that snaps the renderer hint back to the canonical
active provider in cachedConfig (the same source toProviderRows uses for the
badge), then use it in codesign:v1:generate, the legacy codesign:generate
shim, and codesign:apply-comment. baseUrl is now sourced from the active
provider's cached entry, never from a stale per-provider lookup keyed by the
renderer hint. Mismatches are logged as resolve_active.override so the dev
log timeline shows when a snap happened.

Signed-off-by: hqhq1025 <1506751656@qq.com>
Codex Major on PR #55: even after resolveActiveModel snaps to the
canonical active provider, the IPC handlers were still preferring
payload.baseUrl over active.baseUrl. A stale renderer hint like
{provider: openai, baseUrl: duckcoding.ai/v1} would route the active
openrouter API key to duckcoding's host.

Once we've snapped, the renderer baseUrl belongs to a (possibly stale)
different provider and cannot be trusted. Always use the per-provider
baseUrl from cached config — same source the Settings UI reads.

Adds a vitest case covering the cross-provider baseUrl scenario.

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

@github-actions github-actions Bot left a comment

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.

Findings

  • None.

Summary

  • Review mode: follow-up after new commits
  • No new issues found in added/modified lines of the latest diff.
  • docs/VISION.md and docs/PRINCIPLES.md: Not found in repo/docs.
  • Residual risk: behavior changed in codesign:v1:generate, legacy codesign:generate, and codesign:apply-comment, but this diff only adds unit tests for resolveActiveModel and does not add handler-level IPC tests for those call paths.

Testing

  • Not run (automation)
  • Suggested tests: Vitest IPC handler tests that send stale renderer hints and assert canonical provider/modelId plus canonical per-provider baseUrl are used in both generate handlers and apply-comment.

open-codesign Bot

Defense-in-depth for the codex Major finding: even though the IPC
handlers already use active.baseUrl exclusively, the renderer-supplied
payload.baseUrl could still leak into future log/handler additions.
Mutate payload.baseUrl post-snap so the canonical per-provider value is
the only baseUrl reachable from this point onward, and add a vitest case
that pins the openrouter-active vs. openai-hint scenario.

Signed-off-by: hqhq1025 <1506751656@qq.com>
@hqhq1025
hqhq1025 force-pushed the wt/loop-fix-active-provider-sync branch from b9dcd07 to 652fccd Compare April 18, 2026 20:25

@github-actions github-actions Bot left a comment

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.

Findings

  • None.

Summary

  • Review mode: follow-up after new commits
  • No new issues found in added/modified lines of the latest diff.
  • docs/VISION.md and docs/PRINCIPLES.md: Not found in repo/docs.
  • Residual risk: behavior changed in codesign:v1:generate, legacy codesign:generate, and codesign:apply-comment, but this diff only adds unit tests for resolveActiveModel and does not add handler-level IPC tests for those paths.

Testing

  • Not run (automation)
  • Suggested tests: Vitest IPC handler tests that send stale renderer hints and assert canonical provider/modelId and canonical per-provider baseUrl are used in both generate handlers and apply-comment.

open-codesign Bot

@hqhq1025
hqhq1025 merged commit 4d7dcf3 into main Apr 18, 2026
6 checks passed
@hqhq1025
hqhq1025 deleted the wt/loop-fix-active-provider-sync branch April 18, 2026 20:27
hqhq1025 added a commit that referenced this pull request Apr 19, 2026
…are single source of truth (#55)

* fix(desktop): active provider sync — UI badge and generate IPC now share single source of truth

The renderer kept its own copy of cfg.provider/modelPrimary in the Zustand
store and forwarded them in GeneratePayloadV1.model. When that copy drifted
from cachedConfig (e.g. after switching providers in Settings), the generate
handler silently called a different provider than the Active badge displayed
— users would see OpenRouter active but logs showed openai + a leftover
duckcoding baseUrl.

Add resolveActiveModel() that snaps the renderer hint back to the canonical
active provider in cachedConfig (the same source toProviderRows uses for the
badge), then use it in codesign:v1:generate, the legacy codesign:generate
shim, and codesign:apply-comment. baseUrl is now sourced from the active
provider's cached entry, never from a stale per-provider lookup keyed by the
renderer hint. Mismatches are logged as resolve_active.override so the dev
log timeline shows when a snap happened.

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

* fix(desktop): drop renderer baseUrl after provider snap

Codex Major on PR #55: even after resolveActiveModel snaps to the
canonical active provider, the IPC handlers were still preferring
payload.baseUrl over active.baseUrl. A stale renderer hint like
{provider: openai, baseUrl: duckcoding.ai/v1} would route the active
openrouter API key to duckcoding's host.

Once we've snapped, the renderer baseUrl belongs to a (possibly stale)
different provider and cannot be trusted. Always use the per-provider
baseUrl from cached config — same source the Settings UI reads.

Adds a vitest case covering the cross-provider baseUrl scenario.

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

* fix(desktop): explicitly null payload.baseUrl on provider override

Defense-in-depth for the codex Major finding: even though the IPC
handlers already use active.baseUrl exclusively, the renderer-supplied
payload.baseUrl could still leak into future log/handler additions.
Mutate payload.baseUrl post-snap so the canonical per-provider value is
the only baseUrl reachable from this point onward, and add a vitest case
that pins the openrouter-active vs. openai-hint scenario.

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

---------

Signed-off-by: hqhq1025 <1506751656@qq.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.

1 participant