Remove what the installer wrote, recognised by shape rather than by name - #328
Merged
Conversation
`commitlore uninstall` takes back a wrapper, a pinned checkout and one MCP entry
per agent config. Everything interesting about it is what it declines to touch.
An entry is matched on its shape and on the wrapper it points at, never on the
key it sits under. Removing by key would take a server a user happened to name
`commitlore`, and would take the other install's entry on a machine carrying two
-- and in both cases the config just loses a server with nothing to report it.
That is the rule failing in the one place nobody would look.
The shape is not one shape. Both installers write `{command, args:["mcp"]}` into
`mcpServers` for three agents, and opencode gets `{type, command:[wrapper,"mcp"],
enabled}` with the command as an array. A recogniser written for the first shape
alone leaves opencode's entry behind, and a removal that finds nothing looks
exactly like a removal with nothing to do.
The path table found something the ticket's own measured inventory missed. It
lists four agent configs; both installers write five. Windsurf, under Codeium's
directory, is wired by `wire_windsurf` and `Wire-McpServersJson` and appears in
neither the ticket's table nor its count. The assertion that caught it reads the
installers rather than trusting a list, which is the whole reason it is
bidirectional -- and the direction that caught this is the one that is easy to
leave out, because a table nobody contradicts looks complete.
TOML is edited by line rather than parsed and re-emitted. A round-trip normalises
quoting, whitespace and key order across the file, and the live cycle shows a
neighbouring `[mcp_servers.someone-else]` block surviving byte for byte, which a
re-emit would not have left alone.
A config that cannot be parsed is left exactly as it was and reported. Rewriting
a file we could not read is how an uninstall destroys a config it was only meant
to remove one key from.
Record-Id: r-uninstall1123
Limit: the installers write five agent configs, not the four the ticket's measured inventory lists; the fifth is Windsurf at .codeium/windsurf/mcp_config.json
Limit: opencode's entry is shaped differently from the other three -- the command is an array -- so one recogniser cannot serve all of them
Limit: a checkout is 1366 files at this head, not the 1206 the ticket measured at 6e1d46d; any assertion bound to that count is stale
Ruled-out: Removing the entry whose key is `commitlore` | it takes a server the user named that way and the other install's entry on a two-install machine, and neither loss is reported
Ruled-out: Parsing the TOML config and writing it back | the round-trip reformats the whole file, which is the "never reformat beyond the one entry removed" this ticket forbids
Ruled-out: Rewriting a config that failed to parse | the failure mode is losing a config we were only supposed to edit one key of
Ruled-out: Removing the Claude Code plugin cache | it is thousands of files this command did not write, keyed by plugin version; naming the step is the honest boundary
Certainty: firm
Blast: local
Undo: easy
Verified: LIVE install-then-uninstall in a scratch HOME with unrelated entries planted first -- dry-run reported four removals and changed nothing, the real run removed the wrapper, the checkout and both entries, and an unrelated server, an unrelated top-level key and a neighbouring TOML block all survived; 34 focused assertions including ten refusal cases; typecheck clean; dist rebuilt; full suite 78 files, 1914 passed, 1 skipped
Unverified: Windows, where install.ps1 writes the same five configs but no uninstall has been executed; the count of 1366 files is this head's, and will move again
CommitLore — record lintTrailers: clean — 2 commits in Active constraints for the paths this PR touchesLimits (75)
Ruled out (192)
Warnings (54)
Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
…agent
The inventory said four agent configs and bound the contract to that number.
Both installers write five: Windsurf, under Codeium's directory, is wired by
`wire_windsurf` and `Wire-McpServersJson 'windsurf'` and appears in neither the
list nor the count. The checkout figure has moved too -- 1206 files at `6e1d46d`,
1366 re-measured at `51abef8`.
The entry-shape sentence was short by two. It names
`{command, args:["mcp"]}` as *the* shape; that covers the three `mcpServers`
agents, and codex spells the same pair as TOML keys, and opencode's command is an
array with two further keys. An implementation built from that sentence alone
leaves opencode's entry behind -- silently, because a removal that finds nothing
is indistinguishable from a removal with nothing to do.
Both corrections come from executing the shipped installer rather than from
reading it, and both were surfaced by the bidirectional assertion the ticket
itself asks for. That is worth recording where the requirement lives: the
argument for reading the installers instead of trusting a list is that the list
was wrong, in the document making the argument.
Record-Id: r-inventory1123
Limit: a measured number in a ticket ages against the code it measured, so a contract bound to one has to name the head it was taken at
Ruled-out: Leaving the inventory as it was and noting the gap only in the pull request | the ticket is the authority a later reader consults, and a pull request is not where a corrected measurement survives
Ruled-out: Binding the implementation to the corrected file count | it will move again at the next merge; the path table is derived from the installers instead, which does not
Certainty: firm
Blast: local
Undo: easy
Verified: install.sh wires five agents and install.ps1 writes the same five paths; a scratch-HOME install at this head produced 1366 checkout files; the focused suites still pass at 34 assertions
Unverified: nothing new -- this commit changes documentation only
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 #272 (T-1123). Supersedes draft #278.
Why #278 could not be rebased
It removes the entry whose key is
commitlore—delete mcp[key]— with no check on what that entry points at.command,argsandwrapperappear nowhere in itssrc/commands/uninstall.ts.That is the failure this ticket names in its own text: "Matching any entry whose command merely contains
commitlorewould also match an unrelated server a user happened to name that way, which is the 'never remove an entry the installer did not write' rule failing in the one case nobody would notice." Key-matching is weaker still. It also removes the other install's entry on a machine carrying two, and neither loss is reported.It is also 71 commits behind with
dist/committed from that point.Recognised by shape, and by which install
isCommitloreEntry(format, entry, wrapperPath)matches the shape and the wrapper it points at. Ten refusal cases are asserted, including our own shape pointing at a different install.The shape is not one shape:
json-mcpServers{command: <wrapper>, args: ["mcp"]}toml-mcp_servers[mcp_servers.commitlore]+command,argsjson-mcp{type: "local", command: [<wrapper>, "mcp"], enabled: true}The ticket's text names only the first. A recogniser built from it alone leaves opencode's entry behind — and a removal that finds nothing looks exactly like a removal with nothing to do.
The bidirectional table found a gap in the ticket
The ticket's measured inventory lists four agent configs. Both installers write five:
~/.codeium/windsurf/mcp_config.jsonis wired bywire_windsurfininstall.shandWire-McpServersJson 'windsurf'ininstall.ps1, and appears in neither the ticket's table nor its count.test/agent-configs.test.tsasserts in both directions — every row is found in both installers, and every config path either installer writes is in the table. The second direction is the one that is easy to omit, because a table nobody contradicts looks complete. It is the direction that caught this.Also re-measured at this head: a checkout is 1366 files, not the 1206 the ticket recorded at
6e1d46d. Nothing asserts that number, but anything that did would be stale.LIVE cycle (required by the ticket)
Real
install.shinto a scratchHOMEwith unrelated entries planted first, then the installed wrapper's ownuninstall:The surviving TOML block is why the codex path edits by line instead of parsing and re-emitting: a round-trip normalises quoting, whitespace and key order across the whole file, which is the "never reformat beyond the one entry removed" this ticket forbids.
Restraint, asserted
--jsoncontains any other entry's name, command or keyhooks uninstallandinject uninstall-claude-hook; the plugin points at/plugin uninstall. None is touchedXDG_DATA_HOMEis honoured, because the installer honours itVerification
test/agent-configs.test.ts+test/uninstall.test.ts— 34 passednpm run typecheckclean;dist/rebuilt and committedinstall.ps1writes the same five configs, but no uninstall has been executed there.