mirror of
https://github.com/github/spec-kit.git
synced 2026-07-05 21:49:47 +08:00
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
This commit is contained in:
@@ -147,12 +147,28 @@ class IntegrationManifest:
|
||||
return abs_path
|
||||
|
||||
def record_existing(self, rel_path: str | Path) -> None:
|
||||
"""Record the hash of an already-existing file at *rel_path*.
|
||||
"""Record the hash of an already-existing regular file at *rel_path*.
|
||||
|
||||
Raises ``ValueError`` if *rel_path* resolves outside the project root.
|
||||
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)
|
||||
|
||||
|
||||
@@ -435,7 +435,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}")
|
||||
|
||||
@@ -590,7 +590,21 @@ class TestSpeckitManifestRecordsSkippedFiles:
|
||||
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 @@ class TestSpeckitManifestRecordsSkippedFiles:
|
||||
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"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user