[AI-1709] Dedupe MCP registrations and retire the resident node wrapper for kcap mcp - #440
Merged
Merged
Conversation
…trations Pure functions over ~/.claude.json content covering both scopes (top-level mcpServers and projects[<path>].mcpServers). Classification is structural, never name-only: only a semantically canonical copy of a plugin-shipped server is a removable duplicate; divergent same-name entries are reported as conflicts and preserved. Also extracts absolute-path kcap commands for the doctor's stale-path check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ent worktrees The launcher copied every projects[<repo>].mcpServers entry from ~/.claude.json into the agent worktree's .mcp.json, so a user-scope copy of a plugin-shipped kcap server spawned a duplicate resident server process per agent session. Skip entries that are semantically canonical duplicates — structural classification, never name-only, and gated on the Claude plugin actually being installed. Divergent same-name entries and non-kcap servers keep merging unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n install/refresh The plugin ships "command": "kcap", which resolves to the node wrapper — one resident Node runtime (~25 MB idle) per MCP server per open Claude session. runRefreshes (postinstall + kcap update, the two moments the plugin dir is (re)written) now rewrites the shipped kcap/.mcp.json to the resolved native binary: validate-then-atomic-rename, only the six canonical name/args pairs, customized entries preserved, stale absolute paths from a previous layout re-patched, and any failure degrades to ONE warning with the shipped wrapper command left working. Platform/binary resolution moves to a new side-effect-free bin/resolve.js shared by kcap.js and refresh.js — refresh.js must never require kcap.js, whose final module.exports has not yet executed during kcap update's runUpdate. Plain-Node tests cover the release package layout (npm pack can't produce it), and all three wrapper test files are now wired into CI, with kcap.test.js's fixtures made host-native so they pass beyond Windows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Generated MCP configs (the six JSON harnesses + Codex TOML) now write the resolved native binary path (Environment.ProcessPath, with a Func<string?> test seam) as the command instead of the wrapper-resolved literal "kcap", so harnesses spawn the binary directly — no resident node wrapper per server. A null/blank resolution falls back to "kcap". Ownership evolves to make that shape maintainable: - McpMarker v2 records a per-entry fingerprint (SHA-256 over a key-sorted canonical serialization, the JSON analogue of Codex's ownership ledger); Owns matches the exact recorded shape, so refresh-healing and uninstall keep working for absolute-path entries while any user edit relinquishes ownership. v1 markers (bare name arrays) read with the legacy command == "kcap" semantics and migrate to v2 on the next record; the marker is now recorded only AFTER the config write commits (the CodexConfigToml crash ordering: a crash leaks a preserved unowned entry rather than claiming a shape that never reached disk). - CodexConfigToml heals an owned entry whose ledger fingerprint still matches (kcap -> absolute A -> absolute B relayouts re-point), keeps never touching unclaimed or user-customized entries, and unregisters absolute-registered owned entries. Tests cover new-install, v1->absolute migration, relayout healing, customized-entry preservation, and absolute-entry uninstall for both formats (mutation-checked on the Codex fingerprint gate); the four new Codex lifecycle tests use a symlink-free temp root so they also run on macOS, where the suite's existing temp paths trip the writer's symlink guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kcap daemon doctor gains an MCP-registrations section that runs BEFORE the daemon-file early return, so a machine with no daemon state still gets audited. It reports (and with --clean removes) user- and project-scope Claude registrations that duplicate the kcap plugin's shipped servers — structural classification, never name-only; divergent same-name entries are reported as conflicts and preserved; the whole check is gated on the plugin actually being installed. It also scans every kcap-written registration file (the six JSON harnesses, Codex TOML, and ~/.claude.json itself) for absolute binary paths left behind by an npm re-layout, distinguishing a missing binary (re-run kcap setup) from one that merely differs from the current resolution (healed by the next setup/update). Read-only by default; the clean rewrite is atomic (sibling temp + rename). All paths are injected so tests run against fixture files in temp dirs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g the test runner The seven generated-driver conformance arms (and the per-harness command assertions) expected the literal "kcap" and went red on both CI legs once registration started writing the resolved binary path — and asserting Environment.ProcessPath would only have blessed whatever executable happens to run the tests. PluginEnvironment gains ResolveMcpBinaryPath (null = the production ProcessPath default), threaded through all seven PluginCommand register sites (six JSON harnesses + Codex TOML); the conformance suite and the per-harness tests inject a fixed path and assert exactly that value. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…assification
An arbitrary cwd changes the server's execution context (repo scoping), so
treating any string as cosmetic let doctor --clean delete and the launcher
merge-skip suppress a genuinely customized entry. cwd is now canonical only
on a server that requires a project cwd, and only as the shipped
${CLAUDE_PROJECT_DIR} placeholder or - for a project-scoped entry - exactly
the project's own path (what the placeholder expands to). Any other cwd, or
any cwd on a server that takes none, is a conflict and is preserved.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…l CLI path Two gates were weaker than the actions they authorized: - ClaudePluginInstaller.IsInstalled accepts the .kcap-plugin-version marker alone - correct as the refresh gate, but a stale marker (manual removal, failed refresh, npm re-layout) must never authorize doctor's duplicate cleanup or the launcher's merge-skip. Both now gate on the new IsEffectivelyInstalled: an enabled plugin registration AND a resolvable payload (the registered marketplace dir still ships .mcp.json). - Inside the daemon, Environment.ProcessPath is kcap-daemon, not the native kcap the registrations point at, so a canonical absolute-path entry was misclassified as divergent and the duplicate copied into the worktree anyway. DaemonConfig.KcapCliPath resolves the CLI as the daemon's sibling (KcapBinaryCommand.ResolveCliSibling) and is threaded into ClaudeLauncher.WriteMcpConfig, which also now passes the matched project key so a project-scoped cwd equal to the project path stays canonical. Both gates are mutation-checked: reverting either fails the new marker-only / absolute-path tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… the marker - doctor --clean rewrote a snapshot read at report time, so a concurrent ~/.claude.json update (Claude itself, the daemon's trust write) landing between the read and the rename was silently dropped. No shared cross-process lock exists for that file in this codebase (the daemon's trust write serializes only in-process), so the commit now runs under a named mutex keyed on the config path (the CodexConfigToml lock shape), re-reads INSIDE the lock immediately before committing, and aborts with a visible 'changed while doctor was running' notice when the file no longer matches the snapshot - the other writer's update survives. - the clean's temp file took default umask perms, turning a 0600 config into 0644 after the rename; the rewrite now preserves the target's Unix mode (owner-only fail-safe for unreadable/new files). - McpMarker.Record truncate-rewrote the marker in place; an interrupted write would strand every absolute-path entry as unowned forever (no heal, no uninstall). It now writes a sibling temp owner-only and renames atomically. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… source IsEffectivelyInstalled checked the marketplace SOURCE dir for .mcp.json, but Claude loads plugins from its installed cache - the source proves nothing about what is installed or active. The gate now resolves the enabled kcap plugin key's entry in plugins/installed_plugins.json (the per-scope installPath cache dir), with one verified exception: a directory-sourced marketplace is loaded LIVE and its recorded cache path never materializes, so known_marketplaces.json's installLocation is consulted when no cache payload exists. Anything unresolvable stays fail-closed: not effective, no destructive action. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…fe writes - The daemon's TrustWorktreeInClaudeConfig wrote ~/.claude.json under only an in-process lock, so it could commit between doctor --clean's inside-lock re-read and rename and be silently overwritten. The named mutex is extracted into Core (ConfigFileLock) and ALL kcap writers of a shared config file take it: doctor's clean, the trust writer, and the Codex TOML engine. A deterministic interleaving test parks the clean inside its lock and proves the trust write blocks and lands on top - both changes survive (mutation-checked: dropping the trust writer's lock loses the trust entry). - The bare mutex name was session-local on Windows (a service-session daemon and the login-session CLI never excluded each other) and open to same-session squatting. ConfigFileLock uses an explicit Global\ name with a current-user-only DACL (System.Threading.AccessControl, Windows-guarded); CodexConfigToml had the same gap and now shares the helper - cross-version lock-name mismatch during upgrade is accepted and noted, as the lock guards rare explicit admin operations. - The doctor's Windows rename published the temp file's inherited DACL over an explicitly-restricted .claude.json ACL; the commit now uses File.Replace (ReplaceFile semantics, preserves the destination's ACL/attributes) on Windows, keeping the POSIX mode-preserving branch. - ResolveCliSibling documents the accepted daemon-only-layout limitation (unsupported by design, fail-safe: the worktree keeps the wrapper duplicate - no data loss, no config knob). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…al entries Moving the marker record after the config commit (round 1's crash ordering) put it outside Update's exception boundary: a marker-write failure threw through setup/plugin install AFTER the registrations had committed - violating the never-fails-install contract and stranding the absolute-path entries without ownership (no heal, no uninstall). The record is now guarded (a failure degrades silently; the config result - the user-visible artifact - stands), and the recovery story is made real: a register pass that finds an unowned entry EXACTLY equal to what it would write adopts it, re-recording ownership without touching the config. Adoption strands nothing user-authored (uninstall would remove only a shape indistinguishable from kcap's own write) and it is what heals both the marker-failure case and the config-committed-then-crash window. Both halves are mutation-checked: unguarding the record throws the fake marker's IOException through Register; deleting the adoption lane fails the re-adoption round-trip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…imitation A layout shipping kcap-daemon without its sibling kcap resolves null by design - every official layout publishes both binaries together, so this is unsupported rather than configurable. The failure direction is safe: the registration is merely not recognized as canonical and the worktree keeps the wrapper duplicate; nothing is lost or suppressed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…user scope Two conjuncts the round-2 gate was missing: - The live-resolution marketplace fallback now requires source.source == "directory". A git/github marketplace IS cached, so a lingering checkout under installLocation proves nothing once the installed cache payload is gone - accepting it would let doctor --clean delete the user's only working registrations. - v2 install records are filtered to scope == "user": both callers gate on the user-scope settings.json enabled flag, so a project/local-scoped install belonging to an unrelated repo must not make the plugin globally effective. The bare-object pre-v2 record shape predates scopes and stays accepted. Both conjuncts are mutation-checked; fail-closed direction preserved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d path Codex round-4 on #440: a project-only v2 record could fall through to the directory-marketplace fallback — the fallback only excuses a phantom cache path on an eligible record, never the absence of one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
NO FINDINGS |
The fixture interpolated the temp root into a TOML BASIC (double-quoted) string, where backslash introduces escapes - on the Windows CI leg the temp path (C:\Users\...) made the file invalid TOML, Tomlyn threw, the never-throws ReadMcpServerCommands returned an empty list, and the test expected 1 issue but found 0. Verified against the repo's exact Tomlyn 2.4.0: the basic-string form throws TomlException for a Windows path while the literal (single-quoted) form parses for both Windows and POSIX paths. Production code is unaffected: the writer emits properly-escaped TOML via TomlSerializer, the reader accepts any valid TOML, and the absolute-path detection handles real Windows paths. Fixture-only fix - literal strings are also the canonical hand-written form for Windows paths in TOML. Co-Authored-By: Claude Fable 5 <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.
Problem
Every Claude Code session spawns one stdio pair (resident node wrapper + NativeAOT binary) per kcap MCP server — ~36 MB/pair unswapped, ~2 GB across a heavy multi-session day — a primary driver of the swap exhaustion that degraded the daemon and killed review flows on 2026-08-02. The node half exists only because
kcap.js's "exec" is actually spawn-and-wait; a user-scope duplicate registration multiplied it further.Fix (per the reviewed spec/plan on AI-1709 — Phases 1–2; merge-tier and idle-exit deliberately deferred)
McpRegistrationAudit, both Claude scopes — top-level andprojects[…].mcpServers): a user-scope entry is a removable duplicate only when semantically canonical; divergent same-name entries are reported as conflicts and preserved (repo policy: same name ≠ ownership).ClaudeLauncherstops propagating plugin-shadowed canonical entries into agent worktrees (plugin-installed-gated; mutation-checked both ways).kcap daemon doctorgains an MCP-registrations section (runs before the daemon-file early return; honorsCLAUDE_CONFIG_DIR): reports duplicates/conflicts,--cleanremoves only canonical duplicates via atomic rewrite, and a stale-path scan covers the six JSON harness configs + Codex TOML + Claude's config, distinguishing missing-binary ("re-run kcap setup") from differs-from-current-resolution.Environment.ProcessPathwith seam +"kcap"fallback) — threaded through the JSON writers ANDCodexConfigToml.McpMarkerevolved to v2 per-entry fingerprints (SHA-256 over canonical JSON; v1 read with legacy semantics, migrates on next record; marker recorded only after the config write commits) so refresh-healing across binary relayouts and uninstall both keep working with absolute paths, while genuine user edits are never clobbered (mutation-checked).bin/resolve.js(no require-cycle withkcap.js— pinned by a require-cache assertion);refresh.jspatches the shipped plugin.mcp.jsonvalidate-first with sibling-temp + atomic rename, exactly one warning on failure, six canonical pairs only, stale-absolute re-patch. Exercised via a fake release layout (incl. a space in the path); node tests for all three JS files wired into CI on both matrix legs.Tests
Targeted classes green: McpRegistrationAuditTests 14, ClaudeLauncherWriteMcpConfigTests 5, JsonMcpConfigWriterTests 23, McpMarkerTests 13, SetupCommandTests 41, CodingAgentsStepTests 131, six PluginCommand harness classes, McpDoctorSectionTests 6, plus the three node test files (
ok×3). Mutation-checked: launcher skip (both the skip and its structural gate), refresh patch guards (command + args), Codex fingerprint-mismatch heal guard. Pre-existing macOS failure clusters (CodexConfigTomlTests /var-symlink guard ×20, UninstallCommandTests ×11) proven identical failing sets on a detached origin/main baseline — not regressions. AOT publish clean for both csprojs (one IDE0059 in an untouched file, 0-line diff). Full suites delegated to CI.Not done here (needs the live system): the before/after RSS measurement from the issue's recipe — run it post-merge on a fresh session.
Spec + plan + codex spec-review findings: https://linear.app/kurrent/issue/AI-1709
🤖 Generated with Claude Code
Accepted limitation (review finding 5)
ResolveCliSiblingintentionally has no fallback for daemon-without-sibling-kcaplayouts: every official layout (npm platform packages, release archives) ships both binaries together, so those layouts are unsupported by design. The failure direction is safe — the registration is merely not recognized as canonical, so the agent worktree keeps the wrapper duplicate; nothing is lost or suppressed.