From 00cefa2a57894672daa3011d62676dcee9ec37bd Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Thu, 3 Sep 2026 20:26:45 +0000 Subject: [PATCH] feat(mcp): approve multiple tools in one invocation (C146) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mcp tools approve` took exactly one tool id, so approving a toolset meant one process, token fetch and TLS handshake per tool. It now takes one or more ids as variadic positional args and sends one request per id in a single process (the API has no batch approve — the id is in the path and the request message is singular). One confirmation line per tool; a failure doesn't stop the rest, and the command exits non-zero (classified from the first failure) if any fail. Backward compatible: a single id behaves exactly as before. The id stays positional (pluralized to ...), scope ids stay flags, so this extends the id-positional convention rather than moving to a flag. The single-resource guard test excludes approve; TestMcpToolsApproveIsMultiIDPositional pins the new contract, and CLAUDE.md documents the pluralize-the-positional case. Live-validated on a tenant: two ids -> two APPROVED confirmations in one run; one id -> unchanged; one real + one bogus id -> real approved, bogus reported, exit 2 (400 classification), other tool still attempted. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 8 +++ CLAUDE.md | 6 +- README.md | 4 +- cmd/args_positional_test.go | 35 ++++++++++- cmd/mcp_tools_approve.go | 98 +++++++++++++++++++++-------- cmd/mcp_tools_approve_batch_test.go | 46 ++++++++++++++ 6 files changed, 166 insertions(+), 31 deletions(-) create mode 100644 cmd/mcp_tools_approve_batch_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 97e340b..745680b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,14 @@ to follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Changed +- **`c1i mcp tools approve` now takes one or more tool ids.** Approving a + toolset meant one invocation — a fresh process, token and TLS handshake — + per tool; `approve id1 id2 id3` now does them in a single process. The API + has no batch approve, so each id is still its own request (one confirmation + line each, failures don't stop the rest, non-zero exit if any fail), but the + round-trip and startup cost drops sharply. Backward compatible: a single id + behaves exactly as before. + - **Fixtures and command documentation now use placeholder identifiers**, and a test keeps them that way: it rejects tenant-copied object ids, tenant hostnames, and prose naming the tenant an observation came from. diff --git a/CLAUDE.md b/CLAUDE.md index e4fd1da..39c5420 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -95,7 +95,11 @@ tracked root file outside its allowlist. Stage explicit paths rather than `-A`. `deny`, `comment`, `set-owner`, `resync-tools`, `source`, `usage`, …): the resource's own id is positional (`Use: "get "`, `cobra.ExactArgs(1)`, read via `args[0]`); parent ids stay flags - (`--app-id`, `--connector-id` when it is a *parent*). Don't validate the + (`--app-id`, `--connector-id` when it is a *parent*). An action that + applies the *same* mutation to several instances pluralizes that + positional rather than moving to a flag (`mcp tools approve ...`, + `cobra.MinimumNArgs(1)`, loop over `args`) — the id stays positional, + scope ids stay flags. Don't validate the positional id with `requireNonEmpty` — `cobra.ExactArgs` enforces presence; match the flat commands (`users get `). **Presence is not non-emptiness:** `ExactArgs(1)` accepts `""`, `"/"`, or `"."`, and each diff --git a/README.md b/README.md index 4fdba44..2d14e5d 100644 --- a/README.md +++ b/README.md @@ -366,7 +366,7 @@ c1i mcp servers connections list [--page-size N] [--limit N] c1i mcp tools list --app-id --connector-id [--page-size N] [--page-token TOKEN] [--limit N] c1i mcp tools get --app-id --connector-id c1i mcp tools search --app-id --connector-id [--query ] [--state ...] [--classification ...] [--page-size N] [--limit N] -c1i mcp tools approve --app-id --connector-id [--state approved|disabled|pending] +c1i mcp tools approve ... --app-id --connector-id [--state approved|disabled|pending] c1i mcp tools delete --app-id --connector-id c1i mcp tools history --app-id --connector-id [--page-size N] [--limit N] @@ -393,7 +393,7 @@ c1i mcp gateway call [--args '{"k":"v"}'] [--gateway-url ] **Auth for `register` / `update-credentials`:** convenience flags cover the simple methods — `--auth none`, `--auth bearer-token --bearer-token TOKEN`, `--auth custom-header --header-name NAME --header-value VALUE`, `--auth basic-auth --basic-auth-username USER --basic-auth-password PASS`. For OAuth2 / AWS SigV4 / Google service-account auth, pass the full config object via `--hosted-config-file` / `--external-config-file` (JSON file, or `-` for stdin) — generate a ready-to-edit skeleton with `--print-config-template --auth [--type hosted]` instead of hand-writing it. Secrets are sealed server-side; reads only ever return `*_configured` booleans, never the values. `--token-sharing shared|per-user` sets the server's token-sharing mode (case-insensitive; `per_user`/`peruser` are also accepted). Per the register help, `per-user` is only valid with `oauth2` in authorization-code or passthrough mode, `bearerToken`, `customHeader`, or `basicAuth`. Note that a read-back can legitimately differ from what you sent: the backend may store a *resolved* OAuth2 grant such as `..._MODE_AUTHORIZATION_CODE` in place of the input mode, so that is a normal round-trip, not a bug. `--source-app-id` names the source app for a connector-backed HOSTED server. -`mcp tools approve` is the standard post-registration step: newly discovered tools (from `register` or `resync-tools`) start in `PENDING_REVIEW`, and an admin approves each one for the gateway to proxy calls. History endpoints return records newest-first. +`mcp tools approve` is the standard post-registration step: newly discovered tools (from `register` or `resync-tools`) start in `PENDING_REVIEW`, and an admin approves them for the gateway to proxy calls. It takes one or more tool ids — the API has no batch approve, so each id is a separate request, but one invocation covers a whole toolset (pipe `mcp tools search --app-id --connector-id --state pending --fields id | jq -r .id`). History endpoints return records newest-first. **`mcp gateway`** closes the configure-then-verify loop: after registering a server and approving its tools, `list-tools` runs the MCP handshake against the live gateway and shows what's actually callable, and `call` invokes a tool and prints its result. The gateway URL defaults to the `-mcp` host derived from `--url` (e.g. `acme.conductor.one` → `acme-mcp.conductor.one/v1`); override with `--gateway-url`. Your standard C1 token is accepted by the gateway, so no extra auth is needed. `call` always prints the full result, but exits `7` (not `0`) when the result itself reports `isError: true` — the call succeeded, the tool didn't. diff --git a/cmd/args_positional_test.go b/cmd/args_positional_test.go index 6bba6df..70d21b4 100644 --- a/cmd/args_positional_test.go +++ b/cmd/args_positional_test.go @@ -270,7 +270,9 @@ func TestMigratedSingleResourceCommandsUsePositionalID(t *testing.T) { {path: []string{"mcp", "servers", "catalog", "get"}, retiredFlag: "catalog-id"}, {path: []string{"mcp", "tools", "get"}, retiredFlag: "id", mustKeepFlag: []string{"app-id", "connector-id"}}, - {path: []string{"mcp", "tools", "approve"}, retiredFlag: "id", mustKeepFlag: []string{"app-id", "connector-id"}}, + // "approve" is deliberately NOT here: it takes one OR more ids (batch + // approval), so it accepts a 2nd positional. Its contract is pinned by + // TestMcpToolsApproveIsMultiIDPositional instead. {path: []string{"mcp", "tools", "delete"}, retiredFlag: "id", mustKeepFlag: []string{"app-id", "connector-id"}}, {path: []string{"mcp", "tools", "history"}, retiredFlag: "id", mustKeepFlag: []string{"app-id", "connector-id"}}, @@ -336,6 +338,37 @@ func TestMigratedSingleResourceCommandsUsePositionalID(t *testing.T) { } } +// TestMcpToolsApproveIsMultiIDPositional pins C146: approve is the one +// migrated action that takes MORE than one id (batch approval), so unlike its +// single-resource siblings it accepts a 2nd positional. The rest of the +// migration contract still holds: the id stays positional (no --id flag) and +// the scope ids stay flags. +func TestMcpToolsApproveIsMultiIDPositional(t *testing.T) { + cmd := findCommand(t, rootCmd, "mcp", "tools", "approve") + + spec, err := parseUsePositionals(cmd.Use) + if err != nil { + t.Fatalf("%v", err) + } + if spec.required != 1 || spec.optional != 0 { + t.Errorf("Use %q should document a required positional, got required=%d optional=%d", cmd.Use, spec.required, spec.optional) + } + if argsAccepts(cmd, 0) { + t.Error("approve accepted 0 args; at least one tool id is required") + } + if !argsAccepts(cmd, 1) || !argsAccepts(cmd, 2) { + t.Error("approve must accept one OR more tool ids (batch approval)") + } + if hasFlag(cmd, "id") { + t.Error("the tool id must be positional-only, not an --id flag") + } + for _, f := range []string{"app-id", "connector-id"} { + if !hasFlag(cmd, f) { + t.Errorf("scope flag --%s must remain registered", f) + } + } +} + // TestCollectionCommandsKeepFlagIDsAndNoPositional guards against // over-correction: collection/create/relationship commands were NOT part of // the id->positional migration (they don't address one existing resource by diff --git a/cmd/mcp_tools_approve.go b/cmd/mcp_tools_approve.go index 5a9abf9..efba112 100644 --- a/cmd/mcp_tools_approve.go +++ b/cmd/mcp_tools_approve.go @@ -9,19 +9,28 @@ import ( ) var mcpToolsApproveCmd = &cobra.Command{ - Use: "approve ", - Short: "Approve an MCP tool (sets state=APPROVED)", - Long: `Approve an MCP tool by setting its state to MCP_TOOL_STATE_APPROVED. + Use: "approve ...", + Short: "Approve one or more MCP tools (sets state=APPROVED)", + Long: `Approve MCP tools by setting each tool's state to MCP_TOOL_STATE_APPROVED. This is the standard tool-approval workflow: newly discovered tools land in MCP_TOOL_STATE_PENDING_REVIEW and admin approval moves them to APPROVED so the MCP gateway will proxy calls to them. +Pass one or more tool ids. The API has no batch approve, so each id is sent as +its own request and prints its own confirmation line; if one fails the rest are +still attempted and the command exits non-zero. All ids share --app-id, +--connector-id and --state, so a selector pipes straight in: + + c1i mcp tools approve $(c1i mcp tools search --app-id A --connector-id C \ + --classification read --state pending --fields id | jq -r .id) \ + --app-id A --connector-id C + Use --state to target a different lifecycle state (disabled, pending). The approve command is a thin wrapper around the underlying Update RPC; use "c1i api --path=..." directly if you need to update other fields (display name, classification, visibility).`, - Args: cobra.ExactArgs(1), + Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { baseURL, err := GetBaseURL() if err != nil { @@ -30,7 +39,6 @@ classification, visibility).`, appID, _ := cmd.Flags().GetString("app-id") connectorID, _ := cmd.Flags().GetString("connector-id") - id := args[0] state, _ := cmd.Flags().GetString("state") if state == "" { state = "MCP_TOOL_STATE_APPROVED" @@ -44,41 +52,77 @@ classification, visibility).`, return &usageError{fmt.Errorf("--state=removed is system-managed by sync; use \"mcp tools delete\" to soft-delete a tool, or --state=disabled to block it")} } - body := map[string]any{ - "tool": map[string]any{ - "id": id, - "appId": appID, - "connectorId": connectorID, - "state": state, - }, - "updateMask": "state", + pathFor := func(id string) string { + return client.Path("/api/v1/apps/%s/connectors/%s/mcp_tools/%s", appID, connectorID, id) + } + bodyFor := func(id string) map[string]any { + return map[string]any{ + "tool": map[string]any{ + "id": id, + "appId": appID, + "connectorId": connectorID, + "state": state, + }, + "updateMask": "state", + } } - path := client.Path("/api/v1/apps/%s/connectors/%s/mcp_tools/%s", appID, connectorID, id) if dryRunActive() { - return printDryRun(cmd, "POST", path, body) + for _, id := range args { + if err := printDryRun(cmd, "POST", pathFor(id), bodyFor(id)); err != nil { + return err + } + } + return nil } c, err := newClient(cmd, baseURL) if err != nil { return fmt.Errorf("authentication failed: %w", err) } - data, err := c.Post(cmd.Context(), path, body) - if err != nil { - return fmt.Errorf("API error: %w", err) - } - var resp struct { - Tool struct { - ID string `json:"id"` - State string `json:"state"` - } `json:"tool"` + out := cmd.OutOrStdout() + approveOne := func(id string) error { + data, err := c.Post(cmd.Context(), pathFor(id), bodyFor(id)) + if err != nil { + return fmt.Errorf("API error: %w", err) + } + var resp struct { + Tool struct { + ID string `json:"id"` + State string `json:"state"` + } `json:"tool"` + } + if err := json.Unmarshal(data, &resp); err != nil { + return fmt.Errorf("failed to parse response: %w", err) + } + _, _ = fmt.Fprintf(out, "Updated tool: id=%s state=%s\n", resp.Tool.ID, resp.Tool.State) + return nil } - if err := json.Unmarshal(data, &resp); err != nil { - return fmt.Errorf("failed to parse response: %w", err) + + var firstErr error + failed := 0 + for _, id := range args { + if err := approveOne(id); err != nil { + failed++ + if firstErr == nil { + firstErr = err + } + // One line per id only when batching; a single id's error is + // the returned one, so printing it here too would double it. + if len(args) > 1 { + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "tool %s: %v\n", id, err) + } + } } - _, _ = fmt.Fprintf(cmd.OutOrStdout(), "Updated tool: id=%s state=%s\n", resp.Tool.ID, resp.Tool.State) + if firstErr != nil { + if len(args) == 1 { + return firstErr + } + // Wrap the first failure so exit-code classification still fires. + return fmt.Errorf("approved %d of %d tools; %d failed: %w", len(args)-failed, len(args), failed, firstErr) + } return nil }, } diff --git a/cmd/mcp_tools_approve_batch_test.go b/cmd/mcp_tools_approve_batch_test.go new file mode 100644 index 0000000..b866c48 --- /dev/null +++ b/cmd/mcp_tools_approve_batch_test.go @@ -0,0 +1,46 @@ +package cmd + +import ( + "bytes" + "strings" + "testing" +) + +// TestMcpToolsApproveBatchesOnePostPerId pins C146: `approve` takes one or +// more tool ids and sends one request per id (the API has no batch approve). +// Before the fix, ExactArgs(1) rejected a second id outright. +func TestMcpToolsApproveBatchesOnePostPerId(t *testing.T) { + resetRootURLFlag(t) + resetRootDryRunFlag(t) + // approve's own --app-id/--connector-id live on the shared command object; + // clear them before and after so this test neither inherits nor leaks a + // Changed flag (the C182 order-dependence class). + resetCmds(t, mcpToolsApproveCmd) + t.Cleanup(func() { resetCmds(t, mcpToolsApproveCmd) }) + t.Setenv("C1I_URL", "") + withDryRun(t) + + var out bytes.Buffer + rootCmd.SetOut(&out) + rootCmd.SetErr(&out) + t.Cleanup(func() { rootCmd.SetOut(nil); rootCmd.SetErr(nil) }) + rootCmd.SetArgs([]string{ + "mcp", "tools", "approve", "tool-a", "tool-b", "tool-c", + "--app-id", "app-x", "--connector-id", "conn-y", + "--dry-run", "--url", "acme.conductor.one", + }) + if err := rootCmd.ExecuteContext(t.Context()); err != nil { + t.Fatalf("approve of three ids errored: %v", err) + } + + got := out.String() + for _, id := range []string{"tool-a", "tool-b", "tool-c"} { + want := "[dry-run] POST /api/v1/apps/app-x/connectors/conn-y/mcp_tools/" + id + if !strings.Contains(got, want) { + t.Errorf("missing preview for %s;\nwant substring %q\ngot:\n%s", id, want, got) + } + } + if n := strings.Count(got, "[dry-run] POST"); n != 3 { + t.Errorf("got %d POST previews, want 3 (one per id)", n) + } +}