From cca84ae04c471026cd3fd1b374785394d6ce482c Mon Sep 17 00:00:00 2001 From: eldar702 Date: Thu, 7 May 2026 20:57:30 +0300 Subject: [PATCH 1/2] fix(shared-infra): record skipped files in speckit.manifest.json MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `install_shared_infra` skipped files that already existed on disk when `force=False`, but the skip branches in both the scripts loop and the templates loop only appended to `skipped_files` without calling `manifest.record_existing`. So when the function ran with a fresh manifest against an already-populated `.specify/` tree (e.g. after the manifest was deleted, corrupted, or extracted out of band), every file went down the skip path, `planned_copies` / `planned_templates` stayed empty, and `manifest.save()` wrote an empty `files` field — leaving the integration believing nothing was installed. Record every skipped file in the manifest, but only when it is not already tracked. This preserves the original hash for files that were previously recorded so `check_modified()` (used by `integration use` to decide whether a user has customized a template) keeps working correctly. Add `TestSpeckitManifestRecordsSkippedFiles` in `tests/integrations/test_integration_claude.py` covering both the fresh-skip path and the recover-after-lost-manifest path. Fixes #2107 --- src/specify_cli/shared_infra.py | 20 +++- tests/integrations/test_integration_claude.py | 94 +++++++++++++++++++ 2 files changed, 112 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/shared_infra.py b/src/specify_cli/shared_infra.py index 1e8be7b282..abeb04bc51 100644 --- a/src/specify_cli/shared_infra.py +++ b/src/specify_cli/shared_infra.py @@ -266,7 +266,15 @@ 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. + if rel_skip not in manifest.files: + manifest.record_existing(rel_skip) continue _ensure_safe_shared_directory(project_path, dst_path.parent) @@ -284,7 +292,15 @@ 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. + if rel_skip not in manifest.files: + manifest.record_existing(rel_skip) continue content = src.read_text(encoding="utf-8") diff --git a/tests/integrations/test_integration_claude.py b/tests/integrations/test_integration_claude.py index 142db0dd92..5ed3b0fa0a 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,96 @@ 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")) + return data.get("files") or data.get("_files") or {} + + 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]}" + ) From 6790654e50aef5e4baabaa2569fb12b58fa15453 Mon Sep 17 00:00:00 2001 From: eldar702 Date: Sun, 10 May 2026 15:01:28 +0300 Subject: [PATCH 2/2] fix(shared-infra): guard manifest.record_existing against non-file dst MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Copilot review feedback on PR #2483. The previous fix called ``manifest.record_existing(rel_skip)`` from the skip branch of both loops in ``install_shared_infra``, which would crash with ``IsADirectoryError`` (or another ``OSError``) if a directory or other non-regular-file happened to exist at the expected destination path — since ``record_existing`` opens the file to compute its SHA-256. Three coordinated fixes: 1. ``IntegrationManifest.record_existing`` now validates its precondition: it raises ``ValueError`` if the path is a symlink or is not a regular file. The docstring already promised "an already-existing file"; this enforces it. The symlink check runs on the un-resolved path because ``_validate_rel_path`` calls ``resolve()``, which would silently follow the symlink. Mirrors the existing ``_ensure_safe_manifest_destination`` precedent in the same module. 2. In ``install_shared_infra``'s scripts and templates skip branches, guard the ``record_existing`` call with ``dst.is_file()`` and wrap it in ``try/except (OSError, ValueError)``. A directory collision, permission error, or TOCTOU race no longer aborts the whole install — the user gets a per-path warning, the path still surfaces in ``skipped_files``, and the rest of the install continues. 3. ``_read_manifest_files`` in the regression test no longer falls back to ``data.get("_files")`` (Copilot's low-confidence finding): the silent fallback could mask a schema regression where the public ``files`` key is renamed. It now asserts ``"files" in data`` and that the value is a dict. Add two regression tests in ``TestSpeckitManifestRecordsSkippedFiles`` covering the directory-at-destination edge case for both the scripts loop and the templates loop. Both verify (a) install does not crash, (b) the non-file path is not recorded in the manifest, and (c) the path still surfaces in the user-visible warning. The "shared infrastructure file(s)" warning text is changed to "path(s)" so it remains accurate when non-file entries appear in the list. Refs #2107 --- src/specify_cli/integrations/manifest.py | 22 +++- src/specify_cli/shared_infra.py | 32 ++++- tests/integrations/test_integration_claude.py | 110 +++++++++++++++++- 3 files changed, 155 insertions(+), 9 deletions(-) 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 abeb04bc51..8ebfcd5dae 100644 --- a/src/specify_cli/shared_infra.py +++ b/src/specify_cli/shared_infra.py @@ -273,8 +273,19 @@ def install_shared_infra( # ``.specify/`` tree does not silently drop it (#2107). # Skip if already tracked — preserving the original hash # keeps user-modification detection working downstream. - if rel_skip not in manifest.files: - manifest.record_existing(rel_skip) + # 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) @@ -299,8 +310,19 @@ def install_shared_infra( # ``.specify/`` tree does not silently drop it (#2107). # Skip if already tracked — preserving the original hash # keeps user-modification detection working downstream. - if rel_skip not in manifest.files: - manifest.record_existing(rel_skip) + # 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") @@ -319,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 5ed3b0fa0a..b9948a4585 100644 --- a/tests/integrations/test_integration_claude.py +++ b/tests/integrations/test_integration_claude.py @@ -590,7 +590,21 @@ def _read_manifest_files(self, project_path: Path) -> dict: f"speckit.manifest.json not written at {manifest_path}" ) data = json.loads(manifest_path.read_text(encoding="utf-8")) - return data.get("files") or data.get("_files") or {} + # ``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 @@ -650,3 +664,97 @@ def test_install_shared_infra_records_skipped_files(self, tmp_path): 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" + )