From a9c0e96c6303c70f0c70e19b2062e8dd722dc832 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Wed, 23 Sep 2026 11:01:39 +0000 Subject: [PATCH 1/3] fix: sync integration manifest hashes after preset re-registration on upgrade specify integration upgrade --force ran _register_presets_for_agent() after new_manifest.save(), so a preset overriding a core command rendered as a skill (e.g. speckit.tasks) overwrote the file on disk without updating its recorded hash. integration status then reported the legitimate override as manually modified, and a subsequent upgrade without --force was blocked. Resync tracked-file hashes into the manifest right after extensions/presets are re-registered for the active integration, then persist the refresh. Fixes #4696 --- src/specify_cli/integrations/_helpers.py | 45 ++++++++++++ .../integrations/command_upgrade.py | 10 ++- .../integrations/test_command_upgrade.py | 73 +++++++++++++++++++ 3 files changed, 127 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/integrations/_helpers.py b/src/specify_cli/integrations/_helpers.py index 2b7fc65db1..4b6701efa4 100644 --- a/src/specify_cli/integrations/_helpers.py +++ b/src/specify_cli/integrations/_helpers.py @@ -479,6 +479,51 @@ 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 + if abs_path.is_symlink() or not abs_path.is_file(): + continue + try: + new_manifest.record_existing(rel) + changed = True + except (ValueError, OSError): + 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..18e66560a0 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,78 @@ 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_upgrade_non_active_agent_preserves_active_agent_skills(self, tmp_path): """Upgrading a non-active agent must not touch the active agent's skills. From 0d31bec356b40161cdd8458b46ed40891fd4a38a Mon Sep 17 00:00:00 2001 From: chelsealong Date: Wed, 23 Sep 2026 17:39:04 +0000 Subject: [PATCH 2/3] fix: warn on per-file manifest resync failure instead of silently skipping _resync_manifest_after_registration() silently discarded ValueError/OSError for individual files, leaving a stale hash with no signal to the user. Emit the existing best-effort warning per failed file and keep resyncing the rest. --- src/specify_cli/integrations/_helpers.py | 11 +++- .../integrations/test_command_upgrade.py | 59 +++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/integrations/_helpers.py b/src/specify_cli/integrations/_helpers.py index 4b6701efa4..5fe40ede17 100644 --- a/src/specify_cli/integrations/_helpers.py +++ b/src/specify_cli/integrations/_helpers.py @@ -508,7 +508,16 @@ def _resync_manifest_after_registration( try: new_manifest.record_existing(rel) changed = True - except (ValueError, OSError): + 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() diff --git a/tests/specify_cli/integrations/test_command_upgrade.py b/tests/specify_cli/integrations/test_command_upgrade.py index 18e66560a0..1c4198c2d0 100644 --- a/tests/specify_cli/integrations/test_command_upgrade.py +++ b/tests/specify_cli/integrations/test_command_upgrade.py @@ -1354,6 +1354,65 @@ def test_upgrade_force_syncs_manifest_hash_for_preset_overridden_skill( 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_upgrade_non_active_agent_preserves_active_agent_skills(self, tmp_path): """Upgrading a non-active agent must not touch the active agent's skills. From feb086a1841ed100e0efbfa16cb1bb03831132c0 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Wed, 23 Sep 2026 21:12:22 +0000 Subject: [PATCH 3/3] fix: keep per-file probe errors from aborting manifest resync loop Move the is_symlink()/is_file() probes in _resync_manifest_after_registration() inside the per-file try so an OSError from either (e.g. an inaccessible path) is caught by the existing best-effort warning-and-continue handler instead of the outer handler, which was aborting the whole resync and leaving later files' hashes stale. --- src/specify_cli/integrations/_helpers.py | 4 +- .../integrations/test_command_upgrade.py | 57 +++++++++++++++++++ 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/integrations/_helpers.py b/src/specify_cli/integrations/_helpers.py index 5fe40ede17..38e87294ec 100644 --- a/src/specify_cli/integrations/_helpers.py +++ b/src/specify_cli/integrations/_helpers.py @@ -503,9 +503,9 @@ def _resync_manifest_after_registration( changed = False for rel in new_manifest.files: abs_path = new_manifest.project_root / rel - if abs_path.is_symlink() or not abs_path.is_file(): - continue 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: diff --git a/tests/specify_cli/integrations/test_command_upgrade.py b/tests/specify_cli/integrations/test_command_upgrade.py index 1c4198c2d0..42d5ea0a6c 100644 --- a/tests/specify_cli/integrations/test_command_upgrade.py +++ b/tests/specify_cli/integrations/test_command_upgrade.py @@ -1413,6 +1413,63 @@ def fake_record_existing(self, rel_path, **kwargs): 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.