refactor: centralize optional tools lifecycle with registry-driven install and cleanup#76
Merged
Conversation
Multiple component types left orphaned artifacts when removed from config — MCP entries without markers escaped cleanup, uv tools were never uninstalled, and plugins/settings/cache had no uninstall path. This was a recurring bug class (4th instance in git history). Adds _previously_managed_names to MCPManager for marker-independent cleanup, ensure_recall_uninstalled() for uv tool lifecycle, and uninstall() methods to OptionalToolsComponent, ClaudePluginComponent, and SettingsComponent. Also fixes source-deleted hook cleanup in extensions uninstall, broken symlink handling in skills uninstall, and deprecated symlink cleanup in config uninstall.
… tool registry The initial cleanup fix hardcoded all logic to the recall use case — ensure_recall_uninstalled(), if-checks for "recall", and a manual frozenset tombstone. Adding another retired tool would require duplicating all of it. Introduces DeprecatedToolSpec registry in bootstrap/registry.py as the single source of truth. OptionalToolsComponent now iterates the registry generically, GooseMCPManager._previously_managed_names derives from it, and ensure_tool_uninstalled() replaces the recall-specific variant. Also fixes dry-run guards in plugin/settings uninstall, partial-failure manifest clearing, --only scoping for SettingsComponent, and stale-vs-missing status semantics.
wpfleger96
changed the title
…istry ensure_recall_installed() and ensure_statusline_installed() were structurally isomorphic 100-line functions with identical branch structure, return vocabulary, and underlying helpers. Adding a new tool required writing another per-tool function and duplicating the result-handling block in OptionalToolsComponent. Replaces both with a single ensure_tool_installed(spec, source, ...) that operates on any ToolSpec. Adds ACTIVE_TOOLS registry alongside the existing DEPRECATED_TOOLS registry — OptionalToolsComponent now iterates both registries generically. Also removes all recall install infrastructure (RECALL_GITHUB_REPO, _RECALL_SPEC, recall updater helpers) since recall is fully deprecated.
wpfleger96
changed the title
Crossfire review (4 Claude specialists + Codex + Gemini) surfaced 12 findings. Key fixes: - Validate `--only` against all INSTALL_COMPONENTS, not just semantic components — `--only settings` was advertised by shell completion but rejected at runtime - Extract `_compute_stale_tool_ids()` to eliminate plan/install duplication - Rename `DeprecatedToolSpec.is_configured` to `is_still_in_use` for self-documenting semantics at call sites - Move `_is_recall_configured` from installer.py into registry.py (its only consumer) to stop leaking a private function across module boundaries - Add `command_name` to `ActiveToolSpec` and uninstall active tools during `uninstall` (was only cleaning up deprecated tools) - Fix `SettingsComponent.uninstall` early return that skipped tracker_path cleanup when cache_dir was outside $HOME - Use dual `get_tool_source`/`is_command_available` guard in `ensure_tool_uninstalled` to avoid touching non-uv-managed tools - Add `skip_update_check` param to `ensure_tool_installed` so `install` is a fast presence-check without blocking network calls - Fix status messaging for configured deprecated tools (no longer reported as missing since install won't satisfy them) - Add 13 new tests: 11 for ensure_tool_installed/uninstalled, 2 for GooseMCPManager unmarked orphan cleanup migration path
The crossfire review commit introduced 12 mypy errors: untyped `_make_spec` helper, `str | None` operand in `in` expression, `list.append` in expression context, and a `spec` variable shadowing `ToolSpec` with `DeprecatedToolSpec` in the uninstall loop. Adds 5 E2E tests exercising the registry-driven lifecycle via the real CLI binary: install dry-run, status with optional tools, uninstall with active+deprecated tools, `--only settings` acceptance (crossfire finding #1), and invalid `--only` rejection.
Add optional tool registry gotcha with ✅/❌ examples, E2E test commands and fixture pointers, bootstrap/registry.py to project structure, and add/retire tool entry to key files table.
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.
When a managed component is removed from config, several artifact types were left behind — most visibly
recallMCP entries persisting in Goose's config and therecall-mcp-serveruv tool remaining installed after the feature was disabled in #73. More broadly, the optional tools pipeline had per-tool hardcoded functions (ensure_recall_installed,ensure_statusline_installed) that were structurally isomorphic but duplicated across 100+ lines each, with matching result-handling blocks duplicated inOptionalToolsComponent.This introduces two complementary registries in
bootstrap/registry.pythat serve as the single source of truth for tool lifecycle:DEPRECATED_TOOLS(retired tools to clean up) andACTIVE_TOOLS(tools to install/upgrade). Adding a new tool in either category requires one registry entry — no new functions, no hardcoded if-chains.DeprecatedToolSpecregistry withis_still_in_usecallback andget_deprecated_mcp_names()for MCP integration, andActiveToolSpecregistry withcommand_namefield and lazyget_install_specto avoid circular importsensure_recall_installed()andensure_statusline_installed()with a single genericensure_tool_installed(spec, source, ...)that handles fresh install, upgrade, source switching, local reinstall, andskip_update_checkfor fast presence-only checksensure_recall_uninstalled()with genericensure_tool_uninstalled(command_name, package_name, ...)using a dualget_tool_source/is_command_availableguard to avoid touching non-uv-managed toolsOptionalToolsComponentto iterate both registries in all lifecycle methods —uninstall()now removes both active and deprecated tools,status()correctly labels configured deprecated tools as user-managed_compute_stale_tool_ids(),_remove_stale_tools(),_install_active_tools(), and_emit_install_result()to eliminate all per-tool duplicationGooseMCPManager._previously_managed_namestoget_deprecated_mcp_names()instead of a hardcoded frozenset_is_recall_configuredfrominstaller.pyintoregistry.pywhere its only consumer lives; removes all other recall install infrastructure--only settingsvalidation:select_componentsnow validates againstINSTALL_COMPONENTSinstead of just semantic componentsSettingsComponent.uninstallearly return that skipped tracker_path cleanup when cache_dir was outside$HOMEuninstall()toClaudePluginComponent(partial-failure-safe manifest clearing) andSettingsComponent(path containment check)ensure_tool_installed/ensure_tool_uninstalledand GooseMCPManager unmarked orphan cleanup migration path