Skip to content

fix(mcpcatalog): tell the model to proceed after enabling an OAuth server #2925

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.

Sign up for GitHub

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

Merged
dgageot merged 3 commits into from
May 29, 2026
+46 −45
Merged

fix(mcpcatalog): tell the model to proceed after enabling an OAuth server #2925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
438058b
fix(mcpcatalog): tell the model to proceed after enabling an OAuth se…
trungutt May 29, 2026
dddf7fa
Address review feedback on #2925
trungutt May 29, 2026
c3c85b3
Stop leaking OAuth as a concept to the LLM
trungutt May 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 32 additions & 41 deletions pkg/tools/builtin/mcpcatalog/mcpcatalog.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,20 +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.
3. Use the newly activated tools as you would any other.
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 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.`
Expand Down Expand Up @@ -308,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),
Expand Down Expand Up @@ -345,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),
Expand Down Expand Up @@ -593,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)
Copy link
Copy Markdown

@docker-agent docker-agent May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Contradictory model instructions when API-key env vars are missing

When missing is non-empty, the message assembled in handleEnable simultaneously tells the model two conflicting things:

  1. "Proceed with the user's original request; the host handles any required connection setup."
  2. "WARNING: the following env var(s) are NOT set: … Ask the user to set them, then call disable_remote_mcp_server and enable_remote_mcp_server for this id again, otherwise tool calls will fail."

An LLM receiving both instructions in the same tool-result has no explicit prioritisation signal — the Proceed clause comes first and is unconditional, while the WARNING appears later but doesn't say "ignore the previous instruction". Depending on which instruction the model weighs more heavily it may either:

  • dutifully proceed and immediately fail every downstream tool call (env vars not set), or
  • correctly pause and ask the user to set the vars but now via a path the new wording was designed to remove.

Suggested fix: move the "Proceed with the user's original request" sentence into the else branch (i.e., only emit it when len(missing) == 0), so it is never paired with a WARNING that contradicts it:

msg := strings.Builder{}
fmt.Fprintf(&msg, "enabled %q (%s) — the server's tools will appear on your next turn.\n", id, server.Title)
fmt.Fprintf(&msg, "endpoint: %s\n", server.URL)
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)
} else {
    fmt.Fprintf(&msg, "Proceed with the user's original request; the host handles any required connection setup.\n")
}

fmt.Fprintf(&msg, "endpoint: %s\n", server.URL)
switch server.Auth.Type {
case "oauth":
msg.WriteString("auth: OAuth — an authorization URL will be elicited on the next turn.\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
}
Expand Down Expand Up @@ -761,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) {
Expand All @@ -772,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
Expand Down Expand Up @@ -802,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
}
18 changes: 14 additions & 4 deletions pkg/tools/builtin/mcpcatalog/mcpcatalog_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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")
}

Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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")
}

Expand Down
Loading