diff --git a/src/specify_cli/integrations/_helpers.py b/src/specify_cli/integrations/_helpers.py index 2b7fc65db1..38e87294ec 100644 --- a/src/specify_cli/integrations/_helpers.py +++ b/src/specify_cli/integrations/_helpers.py @@ -479,6 +479,60 @@ def _register_presets_for_agent( ) +def _resync_manifest_after_registration( + new_manifest: Any, + agent_key: str, + *, + continuing: str, +) -> None: + """Refresh tracked-file hashes after extensions/presets re-registration. + + ``_register_extensions_for_agent`` / ``_register_presets_for_agent`` run + after ``new_manifest`` is saved and can overwrite files it already + tracks (e.g. a preset overriding a core command rendered as a skill). + Nothing else touches the project between the manifest save and these + calls, so any tracked file whose bytes now differ was changed by our own + registration step, not by the user — re-hash it and persist the refresh + so ``check_modified()`` doesn't misreport a legitimate override as + tampering (see #4696). + + Best-effort: registration itself is best-effort, so a failure here must + not abort the surrounding upgrade/use/switch transaction. + """ + try: + changed = False + for rel in new_manifest.files: + abs_path = new_manifest.project_root / rel + try: + if abs_path.is_symlink() or not abs_path.is_file(): + continue + new_manifest.record_existing(rel) + changed = True + except (ValueError, OSError) as file_err: + from .. import _print_cli_warning + + _print_cli_warning( + "resync manifest hash for", + "file", + str(rel), + file_err, + continuing="Continuing with the remaining files.", + ) + continue + if changed: + new_manifest.save() + except Exception as resync_err: + from .. import _print_cli_warning + + _print_cli_warning( + "resync manifest hashes for", + "integration", + agent_key, + resync_err, + continuing=continuing, + ) + + def _unregister_presets_for_agent( project_root: Path, agent_key: str, diff --git a/src/specify_cli/integrations/command_upgrade.py b/src/specify_cli/integrations/command_upgrade.py index 7a805041d4..b4682b6825 100644 --- a/src/specify_cli/integrations/command_upgrade.py +++ b/src/specify_cli/integrations/command_upgrade.py @@ -22,7 +22,7 @@ _manifest_tracks_skill_layout, ) from ._commands import integration_app -from ._helpers import _MANIFEST_READ_ERRORS, _SharedTemplateRefreshError, _cli_error_detail, _cli_phase_label, _get_speckit_version, _read_integration_json, _refresh_init_options_speckit_version, _register_extensions_for_agent, _register_presets_for_agent, _resolve_integration_options, _resolve_integration_script_type, _unregister_enabled_extension_commands_for_agent, _update_init_options_for_integration, _write_integration_json +from ._helpers import _MANIFEST_READ_ERRORS, _SharedTemplateRefreshError, _cli_error_detail, _cli_phase_label, _get_speckit_version, _read_integration_json, _refresh_init_options_speckit_version, _register_extensions_for_agent, _register_presets_for_agent, _resolve_integration_options, _resolve_integration_script_type, _resync_manifest_after_registration, _unregister_enabled_extension_commands_for_agent, _update_init_options_for_integration, _write_integration_json @integration_app.command("upgrade") @@ -343,6 +343,14 @@ def integration_upgrade( key, continuing="The integration was upgraded, but installed presets may need re-registration.", ) + _resync_manifest_after_registration( + new_manifest, + key, + continuing=( + "The integration was upgraded, but the manifest may report " + "preset/extension overrides as modified files." + ), + ) name = (integration.config or {}).get("name", key) console.print(f"\n[green]✓[/green] Integration '{name}' upgraded successfully") diff --git a/tests/specify_cli/integrations/test_command_upgrade.py b/tests/specify_cli/integrations/test_command_upgrade.py index 2433f823b5..42d5ea0a6c 100644 --- a/tests/specify_cli/integrations/test_command_upgrade.py +++ b/tests/specify_cli/integrations/test_command_upgrade.py @@ -2,6 +2,7 @@ from __future__ import annotations +import hashlib import json # noqa: F401 import os # noqa: F401 import shutil # noqa: F401 @@ -1281,6 +1282,194 @@ def test_upgrade_active_integration_reregisters_presets(self, tmp_path): assert result.exit_code == 0, result.output assert "Preset upgrade body" in skill_file.read_text(encoding="utf-8") + def test_upgrade_force_syncs_manifest_hash_for_preset_overridden_skill( + self, tmp_path + ): + """Regression for #4696. + + A preset overriding a core command that renders as a skill + (e.g. ``speckit.tasks`` for ``codex``/``claude``) is rewritten by + ``_register_presets_for_agent`` *after* ``new_manifest.save()`` during + ``integration upgrade --force``. Without a post-registration resync, + the manifest keeps the base template's hash for that skill file, so + ``integration status`` immediately reports it as modified and a + subsequent ``upgrade`` (without ``--force``) is blocked. + """ + import yaml + + project = _init_project(tmp_path, "claude") + + preset_src = tmp_path / "tasks-preset" + (preset_src / "commands").mkdir(parents=True) + (preset_src / "commands" / "speckit.tasks.md").write_text( + "---\ndescription: Tasks override\n---\nPreset tasks body\n", + encoding="utf-8", + ) + manifest = { + "schema_version": "1.0", + "preset": { + "id": "tasks-preset", + "name": "Tasks Preset", + "version": "1.0.0", + "description": "Preset overriding speckit.tasks", + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "templates": [ + { + "type": "command", + "name": "speckit.tasks", + "file": "commands/speckit.tasks.md", + } + ] + }, + } + (preset_src / "preset.yml").write_text( + yaml.dump(manifest), encoding="utf-8" + ) + + result = _run_in_project(project, ["preset", "add", "--dev", str(preset_src)]) + assert result.exit_code == 0, result.output + + skill_rel = ".claude/skills/speckit-tasks/SKILL.md" + skill_file = project / skill_rel + assert "Preset tasks body" in skill_file.read_text(encoding="utf-8") + + result = _run_in_project(project, [ + "integration", "upgrade", "claude", + "--script", "sh", "--force", + ]) + assert result.exit_code == 0, result.output + + manifest_path = project / ".specify" / "integrations" / "claude.manifest.json" + recorded_hash = json.loads(manifest_path.read_text(encoding="utf-8"))["files"][skill_rel] + actual_hash = hashlib.sha256(skill_file.read_bytes()).hexdigest() + assert recorded_hash == actual_hash, ( + "manifest hash for the preset-overridden skill must match the " + "file `_register_presets_for_agent` just wrote" + ) + + status_result = _run_in_project(project, ["integration", "status"]) + assert "Integration status: OK" in status_result.output, status_result.output + assert "Modified managed files: 0" in status_result.output + assert "managed-files-modified" not in status_result.output + + def test_resync_manifest_warns_on_per_file_failure_and_keeps_going( + self, tmp_path, capsys + ): + """A single file's rehash failure must warn, not vanish silently. + + ``_resync_manifest_after_registration`` best-effort-skips files it + can't rehash, but a skip that produces no warning leaves the user + with a stale hash and no signal that the manifest wasn't fully + synchronized. Rehashing must also continue for the remaining files. + """ + from specify_cli.integrations._helpers import ( + _resync_manifest_after_registration, + ) + from specify_cli.integrations.manifest import IntegrationManifest + + project = tmp_path / "proj" + project.mkdir() + (project / "ok.md").write_text("ok content\n", encoding="utf-8") + (project / "bad.md").write_text("bad content\n", encoding="utf-8") + + manifest = IntegrationManifest("claude", project, version="test") + manifest.record_existing("ok.md") + manifest.record_existing("bad.md") + stale_hash = manifest._files["bad.md"] + manifest.save() + + # Both files' bytes changed on disk after the manifest was saved + # (simulating registration overwriting them), but only "bad.md" + # fails to rehash. + (project / "ok.md").write_text("ok content v2\n", encoding="utf-8") + (project / "bad.md").write_text("bad content v2\n", encoding="utf-8") + + real_record_existing = IntegrationManifest.record_existing + + def fake_record_existing(self, rel_path, **kwargs): + if str(rel_path) == "bad.md": + raise OSError("permission denied") + return real_record_existing(self, rel_path, **kwargs) + + import unittest.mock as mock + + with mock.patch.object( + IntegrationManifest, "record_existing", fake_record_existing + ): + _resync_manifest_after_registration( + manifest, "claude", continuing="Continuing." + ) + + captured = strip_ansi(capsys.readouterr().out) + assert "Warning:" in captured + assert "bad.md" in captured + + reloaded = json.loads(manifest.manifest_path.read_text(encoding="utf-8")) + ok_hash = hashlib.sha256( + (project / "ok.md").read_bytes() + ).hexdigest() + assert reloaded["files"]["ok.md"] == ok_hash + assert reloaded["files"]["bad.md"] == stale_hash + + def test_resync_manifest_probe_error_does_not_abort_remaining_files( + self, tmp_path, capsys + ): + """An ``OSError`` from the pre-rehash filesystem probes must warn + and continue, not abort the whole resync loop. + + ``is_symlink()``/``is_file()`` run before the per-file ``try`` that + wraps ``record_existing()``. If one of those probes raises (e.g. an + inaccessible path), it must not jump past the remaining files in + ``new_manifest.files`` and leave their hashes stale. + """ + from specify_cli.integrations._helpers import ( + _resync_manifest_after_registration, + ) + from specify_cli.integrations.manifest import IntegrationManifest + + project = tmp_path / "proj" + project.mkdir() + (project / "bad.md").write_text("bad content\n", encoding="utf-8") + (project / "ok.md").write_text("ok content\n", encoding="utf-8") + + manifest = IntegrationManifest("claude", project, version="test") + manifest.record_existing("bad.md") + manifest.record_existing("ok.md") + manifest.save() + + # Both files' bytes changed on disk after the manifest was saved + # (simulating registration overwriting them), but "bad.md" fails + # during the pre-rehash filesystem probe, not during rehashing. + (project / "bad.md").write_text("bad content v2\n", encoding="utf-8") + (project / "ok.md").write_text("ok content v2\n", encoding="utf-8") + + real_is_file = Path.is_file + + def fake_is_file(self): + if self.name == "bad.md": + raise OSError("permission denied") + return real_is_file(self) + + import unittest.mock as mock + + with mock.patch.object(Path, "is_file", fake_is_file): + _resync_manifest_after_registration( + manifest, "claude", continuing="Continuing." + ) + + captured = strip_ansi(capsys.readouterr().out) + assert "Warning:" in captured + assert "bad.md" in captured + + reloaded = json.loads(manifest.manifest_path.read_text(encoding="utf-8")) + ok_hash = hashlib.sha256((project / "ok.md").read_bytes()).hexdigest() + assert reloaded["files"]["ok.md"] == ok_hash, ( + "a probe error on an earlier file must not abort rehashing of " + "the remaining tracked files" + ) + def test_upgrade_non_active_agent_preserves_active_agent_skills(self, tmp_path): """Upgrading a non-active agent must not touch the active agent's skills.