diff --git a/design/integration.md b/design/integration.md index 04509705d3..5085d049f4 100644 --- a/design/integration.md +++ b/design/integration.md @@ -383,6 +383,27 @@ partial and shared files are preserved by manifest-only uninstall. `teardown()` preserves modified tracked files by default; `--force` can remove them. Keep agent-specific settings and events consistent with that lifecycle. +Event cleanup releases only the departing integration's dispatcher claim. +It deletes `.specify/events.py` only when that integration's current or prior +manifest records a produced copy and no other installed integration references +it. Unowned, partial, or merely recovered files are preserved, even with `--force`; +modified managed dispatchers are preserved with a warning unless forced. +An exact match with the current generated dispatcher (LF or CRLF) also permits +last-consumer cleanup when another integration regenerated the file and left +this integration's recorded hash stale. Matching generated content alone never +establishes ownership. +If the prior ownership manifest or non-forced dispatcher content cannot be +read, cleanup warns and preserves the dispatcher without blocking cleanup of +other integration-owned files. +Empty-event upgrades consult the prior manifest so the last managed dispatcher +is still cleaned up. Event helpers accept keyword-only `force=False`. +Teardown forwards `force` to `remove_events()` overrides that accept it +explicitly or via `**kwargs`; legacy `(project_root, manifest)` overrides +remain supported without that keyword. Signature compatibility is checked +before invocation, so errors raised inside an override are not retried. +Custom `setup()` implementations forwarding event emission should pass through +`opts["force"]` when present. + The integration does **not** own agent context files such as `AGENTS.md` or `CLAUDE.md`. The opt-in `extensions/agent-context/` owns their defaults, configuration, and managed sections; do not add `context_file` fields or diff --git a/src/specify_cli/events/__init__.py b/src/specify_cli/events/__init__.py index efdec79db5..23572ac21c 100644 --- a/src/specify_cli/events/__init__.py +++ b/src/specify_cli/events/__init__.py @@ -14,6 +14,7 @@ from __future__ import annotations +import hashlib import json import logging import os @@ -1356,6 +1357,8 @@ def install_integration_events( project_root: Path, manifest: IntegrationManifest, events: ResolvedEvents, + *, + force: bool = False, ) -> list[Path]: """Generate dispatcher, merge native config, return created files. @@ -1388,7 +1391,7 @@ def install_integration_events( # new manifest no longer claims it and stale cleanup excludes it). if not filtered: _remove_native_event_hooks(integration, project_root, manifest) - _cleanup_shared_dispatcher(integration, project_root, manifest) + _cleanup_shared_dispatcher(integration, project_root, manifest, force=force) return [] created: list[Path] = [] @@ -1717,51 +1720,74 @@ def _other_event_integrations_reference_dispatcher( def _cleanup_shared_dispatcher( - integration: IntegrationBase, project_root: Path, manifest: IntegrationManifest + integration: IntegrationBase, project_root: Path, manifest: IntegrationManifest, + *, + force: bool = False, ) -> None: - """Drop this integration's manifest claim on the shared dispatcher and - delete the file only when no other installed event-capable integration - still references it (#10, S3). - - The manifest.remove() runs in both branches (S1): if we retain the file - but leave it tracked, the subsequent manifest.uninstall() in teardown() - sees the matching hash and deletes the file another integration still - depends on. Used by full teardown and by the empty-resolved-map install - path so an ``--events false`` upgrade of the last event integration - doesn't orphan ``.specify/events.py`` permanently (S3). - """ + """Release this integration's claim; delete only its last managed copy.""" + from ..integrations.manifest import IntegrationManifest + dispatcher_rel = EVENTS_DISPATCHER_REL - # Drop this integration's manifest claim if present. The remove() is - # conditional (S1): an upgrade passes a *fresh* manifest that may never - # have claimed the dispatcher, so the key may be absent — that's a no-op. + owner = manifest + # Upgrades emit into a fresh manifest before replacing the installed one. + # Only this integration's prior manifest can supply its missing claim. + if dispatcher_rel not in owner.files: + try: + owner = IntegrationManifest.load(integration.key, project_root) + except FileNotFoundError: + owner = manifest + except (OSError, ValueError) as exc: + logger.warning( + "Could not read prior dispatcher manifest for '%s'; preserving %s: %s", + integration.key, project_root / dispatcher_rel, exc, + ) + return + expected_hash = owner.files.get(dispatcher_rel) + recovered = owner.is_recovered(dispatcher_rel) + partial = owner.ownership_modes.get(dispatcher_rel) == "partial" + + # Release even retained claims so manifest-only teardown cannot delete a + # user-owned copy or a dispatcher still used by another integration. if dispatcher_rel in manifest.files: manifest.remove(dispatcher_rel) - # S2: run the no-other-references deletion independently of whether the - # new manifest currently contains the key. An ``integration upgrade - # --events false`` passes a fresh manifest that never recorded the - # dispatcher, so gating the deletion on its presence orphans the file the - # old on-disk manifest owned — and stale cleanup excludes it (C3). If no - # other installed event-capable integration references the dispatcher, - # delete it; otherwise leave it for them. - if not _other_event_integrations_reference_dispatcher(project_root, integration.key): - dispatcher_path = project_root / dispatcher_rel - if dispatcher_path.exists(): - _ensure_safe_destination(dispatcher_path) - _unlink_file(dispatcher_path, missing_ok=True) + if not expected_hash or recovered or partial: + return + if _other_event_integrations_reference_dispatcher(project_root, integration.key): + return + dispatcher_path = project_root / dispatcher_rel + if dispatcher_path.exists(): + _ensure_safe_destination(dispatcher_path) + if not force: + try: + content = dispatcher_path.read_bytes() + except OSError as exc: + logger.warning("Preserving unreadable dispatcher %s: %s", dispatcher_path, exc) + return + # Another integration may have regenerated the shared file since + # this manifest recorded it. Accept only exact generated LF/CRLF. + matches = hashlib.sha256(content).hexdigest() == expected_hash or content in ( + _EVENTS_DISPATCHER_TEMPLATE.encode("utf-8"), + _EVENTS_DISPATCHER_TEMPLATE.replace("\n", "\r\n").encode("utf-8"), + ) + if not matches: + logger.warning("Preserving modified dispatcher: %s", dispatcher_path) + return + _unlink_file(dispatcher_path, missing_ok=True) def remove_integration_events( - integration: IntegrationBase, project_root: Path, manifest: IntegrationManifest + integration: IntegrationBase, project_root: Path, manifest: IntegrationManifest, + *, + force: bool = False, ) -> None: """Remove Specify-authored event entries from native config. - The shared ``.specify/events.py`` dispatcher is deleted only when no other - installed event-capable integration still references it (#10); otherwise - it is left in place so multi-install setups don't lose the dispatcher - mid-stream. + The shared dispatcher is deleted only if this integration owns it and no + other installed integration references it. Modified copies require force; + unowned or recovered copies are always retained. """ _remove_native_event_hooks(integration, project_root, manifest) - _cleanup_shared_dispatcher(integration, project_root, manifest) + _cleanup_shared_dispatcher(integration, project_root, manifest, force=force) # Clean up opencode TS plugin (owned solely by the opencode integration). if integration.key == "opencode": diff --git a/src/specify_cli/integrations/base.py b/src/specify_cli/integrations/base.py index b3dd0abc34..1587da95d3 100644 --- a/src/specify_cli/integrations/base.py +++ b/src/specify_cli/integrations/base.py @@ -1059,7 +1059,15 @@ def teardown( Returns ``(removed, skipped)`` file lists. """ - self.remove_events(project_root, manifest) + import inspect + + remove_events = self.remove_events + try: + inspect.signature(remove_events).bind(project_root, manifest, force=force) + except TypeError: + remove_events(project_root, manifest) + else: + remove_events(project_root, manifest, force=force) return manifest.uninstall(project_root, force=force) def emit_events( @@ -1071,15 +1079,19 @@ def emit_events( **opts: Any, ) -> list[Path]: """Emit native event configuration for this integration.""" - return install_integration_events(self, project_root, manifest, events or {}) + return install_integration_events( + self, project_root, manifest, events or {}, force=opts.get("force", False) + ) def remove_events( self, project_root: Path, manifest: IntegrationManifest, + *, + force: bool = False, ) -> None: """Remove Specify-authored event entries from native config.""" - remove_integration_events(self, project_root, manifest) + remove_integration_events(self, project_root, manifest, force=force) def supports_events(self) -> bool: """Return True if this integration supports agent-native events.""" @@ -1209,7 +1221,8 @@ def setup( # Install agent runtime events event_files = self.emit_events( - project_root, manifest, events=opts.get("events"), parsed_options=parsed_options + project_root, manifest, events=opts.get("events"), parsed_options=parsed_options, + force=opts.get("force", False), ) created.extend(event_files) @@ -1426,7 +1439,8 @@ def setup( # Install agent runtime events event_files = self.emit_events( - project_root, manifest, events=opts.get("events"), parsed_options=parsed_options + project_root, manifest, events=opts.get("events"), parsed_options=parsed_options, + force=opts.get("force", False), ) created.extend(event_files) @@ -1668,7 +1682,8 @@ def setup( # Install agent runtime events event_files = self.emit_events( - project_root, manifest, events=opts.get("events"), parsed_options=parsed_options + project_root, manifest, events=opts.get("events"), parsed_options=parsed_options, + force=opts.get("force", False), ) created.extend(event_files) @@ -1954,7 +1969,8 @@ def setup( # Install agent runtime events event_files = self.emit_events( - project_root, manifest, events=opts.get("events"), parsed_options=parsed_options + project_root, manifest, events=opts.get("events"), parsed_options=parsed_options, + force=opts.get("force", False), ) created.extend(event_files) diff --git a/src/specify_cli/integrations/command_upgrade.py b/src/specify_cli/integrations/command_upgrade.py index 004efa69af..e88f2bc1aa 100644 --- a/src/specify_cli/integrations/command_upgrade.py +++ b/src/specify_cli/integrations/command_upgrade.py @@ -239,6 +239,7 @@ def integration_upgrade( script_type=selected_script, raw_options=raw_options, events=events_map, + force=force, ) settings = _with_integration_setting( current, diff --git a/src/specify_cli/integrations/copilot/__init__.py b/src/specify_cli/integrations/copilot/__init__.py index c41f55e87a..a283dc3df2 100644 --- a/src/specify_cli/integrations/copilot/__init__.py +++ b/src/specify_cli/integrations/copilot/__init__.py @@ -513,7 +513,8 @@ def setup( # Install agent runtime events event_files = self.emit_events( - project_root, manifest, events=opts.get("events"), parsed_options=parsed_options + project_root, manifest, events=opts.get("events"), parsed_options=parsed_options, + force=opts.get("force", False), ) created.extend(event_files) return created diff --git a/tests/integrations/test_base.py b/tests/integrations/test_base.py index 66faeb2b1b..9344cc04a4 100644 --- a/tests/integrations/test_base.py +++ b/tests/integrations/test_base.py @@ -129,6 +129,66 @@ def test_uninstall_delegates_to_teardown(self, tmp_path): assert removed == [] assert skipped == [] + @pytest.mark.parametrize("force", [False, True]) + def test_teardown_supports_legacy_remove_events(self, tmp_path, force): + calls = [] + + class LegacyIntegration(StubIntegration): + def remove_events(self, project_root, manifest): + calls.append((project_root, manifest)) + + integration = LegacyIntegration() + manifest = IntegrationManifest(integration.key, tmp_path) + generated = manifest.record_file(".stub/commands/speckit.plan.md", "# Plan\n") + manifest.save() + + removed, skipped = integration.teardown(tmp_path, manifest, force=force) + + assert calls == [(tmp_path, manifest)] + assert removed == [generated] + assert skipped == [] + assert not generated.exists() + assert not manifest.manifest_path.exists() + + @pytest.mark.parametrize("force", [False, True]) + @pytest.mark.parametrize("style", ["keyword", "kwargs"]) + def test_teardown_forwards_supported_force(self, tmp_path, force, style): + calls = [] + + class KeywordIntegration(StubIntegration): + def remove_events(self, project_root, manifest, *, force=False): + calls.append(force) + + class KwargsIntegration(StubIntegration): + def remove_events(self, project_root, manifest, **kwargs): + calls.append(kwargs["force"]) + + integration = KeywordIntegration() if style == "keyword" else KwargsIntegration() + manifest = IntegrationManifest(integration.key, tmp_path) + + integration.teardown(tmp_path, manifest, force=force) + + assert calls == [force] + + @pytest.mark.parametrize("force", [False, True]) + def test_teardown_does_not_retry_remove_events_type_error(self, tmp_path, force): + calls = [] + + class BrokenIntegration(StubIntegration): + def remove_events(self, project_root, manifest, *, force=False): + calls.append(force) + raise TypeError("adapter cleanup failed") + + integration = BrokenIntegration() + manifest = IntegrationManifest(integration.key, tmp_path) + generated = manifest.record_file(".stub/commands/speckit.plan.md", "# Plan\n") + + with pytest.raises(TypeError, match="adapter cleanup failed"): + integration.teardown(tmp_path, manifest, force=force) + + assert calls == [force] + assert generated.exists() + class TestMarkdownIntegration: def test_is_subclass_of_base(self): diff --git a/tests/specify_cli/events/test_events.py b/tests/specify_cli/events/test_events.py index d3f714d15f..f3e67982d6 100644 --- a/tests/specify_cli/events/test_events.py +++ b/tests/specify_cli/events/test_events.py @@ -3160,7 +3160,8 @@ class TestDispatcherManifestClaimDroppedOnRetain: it), this integration's manifest still drops its claim so the subsequent manifest.uninstall() in teardown() doesn't delete the shared file.""" - def test_full_teardown_keeps_dispatcher_when_other_references_it(self, tmp_path): + @pytest.mark.parametrize("force", [False, True]) + def test_full_teardown_keeps_dispatcher_when_other_references_it(self, tmp_path, force): from specify_cli.integrations.codex import CodexIntegration from specify_cli.integrations.manifest import IntegrationManifest @@ -3196,8 +3197,7 @@ def test_full_teardown_keeps_dispatcher_when_other_references_it(self, tmp_path) # Full teardown of claude (remove + manifest.uninstall): the dispatcher # must survive because codex's manifest still references it. - remove_integration_events(claude, tmp_path, claude_manifest) - claude_manifest.uninstall(tmp_path, force=True) + claude.teardown(tmp_path, claude_manifest, force=force) assert dispatcher_path.exists(), ( "Shared dispatcher was deleted by teardown() despite another " @@ -3278,6 +3278,240 @@ def test_fresh_manifest_upgrade_deletes_dispatcher_when_last(self, tmp_path): "Dispatcher orphaned after upgrade with a fresh manifest (S2)." ) + @pytest.mark.parametrize("operation", ["teardown", "disable"]) + @pytest.mark.parametrize("newline", ["\n", "\r\n"]) + @pytest.mark.parametrize("modified", [False, True]) + def test_cleanup_after_shared_dispatcher_revision_changes( + self, tmp_path, monkeypatch, caplog, operation, newline, modified, + ): + import specify_cli.events as event_module + from specify_cli.integrations.codex import CodexIntegration + + claude = ClaudeIntegration() + codex = CodexIntegration() + current_template = event_module._EVENTS_DISPATCHER_TEMPLATE + claude_manifest = IntegrationManifest(claude.key, tmp_path) + with monkeypatch.context() as old_revision: + old_revision.setattr( + event_module, "_EVENTS_DISPATCHER_TEMPLATE", + current_template + "\n# Previous generated revision\n", + ) + install_integration_events( + claude, tmp_path, claude_manifest, + {"pre_tool_use": [{"command": "speckit.x"}]}, + ) + claude_manifest.save() + codex_manifest = IntegrationManifest(codex.key, tmp_path) + install_integration_events( + codex, tmp_path, codex_manifest, + {"pre_tool_use": [{"command": "speckit.y"}]}, + ) + dispatcher = tmp_path / EVENTS_DISPATCHER_REL + dispatcher.write_bytes(current_template.replace("\n", newline).encode("utf-8")) + codex_manifest.record_existing(EVENTS_DISPATCHER_REL) + codex_manifest.save() + assert claude_manifest.files[EVENTS_DISPATCHER_REL] != codex_manifest.files[EVENTS_DISPATCHER_REL] + state_path = tmp_path / ".specify/integration.json" + state_path.write_text(json.dumps({ + "default_integration": "codex", + "installed_integrations": ["claude", "codex"], + }), encoding="utf-8") + + codex.teardown(tmp_path, codex_manifest) + assert dispatcher.exists() + state_path.write_text(json.dumps({ + "default_integration": "claude", + "installed_integrations": ["claude"], + }), encoding="utf-8") + expected = dispatcher.read_bytes() + if modified: + expected += b"\n# User customization\n" + dispatcher.write_bytes(expected) + + if operation == "teardown": + claude.teardown(tmp_path, IntegrationManifest.load(claude.key, tmp_path)) + else: + fresh = IntegrationManifest(claude.key, tmp_path) + install_integration_events(claude, tmp_path, fresh, {}) + + assert dispatcher.exists() is modified + if modified: + assert dispatcher.read_bytes() == expected + assert "modified dispatcher" in caplog.text + else: + assert "modified dispatcher" not in caplog.text + + +class TestDispatcherCleanupOwnership: + @pytest.mark.parametrize("ownership", ["unowned", "recovered", "partial"]) + @pytest.mark.parametrize("newline", ["\n", "\r\n"]) + def test_generated_template_does_not_establish_ownership(self, tmp_path, ownership, newline): + from specify_cli.events import _EVENTS_DISPATCHER_TEMPLATE + + integration = CopilotIntegration() + manifest = IntegrationManifest(integration.key, tmp_path) + dispatcher = tmp_path / EVENTS_DISPATCHER_REL + dispatcher.parent.mkdir(parents=True) + content = _EVENTS_DISPATCHER_TEMPLATE.replace("\n", newline).encode("utf-8") + dispatcher.write_bytes(content) + if ownership != "unowned": + manifest.record_existing( + EVENTS_DISPATCHER_REL, + recovered=ownership == "recovered", + ownership="partial" if ownership == "partial" else "shared", + ) + manifest.save() + + integration.teardown(tmp_path, manifest) + + assert dispatcher.read_bytes() == content + + @pytest.mark.parametrize("operation", ["install", "remove", "teardown"]) + @pytest.mark.parametrize("saved_manifest", [False, True]) + def test_unowned_dispatcher_is_preserved(self, tmp_path, operation, saved_manifest): + integration = CopilotIntegration() + manifest = IntegrationManifest(integration.key, tmp_path) + if saved_manifest: + manifest.save() + dispatcher = tmp_path / EVENTS_DISPATCHER_REL + dispatcher.parent.mkdir(parents=True, exist_ok=True) + content = b"# User-owned dispatcher\n" + dispatcher.write_bytes(content) + + if operation == "install": + install_integration_events(integration, tmp_path, manifest, {}) + elif operation == "remove": + remove_integration_events(integration, tmp_path, manifest) + else: + integration.teardown(tmp_path, manifest, force=True) + + assert dispatcher.read_bytes() == content + assert EVENTS_DISPATCHER_REL not in manifest.files + + @pytest.mark.parametrize("fresh", [False, True]) + def test_empty_map_preserves_modified_dispatcher(self, tmp_path, fresh, caplog): + integration = ClaudeIntegration() + manifest = IntegrationManifest(integration.key, tmp_path) + install_integration_events( + integration, tmp_path, manifest, + {"pre_tool_use": [{"command": "speckit.x"}]}, + ) + manifest.save() + dispatcher = tmp_path / EVENTS_DISPATCHER_REL + dispatcher.write_text("# User-modified dispatcher\n", encoding="utf-8") + if fresh: + manifest = IntegrationManifest(integration.key, tmp_path) + + install_integration_events(integration, tmp_path, manifest, {}) + + assert dispatcher.read_text(encoding="utf-8") == "# User-modified dispatcher\n" + assert EVENTS_DISPATCHER_REL not in manifest.files + assert "modified dispatcher" in caplog.text + + @pytest.mark.parametrize("force", [False, True]) + def test_teardown_modified_dispatcher_honors_force(self, tmp_path, force): + integration = ClaudeIntegration() + manifest = IntegrationManifest(integration.key, tmp_path) + install_integration_events( + integration, tmp_path, manifest, + {"pre_tool_use": [{"command": "speckit.x"}]}, + ) + dispatcher = tmp_path / EVENTS_DISPATCHER_REL + dispatcher.write_text("# User-modified dispatcher\n", encoding="utf-8") + + integration.teardown(tmp_path, manifest, force=force) + + assert dispatcher.exists() is not force + if not force: + assert dispatcher.read_text(encoding="utf-8") == "# User-modified dispatcher\n" + + @pytest.mark.parametrize( + ("recovered", "ownership"), [(True, "shared"), (True, "whole"), (False, "partial")], + ) + def test_observed_dispatcher_is_not_cleanup_authority(self, tmp_path, recovered, ownership): + integration = CopilotIntegration() + manifest = IntegrationManifest(integration.key, tmp_path) + dispatcher = tmp_path / EVENTS_DISPATCHER_REL + dispatcher.parent.mkdir(parents=True) + dispatcher.write_text("# User-owned dispatcher\n", encoding="utf-8") + manifest.record_existing(EVENTS_DISPATCHER_REL, recovered=recovered, ownership=ownership) + manifest.save() + + integration.teardown(tmp_path, manifest, force=True) + + assert dispatcher.read_text(encoding="utf-8") == "# User-owned dispatcher\n" + + @pytest.mark.parametrize("content", [b"{invalid", b"\xff", b"[]"]) + def test_unreadable_prior_manifest_does_not_authorize_cleanup(self, tmp_path, caplog, content): + integration = CopilotIntegration() + manifest = IntegrationManifest(integration.key, tmp_path) + manifest.save() + manifest.manifest_path.write_bytes(content) + dispatcher = tmp_path / EVENTS_DISPATCHER_REL + dispatcher.write_text("# User-owned dispatcher\n", encoding="utf-8") + + install_integration_events(integration, tmp_path, manifest, {}) + + assert dispatcher.read_text(encoding="utf-8") == "# User-owned dispatcher\n" + assert "prior dispatcher manifest" in caplog.text + assert integration.key in caplog.text + + @pytest.mark.parametrize("error", [ValueError("invalid manifest"), PermissionError("manifest denied")]) + def test_prior_manifest_failure_does_not_block_teardown(self, tmp_path, caplog, error): + integration = CopilotIntegration() + manifest = IntegrationManifest(integration.key, tmp_path) + generated = manifest.record_file(".github/skills/speckit-plan/SKILL.md", "# Plan\n") + manifest.save() + dispatcher = tmp_path / EVENTS_DISPATCHER_REL + dispatcher.write_text("# User-owned dispatcher\n", encoding="utf-8") + + with patch.object(IntegrationManifest, "load", side_effect=error): + removed, skipped = integration.teardown(tmp_path, manifest, force=True) + + assert generated in removed + assert not generated.exists() + assert skipped == [] + assert not manifest.manifest_path.exists() + assert dispatcher.read_text(encoding="utf-8") == "# User-owned dispatcher\n" + assert "prior dispatcher manifest" in caplog.text + assert str(error) in caplog.text + + @pytest.mark.parametrize("force", [False, True]) + def test_unreadable_dispatcher_does_not_block_teardown(self, tmp_path, monkeypatch, caplog, force): + integration = ClaudeIntegration() + manifest = IntegrationManifest(integration.key, tmp_path) + generated = manifest.record_file(".claude/skills/speckit-plan/SKILL.md", "# Plan\n") + install_integration_events( + integration, tmp_path, manifest, + {"pre_tool_use": [{"command": "speckit.x"}]}, + ) + manifest.save() + dispatcher = tmp_path / EVENTS_DISPATCHER_REL + read_bytes = Path.read_bytes + read_attempts = [] + + def deny_dispatcher_read(path): + if path == dispatcher: + read_attempts.append(path) + raise PermissionError("dispatcher denied") + return read_bytes(path) + + monkeypatch.setattr(Path, "read_bytes", deny_dispatcher_read) + + removed, skipped = integration.teardown(tmp_path, manifest, force=force) + + assert generated in removed + assert not generated.exists() + assert skipped == [] + assert not manifest.manifest_path.exists() + assert dispatcher.exists() is not force + if force: + assert read_attempts == [] + else: + assert read_attempts == [dispatcher] + assert "unreadable dispatcher" in caplog.text + assert "dispatcher denied" in caplog.text + # -- Dispatcher stale-cleanup exclusion (C3) --------------------------------- diff --git a/tests/specify_cli/integrations/test_command_switch.py b/tests/specify_cli/integrations/test_command_switch.py index 95d9cd6978..5f293a7b7a 100644 --- a/tests/specify_cli/integrations/test_command_switch.py +++ b/tests/specify_cli/integrations/test_command_switch.py @@ -26,6 +26,71 @@ ) class TestIntegrationSwitch: + @pytest.mark.parametrize("command", ["install", "switch"]) + def test_invalid_incoming_manifest_does_not_block_rollback(self, tmp_path, monkeypatch, caplog, command): + from specify_cli.integrations.copilot import CopilotIntegration + + project = _init_project(tmp_path, "claude", "--events false") + stale_manifest = project / ".specify/integrations/copilot.manifest.json" + stale_manifest.write_text("{invalid", encoding="utf-8") + dispatcher = project / ".specify/events.py" + dispatcher.write_text("# User-owned dispatcher\n", encoding="utf-8") + setup = CopilotIntegration.setup + generated = [] + + def setup_then_fail(self, project_root, manifest, *args, **kwargs): + generated.extend(setup(self, project_root, manifest, *args, **kwargs)) + raise OSError("injected setup failure") + + monkeypatch.setattr(CopilotIntegration, "setup", setup_then_fail) + args = [ + "integration", command, "copilot", "--script", "sh", + "--integration-options", "--events false", + ] + if command == "install": + args.append("--force") + + result = _run_in_project(project, args) + + assert result.exit_code == 1, result.output + assert not (project / ".github/skills/speckit-plan/SKILL.md").exists(), result.output + assert generated + assert all(not path.exists() for path in generated) + assert not stale_manifest.exists() + assert dispatcher.read_text(encoding="utf-8") == "# User-owned dispatcher\n" + output = _normalize_cli_output(result.output) + assert "injected setup failure" in output + assert "Failed to rollback" not in output + assert "prior dispatcher manifest" in caplog.text + + @pytest.mark.parametrize("command", ["install", "switch"]) + @pytest.mark.parametrize("force", [False, True]) + def test_incoming_copilot_preserves_unowned_dispatcher(self, tmp_path, command, force): + project = _init_project( + tmp_path, "claude", "--events false", + ) + if command == "install": + uninstall = _run_in_project(project, ["integration", "uninstall", "claude"]) + assert uninstall.exit_code == 0, uninstall.output + dispatcher = project / ".specify" / "events.py" + content = b"# User-owned dispatcher\n" + dispatcher.write_bytes(content) + + args = [ + "integration", command, "copilot", "--script", "sh", + "--integration-options", "--events false", + ] + if force: + args.append("--force") + result = _run_in_project(project, args) + + assert result.exit_code == 0, result.output + assert dispatcher.read_bytes() == content + manifest = json.loads( + (project / ".specify/integrations/copilot.manifest.json").read_text(encoding="utf-8") + ) + assert ".specify/events.py" not in manifest["files"] + def test_switch_requires_speckit_project(self, tmp_path): old_cwd = os.getcwd() try: diff --git a/tests/specify_cli/integrations/test_command_uninstall.py b/tests/specify_cli/integrations/test_command_uninstall.py index e16099e8c8..3c9fcb2b03 100644 --- a/tests/specify_cli/integrations/test_command_uninstall.py +++ b/tests/specify_cli/integrations/test_command_uninstall.py @@ -47,6 +47,46 @@ def _registered_agents(project, kind, package_id): return set(registry[kind][package_id]["registered_commands"]) class TestIntegrationUninstall: + @pytest.mark.parametrize("command", ["uninstall", "switch"]) + def test_unreadable_dispatcher_preserves_other_cleanup(self, tmp_path, monkeypatch, caplog, command): + from specify_cli.events import install_integration_events + from specify_cli.integrations.claude import ClaudeIntegration + from specify_cli.integrations.manifest import IntegrationManifest + + project = _init_project(tmp_path, "claude", "--events false") + manifest = IntegrationManifest.load("claude", project) + install_integration_events( + ClaudeIntegration(), project, manifest, + {"pre_tool_use": [{"command": "speckit.x"}]}, + ) + manifest.save() + dispatcher = project / ".specify/events.py" + original = dispatcher.read_text(encoding="utf-8") + read_bytes = Path.read_bytes + + def deny_dispatcher_read(path): + if path == dispatcher: + raise PermissionError("dispatcher denied") + return read_bytes(path) + + monkeypatch.setattr(Path, "read_bytes", deny_dispatcher_read) + args = ["integration", command] + if command == "switch": + args += ["copilot", "--integration-options", "--events false"] + + result = _run_in_project(project, args) + + assert result.exit_code == 0, result.output + assert not (project / ".claude/skills/speckit-plan/SKILL.md").exists() + assert not manifest.manifest_path.exists() + assert dispatcher.read_text(encoding="utf-8") == original + assert "unreadable dispatcher" in caplog.text + state_path = project / ".specify/integration.json" + if command == "switch": + assert json.loads(state_path.read_text(encoding="utf-8"))["default_integration"] == "copilot" + else: + assert not state_path.exists() + def test_uninstall_requires_speckit_project(self, tmp_path): old_cwd = os.getcwd() try: diff --git a/tests/specify_cli/integrations/test_command_upgrade.py b/tests/specify_cli/integrations/test_command_upgrade.py index 42d5ea0a6c..979bb5a294 100644 --- a/tests/specify_cli/integrations/test_command_upgrade.py +++ b/tests/specify_cli/integrations/test_command_upgrade.py @@ -27,6 +27,41 @@ ) class TestIntegrationUpgradeDetailed: + @pytest.mark.parametrize("integration", ["claude", "copilot", "gemini"]) + @pytest.mark.parametrize("force", [False, True]) + def test_disabling_events_honors_modified_dispatcher_force(self, tmp_path, integration, force): + from specify_cli.events import install_integration_events + from specify_cli.integrations import get_integration + from specify_cli.integrations.manifest import IntegrationManifest + + project = _init_project(tmp_path, integration) + manifest = IntegrationManifest.load(integration, project) + install_integration_events( + get_integration(integration), project, manifest, + {"pre_tool_use": [{"command": "speckit.x"}]}, + ) + manifest.save() + dispatcher = project / ".specify/events.py" + content = b"# User-modified dispatcher\n" + dispatcher.write_bytes(content) + args = [ + "integration", "upgrade", integration, + "--integration-options", "--events false", + ] + if force: + args.append("--force") + + result = _run_in_project(project, args) + + assert result.exit_code == (0 if force else 1), result.output + if force: + assert not dispatcher.exists() + updated = IntegrationManifest.load(integration, project) + assert ".specify/events.py" not in updated.files + else: + assert dispatcher.read_bytes() == content + assert "modified" in result.output + def test_upgrade_invalid_manifest_reports_cli_error(self, tmp_path): project = _init_project(tmp_path, "claude") _write_invalid_manifest(project, "claude") diff --git a/tests/specify_cli/integrations/test_installed_adapters.py b/tests/specify_cli/integrations/test_installed_adapters.py index 6f8fbadd92..890afa05a8 100644 --- a/tests/specify_cli/integrations/test_installed_adapters.py +++ b/tests/specify_cli/integrations/test_installed_adapters.py @@ -1696,6 +1696,32 @@ def test_switch_to_and_from_external_adapter_removes_durable_code(tmp_path, serv assert not (project / ".sample-agent/skills/speckit-plan/SKILL.md").exists() assert (project / ".claude/skills/speckit-plan/SKILL.md").is_file() +@pytest.mark.parametrize("operation", ["uninstall", "switch"]) +@pytest.mark.parametrize("force", [False, True]) +def test_legacy_remove_events_adapter_lifecycle(tmp_path, server, operation, force): + body = ''' def remove_events(self, project_root, manifest): + (project_root / "legacy-cleanup-called").write_text("called", encoding="utf-8") +''' + publish(server, code=implementation(body=body)) + project = catalog_project(tmp_path, server) + install(project) + unload_installed_integrations() + arguments = ["integration", operation] + if operation == "switch": + arguments.append("claude") + if force: + arguments.append("--force") + + result = run(project, arguments) + + assert result.exit_code == 0, result.output + assert (project / "legacy-cleanup-called").read_text(encoding="utf-8") == "called" + assert not (project / ".sample-agent/skills/speckit-plan/SKILL.md").exists() + assert not (project / f".specify/integrations/{KEY}.manifest.json").exists() + assert KEY not in read_records(project) + if operation == "switch": + assert (project / ".claude/skills/speckit-plan/SKILL.md").exists() + @pytest.mark.parametrize("step_type", ["command", "prompt"]) def test_public_workflow_dispatch_loads_adapter_and_uses_process_double(tmp_path, server, monkeypatch, step_type):