Skip to content

refactor(provider): stop custom loaders using facades#20776

Merged
kitlangton merged 5 commits intodevfrom
kit/provider-loader-dev
Apr 4, 2026
Merged

refactor(provider): stop custom loaders using facades#20776
kitlangton merged 5 commits intodevfrom
kit/provider-loader-dev

Conversation

@kitlangton
Copy link
Copy Markdown
Contributor

Summary

  • stop provider custom loaders from calling static Auth.get(...) and Config.get() facades
  • capture yielded auth and config services inside the provider layer and expose only tiny local Promise bridges where the loader contract still requires promises
  • add focused opencode provider regression tests for config-key and auth-backed paid model loading

Testing

  • bun run test test/provider/provider.test.ts test/provider/amazon-bedrock.test.ts
  • bun typecheck

@luanweslley77
Copy link
Copy Markdown
Contributor

luanweslley77 commented Apr 3, 2026

Hi @kitlangton, noticed this PR is still in draft — wanted to flag a related timing issue we've been investigating that might be worth considering as part of this refactor.
We traced a bug (#20026) where plugin providers registered via the config() hook disappear after /instance/dispose. The root cause is that configProviders = Object.entries(cfg.provider ?? {}) is captured as a stale snapshot before plugin.list() runs and calls the config() hook. When plugins modify cfg.provider to add their providers, it's already too late.
Your refactor injects dep (with auth and config bridges) into custom loaders, which is a nice improvement — but it doesn't address the ordering of plugin.list() relative to configProviders.
Since this PR is still in draft, would you be open to proposing a solution that also addresses the initialization ordering? We have a fix in PR #20777 that moves plugin.list() to run before configProviders, but we'd be happy to see a more elegant approach from you if you think it fits better into this refactor.
Happy to collaborate or adjust our approach if you have ideas.

@kitlangton
Copy link
Copy Markdown
Contributor Author

Hi @kitlangton, noticed this PR is still in draft — wanted to flag a related timing issue we've been investigating that might be worth considering as part of this refactor. We traced a bug (#20026) where plugin providers registered via the config() hook disappear after /instance/dispose. The root cause is that configProviders = Object.entries(cfg.provider ?? {}) is captured as a stale snapshot before plugin.list() runs and calls the config() hook. When plugins modify cfg.provider to add their providers, it's already too late. Your refactor injects dep (with auth and config bridges) into custom loaders, which is a nice improvement — but it doesn't address the ordering of plugin.list() relative to configProviders. Since this PR is still in draft, would you be open to proposing a solution that also addresses the initialization ordering? We have a fix in PR #20777 that moves plugin.list() to run before configProviders, but we'd be happy to see a more elegant approach from you if you think it fits better into this refactor. Happy to collaborate or adjust our approach if you have ideas.

Hey, thanks. I added a regression test which did indeed reproduce the issue! I've included your changes into this PR. 🫡

@kitlangton kitlangton requested a review from Copilot April 3, 2026 21:12
Load plugin hooks before snapshotting config providers so plugin-defined providers survive instance rebuilds. Add a regression test that covers the dispose and reload path.

Co-authored-by: luanweslley77 <213105503+luanweslley77@users.noreply.github.com>
@kitlangton kitlangton marked this pull request as ready for review April 3, 2026 21:13
@kitlangton kitlangton added the beta label Apr 3, 2026
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

Refactors provider-layer custom loaders to stop calling static Auth.get(...) / Config.get() facades by injecting auth and config services into the loader layer, and adds regression tests around opencode paid model loading behavior.

Changes:

  • Converted provider custom loaders to return Effect and added an injected dependency bridge for auth/config.
  • Updated loader execution to run these effects directly (instead of wrapping promise-returning loaders).
  • Added opencode regression tests ensuring paid models remain available when an apiKey is present in config or auth storage.

Reviewed changes

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

File Description
packages/opencode/src/provider/provider.ts Refactors custom provider loaders to use injected auth/config services via Effect instead of static facades.
packages/opencode/test/provider/provider.test.ts Adds regression tests for opencode paid model filtering based on config apiKey or auth presence.

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

Read enabled and disabled provider config after plugin hooks run so plugin config can affect provider filtering. Add a regression test for plugin-driven provider filters and fix the auth cleanup test so cleanup does not swallow assertion failures.
@kitlangton kitlangton enabled auto-merge (squash) April 4, 2026 00:12
@kitlangton kitlangton merged commit 59ca454 into dev Apr 4, 2026
10 checks passed
@kitlangton kitlangton deleted the kit/provider-loader-dev branch April 4, 2026 00:24
robinmordasiewicz added a commit to f5xc-salesdemos/xcsh that referenced this pull request Apr 4, 2026
cavaldos pushed a commit to cavaldos/openpentest that referenced this pull request Apr 6, 2026
Co-authored-by: luanweslley77 <213105503+luanweslley77@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants