[multi-team P1b] Scope llms.rs BYO credential and icon reads to the window's team - #15461
Open
warp-agent-staging[bot] wants to merge 15 commits into
Open
Conversation
…team
Migrates the BYO *credential policy* getters onto per-window team scope, and
splits them from the plan entitlement they were tangled with.
- `is_managed_byok_byoe_enabled` is the workspace-level plan entitlement and
stays workspace-scoped; `are_member_byo_{keys,endpoints}_allowed_for_scope`
and `has_team_first_party_key_for_scope` read the scope's team's `team_byo`.
- Adds `UserWorkspaces::team_context_for_window`, the `WindowId` counterpart of
P0's `team_context`, for callers that hold a window rather than a view handle:
models owned by a window's view tree, and views still inside their own
constructor (which are not yet in `view_to_window`).
- Warp Agent settings resolves its scope per read and now also reacts to
`WindowTeamChanged`, not just `TeamsChanged`.
- Fixes a real gap: `RequestParams::new` gated member keys and endpoints on plan
entitlement alone and never consulted the team's `team_byo`, so an admin
restricting member credentials had no effect on outbound requests.
`apply_team_byo_policy` now strips member-provided credentials the requesting
window's team disallows, resolved fresh per request.
Retires the `WorkspaceSettings.team_byo` read behind the Warp Agent settings
page and behind agent requests. The field stays on the query and model: the
credential-source display in `llms.rs` and the TUI API-keys menu still resolve
through it.
`UserWorkspaces::update_workspaces` pushes telemetry and secret-redaction settings into `PrivacySettings`, so the rebind test's call to it panicked on the unregistered singleton.
Review found the one request path that went around the new enforcement. `spawn_request` computed `byo_allowed` from `is_byo_api_key_enabled` alone -- plan entitlement, no team policy -- and wrote the refreshed Grok OAuth token back into params `apply_team_byo_policy` had already stripped. The write was skipped only when `api_keys` was `None`, which is false whenever Bedrock or GEAP credentials are attached: exactly the managed deployment where an admin is most likely to have set a `team_byo` restriction. `begin_expired_grok_refresh` also kept the background refresh loop armed for a token the team disallows. `RequestParams::member_byo_credentials_allowed` now records the decision, set by `apply_team_byo_policy` and defaulting to `false` because `new` has no window to resolve a team from. `spawn_request` ANDs it into `byo_allowed`, which closes the re-injection and the background refresh together. Also corrects the departed-team claim in the scope-getter docs. No scope this module hands out reaches the `Some(_)` deny arm: `team_context_for_window` resolves the uid through `team_from_uid` before storing it, so a departed team is already indistinguishable from teamless. The arm exists for `TeamContextForOperation`, which captures a uid and can outlive its team, and which the new test uses.
Rebased onto #15441. P11 landed the seam this slice was reimplementing: the controller now owns a `TeamContextResolver` built from its surface's view handle, so it resolves its window's team live at the point of use. Drops this slice's own plumbing as superseded -- the `window_id` field, `set_window_id`, the extra constructor parameter at both call sites, and `TerminalView::on_window_transferred`. P11's resolver reads the framework's view-to-window mapping, which the transfer path already updates, so a pane dragged between windows is handled without a hook. `app/src/terminal/view.rs` and `crates/warp_tui/src/terminal_session_view.rs` are untouched by this slice as a result. `team_context_for_window` stays: the settings page reads policy during its own construction, when the view is not yet in `view_to_window` and a handle resolves to nothing.
Per Isaiah: the API contract should not incentivise passing raw ids around, so the exchange for a `TeamScope` is a view or a `ViewContext`. Replaces `team_context_for_window(WindowId)` with `team_context_for_view<T: Entity>(&ViewContext<T>)`, which reads `ctx.window_id()` internally so the id never appears in a signature. It still serves the case that motivated the helper: a view inside its own constructor is not yet in `view_to_window`, so a `WeakViewHandle` resolves to nothing there while `ViewContext::window_id` is valid throughout. Tests build scopes for bare windows rather than standing up a view for each, so they use a sanctioned `#[cfg(test)]` constructor, `team_context_for_window_for_test`, documented as existing precisely so the production contract holds. Both, and `team_context(handle, app)`, share one private resolution path. Also drops the departed-team story from the getter docs and tests. The backend data model does not allow a team to leave a workspace, so the scenario those docs described cannot occur; the reachable version is a transient, self-healing reconcile artefact that no getter should document a policy for. The test stays, because it is the only thing stopping either getter being "simplified" to `is_none_or` and silently inverting a restriction into inheritance -- but it now says it guards the getter's shape rather than a user scenario, and is renamed `member_byo_policy_denies_a_scope_naming_an_unresolvable_team`.
Per Isaiah: falling back to workspace settings when the user or the window is teamless is the intended behaviour. The teamless arm returned a hardcoded `true`, which is *more* permissive than the workspace setting may be -- a workspace that restricts member credentials was silently ignored. Moves the fallback into `team_byo_for_scope`, so all three getters get it at once and a teamless read is now identical to the ambient getters this slice replaces. A scope that names a team is unaffected: it reads that team's policy or nothing, never another team's. This closes the TUI enforcement gap this PR previously documented. A TUI window is never registered, so it resolves to no team and now reads the workspace's `team_byo` -- exactly what master reads today -- rather than being unrestricted. The test that pinned the gap becomes `apply_team_byo_policy_applies_the_workspace_policy_to_an_unregistered_window`, and the teamless case is now covered in both directions so a permissive constant cannot pass again. Also narrows `is_managed_byok_byoe_enabled` to `pub(crate)`; its callers are this module and the Warp Agent settings page.
#15448 (`61be7e3c`) added `register_window` to the TUI's root view, so "a TUI window is never registered" is no longer true and should not have gone into these docs. The fix it was justifying is unchanged and the reasoning survives: a scope with no team reads the workspace's `team_byo`, which is what the ambient getter it replaces read. An unregistered window is one route to a teamless scope, not the definition of one -- a registered window with no team selected is another -- so the docs and the test now name the fallback rather than the route, and the test is `apply_team_byo_policy_applies_the_workspace_policy_to_a_scope_with_no_team`.
…BYO API key entitlement
Contributor
Author
|
This PR was generated with Warp. Comment |
…solution, and team-switch repaint
…-custom rows and unknown-vs-teamless collapse for team_byo
…ck/Gemini Enterprise host-icon getters
1 task
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
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.


Description
Stacked on #15446. Migrates
app/src/ai/llms.rs's BYO credential and host-icon reads off ambient (workspace-level, "one arbitrarily-chosen team") state onto the requesting window's own team, per the multi-team client scoping effort.Half A —
are_member_byo_keys_allowed()/are_member_byo_endpoints_allowed()call sites inllms.rsmigrated to the existing_for_scopevariants.Half B —
is_using_team_first_party_key_for_provider/is_using_team_byo_endpoint_for_modelfolded into new scoped getters (first_party_key_source_for_provider_for_scope,byo_key_source_for_model) that readscope's team'steam_byoinstead of the workspace's arbitrarily-chosen one.Bedrock / Gemini Enterprise icons — added
is_aws_bedrock_credentials_enabled_for_scope/is_gemini_enterprise_credentials_enabled_for_scopeinuser_workspaces.rs(mirroring theteam_byo_for_scopepattern) and scopedshould_show_key_icon_for_model,should_show_bedrock_icon_for_model,should_show_gemini_enterprise_agent_platform_icon_for_modeltogether so the whole icon family shares one signature: aWeakViewHandle<T>rather than an already-resolved scope, since these are re-exported across thewarp_tuicrate boundary, which cannot name the crate-privateTeamScope/TeamContexttypes.Deliberately left ambient:
first_party_key_source_for_provider/is_using_first_party_key_for_providerandLLMPreferences::custom_inference_enabledstay ambient: they feed the model-catalog resolution core (AvailableLLMs::usable_info_for_id,get_active_base_model, etc.), which resolves once per app with no per-window path today. Documented in place; out of scope for this slice (owned by the catalog-core migration).crates/warp_tui/src/api_keys_menu.rs's Grok OAuth policy gate (start_grok_oauth) still calls the ambientare_member_byo_keys_allowed():TuiApiKeysMenuModelhas no window/view handle to resolve a scope from, unlikeTuiModelMenuModel. Documented on the ambient getter's doc comment as the one remaining caller.Changes
app/src/ai/llms.rs: scoped BYO key/endpoint and icon getters; aViewContext-basedshould_show_key_icon_for_model_for_viewfor callers still under construction.app/src/workspaces/user_workspaces.rs:has_team_byo_endpoint_for_scope,is_aws_bedrock_credentials_enabled_for_scope,is_gemini_enterprise_credentials_enabled_for_scope, and supportingllm_settings_for_scope/host_settings_for_scope/is_host_available_for_scopehelpers.terminal/input/models/{data_source,view}.rs,terminal/input/common.rs,terminal/profile_model_selector.rs,ai/execution_profiles/model_menu_items.rs. Each subscribes toUserWorkspacesEvent::WindowTeamChanged(filtered to its own window) so an open/cached menu repaints when the window switches teams.crates/warp_tui/src/model_menu.rsadds a boxedModelCredentialIconResolver(built from aWeakViewHandle) soTuiModelMenuModeldoesn't need a generic parameter, plus the sameWindowTeamChangedrepaint subscription;terminal_session_view.rspasses the terminal surface's view handle through.Review round 1 (all four findings fixed, pushed at
2ff4c67f)CI note: the
warpbuild/test/clippy workflow only runs for PRs into the default branch. This PR targetsfactory/multi-team-p1-byo-credentials(#15446), notmaster, so it has never run those jobs, in draft or out of it -- the only checks that have run areCheck OWNERS approval,Verify PR base is the default branch, andverification/cla-signed. It will get a full CI run once the stack lands bottom-up and this PR is retargeted tomaster. Until then, every compile/test/clippy result in this description is a local run, not a CI check; see Testing below for exact commands and counts.teamless_scope—team_scope_for_view's fallback for a view that cannot resolve a window (not yet attached, or an unregisteredwarp_tuisession) minted a scope naming no team, whichteam_byo_for_scopethen resolved to the workspace's copy — one arbitrarily-chosen team's policy for a user who does have a team, exactly the bug this migration removes. Fixed by removingteamless_scope/team_scope_for_viewentirely:should_show_key_icon_for_model,should_show_bedrock_icon_for_model,should_show_gemini_enterprise_agent_platform_icon_for_model, and thedata_source.rs/profile_model_selector.rscall sites now report no icon/no BYO source when the scope cannot be resolved, instead of fabricating one. A resolved window with genuinely no team (0-team workspace, or reconciled onto none) is unaffected and still reads the workspace'steam_byo, matching [multi-team P1] Rework: require a team scope on the member BYO key/endpoint getters #15446's own tests for that case.WeakViewHandle(not yet inview_to_window), so the initialInlineModelSelectorViewquery andProfileModelSelector::refresh_model_menu's custom-model key icon read (finding 1's fix, applied literally) as "no icon" during construction. ForProfileModelSelector, addedshould_show_key_icon_for_model_for_view, which resolves from the liveViewContext(ViewContext::window_idis valid throughout construction) instead of a handle — no deferral needed, and it is now used unconditionally inrefresh_model_menu. ForInlineModelSelectorView, the sync data source only has anAppContext, so the initialmixer.run_queryis deferred one tick (ctx.spawn(std::future::ready(()), ...)) until the view is registered. Regression test:should_show_key_icon_for_model_for_view_resolves_the_constructing_views_own_windowconstructs two views (each in its own constructor) on opposing teams and asserts each reads its own team's policy.TuiModelMenuModel,ProfileModelSelector, andInlineModelSelectorViewall cache credential/host icon state and never re-derived it onUserWorkspacesEvent::WindowTeamChanged. All three now subscribe, filtered to their own window, and refresh/requery when it fires. Regression test (TUI):open_menu_repaints_when_its_own_window_switches_teamopens a menu, switches an unrelated window's team (no repaint), then its own window's team (repaint), asserting via emitted-event counts.should_show_key_icon_for_model_reads_teamless_for_an_unregistered_windowassertedfalsefrom a fixture wherefalsewas also what a hardcoded stub or the (pre-fix) ambient leak would have produced, since neither a BYO key nor the workspace'steam_byowas configured. Replaced withshould_show_key_icon_for_model_reports_no_icon_while_the_views_window_is_unknown, which configures a permissiveteam_byoand a real registered key so the wrong (workspace-copy) answer and the correct (no policy) answer differ, and asserts the correct one.Before fixing finding 1, I verified it against the production path rather than assuming it was a fixture problem — see the
byo_key_source_for_model_follows_each_windows_own_teamnote below, which is the same discipline applied here: findings 2 and 4's regression tests were checked to fail without their corresponding fix (temporarily reverted, confirmed red, restored) before being counted as done.Review round 2 (both findings fixed, pushed at
4ea83062)model_menu_items.rswere stillWeakViewHandle-based.make_item_fields(the shared row builder behindavailable_model_menu_items, used for every non-custom/server model) minted its ownWeakViewHandleviactx.handle()for all three icon reads — the same defect round 1's finding 2 fixed forProfileModelSelector's custom-model row, just not carried to the row builder every server model actually goes through. BecauseProfileModelSelector::newreaches this during construction, every server-model row lost its host/key icons on first render, and a BYO-usableRequiresUpgrademodel stayed disabled. Fixed by switchingmake_item_fieldsto the_for_viewvariants, passing the already-availablectxdirectly instead of a minted handle. Regression test:available_model_menu_items_resolves_the_constructing_views_own_window_bedrock_policy(new filemodel_menu_items_tests.rs) constructs two views, each in its own constructor, on opposing Bedrock policies, and asserts the resolved leading icon differs; verified by temporarily reverting to thectx.handle()version and confirming the assertion failed (Some(Agent)instead of the expectedSome(Aws)), then restoring the fix.should_show_key_icon_for_model_for_viewreintroduced round 1's ambient leak via a different path. Itsteam_context_for_viewcall resolves viateam_context_for_window_id, which collapses two different states into the sameteam_uid: None: a window absent fromwindow_team_uids(unknown -- e.g. awarp_tuiwindow whose workspaces-metadata response hasn't landed yet) and a window with an explicitSome(None)entry (registered, and genuinely on no team).byo_key_source_for_modelthen reads the workspace'steam_byofor both, which is correct for the second case but is round 1's original bug for the first: one arbitrarily-chosen team's BYO policy for a user who does have a team. Fixed by addingUserWorkspaces::is_window_known(private) plus two newpub(crate)accessors,team_context_if_knownandteam_context_for_view_if_known, that returnNonefor the unknown case andSome(TeamContext)only once a window has an entry at all.should_show_key_icon_for_model/should_show_key_icon_for_model_for_view(the twoteam_byoreaders) now go through the_if_knownpair.should_show_bedrock_icon_for_model(_for_view)and the Gemini Enterprise equivalent deliberately stay on the plain, always-succeedingteam_context/team_context_for_view:llm_settings's workspace-level fallback is the admin's org-wide host toggle, not another team's data, so those two don't haveteam_byo's ambient-leak problem and don't need the guard. Regression tests:should_show_key_icon_for_model_for_view_reports_no_icon_for_a_window_unknown_to_user_workspacesandshould_show_key_icon_for_model_reports_no_icon_for_a_resolved_window_unknown_to_user_workspaces, each configured so the wrong (workspace-copy) and correct (no policy) answers differ; both verified red/green the same way as finding 1.User-visible consequence worth calling out explicitly (widened in round 3 below):
warp_tuiregisters a window's team on its first successful workspaces-metadata response (RootTuiView::new→register_window). Before this fix, a window that never got that response — offline, or a slow/failed initial fetch — fell into the same bucket as "registered, no team" and read the workspace'steam_byo, so a user with a valid BYO key would still see the key-connected badge while offline. After this fix, that same window correctly reads as unknown and shows no icon instead. This is the right trade — a missing affordance is cheaper than one derived from an arbitrary team's policy — but it is a real regression for that specific window (offline BYO users temporarily lose the badge), not a pure bugfix, and Isaiah should know that going in.Why
user_workspaces.rsgrew by three items:is_window_known,team_context_if_known, andteam_context_for_view_if_knownare new surface in a file that has taken on repeated, reinvented primitives across this stack. These three aren't that: they encode a distinctionTeamScope's own contract requires (absent-from-map vs. explicit-None) that no existing accessor exposes —team_context/team_context_for_viewfold both intoSome(TeamContext{team_uid: None}), which is the right answer for a registered teamless window and the wrong one for an unknown one. The alternative, forkingteam_context_for_viewitself or changing its return type, would have meant duplicatingteam_context_for_window_id's body or altering #15446's already-shipped, already-tested plain accessor out from under its other (correct) callers. Two narrowpub(crate)wrappers plus one private predicate keep the change scoped to exactly the getters that need the distinction, instead of adding another general-purpose primitive to the pile.Review round 3 (one finding, fixed, pushed at
fe46f2c9)_if_knownguard too, not an exemption from it. Round 2's finding 2 reasoned thatllm_settings's workspace-level fallback doesn't haveteam_byo's ambient-leak problem, soshould_show_bedrock_icon_for_model(_for_view)and the Gemini Enterprise equivalent were deliberately left on the plainteam_context/team_context_for_view. That reasoning was wrong:llm_settings_for_scope's no-team fallback iscurrent_workspace().settings.llm_settings-- the same one-arbitrarily-chosen-team's effective settings (GetEffectiveWorkspaceSettingsForWorkspaceserver-side) thatteam_byo_for_scopefalls back to, not a neutral admin-wide default. REV-2205's finding applies to the wholeWorkspaceSettings/TeamSettingsobject, not toteam_byoalone. So a fully resolved but unregistered window could display team A's Bedrock or Gemini host icon while its own team was genuinely unknown -- the identical defect the key icon had, on a different setting. Fixed by routingshould_show_bedrock_icon_for_model(_for_view)andshould_show_gemini_enterprise_agent_platform_icon_for_model(_for_view)throughteam_context_if_known/team_context_for_view_if_knownas well, matchingteam_byo. What does not change: a registered teamless window still reads the workspace fallback for host settings, exactly as it does forteam_byo-- the distinction is unknown-vs-known, not scoped-vs-unscoped, and that ruling stands. Regression tests, mirroring the twoteam_byounknown-window tests:should_show_bedrock_icon_for_model_reports_no_icon_for_a_resolved_window_unknown_to_user_workspaces,should_show_bedrock_icon_for_model_for_view_reports_no_icon_for_a_window_unknown_to_user_workspaces, andshould_show_gemini_enterprise_agent_platform_icon_for_model_reports_no_icon_for_a_resolved_window_unknown_to_user_workspaces(this last one specifically confirms the parallel Gemini implementation received the identical fix, not just an identical doc comment); all three verified red against the pre-fix code, then restored.Offline-TUI consequence, widened: round 2's callout above under-scoped the regression to the key icon. An offline/unregistered
warp_tuiwindow now loses the Bedrock and Gemini Enterprise host badges as well, for the same reason and by the same fix. This is more consistent, not less -- it would be strange for one icon family to keep guessing from an arbitrary team's policy while its neighbour declines to -- but the actual behavior is broader than first reported, so it's called out again here rather than left implicit in round 2's text.Testing
app/src/ai/llms_tests.rsandapp/src/workspaces/user_workspaces_tests.rspin: per-window team resolution for BYO keys/endpoints and Bedrock host credentials (two windows on opposing teams reading independently), the teamless-window fallback to the workspace's own settings, and the unregistered-window (unknown team) display-affordance fallback.byo_key_source_for_model_follows_each_windows_own_teaminitially failed while I was writing it, which I did not take at face value given what this PR touches. I isolated the two concerns with a throwaway diagnostic before concluding anything:are_member_byo_keys_allowed_for_scopecorrectly returnedtruefor the permissive team's window andfalsefor the restrictive team's, and both scopes'team_uid()matched the team each window was assigned — the scope resolution this PR adds is correct. The failure wasis_using_api_key_for_providerreturningfalse, gated on the workspace-levelbilling_metadata.tier.byo_api_key_policyentitlement flag (a pre-existing, deliberately non-team-scoped check, untouched by this PR) that the test fixture never set. Fixed by addingbyo_api_key_policy: Some(ByoApiKeyPolicy { enabled: true })to the fixture's team billing metadata. What the test proves now: with that entitlement satisfied, the permissive team's window reports the member key in use and the restrictive team's window on the same workspace with the same configured key does not../script/format --check,cargo clippy -p warp --all-targets --tests -- -D warnings, andcargo clippy -p warp_tui --all-targets --tests -- -D warningsall pass clean.cargo nextest run -p warp(6588/6592 pass) andenv -u WARP_API_KEY cargo nextest run -p warp_tui(1017/1017 pass -- theWARP_API_KEYenv var leaking into Clap'sapi_keyfield was the sole cause of the round-1 report's1016/1017; unsetting it gives a clean run with no code change needed).-p warp's remaining 4 failures are all pre-existing, with no code path through this PR's diff, and reproduce on this stack's base (4881d725):server::server_api::ai::tests::ambient_agent_headers_for_task_overrides_existing_cloud_agent_headerandterminal::input::decorations::tests::test_decorations_with_multibyte_charsandterminal::input::tests::test_histignorespace_support_in_zshare the three named in round 1;settings::cloud_preferences_syncer::tests::test_sync_local_pref_to_cloud_on_initial_sync_for_returning_userfailed only in the full-suite run and passed cleanly in isolation, touching nothing this PR changed -- consistent with this stack's documented runner-contention flakiness, noted here rather than omitted.warp_tuifailure along the way: see finding 2's write-up above formodel_menu::tests::provider_key_controls_key_connected_callout, whose test double needed to register its own window the wayRootTuiViewdoes in production.cargo nextest run -p warp(6592/6595 pass -- the three pre-existing failures named above;cloud_preferences_syncer's flake from the round-2 run did not reproduce this time, consistent with it being runner-contention rather than a real regression) andenv -u WARP_API_KEY cargo nextest run -p warp_tui(clean 1017/1017).