Skip to content

Suppress spurious 'is now available' MCP toolset notice#2578

Merged
dgageot merged 2 commits intodocker:mainfrom
dgageot:board/suppress-false-mcp-oauth-success-warning-2e6f5046
Apr 28, 2026
Merged

Suppress spurious 'is now available' MCP toolset notice#2578
dgageot merged 2 commits intodocker:mainfrom
dgageot:board/suppress-false-mcp-oauth-success-warning-2e6f5046

Conversation

@dgageot
Copy link
Copy Markdown
Member

@dgageot dgageot commented Apr 28, 2026

What

After completing an OAuth flow for a remote MCP server, the TUI displayed a notification framed as a warning:

mcp(remote host=mcp.slack.com transport=streamable) is now available

This is not a valid warning. The OAuth dialog completing already makes a successful start obvious, and for non-OAuth recoveries the model just uses the tool — the follow-up notification is pure noise.

Changes

Commit 1 — Suppress the notice

Drop the entire tool-notice path in the agent and runtime:

  • pkg/agent/agent.go: remove pendingNotices field, AddToolNotice, DrainNotices. ensureToolSetsAreStarted still resets the failure-streak flag on a successful start (so the next failure is reported as fresh), but only logs at slog.Info — never user-visible.
  • pkg/runtime/loop.go: emitAgentWarnings only drains warnings now; formatToolNotice is gone.
  • Tests updated to assert recoveries are silent.

Commit 2 — Simplify the resulting state machine

With the recovery notice gone, the only consumer of ConsumeRecovery() was a state-reset call in agent.ensureToolSetsAreStarted. Folded that reset into Start() itself: a successful Start now implicitly clears the failure streak.

Before After
StartableToolSet flags 3 (hasEverFailed, freshFailure, pendingRecovery) 2 (inFailureStreak, pendingWarning)
Public methods Start, Stop, ShouldReportFailure, ConsumeRecovery Start, Stop, ShouldReportFailure
ensureToolSetsAreStarted fail-branch + recover-branch (2-arm) one flat try/warn-once loop
Doc comment 4-bullet state-machine spec 1 paragraph
Net diff −47 lines

Behavioural guarantees preserved

  • Failure-once-per-streak: inFailureStreak gate prevents re-queueing pendingWarning on repeated retries.
  • Post-recovery freshness: a successful Start resets both flags, so the next failure is again surfaced as a fresh warning.
  • Deferred-OAuth must not consume the gate: emitToolsProgressively still checks IsAuthorizationRequired(err) before ShouldReportFailure(), so the eventual real failure (Slack 4xx, etc.) is the one the user sees.

These three properties are pinned by tests in pkg/tools/startable_test.go, pkg/agent/agent_test.go, and pkg/runtime/runtime_test.go.

Validation

  • mise lint0 issues (golangci-lint) + no offenses detected (in-tree linter, 886 files)
  • mise test → all packages pass

Files touched

pkg/agent/agent.go
pkg/agent/agent_test.go
pkg/runtime/loop.go
pkg/runtime/runtime.go
pkg/runtime/runtime_test.go
pkg/tools/startable.go
pkg/tools/startable_test.go

dgageot added 2 commits April 28, 2026 16:39
After a user completes an OAuth flow for an MCP server, the TUI displayed a notification framed as a warning, e.g.:

    mcp(remote host=mcp.slack.com transport=streamable) is now available

It is not a valid warning: the OAuth dialog completing already makes a successful start obvious, and for non-OAuth recoveries the model just uses the tool. The follow-up notification was pure noise.

Drop the entire tool-notice path in the agent and runtime:

  - pkg/agent/agent.go: remove pendingNotices field, AddToolNotice, DrainNotices. ensureToolSetsAreStarted still calls ConsumeRecovery() so the failure-streak flag is reset (next failure is reported as fresh), but only logs at slog.Info — never user-visible.

  - pkg/runtime/loop.go: emitAgentWarnings only drains warnings now; formatToolNotice is gone.

  - tests updated to assert recoveries are silent.

Assisted-By: docker-agent
Now that the recovery notice has been dropped, the only consumer of `ConsumeRecovery()` was a state-reset call in agent.ensureToolSetsAreStarted. Fold that reset into Start() itself: a successful Start now implicitly clears the failure streak, so any future failure is again reported as fresh.

  - pkg/tools/startable.go:

      * 3 booleans (`hasEverFailed`, `freshFailure`, `pendingRecovery`) collapse to 2 (`inFailureStreak`, `pendingWarning`)

      * `ConsumeRecovery()` API removed entirely

      * struct doc reads as one paragraph instead of a four-bullet state-machine spec

  - pkg/agent/agent.go: ensureToolSetsAreStarted shrinks from a fail/recover diptych to a flat 'try, warn-once-on-failure' loop. `desc := …` is computed once per iteration.

  - pkg/tools/startable_test.go: tests assert observable behaviour (warn-once-per-streak, recovery resets streak, Stop resets state) instead of probing internal flags via `ConsumeRecovery`.

  - pkg/runtime/runtime.go, runtime_test.go: comments and test name updated to match the new vocabulary (no "freshFailure" terminology).

All callers untouched in observable behaviour. Net: -47 lines, one boolean and one public method removed, behaviour identical and verified by the unchanged end-to-end tests in pkg/agent and pkg/runtime.

Assisted-By: docker-agent
@dgageot dgageot requested a review from a team as a code owner April 28, 2026 15:06
Copy link
Copy Markdown
Contributor

@aheritier aheritier left a comment

Choose a reason for hiding this comment

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

LGTM

@dgageot dgageot merged commit 75a44d7 into docker:main Apr 28, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants