[ian-de-marcellus]

Problem

Every MCPL reconnect re-lists all servers' tools (handleToolsListChanged → refreshMcplTools). A server whose tools/list throws at that moment (mid-reconnect, hung, briefly down) was skipped silently, so its tools disappeared from mcplTools. With several connectors, one reconnecting while another is briefly unavailable:

• changes the tool list, which sits at the top of every request, forcing a full prompt-cache rewrite (system prompt and history included);
• hides the unavailable server's tools without announcement (the diff announces only new tools);
• forces another full rewrite when they come back at the next re-list.

We found this while tracing unexplained cache rewrites on a long-context resident with several connectors.

Fix

A failed re-list carries that server's previously known tools (and their feature sets) forward, and logs one line. A call to a down server fails on its own, as it already did. A server that has never listed tools still contributes nothing.

Tests

test/mcpl-tools-keep-on-failed-relist.test.ts: two servers list their tools; on a re-list one fails and keeps its tools while the other picks up a new one; then it recovers. A mutation that drops the kept tools fails the test. Full suite 958/958 on main.

🤖 Generated with Claude Code

[greptile-apps]

Confidence Score: 2/5

[Medium risk] Changes how the system handles tool-list failures during reconnects.

This PR is not safe to merge while failed re-lists can restore disconnected or other servers’ tools and produce duplicate definitions.

Findings

1.  Disconnected tools can return ▶
2.  Nested prefixes copy foreign tools ▶
3.  Partial lists produce duplicates ▶

Fix with agent prompt

### Issue 1
src/framework.ts:13134-13135
If a refresh starts before server B is disconnected, it can retain B in its server snapshot. Once B closes, the failed `tools/list` call restores B’s tools from the earlier snapshot. If that refresh finishes after the disconnect’s own refresh, it overwrites the list that removed B, leaving disconnected tools advertised until another refresh.

### Issue 2
src/framework.ts:13172
Two allowed server prefixes can be nested, such as `mcpl--foo` and `mcpl--foo--bar`. If the shorter-prefix server fails its re-list, this filter also copies the longer-prefix server’s old tools. A tool that the longer-prefix server just removed can remain advertised, or an updated tool can appear twice.

### Issue 3
src/framework.ts:13173-13174
The catch also runs when processing a returned list fails after some tools have already been appended. For example, a valid known tool followed by a malformed `null` entry appends the first tool, throws on the second, then appends the old copy of the first tool again. The request list consequently contains duplicate tool names.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

The PR retains a server’s previously listed MCPL tools and feature-set attributions when a re-list fails, and adds a regression test and changelog entry.

• The fallback needs to distinguish tool ownership and avoid restoring partial or obsolete results.
• The test covers name retention but not the full cache-stability and attribution behavior.

_{Reviews (1) · Last reviewed commit: "fix(mcpl): a server that fails a tools r..."}

[greptile-apps]

Disconnected tools can return If a refresh starts before server B is disconnected, it can retain B in its server snapshot. Once B closes, the failed tools/list call restores B’s tools from the earlier snapshot. If that refresh finishes after the disconnect’s own refresh, it overwrites the list that removed B, leaving disconnected tools advertised until another refresh.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/framework.ts
Line: 13134-13135

Comment:
**Disconnected tools can return** If a refresh starts before server B is disconnected, it can retain B in its server snapshot. Once B closes, the failed `tools/list` call restores B’s tools from the earlier snapshot. If that refresh finishes after the disconnect’s own refresh, it overwrites the list that removed B, leaving disconnected tools advertised until another refresh.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

[greptile-apps]

Nested prefixes copy foreign tools Two allowed server prefixes can be nested, such as mcpl--foo and mcpl--foo--bar. If the shorter-prefix server fails its re-list, this filter also copies the longer-prefix server’s old tools. A tool that the longer-prefix server just removed can remain advertised, or an updated tool can appear twice.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/framework.ts
Line: 13172

Comment:
**Nested prefixes copy foreign tools** Two allowed server prefixes can be nested, such as `mcpl--foo` and `mcpl--foo--bar`. If the shorter-prefix server fails its re-list, this filter also copies the longer-prefix server’s old tools. A tool that the longer-prefix server just removed can remain advertised, or an updated tool can appear twice.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

[greptile-apps]

Partial lists produce duplicates The catch also runs when processing a returned list fails after some tools have already been appended. For example, a valid known tool followed by a malformed null entry appends the first tool, throws on the second, then appends the old copy of the first tool again. The request list consequently contains duplicate tool names.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/framework.ts
Line: 13173-13174

Comment:
**Partial lists produce duplicates** The catch also runs when processing a returned list fails after some tools have already been appended. For example, a valid known tool followed by a malformed `null` entry appends the first tool, throws on the second, then appends the old copy of the first tool again. The request list consequently contains duplicate tool names.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

[slimepriestess]

Reviewed at head 62553ee, trial-merged onto current main (5498805): zero conflicts. Head suite 962/958/0/4 + tsc clean (clean dist); merged state 972/968/0/4 + tsc clean. Revert-goes-red on your test verified (0/1 with the catch reverted). The mechanism claims were verified against the real code: reconnect-enabled servers stay in the registry across drops, so the keep-path guards exactly the mid-reconnect case; kept-tool order is stable, so the cache-rewrite motive holds; the carried-forward feature sets are only read at dispatch for checkpoint attribution (fail-soft, and checkpoint state deliberately survives transient close). The diagnosis and direction are right — the two blockers below are both in the catch's merge strategy, and one small restructure fixes both.

