Files
github-spec-kit/src/specify_cli/integrations/manifest.py
Eldar Shlomi 39921ddd3b fix(shared-infra): record skipped files in speckit.manifest.json (#2483)
* fix(shared-infra): record skipped files in speckit.manifest.json

`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

* fix(shared-infra): guard manifest.record_existing against non-file dst

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

* fix(manifest): lexical pre-check for record_existing + add error-case tests

Address Copilot review (2026-05-11, review id 4266902103):

1. `record_existing` was calling `(self.project_root / rel).is_symlink()`
   BEFORE validating containment. For absolute paths or paths containing
   `..`, this performed a filesystem stat outside the project root before
   `_validate_rel_path()` raised. Add a cheap lexical pre-check that
   delegates to `_validate_rel_path()` for the canonical error messages,
   so the symlink stat only ever runs on paths that are already lexically
   inside the project root.

2. Add focused unit tests in `tests/integrations/test_manifest.py` for
   the symlink and non-regular-file error paths, including:
     - symlink target rejection
     - dangling symlink rejection (caught by the symlink guard before
       the is_file check)
     - directory path rejection (is_file == False)
     - missing-path rejection (is_file == False)
     - absolute-path lexical pre-check
   The Copilot reviewer noted these guards had no focused coverage in
   `test_manifest.py`, only via the `test_integration_claude.py`
   regression test.

3. The third Copilot finding (repeated `dict(self._files)` copies via
   `manifest.files` in the skip branches) is already resolved on this
   branch by using `prior_hashes` — the function-scope snapshot taken at
   the top of `install_shared_infra` — for the membership check, instead
   of `manifest.files`.

AI disclosure: drafted with assistance from Claude (Opus 4.7).

* fix(manifest): track recovered files separately + symlink-ancestor + canonical-path guards

Address Copilot review id 4309888722 (2026-05-18) on PR #2483:

1. Recovery semantics (shared_infra.py:371, 412) — install_shared_infra
   now passes ``recovered=True`` when re-recording a skipped existing
   file. This flag funnels into a new ``recovered_files`` array in the
   manifest JSON, so a future ``refresh_managed`` run can distinguish
   "hash I produced" from "hash I observed on a file that may be a user
   customization" and avoid silent overwrite without ``--refresh-shared-infra``.
   Schema is purely additive: ``files: dict[str, str]`` is unchanged; the
   new ``recovered_files: list[str]`` is omitted when empty.

2. Symlinked ancestor (manifest.py:172) — ``record_existing`` now walks
   every component of the rel path and rejects any symlinked ancestor,
   not just a symlinked leaf. Catches ``linked_dir/file.txt`` where
   ``linked_dir`` is a symlink, which previously slipped past the leaf-only
   ``is_symlink()`` check and was resolved through by ``_validate_rel_path``.
   Mirrors the component-walk pattern in ``_ensure_safe_manifest_directory``.

3. Misleading "escapes project root" message (manifest.py:168) — paths
   like ``dir/../file.txt`` normalize inside the project, so the old
   message lied about what was wrong. New message: "Manifest paths must
   be canonical; '..' segments are not allowed". Still rejects (canonical
   keys are required so ``check_modified``/``uninstall`` cannot key the
   same file under two paths).

Tests: 7 new test methods across TestManifestRecoveredFiles and
TestRecordExistingNewGuards covering all 4 Copilot findings. Full suite
passes locally.

🤖 AI disclosure: drafted with assistance from Claude (Opus 4.7).

* fix(manifest): normalize is_recovered input through _validate_rel_path

Address Copilot review comment id 4309888722 round-5 (2026-05-21) on PR #2483:

``is_recovered()`` previously checked ``self._recovered_files`` membership
with bare ``Path(rel).as_posix()``, while ``record_existing()`` stores keys
via ``_validate_rel_path(rel, root).relative_to(root).as_posix()``. The two
normalizations disagreed on absolute paths and paths that escape the
project root — ``is_recovered`` would silently return False for inputs that
``record_existing`` would have refused entirely.

The fix routes ``is_recovered`` through the same ``_validate_rel_path``
pipeline; ``ValueError`` from the validator is caught and converted to
False so query semantics stay exception-free (Python ``__contains__``
convention).

Tests: 2 new methods in ``TestManifestRecoveredFiles``:
- ``test_is_recovered_absolute_path_returns_false``
- ``test_is_recovered_escaping_path_returns_false``

🤖 AI disclosure: drafted with assistance from Claude (Opus 4.7).

* fix(manifest): clear recovered marker on managed re-record + reject '..' in is_recovered

Address Copilot Round-7 review comments on PR #2483:

1. record_existing(recovered=False) and record_file now BOTH discard the
   path from _recovered_files. The marker is meant to flag "we observed
   this file but cannot vouch it's a managed baseline" — once the same
   path is re-recorded as managed (either explicitly or by writing fresh
   bytes), the marker is stale and must clear so refresh_managed and
   future is_recovered queries return the truthful answer.

2. is_recovered now applies the same canonical-key guard as record_existing
   (rejects absolute paths and '..' segments lexically before delegating
   to _validate_rel_path). Such paths can never be stored keys, so the
   query correctly returns False without depending on _validate_rel_path
   semantics that diverged from record_existing's stricter contract.

record_file docstring updated to mention the side-effect on recovered
markers.

Tests: 3 new methods in TestManifestRecoveredFiles covering
record_existing(false) clearing, record_file clearing, and is_recovered
dotdot rejection.

* test(manifest): update is_recovered comments to reflect Round-7 lexical guard

Round 8 — addresses Copilot review comment on tests/integrations/test_manifest.py:362.

After Round-7 (1dbf0c2), is_recovered() rejects absolute paths and '..' segments
up front via a lexical guard, returning False without calling _validate_rel_path
at all. The test comments still described the prior "_validate_rel_path raises;
we catch" code path, which is misleading for readers.

Updated comments in both:
  - test_is_recovered_absolute_path_returns_false (Copilot's exact target)
  - test_is_recovered_escaping_path_returns_false (same comment-class issue;
    fixed preemptively to avoid a Round-9 finding on the same drift)

Pure documentation change. Test assertions and behavior unchanged; all manifest
tests still green.

* fix(manifest): document OS errors on record_existing + filter orphan recovered_files on load

Round 9 — addresses Copilot review on PR #2483:

1. record_existing's docstring now documents OSError/PermissionError as
   possible raises (in addition to ValueError) — the implementation has
   always been able to raise them from is_symlink, is_file, or the
   file-read used to hash, but the contract did not reflect that.
   Callers should be prepared for both surfaces.

2. load() now filters recovered_files entries that don't correspond to
   keys in files. An externally-edited or partially-corrupted manifest
   can deserialize with orphan recovered paths; rather than reject the
   whole manifest (too strict on the upgrade path), we drop the orphans
   and let the inconsistency self-correct on the next save(). is_recovered
   then returns the truthful False for the orphan.

Tests: new test_load_filters_recovered_files_not_in_files asserting an
orphan recovered entry is dropped on load.
2026-06-02 08:06:31 -05:00

445 lines
18 KiB
Python

"""Hash-tracked installation manifest for integrations.
Each installed integration records the files it created together with
their SHA-256 hashes. On uninstall only files whose hash still matches
the recorded value are removed — modified files are left in place and
reported to the caller.
"""
from __future__ import annotations
import hashlib
import json
import os
import tempfile
from datetime import datetime, timezone
from pathlib import Path
from typing import Any
def _sha256(path: Path) -> str:
"""Return the hex SHA-256 digest of *path*."""
h = hashlib.sha256()
with open(path, "rb") as fh:
for chunk in iter(lambda: fh.read(8192), b""):
h.update(chunk)
return h.hexdigest()
def _validate_rel_path(rel: Path, root: Path) -> Path:
"""Resolve *rel* against *root* and verify it stays within *root*.
Raises ``ValueError`` if *rel* is absolute, contains ``..`` segments
that escape *root*, or otherwise resolves outside the project root.
"""
if rel.is_absolute():
raise ValueError(
f"Absolute paths are not allowed in manifests: {rel}"
)
resolved = (root / rel).resolve()
root_resolved = root.resolve()
try:
resolved.relative_to(root_resolved)
except ValueError:
raise ValueError(
f"Path {rel} resolves to {resolved} which is outside "
f"the project root {root_resolved}"
) from None
return resolved
def _manifest_path_label(root: Path, path: Path) -> str:
try:
return path.relative_to(root).as_posix()
except ValueError:
return path.as_posix()
def _ensure_safe_manifest_directory(root: Path, directory: Path) -> None:
"""Create a manifest directory without following symlinked parents."""
root_resolved = root.resolve()
try:
rel = directory.relative_to(root)
except ValueError:
label = _manifest_path_label(root, directory)
raise ValueError(f"Integration manifest directory escapes project root: {label}") from None
current = root
for part in rel.parts:
current = current / part
label = _manifest_path_label(root, current)
if current.is_symlink():
raise ValueError(f"Refusing to use symlinked integration manifest directory: {label}")
if current.exists():
if not current.is_dir():
raise ValueError(f"Integration manifest directory path is not a directory: {label}")
try:
current.resolve().relative_to(root_resolved)
except (OSError, ValueError):
raise ValueError(f"Integration manifest directory escapes project root: {label}") from None
continue
current.mkdir()
try:
current.resolve().relative_to(root_resolved)
except (OSError, ValueError):
raise ValueError(f"Integration manifest directory escapes project root: {label}") from None
def _ensure_safe_manifest_destination(root: Path, path: Path) -> None:
"""Refuse manifest writes that would escape the project or follow symlinks."""
root_resolved = root.resolve()
_ensure_safe_manifest_directory(root, path.parent)
label = _manifest_path_label(root, path)
if path.is_symlink():
raise ValueError(f"Refusing to overwrite symlinked integration manifest path: {label}")
if path.exists():
if not path.is_file():
raise ValueError(f"Integration manifest path is not a file: {label}")
try:
path.resolve().relative_to(root_resolved)
except (OSError, ValueError):
raise ValueError(f"Integration manifest path escapes project root: {label}") from None
class IntegrationManifest:
"""Tracks files installed by a single integration.
Parameters:
key: Integration identifier (e.g. ``"copilot"``).
project_root: Absolute path to the project directory.
version: CLI version string recorded in the manifest.
"""
def __init__(self, key: str, project_root: Path, version: str = "") -> None:
self.key = key
self.project_root = project_root.resolve()
self.version = version
self._files: dict[str, str] = {} # rel_path → sha256 hex
self._recovered_files: set[str] = set()
self._installed_at: str = ""
# -- Manifest file location -------------------------------------------
@property
def manifest_path(self) -> Path:
"""Path to the on-disk manifest JSON."""
return self.project_root / ".specify" / "integrations" / f"{self.key}.manifest.json"
# -- Recording files --------------------------------------------------
def record_file(self, rel_path: str | Path, content: bytes | str) -> Path:
"""Write *content* to *rel_path* (relative to project root) and record its hash.
Creates parent directories as needed. Returns the absolute path
of the written file.
If the path was previously marked as recovered via
``record_existing(recovered=True)``, the recovered marker is
cleared because the bytes are now produced, not merely observed.
Raises ``ValueError`` if *rel_path* resolves outside the project root.
"""
rel = Path(rel_path)
abs_path = _validate_rel_path(rel, self.project_root)
abs_path.parent.mkdir(parents=True, exist_ok=True)
if isinstance(content, str):
content = content.encode("utf-8")
abs_path.write_bytes(content)
normalized = abs_path.relative_to(self.project_root).as_posix()
self._files[normalized] = hashlib.sha256(content).hexdigest()
# ``record_file`` writes *produced* content, so any prior
# recovered marker for this path is no longer accurate.
self._recovered_files.discard(normalized)
return abs_path
def record_existing(self, rel_path: str | Path, *, recovered: bool = False) -> None:
"""Record the hash of an already-existing regular file at *rel_path*.
When ``recovered=True``, the path is also marked in the manifest's
``recovered_files`` list to signal that the file's on-disk hash was
*observed* during install (because the file already existed and was not
overwritten), not *produced* by the install. Future ``refresh_managed``
runs should consult ``is_recovered`` before treating the recorded hash
as a managed baseline.
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.
OSError: if the underlying filesystem call (``is_symlink``,
``is_file``, or the file-read used to compute the hash)
fails — for example a ``PermissionError`` on the path.
Callers should be prepared to handle ``OSError`` (and its
subclasses such as ``PermissionError``) in addition to
``ValueError``.
"""
rel = Path(rel_path)
# Cheap lexical pre-check first so absolute / parent-traversal paths
# don't trigger a filesystem stat outside the project root before
# ``_validate_rel_path`` raises. ``_validate_rel_path`` produces the
# canonical error messages used elsewhere.
if rel.is_absolute() or ".." in rel.parts:
_validate_rel_path(rel, self.project_root)
# _validate_rel_path raised for any actually-escaping path. If we reach
# here the path normalizes inside root (e.g. ``dir/../file.txt``).
# Reject anyway: manifest keys must be canonical so ``check_modified``
# and ``uninstall`` cannot key the same file under two paths.
raise ValueError(
f"Manifest paths must be canonical; '..' segments are not "
f"allowed (got {rel})"
)
# Walk each path component before resolution so a symlinked ancestor
# (e.g. ``linked_dir/file.txt`` where ``linked_dir`` is a symlink)
# cannot be silently followed by ``_validate_rel_path().resolve()``
# down to a target outside the project root. ``_ensure_safe_manifest_directory``
# uses the same pattern.
_walk = self.project_root
for part in rel.parts:
_walk = _walk / part
if _walk.is_symlink():
raise ValueError(
f"Refusing to record symlinked manifest path: {rel} "
f"(symlinked at {_walk.relative_to(self.project_root).as_posix()})"
)
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)
if recovered:
self._recovered_files.add(normalized)
else:
# ``recovered=False`` means the caller is asserting this path is
# managed-baseline now, not merely observed; drop any stale
# recovered marker so future is_recovered() queries reflect the
# transition. ``discard`` is a no-op when the key is absent.
self._recovered_files.discard(normalized)
# -- Querying ---------------------------------------------------------
@property
def files(self) -> dict[str, str]:
"""Return a copy of the ``{rel_path: sha256}`` mapping."""
return dict(self._files)
@property
def recovered_files(self) -> set[str]:
"""Return a copy of the set of paths recorded with ``recovered=True``.
These entries had their hashes observed (not produced) during install
because the file already existed on disk and the install skipped it.
Their on-disk bytes may be user customizations — callers that would
overwrite based on hash equality (e.g. ``refresh_managed``) MUST check
``is_recovered`` first.
"""
return set(self._recovered_files)
def is_recovered(self, rel_path: str | Path) -> bool:
"""Return True if *rel_path* was recorded via ``record_existing(recovered=True)``.
Input is normalized through the same pipeline as ``record_existing``:
absolute paths, paths escaping the project root, AND paths containing
``'..'`` segments are rejected (returned as ``False``). This mirrors
``record_existing``'s canonicalization guard — such paths can never
appear as stored keys, so the answer is always ``False``.
"""
rel = Path(rel_path)
if rel.is_absolute() or ".." in rel.parts:
return False
try:
abs_path = _validate_rel_path(rel, self.project_root)
normalized = abs_path.relative_to(self.project_root).as_posix()
except ValueError:
return False
return normalized in self._recovered_files
def check_modified(self) -> list[str]:
"""Return relative paths of tracked files whose content changed on disk."""
modified: list[str] = []
for rel, expected_hash in self._files.items():
rel_path = Path(rel)
# Skip paths that are absolute or attempt to escape the project root
if rel_path.is_absolute() or ".." in rel_path.parts:
continue
abs_path = self.project_root / rel_path
if not abs_path.exists() and not abs_path.is_symlink():
continue
# Treat symlinks and non-regular-files as modified
if abs_path.is_symlink() or not abs_path.is_file():
modified.append(rel)
continue
if _sha256(abs_path) != expected_hash:
modified.append(rel)
return modified
# -- Uninstall --------------------------------------------------------
def uninstall(
self,
project_root: Path | None = None,
*,
force: bool = False,
) -> tuple[list[Path], list[Path]]:
"""Remove tracked files whose hash still matches.
Parameters:
project_root: Override for the project root.
force: If ``True``, remove files even if modified.
Returns:
``(removed, skipped)`` — absolute paths.
"""
root = (project_root or self.project_root).resolve()
removed: list[Path] = []
skipped: list[Path] = []
for rel, expected_hash in self._files.items():
# Use non-resolved path for deletion so symlinks themselves
# are removed, not their targets.
path = root / rel
# Validate containment lexically (without following symlinks)
# by collapsing .. segments via Path resolution on the string parts.
try:
normed = Path(os.path.normpath(path))
normed.relative_to(root)
except (ValueError, OSError):
continue
if not path.exists() and not path.is_symlink():
continue
# Skip directories — manifest only tracks files
if not path.is_file() and not path.is_symlink():
skipped.append(path)
continue
# Never follow symlinks when comparing hashes. Only remove
# symlinks when forced, to avoid acting on tampered entries.
if path.is_symlink():
if not force:
skipped.append(path)
continue
else:
if not force and _sha256(path) != expected_hash:
skipped.append(path)
continue
try:
path.unlink()
except OSError:
skipped.append(path)
continue
removed.append(path)
# Clean up empty parent directories up to project root
parent = path.parent
while parent != root:
try:
parent.rmdir() # only succeeds if empty
except OSError:
break
parent = parent.parent
# Remove the manifest file itself
manifest = root / ".specify" / "integrations" / f"{self.key}.manifest.json"
if manifest.exists():
manifest.unlink()
parent = manifest.parent
while parent != root:
try:
parent.rmdir()
except OSError:
break
parent = parent.parent
return removed, skipped
# -- Persistence ------------------------------------------------------
def save(self) -> Path:
"""Write the manifest to disk. Returns the manifest path."""
self._installed_at = self._installed_at or datetime.now(timezone.utc).isoformat()
data: dict[str, Any] = {
"integration": self.key,
"version": self.version,
"installed_at": self._installed_at,
"files": self._files,
**(
{"recovered_files": sorted(self._recovered_files)}
if self._recovered_files
else {}
),
}
path = self.manifest_path
content = json.dumps(data, indent=2) + "\n"
_ensure_safe_manifest_destination(self.project_root, path)
fd, temp_name = tempfile.mkstemp(prefix=f".{path.name}.", dir=path.parent)
temp_path = Path(temp_name)
try:
with os.fdopen(fd, "w", encoding="utf-8") as fh:
fh.write(content)
temp_path.chmod(0o644)
_ensure_safe_manifest_destination(self.project_root, path)
os.replace(temp_path, path)
finally:
if temp_path.exists():
temp_path.unlink()
return path
@classmethod
def load(cls, key: str, project_root: Path) -> IntegrationManifest:
"""Load an existing manifest from disk.
Raises ``FileNotFoundError`` if the manifest does not exist.
"""
inst = cls(key, project_root)
path = inst.manifest_path
try:
data = json.loads(path.read_text(encoding="utf-8"))
except json.JSONDecodeError as exc:
raise ValueError(
f"Integration manifest at {path} contains invalid JSON"
) from exc
if not isinstance(data, dict):
raise ValueError(
f"Integration manifest at {path} must be a JSON object, "
f"got {type(data).__name__}"
)
files = data.get("files", {})
if not isinstance(files, dict) or not all(
isinstance(k, str) and isinstance(v, str) for k, v in files.items()
):
raise ValueError(
f"Integration manifest 'files' at {path} must be a "
"mapping of string paths to string hashes"
)
inst.version = data.get("version", "")
inst._installed_at = data.get("installed_at", "")
inst._files = files
recovered = data.get("recovered_files", [])
if not isinstance(recovered, list) or not all(
isinstance(p, str) for p in recovered
):
raise ValueError(
f"Integration manifest 'recovered_files' at {path} must be a "
"list of string paths"
)
inst._recovered_files = set(recovered)
# Drop any recovered_files entries that don't correspond to tracked
# files — defensive against externally-edited or partially-corrupted
# manifests. Inconsistent state self-corrects on next save().
inst._recovered_files &= set(inst._files.keys())
stored_key = data.get("integration", "")
if stored_key and stored_key != key:
raise ValueError(
f"Manifest at {path} belongs to integration {stored_key!r}, "
f"not {key!r}"
)
return inst