feat(mcp_registry): #201 PR 3 of 5. Filesystem install/uninstall + LockBackend lease + serializer + CLI#332
Merged
Conversation
Spec/36 PR 3 prep landed 6 amendments closing prep-pass findings before implementer dispatch: 1. FilesystemMCPServerRegistryBackend constructor: default lock_backend factory routes through get_default_lock_backend(agent_root) so multi-host operators pinning ATOMIC_AGENTS_LOCK_BACKEND=redis get the right backend automatically (Stream A finding A-2, P0). 2. Constructor signature adds install_lock_timeout: float = 30.0 kwarg per the spec/21 apply_staging_lock_timeout precedent (Stream A finding A-7, P1). 3. New "Install / uninstall semantics" subsection documents the 7-step critical section with dual-probe collision detection (Stream B finding B-4, P1), check_lock_lost discipline (Stream A finding A-6, P1), full crash safety analysis, and the renderer round-trip property the new mcp.py serializer must satisfy (Stream B+C convergent finding B-1, P0 blocker). 4. New "LockBackend integration" subsection documents the context-manager idiom, install_lock_timeout knob, LockBusy -> MCPRegistryUnavailable translation (Stream A+B+D convergent), custom-lock-backend operator surface failure mode (Stream A finding A-8, P2), and non-reentrant default. 5. MUST 9 contract updated to require the context-manager idiom (closing Stream A finding A-3, P0: handle.release() does not exist on LockHandle per spec/21), explicit LockBusy mapping, check_lock_lost discipline, and no-fast-path discipline for absent-name uninstall (Stream A finding A-9, P2). 6. Capabilities label flipped from "PR 1/2" to "PR 3+" reflecting the supports_install / supports_uninstall flag flip landing in this PR. These amendments are spec-only at this commit. Implementation lands in the following commits via parallel Sonnet implementer streams. Per the PR 1+2 cadence: spec amendments commit first, implementation follows, the /ship review army then verifies spec-code coherence end-to-end. Cross-stream prep findings: 58 total (11 P0, 17 P1, 21 P2, 9 P3). Five critical convergences confirmed by 2+ streams independently: the mcp.md serializer gap (B+C), LockBusy -> MCPRegistryUnavailable mapping (A+B+D), lock try/finally discipline for all exit paths (A+B+C), default lock factory instantiation (A+B), CLI exception handler MCPServerAlreadyInstalled gap (D+A). Refs #201. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…alizer + capability flip + MCPRegistryError base Lands the filesystem write paths per spec/36 PR 3 amendments at 3a3a23e: - atomic_agents/mcp.py: render_mcp_md_section + render_mcp_md_full serializers. Round-trip property pinned with 4 new tests; $VAR refs written verbatim (never resolved) per Decision 7. Also removes the pre-existing unused safe_resolve_under import in validate_mcp_server_args (was masked by the re unused-import which serializers now fix). - atomic_agents/mcp_registry/filesystem.py: install + uninstall via context-manager lock idiom + dual-probe collision detection + check_lock_lost discipline + cleanup_stale_tempfiles in __init__. Constructor adds install_lock_timeout kwarg + lazy lock-backend resolver routing through get_default_lock_backend so ATOMIC_AGENTS_LOCK_BACKEND=redis works automatically. Capability flags flip supports_install=True, supports_uninstall=True (MUST 3). - atomic_agents/mcp_registry/backend.py: MCPRegistryError now inherits from AtomicAgentsError so framework-wide catch-alls see registry failures. All subclasses inherit transitively. Closes prep-pass findings: A-1/A-2/A-3/A-4/A-5/A-6/A-7/A-8 (LockBackend integration), B-1/B-2/B-3/B-4/B-5/B-7/B-9/B-10 (install atomicity + serializer), C-Serializer/C4/C5/C1/C2/C9 (uninstall idempotency), E12 (MCPRegistryError base class). Stream 2 ships the CLI + tests + conformance updates in parallel. Refs #201. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ilesystem install tests + conformance MUST 9 / 10 coverage Lands the CLI surface + behavior tests for the PR 3 filesystem write paths per spec/36 amendments at 3a3a23e: - atomic_agents/cli.py: install + uninstall subparsers under mcp-registry with --command / --args / --env / --description / --transport flags; WARN on stderr for --env values that don't start with $ (per decision 3); refuse --description with H2 lines (defense-in-depth); success output prints ONLY the returned Ref.name (never echo env / command / args per PR 1 P0 secret-leak class). Lazy import block updated to include MCPServerAlreadyInstalled + MCPRegistryError. Exception handler adds MCPServerAlreadyInstalled + MCPRegistryError base-class backstop. Top-level description + module docstring updated to remove "PR 3 deferred" language. Pre-existing F541 f-string lint fixed in _corpus_show. - tests/test_mcp_server_registry_filesystem_install.py NEW: 27 tests covering install/uninstall happy paths + collision + cold-start + path-traversal + idempotency + cycle + concurrent install + CLI WARN/refuse/no-leak discipline + _parse_env_flag / _parse_args_flag unit tests. 18 fail until Stream 1 merges (expected); 9 pass now (CLI parser + H2-guard tests that don't reach the unimplemented backend). - tests/test_mcp_server_registry_conformance.py: tightened MUST 3 True-branch for install (assert isinstance returns MCPServerRef, not broad except Exception pass); tightened MUST 3 True-branch for uninstall (assert result is None); added MUST 9 install atomicity + uninstall idempotency tests guarded on capability flags so HTTP backend at PR 4 skips automatically; added MUST 10 post-install consistency test; fixed docstring lie at line 714 (filesystem uses custom single-read-parse, NOT _default_load_all); removed unused os + ValidationResult imports. - tests/test_mcp_server_registry_filesystem_backend.py: removed obsolete @pytest.mark.skip placeholders for install/uninstall (real tests now in test_mcp_server_registry_filesystem_install.py). Closes prep-pass findings: D-F1/D-F2/D-F3/D-F4/D-F5/D-F9/D-F10 (CLI surface), E2/E3/E4/E5/E6/E10 (conformance), B-6 (CLI literal env contract), B-9 (test scaffold for concurrent install), C10/C11 (uninstall conformance tests). Stream 1 ships the serializer + filesystem.install/uninstall + MCPRegistryError base class fix in parallel. Refs #201. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR 3 of #201 flipped FilesystemMCPServerRegistryBackend's static supports_install + supports_uninstall from False to True (per spec/36 Decision 5 capability evolution table + MUST 3 honesty: methods now work, static flag flips True). The doctor capability snapshot test was pinned to the PR 2 baseline (install/uninstall False) and started failing on the PR 3 capability flip. Update the assertions + comment to reflect PR 3+ baseline. Pattern: the same assertion will flip again at PR 4/5 when HTTP backend joins the parametrization (HTTP backend has different static defaults per Decision 5 table). Refs #201. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…fix cluster) Apply 12 fixes from the /ship review army (7 parallel Sonnet specialists + Codex cross-model verification). Five findings were triple-confirmed by multiple independent reviewers. P1 cross-model triple-confirmed: - H2 injection refusal in render_mcp_md_section (mcp.py): newlines in spec.command, any spec.args item, any spec.env key, or any spec.env value now raise ValueError before rendering. Defense-in-depth at CLI (cli.py) refuses newlines at parse time with operator-readable messages naming the offending flag. The phantom-section-injection vector that bypassed collision detection + name validation is closed. Also aligned the description H2 guard to re.match(r'^##\s', line) so tab-separated ##\t<name> is caught (was bypass-able via startswith). - cleanup_stale_tempfiles moved out of __init__ (MUST 2 violation). Constructor is side-effect-free again. New cleanup_stale_tempfiles_for_file in _io.py is scoped to a single target file's siblings (glob, not rglob); install/uninstall call it inside the lock before reading mcp.md. - BackendNotRegistered escape from locks module fixed. _resolve_lock_backend wraps get_default_lock_backend in try/except and re-raises as MCPRegistryUnavailable so the CLI's existing MCPRegistryError catch-all handles it cleanly. Operator typos in ATOMIC_AGENTS_LOCK_BACKEND now produce clean errors instead of raw Python tracebacks. - check_lock_lost broaden except clause: non-LockLost exceptions (ImportError from broken redis dep, AttributeError from malformed backend_state, etc.) now translate to MCPRegistryUnavailable instead of escaping raw. - Lock timeout test added: test_install_lock_timeout_zero_under_contention exercises the spec/36 MUST 9 LockBusy -> MCPRegistryUnavailable contract. Auto-fix cluster: - Late imports (render_mcp_md_full + check_lock_lost) moved to top-level imports for visibility + micro-perf. - Stale docstrings in filesystem.py + backend.py updated to present-tense PR 3+ baseline (removed PR-1 historical claims that misled readers). - test_uninstall_idempotent_double_call now asserts result2 is None. - test_cli_install_warns_on_literal_env_value now asserts exit_code == 0. - test_install_empty_command_raises tightened from pytest.raises((ValueError, Exception)) to pytest.raises(ValueError). - CLI H2 description guard aligned to renderer's regex (catches ##\t). Refs #201. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…resh CHANGELOG.md [Unreleased] gains the PR 3 of 5 entry covering: - Filesystem install/uninstall write paths with LockBackend lease via the context-manager idiom - render_mcp_md_section + render_mcp_md_full serializers with the round-trip property pinned - CLI install + uninstall subcommands with WARN-on-literal-env + H2 injection refusal at both CLI and renderer layers - spec/36 PR 3 amendments (install_lock_timeout kwarg, default lock factory routing through get_default_lock_backend, new Install/uninstall semantics + LockBackend integration subsections, MUST 9 contract updates, Capabilities label flip to PR 3+) - MCPRegistryError rebased to inherit from AtomicAgentsError - 5 cross-model triple-confirmed review army findings closed inline - 5-stream prep pass + 7-subagent review army cross-checked - Test delta +33 net new (3199 to 3232, 0 regressions) CLAUDE.md test count refreshed from 3,199 (2026-06-03 baseline) to 3,232 (2026-06-04) at both occurrences. Refs #201. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Sync README + CLAUDE.md + architecture + other docs that reference MCPServerRegistryBackend or the test count. Fix drift where docs cite the pre-PR-3 baseline (e.g., supports_install=False on the filesystem backend, stale test counts, "install deferred to PR 3" language). Refs #201. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
1 participant
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
PR 3 of 5 of the MCPServerRegistryBackend arc (#201). Ships the filesystem write paths + the mcp.md serializer that was missing from the codebase + the LockBackend lease around the read-modify-write critical section + CLI install/uninstall subcommands. After this PR, operators on every backend have
atomic-agents mcp-registry installas the canonical install surface; filesystem write paths are proven before PR 4 ships the HTTP backend.Architecture (7 substantive commits):
atomic_agents/mcp.pygainsrender_mcp_md_section+render_mcp_md_fullserializers (4 round-trip tests);atomic_agents/mcp_registry/filesystem.pylandsinstall()+uninstall()with context-manager lock idiom + dual-probe collision detection + check_lock_lost discipline; capability flags flip True;MCPRegistryErrorrebased to inherit fromAtomicAgentsErroratomic_agents/cli.pyaddsinstall+uninstallsubparsers + handlers with WARN-on-literal-env per decision 3 + H2 injection refusal + exception handler updates + lazy import block updates;tests/test_mcp_server_registry_filesystem_install.pyNEW (28 tests); conformance suite gains MUST 3 True-branch tightening + MUST 9 atomicity + MUST 9 idempotency + MUST 10 post-install consistency testsTest Coverage
Test delta: +33 net new (3,199 collected before PR 3, 3,232 after; 3,176 passing + 56 skipped + 0 failures + 0 regressions across the full suite).
tests/test_mcp.py(+4): serializer round-trip basic / env-not-resolved / strips-description-newlines / full-filetests/test_mcp_server_registry_filesystem_install.pyNEW (28 tests): install happy path + cold-start mcp.md creation + collision raising MCPServerAlreadyInstalled + path-traversal name raising ValueError + empty-command rejection +$VARenv round-trip + install/load round-trip + uninstall present/absent/double-call/install-uninstall-install cycle + concurrent same-name exactly-one-wins + concurrent different-names all-win + lock-timeout-zero-under-contention + CLI no-env-echo + CLI WARN on literal env + CLI refuses H2 in description +_parse_env_flag/_parse_args_flagunit teststests/test_mcp_server_registry_conformance.py(+3 new + 2 tightened + 1 docstring fix): MUST 9 install atomicity + MUST 9 uninstall idempotency + MUST 10 post-install consistency added; MUST 3 install/uninstall True-branches tightened fromexcept Exception: passto typed assertions; line-714 docstring corrected to reflect the custom single-read-parse implementationtests/test_mcp_server_registry_filesystem_backend.py(-21): placeholder@pytest.mark.skip("PR 3")stubs removedtests/test_mcp_server_registry_doctor.py(+6/-5): capability snapshot assertions flipped from PR 1/2 False baseline to PR 3+ True baselinePlan Completion
14/14 DONE per /ship Step 8 plan-completion audit against
~/.gstack/projects/dep0we-atomic-agents-stack/dep0we-main-pr3-prep-notes-20260604.md. All 6 spec amendments, 8 code items, and 4 test items delivered with file:line citations. Cross-stream handoffs (serializer dependency between Stream 1 and Stream 2 tests) merged cleanly via worktree isolation.Pre-Landing Review
7 issues found (1 critical, 6 informational) by the /ship checklist pass. All triple-confirmed CRITICAL/P1 items closed inline in commit c4ffd14:
MCPServerSpec(command="npx\n## evil\ncommand: sh", ...)andinstall()would write multi-section content the parser interpreted as MULTIPLE H2 sections, bypassing collision detection + name validation with no audit record.render_mcp_md_sectionnow raises ValueError on newlines in any of these fields; CLI defense-in-depth refuses newlines at parse time with operator-readable errors naming the offending flag. Description H2 guard regex aligned (re.match(r'^##\s', line)) to catch the##\ttab-separated case..*.tmpanywhere underagent_rootviarglob, changing filesystem state during construction (violates side-effect-free MUST) and could delete unrelated user/application tempfiles, including from read-only commands likelist. Newcleanup_stale_tempfiles_for_file(target)helper in_io.pyscoped to a single target file's siblings (glob, not rglob); install/uninstall call it inside the lock before reading mcp.md. Constructor is side-effect-free again.ATOMIC_AGENTS_LOCK_BACKENDraisedatomic_agents.exceptions.BackendNotRegistered(NOT subclass of MCPRegistryError) which escaped the CLI handler as a raw Python traceback._resolve_lock_backendnow wrapsget_default_lock_backendin try/except and re-raises asMCPRegistryUnavailableso the CLI's existing catch-all handles it cleanly.try/except LockLostpattern only caught LockLost; ImportError from broken redis dep, AttributeError from malformed handle.backend_state, etc. escaped raw. Broadened to catch any exception and translate to MCPRegistryUnavailable.test_install_lock_timeout_zero_under_contentionnow exercises the spec/36 MUST 9 LockBusy → MCPRegistryUnavailable contract.Auto-fix cluster also applied: late imports (
render_mcp_md_full+check_lock_lost) moved to top-level; stale docstrings in filesystem.py + backend.py updated to PR 3+ baseline; test assertion gaps closed; CLI H2 description guard aligned to renderer.Specialist Review
Seven parallel Sonnet specialists (testing + maintainability + security + performance + plan-completion + checklist-pass + Claude-adversarial) plus Codex cross-model verification. Zero P0 findings for the secret-leak class (PR 1's symmetric bug class did NOT return in PR 3). Cross-stream convergence on 5 findings — strongest possible "this is real" signal.
Adversarial Review
Claude adversarial subagent + Codex
codex exec(both at high reasoning). Both independently flagged the H2 injection vector (P1, Recommendation: "block before shipping") and cleanup_stale_tempfiles (P1). Both fixes applied in c4ffd14.Eval Results
No prompt-related files changed — evals skipped per /ship Step 6 gate.
Greptile Review
Not applicable — PR didn't exist during /ship Step 10.
TODOS
No TODOS.md exists in the repository. Per /ship Step 14, the user declined to create one this cycle. Project follow-up tracking remains in GitHub Issues.
Documentation
[Unreleased]covering filesystem install/uninstall + LockBackend lease + render_mcp_md serializers + CLI install/uninstall + spec/36 PR 3 amendments + the 5 cross-model triple-confirmed review army findings + test delta (+33).Test plan
uv run ruff checkclean on all PR 3 filesuv run ruff format --checkclean on all PR 3 files🤖 Generated with Claude Code