Skip to content

fix: don't pass stale lastProvider to createLLMClient after config wizard#466

Closed
ragini-pandey wants to merge 2 commits intoNano-Collective:mainfrom
ragini-pandey:fix/issue-460-provider-name-mismatch-on-config-wizard
Closed

fix: don't pass stale lastProvider to createLLMClient after config wizard#466
ragini-pandey wants to merge 2 commits intoNano-Collective:mainfrom
ragini-pandey:fix/issue-460-provider-name-mismatch-on-config-wizard

Conversation

@ragini-pandey
Copy link
Copy Markdown
Contributor

Description

After the provider wizard saves a new agents.config.json, the app immediately calls createLLMClient(preferences.lastProvider) in handleConfigWizardComplete. If the old lastProvider preference (e.g. "GitHub Models") is not present in the freshly-written config (which only has "GitHub Copilot"), createLLMClient throws:

ConfigurationError: Provider 'GitHub Models' not found in agents.config.json. Available providers: GitHub Copilot

…which surfaces in the UI as the Failed to initialize with new configuration: ConfigurationError message reported in #460.

Root cause: handleConfigWizardComplete passed the stale preferences.lastProvider string explicitly. The validation block in createLLMClient treats an explicit provider name as a hard requirement and throws when it is absent from the new config.

Fix: Call createLLMClient() without an explicit provider argument in the post-wizard reinitialisation path. Without that argument the validation block is skipped; the function's internal fallback loop uses preferences.lastProvider as a preference hint (not a hard requirement) and gracefully falls through to the first available provider in the freshly-loaded config.

Type of Change

  • Bug fix

Testing

Automated Tests

  • New features include passing tests in .spec.ts/tsx files
  • All existing tests pass (pnpm test:all completes successfully)
  • Tests cover both success and error scenarios

