AI: initial support for multiple custom providers - #15675
Open
sharon-wang wants to merge 48 commits into
Open
Conversation
New ai-config refuses to mutate a providers.json it can't parse, so it never silently discards user config. That broke the migration's explicit Overwrite confirmation, which relied on the old read coercing an unparseable file to an empty config. runMigration now drops the unusable file and retries, but only once the user has confirmed the overwrite. Also updates the Bedrock region expectation: ai-lib#50 preserves region provenance and no longer stamps its default onto the connection, and production already falls back to DEFAULT_AWS_REGION.
Everything a hand-edited providers.custom.<name> entry needs to work end to end in Positron. No new UI of its own: the Add and Edit forms are #12747. - providerCatalog gains create, update, and delete for custom entries. Updates overlay only the fields the UI owns, under the config lock, so customHeaders, protocol, endpoints, models, and enabled survive a write. Edits read the raw user layer rather than the resolved catalog, so an admin's enforced value is never baked into the user's own file. - The catalog change event reports addedIds and removedIds. An added id was only visible as a connection change and a removed one was not reported at all, which is exactly how custom entries come and go. - CustomProviderRegistry keeps one auth provider and one model source registered per enabled entry, reconciled against the catalog rather than snapshotted at activation. The auth provider id is the entry name, which is the contract Posit Assistant resolves credentials against. - The modal's source list is live. A provider can now appear or disappear while it is open, which the fixed-at-open snapshot could not express. A custom entry's connect form offers an API key only when its client kind takes one, mirroring ai-credentials' auth descriptors. google-vertex and aws resolve a credential from the environment, and local kinds need none.
Trims the previous commit to what the code actually reaches. No behaviour change; ~200 lines out. - createCustomProvider, deleteCustomProvider, and the name-validation wrapper are gone. Nothing calls them, and the Add and Edit forms (#12747) are what decide their shape anyway, including the per-kind aws / snowflake / googleCloud sections this already deferred. - updateCustomProviderConnection and its omitted/blank/set tri-state collapse to saveCustomProviderBaseUrl(name, baseUrl). The one caller had already checked the value was non-empty, so the remove-the-key branch was unreachable. - With create and delete gone, mutateCustomEntries had nothing left to do: no map to clone, no mutator that throws, no emptied custom key to drop. Inlined as a single-entry write. The mutateProvidersConfig logger literal is now shared instead of written twice. - addedIds and removedIds leave the catalog change event. No listener read either one: the registry reconciles against the catalog, and the built-in loop only reads changedConnectionIds and disabledIds. A removal still has to fire, so the guard keeps a local boolean for it. - getCustomProviderSources was only ever called from tests. The tests that covered the cut code go with it, and the removal path gets one of its own: rewrite the config file, refresh, and assert a change fires even though no remaining provider moved.
A custom entry's client kind decides which credential it needs, and that answer isn't ours: Posit Assistant derives the same value for the same entry when it picks an auth provider to read from, so both sides have to agree. customProviderAuth.ts holds that map. What the connect form collects, and how Positron obtains the credential, are Positron's own and now sit next to it instead of being flattened into one table. Before this, every kind got a bare API-key auth provider, so the four non-apikey kinds were offered but couldn't connect: - aws and google-vertex now resolve from the environment with the same resolvers the built-in Bedrock and GEAP providers use. Bedrock's moved to credentials/aws.ts as resolveAwsCredential, so there's one path and one token shape. Without a chain these entries offered no API key field (right: they take none) and had nothing to resolve either, so Connect prompted for a key the kind can't use. - ollama and lmstudio get no auth provider, which is what the assistant expects: the endpoint comes from the entry and there's no credential. They still need that endpoint saved, so registerProviderCallbacks registers the save callback without a provider, and handleSave and handleDelete tolerate a provider that has callbacks and no credential. - snowflake collects a key but no URL. Its Cortex URL is derived from snowflake.host, so a single URL field would write a key the runtime never reads. The per-kind forms own that field (#12747). Which key holds the URL follows the same rule: endpoint for a local entry, baseUrl otherwise. Writing the wrong one looks saved and changes nothing. Registration is gated on having an auth method rather than on a second copy of the supported-kind list, so a kind we can't authenticate is never offered.
Posit Assistant remembers "no such auth provider" for the rest of the session, and registering one emits nothing to other extensions, so an entry whose key was already stored stayed dead until the user happened to sign in or out. Nothing signals it otherwise: there's no activation event a user-chosen id can declare, so an early lookup times out and that verdict sticks. register() now fires a session change for the entry name once it has swept sessions, which is the signal they asked for. Also revert the snowflake exception. C dropped the URL field for a snowflake entry on the grounds that Cortex derives its URL from snowflake.host, but a flat baseUrl is a shape both hosts have to read: it's what the standalone form writes in its custom-URL mode, and what a user hand-authors when they have a full Cortex URL rather than a bare account. The assistant is fixing its shaper to read baseUrl before deriving one, so the field stays and a snowflake entry looks like the other API-key kinds. The per-kind account and host fields are still B's (#12747), along with the mode switch that clears whichever shape the user isn't using.
The registration test stubbed positron.ai.registerProvider through an `any` cast, which the eslint rule and the repo's own guidance both say not to do: make the dependency injectable instead. CustomProviderRegistry takes it as an optional constructor parameter defaulting to the real function, so the test passes a disposable-returning fake and no global is mutated or restored.
Narrows `providers.custom` to `openai-compatible`, `anthropic`, and `openai`, and gives each one the form and the key check its built-in counterpart already has. All three authenticate with a typed key and a base URL, both of which the modal already collects. The other eleven kinds need a connection field that has no `LanguageModelConfig` key yet, and that field is how an entry names an account of its own: ai-config's env layer writes connection fields for built-in provider ids only, so an `aws` entry with no profile of its own doesn't fail, it falls through to the ambient profile, which is the credential built-in Bedrock resolves. The user gets a second row, signed in, indistinguishable from the first, when the reason for adding it was a different account. Those kinds come back with the Add and Edit UI, which can ask (#12747). The per-kind form is read from the built-in's own `supportedOptions` rather than restated, minus what only the one built-in instance can use (`autoconfigure`, `oauth`) and the API type field this work removes (`protocol`), plus `customModels`. A field added to the Anthropic tile now shows up on custom Anthropic entries too. The descriptor table also mirrors `apiKeyOptional`, which the previous copy dropped. It has teeth in the new validator: a gateway with auth switched off saves with a blank key, a kind that requires one says so at save time, and an optional key still runs the check, because for `openai-compatible` that check is also what requires the base URL.
An audit of the branch against its own scope. Four things existed only to serve kinds that are no longer offered, or nothing at all. `customModels` leaves the custom form. Nothing writes it for a custom entry: `saveCustomProviderModels` writes `providers[catalogId]`, a top-level block, which is the built-in `openai-compatible` provider's own entry rather than `providers.custom[name]`. Offering the field rendered an input that silently discarded what the user typed. It returns with create and edit, which is what grows the write path. `registerProviderCallbacks` and the four relaxations around it come out of `configDialog.ts`. They let a provider register callbacks with no auth provider, which only the local kinds did. `registerAuthProvider` sets `authProviders` and `onSaveCallbacks` together, so with that caller gone `!provider && !onSaveCallbacks.has(id)` was just `!provider`, and the rest was unreachable. Not merely dead: it weakened the "No auth provider registered" guard on the save path every built-in provider shares. `saveCustomProviderUrl` loses its `field` argument. Only `endpoint`, for `ollama` and `lmstudio`, ever wanted anything but `baseUrl`. `resolveAwsCredential` is reverted, leaving `credentials/aws.ts` identical to before the branch. It was extracted so a custom `aws` entry could share the built-in Bedrock resolver; with that entry gone it was a single-caller refactor of a working provider, and Bedrock has no business being touched by this PR.
Adds the Add screen and the affordance that opens it, in the provider list below the sections. Nothing writes anything: submitting invokes `authentication.addCustomProvider`, which no extension registers yet, so the button reports a missing command. The extension half is the next commit. Every other path through the modal behaves as before. The form asks for the two things a custom entry adds -- a name, and an optional list of model IDs -- and gets the rest from the built-in its type borrows from, read from that provider's own registered source. Picking Anthropic gives you Anthropic's inputs, its base URL label, its icon, and its terms notice, so a field added to that tile appears here too rather than drifting from a second copy. Interaction follows posit-dev/assistant#2056: name, then type, then the connection fields; nothing validates on blur; problems report on submit in one inline message, so the button is never greyed out with no explanation. Changing the type keeps only the name, so a key typed for one provider can't be submitted against another. Only the name is checked here (blank, or already taken) -- the key and URL rules belong to the writer, which runs the same check the built-in provider runs, and duplicating them would only let the two disagree. The API key and base URL inputs, and the model ID rows, move out of the connect view into components both forms use, so the two can't render different fields for the same provider. The models rows gain a collapsed mode: every offered kind publishes its own list, so they are an override and stay out of the way until asked for. The affordance is gated on `posit-assistant.supportsCustomProviders`, which Posit Assistant sets at activation on a build that serves models for custom entries. A capability key rather than a version check: the assistant auto-updates on its own cadence, so a version comparison goes stale.
Registers `authentication.addCustomProvider`, which is what the Add form submits to. It can't go through the modal's usual provider action, which is keyed on an already registered provider id: a new entry has none until this creates it. A create is three steps in one operation, in this order: check the key, write the entry, register it, then store the credential under the entry name. The key is checked first, by the same check the matching built-in provider runs, so a rejected key doesn't leave a signed-out entry behind for a provider the user never managed to add. A blank key is still stored when the kind allows one -- a gateway with auth switched off has none, and the session is what makes the entry usable rather than merely present. `createCustomProviderEntry` writes under the config lock, and calls `mintCustomProviderId` inside it for its throw rather than its value: that is ai-config's own rule about built-in ids, reserved keys, and `__proto__`, and running it in the lock means a name can't be taken between the check and the write. A name that already exists throws rather than merging -- two entries of one name are one entry, and a silent merge would attach the new key to someone else's endpoint. `enabled: true` is explicit so a `providers.default` block that turns things off doesn't swallow a provider the user just added, and declared model ids write `models.custom` with discovery off, since an endpoint with no listing has nothing to discover. Reconciles are now serialized. Every write refreshes the catalog and fires a change event, so a create both reconciles directly and trips the watcher's reconcile; run concurrently, both would see the new entry as unregistered and register its auth provider twice. The key check is injectable on the registry, the way the model-source registration already was, so a test can add an entry without a live endpoint to check against.
Three things a newly added provider got wrong. **It didn't appear until the modal was reopened.** A source is shown only when the catalog says its provider is enabled, and the workbench reads that catalog itself, on its own file watch. For a provider that has just been added, registration arrives first and the enablement that makes it visible lands after, on an event the modal wasn't listening to, so the row waited for the next time the modal was opened. The provider-updates hook now treats an enablement change the same as a registration change: re-read the list. **It used the generic icon.** A custom entry now carries its type (`customKind`) on its provider metadata, and the row and the detail header resolve their icon through it, so a custom Anthropic entry appears under Anthropic's icon rather than as an anonymous row. That mirrors the form, which already borrows the built-in's fields. **It was tagged Experimental.** That's a maturity claim about Positron's support, and a custom entry is as mature as whatever it connects to. The tag is now Custom, which says the thing a user actually needs to know about that row, and the stand-in status goes away with it.
A `providers.custom` entry's display name is whatever the user typed, so the generic vendor copy read "Your use of my anthropic is optional and at your sole risk", and the terms sentence called that name Third Party Materials with unlinked Terms of Service and Privacy Policy labels. Anything with a customKind now takes the same path as the OpenAI-compatible tile: the terms point at whoever operates the endpoint, and the disclaimer says "this provider". The kind it borrows from isn't necessarily who runs the endpoint, so it isn't named and its terms aren't linked. The row description and icon already say which type it is.
"Custom Provider" was a fine name when it was the only way to reach an endpoint
of your own. Now that a named `providers.custom` entry is the custom provider,
the built-in tile needs to say what it actually is, and it matches the type
picker's own label for the same kind.
The row description no longer echoes the name back ("Connect any
OpenAI-compatible API endpoint"), and the validation errors drop the provider
name entirely: the dialog already says which provider you're configuring, and
the same validator serves custom openai-compatible entries.
The `provider.customProvider.enable` and `models.overrides.customProvider`
setting keys keep their names so existing settings still resolve; only their
descriptions change.
Sections were chosen purely by state, so clearing a custom entry's key
dropped it into Model Providers, between Amazon Bedrock and Snowflake
Cortex with a Connect button, as if Positron shipped it. That section is
the catalogue of providers we offer; an entry you named yourself doesn't
belong in it.
Add a fourth section, custom, rendered last, directly above the Add
Custom Provider button. It comes after the error and signed-in checks,
so an entry with an expired key still floats to Needs Attention rather
than sinking to the bottom, and a connected one still shows as
connected. Model Providers goes back to meaning one thing.
The row's type line ("Custom Anthropic provider") was written but never
wired up, so a custom row showed no description at all. Wire it, and
show it in the new section.
Rename the credential action from Remove to Disconnect on both the
connected and Fix Connection screens (Sign Out stays for OAuth). It says
what happens: you stay in the list, your settings stay, your key is
gone. Deleting an entry is a different verb and lands with the Edit
screen, and two buttons that both read as removal would be a trap. The
'delete' dispatch verb is unchanged; the extension contract isn't
user-facing.
The e2e Connect selector now excludes Disconnect: the two sit together
on the Fix Connection screen and :has-text("Connect") matches both.
A custom entry's authentication provider id was the name the user chose. trustedExtensionAuthAccess in product.json is keyed by authentication provider id, so no static allowlist could ever contain it: Posit Assistant's silent getSession for such an entry resolved to undefined with no error, and the entry's models never reached the model picker. The provider configured cleanly and served nothing. Register one provider, positron-custom-provider, for every entry, with the entry name as the scope. The trust check ignores the account entirely, so one product.json key covers every entry a user ever adds. No credential moves. Each entry keeps its own AuthProvider, constructed with the entry name, so its storage keys are untouched and there is nothing to migrate. Only the registerAuthenticationProvider call goes away, and the aggregate re-stamps the entry name on as the scope, since AuthProvider is shared with the built-ins and mints scopes: []. Declared in contributes.authentication with an activation event, which also retires the multi-second reject a dynamic name could only ever lose: a static id can be activated on, so the caller no longer waits for the registration arm of the race.
The shared auth provider stays registered when an entry is deleted or disabled, so the two signals the workbench drops a cached account on are both absent: the provider never unregisters, and AuthProvider's own dispose fires nothing (it sets a flag, stops its refresh timer, and disposes its emitter). A deleted entry left a stale account in the Accounts menu until the window reloaded. So the shared provider owns its membership explicitly rather than through a passive map. addProvider subscribes and snapshots before the delegate is reachable, then fires the snapshot as added. removeProvider detaches the listener first, keeping the delegate, then reads its final sessions, then drops the membership, then fires removed. The detach has to come first because getSessions is asynchronous: read the snapshot while still subscribed and a delegate event landing in that window is forwarded as added and then left out of removed, which is the stale account all over again. removeProvider also has to run ahead of AuthProvider.dispose(), which unregister() does in one loop over the disposables, so the reconcile path now detaches before disposing rather than relying on a disposable inside the list.
Three places assumed a custom entry's auth provider id and its model source id were the same string, and all three broke once the entries moved to one shared provider. The extension's session listener looked the entry up in authProviders by the event's provider id. Nothing answers to positron-custom-provider there, so every custom row would have stopped updating its signed-in state. Fan the event out to the registered entries instead, which are still keyed by entry name. The workbench's session sync filtered on the provider id and then read sessions unscoped. Under a shared provider nothing matched, and if it had, an unscoped read returns every entry's sessions, so one entry signing in would have marked them all signed in. It now takes targets that say which ids are custom and reads each by its own scope. By scope rather than by mapping the event's account label back to an entry. The label is the entry name today, so that would work, but it makes a display string load-bearing for identity: the moment a label and an id diverge it flips the wrong row, silently. Carrying which ids are custom means useProviderUpdates takes structured targets rather than a list of ids, which also fixes a bug that was already there: it stabilized the list by joining on a comma and splitting it back, and nothing restricts the characters in an entry name, so a provider named `Acme, Inc.` split into two bogus ids.
Four comments still described the old contract, that Positron registers an authentication provider under the entry name so the credential is derivable from the provider id alone. The entry name is still what the credential is filed under, but now as a scope on one shared provider rather than as a provider id of its own. A stale contract comment here is how the next person reintroduces the bug, so the prose says scope where it used to say auth provider id.
configDialog keys authProviders, apiKeyValidators, onSaveCallbacks, and onDeleteCallbacks by the same string. A providers.custom entry named after one of the authentication provider ids this extension already registers replaces the built-in's row in all four, and unregistering it deletes them. ai-config's name policy doesn't cover this: it rejects built-in provider ids (anthropic, openai, openai-compatible), which is a different set. Ten of our auth provider ids are legal entry names as far as it is concerned, including anthropic-api, google, and databricks. The shared provider's own id is reserved with them. An entry called positron-custom-provider would answer to both branches of the session fan-out and of the workbench's session sync, with two code paths claiming one id. The check that matters is on the registration path, not the form: reconcile registers whatever the catalog holds, so a hand-written or externally managed entry never passes through the form at all. create() checks too, so the form reports the refusal rather than writing an entry that then declines to register. Snowflake's id becomes a constant along the way, since the reserved list has to name it and it was three string literals before.
5dc9a20 reworded the custom provider's validation error from "Custom Provider base URL is required" to "Base URL is required". The test matched the old wording as a substring, so it has been failing since that commit.
Three groups, matching what the design can get wrong. Routing: a scoped read names one entry, an unscoped read is the union, and an unknown or two-scope read returns nothing, all in one assertion so the scope rules read as a table. Sessions come back stamped with the entry name, which is how a caller tells whose key it was handed. createSession's refusals are asserted by message; its delegating path prompts for a key, so it is left to the live test. Lifecycle: an entry removed while it still had a live session reports that session as removed, and reports it again as added when the entry comes back. This is the group the design would have shipped without, and the stale Accounts menu entry is what it prevents. Names: the entry that matters is hand-written, not typed into the form. A guard sitting only in create() would let a reconcile register an entry named anthropic-api anyway, and a form-path test would pass with the guard in exactly that wrong place. A second test keeps the reserved list in step with contributes.authentication, since the list exists to protect ids the manifest declares. The shared registration is now injectable, which the tests need for a reason worth stating: the extension's own activation already holds that id, the extension host is first-one-wins on a duplicate, and disposing the losing registration unregisters the winner. The registration test listened to the cross-extension event before, and only passed because of that collision; it now listens to the provider the registry actually built and leaves event delivery to extHostAuthentication. The workbench sync's fake records the scopes it was asked for, so a regression back to an unscoped read fails here rather than in a bug report, and it moves to stubInterface while being touched.
Declaring positron-custom-provider in contributes.authentication broke the test that every declared contribution has a PROVIDER_METADATA entry. It is the one declared id that isn't a provider in the catalogue: it exists so the id can be allowlisted in product.json and activated on, holds no credential of its own, and has no tile.
The Accounts menu renders one row per entry, reading "<account label> (<provider label>)". The account label is the entry name, so a plural provider label described the row wrong: "my anthropic (Custom Providers)". The modal's section heading, which really does cover many, stays plural.
Adding a custom provider was one-way: nothing in the modal removed one, so the only way out was hand-editing providers.json. Delete Provider sits on the entry's own screen, connected or not, and confirms on a screen of its own rather than a dialog stacked on the modal, so Cancel lands back where the user was. Deliberately not in the footer next to Disconnect: two buttons that both read as removal is how an entry gets deleted by someone meaning to clear its key. The write is the extension's, through a command, for the same reason the add is: the modal's provider action is keyed on a provider id, and this one unregisters it. It clears the credential first, while the entry's auth provider is still registered, then removes the entry; the reconcile that follows disposes the auth provider, and a key left behind would come back to life under a re-created entry of the same name. An entry Positron didn't author (defined by a default or enforced layer) is refused, with the message pointing at the providers.json that defines it. Hiding the action for those, rather than refusing, waits on the user-managed split that Edit needs anyway.
Two things the form did on the user's behalf, neither of which it should. The base URL arrived prefilled from the borrowed built-in's source. That default is getSavedBaseUrl(...): whatever the user already saved for the built-in, falling back to a hardcoded example. Offering it back suggested pointing a second entry at the same endpoint as the first, and for the cloud kinds it was redundant anyway, since an entry written with no URL leaves the client calling the vendor's own. Changing the type also cleared the key, the URL, and the model rows. The guard was that a key typed for one provider shouldn't be submitted against another, but the type is often the last thing corrected, and retyping everything to fix it costs more than that risk. The extension still runs the target kind's own key check on submit.
|
E2E Tests 🚀 Why these tags?
More on automatic tags from changed files. |
Member
Author
|
Roughly I think the review buckets are:
|
2 tasks
melissa-barca
requested changes
Aug 26, 2026
melissa-barca
left a comment
melissa-barca
left a comment
Contributor
There was a problem hiding this comment.
I've done an initial pass through, but haven't tested this yet. Leaving my feedback now so you can get started with it. I focused on the auth and core code, but also surfaced a couple UI things copilot flagged that checked out as far as I could tell.
Awesome work on this! Really excited for it to land!
| /** | ||
| * Local copy of ai-credentials' `CUSTOM_CLIENT_KIND_AUTH_DESCRIPTORS`, the | ||
| * authority, which can't be imported: it publishes only an `import` condition and | ||
| * isn't a dependency here (posit-dev/ai-lib issue pending). Exhaustive, so a kind |
Contributor
There was a problem hiding this comment.
is "posit-dev/ai-lib issue pending" a hallucination? Or can we move this to ai-provider-bridge so we don't need to redefine it?
| @@ -0,0 +1,780 @@ | |||
| /*--------------------------------------------------------------------------------------------- | |||
Contributor
There was a problem hiding this comment.
This introduces extensions/authentication/src/test/customProviders.test.ts when we already have extensions/authentication/src/test/customProvider.test.ts -- note the S at the end of providers. Can we consolidate this into one? I thought grep was somehow wrong when I was searching for something that didn't exist in the file 😅
Comment on lines
+87
to
+101
| const RESERVED_AUTH_PROVIDER_IDS: readonly string[] = [ | ||
| ANTHROPIC_AUTH_PROVIDER_ID, | ||
| POSIT_AUTH_PROVIDER_ID, | ||
| FOUNDRY_AUTH_PROVIDER_ID, | ||
| AWS_AUTH_PROVIDER_ID, | ||
| SNOWFLAKE_AUTH_PROVIDER_ID, | ||
| OPENAI_AUTH_PROVIDER_ID, | ||
| CUSTOM_PROVIDER_AUTH_PROVIDER_ID, | ||
| GEMINI_AUTH_PROVIDER_ID, | ||
| GOOGLE_CLOUD_AUTH_PROVIDER_ID, | ||
| DEEPSEEK_AUTH_PROVIDER_ID, | ||
| DATABRICKS_AUTH_PROVIDER_ID, | ||
| POSITRON_CUSTOM_AUTH_PROVIDER_ID, | ||
| ]; | ||
|
|
Contributor
There was a problem hiding this comment.
I think we can replace this constant and the associated test 'the reserved names are every auth provider id the manifest declares', with the mechanism that test uses to list and iterate on the provider names. Then we don't need to keep anything in sync and can remove all the imports.
Contributor
There was a problem hiding this comment.
One caveat, this list and package.json don't include copilot-auth because it isn't wrapped by Positron's auth extension. Initially I didn't think it made sense to wrap it as it is an already established AuthProvider, but I could see an argument for doing it, outside of this PR's scope though.
| if (!await readCustomProviderEntry(name)) { | ||
| log.info(`Not saving a URL for externally managed custom provider: ${name}`); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Two things here:
- I think we're conflating default with enforced here.
readCustomProviderEntry()is also undefined when the provider comes fromPOSIT_AI_PROVIDERS_DEFAULT, but should be user-overridable, so in that case we should persist the base URL. For values fromPOSIT_AI_PROVIDERS_ENFORCED, can we disable the base URL input so that the modal doesn't accept a change? - Also, this returns early but
configDialog.ts:187-194still callspositron.ai.updateProvider(providerId, { defaults: { baseUrl: config.baseUrl } });so the user entered base url will live in memory until a reload overwrites it.
| baseUrl={baseUrl} | ||
| providerId={props.source.provider.id} | ||
| showApiKey={authMethod === AuthMethod.API_KEY} | ||
| showBaseUrl={supportsBaseUrl} |
Contributor
There was a problem hiding this comment.
showBaseUrl isn't necessarily the same as supportsBaseUrl if there are cases when the base URL should be admin enforced and not user editable.
Comment on lines
+70
to
+78
| primaryButton={{ | ||
| title: pending | ||
| ? localize('positron.deleteCustomProvider.deleting', "Deleting...") | ||
| : localize('positron.deleteCustomProvider.confirm', "Delete Provider"), | ||
| disable: pending, | ||
| loading: pending, | ||
| onClick: onDelete, | ||
| }} | ||
| onClose={props.onClose} |
Contributor
There was a problem hiding this comment.
Because this uses ProviderModalFooter, the primary action is rendered with the modal-wide .default behavior. Here the primary action calls onDelete, so pressing Enter while Cancel is focused still invokes onDelete. Can the footer/action config expose an isDefault: false opt-out and set it on this primaryButton, so Enter activates only the focused button on this destructive confirmation?
Contributor
There was a problem hiding this comment.
Yeah, Enter fires onDelete here no matter what has focus. That is the old dialog's doing: PositronModalDialog handles Enter by looking for the first button with the default class anywhere in the box and clicking it. The default css class is doing two separate jobs, (1) the button's color and (2) the action Enter fires, and there is no way to keep one without the other. So an isDefault: false opt-out would have to give up the primary button's styling to get the Enter behavior right.
I am actually addressing this problem in #15703. It moves this modal to PositronDynamicModalDialog, which wraps its content in a <form> and lets the browser handle Enter by clicking the form's submit button. default is only the fill color for the button in my PR, and a new submit flag on ProviderFooterButtonConfig is what makes a button the Enter target. It is opt-in, and the connect view's Connect button is the only one that sets it. A destructive confirmation gets what you are asking for by leaving the submit flag off: Enter then only activates whatever is focused.
That last part matters beyond this view. The old dialog consumes Enter before it reaches the focused button, so a keyboard user who tabs to Cancel and presses Enter fires the default action instead of cancelling, in any dialog on the old stack. The new dialog leaves Enter alone and lets the focused button fire its action.
So I do not think you need to do anything here. Since our PRs are landing close together, I will pick this up when I rebase #15703 and the delete view gets it by simply not setting submit.
Contributor
There was a problem hiding this comment.
^ This is irrelevant after the live review I did with @sharon-wang since we're actually just getting rid of this button/functionality
| const entry = { | ||
| type: kind, | ||
| enabled: true, | ||
| ...(connection.baseUrl ? { baseUrl: connection.baseUrl } : {}), |
Contributor
There was a problem hiding this comment.
Suggested change
| ...(connection.baseUrl ? { baseUrl: connection.baseUrl } : {}), | |
| ...(connection.baseUrl ? { baseUrl: normalizeSavedBaseUrl(kind, connection.baseUrl) } : {}), |
Here and below we should normalize the url first
| } | ||
| return { | ||
| ...current, | ||
| providers: { ...current.providers, custom: { ...custom, [name]: { ...existing, baseUrl: url } } }, |
Contributor
There was a problem hiding this comment.
Suggested change
| providers: { ...current.providers, custom: { ...custom, [name]: { ...existing, baseUrl: url } } }, | |
| providers: { ...current.providers, custom: { ...custom, [name]: { ...existing, baseUrl: normalizeSavedBaseUrl(getCachedProvider(name)?.clientKind ?? name, url) } } }, |
| className='connect-provider-apikey-input' | ||
| id={`${prefix}-apikey-input`} | ||
| spellCheck={false} | ||
| type='password' |
Contributor
There was a problem hiding this comment.
The modal focus trap's selector in positronModalDialog.tsx:21-29 includes text inputs but not input[type="password"]. Using password here means tabbing from it is treated as though focus were outside the dialog and wraps to the first control instead of advancing to Base URL. Can we include enabled password inputs in that selector and add keyboard coverage?
(This feedback from copilot seems reasonable but I'm not sure I have the big picture understanding to claim it. Definitely defer to @dhruvisompura)
Contributor
There was a problem hiding this comment.
I actually ran into this exact problem while I was fixing the whitespace problem in this modal and have a fix for ensuring we can tab/focus any element that is tab-able (#15679). I would say to not worry about this since it should be fixed once my PRs get merged (I'm hoping that I can get a review from Brian on Thursday).
A lot of the keyboard accessibility/focus issues are specific to the dialog itself and not necessarily to the logic here.
Member
Author
|
super quick live review session with Dhruvi: we're gonna remove the "delete provider" for custom providers, and just include something like |
dhruvisompura
left a comment
dhruvisompura
left a comment
Contributor
There was a problem hiding this comment.
FYI, I did a keyboard accessibility pass on the dialog and I do keep ending up in situations where tabbing to different input fields in the "Add Custom Provider" dialog fails to work. I think it might be caused by the dropdown component we are using but not 100% sure (that's the only new form field that is new to this dialog).
I think it will be easier to test and fix these issues once we've migrated to the new dialog component which tries to fix some of these focus/tab issues.
I think we can open a follow up PR for any fixes we may need - doesn't need to be a blocker for this PR.
The old comment cited a hallucinated ai-lib issue for why CUSTOM_CLIENT_KIND_AUTH_DESCRIPTORS can't be imported from ai-credentials. Filed the real one (ai-lib#89) and linked it instead.
…list RESERVED_AUTH_PROVIDER_IDS duplicated contributes.authentication as a hardcoded array of imported constants, with a test separately deriving the same list from the manifest to catch drift between the two. Read the manifest directly instead, so there's only one source and nothing to drift.
Merge the validateCustomProviderApiKey suite into customProviders.test.ts and delete the near-duplicate customProvider.test.ts.
copilot-auth is a synthetic id this extension invents to key Copilot's bookkeeping (Copilot rides GitHub's built-in auth, so it never appears in contributes.authentication). A providers.custom entry named copilot-auth would collide bidirectionally with the real Copilot row in the assistant's provider map, so reserve it alongside the manifest-derived ids.
providerAction always called positron.ai.updateProvider with the typed base URL after a save, even when the registered onSave silently skipped writing it (e.g. a custom provider with no user-owned providers.json record). That left the in-memory provider showing a value that wasn't actually saved, until a reload reverted it. OnSaveCallback can now return false to report it didn't persist anything; handleSave/handleApiKeySave propagate that back to providerAction, which only reflects the base URL when it was actually written. Doesn't address the separate default-vs-enforced conflation in the same onSave (a providers.custom entry with no user-layer record could be default-sourced, which should still be overridable, or enforced-sourced, which shouldn't) -- that needs a provenance API ai-config doesn't have yet, tracked as a follow-up issue in ai-lib.
createCustomProviderEntry and saveCustomProviderUrl wrote the base URL as typed, unlike saveProviderBaseUrl (the built-in path) which already runs it through normalizeSavedBaseUrl. A bare host for anthropic/openai/ gemini kinds needs the version segment appended or the SDK client can't find it. Applies Melissa's suggested diffs from PR #15675 review.
Melissa flagged that supportsBaseUrl (does this provider have a base URL concept) isn't the same thing as whether the value should be user-editable (it shouldn't be, for an admin-enforced one). Left as supportsBaseUrl for now: there's no signal to check yet (ai-config has no per-field provenance a host could read, tracked as ai-lib#90) and the input has no disabled/read-only state regardless. Documented the gap and what the real fix needs so it doesn't look overlooked.
Agreed live with Dhruvi on PR #15675: deleting a custom provider from the modal is going away in favour of pointing users at providers.json directly (added in a follow-up commit). Removes DeleteCustomProviderAction, DeleteCustomProviderView, the 'delete-custom' modal view, and their tests. The extension-side authentication.removeCustomProvider command and its handler are untouched -- this only removes the UI path that reached it. Also makes the deleteCustomProviderView.tsx Enter-key/default-button review thread moot, since that view no longer exists.
Points a custom entry's connect and connected screens at providers.json for advanced editing (including removal), in the exact slot where Delete Provider used to render. Reuses the onEditRawConfig plumbing already wired for the Add Custom Provider flow, extended here to the connect and connected views.
EditRawConfigLink was defined once in connectProviderView.tsx and copy-pasted again inline in providerModelsSection.tsx. Pulled it into its own file so both (and connectedProviderView.tsx) import the same component instead of two copies of the same markup.
Cut em-dash-style asides and task-history narration ("used to sit
here") from comments added in the last several commits. No behavior
change.
The Add Custom Provider button was gated only on extension capability, so it could show up with no "Custom Providers" heading above it before any custom provider was added. Pull the custom section out of the grouped-sections loop and render its heading whenever the button is available, not just when a custom provider already exists.
… ones The link was gated on provider.customKind, hiding it for every built-in provider even though providers.json can configure any of them. Make onEditRawConfig a required prop (both modal call sites already always provide it) and drop the gate. Also removes the duplicate copy of the link that ProviderModelsSection rendered internally, now that the connect view always renders one in the same spot regardless of whether that section shows.
The custom-provider feature is still evolving (protocol selection is gated off, only chat completions is wired up), so mark it as such in the UI: every custom provider row now shows an Experimental badge regardless of its own status, and the Custom Providers section heading carries the same badge.
"Connect any endpoint that speaks the OpenAI API" was vague about which API and didn't match the "Access X via Y" phrasing the other provider descriptions use. Say specifically that it's the Chat Completions API.
…ation Conflict resolutions: - configDialog.ts: main added createSessionWithRecovery for the AWS SSO re-login, this branch changed handleSave to return a SaveResult. Kept both. - customProvider.test.ts: this branch folded it into customProviders.test.ts, main added a header-merging test to it. Kept the deletion and ported the new test into the consolidated file. - validationTestUtils.ts wasn't conflicted but stopped compiling: main's new stub builds a ResolvedProviderLike and this branch made clientKind required, so the stub now takes it as an optional input.
2 tasks
… sites Brings in ai-lib#71, the backend half of custom providers: the provider map is read lazily, so a custom id added mid-session keeps resolving and firing credential-change events. Three API changes needed call-site updates in the headless LM facade: - shapeCredentials takes a leading providerId. - ProviderCredentials gained an azure-entra variant (Foundry Entra ID). Shaping never emits it (only Foundry's own provider path builds it), so it is dropped alongside local, which ICredentials also doesn't mirror. - The CredentialConfig readers take a CredentialConfigTarget instead of a configKey string, which carries providerId directly and removes the configKey -> providerId reverse lookup through _loadedMappings. getAws / getSnowflake / getDatabricks keep their hardcoded built-in catalog ids: these mappings only ever cover MAPPED_PROVIDER_IDS, never a providers.custom entry, so the mixup ai-lib#71 fixes can't arise here. The lockfile change is the bump's own doing: ai-provider-bridge now declares @azure/identity as a runtime dependency, so its tree (msal, jsonwebtoken) stops being dev-only.
Member
Author
|
Thanks both for the thorough reviews! Here's a rundown of what changed. @melissa-barcaThe "ai-lib issue pending" comment
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Currently, Positron has one "Custom Provider". It can't be renamed, you can't have more than one, and it is specifically openai-compatible (not anthropic messages, or another api type).
This PR adds named
providers.customentries: as many as you like, each with its own name, type, endpoint, credential, and model list. It also relabels the old single provider to OpenAI Compatible, which is what it always was, and what Posit Assistant standalone already calls it. Nothing migrates and nobody re-enters a credential. Anyone who already configured this will still have it configured, but it will show as "OpenAI Compatible" now.This is of #13823, and a port of posit-dev/assistant#2056 rather than a new design: form shape, offered types, and the credential and write contracts all come from there, because both products read and write the same
~/.posit/ai/providers.json.Requires the following changes too -- these should merge before this PR merges:
This PR:
Follow-up work: The other eleven types (
aws,snowflake,google-vertex,ollama,lmstudio,openrouter,litellm,portkey, ...) are not yet supported in the modal. Keeping these out for now to avoid further inflating the size of this PR.Implementation
UI
Adding a custom provider
Custom provider in provider list
Deleting a custom provider
Accounts menu
Connected custom providers show in the accounts menu with the user's specified provider name along with "(Custom Provider)".
Known limitations
displayNamefield in the shared schema to support this. Filed as ai-config: add a displayName field to custom provider entries ai-lib#73.Auth & providers.json
Positron and Posit Assistant both read and write
~/.posit/ai/providers.json, so most of what follows is ported from #2056 rather than decided here.Single auth provider
All custom entries share one authentication provider,
positron-custom-provider, and the entry's name is the scope. The other option was a separate provider per entry, named after the entry.That per-entry version doesn't work. The
trustedExtensionAuthAccesslist inproduct.jsonis keyed by authentication provider id, and the user picks the entry names, so there's no way to write that list ahead of time. When Posit Assistant callsgetSession(name, [], { silent: true })for a name that isn't on the list, it gets backundefinedwith no error, so the provider looks perfectly set up while handing out nothing. One fixed id solves that, and it's the only fix that doesn't need an upstream file changed.Reads go strictly by scope: one scope means one entry, no scopes means all of them (that's what the Accounts menu asks for), and a scope we don't recognise, or that could mean more than one entry, returns nothing rather than someone else's key.
Nothing migrates. Each entry still has its own credential store. The only part now shared is the single
registerAuthenticationProvidercall.Saving to providers.json
customHeaders,protocol,endpoints,models,enabled, your comments, your formatting, and every other entry in the file.example providers.json custom section
Credentials
endpointfor local runtimes,baseUrlfor everything else). The wrong one looks like it saved and does nothing.Fields a custom entry doesn't offer
A custom entry's form is the matching built-in's form: the field list is read from that built-in's own
supportedOptionsrather than restated, so the two can't drift. Four options are dropped on the way through:autoconfigure(the env-var credential path) andoauthbelong to the one built-in instance of a provider.ANTHROPIC_API_KEYis a single value and can't say which of three custom Anthropic entries it's for, and none of the OAuth-based types are offered as custom entries anyway.protocolis the API type field this PR removes. The entry'stypecarries the wire format (Assistant: select provider type for Custom Provider #13817).customModelshas no write path for a custom entry yet:saveCustomProviderModelswrites the top-levelproviders[catalogId]block, notproviders.custom[name], so the input would render fine and silently discard what you typed. It comes back with edit support (Assistant: add ability to add multiple custom providers #12747).Reserved names
Eleven names are refused, in the form and again on the write path, since you can hand-edit the file too:
anthropic-api,posit-ai,ms-foundry,amazon-bedrock,snowflake-cortex,openai-api,google,google-cloud,deepseek-api,databricks), which ai-config's own name rules let through.positron-custom-provider, our shared provider's own id.Name an entry after a built-in and you don't just break that entry. It takes over the built-in's row in our provider, validator, and callback maps,
mintCustomProviderIdthrows,build-catalogdoesn't catch it, and the catalog build fails, so you'd lose every provider in the list rather than the one you named badly. (openai-compatiblewas already refused as a built-in id.)One thing we can't save you from: write
githuboranthropic-apistraight into the file by hand and the entry shows up here but comes back with no models. That's a bug on the Assistant side and it's filed. Our guard keeps those names out of the form, not out of the file.Release Notes
New Features
Validation Steps
@:assistant
This needs to be tested with the changes in https://github.com/posit-dev/assistant/pull/2170 and posit-dev/ai-lib#71. Best to use the
Positron (with Assistant)launch config with these PRs checked out locally.providers["openai-compatible"].~/.posit/ai/providers.jsonwith the modal open: add an entry, set"enabled": false, set it back. The list should follow without reopening, and your comments and other entries should survive a save from the UI.anthropic-api. It should be refused with a message.test coverage
30 new extension-host tests in
extensions/authentication, and 35 new or reworked Vitest tests. By user setup rather than by file:ollama,aws, ...)endpointinstead ofbaseUrlstill reports it"enabled": falseon an entryproviders.jsonwith the modal open