fix(workspace): address PR review findings and fix clippy errors

Fix bugs found during PR NVIDIA#2243 review: correct migration type strings,
resolve platform-scoped profile workspace handling, fix sandbox
workspace_tx single-fire, add missing workspace deletion blockers,
reset TUI row indices on workspace switch, fix TUI provider update
workspace, and update e2e Python tests for workspace and inference
route API changes. Add Phase 2 TODO comments for authorization gaps.
Fix pre-existing clippy errors (too_many_arguments, cast_sign_loss,
iter_on_single_items).

Signed-off-by: Derek Carr <decarr@redhat.com>

Author: derekwaynecarr

crates/openshell-cli/src/ssh.rs

@@ -956,6 +956,7 @@ fn resolve_file_download_target(
 /// Files are streamed as a tar archive to `ssh ... tar xf - -C <dest>` on
 /// the sandbox side.  When `dest` is `None`, files are uploaded to the
 /// sandbox user's home directory.
+#[allow(clippy::too_many_arguments)]
 pub async fn sandbox_sync_up_files(
     server: &str,
     name: &str,

crates/openshell-sandbox/src/lib.rs

@@ -2386,7 +2386,10 @@ async fn run_policy_poll_loop(ctx: PolicyPollLoopContext) -> Result<()> {
         } else {
             tokio::time::sleep(interval).await;
             match client.poll_settings(&ctx.sandbox_id).await {
-                Ok(result) => result,
+                Ok(result) => {
+                    let _ = ctx.workspace_tx.send(client.workspace());
+                    result
+                }
                 Err(e) => {
                     debug!(error = %e, "Settings poll: server unreachable, will retry");
                     continue;

crates/openshell-server/migrations/postgres/006_add_workspace_column.sql

@@ -2,9 +2,11 @@
 ALTER TABLE objects ADD COLUMN workspace TEXT NOT NULL DEFAULT '';
 
 -- Backfill workspace-scoped object types to 'default'.
+-- stored_provider_profile is intentionally omitted: profiles use workspace=""
+-- for platform scope and are created with an explicit workspace when scoped.
 UPDATE objects SET workspace = 'default'
   WHERE workspace = '' AND name IS NOT NULL
-  AND object_type IN ('sandbox', 'provider', 'service_endpoint', 'inference_route', 'ssh_session', 'provider_credential_refresh_state', 'policy', 'draft_chunk', 'sandbox_settings');
+  AND object_type IN ('sandbox', 'provider', 'service_endpoint', 'inference_route', 'ssh_session', 'provider_credential_refresh_state', 'sandbox_policy', 'draft_policy_chunk', 'sandbox_settings');
 
 -- Replace global name uniqueness with workspace-scoped uniqueness.
 DROP INDEX IF EXISTS objects_name_uq;

crates/openshell-server/migrations/sqlite/006_add_workspace_column.sql

@@ -2,9 +2,11 @@
 ALTER TABLE objects ADD COLUMN workspace TEXT NOT NULL DEFAULT '';
 
 -- Backfill workspace-scoped object types to 'default'.
+-- stored_provider_profile is intentionally omitted: profiles use workspace=""
+-- for platform scope and are created with an explicit workspace when scoped.
 UPDATE objects SET workspace = 'default'
   WHERE workspace = '' AND name IS NOT NULL
-  AND object_type IN ('sandbox', 'provider', 'service_endpoint', 'inference_route', 'ssh_session', 'provider_credential_refresh_state', 'policy', 'draft_chunk', 'sandbox_settings');
+  AND object_type IN ('sandbox', 'provider', 'service_endpoint', 'inference_route', 'ssh_session', 'provider_credential_refresh_state', 'sandbox_policy', 'draft_policy_chunk', 'sandbox_settings');
 
 -- Replace global name uniqueness with workspace-scoped uniqueness.
 DROP INDEX IF EXISTS objects_name_uq;

crates/openshell-server/src/grpc/mod.rs

@@ -249,6 +249,9 @@ impl OpenShell for OpenShellService {
 
     type WatchSandboxStream = ReceiverStream<Result<SandboxStreamEvent, Status>>;
 
+    // TODO(phase2): data-plane RPCs do not carry a workspace field. Add
+    // workspace verification to confirm the sandbox belongs to the caller's
+    // workspace before proxying.
     #[rpc_auth(auth = "bearer", scope = "sandbox:read", role = "user")]
     async fn watch_sandbox(
         &self,
@@ -265,6 +268,8 @@ impl OpenShell for OpenShellService {
         sandbox::handle_get_sandbox(&self.state, request).await
     }
 
+    // TODO(phase2): all_workspaces flag is currently accessible to any
+    // authenticated user. Restrict to Platform Admin role in Phase 2.
     #[rpc_auth(auth = "bearer", scope = "sandbox:read", role = "user")]
     async fn list_sandboxes(
         &self,
@@ -309,6 +314,7 @@ impl OpenShell for OpenShellService {
 
     type ExecSandboxStream = ReceiverStream<Result<ExecSandboxEvent, Status>>;
 
+    // TODO(phase2): no workspace field — see watch_sandbox comment.
     #[rpc_auth(auth = "bearer", scope = "sandbox:write", role = "user")]
     async fn exec_sandbox(
         &self,
@@ -320,6 +326,7 @@ impl OpenShell for OpenShellService {
     type ForwardTcpStream =
         Pin<Box<dyn tokio_stream::Stream<Item = Result<TcpForwardFrame, Status>> + Send + 'static>>;
 
+    // TODO(phase2): no workspace field — see watch_sandbox comment.
     #[rpc_auth(auth = "bearer", scope = "sandbox:write", role = "user")]
     async fn forward_tcp(
         &self,
@@ -340,6 +347,7 @@ impl OpenShell for OpenShellService {
 
     // --- SSH sessions ---
 
+    // TODO(phase2): no workspace field — see watch_sandbox comment.
     #[rpc_auth(auth = "bearer", scope = "sandbox:write", role = "user")]
     async fn create_ssh_session(
         &self,
@@ -364,6 +372,8 @@ impl OpenShell for OpenShellService {
         service::handle_get_service(&self.state, request).await
     }
 
+    // TODO(phase2): all_workspaces flag is currently accessible to any
+    // authenticated user. Restrict to Platform Admin role in Phase 2.
     #[rpc_auth(auth = "bearer", scope = "sandbox:read", role = "user")]
     async fn list_services(
         &self,
@@ -406,6 +416,8 @@ impl OpenShell for OpenShellService {
         provider::handle_get_provider(&self.state, request).await
     }
 
+    // TODO(phase2): all_workspaces flag is currently accessible to any
+    // authenticated user. Restrict to Platform Admin role in Phase 2.
     #[rpc_auth(auth = "bearer", scope = "provider:read", role = "user")]
     async fn list_providers(
         &self,

crates/openshell-server/src/grpc/policy.rs

@@ -2423,6 +2423,8 @@ pub(super) async fn handle_get_sandbox_logs(
     request: Request<GetSandboxLogsRequest>,
 ) -> Result<Response<GetSandboxLogsResponse>, Status> {
     let req = request.into_inner();
+    // TODO(phase2): workspace is resolved but not used for authorization.
+    // Verify the sandbox belongs to this workspace before returning logs.
     let _workspace =
         super::workspace::resolve_workspace(state.store.as_ref(), &req.workspace).await?;
     if req.sandbox_id.is_empty() {

crates/openshell-server/src/grpc/provider.rs

@@ -1333,7 +1333,8 @@ pub(super) async fn handle_list_provider_profiles(
 ) -> Result<Response<ListProviderProfilesResponse>, Status> {
     let request = request.into_inner();
     let workspace =
-        super::workspace::resolve_workspace(state.store.as_ref(), &request.workspace).await?;
+        super::workspace::resolve_profile_workspace(state.store.as_ref(), &request.workspace)
+            .await?;
     let limit = clamp_limit(request.limit, 100, MAX_PAGE_SIZE) as usize;
     let offset = request.offset as usize;
     let mut profiles = merged_provider_profiles(state.store.as_ref(), &workspace).await?;
@@ -1353,7 +1354,7 @@ pub(super) async fn handle_get_provider_profile(
 ) -> Result<Response<ProviderProfileResponse>, Status> {
     let req = request.into_inner();
     let workspace =
-        super::workspace::resolve_workspace(state.store.as_ref(), &req.workspace).await?;
+        super::workspace::resolve_profile_workspace(state.store.as_ref(), &req.workspace).await?;
     let id = req.id;
     let id = normalize_profile_id_request(&id)?;
     let profile = get_provider_type_profile(state.store.as_ref(), &workspace, &id)
@@ -1372,7 +1373,8 @@ pub(super) async fn handle_import_provider_profiles(
 ) -> Result<Response<ImportProviderProfilesResponse>, Status> {
     let request = request.into_inner();
     let workspace =
-        super::workspace::resolve_workspace(state.store.as_ref(), &request.workspace).await?;
+        super::workspace::resolve_profile_workspace(state.store.as_ref(), &request.workspace)
+            .await?;
     let (profiles, mut diagnostics) = profiles_from_import_items(&request.profiles);
     add_empty_profile_set_diagnostic(&profiles, &mut diagnostics);
     let _sandbox_sync_guard = state.compute.sandbox_sync_guard().await;
@@ -1438,7 +1440,8 @@ pub(super) async fn handle_update_provider_profiles(
 ) -> Result<Response<UpdateProviderProfilesResponse>, Status> {
     let request = request.into_inner();
     let workspace =
-        super::workspace::resolve_workspace(state.store.as_ref(), &request.workspace).await?;
+        super::workspace::resolve_profile_workspace(state.store.as_ref(), &request.workspace)
+            .await?;
     let items = request.profile.into_iter().collect::<Vec<_>>();
     let (profiles, mut diagnostics) = profiles_from_import_items(&items);
     add_empty_profile_set_diagnostic(&profiles, &mut diagnostics);
@@ -1553,7 +1556,8 @@ pub(super) async fn handle_lint_provider_profiles(
 ) -> Result<Response<LintProviderProfilesResponse>, Status> {
     let request = request.into_inner();
     let workspace =
-        super::workspace::resolve_workspace(state.store.as_ref(), &request.workspace).await?;
+        super::workspace::resolve_profile_workspace(state.store.as_ref(), &request.workspace)
+            .await?;
     let (profiles, mut diagnostics) = profiles_from_import_items(&request.profiles);
     add_empty_profile_set_diagnostic(&profiles, &mut diagnostics);
     diagnostics
@@ -1573,7 +1577,7 @@ pub(super) async fn handle_delete_provider_profile(
 ) -> Result<Response<DeleteProviderProfileResponse>, Status> {
     let req = request.into_inner();
     let workspace =
-        super::workspace::resolve_workspace(state.store.as_ref(), &req.workspace).await?;
+        super::workspace::resolve_profile_workspace(state.store.as_ref(), &req.workspace).await?;
     let id = req.id;
     let id = normalize_profile_id_request(&id)?;
     let _sandbox_sync_guard = state.compute.sandbox_sync_guard().await;
@@ -7086,9 +7090,7 @@ mod tests {
                 workspace: "default".to_string(),
             }),
             r#type: "claude".to_string(),
-            credentials: [("ANTHROPIC_API_KEY".to_string(), "sk-123".to_string())]
-                .into_iter()
-                .collect(),
+            credentials: HashMap::from([("ANTHROPIC_API_KEY".to_string(), "sk-123".to_string())]),
             config: HashMap::new(),
             credential_expires_at_ms: HashMap::new(),
             profile_workspace: "other-workspace".to_string(),
@@ -7113,9 +7115,7 @@ mod tests {
                 workspace: "default".to_string(),
             }),
             r#type: "claude".to_string(),
-            credentials: [("ANTHROPIC_API_KEY".to_string(), "sk-123".to_string())]
-                .into_iter()
-                .collect(),
+            credentials: HashMap::from([("ANTHROPIC_API_KEY".to_string(), "sk-123".to_string())]),
             config: HashMap::new(),
             credential_expires_at_ms: HashMap::new(),
             profile_workspace: String::new(),
@@ -7139,9 +7139,7 @@ mod tests {
                 workspace: "default".to_string(),
             }),
             r#type: "claude".to_string(),
-            credentials: [("ANTHROPIC_API_KEY".to_string(), "sk-123".to_string())]
-                .into_iter()
-                .collect(),
+            credentials: HashMap::from([("ANTHROPIC_API_KEY".to_string(), "sk-123".to_string())]),
             config: HashMap::new(),
             credential_expires_at_ms: HashMap::new(),
             profile_workspace: "default".to_string(),
@@ -7165,9 +7163,7 @@ mod tests {
                 workspace: "default".to_string(),
             }),
             r#type: "claude".to_string(),
-            credentials: [("ANTHROPIC_API_KEY".to_string(), "sk-123".to_string())]
-                .into_iter()
-                .collect(),
+            credentials: HashMap::from([("ANTHROPIC_API_KEY".to_string(), "sk-123".to_string())]),
             config: HashMap::new(),
             credential_expires_at_ms: HashMap::new(),
             profile_workspace: "default".to_string(),
@@ -7260,9 +7256,7 @@ mod tests {
                     workspace: "default".to_string(),
                 }),
                 r#type: "ws-custom".to_string(),
-                credentials: [("TOKEN".to_string(), "val".to_string())]
-                    .into_iter()
-                    .collect(),
+                credentials: HashMap::from([("TOKEN".to_string(), "val".to_string())]),
                 config: HashMap::new(),
                 credential_expires_at_ms: HashMap::new(),
                 profile_workspace: "default".to_string(),

crates/openshell-server/src/grpc/workspace.rs

@@ -15,13 +15,16 @@ use openshell_core::proto::{
     GetWorkspaceResponse, InferenceRoute, ListWorkspaceMembersRequest,
     ListWorkspaceMembersResponse, ListWorkspacesRequest, ListWorkspacesResponse, Provider,
     RemoveWorkspaceMemberRequest, RemoveWorkspaceMemberResponse, Sandbox, ServiceEndpoint,
-    SshSession, StoredProviderProfile, Workspace, WorkspaceMember, WorkspaceRole,
+    SshSession, StoredProviderCredentialRefreshState, StoredProviderProfile, Workspace,
+    WorkspaceMember, WorkspaceRole,
 };
 use prost::Message;
 use tonic::{Request, Response, Status};
 
 use crate::ServerState;
-use crate::persistence::{ObjectType, WriteCondition, current_time_ms};
+use crate::persistence::{
+    DRAFT_CHUNK_OBJECT_TYPE, ObjectType, POLICY_OBJECT_TYPE, WriteCondition, current_time_ms,
+};
 
 use super::{MAX_PAGE_SIZE, clamp_limit};
 
@@ -48,10 +51,30 @@ fn validate_workspace_name(name: &str) -> Result<(), Status> {
     super::validation::validate_dns1123_label(name, "workspace name")
 }
 
+/// Resolve a workspace for provider profile operations.
+///
+/// Provider profiles support a platform scope where `""` is a distinct,
+/// meaningful value (not an alias for `"default"`). This function preserves
+/// `""` as-is for platform-scoped operations. Non-empty workspace values are
+/// validated for existence via [`resolve_workspace`].
+pub async fn resolve_profile_workspace(
+    store: &crate::persistence::Store,
+    workspace: &str,
+) -> Result<String, Status> {
+    if workspace.is_empty() {
+        return Ok(String::new());
+    }
+    resolve_workspace(store, workspace).await
+}
+
 /// Resolve and validate a workspace name from a request field.
 ///
 /// Empty strings are normalized to `"default"`. The workspace must exist in the
 /// store; returns `NOT_FOUND` if it doesn't.
+///
+/// TODO(phase2): this only validates existence. Workspace membership enforcement
+/// (checking the caller is a member of the resolved workspace) is deferred to
+/// Phase 2.
 pub async fn resolve_workspace(
     store: &crate::persistence::Store,
     workspace: &str,
@@ -190,7 +213,15 @@ pub(super) async fn handle_delete_workspace(
         ));
     }
 
+    // TOCTOU: the blocker scan and the subsequent delete are not transactional,
+    // so a resource could be created between the check and the delete. The
+    // persistence layer does not currently offer a transaction primitive that
+    // spans both reads and deletes. Acceptable for now — workspace deletion is
+    // an admin-only, low-frequency operation.
     let mut blocking = Vec::new();
+    // Policy revisions and draft chunks are normally deleted when their parent
+    // sandbox is removed, and refresh state is transient; these checks are a
+    // safety net against regressions.
     for (object_type, label) in [
         (Sandbox::object_type(), "sandbox"),
         (Provider::object_type(), "provider"),
@@ -199,6 +230,9 @@ pub(super) async fn handle_delete_workspace(
         (InferenceRoute::object_type(), "inference route"),
         (SshSession::object_type(), "ssh session"),
         (super::policy::SANDBOX_SETTINGS_OBJECT_TYPE, "sandbox settings"),
+        (POLICY_OBJECT_TYPE, "sandbox policy"),
+        (DRAFT_CHUNK_OBJECT_TYPE, "draft policy chunk"),
+        (StoredProviderCredentialRefreshState::object_type(), "credential refresh state"),
     ] {
         let records = state
             .store

crates/openshell-server/src/persistence/mod.rs

@@ -140,6 +140,11 @@ pub fn generate_name() -> String {
 /// Decode a single [`ObjectRecord`] into a protobuf message, hydrating
 /// `resource_version` from the authoritative DB row.
 ///
+/// Only `resource_version` is hydrated here; `workspace` is NOT backfilled from
+/// the DB column because the workspace field is authoritative in the protobuf
+/// payload at creation time. This is a breaking upgrade — pre-workspace records
+/// will carry an empty workspace until they are re-created.
+///
 /// Extracted to avoid repeating the identical decode-and-hydrate block across
 /// `get_message`, `get_message_by_name`, `list_messages`, and
 /// `list_messages_with_selector`.

crates/openshell-server/src/persistence/postgres.rs

@@ -361,7 +361,7 @@ WHERE object_type = $1 AND workspace = $2 AND name = $3
         .fetch_one(&self.pool)
         .await
         .map_err(|e| map_db_error(&e))?;
-        Ok(row.0 as u64)
+        Ok(u64::try_from(row.0).unwrap_or(0))
     }
 
     pub async fn delete_all_in_workspace(