Stop leaking OAuth as a concept to the LLM

trungutt · trungutt · commit c3c85b346780 · 2026-05-29T11:14:01.000+02:00
Per @rumpl's review feedback on #2925: the LLM has no agency over the OAuth flow — it can't trigger it, grant it, or recover from it. Every OAuth-related string the catalog exposes to the model is a leaky abstraction, and the original bug (model stops and asks the user to repeat themselves after enabling an OAuth-protected server) is a direct consequence of leaking 'auth: OAuth — elicited on the next turn' in the enable result. This commit removes OAuth as a model-visible concept from every LLM-facing string in the catalog: - handleEnable: drop the per-auth-type switch. The success message is now a single line saying 'tools will appear on your next turn, proceed with the user's original request'. The only conditional emission is the WARNING for unresolved env vars, which is the one case the model has actual agency over (it can ask the user to set the variable and retry). - Instructions() workflow: step 2 drops the 'Authentication (OAuth or API key) is deferred' sentence; step 3 drops 'do not narrate the OAuth flow' (the model no longer learns OAuth exists, so the instruction can shrink to 'do not narrate connection setup'); step 5 reframes reset_remote_mcp_server_auth as 'if a previously enabled server starts rejecting requests, clear any persisted credentials' without mentioning OAuth tokens or authorization URLs. - Tool descriptions: enable_remote_mcp_server no longer mentions 'OAuth flow or API-key check'; reset_remote_mcp_server_auth no longer mentions 'OAuth credentials' or 'fresh authorization flow'. - ResetAuthArgs jsonschema and handleResetAuth result strings: drop every 'OAuth credentials' and 'fresh authorization flow' reference. Internal field names (oauthSuccessHandler, managedOAuth, etc.), Go package-doc comments, and slog debug messages still mention OAuth — those are developer-facing, not LLM-facing, and removing them would obscure intent for anyone reading the package. Tests updated: - TestEnableAddsServerAndFiresChangeNotification now asserts the result does NOT contain 'OAuth' or 'authorization', protecting against a regression where someone re-adds the leaky wording. - TestEnableAPIKeyEnvPresent no longer expects 'auth: API key' (the line is gone); still asserts no WARNING. - TestResetAuthClearsCredentials and TestResetAuthNoOpForNonOAuth updated for the new 'cleared credentials' / 'no persisted credentials' wording. The unresolved-header WARNING test still passes because the WARNING branch survived the rewrite under the unified 'len(missing) > 0' check.
diff --git a/pkg/tools/builtin/mcpcatalog/mcpcatalog.go b/pkg/tools/builtin/mcpcatalog/mcpcatalog.go
@@ -143,27 +143,22 @@ Workflow:
      Use any term related to the user's intent ("notion", "stripe",
      "docs", "search", "browser", …).
   2. Call ` + ToolNameEnable + ` with the server's "id" to activate it.
-     This adds the server's tools to your set on the *next* turn.
-     Authentication (OAuth or API key) is deferred — it is triggered when
-     the host enumerates the server's tools, which happens once enabling
-     completes. For api_key servers, make sure the listed env var(s) are
-     set in the user's shell BEFORE enabling, otherwise the server will
-     refuse the connection.
+     The server's tools appear in your set on the *next* turn; the host
+     handles any required connection setup. For api_key servers, make
+     sure the listed env var(s) are set in the user's shell BEFORE
+     enabling, otherwise the server will refuse the connection.
   3. On your NEXT turn (once the activated server's tools appear in
      your set), go straight to the user's ORIGINAL request using those
      tools. Do NOT stop and ask the user to repeat themselves; do NOT
-     narrate the OAuth flow or say things like "OAuth will happen on
-     the next turn" — if OAuth is needed the host surfaces the
-     authorization dialog and the tool call resumes once the user
-     grants access. Enabling a server is a means to an end, not a
-     stopping point.
+     narrate connection setup. Enabling a server is a means to an end,
+     not a stopping point.
   4. Call ` + ToolNameDisable + ` to remove a server when no longer needed.
      This tool only appears once at least one server is enabled.
