MCP part 3: adding, enabling and removing a server in Settings - #88
Open
shanforge wants to merge 4 commits into
Open
MCP part 3: adding, enabling and removing a server in Settings#88shanforge wants to merge 4 commits into
shanforge wants to merge 4 commits into
Conversation
Closes #83, and the last box of #63 — which closes #55. #76 landed everything about MCP that has no network in it: the server model, the namespace a server cannot pick, the trust rules, the four registry gates, the wire format and a transport. It was a draft for one reason — there was no way to *add* a server, so none of it could be reached. This is that way. ## The screen does not re-decide anything The rules live in `MCPServerStore`, `MCPEndpoint` and `MCPRegistryPlan` and were settled and tested before there was a screen. What this file is responsible for is not undoing them: adding does not enable, an edit moves the switch in neither direction, and the address is validated before it is saved. The field validates **while the user types**, and the message it shows is `MCPEndpoint.Rejection`'s rather than one written here, so the field and the store cannot disagree about what is allowed. It is deliberately not the only check — #76's review moved the rule into the store precisely because a rule that lives in a form is a rule the next caller does not get. ## The three loose ends #76's review recorded - **`MCPCatalog.refresh()` had no caller**, so `discovered` stayed empty and no MCP tool was ever published. It now runs when the section appears, when a server is switched on, and after an edit — a server switched on has no tool list yet, so without the second of those it would be enabled and offer nothing. - **`forget(id:)` had no caller**, so a removed server's tool list and health outlived it. Nothing offered them, because `MCPRegistryPlan` walks the store, but they were held for a server that no longer exists. - **`hasNetworkEgress` had no reader.** It is said once, plainly, next to the switches — rather than only on the Privacy tab, which is not where someone is standing when they turn a server on. ## Renaming a server no longer re-enables its tools The one this issue flagged as untested. A server's tools are published under a namespace derived from its **name**, and the per-tool disable list is keyed on the published name — the one the user saw when they turned the tool off. So renaming `GitHub` to `GitLab` turns `github__delete_repo` into `gitlab__delete_repo`, which is not on the list, and the tool comes back under a name the user has never seen. That is the worst direction for this kind of mistake to go: "I never want the agent to do this" has to survive an edit that had nothing to do with it, and there is no signal when it does not. `MCPRenameMigration` is the rule, pure and separate from both the store and the view — whatever else eventually renames a server has to apply it too, and a copy inside one form would not be. Where two servers fold to the same namespace the entry cannot be attributed to one of them and both move. That is the same ambiguity `MCPRegistryPlan` resolves by dropping the colliding tool, and it errs the same way: towards the tool staying **off**. Mutation-checked — making the remap a no-op turns "A disabled tool stays disabled through a rename" and "The rewritten name is one the registry will actually look for" red. The second is there because pasting the namespace and tool halves together can exceed the length bound `published` applies, and a name over that bound never matches anything. ## Verification - `xcodebuild build` — succeeds - `./scripts/test-no-llm.sh` — 1777 tests in 158 suites pass - SwiftFormat 0.62.1 `--lint` — 0/559 files require formatting - SwiftLint 0.65.0 `--strict` — 0 violations in 712 files Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3Wpnj9ZmWPKYdFPB1AVY3
**A refused save cleared the form and said nothing (major).** `commit()` called `store.add`/`store.update` and discarded the `Bool` they return, then closed the form and cleared the fields. The store validates the address independently — that was the point of moving the rule into it — so any disagreement between the field and the store meant pressing Add wiped what the user typed and saved nothing, with no message. The two agree today; this is what stops the day they don't being a silent loss. **A namespace collision was invisible (major).** Namespaces are derived from names and the derivation folds, so two servers can publish under one prefix — and `MCPRegistryPlan` resolves that by keeping the first and dropping the rest. Correct, and unobservable: the second server sits in Settings enabled, reachable, reporting its tool count, and offers the model nothing. It is now said while the name is still being typed. Not blocked — two servers may legitimately be called similar things — but named. **A decision was living inside a `View` (minor).** The clash check was a private computed property on the section, which is the shape `CLAUDE.md` singles out: a decision made inside a `View` is one the next caller cannot reach. `MCPNamespaceClash` is the rule, the view supplies the names. **Editing replaced the wrong thing (minor).** The row being edited returned `EmptyView()` and the form appeared below the whole list, so with several servers there was nothing saying which one was open. The form now replaces the row it is editing. ## A test that passed for the wrong reason, again The first version of the clash cases asserted that `git hub` clashes with `GitHub`. It does not: `GitHub` folds to `github` and `git hub` folds to `git_hub`, because a run of non-alphanumerics becomes a separator. They look like the same name and are not — and this is the *same pair* #76's review found already passing vacuously once, in the collision test there. There is now a case pinning that they do **not** clash, with a note saying why, so the next person to write this reaches for a genuinely-folding pair. Mutation-checked: comparing raw names instead of namespaces turns four cases red. Verified: build succeeds, 1785 tests in 159 suites pass, SwiftFormat --lint clean over 560 files, SwiftLint --strict 0 violations in 714 files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3Wpnj9ZmWPKYdFPB1AVY3
**The spinner stopped before the work did (minor).** `isRefreshing` was a boolean, so enabling two servers in quick succession started two refreshes and the first to finish cleared it — leaving the row that had not been contacted yet looking settled. It is a count of what is in flight now, cleared in a `defer` so a cancelled task cannot leave it above zero and the spinner turning for the rest of the session. **A removed server could come back (minor).** `MCPCatalog.refresh()` read the server list when it built its task group and never again. A refresh takes as long as its slowest server, and the user can remove one while it runs: `forget(id:)` cleared the entries and the results loop then wrote them straight back, for a server that no longer exists. Nothing would have offered those tools — `MCPRegistryPlan` walks the store — but they would be held for the rest of the session, and anything reading the catalog directly would have seen them. The list is re-read as each result arrives. Mutation-checked: dropping the guard turns "A server removed mid-refresh does not come back" red, driven through a deliberately slow stub transport so no test touches a socket. Verified: build succeeds, 1786 tests in 160 suites pass, SwiftFormat --lint clean over 560 files, SwiftLint --strict 0 violations in 714 files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3Wpnj9ZmWPKYdFPB1AVY3
Round three. Every server's Edit and Remove announced only "Edit" and "Remove", so with several servers a VoiceOver user was told which action but not which server — and Remove is the one where guessing is expensive. `displayName(of:)` also gives the empty-name case one answer rather than two. A name can be empty: the decoder defaults it so a file written before the field existed still reads, and a row labelled with nothing cannot be told from the next one, on screen or aloud. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3Wpnj9ZmWPKYdFPB1AVY3
This was referenced Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #83. The last box of #63 — which closes #55.
Stacked on #76 — review that first; this branch contains it.
#76 landed everything about MCP with no network in it: the server model, the namespace a
server cannot pick, the trust rules, the four registry gates, the wire format and a
transport. It was a draft for exactly one reason — there was no way to add a server,
so none of it could be reached. This is that way.
The screen does not re-decide anything
The rules live in
MCPServerStore,MCPEndpointandMCPRegistryPlan, and were settledand tested before there was a screen. What this file is responsible for is not undoing
them: adding does not enable, an edit moves the switch in neither direction, and the
address is validated before it is saved.
The field validates while you type, and the message it shows is
MCPEndpoint.Rejection's rather than one written here, so the field and the store cannotdisagree about what is allowed. It is deliberately not the only check — #76's review moved
that rule into the store precisely because a rule living in a form is a rule the next
caller does not get.
The three loose ends #76's review recorded, now closed
MCPCatalog.refresh()had no caller, sodiscoveredstayed empty and no MCP toolwas ever published. It runs when the section appears, when a server is switched on, and
after an edit — a server just switched on has no tool list yet, so without the second of
those it would sit there enabled and offer nothing.
forget(id:)had no caller, so a removed server's tool list and health outlived it.hasNetworkEgresshad no reader. It is said next to the switches, rather than onlyon the Privacy tab — which is not where someone is standing when they turn a server on.
Renaming a server no longer re-enables its tools
The thing the issue flagged as having no test. Published names are derived from the server
name, and the per-tool disable list is keyed on the published name — the one the user
saw when they turned the tool off. So renaming
GitHubtoGitLabturnsgithub__delete_repointogitlab__delete_repo, which is not on the list, and the toolcomes back under a name they have never seen.
That is the worst direction for this kind of mistake: "I never want the agent to do this"
has to survive an edit that had nothing to do with it, and nothing signals when it does not.
MCPRenameMigrationis the rule — pure, and outside both the store and the view, becausewhatever else eventually renames a server has to apply it too.
Where two servers fold to the same namespace, an entry cannot be attributed to one of them
and both move. Same ambiguity
MCPRegistryPlanresolves by dropping the colliding tool, andit errs the same way: towards the tool staying off.
A collision you could not otherwise see
Namespaces fold —
GIT-HUB,Git.Hubandgit__hubare allgit_hub— so two servers canpublish under one prefix, and the registry keeps the first and drops the rest. Correct, and
invisible: the second server sits there enabled, reachable, reporting its tool count, and
offers the model nothing. It is now named while you are still typing. Not blocked, because
two servers may legitimately be called similar things.
What the review rounds found
Round 1 — four findings. A refused save cleared the form and said nothing (
commit()discarded the
Boolthe store returns, so any disagreement between field and store wouldwipe what was typed and save nothing). The namespace collision above was invisible. The
clash check was a private computed property on the view — the shape
CLAUDE.mdsingles out,since a decision inside a
Viewis one the next caller cannot reach. And editing made therow vanish with the form appearing below the whole list, so nothing said which server was open.
Round 2 — two. The spinner was a boolean, so enabling two servers in quick succession
let the first refresh to finish clear it while the second was still going; it is a count now,
cleared in a
deferso a cancelled task cannot leave it turning for the session. AndMCPCatalog.refresh()read the server list only when it built its task group — remove aserver mid-refresh and
forget(id:)cleared its entries, then the results loop wrote themstraight back.
Round 3 — one. Every row's Edit and Remove announced only "Edit" and "Remove", so with
several servers VoiceOver said which action but not which server. Remove is the one where
guessing is expensive.
A test that passed for the wrong reason
The first version of the clash cases asserted
git hubclashes withGitHub. It does not —GitHubfolds togithub,git hubtogit_hub, because a run of non-alphanumericsbecomes a separator. They look like the same name and are not, and it is the same pair
#76's review found already passing vacuously once. There is now a case pinning that they do
not clash, with a note saying why.
Verification
xcodebuild build— succeeds./scripts/test-no-llm.sh— 1786 tests in 160 suites pass--lint— 0/560 files require formatting--strict— 0 violations in 714 filesMutation-checked:
No test touches a socket — the lifecycle case drives a deliberately slow stub transport.
What still needs a person
This is the first code in the feature that can actually reach a network, and the transport
has still only ever been exercised against a stub and the parser. It needs a real server:
been contacted.
tools should be callable from Ask Logue and from the Command Center island, since there
is one pipeline.
gets, and the card should name the server it came from.
http://to anywhere but localhost should be refused as you type;http://127.0.0.1should be accepted and should say "on this Mac".
could not reach it, not fail.
git hubthenGIT-HUB) — theform should say so before you save.
🤖 Generated with Claude Code
https://claude.ai/code/session_01M3Wpnj9ZmWPKYdFPB1AVY3