feat(mcp_catalog): block enable_remote_mcp_server until the server is connected

[trungutt]

Issues on main this PR fixes

1. The user has to re-ask their original question after enabling a server

enable_remote_mcp_server's handler defers the MCP connect (and any OAuth handshake) to the next Tools() enumeration. The tool's result text and the catalog Instructions block both tell the model "tools appear on your next turn". Every model reading that ends its turn after enable with a "please try again" message; the user then has to re-send their original question for the activated server's tools to actually be invoked.

User : List all Notion posts by David
Model: <calls search → calls enable("notion")>
       Notion is being set up. Please try again on your next message.
User : List all Notion posts by David          ← forced re-ask
Model: <now actually uses notion_* tools>

This was never a runtime constraint — getTools rebuilds the tool list at the top of every iteration of the run loop, and reprobe (with its regression test TestReprobe_NewToolsAvailableAfterToolCall) explicitly supports new tools appearing mid-stream. The deferred-Start design was an artifact: registering the toolset was cheap, but the prompt then had to admit the runtime couldn't guarantee tools would actually be available, since OAuth might be declined or the server might refuse the connection.

After this PR handleEnable drives Start synchronously inside the tool call. OAuth elicitation surfaces a dialog and blocks until the user responds; the success / failure result reaches the model at the point of the call, in the same RunStream. The Instructions block and the tool description are rewritten to match (no more "appears on your next turn" wording on the happy path).

2. OAuth is still required on the next question even if the previous OAuth failed

If the user stops a turn while the OAuth dialog is parked (the browser-side flow failed externally, the user gave up, etc.), Start returns context.Canceled. On main this falls into a generic warning-log branch and the entry stays in t.enabled in an unstarted state. The next user message — typically unrelated to the abandoned server — calls Tools(), which silently re-Starts the wrapper, hits 401, and re-pops the same dialog the user just abandoned.

The root cause is a lifetime mismatch: the catalog Toolset is owned by the RuntimeSession, not the RunStream, so Toolset.Stop does NOT run between a stopped turn and the user's next message.

                  RuntimeSession  (lives across many user messages)
┌─────────────────────────────────────────────────────────────┐
│  RunStream #1   │   RunStream #2   │   RunStream #3         │
│   Stop here─┘                                               │
│                                                             │
│  mcpcatalog.Tools()        runs at the start of every       │
│                            RunStream — iterates t.enabled   │
│                            and Start()s any unstarted entry │
│                                                             │
│  mcpcatalog.Toolset.Stop() runs ONCE, at session teardown   │
└─────────────────────────────────────────────────────────────┘

context.Canceled is now routed through disableAfterDecline (the same path that already handles an explicit OAuthDeclined) in both code paths that can Start a catalog server — the synchronous handleEnable and the per-turn Tools() iteration. The model-facing result tells the model the user stopped the turn while authorization was pending and to only retry if the user asks.

Public docs at docs/tools/mcp-catalog/index.md are updated for the new contract.

[trungutt]

Good catch — fixed in 33d73f5.

The root cause was the same for both your comments: the top-of-handleEnable guard short-circuited any entry already in t.enabled regardless of IsStarted(). Now the guard detects existing-but-unstarted entries and falls through to a fresh Start attempt on the same wrapper, so the AuthorizationRequired branch's "call enable again to surface a fresh authorization dialog" instruction actually does something.

StartableToolSet.Start is already idempotent and single-flight: when the previous attempt left the wrapper not-started, the retry re-invokes the inner Start without re-creating the toolset or re-firing tools_changed. The "already enabled but not yet connected" dead-end message is gone.

Regression test: TestEnableRetriesStartOnExistingUnstartedEntry.

[trungutt]

Fixed in 33d73f5 — same root cause as the AuthorizationRequired comment above.

The context.Canceled branch still intentionally leaves the entry in t.enabled (rolling back would race a Ctrl+C-driven Toolset.Stop on the same map), but the dead-end you flagged is gone: the next enable for the same id now falls through the guard and re-attempts Start on the existing wrapper. The "call enable_remote_mcp_server again to retry" hint in the cancel result text has somewhere to land.

I also added a sentence to the branch comment so the invariant the design relies on (guard retries unstarted entries) is documented at the failure site, not just at the guard.

Regression test: TestEnableRetriesStartAfterCancellation.

[trungutt]

I don't think this race exists — the lookup and the insert both happen inside the same t.mu critical section, so two goroutines for the same id can't both observe alreadyEnabled=false:

