scripts(mcp_swap): add --scope {user,project} for Claude two-layer config#35
Merged
scripts(mcp_swap): add --scope {user,project} for Claude two-layer config#35
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #35 +/- ##
==========================================
+ Coverage 83.77% 84.81% +1.04%
==========================================
Files 40 40
Lines 2132 2233 +101
Branches 270 290 +20
==========================================
+ Hits 1786 1894 +108
+ Misses 266 255 -11
- Partials 80 84 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
Author
Code reviewFound 1 issue:
libtmux-mcp/scripts/mcp_swap.py Lines 810 to 827 in 7c3e7e7 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…yer config
Claude's MCP config has two layers — top-level mcpServers (used as a
fallback for any project without an override) and per-project
projects.<abs>.mcpServers. Until now mcp_swap only wrote the per-project
entry under the current repo's path, so projects with no per-project
override (e.g. ~/work/cv) kept seeing the user-level fallback unchanged.
Adds an explicit --scope {user,project} flag on use-local and revert.
Default 'project' preserves pre-flag behaviour. --scope user flips the
top-level fallback so unrelated project directories pick up the swap.
Codex / Cursor / Gemini have no per-project layer in their config files;
the flag is silently coerced to 'user' for them via _normalize_scope.
State-file schema bumps to v2: keys go from bare cli names to
"cli:scope" so a single Claude install can hold simultaneous user-scope
and project-scope swaps with independent backups. v1 entries migrate
transparently in load_state — bare 'claude' becomes ('claude','project')
(only mode that existed); the others become ('<cli>','user').
cmd_status now prints per-scope lines for Claude when both exist —
[claude:user] and [claude:project] — and a single [<cli>] line for the
others.
Updates the two existing tests that reach into the state dict to use
the new (cli, scope) tuple keys; all other tests pass unchanged.
…p disambiguation
Five new tests:
- test_claude_user_scope_writes_top_level_mcpServers — confirms the
user scope writes to ~/.claude.json's top-level mcpServers and does
NOT touch the per-project layer.
- test_claude_user_scope_round_trip_restores_byte_identical — swap
then revert at user scope yields byte-identical bytes.
- test_claude_user_and_project_swaps_coexist_independently — both
scopes can swap in the same Claude install with separate state
entries and separate backups; reverting one scope leaves the other
intact.
- test_legacy_v1_state_migrates_to_v2_keys — v1 state files with bare
cli keys load as the right (cli, scope) tuple; bare claude becomes
('claude', 'project'); other CLIs become user-scope.
- test_non_claude_scope_user_passes_through_to_global_config — the
flag is silently coerced for non-Claude CLIs; --scope project
against cursor never produces a phantom (cursor, project) state
entry.
Also fixes a real bug uncovered by the coexist test: two Claude
swaps in the same second collided on backup filename, so the
second backup overwrote the first. Backup suffix now embeds the
scope for Claude (only — non-Claude filenames stay byte-compatible
with v1 backups).
…tion Add a "Scope" subsection to scripts/README.md explaining Claude's two-layer config (top-level mcpServers fallback vs per-project mcpServers override) and how --scope user vs --scope project select between them. Note that the flag is silently coerced for non-Claude CLIs and that both scopes can coexist with independent backups. Add a Dev scripts bullet under unreleased "What's new" summarising the new flag and the v1→v2 state-file migration.
mcp_swap.py is dev-support tooling, not shipped product surface — it helps contributors iterate on the server, but downstream package consumers never see it. ### What's new is reserved for things a user would scan a release for; dev scripts go under ### Development.
… complete cmd_revert iterated cli_keys in dict-insertion (i.e. swap-chronological) order and applied atomic_write to each backup against the same dest. When two scopes back the same physical file (Claude user + project), the second backup contains the first swap's modifications, so forward iteration overwrote the first restoration with the second backup's contents — the file ended up half-rolled-back while state.json was fully cleared. Switch to reversed(cli_keys) so backups are unwound in reverse-registration order (LIFO). Same discipline as contextlib.ExitStack and atexit: when layered modifications form a stack, each layer must be peeled off newest first so it restores the prior layer's state, not the original. Test added: test_claude_full_revert_unwinds_both_scopes_in_lifo_order covers two swaps (project then user) followed by a no-scope revert and asserts byte-identical original. Confirmed to fail on the unfixed code and pass with the fix.
…rite
set_server's user-scope branch did config.setdefault("mcpServers", {})
without first checking the shape. A non-mapping top-level mcpServers
(scalar, list, malformed Claude release) would surface as an opaque
AttributeError from .setdefault() rather than the actionable RuntimeError
that the project-scope path already raises via _claude_project_node.
Add the same isinstance check + RuntimeError pattern that the
project-scope path uses, so the failure mode is symmetric. Mirrors
hatchling's pre-mutation config validation in builders/config.py and
flit's typed-error pattern in flit_core/flit_core/config.py.
Test test_claude_user_scope_rejects_non_mapping_mcpServers covers the
guard; pairs with the existing test_claude_project_node_rejects_non_mapping_*
tests so the shape-validation contract reads symmetrically across both
Claude scopes.
The module docstring's "Scope" section was specifically about scoping behaviour (which configs are walked, which are ignored, how Claude's per-project layer fits in) but did not mention the new --scope flag that this branch added. A reader skimming the docstring would not know the user-level fallback layer is now writeable. Add a "Claude scope" bullet covering: the project / user trade-off, the silent coercion for non-Claude CLIs, the coexistence guarantee, and the LIFO unwind discipline. The bullet sits next to the existing "Global configs only" bullet so the layered config narrative reads top-to-bottom. scripts/README.md and the --scope argparse help already cover this; the docstring update makes the in-file narrative consistent with both.
The comment "No --cli filter:" sat directly above a line that DOES honour args.cli when supplied — only the else branch is "no filter". A reader scanning for CLI-filter logic would expect the line to ignore args.cli entirely, which is the opposite of what it does. Reword as "Without --cli, …" so the caveat is clearly scoped to the fallback path, not the whole expression.
…symmetric guard
The previous round of follow-ups added a shape guard inline at the
write site of set_server's user-scope Claude branch, but get_server
and delete_server's user-scope branches still reached into
config.get("mcpServers", {}) without one. A non-mapping mcpServers
would raise an opaque AttributeError on read or a TypeError on delete
instead of the actionable RuntimeError the project-scope path gives
via _claude_project_node.
Extract _claude_user_servers as a small companion helper to
_claude_project_node. Both now centralise the shape guard once and
serve all three of read / write / delete via overloads on a `create:
bool` parameter, mirroring the pattern Flit uses (`_check_type`
extracted as a utility) and CPython's configparser uses (uniform
`_sections[section]` access across get/set/remove).
Tests: add the read- and delete-side companions to the existing
test_claude_user_scope_rejects_non_mapping_mcpServers so all three
paths raise the same actionable RuntimeError. The existing test stays
in place; the two new ones land beside it so the symmetric coverage
reads top-to-bottom.
… LIFO order cmd_revert's LIFO unwind used reversed(cli_keys) where cli_keys was built by iterating the state dict — making correctness depend on a chain of implicit guarantees (Python 3.7+ dict insertion order → json.dumps preservation → json.loads round-trip preservation). None of these were documented or asserted. CPython's contextlib.ExitStack (the precedent the LIFO comment cites) uses an explicit deque with .pop(), not dict iteration. uv lockfiles .sort_by(id) for deterministic order. Adopt the same pattern: track a swapped_at: str = "" timestamp on each SwapEntry and sort cli_keys by (swapped_at, original_index) DESC for true LIFO. The original_index secondary key preserves the previous behaviour for sub-second collisions (two scopes registered in the same second share the same swapped_at; the larger original index wins, matching the old reversed() semantics). And the comment update folds in the "reapplied" → "restored" wording correction surfaced in the same review pass. Schema bumps to v3 with transparent migration: v2 entries that lack swapped_at synthesise it from the timestamp embedded in backup_path (<config>.bak.mcp-swap-<YYYYMMDDHHMMSS>[-scope]) via the new _BACKUP_TS_RE / _swapped_at_from_backup pair. The load path also filters unknown SwapEntry kwargs so a forward-version state file with extra metadata still loads without crashing. Tests: three new tests cover (a) swapped_at is populated by use-local, (b) v2 → v3 migration synthesises the field from backup_path, and (c) revert sorts by swapped_at even when the state file's dict order is deliberately wrong. The pre-existing test_claude_full_revert_unwinds_both_scopes_in_lifo_order continues to pass since same-second collisions are handled by the secondary index key.
Compatibility layers were accumulating: a regex parser for legacy backup_path-derived timestamps, a kwargs filter for forward-compat, a v1-bare-cli branch in _parse_state_key, a swapped_at synthesis block in load_state, a STATE_VERSION constant whose docstring narrated three schema versions. The script is dev tooling under scripts/ — no published API contract — so we declare no-compat and strip it all. Drops: - _BACKUP_TS_RE and _swapped_at_from_backup (regex + helper) - The legacy bare-cli branch in _parse_state_key (only v3 cli:scope form parses now) - The kwargs filter in load_state (SwapEntry(**v) directly) - The "if not entry.swapped_at" migration block - STATE_VERSION constant + its multi-version docstring - The unused `import re` - Two migration tests: test_legacy_v1_state_migrates_to_v2_keys test_legacy_v2_state_migrates_swapped_at_from_backup_path Marks SwapEntry.swapped_at as a required field (no default) so forgetting to populate it would now be a programming error caught at instantiation. Updates load_state docstring to declare "schema is internal — no compatibility contract." Test test_save_state_writes_atomically updated to pass swapped_at explicitly per the new required-field contract. Local state on this machine was reverted and ~/.local/state/libtmux-mcp-dev/swap/ deleted before this commit so the schema change starts clean. Net: -90 lines / +20 lines. The CHANGES + scripts/README.md updates land in a follow-up commit.
…ed.py precedent
cmd_revert previously sorted by (swapped_at, original_index) with
reverse=True, where original_index derived from dict iteration order
through the JSON round-trip — exactly the implicit dependency the
preceding LIFO comment claimed to have eliminated. Same-second
collisions (two scopes registered in one second) fell back entirely
to dict order.
Add a required seq_no: int field on SwapEntry. cmd_use_local computes
the next value as max(existing_seq_nos, default=-1) + 1 — strictly
monotonic per swap, immune to wall-clock collisions, JSON parse
order, and hand-edited state. cmd_revert now sorts by seq_no DESC
alone, dropping the enumerate() plumbing and the (swapped_at,
original_index) tuple key. The LIFO comment becomes honestly true.
Same explicit-counter pattern CPython's Lib/sched.py uses:
Event = namedtuple('Event', 'time, priority, sequence, ...')
self._sequence_generator = count()
swapped_at stays as a human-readable timestamp for anyone inspecting
state.json directly; seq_no enforces sort order.
Tests:
- test_use_local_populates_swapped_at → split into
test_use_local_populates_swapped_at_and_seq_no (asserts both
fields populated) and test_seq_no_increments_across_swaps
(asserts second swap's seq_no = first + 1).
- test_lifo_revert_orders_by_swapped_at_not_dict_iteration →
renamed to test_lifo_revert_orders_by_seq_no_not_dict_iteration;
state-file seed now includes explicit seq_no values; assertion
unchanged (file restores to original).
- test_save_state_writes_atomically updated to pass seq_no.
…2/v3 talk Both the CHANGES Development entry and the scripts/README.md Safety section referenced "v2 schema" / "v1 migration" — narratives that were stale even before the round-2 work bumped to v3, and irrelevant now that all migration code is gone (no-compat dev tooling). Replace with what actually shipped: --scope flag (with both scopes coexisting), centralised shape validation via _claude_user_servers / _claude_project_node helpers, and explicit seq_no LIFO ordering. Note that the schema is internal — no compatibility contract; running older mcp_swap against newer state.json is undefined behaviour.
…e claim The cmd_revert LIFO comment listed "hand-edited state" alongside "JSON parse order", "dict iteration", and "wall-clock collisions" as things the seq_no sort is independent of. That overclaimed: load_state doesn't validate seq_no's type or values, so a hand-edited state.json with seq_no="foo" or duplicates would crash or misorder. Drop "hand-edited state" from the independence list and add a sentence clarifying that hand-edits to seq_no values are outside the guarantee. The remaining claims (JSON parse order, dict iteration, wall-clock collisions) are actually true.
After commit 874dbcb stripped all migration code, the script no longer carries any "v1" / version-tracking vocabulary — except this one comment in cmd_use_local that still referenced "byte-compatible with v1 backups". The intent was that non-Claude backup filenames keep the un-suffixed shape, not introduce a scope suffix. Restate without the version language: "Non-Claude backup filenames carry no scope suffix." Same observable behaviour, no orphaned vocabulary.
…ion" The "Out of scope: Custom binary install locations" bullet referenced "post-migration paths (~/.claude/local/claude, ~/.gemini/local/gemini)" — meaning the install layouts users land on after Claude / Gemini moved away from npm-global. The word "migration" reads ambiguously now that the script's own state-schema migration code has been stripped: a fresh reader sees "migration" and pattern-matches to the recently removed compat layer. Restate as "the canonical local-install layouts" — same paths, no "migration" word, no ambiguity. Pure terminology cleanup; no behavioural change.
…— no traceback set_server / get_server raise RuntimeError from the Claude shape guards (_claude_user_servers, _claude_project_node) when ~/.claude.json's mcpServers or projects key is not the expected mapping shape. The error message is actionable, but cmd_use_local's per-CLI try/except wrapped only atomic_write + _revalidate, so the shape-guard exception propagated past it and surfaced as a Python traceback at the CLI boundary. Add a per-CLI try/except RuntimeError around the config-prep region (read + load_config + get_server + set_server + dump) in both cmd_use_local and cmd_status. Same per-CLI continuation pattern the existing write-failure handler uses. Pattern follows pytest's main-level UsageError formatter (_pytest/config/__init__.py:168-218): catch the *domain* exception specifically (RuntimeError), not Exception broadly — that would mask bugs. Tests: a malformed top-level mcpServers triggers exit 1 with the RuntimeError message tagged [claude:user] on stderr (no traceback); status continues to print other CLIs' lines when one CLI's config is corrupt.
…e load
cmd_revert sorts cli_keys by SwapEntry.seq_no for LIFO ordering. If a
hand-edited state.json had a non-int seq_no in one of two same-CLI
entries (e.g. claude:user + claude:project), the sort raised
"TypeError: '<' not supported between instances of 'int' and 'str'"
and the entire revert aborted with a traceback. Failure mode was
asymmetric: cross-CLI buckets (cursor + gemini) are length-1 and
never invoke comparison, so the same corruption silently worked
there. The cmd_revert comment acknowledged the gap explicitly
("hand-edited state.json with corrupted counter values is outside
the guarantees here") rather than fixing it.
Validate seq_no at load time. New helper _parse_state_entry coerces
seq_no via int() and returns None for any KeyError / TypeError /
ValueError; load_state drops the entry. Pattern follows CPython
sched.py's discipline (Lib/sched.py:73,138): validate at the
counter's *origin* (enterabs/load_state), not at sort time. Extends
the existing _parse_state_key drop-on-malformed pattern so the
script's resilience model stays uniform — schema is internal, no
compatibility contract, silent drop on parse failure.
The cmd_revert sort comment is updated to reflect that seq_no is
now always int-vs-int by the time the sort runs.
Tests: a non-coercible seq_no string drops the entry without
crashing; a numeric-string seq_no is coerced rather than dropped
(distinguishes "user typed quotes around the number" from "user
typed something non-numeric"); missing required fields drop the
entry; a same-CLI two-scope state with one corrupt entry no longer
raises TypeError on revert.
cmd_revert restored each CLI's config from its .bak.mcp-swap-<ts>[-scope] backup but never deleted the backup file. Repeated swap/revert cycles left ~/.claude.json.bak.* and friends accumulating indefinitely — particularly visible for Claude where each scope swap writes its own scope-suffixed backup. clear_state cleaned up state.json entries; no parallel discipline existed for the backup files. Delete on success, keep on error — the standard idiom from CPython's tempfile.NamedTemporaryFile (Lib/tempfile.py:614-618). The new backup.unlink() runs only after atomic_write succeeds, so a failed revert (e.g. dest unwritable mid-restore) still leaves the backup on disk for post-mortem. The dry-run path was already a `continue` before the restore, so it naturally skips the unlink as well. Tests: the backup file is gone after a successful revert; --dry-run preserves it. Both lock the lifecycle invariant against future refactors of the loop body.
use-local and revert both accept --scope {user,project}; status was
the asymmetric outlier — it always printed both Claude scope lines
when both had entries. For someone QA-ing only the user-level
fallback (e.g. across many unrelated repos), or only the per-project
override for the current repo, scoped status output is useful.
Add --scope to the status argparse with the same shape and help
text used on use-local / revert. cmd_status gates the user-scope
and project-scope get_server reads on args.scope (lazy reads — skip
the get_server call entirely for the filtered-out scope so a
malformed projects node doesn't raise when the user only asked
about user scope). Non-Claude CLIs ignore the flag (their config
has no per-project layer). When the selected scope has no entry,
the "no entry" line is scope-tagged ([claude:user] no entry for
...) for symmetry with use-local / revert output.
Tests: --scope user filters out project, --scope project filters
out user, default behaviour unchanged, --scope is a no-op for
non-Claude CLIs, scope-tagged "no entry" line appears when the
selected layer is missing.
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
2 participants
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.
Summary
--scope {user,project}toscripts/mcp_swap.py'suse-localandrevertsubcommands.--scope userrewrites Claude's top-levelmcpServersfallback (every project without an override picks it up);--scope project(the default — preserves pre-flag behaviour) writes the per-projectprojects[<abs>].mcpServersentry.clistrings tof"{cli}:{scope}"so a single Claude install can hold both scopes simultaneously with independent backups. v1 entries migrate transparently on load (claude→("claude","project"); others →("<cli>","user")).scripts/README.mdwith a "Scope" subsection and the layered-config explanation that motivates it.Motivation
mcp_swap.pywas conservative by design — it only wrote under the current repo's per-project key in~/.claude.json, leaving every other project's MCP entry untouched. Side-effect: when QA-ing a libtmux-mcp branch from any non-libtmux-mcp directory (e.g.~/work/cv), Claude kept resolving the user-level fallback to the PyPI release. The flag adds an explicit "yes, swap the global default" path without losing the safe-by-default per-project behaviour.Changes by area
Script (
scripts/mcp_swap.py)Scopeliteral + helpers: NewScope = Literal["user","project"], plus_normalize_scope(coerces non-Claude scope to"user"),_state_key, and_parse_state_key(the v1→v2 migration shim). All centralised so the rest of the file just threadsscopethrough.get_server/set_server/delete_server: New keyword-onlyscopeparameter (default"project"). Claude branches gate on it; non-Claude branches ignore it._claude_project_nodeis unchanged — only called whenscope == "project".load_state/save_state/clear_state: Switched fromdict[CLIName, SwapEntry]todict[tuple[CLIName, Scope], SwapEntry]. Migration runs inload_statevia_parse_state_key; unknown keys are dropped silently.cmd_status: For Claude, prints separate[claude:user]/[claude:project]lines when both have entries (or one if only one exists). Non-Claude CLIs continue to print one[<cli>]line.cmd_use_local/cmd_revert: Readargs.scope, normalise per CLI, key state by(cli, scope).revertwithout--scoperolls back every recorded scope for the targeted CLIs; with--scope, only that one.build_parser: New--scopeargument onuse-localandrevertwithchoices=ALL_SCOPESanddefault=None.Tests (
tests/test_mcp_swap.py)test_claude_user_scope_writes_top_level_mcpServers--scope userwritesmcpServersand creates noprojects[<abs>]nodetest_claude_user_scope_round_trip_restores_byte_identicaltest_claude_user_and_project_swaps_coexist_independentlytest_legacy_v1_state_migrates_to_v2_keyscliv1 keys load as the right(cli, scope)tupletest_non_claude_scope_user_passes_through_to_global_config--scopeis silently coerced for non-Claude CLIs; never produces a phantom(cursor, project)Two existing tests were updated to use the new
(cli, scope)tuple keys (test_codex_adds_block_when_absent_and_revert_removes_it,test_save_state_writes_atomically).Docs
scripts/README.md— new "--scope {user,project}(Claude only)" subsection with the two-layer config explanation, the silent-coerce-for-others rule, and the simultaneous-scopes paragraph. Safety section updated to note the scope-suffixed Claude backups and the v2 schema with v1 migration.CHANGES— entry under### Development(per project convention: dev tooling is not shipped product surface).Design decisions
--scope project: keeps every existing call site — includingjust mcp-use-localfrom the libtmux-mcp repo itself — behaviourally identical. The flag is purely additive.--scopeis meaningless for them. Erroring would create friction in scripts that pass--scope userfor cross-CLI consistency._normalize_scopedoes the coerce in one spot.(cli, scope)) in memory;cli:scopestrings on disk: Tuple gives static guarantees on key shape; the string form is friendlier for the JSON file and stays human-editable._parse_state_key, not at write time:save_statealways emits v2; the migration runs once whenever a v1 file is read, then disappears on the next write. No long-lived "v1-mode" state to leak.*.bak.mcp-swap-<ts>files for users mid-revert.Verification
The flag-related code paths are all in
scripts/mcp_swap.py:rg -n 'scope' scripts/mcp_swap.pyState-file schema bump is reflected at one constant:
rg -n 'STATE_VERSION' scripts/mcp_swap.pyThe two existing tests that reach into the in-memory state dict were updated; no other test file references the swap state shape:
rg -n '("claude"|"codex"|"cursor"|"gemini")\s*[:,].*Swap' tests/Test plan
test_claude_user_scope_writes_top_level_mcpServers— user-scope writesmcpServers, leavesprojects[<abs>]untouchedtest_claude_user_scope_round_trip_restores_byte_identical— round-trip preserves bytestest_claude_user_and_project_swaps_coexist_independently— both scopes coexist; selective revert workstest_legacy_v1_state_migrates_to_v2_keys— v1 keys (claude,codex, ...) migrate to(cli, scope)tuplestest_non_claude_scope_user_passes_through_to_global_config—--scopesilently coerced for non-Claudetests/test_mcp_swap.pytests still pass (two updated for new key shape)just ruff— cleanjust ruff-format— no diffjust mypy— strict, zero errorsuv run pytest --reruns 0— full suite greenrm -rf docs/_build && just build-docs— same warning baseline asmainManual QA (real configs, post-backup)
Run from
/home/d/work/python/libtmux-mcpafter backing up~/.claude.json/~/.codex/config.toml/~/.cursor/mcp.json/~/.gemini/settings.jsonto a safe location:Confirmed end-to-end on this branch. The user-scope swap reaches every project that has no per-project override; reverting it leaves the per-project libtmux-mcp swap intact.