-  5. If a previously authorized OAuth server starts rejecting requests
-     (token revoked, scopes changed, signed in to the wrong account),
-     call ` + ToolNameResetAuth + ` to wipe the persisted credentials.
-     The next enable will trigger a fresh authorization URL. This tool
-     also only appears once at least one server is enabled.
+  5. If a previously enabled server starts rejecting requests
+     (credentials revoked, scopes changed, signed in to the wrong
+     account), call ` + ToolNameResetAuth + ` to clear any persisted
+     credentials so the next enable starts fresh. This tool also only
+     appears once at least one server is enabled.
 
 Prefer enabling only the servers you actually need — every server adds
 tools to the prompt and contributes to context usage.`
@@ -315,7 +310,7 @@ func (t *Toolset) Tools(ctx context.Context) ([]tools.Tool, error) {
 		{
 			Name:         ToolNameEnable,
 			Category:     "mcp_catalog",
-			Description:  "Activate a remote MCP server from the catalog by id. Connection (and any required OAuth flow or API-key check) is deferred until the host next lists the agent's tools.",
+			Description:  "Activate a remote MCP server from the catalog by id. Connection setup is deferred until the host next enumerates tools; the server's tools become available on the next turn.",
 			Parameters:   tools.MustSchemaFor[EnableArgs](),
 			OutputSchema: tools.MustSchemaFor[string](),
 			Handler:      tools.NewHandler(t.handleEnable),
@@ -352,7 +347,7 @@ func (t *Toolset) Tools(ctx context.Context) ([]tools.Tool, error) {
 			tools.Tool{
 				Name:         ToolNameResetAuth,
 				Category:     "mcp_catalog",
-				Description:  "Clear persisted OAuth credentials (access token, refresh token, dynamic-client-registration data) for a catalog server. The next enable will trigger a fresh authorization flow. No-op for api_key/none servers.",
+				Description:  "Clear any persisted credentials for a catalog server so the next enable starts a fresh connection. Use when a previously enabled server starts rejecting requests. No-op for servers with no persisted credentials.",
 				Parameters:   tools.MustSchemaFor[ResetAuthArgs](),
 				OutputSchema: tools.MustSchemaFor[string](),
 				Handler:      tools.NewHandler(t.handleResetAuth),
@@ -600,28 +595,17 @@ func (t *Toolset) handleEnable(ctx context.Context, args EnableArgs) (*tools.Too
 	}
 
 	msg := strings.Builder{}
-	fmt.Fprintf(&msg, "enabled %q (%s) — connection deferred until the host next enumerates tools.\n", id, server.Title)
+	fmt.Fprintf(&msg, "enabled %q (%s) — the server's tools will appear on your next turn. Proceed with the user's original request; the host handles any required connection setup.\n", id, server.Title)
 	fmt.Fprintf(&msg, "endpoint: %s\n", server.URL)
-	switch server.Auth.Type {
-	case "oauth":
-		msg.WriteString("auth: OAuth — the host will surface any required authorization (browser redirect or in-app dialog) the first time the server's tools are used, and resume the tool call once access is granted. On your next turn, continue with the user's original request; do not pause to announce the OAuth flow.\n")
-	case "api_key":
-		if len(missing) > 0 {
-			fmt.Fprintf(&msg, "auth: API key — WARNING: the following env vars are NOT set: %s. Set them, then call %s and %s for this id again, otherwise tool calls will fail.\n",
-				strings.Join(missing, ", "), ToolNameDisable, ToolNameEnable)
-		} else {
-			msg.WriteString("auth: API key — env vars present, ready to use.\n")
-		}
-	default:
-		if len(missing) > 0 {
-			// Headers reference env vars that didn't resolve, even though
-			// the catalog says no auth is required — surface it so the
-			// user is not surprised by a 401 on the first tool call.
-			fmt.Fprintf(&msg, "auth: none — WARNING: header(s) reference unresolved env var(s): %s. Set them, then call %s and %s for this id again.\n",
-				strings.Join(missing, ", "), ToolNameDisable, ToolNameEnable)
-		} else {
-			msg.WriteString("auth: none — ready to use.\n")
-		}
+	// Only surface unresolved env vars — the model can act on those by
+	// asking the user to set the variable and retrying. Every other auth
+	// detail (OAuth flow type, redirect URI, token persistence) is the
+	// host's concern and is intentionally not exposed to the model; the
+	// previous wording leaked "auth: OAuth — elicited on the next turn"
+	// and led the model to stop and ask the user to repeat themselves.
+	if len(missing) > 0 {
+		fmt.Fprintf(&msg, "WARNING: the following env var(s) are NOT set: %s. Ask the user to set them, then call %s and %s for this id again, otherwise tool calls will fail.\n",
+			strings.Join(missing, ", "), ToolNameDisable, ToolNameEnable)
 	}
 	return tools.ResultSuccess(msg.String()), nil
 }
@@ -768,7 +752,7 @@ func (t *Toolset) handleList(_ context.Context, _ ListArgs) (*tools.ToolCallResu
 
 // ResetAuthArgs is the input schema for reset_remote_mcp_server_auth.
 type ResetAuthArgs struct {
-	ID string `json:"id" jsonschema:"Catalog id of the server whose persisted OAuth credentials should be cleared."`
+	ID string `json:"id" jsonschema:"Catalog id of the server whose persisted credentials should be cleared."`
 }
 
 func (t *Toolset) handleResetAuth(ctx context.Context, args ResetAuthArgs) (*tools.ToolCallResult, error) {
@@ -779,7 +763,7 @@ func (t *Toolset) handleResetAuth(ctx context.Context, args ResetAuthArgs) (*too
 	}
 
 	if server.Auth.Type != "oauth" {
-		return tools.ResultSuccess(fmt.Sprintf("server %q uses %s auth — nothing to reset.", id, server.Auth.Type)), nil
+		return tools.ResultSuccess(fmt.Sprintf("server %q has no persisted credentials — nothing to reset.", id)), nil
 	}
 
 	// Stop and forget any live MCP toolset for this server. The active
@@ -809,15 +793,15 @@ func (t *Toolset) handleResetAuth(ctx context.Context, args ResetAuthArgs) (*too
 	}
 
 	if err := t.removeOAuthToken(server.URL); err != nil {
-		return tools.ResultError(fmt.Sprintf("failed to clear OAuth credentials for %q: %v", id, err)), nil
+		return tools.ResultError(fmt.Sprintf("failed to clear credentials for %q: %v", id, err)), nil
 	}
 
 	msg := strings.Builder{}
-	fmt.Fprintf(&msg, "cleared OAuth credentials for %q (%s).\n", id, server.URL)
+	fmt.Fprintf(&msg, "cleared credentials for %q (%s).\n", id, server.URL)
 	if wasEnabled {
-		msg.WriteString("the server was enabled and has been disabled; re-enable it to start a fresh authorization flow.\n")
+		msg.WriteString("the server was enabled and has been disabled; re-enable it to start a fresh connection.\n")
 	} else {
-		msg.WriteString("enable the server to start a fresh authorization flow.\n")
+		msg.WriteString("enable the server to start a fresh connection.\n")
 	}
 	return tools.ResultSuccess(msg.String()), nil
 }
diff --git a/pkg/tools/builtin/mcpcatalog/mcpcatalog_test.go b/pkg/tools/builtin/mcpcatalog/mcpcatalog_test.go
@@ -122,7 +122,14 @@ func TestEnableDisableLifecycle(t *testing.T) {
 	res, err := ts.handleEnable(ctx, EnableArgs{ID: oauthID})
 	require.NoError(t, err)
 	require.False(t, res.IsError, "enable failed: %s", res.Output)
-	assert.Contains(t, res.Output, "OAuth")
+	assert.Contains(t, res.Output, "enabled")
+	// The OAuth-branch wording was intentionally removed: the model has
+	// no agency over the OAuth flow, so the tool result no longer mentions
+	// "OAuth" or "authorization" — the previous "elicited on the next
+	// turn" wording caused the model to stop and ask the user to repeat
+	// themselves. See handleEnable for the rationale.
+	assert.NotContains(t, res.Output, "OAuth", "tool result must not leak OAuth details to the model")
+	assert.NotContains(t, res.Output, "authorization", "tool result must not leak OAuth details to the model")
 	assert.Equal(t, int32(1), changes.Load(), "enable should fire tools-changed handler exactly once")
 
 	ts.mu.RLock()
@@ -322,7 +329,10 @@ func TestEnableAPIKeyEnvPresent(t *testing.T) {
 	res, err := ts.handleEnable(t.Context(), EnableArgs{ID: apiKeyID})
 	require.NoError(t, err)
 	require.False(t, res.IsError)
-	assert.Contains(t, res.Output, "auth: API key")
+	assert.Contains(t, res.Output, "enabled")
+	// With every required env var present, no WARNING line is emitted —
+	// the tool result is intentionally terse so the model proceeds to the
+	// user's original request rather than narrating setup.
 	assert.NotContains(t, res.Output, "WARNING")
 }
 
@@ -550,7 +560,7 @@ func TestResetAuthForwardsToTokenStore(t *testing.T) {
 	res, err := ts.handleResetAuth(t.Context(), ResetAuthArgs{ID: oauthServer.ID})
 	require.NoError(t, err)
 	require.False(t, res.IsError, "reset auth: %s", res.Output)
-	assert.Contains(t, res.Output, "cleared OAuth credentials")
+	assert.Contains(t, res.Output, "cleared credentials")
 	assert.Equal(t, []string{oauthServer.URL}, removedURLs,
 		"removeOAuthToken must be called once with the catalog URL")
 }
@@ -588,7 +598,7 @@ func TestResetAuthNoOpForNonOAuth(t *testing.T) {
 	res, err := ts.handleResetAuth(t.Context(), ResetAuthArgs{ID: apiKeyID})
 	require.NoError(t, err)
 	require.False(t, res.IsError)
-	assert.Contains(t, res.Output, "nothing to reset")
+	assert.Contains(t, res.Output, "no persisted credentials")
 	assert.Zero(t, called, "api_key servers must not touch the OAuth token store")
 }
 