Two new serial tests added to source/client-factory.spec.ts:

  1. createLLMClient: throws ConfigurationError when explicit provider not in config (issue #460) — confirms that passing a stale explicit provider name still throws ConfigurationError (the existing contract is preserved for other callers).
  2. createLLMClient: succeeds without explicit provider even when config changed (issue #460) — confirms the no-argument path succeeds, using the first available provider in the new config ("GitHub Copilot").

Manual Testing

  • Tested with Ollama
  • Tested with OpenRouter
  • Tested with OpenAI-compatible API
  • Tested MCP integration (if applicable)

Checklist

  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed)
  • No breaking changes (or clearly documented)
  • Appropriate logging added using structured logging (see CONTRIBUTING.md)

Copilot AI review requested due to automatic review settings April 21, 2026 19:25
@ragini-pandey ragini-pandey requested a review from Avtrkrb as a code owner April 21, 2026 19:25
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a post-provider-wizard initialization failure where the app attempted to reinitialize using a stale preferences.lastProvider value that may not exist in the newly-written agents.config.json (issue #460).

Changes:

  • Updates the post-wizard reinitialization path to call createLLMClient() without an explicit provider argument, allowing graceful fallback to the first available configured provider.
  • Adds regression tests to preserve the “explicit provider name must exist” contract while ensuring the no-argument path succeeds after config changes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
source/hooks/useModeHandlers.tsx Avoids passing a stale provider name after the config wizard; relies on createLLMClient() fallback behavior.
source/client-factory.spec.ts Adds regression tests covering the explicit-provider error path and the no-argument fallback path for issue #460.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread source/client-factory.spec.ts Outdated
…zard

After the provider wizard saves a new agents.config.json the app calls
createLLMClient with the old preferences.lastProvider value.  If that
name is no longer present in the freshly-written config (e.g. the user
switched from 'GitHub Models' to 'GitHub Copilot') createLLMClient throws
ConfigurationError: Provider '...' not found.

Fix: call createLLMClient() without an explicit provider argument in
handleConfigWizardComplete so the function falls through to the first
available provider in the new config instead of failing on a stale name.

Regression tests added to client-factory.spec.ts covering both the
explicit stale-provider path (must still throw ConfigurationError) and
the no-argument path (must succeed with the available provider).

Closes Nano-Collective#460
@ragini-pandey ragini-pandey force-pushed the fix/issue-460-provider-name-mismatch-on-config-wizard branch from c6ac166 to 524eb51 Compare April 21, 2026 19:30
Scope NANOCODER_CONFIG_DIR to a temp subdirectory for the
'succeeds without explicit provider even when config changed (issue Nano-Collective#460)'
test so that loadPreferences() never touches the real user config
directory (~/.config/nanocoder or ~/Library/Preferences/nanocoder).

Restore the original env var in a finally block to avoid leaking state
between tests.

Addresses review comment on PR Nano-Collective#466.
@ragini-pandey ragini-pandey changed the title fix: don't pass stale lastProvider to createLLMClient after config wizard (closes #460) fix: don't pass stale lastProvider to createLLMClient after config wizard Apr 21, 2026
@electricwolfemarshmallowhypertext
Copy link
Copy Markdown

{4EA7A309-D108-488B-8543-659FAEACA2F5}

@ragini-pandey

@will-lamerton
Copy link
Copy Markdown
Member

Hey @ragini-pandey, thanks for the PR and for jumping in to help.

Quick heads up for future contributions: @electricwolfemarshmallowhypertext had already offered to take this one earlier in the thread and we'd agreed they'd handle it. When someone's visibly claimed an issue in the comments, please give them a chance to submit their PR before opening your own - it's discouraging for contributors who've done the diagnostic work to find someone else has shipped code first. That's on us too for not assigning the issue; we'll start doing that going forward to make claims clearer.

I've asked @electricwolfemarshmallowhypertext to review since they traced the root cause. Assuming the fix aligns with what they found, we'll move forward with it. Appreciate the work and hope you'll pick up one of the other open issues :)

@ragini-pandey
Copy link
Copy Markdown
Contributor Author

Hey @ragini-pandey, thanks for the PR and for jumping in to help.

Quick heads up for future contributions: @electricwolfemarshmallowhypertext had already offered to take this one earlier in the thread and we'd agreed they'd handle it. When someone's visibly claimed an issue in the comments, please give them a chance to submit their PR before opening your own - it's discouraging for contributors who've done the diagnostic work to find someone else has shipped code first. That's on us too for not assigning the issue; we'll start doing that going forward to make claims clearer.

I've asked @electricwolfemarshmallowhypertext to review since they traced the root cause. Assuming the fix aligns with what they found, we'll move forward with it. Appreciate the work and hope you'll pick up one of the other open issues :)

Thanks for the context! I saw the issue was unassigned and open with no activity, so I went ahead and worked on a fix. Totally get that comment-based claims can be easy to miss without a formal assignment. I'm glad you're moving to assigning issues going forward, that'll make things much clearer for everyone.

Happy to let @electricwolfemarshmallowhypertext review and take it from here. I'll pick up something else from the open issues!

@electricwolfemarshmallowhypertext
Copy link
Copy Markdown

Other PR:

❌ Unit tests failing
❌ Package audit failing
❌ Dependency issues

The change avoids the crash, but does not resolve the underlying state inconsistency. preferences.lastProvider remains stale, and initialization now relies on implicit fallback behavior instead of restoring a valid provider. Weakens determinism where explicit configuration previously existed...

My approach:

✅ Normalizes lastProvider against the updated config
✅ Preserves explicit provider contract (no silent fallback)
✅ Ensures deterministic, state-consistent initialization

Suggested:

Normalize lastProvider against the new config prior to calling createLLMClient, so initialization remains deterministic and state-consistent.

Above ⬆️ already implemented in my PR. @will-lamerton

@will-lamerton
Copy link
Copy Markdown
Member

will-lamerton commented Apr 22, 2026

Other PR:

❌ Unit tests failing

❌ Package audit failing

❌ Dependency issues

The change avoids the crash, but does not resolve the underlying state inconsistency. preferences.lastProvider remains stale, and initialization now relies on implicit fallback behavior instead of restoring a valid provider. Weakens determinism where explicit configuration previously existed...

My approach:

✅ Normalizes lastProvider against the updated config

✅ Preserves explicit provider contract (no silent fallback)

✅ Ensures deterministic, state-consistent initialization

Suggested:

Normalize lastProvider against the new config prior to calling createLLMClient, so initialization remains deterministic and state-consistent.

Above ⬆️ already implemented in my PR. @will-lamerton

Feel free to open your PR and I'll look at that one if this one doesn't implement your identified true fix. It would be preferable to have the most robust solution put in. Happy to close in favour of as mentioned in the issue thread 👌

@electricwolfemarshmallowhypertext
Copy link
Copy Markdown

Circling back and will PR the fix today. @will-lamerton

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.

4 participants