1. Blocker: a throw after partial pushes produces duplicate tool names. The try wraps the whole per-server loop and pushes go straight into the shared tools array, so a tools/list that succeeds but throws mid-iteration (e.g. tools: [read, null, write] → Cannot read properties of null (reading 'name') — there's no response validation; sendToolsList is a bare cast) lands in the catch with partial pushes already made, and the catch appends the full previous set on top. Reproduced against this head's build:

tools: [ 'mcpl--b--read', 'mcpl--b--read', 'mcpl--b--write' ]  // duplicates

The Anthropic API rejects duplicate tool names with a 400, and nothing in AF or membrane dedupes definitions before the wire (the only Set over names is the driveStream allowlist). So one malformed tools/list entry from a buggy or hostile server now fails every request until it re-lists cleanly — the same bricked-turns outcome this PR exists to prevent, where the old code degraded to a partial-but-valid list. The log line is also misleading for this case: it says did not answer tools/list when the server did answer.

Fix shape: stage each server's tools into a local array + local feature-set map and merge into tools only after that server's loop completes — then the catch is all-kept or all-new, never both. A test feeding [valid, null] goes red today.

2. Should-fix (same pass): a permanently removed server's tools can come back. disconnectMcplServer / restartMcplServer / connect call refreshMcplTools() directly, bypassing the mcplToolRefreshInFlight collapse (only handleToolsListChanged is gated). Interleaving, driven through the real entry points: refresh A snapshots the registry and awaits a slow server; disconnect(b) removes b and its own refresh writes the correct list; A resumes, b's list rejects (connection closed), the catch keeps b's tools, and A commits last:

after disconnect(b): [ 'mcpl--a--send' ]
after A lands:       [ 'mcpl--a--send', 'mcpl--b--read', 'mcpl--b--write' ]   // registered: [ 'a' ]

Ghost tools for a removed server, unreported by the diff, until the next list_changed. The last-writer race predates this PR (A listing b before the removal resurrects it too), but the keep-path widens it to servers removed mid-list. Cleanest close: at commit time, drop kept tools whose server is no longer registered/configured (commit-time, not in the catch — removeServer closes the connection before the config is deleted, so the catch can fire first). Routing the direct callers through the in-flight gate also works.

Non-blocking:

• The default-prefix filter can cross-keep: server ids a and a--x produce default prefixes where a's failure also keeps (and duplicates) a--x's tools via startsWith. Ids with -- are unusual; filtering kept tools by owning server id rather than name prefix closes it and falls out naturally from the per-server staging in fix 1.
• Kept tools bypass the isToolAllowed re-check. Only reachable when a config update lands and the very next refresh fails before the server answers once — narrow, noting for the record.
• A permanently failing server keeps stale tools forever; calls fail on their own and your log line covers it. Fine as designed.
• The status snapshot now reports connected: false with a nonzero toolCount for a down server. Arguably more honest, but worth a doc word so nobody reads toolCount as liveness.

(Weft — reviewed via Ra's account per the usual convention; adversarial second read by a peer session, which independently reproduced both blockers through this head's build.)

[slimepriestess]

Hi Ian, sorry this sat; the last few weeks were busy. We re-read it against current main (c4ba80b), and the short version is that it's still needed. Main moved under it, though, so the rebase needs more than conflict resolution.

Still needed (ran). On main, a reconnecting server whose tools/list fails still loses its tools: in a probe with server b mid-reconnect (isConnected:false, willReconnect:true) and its list throwing, main publishes only server a's tools. The catch at framework.ts:25818 skips the server, which is the churn your PR exists to prevent.

What changed under it:

1. Main now removes a server halted for good (b965af0): if (params.permanent) this.handleToolsListChanged(connection.id), with the comment "Halted for good: its tools leave the agents' list" (:25554). That refresh re-runs this loop with the halted server still registered and its list throwing. Your keep block keeps on any failure, so pasted onto main as-is it would put the halted server's tools back for good (read here; our first reader also ran it on a trial rebase: main [a--send], rebase [a--send, b--read, b--write]). Our September review called "a permanently failing server keeps stale tools" fine as designed; main has since decided the other way, so that's us updating, not a mistake on your side. The fix is one predicate main already has: keep only when AgentFramework.mcpConnectionServes(server) (connected or reconnecting, :23211), the same test main uses for "this listing still counts".
2. The test's fakes need main's fields. Main reads isConnected, willReconnect and mcpToolsAdvertised, so the test as written fails on main before the keep path runs. With those set (the failing server as reconnecting), the keep path does what you intended on a trial rebase.

Still standing from 9/29:

3. Duplicate names on a throw after partial pushes is still our blocker. A list that answers but throws mid-iteration ([read, null, write]) leaves partial pushes in tools, and the catch appends the full previous set on top. The API rejects duplicate tool names, so one malformed entry would fail every request. Staging each server's tools locally and merging after its loop completes makes the catch all-kept or all-new.
4. The feature-set carry has no test that goes red. Reverting it leaves your test green. One assert closes it (a tool listed with a featureSet, re-list fails, the feature set is still mapped).

Smaller, for the rebase:

• Kept tools should also go into toolServers under their server id. Main filters published tools by toolServers whenever another server's listing is superseded (:25905), so a kept tool without an entry would drop out intermittently. Per-server staging gives you this for free.
• Please don't carry RFC-008 class hints along with feature sets. Main drops a closed server's classes on purpose ("unclassed, the restrictive answer", :25637), and your PR already leaves them alone, which is right.
• Main has closed two of our 9/29 notes for you: the overtaken-refresh rule (#262, :25899) covers most of the ghost-tools-for-a-removed-server case, and prefixesMeet (#469) covers the a / a--x cross-keep.

The diagnosis was right in September and it's still right. It's the surroundings that changed. Happy to look again whenever you rebase.