From 080be006ba3bd69dda9edb874a766242212bf5d9 Mon Sep 17 00:00:00 2001 From: kartsan03 Date: Tue, 27 Oct 2026 20:50:59 +0000 Subject: [PATCH 1/6] fix(integrations): refresh manifest hashes after preset registration on switch and use `integration switch` and `integration use` re-register installed presets for the target integration after its manifest is written, but never re-hash the files those presets rewrite. A preset that overrides a core command then shows up as a modified file in `integration status`, and a later `integration upgrade` refuses without `--force`. `upgrade` already resyncs the manifest for this case (#4696). A switch to a newly installed integration resyncs its freshly saved manifest, like upgrade. `use` and a switch to an already-installed integration reuse an older manifest, so they snapshot which tracked files were unmodified before registration and refresh only those whose bytes registration changed. Files the user edited earlier keep their recorded hash and are still reported as modified. Refs #5078 Assisted-by: Codex CLI (model: gpt-6.1-sol, autonomous) Assisted-by: Claude Code (model: claude-opus-5-5, autonomous) --- src/specify_cli/integrations/_helpers.py | 43 +++++++++++++-- .../integrations/command_switch.py | 16 +++++- src/specify_cli/integrations/command_use.py | 10 ++++ tests/specify_cli/integrations/conftest.py | 17 ++++++ .../integrations/test_command_switch.py | 53 +++++++++++++++++++ .../integrations/test_command_use.py | 38 +++++++++++++ 6 files changed, 171 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/integrations/_helpers.py b/src/specify_cli/integrations/_helpers.py index 9bfec2e8c7..fbddd97954 100644 --- a/src/specify_cli/integrations/_helpers.py +++ b/src/specify_cli/integrations/_helpers.py @@ -483,29 +483,62 @@ def _register_presets_for_agent( ) +def _snapshot_manifest_before_registration( + project_root: Path, agent_key: str, +) -> tuple[Any, set[str]]: + """Snapshot an installed manifest's unmodified files before rescaffolding (#5078).""" + try: + from .manifest import IntegrationManifest + + manifest = IntegrationManifest.load(agent_key, project_root) + return manifest, set(manifest.files) - set(manifest.check_modified()) + except FileNotFoundError: + return None, set() + except Exception as manifest_err: # noqa: BLE001 - manifest refresh is best-effort + from .. import _print_cli_warning + + _print_cli_warning( + "snapshot manifest hashes for", + "integration", + agent_key, + manifest_err, + continuing="Continuing without refreshing manifest hashes.", + ) + return None, set() + + def _resync_manifest_after_registration( new_manifest: Any, agent_key: str, *, continuing: str, + unmodified_files: set[str] | None = None, ) -> 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). + For ``upgrade`` (#4696) and a new-target ``switch`` (#5078), nothing else + touches the project between the manifest save and these calls, so any + tracked file whose bytes now differ was changed by registration. Re-hash + it so ``check_modified()`` doesn't misreport a legitimate override. + + ``use`` and installed-target ``switch`` reuse an older manifest (#5078). + They must pass ``unmodified_files`` from before registration; only those + files whose bytes changed are refreshed, preserving earlier user edits. Best-effort: registration itself is best-effort, so a failure here must not abort the surrounding upgrade/use/switch transaction. """ try: changed = False + modified_files = set(new_manifest.check_modified()) if unmodified_files is not None else None for rel in new_manifest.files: + if unmodified_files is not None and ( + rel not in unmodified_files or rel not in modified_files + ): + continue abs_path = new_manifest.project_root / rel try: if abs_path.is_symlink() or not abs_path.is_file(): diff --git a/src/specify_cli/integrations/command_switch.py b/src/specify_cli/integrations/command_switch.py index 24d0fd2c5a..1ffe88a029 100644 --- a/src/specify_cli/integrations/command_switch.py +++ b/src/specify_cli/integrations/command_switch.py @@ -16,7 +16,7 @@ ) from ._commands import integration_app from ._lifecycle import external_lifecycle, lifecycle_owns_rollback, lifecycle_success -from ._helpers import _MANIFEST_READ_ERRORS, _SharedTemplateRefreshError, _clear_init_options_for_integration, _cli_error_detail, _cli_phase_label, _get_speckit_version, _read_integration_json, _register_extensions_for_agent, _register_presets_for_agent, _remove_integration_json, _resolve_integration_options, _resolve_script_type, _set_default_integration, _set_default_integration_or_exit, _unregister_extensions_for_agent, _unregister_presets_for_agent, _write_integration_json +from ._helpers import _MANIFEST_READ_ERRORS, _SharedTemplateRefreshError, _clear_init_options_for_integration, _cli_error_detail, _cli_phase_label, _get_speckit_version, _read_integration_json, _register_extensions_for_agent, _register_presets_for_agent, _remove_integration_json, _resync_manifest_after_registration, _resolve_integration_options, _resolve_script_type, _set_default_integration, _set_default_integration_or_exit, _snapshot_manifest_before_registration, _unregister_extensions_for_agent, _unregister_presets_for_agent, _write_integration_json def _missing_required_options(integration, parsed_options) -> list[str]: @@ -114,6 +114,7 @@ def integration_switch( parsed_options=parsed_options, refresh_templates_force=force, ) + manifest, unmodified_files = _snapshot_manifest_before_registration(project_root, target) _register_extensions_for_agent( project_root, target, @@ -130,6 +131,13 @@ def integration_switch( "need re-registration." ), ) + if manifest is not None: + _resync_manifest_after_registration( + manifest, + target, + unmodified_files=unmodified_files, + continuing="The integration switch succeeded, but its manifest hashes may be stale.", + ) lifecycle_success(f"\n[green]✓[/green] Default integration set to [bold]{target}[/bold].") raise typer.Exit(0) @@ -390,5 +398,11 @@ def integration_switch( continuing="The integration switch succeeded, but installed presets may need re-registration.", ) + _resync_manifest_after_registration( + manifest, + target, + continuing="The integration switch succeeded, but its manifest hashes may be stale.", + ) + name = (target_integration.config or {}).get("name", target) lifecycle_success(f"\n[green]✓[/green] Switched to integration '{escape(str(name))}'") diff --git a/src/specify_cli/integrations/command_use.py b/src/specify_cli/integrations/command_use.py index 97ba3172b5..8af8765029 100644 --- a/src/specify_cli/integrations/command_use.py +++ b/src/specify_cli/integrations/command_use.py @@ -12,8 +12,10 @@ _read_integration_json, _register_extensions_for_agent, _register_presets_for_agent, + _resync_manifest_after_registration, _resolve_integration_options, _set_default_integration_or_exit, + _snapshot_manifest_before_registration, ) @@ -58,6 +60,7 @@ def integration_use( f"[cyan]specify integration use {key} --force[/cyan]." ), ) + manifest, unmodified_files = _snapshot_manifest_before_registration(project_root, key) _register_extensions_for_agent( project_root, key, @@ -68,4 +71,11 @@ def integration_use( key, continuing="The integration was selected, but installed presets may need re-registration.", ) + if manifest is not None: + _resync_manifest_after_registration( + manifest, + key, + unmodified_files=unmodified_files, + continuing="The integration was selected, but its manifest hashes may be stale.", + ) lifecycle_success(f"[green]✓[/green] Default integration set to [bold]{key}[/bold].") diff --git a/tests/specify_cli/integrations/conftest.py b/tests/specify_cli/integrations/conftest.py index c79582bfdf..64f14334a4 100644 --- a/tests/specify_cli/integrations/conftest.py +++ b/tests/specify_cli/integrations/conftest.py @@ -20,3 +20,20 @@ def copilot_project(tmp_path, status_copilot_template): @pytest.fixture def claude_project(tmp_path, status_claude_template): return _copy_project_template(tmp_path, status_claude_template) + + +@pytest.fixture +def tasks_preset_src(tmp_path): + preset = tmp_path / "tasks-preset" + (preset / "commands").mkdir(parents=True) + (preset / "commands/speckit.tasks.md").write_bytes( + b"---\ndescription: Tasks override\n---\nPreset tasks body\n" + ) + (preset / "preset.yml").write_bytes(b"""schema_version: '1.0' +preset: {id: tasks-preset, name: Tasks Preset, version: 1.0.0, description: x} +requires: {speckit_version: '>=0.1.0'} +provides: + templates: + - {type: command, name: speckit.tasks, file: commands/speckit.tasks.md} +""") + return preset diff --git a/tests/specify_cli/integrations/test_command_switch.py b/tests/specify_cli/integrations/test_command_switch.py index 5f293a7b7a..7ad81d3c4e 100644 --- a/tests/specify_cli/integrations/test_command_switch.py +++ b/tests/specify_cli/integrations/test_command_switch.py @@ -2,6 +2,7 @@ from __future__ import annotations +import hashlib import json # noqa: F401 import os # noqa: F401 import shutil # noqa: F401 @@ -26,6 +27,58 @@ ) class TestIntegrationSwitch: + @pytest.mark.parametrize("installed", [False, True]) + def test_switch_syncs_manifest_hash_for_preset_override( + self, tmp_path, tasks_preset_src, installed + ): + """Both switch paths must record the preset's final bytes (#5078).""" + project = _init_project(tmp_path, "claude") + if installed: + result = _run_in_project(project, [ + "integration", "install", "codex", "--script", "sh", + ]) + assert result.exit_code == 0, result.output + result = _run_in_project(project, ["preset", "add", "--dev", str(tasks_preset_src)]) + assert result.exit_code == 0, result.output + + result = _run_in_project(project, ["integration", "switch", "codex", "--script", "sh"]) + assert result.exit_code == 0, result.output + + rel = ".agents/skills/speckit-tasks/SKILL.md" + content = (project / rel).read_bytes() + assert b"Preset tasks body" in content + manifest = json.loads((project / ".specify/integrations/codex.manifest.json").read_bytes()) + assert manifest["files"][rel] == hashlib.sha256(content).hexdigest(), ( + "manifest hash must match the preset-overridden skill (#5078)" + ) + result = _run_in_project(project, ["integration", "upgrade", "codex", "--script", "sh"]) + assert result.exit_code == 0, result.output + + @pytest.mark.parametrize("command", ["plan", "tasks"]) + def test_switch_installed_target_preserves_modified_manifest_hash( + self, tmp_path, tasks_preset_src, command + ): + """Rescaffolding must not bless earlier user edits as pristine (#5078).""" + from specify_cli.integrations.manifest import IntegrationManifest + + project = _init_project(tmp_path, "claude") + result = _run_in_project(project, ["integration", "install", "codex", "--script", "sh"]) + assert result.exit_code == 0, result.output + result = _run_in_project(project, ["preset", "add", "--dev", str(tasks_preset_src)]) + assert result.exit_code == 0, result.output + rel = f".agents/skills/speckit-{command}/SKILL.md" + before = IntegrationManifest.load("codex", project).files[rel] + (project / rel).write_bytes(b"User-edited skill\n") + + result = _run_in_project(project, ["integration", "switch", "codex"]) + assert result.exit_code == 0, result.output + manifest = IntegrationManifest.load("codex", project) + assert manifest.files[rel] == before, "pre-existing edits must not be recorded as pristine" + assert rel in manifest.check_modified() + result = _run_in_project(project, ["integration", "upgrade", "codex", "--script", "sh"]) + assert result.exit_code == 1, result.output + assert "modified since installation" in result.output + @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 diff --git a/tests/specify_cli/integrations/test_command_use.py b/tests/specify_cli/integrations/test_command_use.py index a48056eb88..7d5b337751 100644 --- a/tests/specify_cli/integrations/test_command_use.py +++ b/tests/specify_cli/integrations/test_command_use.py @@ -2,6 +2,7 @@ from __future__ import annotations +import hashlib import json # noqa: F401 import os # noqa: F401 import shutil # noqa: F401 @@ -22,6 +23,43 @@ ) class TestIntegrationUse: + @pytest.mark.parametrize("edited_command", [None, "plan", "tasks"]) + def test_use_syncs_manifest_without_blessing_user_edits( + self, tmp_path, tasks_preset_src, edited_command + ): + """Refresh preset hashes, retaining evidence of earlier edits (#5078).""" + from specify_cli.integrations.manifest import IntegrationManifest + + project = _init_project(tmp_path, "claude") + result = _run_in_project(project, ["integration", "install", "codex", "--script", "sh"]) + assert result.exit_code == 0, result.output + result = _run_in_project(project, ["preset", "add", "--dev", str(tasks_preset_src)]) + assert result.exit_code == 0, result.output + if edited_command: + edited_rel = f".agents/skills/speckit-{edited_command}/SKILL.md" + before = IntegrationManifest.load("codex", project).files[edited_rel] + (project / edited_rel).write_bytes(b"User-edited skill\n") + + result = _run_in_project(project, ["integration", "use", "codex"]) + assert result.exit_code == 0, result.output + manifest = IntegrationManifest.load("codex", project) + if edited_command: + assert manifest.files[edited_rel] == before, "pre-existing edits must not be recorded as pristine" + assert edited_rel in manifest.check_modified() + else: + rel = ".agents/skills/speckit-tasks/SKILL.md" + content = (project / rel).read_bytes() + assert b"Preset tasks body" in content + assert manifest.files[rel] == hashlib.sha256(content).hexdigest(), ( + "manifest hash must match the preset-overridden skill (#5078)" + ) + result = _run_in_project(project, ["integration", "upgrade", "codex", "--script", "sh"]) + if edited_command: + assert result.exit_code == 1, result.output + assert "modified since installation" in result.output + else: + assert result.exit_code == 0, result.output + def test_use_installed_integration_sets_default(self, tmp_path): project = _init_project(tmp_path, "claude") old_cwd = os.getcwd() From 51ef812a9caa941bbb831531959d20eb3f8a96c6 Mon Sep 17 00:00:00 2001 From: kartsan03 Date: Wed, 28 Oct 2026 15:07:23 +0000 Subject: [PATCH 2/6] fix(integrations): compare each file inside the resync error handler With `unmodified_files`, the resync called `check_modified()` before its per-file `OSError` handler, so one path whose probe failed aborted the refresh for every file. Each file is now compared inside that handler: a file that can't be read gets a warning and the others are still refreshed. Refs #5078 Assisted-by: Claude Code (model: claude-opus-5-5, autonomous) --- src/specify_cli/integrations/_helpers.py | 13 +++-- .../integrations/test_command_upgrade.py | 52 +++++++++++++++++-- 2 files changed, 57 insertions(+), 8 deletions(-) diff --git a/src/specify_cli/integrations/_helpers.py b/src/specify_cli/integrations/_helpers.py index fbddd97954..0488a6ba8e 100644 --- a/src/specify_cli/integrations/_helpers.py +++ b/src/specify_cli/integrations/_helpers.py @@ -532,17 +532,20 @@ def _resync_manifest_after_registration( not abort the surrounding upgrade/use/switch transaction. """ try: + from .manifest import _sha256 + changed = False - modified_files = set(new_manifest.check_modified()) if unmodified_files is not None else None - for rel in new_manifest.files: - if unmodified_files is not None and ( - rel not in unmodified_files or rel not in modified_files - ): + for rel, expected_hash in new_manifest.files.items(): + if unmodified_files is not None and rel not in unmodified_files: continue abs_path = new_manifest.project_root / rel try: if abs_path.is_symlink() or not abs_path.is_file(): continue + # Only re-hash bytes registration changed: record_existing() + # also clears a file's recovered marker. + if unmodified_files is not None and _sha256(abs_path) == expected_hash: + 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 979bb5a294..b6ac6fff0a 100644 --- a/tests/specify_cli/integrations/test_command_upgrade.py +++ b/tests/specify_cli/integrations/test_command_upgrade.py @@ -1448,8 +1448,13 @@ def fake_record_existing(self, rel_path, **kwargs): assert reloaded["files"]["ok.md"] == ok_hash assert reloaded["files"]["bad.md"] == stale_hash + @pytest.mark.parametrize( + "unmodified_files", + [None, {"bad.md", "ok.md"}], + ids=["upgrade", "use-or-switch-snapshot"], + ) def test_resync_manifest_probe_error_does_not_abort_remaining_files( - self, tmp_path, capsys + self, tmp_path, capsys, unmodified_files ): """An ``OSError`` from the pre-rehash filesystem probes must warn and continue, not abort the whole resync loop. @@ -1457,7 +1462,9 @@ def test_resync_manifest_probe_error_does_not_abort_remaining_files( ``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. + ``new_manifest.files`` and leave their hashes stale. With + ``unmodified_files`` (``use`` and installed-target ``switch``, #5078) + the changed-bytes check also runs per file, inside that ``try``. """ from specify_cli.integrations._helpers import ( _resync_manifest_after_registration, @@ -1491,7 +1498,10 @@ def fake_is_file(self): with mock.patch.object(Path, "is_file", fake_is_file): _resync_manifest_after_registration( - manifest, "claude", continuing="Continuing." + manifest, + "claude", + continuing="Continuing.", + unmodified_files=unmodified_files, ) captured = strip_ansi(capsys.readouterr().out) @@ -1505,6 +1515,42 @@ def fake_is_file(self): "the remaining tracked files" ) + def test_resync_with_snapshot_skips_files_registration_left_unchanged( + self, tmp_path + ): + """With ``unmodified_files`` only changed bytes are re-recorded (#5078). + + ``record_existing()`` clears a file's recovered marker, so a file + that registration didn't touch must keep its entry as it was. + """ + from specify_cli.integrations._helpers import ( + _resync_manifest_after_registration, + ) + from specify_cli.integrations.manifest import IntegrationManifest + + project = tmp_path / "proj" + project.mkdir() + (project / "kept.md").write_text("kept 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("kept.md", recovered=True) + manifest.record_existing("ok.md") + manifest.save() + (project / "ok.md").write_text("ok content v2\n", encoding="utf-8") + + _resync_manifest_after_registration( + manifest, + "claude", + continuing="Continuing.", + unmodified_files={"kept.md", "ok.md"}, + ) + + reloaded = IntegrationManifest.load("claude", project) + assert reloaded.is_recovered("kept.md") + ok_hash = hashlib.sha256((project / "ok.md").read_bytes()).hexdigest() + assert reloaded.files["ok.md"] == ok_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 00fc9d60a4c64ee6bc0be0ac567685837417a53b Mon Sep 17 00:00:00 2001 From: kartsan03 Date: Wed, 28 Oct 2026 16:54:51 +0000 Subject: [PATCH 3/6] fix(integrations): keep paths outside the project out of the snapshot `check_modified()` skips absolute and `..` keys, so the snapshot counted them as unmodified, and the resync's changed-bytes check then hashed files outside the project before `record_existing()` rejected them. The snapshot now keeps only keys that pass `record_existing()`'s own path check, which also rejects keys under a symlinked directory. Refs #5078 Assisted-by: Claude Code (model: claude-opus-5-5, autonomous) --- src/specify_cli/integrations/_helpers.py | 14 +++++- .../integrations/test_command_upgrade.py | 45 +++++++++++++++++++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/integrations/_helpers.py b/src/specify_cli/integrations/_helpers.py index 0488a6ba8e..7be03e3076 100644 --- a/src/specify_cli/integrations/_helpers.py +++ b/src/specify_cli/integrations/_helpers.py @@ -488,10 +488,20 @@ def _snapshot_manifest_before_registration( ) -> tuple[Any, set[str]]: """Snapshot an installed manifest's unmodified files before rescaffolding (#5078).""" try: - from .manifest import IntegrationManifest + from .manifest import IntegrationManifest, _validate_record_path manifest = IntegrationManifest.load(agent_key, project_root) - return manifest, set(manifest.files) - set(manifest.check_modified()) + unmodified: set[str] = set() + for rel in set(manifest.files) - set(manifest.check_modified()): + # Same path check as record_existing(), so the resync never + # hashes a file outside the project. RuntimeError is a symlink + # loop in Path.resolve() before Python 3.13. + try: + _validate_record_path(Path(rel), manifest.project_root) + except (OSError, RuntimeError, ValueError): + continue + unmodified.add(rel) + return manifest, unmodified except FileNotFoundError: return None, set() except Exception as manifest_err: # noqa: BLE001 - manifest refresh is best-effort diff --git a/tests/specify_cli/integrations/test_command_upgrade.py b/tests/specify_cli/integrations/test_command_upgrade.py index b6ac6fff0a..369ed87a74 100644 --- a/tests/specify_cli/integrations/test_command_upgrade.py +++ b/tests/specify_cli/integrations/test_command_upgrade.py @@ -1551,6 +1551,51 @@ def test_resync_with_snapshot_skips_files_registration_left_unchanged( ok_hash = hashlib.sha256((project / "ok.md").read_bytes()).hexdigest() assert reloaded.files["ok.md"] == ok_hash + @pytest.mark.parametrize("escape", ["dotdot", "symlinked-parent"]) + def test_snapshot_skips_manifest_paths_outside_the_project( + self, tmp_path, escape + ): + """Paths that record_existing() rejects stay out of the snapshot. + + Otherwise the resync's changed-bytes check (#5078) would hash a + file outside the project before ``record_existing()`` refuses it. + """ + from specify_cli.integrations._helpers import ( + _snapshot_manifest_before_registration, + ) + from specify_cli.integrations.manifest import IntegrationManifest + + project = tmp_path / "proj" + project.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + secret = outside / "secret.md" + secret.write_text("outside project\n", encoding="utf-8") + (project / "ok.md").write_text("ok content\n", encoding="utf-8") + + if escape == "dotdot": + escaping_key = "../outside/secret.md" + else: + try: + (project / "outside-link").symlink_to( + outside, target_is_directory=True + ) + except OSError as exc: + pytest.skip(f"symlinks unavailable: {exc}") + escaping_key = "outside-link/secret.md" + + manifest = IntegrationManifest("claude", project, version="test") + manifest.record_existing("ok.md") + manifest._files[escaping_key] = hashlib.sha256(secret.read_bytes()).hexdigest() + manifest.save() + + snapshot, unmodified = _snapshot_manifest_before_registration( + project, "claude" + ) + + assert snapshot is not None + assert unmodified == {"ok.md"} + 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 8545dfaff949d5323ab309dd3ec077012b88fbf0 Mon Sep 17 00:00:00 2001 From: kartsan03 Date: Wed, 28 Oct 2026 17:28:07 +0000 Subject: [PATCH 4/6] fix(integrations): check manifest paths before hashing, keep recovered markers on switch The snapshot used `check_modified()`, which hashes every tracked file before any path check, so a key under a symlinked directory was still read outside the project. The snapshot now runs `record_existing()`'s path check on each key first and then applies the same rules one file at a time. The resync also checks the path before its changed-bytes hash. A new-target `switch` resynced without `unmodified_files`, so it re-recorded every file and cleared recovered markers on files registration didn't change. It now passes every tracked file, since its manifest was saved just before registration. Refs #5078 Assisted-by: Claude Code (model: claude-opus-5-5, autonomous) --- src/specify_cli/integrations/_helpers.py | 35 ++++++--- .../integrations/command_switch.py | 4 + .../integrations/test_command_switch.py | 24 ++++++ .../integrations/test_command_upgrade.py | 78 +++++++++++++++++-- 4 files changed, 122 insertions(+), 19 deletions(-) diff --git a/src/specify_cli/integrations/_helpers.py b/src/specify_cli/integrations/_helpers.py index 7be03e3076..29be8ac602 100644 --- a/src/specify_cli/integrations/_helpers.py +++ b/src/specify_cli/integrations/_helpers.py @@ -486,21 +486,27 @@ def _register_presets_for_agent( def _snapshot_manifest_before_registration( project_root: Path, agent_key: str, ) -> tuple[Any, set[str]]: - """Snapshot an installed manifest's unmodified files before rescaffolding (#5078).""" + """Snapshot an installed manifest's unmodified files before rescaffolding (#5078). + + Same rules as ``check_modified()``, applied one file at a time after + ``record_existing()``'s path check, so no file outside the project is + read. A path that fails the check or can't be read counts as modified. + """ try: - from .manifest import IntegrationManifest, _validate_record_path + from .manifest import IntegrationManifest, _sha256, _validate_record_path manifest = IntegrationManifest.load(agent_key, project_root) unmodified: set[str] = set() - for rel in set(manifest.files) - set(manifest.check_modified()): - # Same path check as record_existing(), so the resync never - # hashes a file outside the project. RuntimeError is a symlink - # loop in Path.resolve() before Python 3.13. + for rel, expected_hash in manifest.files.items(): + # RuntimeError is a symlink loop in Path.resolve() before 3.13. try: - _validate_record_path(Path(rel), manifest.project_root) + abs_path = _validate_record_path(Path(rel), manifest.project_root) + if not abs_path.exists() or ( + abs_path.is_file() and _sha256(abs_path) == expected_hash + ): + unmodified.add(rel) except (OSError, RuntimeError, ValueError): continue - unmodified.add(rel) return manifest, unmodified except FileNotFoundError: return None, set() @@ -537,12 +543,14 @@ def _resync_manifest_after_registration( ``use`` and installed-target ``switch`` reuse an older manifest (#5078). They must pass ``unmodified_files`` from before registration; only those files whose bytes changed are refreshed, preserving earlier user edits. + A new-target ``switch`` passes every tracked file, so files registration + didn't change keep their recovered markers. Best-effort: registration itself is best-effort, so a failure here must not abort the surrounding upgrade/use/switch transaction. """ try: - from .manifest import _sha256 + from .manifest import _sha256, _validate_record_path changed = False for rel, expected_hash in new_manifest.files.items(): @@ -553,12 +561,15 @@ def _resync_manifest_after_registration( if abs_path.is_symlink() or not abs_path.is_file(): continue # Only re-hash bytes registration changed: record_existing() - # also clears a file's recovered marker. - if unmodified_files is not None and _sha256(abs_path) == expected_hash: + # also clears a file's recovered marker. Check the path the + # way record_existing() does before reading it. + if unmodified_files is not None and _sha256( + _validate_record_path(Path(rel), new_manifest.project_root) + ) == expected_hash: continue new_manifest.record_existing(rel) changed = True - except (ValueError, OSError) as file_err: + except (ValueError, OSError, RuntimeError) as file_err: from .. import _print_cli_warning _print_cli_warning( diff --git a/src/specify_cli/integrations/command_switch.py b/src/specify_cli/integrations/command_switch.py index 1ffe88a029..f9a42de3ea 100644 --- a/src/specify_cli/integrations/command_switch.py +++ b/src/specify_cli/integrations/command_switch.py @@ -398,9 +398,13 @@ def integration_switch( continuing="The integration switch succeeded, but installed presets may need re-registration.", ) + # The manifest was saved right before registration, so every tracked + # file is unmodified; passing them all keeps recovered markers on files + # registration didn't change. _resync_manifest_after_registration( manifest, target, + unmodified_files=set(manifest.files), continuing="The integration switch succeeded, but its manifest hashes may be stale.", ) diff --git a/tests/specify_cli/integrations/test_command_switch.py b/tests/specify_cli/integrations/test_command_switch.py index 7ad81d3c4e..04eaf32be8 100644 --- a/tests/specify_cli/integrations/test_command_switch.py +++ b/tests/specify_cli/integrations/test_command_switch.py @@ -54,6 +54,30 @@ def test_switch_syncs_manifest_hash_for_preset_override( result = _run_in_project(project, ["integration", "upgrade", "codex", "--script", "sh"]) assert result.exit_code == 0, result.output + def test_switch_new_target_keeps_recovered_markers(self, tmp_path, monkeypatch): + """The new-target resync must not re-record unchanged files (#5078). + + ``record_existing()`` clears a recovered marker, and ownership checks + such as the events dispatcher cleanup rely on it. + """ + from specify_cli.integrations.codex import CodexIntegration + from specify_cli.integrations.manifest import IntegrationManifest + + rel = ".agents/skills/speckit-plan/SKILL.md" + original_setup = CodexIntegration.setup + + def setup_with_recovered_file(self, project_root, manifest, *args, **kwargs): + created = original_setup(self, project_root, manifest, *args, **kwargs) + manifest.record_existing(rel, recovered=True) + return created + + project = _init_project(tmp_path, "claude") + monkeypatch.setattr(CodexIntegration, "setup", setup_with_recovered_file) + + result = _run_in_project(project, ["integration", "switch", "codex", "--script", "sh"]) + assert result.exit_code == 0, result.output + assert IntegrationManifest.load("codex", project).is_recovered(rel) + @pytest.mark.parametrize("command", ["plan", "tasks"]) def test_switch_installed_target_preserves_modified_manifest_hash( self, tmp_path, tasks_preset_src, command diff --git a/tests/specify_cli/integrations/test_command_upgrade.py b/tests/specify_cli/integrations/test_command_upgrade.py index 369ed87a74..955ba385fe 100644 --- a/tests/specify_cli/integrations/test_command_upgrade.py +++ b/tests/specify_cli/integrations/test_command_upgrade.py @@ -1551,16 +1551,62 @@ def test_resync_with_snapshot_skips_files_registration_left_unchanged( ok_hash = hashlib.sha256((project / "ok.md").read_bytes()).hexdigest() assert reloaded.files["ok.md"] == ok_hash + def test_snapshot_follows_check_modified_rules_one_file_at_a_time( + self, tmp_path + ): + """The snapshot matches ``check_modified()`` for paths in the project. + + A deleted file counts as unmodified, an edited one as modified, and a + file that can't be probed only drops itself from the snapshot (#5078). + """ + import unittest.mock as mock + + from specify_cli.integrations._helpers import ( + _snapshot_manifest_before_registration, + ) + from specify_cli.integrations.manifest import IntegrationManifest + + project = tmp_path / "proj" + project.mkdir() + for name in ("ok.md", "edited.md", "deleted.md", "bad.md"): + (project / name).write_bytes(f"{name} content\n".encode()) + + manifest = IntegrationManifest("claude", project, version="test") + for name in ("ok.md", "edited.md", "deleted.md", "bad.md"): + manifest.record_existing(name) + manifest.save() + (project / "edited.md").write_bytes(b"user edit\n") + (project / "deleted.md").unlink() + + 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) + + with mock.patch.object(Path, "is_file", fake_is_file): + snapshot, unmodified = _snapshot_manifest_before_registration( + project, "claude" + ) + + assert snapshot is not None, "one unreadable file must not drop the snapshot" + assert unmodified == {"ok.md", "deleted.md"} + @pytest.mark.parametrize("escape", ["dotdot", "symlinked-parent"]) def test_snapshot_skips_manifest_paths_outside_the_project( self, tmp_path, escape ): """Paths that record_existing() rejects stay out of the snapshot. - Otherwise the resync's changed-bytes check (#5078) would hash a - file outside the project before ``record_existing()`` refuses it. + They are never hashed either, so the snapshot and the resync's + changed-bytes check (#5078) can't read a file outside the project. """ + import unittest.mock as mock + + from specify_cli.integrations import manifest as manifest_module from specify_cli.integrations._helpers import ( + _resync_manifest_after_registration, _snapshot_manifest_before_registration, ) from specify_cli.integrations.manifest import IntegrationManifest @@ -1589,12 +1635,30 @@ def test_snapshot_skips_manifest_paths_outside_the_project( manifest._files[escaping_key] = hashlib.sha256(secret.read_bytes()).hexdigest() manifest.save() - snapshot, unmodified = _snapshot_manifest_before_registration( - project, "claude" - ) + hashed: list[str] = [] + real_sha256 = manifest_module._sha256 + + def recording_sha256(path): + hashed.append(Path(path).name) + return real_sha256(path) + + with mock.patch.object(manifest_module, "_sha256", recording_sha256): + snapshot, unmodified = _snapshot_manifest_before_registration( + project, "claude" + ) + assert snapshot is not None + assert unmodified == {"ok.md"} + assert "secret.md" not in hashed, "the snapshot hashed a path outside the project" - assert snapshot is not None - assert unmodified == {"ok.md"} + # The resync checks paths itself too, as a new-target switch + # passes every tracked file. + _resync_manifest_after_registration( + snapshot, + "claude", + continuing="Continuing.", + unmodified_files=set(snapshot.files), + ) + assert "secret.md" not in hashed, "the resync hashed a path outside the project" 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 a178d2ec1fe89850af66342b40723ce55f61d719 Mon Sep 17 00:00:00 2001 From: kartsan03 Date: Wed, 28 Oct 2026 19:04:58 +0000 Subject: [PATCH 5/6] fix(integrations): warn when the manifest snapshot skips a file A tracked file that failed the path check, a probe or a read was left out of the snapshot without a word. If registration then changed it, the command reported success and left a stale hash behind. The snapshot now prints a warning that names each such file. The file still stays out of the refresh, so an unknown user edit isn't recorded as pristine. Refs #5078 Assisted-by: Claude Code (model: claude-opus-5-5, autonomous) --- src/specify_cli/integrations/_helpers.py | 15 ++++++++++++--- .../integrations/test_command_upgrade.py | 15 ++++++++++++--- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/integrations/_helpers.py b/src/specify_cli/integrations/_helpers.py index 29be8ac602..9fbb149daa 100644 --- a/src/specify_cli/integrations/_helpers.py +++ b/src/specify_cli/integrations/_helpers.py @@ -490,7 +490,8 @@ def _snapshot_manifest_before_registration( Same rules as ``check_modified()``, applied one file at a time after ``record_existing()``'s path check, so no file outside the project is - read. A path that fails the check or can't be read counts as modified. + read. A path that fails the check or can't be read counts as modified, + with a warning: its hash isn't refreshed after registration. """ try: from .manifest import IntegrationManifest, _sha256, _validate_record_path @@ -505,8 +506,16 @@ def _snapshot_manifest_before_registration( abs_path.is_file() and _sha256(abs_path) == expected_hash ): unmodified.add(rel) - except (OSError, RuntimeError, ValueError): - continue + except (OSError, RuntimeError, ValueError) as file_err: + from .. import _print_cli_warning + + _print_cli_warning( + "snapshot manifest hash for", + "file", + rel, + file_err, + continuing="Its hash won't be refreshed after registration.", + ) return manifest, unmodified except FileNotFoundError: return None, set() diff --git a/tests/specify_cli/integrations/test_command_upgrade.py b/tests/specify_cli/integrations/test_command_upgrade.py index 955ba385fe..1663a720ef 100644 --- a/tests/specify_cli/integrations/test_command_upgrade.py +++ b/tests/specify_cli/integrations/test_command_upgrade.py @@ -1552,12 +1552,13 @@ def test_resync_with_snapshot_skips_files_registration_left_unchanged( assert reloaded.files["ok.md"] == ok_hash def test_snapshot_follows_check_modified_rules_one_file_at_a_time( - self, tmp_path + self, tmp_path, capsys ): """The snapshot matches ``check_modified()`` for paths in the project. A deleted file counts as unmodified, an edited one as modified, and a - file that can't be probed only drops itself from the snapshot (#5078). + file that can't be probed only drops itself from the snapshot (#5078), + with a warning, since its hash won't be refreshed. """ import unittest.mock as mock @@ -1593,9 +1594,14 @@ def fake_is_file(self): assert snapshot is not None, "one unreadable file must not drop the snapshot" assert unmodified == {"ok.md", "deleted.md"} + captured = strip_ansi(capsys.readouterr().out) + assert "Warning:" in captured + assert "'bad.md'" in captured, "an unreadable file was skipped silently" + assert "edited.md" not in captured, "a user edit is not an error" + @pytest.mark.parametrize("escape", ["dotdot", "symlinked-parent"]) def test_snapshot_skips_manifest_paths_outside_the_project( - self, tmp_path, escape + self, tmp_path, capsys, escape ): """Paths that record_existing() rejects stay out of the snapshot. @@ -1649,6 +1655,9 @@ def recording_sha256(path): assert snapshot is not None assert unmodified == {"ok.md"} assert "secret.md" not in hashed, "the snapshot hashed a path outside the project" + assert f"'{escaping_key}'" in strip_ansi(capsys.readouterr().out), ( + "a path left out of the snapshot was skipped silently" + ) # The resync checks paths itself too, as a new-target switch # passes every tracked file. From 2d6351c414e1e5c4c3dfdf8a546115fd9c22eca2 Mon Sep 17 00:00:00 2001 From: kartsan03 Date: Wed, 28 Oct 2026 19:49:12 +0000 Subject: [PATCH 6/6] test(integrations): cover use and switch with a missing or corrupt target manifest `use` and an installed-target `switch` now load the target's manifest before registration. When it's missing, they skip the hash refresh quietly; when it can't be read, they warn and skip it. Either way the command succeeds, registration runs, and the manifest isn't written. Refs #5078 Assisted-by: Claude Code (model: claude-opus-5-5, autonomous) --- .../integrations/test_command_switch.py | 31 +++++++++++++++++++ .../integrations/test_command_use.py | 31 +++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/tests/specify_cli/integrations/test_command_switch.py b/tests/specify_cli/integrations/test_command_switch.py index 04eaf32be8..8341c9adee 100644 --- a/tests/specify_cli/integrations/test_command_switch.py +++ b/tests/specify_cli/integrations/test_command_switch.py @@ -78,6 +78,37 @@ def setup_with_recovered_file(self, project_root, manifest, *args, **kwargs): assert result.exit_code == 0, result.output assert IntegrationManifest.load("codex", project).is_recovered(rel) + @pytest.mark.parametrize("state", ["missing", "corrupt"]) + def test_switch_installed_target_without_readable_manifest_skips_hash_refresh( + self, tmp_path, tasks_preset_src, state + ): + """A missing or unreadable target manifest skips the hash refresh, nothing else (#5078).""" + project = _init_project(tmp_path, "claude") + result = _run_in_project(project, ["integration", "install", "codex", "--script", "sh"]) + assert result.exit_code == 0, result.output + result = _run_in_project(project, ["preset", "add", "--dev", str(tasks_preset_src)]) + assert result.exit_code == 0, result.output + manifest_path = project / ".specify/integrations/codex.manifest.json" + if state == "missing": + manifest_path.unlink() + else: + manifest_path.write_text("{invalid", encoding="utf-8") + + result = _run_in_project(project, ["integration", "switch", "codex"]) + + assert result.exit_code == 0, result.output + data = json.loads((project / ".specify/integration.json").read_text(encoding="utf-8")) + assert data["integration"] == "codex" + skill = project / ".agents/skills/speckit-tasks/SKILL.md" + assert b"Preset tasks body" in skill.read_bytes(), "registration must still run" + output = strip_ansi(result.output) + if state == "missing": + assert not manifest_path.exists(), "no manifest may be created" + assert "Warning:" not in output + else: + assert manifest_path.read_text(encoding="utf-8") == "{invalid" + assert "snapshot manifest hashes for integration 'codex'" in output + @pytest.mark.parametrize("command", ["plan", "tasks"]) def test_switch_installed_target_preserves_modified_manifest_hash( self, tmp_path, tasks_preset_src, command diff --git a/tests/specify_cli/integrations/test_command_use.py b/tests/specify_cli/integrations/test_command_use.py index 7d5b337751..ad47da7d44 100644 --- a/tests/specify_cli/integrations/test_command_use.py +++ b/tests/specify_cli/integrations/test_command_use.py @@ -60,6 +60,37 @@ def test_use_syncs_manifest_without_blessing_user_edits( else: assert result.exit_code == 0, result.output + @pytest.mark.parametrize("state", ["missing", "corrupt"]) + def test_use_without_readable_manifest_skips_hash_refresh( + self, tmp_path, tasks_preset_src, state + ): + """A missing or unreadable manifest skips the hash refresh, nothing else (#5078).""" + project = _init_project(tmp_path, "claude") + result = _run_in_project(project, ["integration", "install", "codex", "--script", "sh"]) + assert result.exit_code == 0, result.output + result = _run_in_project(project, ["preset", "add", "--dev", str(tasks_preset_src)]) + assert result.exit_code == 0, result.output + manifest_path = project / ".specify/integrations/codex.manifest.json" + if state == "missing": + manifest_path.unlink() + else: + manifest_path.write_text("{invalid", encoding="utf-8") + + result = _run_in_project(project, ["integration", "use", "codex"]) + + assert result.exit_code == 0, result.output + data = json.loads((project / ".specify/integration.json").read_text(encoding="utf-8")) + assert data["integration"] == "codex" + skill = project / ".agents/skills/speckit-tasks/SKILL.md" + assert b"Preset tasks body" in skill.read_bytes(), "registration must still run" + output = strip_ansi(result.output) + if state == "missing": + assert not manifest_path.exists(), "no manifest may be created" + assert "Warning:" not in output + else: + assert manifest_path.read_text(encoding="utf-8") == "{invalid" + assert "snapshot manifest hashes for integration 'codex'" in output + def test_use_installed_integration_sets_default(self, tmp_path): project = _init_project(tmp_path, "claude") old_cwd = os.getcwd()