fix(shared-infra): record skipped files in speckit.manifest.json

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)
 

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}")

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"
+        )