diff --git a/src/specify_cli/integrations/manifest.py b/src/specify_cli/integrations/manifest.py index 258c536e5b..c05b85a991 100644 --- a/src/specify_cli/integrations/manifest.py +++ b/src/specify_cli/integrations/manifest.py @@ -147,12 +147,28 @@ def record_file(self, rel_path: str | Path, content: bytes | str) -> Path: return abs_path def record_existing(self, rel_path: str | Path) -> None: - """Record the hash of an already-existing file at *rel_path*. - - Raises ``ValueError`` if *rel_path* resolves outside the project root. + """Record the hash of an already-existing regular file at *rel_path*. + + Raises: + ValueError: if *rel_path* resolves outside the project root, is + a symlink, or is not a regular file. A directory or other + non-file path cannot be silently recorded — its hash would + be meaningless and ``check_modified``/``uninstall`` would + treat the entry as permanently broken. """ rel = Path(rel_path) + # Check ``is_symlink()`` on the un-resolved path because + # ``_validate_rel_path`` resolves the path (which would follow + # the symlink and silently record the target instead). + if (self.project_root / rel).is_symlink(): + raise ValueError( + f"Refusing to record symlinked manifest path: {rel}" + ) abs_path = _validate_rel_path(rel, self.project_root) + if not abs_path.is_file(): + raise ValueError( + f"Manifest path is not a regular file: {rel}" + ) normalized = abs_path.relative_to(self.project_root).as_posix() self._files[normalized] = _sha256(abs_path) diff --git a/src/specify_cli/shared_infra.py b/src/specify_cli/shared_infra.py index 1e8be7b282..8ebfcd5dae 100644 --- a/src/specify_cli/shared_infra.py +++ b/src/specify_cli/shared_infra.py @@ -266,7 +266,26 @@ def install_shared_infra( dst_path = dest_variant / rel_path _ensure_safe_shared_destination(project_path, dst_path, parent_must_exist=False) if dst_path.exists() and not force: - skipped_files.append(dst_path.relative_to(project_path).as_posix()) + rel_skip = dst_path.relative_to(project_path).as_posix() + skipped_files.append(rel_skip) + # Record the existing-on-disk file in the manifest so a + # fresh manifest run against an already-populated + # ``.specify/`` tree does not silently drop it (#2107). + # Skip if already tracked — preserving the original hash + # keeps user-modification detection working downstream. + # Also skip non-files (directory collision, FIFO, …): + # ``record_existing`` would refuse them, and the path + # still surfaces via the ``skipped_files`` warning below. + if dst_path.is_file() and rel_skip not in manifest.files: + try: + manifest.record_existing(rel_skip) + except (OSError, ValueError) as exc: + # Tolerate races / permission issues / non-file + # collisions so one weird path does not abort + # the whole install. + console.print( + f"[yellow]⚠[/yellow] could not record {rel_skip} in manifest: {exc}" + ) continue _ensure_safe_shared_directory(project_path, dst_path.parent) @@ -284,7 +303,26 @@ def install_shared_infra( dst = dest_templates / src.name _ensure_safe_shared_destination(project_path, dst) if dst.exists() and not force: - skipped_files.append(dst.relative_to(project_path).as_posix()) + rel_skip = dst.relative_to(project_path).as_posix() + skipped_files.append(rel_skip) + # Record the existing-on-disk template in the manifest so a + # fresh manifest run against an already-populated + # ``.specify/`` tree does not silently drop it (#2107). + # Skip if already tracked — preserving the original hash + # keeps user-modification detection working downstream. + # Also skip non-files (directory collision, FIFO, …): + # ``record_existing`` would refuse them, and the path + # still surfaces via the ``skipped_files`` warning below. + if dst.is_file() and rel_skip not in manifest.files: + try: + manifest.record_existing(rel_skip) + except (OSError, ValueError) as exc: + # Tolerate races / permission issues / non-file + # collisions so one weird path does not abort + # the whole install. + console.print( + f"[yellow]⚠[/yellow] could not record {rel_skip} in manifest: {exc}" + ) continue content = src.read_text(encoding="utf-8") @@ -303,7 +341,7 @@ def install_shared_infra( if skipped_files: console.print( - f"[yellow]⚠[/yellow] {len(skipped_files)} shared infrastructure file(s) already exist and were not updated:" + f"[yellow]⚠[/yellow] {len(skipped_files)} shared infrastructure path(s) already exist and were not updated:" ) for path in skipped_files: console.print(f" {path}") diff --git a/tests/integrations/test_integration_claude.py b/tests/integrations/test_integration_claude.py index 142db0dd92..b9948a4585 100644 --- a/tests/integrations/test_integration_claude.py +++ b/tests/integrations/test_integration_claude.py @@ -3,6 +3,7 @@ import codecs import json import os +from pathlib import Path from unittest.mock import patch import yaml @@ -556,3 +557,204 @@ def test_post_process_injects_all_claude_flags(self): assert "user-invocable: true" in result assert "disable-model-invocation: false" in result assert "replace dots" in result + + +class TestSpeckitManifestRecordsSkippedFiles: + """Regression test for issue #2107. + + ``install_shared_infra`` must record every shared-infrastructure file + under ``.specify/`` in ``speckit.manifest.json``, including files that + were *skipped* because they already existed on disk and ``force=False``. + + Before the fix, the skip branches in the scripts and templates loops + appended to ``skipped_files`` without calling ``manifest.record_existing``. + So when ``install_shared_infra`` ran with a fresh (or lost) manifest + against an already-populated ``.specify/`` tree, every file went down the + skip path, ``planned_copies`` and ``planned_templates`` stayed empty, and + ``manifest.save()`` wrote an empty ``files`` field — leaving the + integration believing nothing was installed. + + Reproduction (without the fix) using ``install_shared_infra`` directly: + + install_shared_infra(p, "sh", ..., force=False) # 1st run → 10 files + (p / ".specify/integrations/speckit.manifest.json").unlink() + install_shared_infra(p, "sh", ..., force=False) # 2nd run → 0 files + # ^^ BUG: empty + """ + + def _read_manifest_files(self, project_path: Path) -> dict: + manifest_path = ( + project_path / ".specify" / "integrations" / "speckit.manifest.json" + ) + assert manifest_path.exists(), ( + f"speckit.manifest.json not written at {manifest_path}" + ) + data = json.loads(manifest_path.read_text(encoding="utf-8")) + # ``IntegrationManifest.save`` serialises a ``files`` dict — assert + # the schema explicitly so a regression to a different key (e.g. + # the internal ``_files`` attribute name) fails loudly instead of + # being masked by a silent fallback. + assert isinstance(data, dict), ( + f"manifest root is not a dict, got {type(data).__name__}" + ) + assert "files" in data, ( + f"manifest missing 'files' key, got keys: {sorted(data.keys())}" + ) + files = data["files"] + assert isinstance(files, dict), ( + f"manifest 'files' is not a dict, got {type(files).__name__}" + ) + return files + + def test_install_shared_infra_records_skipped_files(self, tmp_path): + """With ``force=False`` and ``.specify/`` already populated, the + manifest must still record every file — the skip branches are not + allowed to drop files from the manifest.""" + from rich.console import Console + from specify_cli.shared_infra import install_shared_infra + + # Resolve the project's own packaged sources by walking up from this + # test file to the repo root (which contains ``scripts/`` and + # ``templates/`` that ``shared_scripts_source`` looks for). + repo_root = Path(__file__).resolve().parents[2] + console = Console(quiet=True) + + # First run — fresh project, manifest gets populated normally. + install_shared_infra( + tmp_path, + "sh", + version="0.0.0", + core_pack=None, + repo_root=repo_root, + console=console, + force=False, + ) + first_files = self._read_manifest_files(tmp_path) + assert first_files, "first install produced an empty manifest" + + # Simulate a lost manifest while ``.specify/`` is still on disk + # (e.g. the manifest was deleted, corrupted, or the layout was + # extracted out-of-band). + manifest_path = ( + tmp_path / ".specify" / "integrations" / "speckit.manifest.json" + ) + manifest_path.unlink() + + # Second run — every file already exists, so every iteration takes + # the skip branch. With the fix, those files are still recorded. + install_shared_infra( + tmp_path, + "sh", + version="0.0.0", + core_pack=None, + repo_root=repo_root, + console=console, + force=False, + ) + second_files = self._read_manifest_files(tmp_path) + assert second_files, ( + "speckit.manifest.json files dict is empty after install with " + "skipped files (issue #2107) — every file went down the skip " + "branch but none were recorded" + ) + + # The recovered manifest must cover everything the first run tracked. + missing = set(first_files) - set(second_files) + assert not missing, ( + f"these files were tracked on the first install but missing after " + f"the skipped-files re-install: {sorted(missing)[:5]}" + ) + + def test_install_shared_infra_handles_directory_at_script_destination( + self, tmp_path + ): + """A non-file (directory) at a script's destination must NOT crash + ``install_shared_infra`` and must NOT be recorded in the manifest — + the path still appears in the user-visible skipped-paths warning. + """ + from io import StringIO + from rich.console import Console + from specify_cli.shared_infra import install_shared_infra + + repo_root = Path(__file__).resolve().parents[2] + output = StringIO() + console = Console(file=output, force_terminal=False, width=200) + + # Pre-create the .specify/scripts/bash tree, then plant a directory + # where a script file is expected so the skip branch hits a + # non-regular-file path. + bash_dir = tmp_path / ".specify" / "scripts" / "bash" + bash_dir.mkdir(parents=True) + (bash_dir / "common.sh").mkdir() # collision: dir where file expected + + # Must not crash. + install_shared_infra( + tmp_path, + "sh", + version="0.0.0", + core_pack=None, + repo_root=repo_root, + console=console, + force=False, + ) + + files = self._read_manifest_files(tmp_path) + assert ".specify/scripts/bash/common.sh" not in files, ( + "directory at script dst must not be recorded in the manifest" + ) + text = output.getvalue() + assert "common.sh" in text, ( + "directory-at-script-dst path must surface in the skipped warning" + ) + + def test_install_shared_infra_handles_directory_at_template_destination( + self, tmp_path + ): + """Symmetric coverage for the templates loop: a directory at a + template's destination must NOT crash install nor be recorded.""" + from io import StringIO + from rich.console import Console + from specify_cli.shared_infra import install_shared_infra + + repo_root = Path(__file__).resolve().parents[2] + output = StringIO() + console = Console(file=output, force_terminal=False, width=200) + + templates_dir = tmp_path / ".specify" / "templates" + templates_dir.mkdir(parents=True) + + src_templates = repo_root / "templates" + real_template = next( + ( + p.name + for p in src_templates.iterdir() + if p.is_file() + and not p.name.startswith(".") + and p.name != "vscode-settings.json" + ), + None, + ) + assert real_template, ( + "no real template found in repo to collide against" + ) + (templates_dir / real_template).mkdir() # collision + + install_shared_infra( + tmp_path, + "sh", + version="0.0.0", + core_pack=None, + repo_root=repo_root, + console=console, + force=False, + ) + + files = self._read_manifest_files(tmp_path) + template_rel = f".specify/templates/{real_template}" + assert template_rel not in files, ( + "directory at template dst must not be recorded in manifest" + ) + text = output.getvalue() + assert real_template in text, ( + "directory-at-template-dst path must surface in the skipped warning" + )