Merge pull request #2512 from dgageot/oauth-slack

Make the slack remote MCP server work

Author: dgageot

pkg/agent/agent.go

@@ -44,11 +44,13 @@ type Agent struct {
 	hooks                   *latest.HooksConfig
 	cache                   *cache.Cache
 
-	// warningsMu guards pendingWarnings. addToolWarning and DrainWarnings
-	// may be called concurrently from the runtime loop, the MCP server,
-	// the TUI and session manager.
+	// warningsMu guards pendingWarnings and pendingNotices. AddToolWarning,
+	// AddToolNotice, DrainWarnings and DrainNotices may be called
+	// concurrently from the runtime loop, the MCP server, the TUI and
+	// session manager.
 	warningsMu      sync.Mutex
 	pendingWarnings []string
+	pendingNotices  []string
 }
 
 // New creates a new agent
@@ -288,7 +290,7 @@ func (a *Agent) collectTools(ctx context.Context) ([]tools.Tool, error) {
 		if err != nil {
 			desc := tools.DescribeToolSet(toolSet)
 			slog.Warn("Toolset listing failed; skipping", "agent", a.Name(), "toolset", desc, "error", err)
-			a.addToolWarning(fmt.Sprintf("%s list failed: %v", desc, err))
+			a.AddToolWarning(fmt.Sprintf("%s list failed: %v", desc, err))
 			continue
 		}
 		agentTools = append(agentTools, ta...)
@@ -321,7 +323,7 @@ func (a *Agent) ensureToolSetsAreStarted(ctx context.Context) {
 			if toolSet.ShouldReportFailure() {
 				desc := tools.DescribeToolSet(toolSet)
 				slog.Warn("Toolset start failed; will retry on next turn", "agent", a.Name(), "toolset", desc, "error", err)
-				a.addToolWarning(fmt.Sprintf("%s start failed: %v", desc, err))
+				a.AddToolWarning(fmt.Sprintf("%s start failed: %v", desc, err))
 			} else {
 				desc := tools.DescribeToolSet(toolSet)
 				slog.Debug("Toolset still unavailable; retrying next turn", "agent", a.Name(), "toolset", desc, "error", err)
@@ -332,13 +334,17 @@ func (a *Agent) ensureToolSetsAreStarted(ctx context.Context) {
 		if toolSet.ConsumeRecovery() {
 			desc := tools.DescribeToolSet(toolSet)
 			slog.Info("Toolset now available", "agent", a.Name(), "toolset", desc)
-			a.addToolWarning(desc + " is now available")
+			a.AddToolNotice(desc + " is now available")
 		}
 	}
 }
 
-// addToolWarning records a warning generated while loading or starting toolsets.
-func (a *Agent) addToolWarning(msg string) {
+// AddToolWarning records a warning generated while loading or starting toolsets.
+// Warnings represent real failures the user should know about (a remote MCP
+// server returning 4xx, an MCP binary missing, ...). For positive notices
+// (a previously-failed toolset becoming available again) use AddToolNotice
+// instead so the message isn't framed as a failure.
+func (a *Agent) AddToolWarning(msg string) {
 	if msg == "" {
 		return
 	}
@@ -347,6 +353,19 @@ func (a *Agent) addToolWarning(msg string) {
 	a.warningsMu.Unlock()
 }
 
+// AddToolNotice records a positive, informational notice about a toolset
+// (typically: a previously-failed toolset is now available). Notices are
+// surfaced to the user separately from warnings so the framing doesn't
+// say "failed to initialize" for a recovery message.
+func (a *Agent) AddToolNotice(msg string) {
+	if msg == "" {
+		return
+	}
+	a.warningsMu.Lock()
+	a.pendingNotices = append(a.pendingNotices, msg)
+	a.warningsMu.Unlock()
+}
+
 // DrainWarnings returns pending warnings and clears them.
 func (a *Agent) DrainWarnings() []string {
 	a.warningsMu.Lock()
@@ -356,6 +375,15 @@ func (a *Agent) DrainWarnings() []string {
 	return warnings
 }
 
+// DrainNotices returns pending notices and clears them.
+func (a *Agent) DrainNotices() []string {
+	a.warningsMu.Lock()
+	defer a.warningsMu.Unlock()
+	notices := a.pendingNotices
+	a.pendingNotices = nil
+	return notices
+}
+
 func (a *Agent) StopToolSets(ctx context.Context) error {
 	for _, toolSet := range a.toolsets {
 		// Only stop toolsets that were successfully started

pkg/agent/agent_test.go

@@ -370,21 +370,23 @@ func TestAgentReProbeEmitsWarningThenNotice(t *testing.T) {
 	}
 	a := New("root", "test", WithToolSets(stub))
 
-	// Turn 1: start fails → 1 warning, 0 tools.
+	// Turn 1: start fails → 1 warning, 0 tools, no notice yet.
 	got, err := a.Tools(t.Context())
 	require.NoError(t, err)
 	assert.Empty(t, got, "turn 1: no tools while toolset is unavailable")
 	warnings := a.DrainWarnings()
 	require.Len(t, warnings, 1, "turn 1: exactly one warning expected")
 	assert.Contains(t, warnings[0], "start failed")
+	assert.Empty(t, a.DrainNotices(), "turn 1: no recovery notice while still failing")
 
-	// Turn 2: start succeeds → 1 recovery warning, tools available.
+	// Turn 2: start succeeds → 1 recovery NOTICE (not warning), tools available.
 	got, err = a.Tools(t.Context())
 	require.NoError(t, err)
 	assert.Len(t, got, 1, "turn 2: tool should be available after recovery")
-	recovery := a.DrainWarnings()
-	require.Len(t, recovery, 1, "turn 2: exactly one recovery warning expected")
-	assert.Contains(t, recovery[0], "now available", "turn 2: recovery warning must mention availability")
+	assert.Empty(t, a.DrainWarnings(), "turn 2: recovery is a notice, not a failure warning")
+	notices := a.DrainNotices()
+	require.Len(t, notices, 1, "turn 2: exactly one recovery notice expected")
+	assert.Contains(t, notices[0], "now available", "turn 2: recovery notice must mention availability")
 }
 
 // TestAgentNoDuplicateStartWarnings verifies that repeated failures generate
@@ -435,7 +437,7 @@ func TestAgentWarningsConcurrentAccess(t *testing.T) {
 		go func() {
 			defer wg.Done()
 			for range perWriter {
-				a.addToolWarning("boom")
+				a.AddToolWarning("boom")
 			}
 		}()
 	}

pkg/agent/opts.go

@@ -163,7 +163,7 @@ func WithCommands(commands types.Commands) Opt {
 func WithLoadTimeWarnings(warnings []string) Opt {
 	return func(a *Agent) {
 		for _, w := range warnings {
-			a.addToolWarning(w)
+			a.AddToolWarning(w)
 		}
 	}
 }

pkg/runtime/loop.go

@@ -685,15 +685,23 @@ func (r *LocalRuntime) configureToolsetHandlers(a *agent.Agent, events chan Even
 	}
 }
 
-// emitAgentWarnings drains and emits any pending toolset warnings as persistent
-// TUI notifications. Both start failures and recovery notices are emitted as
-// warnings so they remain visible until the user dismisses them.
+// emitAgentWarnings drains and emits any pending toolset warnings and notices
+// as persistent TUI notifications. Failures ("start failed", "list failed")
+// and recoveries ("is now available") flow through separate queues on the
+// agent so each can be framed correctly — a single mixed message saying
+// "Some toolsets failed to initialize ... is now available" reads as a
+// contradiction.
 func (r *LocalRuntime) emitAgentWarnings(a *agent.Agent, send func(Event)) {
 	warnings := a.DrainWarnings()
 	if len(warnings) > 0 {
 		slog.Warn("Tool setup partially failed; continuing", "agent", a.Name(), "warnings", warnings)
 		send(Warning(formatToolWarning(a, warnings), a.Name()))
 	}
+	notices := a.DrainNotices()
+	if len(notices) > 0 {
+		slog.Info("Toolset status update", "agent", a.Name(), "notices", notices)
+		send(Warning(formatToolNotice(a, notices), a.Name()))
+	}
 }
 
 func formatToolWarning(a *agent.Agent, warnings []string) string {
@@ -705,6 +713,15 @@ func formatToolWarning(a *agent.Agent, warnings []string) string {
 	return strings.TrimSuffix(builder.String(), "\n")
 }
 
+func formatToolNotice(a *agent.Agent, notices []string) string {
+	var builder strings.Builder
+	fmt.Fprintf(&builder, "Toolset status update for agent '%s':\n\n", a.Name())
+	for _, notice := range notices {
+		fmt.Fprintf(&builder, "- %s\n", notice)
+	}
+	return strings.TrimSuffix(builder.String(), "\n")
+}
+
 // filterExcludedTools removes tools whose names appear in the excluded list.
 // This is used by skill sub-sessions to prevent recursive run_skill calls.
 func filterExcludedTools(agentTools []tools.Tool, excluded []string) []tools.Tool {

pkg/runtime/runtime.go

@@ -848,12 +848,23 @@ func (r *LocalRuntime) EmitStartupInfo(ctx context.Context, sess *session.Sessio
 		send(NewTokenUsageEvent(sess.ID, r.CurrentAgentName(), usage))
 	}
 
-	// Emit agent warnings (if any) - these are quick
+	// Tool loading can be slow (MCP servers need to start). Mark the
+	// context as non-interactive so toolsets that require user-driven
+	// flows (e.g. an OAuth elicitation for a remote MCP server) fail
+	// fast with a recognisable error rather than blocking on a dialog
+	// the TUI is not yet ready to render. The actual prompt happens on
+	// the first RunStream when the user is interacting with the agent.
+	nonInteractiveCtx := mcptools.WithoutInteractivePrompts(ctx)
+	r.emitToolsProgressively(nonInteractiveCtx, a, send)
+
+	// Flush any agent warnings: load-time warnings recorded at agent
+	// construction (WithLoadTimeWarnings) and per-toolset warnings recorded
+	// during startup above (e.g. a remote MCP server returning 4xx during
+	// initialize). Surfacing them as WarningEvents lets the TUI show a
+	// persistent notice with the actual server-side explanation — otherwise
+	// the user only sees the toolset disappear from the sidebar with no clue
+	// as to why.
 	r.emitAgentWarnings(a, func(e Event) { send(e) })
-
-	// Tool loading can be slow (MCP servers need to start)
-	// Emit progressive updates as each toolset loads
-	r.emitToolsProgressively(ctx, a, send)
 }
 
 // emitToolsProgressively loads tools from each toolset and emits progress updates.
@@ -888,7 +899,35 @@ func (r *LocalRuntime) emitToolsProgressively(ctx context.Context, a *agent.Agen
 		if startable, ok := toolset.(*tools.StartableToolSet); ok {
 			if !startable.IsStarted() {
 				if err := startable.Start(ctx); err != nil {
-					slog.Warn("Toolset start failed; skipping", "agent", a.Name(), "toolset", fmt.Sprintf("%T", startable.ToolSet), "error", err)
+					desc := tools.DescribeToolSet(startable.ToolSet)
+					// IsAuthorizationRequired must be checked BEFORE
+					// ShouldReportFailure: this is the first — expected —
+					// failure of a deferred-OAuth toolset, and consuming
+					// freshFailure here would suppress the *real*
+					// failure (e.g. server 4xx on the eventual interactive
+					// retry) that the user actually needs to see.
+					if mcptools.IsAuthorizationRequired(err) {
+						// The toolset just needs an OAuth approval that we
+						// deliberately deferred until the user is interacting
+						// with the agent. The dialog will appear naturally on
+						// the first RunStream — no need to pre-announce it.
+						slog.Debug("Toolset deferred until first message", "agent", a.Name(), "toolset", desc, "reason", err)
+						continue
+					}
+					// Route real failures through the agent's warning
+					// channel so the TUI surfaces a persistent,
+					// user-visible notice that includes the actual
+					// server-side cause (threaded through by
+					// remoteMCPClient.Initialize). Use the same
+					// once-per-streak guard as ensureToolSetsAreStarted
+					// so a failing toolset doesn't flood the UI with a
+					// new warning every time the agent is restarted.
+					if !startable.ShouldReportFailure() {
+						slog.Debug("Toolset still unavailable; skipping", "agent", a.Name(), "toolset", desc, "error", err)
+						continue
+					}
+					slog.Warn("Toolset start failed; skipping", "agent", a.Name(), "toolset", desc, "error", err)
+					a.AddToolWarning(fmt.Sprintf("%s start failed: %v", desc, err))
 					continue
 				}
 			}