Split the legacy-positron-enforced layer behind an independent loader option - #47
Merged
Conversation
…PositronEnforcedSettings loader option
wch
approved these changes
Aug 6, 2026
wch
left a comment
wch
left a comment
Collaborator
There was a problem hiding this comment.
Looks good to me. Just some small findings from automated review:
-
important (simplification) —
packages/ai-config/src/legacy-positron-settings/sources.ts:42: The shared factory renames the two public options toreader/enforcedSettings, so bothload-catalog.ts:58andwatch-catalog.ts:58must duplicate the same adapter object and?? falsepolicy. Accepting the public-shaped fields (legacyPositronSettings?,legacyPositronEnforcedSettings?) and passing each caller'soptsdirectly would delete both copies and make this factory the actual single source of truth for which option enables which layer. -
minor (clarity) —
packages/ai-config/src/node/watch-catalog.ts:8: Several source-level API comments still describelegacyPositronSettingsas the opt-in for the legacy layers, despite the new independent flag (watch-catalog.ts:39,node/types.ts:107, andsrc/index.ts:132repeat the old reader-only wording). Update these comments alongside the README/memory-bank edits so maintainers are not told that the reader still controls both layers.
… directly; update stale reader-only comments
melissa-barca
added a commit
to posit-dev/positron
that referenced
this pull request
Aug 7, 2026
…alogs (#15375) Fixes #15377 Copied over from fork contributed by @khusmann in #15371 Companion to posit-dev/ai-lib#47 (splits the `legacy-positron-enforced` layer behind an independent `legacyPositronEnforcedSettings` flag) — **depends on it**; the `ai-lib` submodule pin points at its tip (`52f2f5f`), so bump the pin to the merged sha if ai-lib squashes. The assistant-extension adoption is posit-dev/assistant#1996. ## Problem The providers.json migration (#15121) copies the legacy provider settings — `authentication.*` connection/credential settings, provider enablement toggles, and model overrides — into providers.json but deliberately does not clear them (old builds and settings sync still read the legacy channel). Every catalog site also passes a `legacyPositronSettings` reader, which folds those same settings back in **below** providers.json — so clearing a migrated providers.json value silently resurrects the stale settings.json value it was copied from. Users have to clear the setting in both places to actually clear it. ## What this does This Positron always has the migration, so its catalogs stop reading user-set legacy settings entirely and keep only the admin-enforced channel: - **Both `createConfigurationLegacySettingsReader` helpers are deleted** — the configuration-service one in `platform/positronAiProvider/node/aiProviderCatalog.ts` (used by the shared-process and server catalog sites) and the extension-host one in `extensions/authentication/src/providerCatalog.ts`. `AiProviderCatalog`'s and `ProviderCatalogOptions`' `legacyPositronSettings` members go with them. - **Every catalog site passes `legacyPositronEnforcedSettings: true`** (hardcoded inside `AiProviderCatalog.loadOptions()` and the auth extension's `loadCatalog`/`initProviderCatalog`), so `POSITRON_ENFORCED_SETTINGS` keeps applying above the user file, below the canonical `POSIT_AI_PROVIDERS_ENFORCED`. Tests still control the payload via the existing `envVars` seam. - **The migration is untouched** — it keeps using ai-config's shared translator with its own `MigrationSettingsReader`. The stale "order matters" rationale on `migrateSettingsAndPrimeCatalog` (the catalog no longer reads the keys the AWS/Snowflake migrations write) is rewritten; those keys now reach the catalog only via the providers.json migration + the catalog's file watch. - **Pin bump fallout:** none — the new pin (ai-lib main + the flag split) changes no API this repo consumes besides the loader options. **Accepted behavior change**: user-set legacy settings that the auto-migration skipped — populated-providers.json users, and workspace-scoped legacy settings (never migrated; no providers.json equivalent) — stop applying. The manual "Migrate AI Provider Settings to providers.json" command covers the global case. ### Release Notes #### New Features - N/A #### Bug Fixes - Clearing an AI provider setting in `providers.json` no longer silently falls back to the old `authentication.*` value the migration copied it from. Legacy user settings are no longer read by the provider catalogs; the `POSITRON_ENFORCED_SETTINGS` admin channel still applies. ### Validation Steps Automated (run on this branch): - `npx vitest run src/vs/platform/positronAiProvider/` — 10 passed. `configurationLegacySettingsReader.vitest.ts` is deleted; the pass-through block collapses to one test pinning that `POSITRON_ENFORCED_SETTINGS` applies above the user file with no reader wiring. - `npx tsc -p src/tsconfig.json --noEmit` and `npx tsc -p extensions/authentication/tsconfig.json` — clean. - `npm run test-extension -- -l authentication` — 188 passing, 0 failures. Includes the reworked pins: `providerCatalog.test.ts` ("POSITRON_ENFORCED_SETTINGS applies above the user file without any reader wiring") and `activationOrdering.test.ts` ("the primed catalog does not read legacy `authentication.*` settings" — fails if anyone re-adds a reader; hermetic via `envVars: {}` so ambient `AWS_PROFILE`/`AWS_REGION` can't leak in). Manual (run in a dev Positron): 1. **Cleared values stay cleared** ✓: with a legacy `authentication.<provider>.baseUrl` set, migrated it to providers.json, then cleared the value in providers.json → the stale legacy value did NOT resurface in the catalog (on `main` it falls back). 2. **Enforced channel still live** ✓: launched with `POSITRON_ENFORCED_SETTINGS` carrying a provider key → it still beats the user's providers.json. --------- Co-authored-by: Kyle Husmann <kyle.husmann@posit.co>
This was referenced Aug 7, 2026
melissa-barca
pushed a commit
to posit-dev/positron
that referenced
this pull request
Aug 7, 2026
Follow-up to #15375: its `ai-lib` submodule pin (`52f2f5f`) is the pre-squash tip of the posit-dev/ai-lib#47 branch, which is not reachable from ai-lib `main` — it resolves today only because the PR branch still exists, and deleting that branch would break fresh recursive clones and CI (submodule fetches don't pull `refs/pull/*`). This bumps the pin to the merged squash sha `d04e8c674`, which additionally picks up posit-dev/ai-lib#47's review refactor (behavior-neutral) and ai-lib#48 (Kimi K3 context length). ### Release Notes #### New Features - N/A #### Bug Fixes - N/A ### Validation Steps - `npm run build:ai-lib` at the new pin, `npx vitest run src/vs/platform/positronAiProvider/` (10 passed), and `npx tsc -p extensions/authentication/tsconfig.json` — clean.
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.
Companion PRs (land after this one): assistant posit-dev/assistant#1996, positron posit-dev/positron#15375 (supersedes #15371). Both pin their submodule at this branch's tip — bump the pins to the merged sha if this squashes.
Problem
The
legacyPositronSettingsloader option enables both legacy layers at once:legacy-positron(user-set legacy settings, belowuser) andlegacy-positron-enforced(POSITRON_ENFORCED_SETTINGS, aboveuser). Positron ≥ 2026.08 migrates the legacy provider settings —authentication.*connection/credential settings, provider enablement toggles, and model overrides — into providers.json (posit-dev/positron#15121) without clearing them (old builds and settings sync still read the legacy channel), so on migrated hosts the reader layer resurrects stale values: clear a migrated providers.json setting and the old settings.json value silently comes back. Users have to clear both places. But the enforced admin channel must keep working there — and dropping the reader today drops both.What this does
Each layer becomes an independent opt-in on
LoadCatalogOptions(and thereforeWatchCatalogOptions):legacyPositronSettings(existing reader)legacy-positronlayer nowlegacyPositronEnforcedSettings(newboolean)legacy-positron-enforcedlayer: unset/false → off,true→ oncreateLegacyPositronSourceProviders(loader-internal,sources.ts) takes{ reader?, enforcedSettings }and stays the single assembly point, so the load and watch paths can't diverge on which options enable which layers.enforced > legacy-positron-enforced > env > user > legacy-positron > default.Who passes what after the companion PRs: Positron core catalogs (always ≥ 2026.08) pass only the flag; the assistant extension passes the flag on any Positron plus the reader only on pre-migration Positron (< 2026.08); every other host passes neither.
Testing
npm run build,check-types(incl. shape guard),lint,format:checkall clean.npm test: ai-config 284 passed (new pins: reader alone does not enable the enforced layer; flag alone folds the enforced layer into the load path without a reader; flag folds it into watch-path rebuilds), ai-credentials 134 passed.main(1c47e7e) inopenai-explicit-prompt-caching-wire.test.ts— reproduced on a clean checkout, unrelated to this change.legacyPositronEnforcedSettings: true, clearing a migrated providers.json value no longer falls back to the stale legacy setting, andPOSITRON_ENFORCED_SETTINGSstill applies above the user file.