feat(installer): unify skill + MCP agent registration (16 agents)#41
Conversation
|
@claude review |
|
🤖 Claude security review requested by @lxcong. Running against HEAD |
|
🤖 Claude security review — HEAD: de7ef49 Scope: 6-file installer refactor unifying skill + MCP agent registration for 16 agents (install.sh, install.ps1, uninstall.sh, uninstall.ps1, dev-smoke.sh, .claude/CLAUDE.md) 💡 Suggestion (nice-to-have)
Review triggered by @lxcong |
Drive both `npx skills add -a` and `@agentkey/mcp --auth-login --only` from a single detected-agent list so MCP registration follows the same per-host auto-detection we already do for skill install. Lifts MCP auto-registration from 3 clients (Claude Code, Claude Desktop on mac/win, Cursor) to 16 — see `MCP_AUTO_AGENTS` in install.sh / `$McpAutoAgents` in install.ps1, mirroring `AGENT_REGISTRY` in AgentKey-Server/mcp-server/src/lib/mcp-clients.ts. Highlights: * Split `claude-desktop` out of the `claude-code` marker. The old marker treated `~/Library/Application Support/Claude` (Desktop's config dir) as a `claude-code` hit, which made `skills add -a claude-code` target a nonexistent Claude Code on Desktop-only machines. Now claude-desktop is its own id, listed in `MCP_ONLY_AGENTS` so it's passed to `--auth-login --only` but never to `skills add -a` (which would error — Claude Desktop has no skill install path). * Detect Claude Desktop two ways: app bundle (`/Applications/Claude.app` on mac, `%LOCALAPPDATA%\AnthropicClaude` on win) OR config dir. Covers "installed but never launched" where the dir doesn't exist yet. Linux still requires the config dir. * New edge-case branch in both scripts: `--only claude-desktop` (or any selection of only MCP-only ids) now SKIPS the skill step entirely instead of falling through to `skills add -a` with no filter — which was a silent target-list-defeat bug. * Pass `--only $MCP_TARGETS` to `--auth-login`. Older `@agentkey/mcp` versions silently ignore unknown flags, so this is forward-compatible. Uninstaller (the bigger gap): * Extend MCP config cleanup from 3 paths to 14 (codex TOML included). * JSON scrub is now schema-agnostic — walks the tree and drops any dict key whose name EXACTLY matches our server name (current + legacy), so the same scrubber handles `mcpServers.<name>`, `mcp.<name>`, `amp.mcpServers.<name>`, and `projects.X.mcpServers.<name>` in one pass. Exact-match (not substring) preserves user keys like `agentkey-helper`. * Codex TOML cleanup via awk / PowerShell line-splice — drops the `[mcp_servers.agentkey]` block (bare + legacy quoted name) without needing a TOML parser dep, preserving sibling sections. * `droid mcp remove` / `openclaw mcp unset` for the CLI-registered agents — they have no file-edit path so we have to unregister via CLI. Bug fixes spotted during review: * install.sh:487 referenced an unbound `$TARGETS` variable (renamed to `$ALL_TARGETS` during the refactor); `set -u` would have made this fatal. Fixed. * install.ps1 used `$SkillTargets.Count -gt 0` for the `-y` decision while install.sh used `$ALL_TARGETS`; the divergence meant the same `--only` invocation behaved differently on mac vs Windows. Aligned to AllTargets. * `_filter_csv` / `detect_agents` had leaked-scope loop vars; declared them `local` to prevent caller pollution. New: scripts/dev-smoke.sh — sandboxed regression suite, 42 assertions across 4 phases (unit tests / installer / writer schemas / uninstaller + false-positive guard). Runs in under 10s, never touches real $HOME. Use before opening PRs touching install* or uninstall* scripts.
f0rmatting
force-pushed
the
feat/unified-mcp-registration
branch
from
dafd3d9 to
4e0d8b9
Compare
|
@claude review |
|
🤖 Claude security review requested by @f0rmatting. Running against HEAD |
|
🤖 Claude security review — HEAD: 4e0d8b9 Scope: 6-file unification of skill + MCP installer/uninstaller across 16 agents ( ✅ No security or convention issues found. Review triggered by @f0rmatting |
3 participants
Summary
npx skills add -aand@agentkey/mcp --auth-login --onlyfrom a single detected-agent list. MCP registration now follows the same per-host auto-detection that skill install already does, expanding MCP auto-registration from 3 clients to 16.claude-codemarker bug: it included Claude Desktop's config dir, causing skills CLI to target a nonexistent Claude Code on Desktop-only machines.claude-desktopis now its own id (MCP-only) — passed to--auth-login --onlybut never toskills add./Applications/Claude.app/%LOCALAPPDATA%\AnthropicClaudeso "installed but never launched" still registers (Linux still requires the config dir).scripts/dev-smoke.sh— sandboxed 4-phase regression suite, 42 assertions, ~10s, never touches real$HOME. Run before any PR touching install/uninstall scripts.Depends on
chainbase-labs/AgentKey-Server#9 — adds
--only <ids>to@agentkey/mcp --auth-login. Older CLI versions silently ignore the flag, so this PR is forward-compatible either way.Uninstaller (the bigger gap before this)
The previous uninstaller only cleaned 3 config paths and only knew the
mcpServers.<name>JSON shape. With 13 new agents using 4 different schema dialects, that left AgentKey configured everywhere after uninstall.mcpServers.<name>/mcp.<name>/amp.mcpServers.<name>/projects.X.mcpServers.<name>in one pass[mcp_servers.agentkey]+ legacy quoted block, preserves sibling sectionsdroid mcp remove+openclaw mcp unsetfor CLI-registered agentsagentkey-helperare preserved (regression test included in dev-smoke)Bugs fixed during review
$TARGETSvariable (renamed during refactor) —set -uwould have made this fatal$SkillTargets.Countused where$AllTargets.Countwas meant — diverged from install.sh behavior--only claude-desktopranskills add -awith no filter, defeating the user's--onlyintent. Now correctly skips the skill step_ids,_id) — declaredlocal -aTest plan
scripts/dev-smoke.sh— 42 passing / 0 failing (Phase 1 unit tests + Phase 2 installer + Phase 3 writer schemas + Phase 4 uninstaller w/ false-positive guard)bash -nclean for install.sh + uninstall.sh--list-agentscorrectly listsclaude-desktopas a separate id--only claude-desktop --skip-mcp --yeswalks the new "MCP-only, skip skill" branchbash -xtrace underset -u(no unbound-variable explosion)agentkey-helper,other-svr,[mcp_servers.other],[unrelated_section]all preserved after scrubinstall.ps1/uninstall.ps1syntax-checked but not runtime-tested (no Windows box handy — happy to test if reviewer has one)