Managed inference: make the toggle actually govern OpenRouter routing - #246
Merged
Aayam Bansal (aayambansal) merged 25 commits intoAug 3, 2026
Merged
Conversation
…catalog A first URL segment that is not a project selector was treated as a legacy base64 directory. Plenty of opaque tokens decode cleanly into binary junk, which the server then resolved against its own cwd and registered as a real project — leaving phantom entries on the home list. Reject anything that cannot be an absolute path, and guard the server side with Project.assertDirectory (400 ProjectDirectoryError). The same stale link then broke the app persistently. Project.resolve mapped every read failure to "absent", so a torn file or transient fs error became a 410; 410 is cacheable by default and the API sent no Cache-Control, so a browser cached one 410 for /provider and answered every later request from its own cache — through restarts and reloads, with the server seeing no traffic. Mark JSON responses no-store, and only treat a genuine NotFound as absent. Downstream, a failed project-scoped catalog load emptied the provider store, which reads as "my API keys vanished". The catalog belongs to the install, so fall back to it; refresh it explicitly after a key change or OAuth sign-in rather than waiting on the event stream; and bound the in-flight share with a timeout so a hung request cannot pin the key and stop every later refresh.
Lock is an in-process map, so it orders writers inside one process and nothing at all between processes — and several openscience processes share this directory routinely (a CLI run alongside a running server; Project.fromDirectory rewrites a record on every instance creation). A plain write truncates in place, so a reader in another process can observe a half-written file and fail to parse it. Write to a unique temp path and rename, so every reader sees either the old record or the new one.
--ro-bind-try only tolerates a missing source. The destination is a mount
point bwrap has to create, and everything above it is bound read-only, so
masking a credential file the user has never created aborted the whole
sandbox ("Can't create file at ...: Read-only file system") before the
command ran — which killed every terminal on a machine without a
credentials.json. A file that is not there has nothing to leak.
Covered by a test that runs the produced argv through real bwrap, since the
failure was in bwrap's acceptance of the arguments, not in their shape.
Bun.spawn inherits the environment the test runner was launched with, not the one preload.ts assembles at import time — so a child booting the CLI resolved XDG paths against the developer's real home and wrote projects, sessions and auth into it. Those showed up as phantom entries on their home list. Route child spawns through a fixture that always forwards the live process.env.
A flat accent stroke around a field reads as a validation error, and the caret already says "you are typing here" — the field only has to be found, not flagged. --focus-lit stacks four layers into one falloff (a highlight on the top edge, a hairline, then two widening washes) so a focused field looks lit rather than outlined, leaving the hard accent border to mean something went wrong. Fields whose box is the glyphs themselves keep an outline for its outline-offset, the one way CSS holds a ring off the box, with the shadow as the spill past it; those also get padding so the caret does not start on the edge. Composers and TextField light their own frame on :focus-within, so the control inside stays dark rather than drawing a second, clipped inner ring. Restated for the in-app theme toggle, which prefers-color-scheme cannot see.
…own key billing.llm="managed" previously changed nothing for OpenRouter: the own-key branch in CUSTOM_LOADERS.openrouter always won by key presence alone, so a stored BYOK key kept billing the user directly instead of routing through the Atlas managed proxy on their prepaid credits. - Gate the own-key branch on an explicit managed opt-in; byok and auto-detect (billing.llm unset/null) are unchanged, and the stored key is retained, never deleted or rewritten. - Add "managed" to the provider source enum and report it truthfully from the managed-proxy branch, even when an earlier stage (env/api) already registered the provider under a different source. - Stop the config-provider loop from unconditionally re-stamping source: "config" over a credential genuinely supplied by an earlier stage; a provider that appears in config.provider only for its whitelist is no longer mislabeled. Also widens three narrower "source" literal unions elsewhere in the type graph (Inference.classify, the plugin SDK's ProviderContext, and the generated Provider type consumed by plugin.auth.loader) that would otherwise fail to typecheck against the new enum member.
…c's protected set Review fix round 1: - Inference.classify had no branch for providerSource "managed", so auto-detect users with a synced thk_ token and no own key (a route that is genuinely managed) reported "unknown" instead of "managed". provider.ts already set that source unconditionally; the gap was entirely in the downstream classifier. - Narrow the "load config" loop's protected-source set to env/api/ managed, dropping "custom". "custom" is loader-assigned rather than credential-derived, so an autoloaded provider (Bedrock, google-vertex, synsci, cloudflare-ai-gateway, gitlab, sap-ai-core, ...) that also appears in config.provider for its whitelist must keep resolving to "config", matching pre-existing behaviour. - Strengthen the byok regression-guard test with a genuine config.provider.openrouter entry, so it actually exercises the config loop instead of passing vacuously. - Sync the generated SDK artifacts (tooling/sdk/js/src/v2/gen, tooling/sdk/openapi.json) via the real generator instead of a hand-edit, and drop an unneeded type widening in tooling/plugin.
…k on key add settings/billing.ts's PUT wrote the config but never busted the provider cache, so a managed/byok switch needed a restart to take effect. Worse, Config.updateGlobal's own cache-bust (Instance.disposeAll()) is fired without being awaited, so even after invalidating Provider's cache the next Config.get() for an already-open project could still read the pre-write value. Await the disposal explicitly before returning so the toggle is guaranteed visible to the very next request. Also, adding a real OpenRouter key via auth.set while Managed spend is on now flips billing.llm to byok server-side (so `openscience auth login` and the Settings UI behave the same), using the newly-exported Provider.isAtlasApiKey to tell a user-owned key apart from an Atlas thk_ token.
…al at the source Correction to the previous commit: the "openscience auth login gets the same behaviour as the browser flow" claim was false. The CLI calls Auth.set(...) directly (cli/cmd/auth.ts) and never goes through PUT /auth/:providerID, so gating the flip in that route only ever covered the Settings UI. Move the managed -> byok flip into Auth.set itself, the one choke point both paths actually share, and drop it from the route handler. isAtlasApiKey is duplicated locally in auth/index.ts rather than imported from Provider: provider.ts already imports both Auth and Config, so Auth importing Provider would open a second, larger cycle through a much heavier module. Also fixes the visibility gap at its root instead of patching call sites: Config.updateGlobal (and setMcp/setProvider/setSandbox/ unsetGlobal/replaceGlobal, which share the same global-config write path) fired Instance.disposeAll() without awaiting it, so a global write was not reliably visible to the very next Config.get() for an already-instantiated project directory. All of these now await a shared disposeGlobalInstances() helper before returning; the two call-site disposeAll() awaits added in the previous commit are gone. Billing PUT also re-invalidates the provider cache after setBillingMode/syncServices, not just before: syncServices writes fresh credentials into process.env, and the first invalidate() ran before that write existed to observe. Test changes: replaced test/server/auth-set-billing-flip.test.ts (exercised the route only, encoding the same false CLI-parity claim) with test/auth/billing-flip.test.ts, calling Auth.set directly. Added a visibility-guarantee test in test/config/config.test.ts, reverted unrelated quote-style reformatting in managed-routing.test.ts, widened global-config-candidate cleanup to all three filenames (openscience.jsonc/openscience.json/config.json) in the tests that touch it, and cleared OPENSCIENCE_CONFIG_DIR in test/preload.ts so a developer's real config directory can never be touched by the suite.
…-config test Finishes fix round 2: Auth.isAtlasApiKey is now the canonical predicate (auth/index.ts), and provider.ts's 5 remaining isAtlasApiKey call sites route through it instead of the deleted Provider.isAtlasApiKey export. Also adds the billing-flip test N1 needed: an OpenRouter key added while the global config is malformed (unparseable JSONC) must still persist the key and must not throw, since the billing.llm flip in Auth.set is wrapped in try/catch specifically to degrade gracefully there.
Provider["source"] gained a fifth "managed" variant in Task 1/2, which left two Settings surfaces uncompiled and one silently wrong: - ProviderKeys.tsx's SOURCES lookup didn't cover "managed", so a managed-routed provider had no row shape (compile error). - ManagedInference.update() never refreshed the provider catalog after a mode switch, so the row below it kept showing the pre-switch route until a reload. - model-settings-popover.tsx's inferenceSource() didn't accept the widened credential union (compile error) and had no branch for it, so the trigger chip rendered nothing for a route the backend can now prove is managed. Relabeled the chip from the placeholder "managed" to "Atlas credits" to match the register of the existing BYOK / Codex subscription labels.
Review finding: the mode-switch ordering (billing write resolves and its data is applied before refreshProviders() runs, and the refresh never runs on a failed write) had no test, so a later edit hoisting the refresh or dropping the await would regress silently into the exact stale-row bug this task closed. Extract the sequence into commitBilling(write, apply, refresh), a small exported function parameterized over its side effects, and unit test it with plain async stand-ins asserted on call order — no SDK/backend mocking needed.
…pped await Re-review caught that every refresh stand-in in the round-1 test ran its push synchronously the instant it was called, whether or not the caller awaited it, so a dropped `await refresh()` in commitBilling passed the suite unchanged. Give refresh a real macrotask hop (setTimeout) before recording, so the order assertion only sees "refresh" when commitBilling genuinely waited for it, and add a refresh-rejection propagation test for the other consequence of a dropped await (an unhandled rejection instead of reaching .catch(fail)). Verified by mutation: dropping the await in commitBilling now fails both tests; reverting restores a clean pass. commitBilling itself is unchanged.
publish() stages every write as a sibling `<target>.<pid>.<uuid>.tmp` inside the record directory, but Storage.list globbed `**/*` there and stripped a fixed 5 characters off each hit assuming ".json". A staging file caught in the write->rename window — or left behind forever by a writer that died in between, since nothing sweeps them — became a phantom key whose Storage.read throws NotFoundError. Project.list(), Session.list()/children(), KernelRegistry.restoreSession and settings/memory-index all read through it. Match `**/*.json` rather than relocating the staging path: relocating only prevents future debris, while publish-by-rename has already shipped and any crashed writer's leftovers are still enumerable. Filtering also makes list()'s slice(0, -5) correct by construction instead of by assumption.
…aged opt-in Task 1a gated only the OpenRouter loader's own-key branch. When billing.llm is "managed" but no managed credential can be found — a lapsed Atlas session, no thk_ in env — the loader returns headers with no credential, but the earlier "load apikeys" stage has already stamped provider.key from auth.json and getSDK picks it up with baseURL falling back to public OpenRouter. The user keeps chatting on their own key while the toggle reads "Managed" and the wallet is never touched. Add the managed mirror of the existing byok guard: under an explicit managed opt-in, drop any provider whose effective credential is a BYOK key. Seeing no OpenRouter models is honest; silent own-key spend is not. Exempt the two classes this file already treats as BYOK-by-design and deliberately keeps in managed mode — the user's ChatGPT subscription and local endpoints (Ollama's config block carries `apiKey: "local"`, which isByokKey classes as BYOK). Auto-detect and byok cannot reach the branch. Also correct the loader comment, which claimed an explicit managed opt-in "always wins... the key is retained but goes unused". It does not always win.
….llm Auth.set flips billing.llm managed -> byok when a real provider key is added. ProviderKeys.save() then calls refreshProviders(), which refreshes the CATALOG only, so the row below repaints to "local file / remove" while the toggle above still shows Managed selected and highlighted. No focus event fires in the same window, so the panel stayed self-contradictory until reload. refreshProviders is already the shared choke point every credential change goes through, on both the explicit call and the global.disposed event, so hang a subscriber list off it and let ManagedInference re-read billing there. Also make refreshProviders' error handling match its own docblock. It promised errors surface — "a silent failure here looks exactly like 'the key was never saved'" — while catching everything into console.error, so the error UI in ProviderKeys and CodexConnection could never fire. Errors now propagate; the global.disposed handler, which has no awaiting caller to surface into, keeps an explicit catch so the change adds no unhandled rejection. Listeners fire in a finally: the server-side flip already happened, so a failed catalog reload must not leave the toggle stale.
Last hand-rolled startsWith("thk_") after Auth.isAtlasApiKey became canonical.
The case-insensitivity it added is not load-bearing and is worth losing: Atlas
mints thk_ lowercase, and every other call site is case-sensitive, so keeping
this one wider would let `openscience models` label a key "managed" that the
router treats as BYOK. The baseURL check is a separate signal — a stale synced
*_BASE_URL can point at the Atlas proxy while the key is not a thk_ token — and
is kept, now with a comment saying so.
Cleanup was commented out and leaked a temp directory per fixture, filling /tmp and producing a false EDQUOT failure spike. `path` is the realpath of a directory the fixture created and every consumer uses `await using`; no test needed the escape hatch, and `dispose` is still there for one that does.
isUserProviderConnection was added upstream to keep a misleading row out of the Provider keys list: before the routing fix, an OpenRouter row could not say whether the user's key or the wallet was paying, so any non-`api` source under managed billing was hidden. The backend now reports the Atlas-proxied route as source "managed" and the panel labels it "Managed by Atlas", so the row describes itself honestly and is what the reader came for. Widen the source union to match the provider enum and admit that one case; every other branch is untouched, and none of the existing assertions change.
The suppression rule meant to keep a framed field's control dark used a descendant combinator with an input/textarea on the right, but the composer's editable is the contenteditable div itself and carries data-component="prompt-input" — an element is not its own descendant, so it matched nothing and the editable painted the unframed-field halo underneath the lit form, on a box with a different radius and bounds. Match contenteditables as descendants of the elements that actually light (.workspace-composer / .g-composer / the TextField root / a [data-focus-frame] wrapper), and drop prompt-input from that frame list since it is the control, not the frame. message-part's custom question input declared the lit frame without !important, so the app-wide halo won and --focus-lit there was dead code. That field frames itself — the border and radius are its own — so it takes the frame treatment; give it the weight it needs to render, outline included, or the halo's offset ring survives on top of it.
Config.updateGlobal emits global.disposed, the SPA refetches GET /provider the instant it sees it, and Provider.state() only rebuilds when the directory or trust changes — neither of which a config write touches. Instance.disposeAll() does not reach that module-level memo, so a refetch landing before the caller's Provider.invalidate() re-memoised the pre-write map with nothing left to invalidate it afterwards. Move the invalidation into disposeGlobalInstances, between the disposal and the emit, so it covers every global write — replaceGlobal and the patchConfigPath branch behind setMcp/setProvider/setSandbox/unsetGlobal had no invalidation at any call site. Reached by dynamic import because provider.ts imports Config, the same cycle-break models.ts already uses. billing.ts loses its now-redundant first invalidate; the post-syncServices one stays, since that leg drops a cache rebuilt from pre-sync env. server.ts keeps its own: Auth.set writes the auth file, which no config write covers.
…save refreshProviders now propagates errors instead of logging them, but the settings panels still wrapped it in the same try/catch as the credential write. A refresh rejection — reachable since inflight-cache rejects any load after 30s and the catalog is megabytes — rendered a completed OAuth sign-in as "sign-in failed", skipped onConnected, and put an error banner over an already-cleared key input for a key that is on disk. credentialChange splits the two: the write decides success, the refresh can only add a notice. The failure stays visible — swallowing it was the older bug — but worded outcome-first and without claiming the operation failed. Also isolate notifyAfter's subscribers from each other. A listener that threw escaped the finally, replaced the body's outcome with its own and skipped every later listener, turning a successful refresh into exactly the rejection above.
commitBilling's refresh rejection was falling straight into ManagedInference's bare .catch(fail), so a billing-mode switch that saved fine but hit a catalog reload failure painted an undifferentiated error banner. Track whether apply() landed and, if so, report the refresh problem with outcome-first wording instead, matching the split credentialChange already draws for CodexConnection/ProviderKeys. disposeGlobalInstances() awaited Provider.invalidate() unguarded, unlike the Instance.disposeAll().catch(() => undefined) directly above it. A throw during provider-module init would reject every global config write after the file was already on disk. Make the invalidate best-effort and log the failure instead.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…panel An earlier commit admitted source "managed" into the panel so the row could label itself. Hiding it is cleaner: the panel is about connections the reader set up, and a wallet route is not one — it belongs to the account, has no key to manage, and nothing there is actionable. This restores the upstream filter's behaviour for the case it could not previously express, now that the backend reports "managed" instead of leaving a wallet route indistinguishable from a key the user brought. Which credential is paying is still visible where it is useful: the routing chip on the model itself. The SOURCES entry stays for exhaustiveness over Provider["source"].
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
2 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.
What does this PR do?
Makes the Settings → Models "Managed inference" toggle actually govern OpenRouter routing.
Selecting Managed did nothing.
CUSTOM_LOADERS.openrouterresolved purely by key presence — its own comment stated the rule: "the user's OWN OpenRouter key wins… Deleting the own key restores the managed route automatically." So withbilling.llm = "managed"and a key inauth.json, calls went to public OpenRouter on the user's own key while the UI said Managed and credits sat unused. Confirmed on a live install:The byok direction was already enforced (providers on a
thk_token are dropped under an explicit byok opt-in); there was no mirror for managed. This adds one.Three defects combined to make it inescapable:
config.billing.llm.source: "config"on anything named inconfig.provider— and the Atlas sync writesprovider.openrouter.whitelist. So the UI showedconfig/ "set in openscience.json" and hid the remove button for a key that lives inauth.jsonand is removable — the exact escape hatch the loader's comment told you to use.Now:
sourcethk_managedmanagedapinull)apiThe panel is about connections the reader set up; a wallet route is not one, so it stays out. Which credential is actually paying is shown where it is useful — as the routing chip on the model itself in the picker.
Mode and key changes both take effect without a restart. The auto-flip lives in
Auth.set, the one choke pointopenscience auth loginand the HTTP route share, so terminal and UI behave identically. Under an explicit Managed opt-in with no managed credential available, the provider is now dropped rather than silently falling back to the user's own key.Also fixed along the way (each independently reviewed):
Project.resolvemapped every read failure to "absent", so a torn file became a 410; 410 is cacheable and the API sent noCache-Control, so a browser answered every later/providerrequest from its own cache — through restarts and reloads, with the server seeing no traffic. This reads as "my API keys vanished."Config.updateGlobalnever reliably published its writes. The cache-bust was fire-and-forget, so a write was not visible to the nextConfig.get(). Also affectedsetMcp/setProvider/setSandbox/unsetGlobal/replaceGlobal.Storage.listso they cannot surface as phantom keys.test/preload.tsoverrodeXDG_CONFIG_HOMEbut never clearedOPENSCIENCE_CONFIG_DIR, whichGlobal.Path.configprefers.tmpdir()fixture cleanup was commented out, leaking ~14k temp directories and producing false mass-failure spikes when/tmpfilled.How did you verify your code works?
Every task was implemented by a fresh agent, reviewed adversarially, and fix-looped to clean; a whole-branch review followed, then a separate
/code-reviewpass. Findings that survived were fixed and re-verified by mutation — reverting each fix and watching the corresponding test fail.Measured against an
origin/mainbaseline run in a clean worktree:origin/mainbackend/clifrontend/workspaceturbo typecheckAll 4 failures are pre-existing and reproduce on
origin/main:npm selects every supported native package contract with lifecycle scripts disabled,spa fallback > browser navigation…, andkeeps the Files pane absent…. The extra one,killTree SIGKILLs a detached group, passes 3/3 standalone — a full-suite-order flake, verified independently of this branch.Manual: built locally (
bun run build --single) and exercised the flow — under Managed the OpenRouter row is absent and the picker's routing chip reads the wallet; pasting an own key flips the toggle to Own keys and the row appears without a reload; switching back to Managed hides it again with the stored key untouched inauth.json.Notes for the reviewer
mainhas since deleted. Another was reduced to its unique test, since its source fix is already upstream.isUserProviderConnectionwas added upstream as a frontend workaround for this bug: before the routing fix, an OpenRouter row could not say whether the user's key or the wallet was paying, so it hid any non-apisource under managed billing. Its intent is preserved — an Atlas-carried route stays hidden — but the case it keyed on (source: "config", billing: "managed") is now unreachable, since such a route reportssource: "managed". The filter simply covers the new value..tmpstaging files;fs.renameover a destination another process holds open fails on Windows;Instance.disposeAll's single-flight snapshot means the "visible to the very nextConfig.get()" guarantee is not airtight under concurrent global writes.Checklist
bun run typecheckpassesbun test(inbackend/cli) passes — modulo the pre-existing failures above, which reproduce onorigin/mainbunx prettier --checkis clean on all changed files