t.mu.Lock()                                  // ← serializes
wrapped, alreadyEnabled := t.enabled[id]     // read
...
if !alreadyEnabled {
    mcpToolset := mcp.NewRemoteToolset(...)
    ...
    wrapped = tools.NewStartable(mcpToolset)
    t.enabled[id] = wrapped                   // write — still under lock
    notify = t.toolsChangedHandler
}
t.mu.Unlock()

Whichever goroutine acquires the lock first does the read, creates the toolset, inserts it, and releases. The second goroutine then acquires, finds the just-inserted entry, observes alreadyEnabled=true, and either returns the "already enabled and connected" fast path or falls through to the Start retry — which delegates to StartableToolSet.Start, itself documented as single-flight (it holds an internal mutex and short-circuits if s.started is already true). So we get at most one NewRemoteToolset, one notify(), and one in-flight Start per id.

I don't want to plumb a "pending" sentinel through this path just to defend against a race the lock already prevents — happy to revisit if I'm missing a scenario where the lock is released between the read and the write.

[trungutt]

The runtime already has a generic safeguard for this: the loop detector at pkg/runtime/loop.go (NewLoopDetector(loopThreshold, ...), default 5) terminates the agent with ErrorCodeLoopDetected after N consecutive identical tool calls. A model that keeps re-issuing enable_remote_mcp_server with the same id would trip it long before this turned into a real infinite loop, and the failure surface is already user-visible.

I don't think adding a per-id retry counter inside the catalog earns its keep when the runtime already handles "model is stuck calling the same tool" generically — it'd be a second, narrower copy of the same policy.

For context, this branch is only reachable when Start returns AuthorizationRequired, which happens when the request context disallows interactive prompts (mcp.WithoutInteractivePrompts). Tool handlers run with the interactive context, so in normal operation Start blocks on the OAuth elicitation instead and we never enter this branch. It is, as the comment says, a defensive fallback for hosts that haven't wired up the elicitation bridge.

[docker-agent]

Assessment: 🟢 APPROVE

The synchronous-connect refactor is well-structured. The mutex discipline is correct, the injectable startToolset seam keeps tests deterministic, and all four Start-failure branches (OAuthDeclined, AuthorizationRequired, Canceled, transport error) are individually tested with clear rollback semantics. The pre-flight env-var guard is a welcome improvement — blocking instead of warning prevents the model from claiming a connection that will immediately fail.

Three minor observations (none block merge):

1. stubStartOK/stubStartErr bypass wrapped.Start so wrapped.IsStarted() is never true in stub-based tests. The "already enabled and connected" fast-path guard (alreadyEnabled && IsStarted()) is consequently never exercised. Production behaviour is correct because the real startToolset calls wrapped.Start(ctx) which sets the flag. A future test could use a real StartableToolSet with a no-op inner toolset to close this gap — not urgent.

2. disableAfterDecline called with a potentially-cancelled ctx in the default error branch. A wrapped error containing context.Canceled falls through to default (only unwrapped context.Canceled matches the earlier branch), so wrapped.Stop(ctx) receives a cancelled context. Stop silently swallows its error, so no observable failure — low risk.

3. Concurrent re-enable of an unstarted entry: two goroutines can both pass alreadyEnabled && !IsStarted() and both call startToolset. Because StartableToolSet.Start is single-flight, this is benign — the second call returns immediately. Mentioned for completeness.

All high-risk paths (rollback correctness, error-branch coverage, idempotency) look solid.

[docker-agent]

Assessment: 🟢 APPROVE

The PR correctly implements synchronous blocking in enable_remote_mcp_server with proper error classification (OAuthDeclined, AuthRequired, Canceled, transport error) and appropriate state rollback on each failure path. The disableAfterDecline guard (current == wrapped) correctly handles concurrent failure races. Test coverage for the new failure branches is thorough.

[docker-agent]

Assessment: 🟢 APPROVE

The PR correctly implements synchronous blocking Start() in handleEnable and properly routes context.Canceled through disableAfterDecline. The logic is sound and the approach resolves both stated issues (forced re-ask and OAuth re-pop on the next turn).

What was checked:

• Concurrency safety of handleEnable for concurrent calls on the same unstarted entry → StartableToolSet.Start is documented and implemented as single-flight/idempotent; no race
• Handler propagation to existing unstarted entries on retry → Set*Handler methods on Toolset already iterate over t.enabled (including unstarted entries) and push updates through, so the retry path has current handlers
• context.Canceled handling in both handleEnable and the Tools() loop → correctly routes to disableAfterDecline in all affected code paths
• disableAfterDecline concurrent-access guard (current == wrapped pointer check) → correct
• Test coverage for the new synchronous Start path → comprehensive (success, OAuthDeclined, AuthorizationRequired, context.Canceled, retry branches all covered)
• Documentation update in docs/tools/mcp-catalog/index.md → consistent with new contract