Skip to content

fix: handle None providers config + tighten custom-provider model guards - #3967

Closed
lidi1011 wants to merge 2 commits into
nesquena:masterfrom
lidi1011:fix/config-none-providers
Closed

fix: handle None providers config + tighten custom-provider model guards#3967
lidi1011 wants to merge 2 commits into
nesquena:masterfrom
lidi1011:fix/config-none-providers

Conversation

@lidi1011

@lidi1011 lidi1011 commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Eight small fixes across 6 files:

1. Lower CLI session display threshold

  • api/agent_sessions.py: CLI_MIN_UNTITLED_USER_MESSAGE_COUNT 2→1, so single-message CLI sessions appear in the sidebar.

2. Handle None value for providers config key

  • api/config.py, api/onboarding.py, api/providers.py, api/routes.py: Replace cfg.get("providers", {}) with (cfg.get("providers") or {}) (17 call sites total).

Why: When the config YAML has a providers: key whose value is null/empty, Python's dict.get("providers", {}) returns None instead of {} — the default is only used when the key is absent, not when it holds None. Downstream .get() calls then crash. The or {} pattern correctly handles both absent and None-valued keys.

3. Tighten custom provider auto-detection guards

  • api/config.py:
    • Don't discard "custom" provider when auto-detected models exist (and not auto_detected_models)
    • Scope the auto_detected_models fallback to custom/local PIDs only (elif pid in ("custom", "local") and auto_detected_models:), preventing non-custom providers from incorrectly inheriting the global fallback list

4. Clear stale cross-profile session on profile switch

- agent_sessions: lower CLI_MIN_UNTITLED_USER_MESSAGE_COUNT 2→1
  so single-message CLI sessions appear in sidebar

- config / onboarding / providers / routes: replace cfg.get("providers", {})
  with (cfg.get("providers") or {}) everywhere. When the config key exists
  but holds None (broken/malformed config), dict.get() returns None instead
  of the default {}, causing downstream .get() calls to crash.

- config: don't discard "custom" provider when auto-detected models
  are present (add 'and not auto_detected_models' guard)

- config: scope the auto_detected_models fallback to custom/local PIDs
  only (elif pid in ("custom", "local") and auto_detected_models:),
  preventing non-custom providers from inheriting the global fallback
@greptile-apps

greptile-apps Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR applies three fixes: lowers the CLI session display threshold so single-message sessions appear in the sidebar, replaces all cfg.get(\"providers\", {}) calls with (cfg.get(\"providers\") or {}) across 5 files to handle a null YAML value, and tightens custom-provider auto-detection guards in api/config.py to prevent non-custom providers from inheriting a global fallback model list.

  • static/panels.js: Adds stale-session detection when switching profiles with no messages, creating a fresh session to avoid cross-profile operations — but the file ships with an unresolved Git merge conflict at line 5650 that will crash the browser JS parser.
  • api/config.py (and providers.py, routes.py, onboarding.py): 17 cfg.get(\"providers\", {}) call sites patched to (cfg.get(\"providers\") or {}), correctly handling a config key whose value is explicitly null.
  • api/agent_sessions.py: CLI_MIN_UNTITLED_USER_MESSAGE_COUNT reduced from 2 → 1.

Confidence Score: 2/5

Not safe to merge — static/panels.js contains raw Git conflict markers that will crash the browser JS parser on load.

The Python-side fixes are correct and well-scoped. However, static/panels.js ships with an unresolved merge conflict (lines 5650–5659): the conflict markers are literally present in the file and any browser loading this script will throw a syntax error, breaking the entire profile-switching function.

static/panels.js — unresolved merge conflict at lines 5650–5659 must be resolved before this file can be served to a browser.

Important Files Changed

Filename Overview
static/panels.js Contains an unresolved Git merge conflict (lines 5650–5659) that will cause a JS syntax error at runtime, breaking profile-switching logic entirely.
api/config.py All cfg.get("providers", {}) calls replaced with (cfg.get("providers") or {}); custom-provider discard guard now also checks auto_detected_models; auto_detected_models fallback scoped to custom/local PIDs only.
api/providers.py Five cfg.get("providers", {}) call sites updated to (cfg.get("providers") or {}); logic is otherwise unchanged.
api/routes.py Two cfg.get("providers", {}) call sites updated to (cfg.get("providers") or {}); no behavioral change beyond None-safety.
api/onboarding.py Single cfg.get("providers", {}) call updated to (cfg.get("providers") or {}); straightforward None-safety fix.
api/agent_sessions.py CLI_MIN_UNTITLED_USER_MESSAGE_COUNT lowered from 2 to 1 so single-message CLI sessions appear in the sidebar.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["switchToProfile(name)"] --> B{Has messages?}
    B -- Yes --> C["newSession(false, opts)\nsyncTopbar()\nrenderSessionList()"]
    B -- No --> D{Session stale\nfor new profile?}
    D -- Yes --> E["newSession(false)"]
    E --> F{_profileDefaultWorkspace\n& S.session?}
    F -- Yes --> G["POST /api/session/update\n(workspace, model, provider)"]
    F -- No --> H
    G --> H["renderSessionList()\nsyncTopbar()"]
    D -- No --> H
    H --> I["⚠️ MERGE CONFLICT\n<<<<<<< Updated upstream\nloadDir('.') with await+mode check\n=======\nloadDir('.') fire-and-forget\n>>>>>>> Stashed changes"]
    I --> J["showToast(profile_switched)"]
Loading

Reviews (2): Last reviewed commit: "fix(profiles): clear stale cross-profile..." | Re-trigger Greptile

Comment thread api/config.py
…esquena#2535)

When switching profiles without sending a message first, the old
profile's session could remain active, causing subsequent workspace
switches and sends to operate on the wrong profile.

Now detect a stale session (profile mismatch) after switch and start
a fresh session, applying the profile's default workspace.
Comment thread static/panels.js
Comment on lines +5650 to +5659
<<<<<<< Updated upstream
// Refresh workspace file tree so the right panel shows the new
// profile's workspace, not the previous one (#1214).
if (S.session && S.session.workspace) {
const dirLoad = loadDir('.');
if (typeof _workspacePanelMode !== 'undefined' && _workspacePanelMode !== 'closed') await dirLoad;
}
=======
if (S.session && S.session.workspace) loadDir('.');
>>>>>>> Stashed changes

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.

P0 Unresolved Git merge conflict

The file contains raw conflict markers (<<<<<<< Updated upstream, =======, >>>>>>> Stashed changes) at lines 5650–5659. When a browser parses this file, it will throw a syntax error the moment it reaches <<<<<<< Updated upstream, breaking the entire switchToProfile function and everything that follows. The two conflicting versions differ on whether loadDir('.') is awaited conditionally (the upstream version, which also respects _workspacePanelMode) or fired-and-forgotten (the stashed version). One of these must be chosen and the conflict markers removed before merging.

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Summary

I pulled the branch into a read-only worktree and diffed origin/master...HEAD. The (cfg.get("providers") or {}) hardening is correct and the core idea is good, but there's a hard blocker: static/panels.js ships committed git stash conflict markers. That's invalid JavaScript — the bundle won't parse, so the autocomplete/profile-switch UI (and anything else that imports panels.js) breaks at load.

Code reference — the blocker

static/panels.js:5650-5659 in the PR head:

      syncTopbar();
<<<<<<< Updated upstream
      // Refresh workspace file tree so the right panel shows the new
      // profile's workspace, not the previous one (#1214).
      if (S.session && S.session.workspace) {
        const dirLoad = loadDir('.');
        if (typeof _workspacePanelMode !== 'undefined' && _workspacePanelMode !== 'closed') await dirLoad;
      }
=======
      if (S.session && S.session.workspace) loadDir('.');
>>>>>>> Stashed changes
      showToast(t('profile_switched', name));

The "Updated upstream" side is the correct one — it matches the #1214 await-aware refresh already on master at static/panels.js:5633-5637. The "Stashed changes" side (loadDir('.') fire-and-forget) is stale and predates #1214. Resolve by keeping the upstream side and deleting the three marker lines plus the stashed alternative.

The or {} changes are sound

The premise checks out — dict.get("providers", {}) only uses the default when the key is absent, not when it holds None:

>>> {'providers': None}.get('providers', {})   # None  ← crashes downstream .get()
>>> {'providers': None}.get('providers') or {}  # {}    ← correct

So the 17 call-site swaps across api/config.py, api/onboarding.py, api/providers.py, api/routes.py are the right fix for a providers: YAML key left empty/null.

Two changes worth a second look

  1. api/config.py:5012 — narrowing elif auto_detected_models: to elif pid in ("custom", "local") and auto_detected_models:. Note the existing branch at config.py:5020 already guards the bare-custom phantom-dup case (if pid == "custom" and active_provider and active_provider != "custom": models_for_group = [], Bug: Phantom duplicate model entries in model list when using custom_providers alongside ai-gateway #1881). Your narrowing now sends every other unconfigured pid to models_for_group = [] instead of the global fallback. That's plausibly the intent, but please confirm it doesn't regress the fix: remove redundant provider label concatenation in _deduplicate_model_ids #1511 multi-unconfigured-provider grouping that block was written for.

  2. api/agent_sessions.py:21CLI_MIN_UNTITLED_USER_MESSAGE_COUNT 2→1 is unrelated to the providers fix and probably belongs in its own PR (it also overlaps the sidebar-listing area in Sessions list not listing all items #3966). Splitting it would make this one a clean, reviewable security/robustness fix.

Test plan

After resolving the conflict: node --check static/panels.js must pass (it currently can't), then exercise a profile switch with no messages sent yet to confirm the #2535 stale-session reset still starts a fresh session with the new profile's default workspace.

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Thanks @lidi1011 — salvaging the best part of this into a fresh, focused PR rather than rebasing the bundle.

The standout fix here is the None-providers hardening: cfg.get("providers", {}) only defaults when the key is absent, so a providers: key left empty/null in config.yaml returns None and crashes the downstream .get(...). That guard is genuinely needed and is currently missing in api/onboarding.py, api/providers.py, and api/routes.py on master (only config.py has it). We're porting just that hardening fresh, with a regression test, crediting you — I'll link the new PR here.

The other changes in this bundle we're intentionally dropping: the panels.js merge conflict (it conflicts with the #1214 await-aware refresh already in master), the CLI_MIN_UNTITLED_USER_MESSAGE_COUNT 2→1 change (unrelated, overlaps #3966 — belongs in its own PR), and the config.py elif narrowing (needs a #1511 multi-unconfigured-provider regression check before it's safe). Keeping the salvage to the one clean, verifiable robustness fix.

Closing in favor of the focused salvage PR — appreciate catching the empty-config crash.

nesquena-hermes added a commit that referenced this pull request Jun 28, 2026
…(salvage of #3967)

A config.yaml with an explicit empty/null `providers:` key parses to None in
PyYAML. `cfg.get("providers", {})` only returns the {} default when the key is
*absent*, so an explicit null yields None instead of an empty mapping. master
already guards each read with `isinstance(providers_cfg, dict)`, but the value
fed to that guard is still a footgun: the natural chained form
`cfg.get("providers", {}).get(...)` (already fixed once in api/config.py)
crashes with AttributeError on None.

Harden every providers-key read in the three remaining files to
`cfg.get("providers") or {}` so an explicit null degrades to an empty dict at
the source, matching the existing api/config.py convention. Sites hardened:

  api/onboarding.py:578  (_provider_api_key_present)
  api/providers.py:1051  (_provider_has_shadowed_codex_oauth_value)
  api/providers.py:1219  (_provider_has_key)
  api/providers.py:1265  (_get_provider_api_key)
  api/providers.py:2348  (get_providers catalog)
  api/providers.py:2781  (_clean_provider_key_from_config)
  api/routes.py:5045     (_context_length_config_api_key_for_provider)
  api/routes.py:5110     (_context_length_lookup_inputs_for_model)
  api/routes.py:15916    (_handle_live_models)

Adds tests/test_none_providers_config_guard.py: per-file source-form pins (fail
on master / on any single-file revert) plus behavioural checks driving the real
functions with `providers: None` to prove no crash and parity with the
empty-mapping config. Updates the existing test_issue3717 routes-string pin to
match the hardened form.

This is a focused salvage of grab-bag PR #3967 -- only the None-providers
hardening is taken; the PR's unrelated changes (account-usage worker-pool
refactor, function removals) are intentionally dropped.

Co-authored-by: lidi1011 <lidi1011@users.noreply.github.com>
nesquena-hermes added a commit that referenced this pull request Jun 28, 2026
…(salvage of #3967)

A config.yaml with an explicit empty/null `providers:` key parses to None in
PyYAML. `cfg.get("providers", {})` only returns the {} default when the key is
*absent*, so an explicit null yields None instead of an empty mapping. master
already guards each read with `isinstance(providers_cfg, dict)`, but the value
fed to that guard is still a footgun: the natural chained form
`cfg.get("providers", {}).get(...)` (already fixed once in api/config.py)
crashes with AttributeError on None.

Harden every providers-key read in the three remaining files to
`cfg.get("providers") or {}` so an explicit null degrades to an empty dict at
the source, matching the existing api/config.py convention. Sites hardened:

  api/onboarding.py:578  (_provider_api_key_present)
  api/providers.py:1051  (_provider_has_shadowed_codex_oauth_value)
  api/providers.py:1219  (_provider_has_key)
  api/providers.py:1265  (_get_provider_api_key)
  api/providers.py:2348  (get_providers catalog)
  api/providers.py:2781  (_clean_provider_key_from_config)
  api/routes.py:5045     (_context_length_config_api_key_for_provider)
  api/routes.py:5110     (_context_length_lookup_inputs_for_model)
  api/routes.py:15916    (_handle_live_models)

Adds tests/test_none_providers_config_guard.py: per-file source-form pins (fail
on master / on any single-file revert) plus behavioural checks driving the real
functions with `providers: None` to prove no crash and parity with the
empty-mapping config. Updates the existing test_issue3717 routes-string pin to
match the hardened form.

This is a focused salvage of grab-bag PR #3967 -- only the None-providers
hardening is taken; the PR's unrelated changes (account-usage worker-pool
refactor, function removals) are intentionally dropped.

Co-authored-by: lidi1011 <lidi1011@users.noreply.github.com>
nesquena-hermes added a commit that referenced this pull request Jun 28, 2026
fix: harden None providers config across onboarding/providers/routes (salvage of #3967)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M Medium PR (≤10 files, ≤250 LOC)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants