-
Notifications
You must be signed in to change notification settings - Fork 962
feat(cli/sdk/mcp): add workspaces update for renaming #4189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
11c52ca
7fc930c
30df2ec
d3f10b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import { CLIError, positional, string } from "@superset/cli-framework"; | ||
| import { command } from "../../../lib/command"; | ||
|
|
||
| export default command({ | ||
| description: "Update a workspace", | ||
| args: [positional("id").required().desc("Workspace UUID")], | ||
| options: { | ||
| name: string().desc("Workspace name"), | ||
| }, | ||
| run: async ({ ctx, args, options }) => { | ||
| const id = args.id as string; | ||
| const organizationId = ctx.config.organizationId; | ||
| if (!organizationId) { | ||
| throw new CLIError("No active organization", "Run: superset auth login"); | ||
| } | ||
|
|
||
| if (options.name === undefined) { | ||
| throw new CLIError("No fields to update", "Pass --name <new-name>"); | ||
| } | ||
|
|
||
| const updated = await ctx.api.v2Workspace.update.mutate({ | ||
| id, | ||
| name: options.name, | ||
| }); | ||
|
|
||
| return { | ||
| data: updated, | ||
| message: `Updated workspace ${id}`, | ||
| }; | ||
| }, | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,20 @@ | ||||||||||||||||||||||||||
| import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; | ||||||||||||||||||||||||||
| import { z } from "zod"; | ||||||||||||||||||||||||||
| import { createMcpCaller } from "../../caller"; | ||||||||||||||||||||||||||
| import { defineTool } from "../../define-tool"; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| export function register(server: McpServer): void { | ||||||||||||||||||||||||||
| defineTool(server, { | ||||||||||||||||||||||||||
| name: "workspaces_update", | ||||||||||||||||||||||||||
| description: | ||||||||||||||||||||||||||
| "Update fields on an existing workspace. At least one field is required.", | ||||||||||||||||||||||||||
| inputSchema: { | ||||||||||||||||||||||||||
| id: z.string().uuid().describe("Workspace UUID."), | ||||||||||||||||||||||||||
| name: z.string().min(1).optional().describe("New workspace name."), | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| handler: async (input, ctx) => { | ||||||||||||||||||||||||||
| const caller = createMcpCaller(ctx); | ||||||||||||||||||||||||||
| return caller.v2Workspace.update(input); | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
|
Comment on lines
+15
to
+18
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/mcp-v2/src/tools/workspaces/update.ts
Line: 15-18
Comment:
No client-side guard against an all-optional payload. The CLI throws "No fields to update" before touching the network; without the same check here the MCP handler forwards `{ id }` to the server and returns an opaque cloud error instead of a clear message. Adding a pre-flight check mirrors the CLI's behaviour.
```suggestion
handler: async (input, ctx) => {
const { id, ...fields } = input;
if (Object.keys(fields).length === 0) {
throw new Error("No fields to update. Pass at least one field such as name.");
}
const caller = createMcpCaller(ctx);
return caller.v2Workspace.update(input);
},
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -58,6 +58,25 @@ export class Workspaces extends APIResource { | |||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||
| * Update fields on a workspace. At least one field is required. Currently | ||||||||||||||||||||||||||||||
| * only `name` is exposed — branch and host moves require host-side | ||||||||||||||||||||||||||||||
| * orchestration and aren't safe to set directly. | ||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||
| * Mirrors `superset workspaces update`. | ||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||
| update( | ||||||||||||||||||||||||||||||
| id: string, | ||||||||||||||||||||||||||||||
| params: WorkspaceUpdateParams, | ||||||||||||||||||||||||||||||
| options?: RequestOptions, | ||||||||||||||||||||||||||||||
| ): APIPromise<WorkspaceUpdateResult> { | ||||||||||||||||||||||||||||||
| return this._client.mutation<WorkspaceUpdateResult>( | ||||||||||||||||||||||||||||||
| "v2Workspace.update", | ||||||||||||||||||||||||||||||
| { id, ...params }, | ||||||||||||||||||||||||||||||
| options, | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||
| * Delete a workspace by id. Looks up the host the workspace lives on (via | ||||||||||||||||||||||||||||||
| * the cloud index) and routes the delete to that host's service through | ||||||||||||||||||||||||||||||
|
|
@@ -179,6 +198,26 @@ export interface WorkspaceCreateResult { | |||||||||||||||||||||||||||||
| alreadyExists: boolean; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| export interface WorkspaceUpdateParams { | ||||||||||||||||||||||||||||||
| /** New workspace name. */ | ||||||||||||||||||||||||||||||
| name?: string; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
Comment on lines
+201
to
+204
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/sdk/src/resources/workspaces.ts
Line: 201-204
Comment:
Empty-object update compiles cleanly but fails at runtime. `WorkspaceUpdateParams` has only optional fields, so `client.workspaces.update(id, {})` passes the TypeScript compiler and then hits the cloud with nothing to change, returning a server-side BAD_REQUEST. A `RequireAtLeastOne` constraint makes the invalid call a compile-time error rather than a silent runtime failure.
```suggestion
/** Requires at least one field — an all-optional update is rejected by the server. */
export type WorkspaceUpdateParams = RequireAtLeastOne<{
/** New workspace name. */
name?: string;
}>;
/** Utility: at least one key of T must be present. */
type RequireAtLeastOne<T> = {
[K in keyof T]-?: Required<Pick<T, K>> & Partial<Omit<T, K>>;
}[keyof T];
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| export interface WorkspaceUpdateResult { | ||||||||||||||||||||||||||||||
| id: string; | ||||||||||||||||||||||||||||||
| name: string; | ||||||||||||||||||||||||||||||
| branch: string; | ||||||||||||||||||||||||||||||
| organizationId: string; | ||||||||||||||||||||||||||||||
| projectId: string; | ||||||||||||||||||||||||||||||
| hostId: string; | ||||||||||||||||||||||||||||||
| type: "main" | "worktree"; | ||||||||||||||||||||||||||||||
| createdByUserId: string | null; | ||||||||||||||||||||||||||||||
| taskId: string | null; | ||||||||||||||||||||||||||||||
| createdAt: Date; | ||||||||||||||||||||||||||||||
| updatedAt: Date; | ||||||||||||||||||||||||||||||
| txid: number; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| export interface WorkspaceDeleteResult { | ||||||||||||||||||||||||||||||
| [key: string]: unknown; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
@@ -193,6 +232,8 @@ export declare namespace Workspaces { | |||||||||||||||||||||||||||||
| WorkspaceAgentLaunch, | ||||||||||||||||||||||||||||||
| WorkspaceCreateAgentResult, | ||||||||||||||||||||||||||||||
| WorkspaceCreateResult, | ||||||||||||||||||||||||||||||
| WorkspaceUpdateParams, | ||||||||||||||||||||||||||||||
| WorkspaceUpdateResult, | ||||||||||||||||||||||||||||||
| WorkspaceDeleteResult, | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reject blank workspace names before calling the API.
Line 17 only checks
undefined, so empty/whitespace values can still be sent at Line 23. Add a trimmed non-empty check for better CLI-side validation.Proposed patch
📝 Committable suggestion
🤖 Prompt for AI Agents