mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
fix(integrations): don't abort uninstall when the manifest can't be deleted (#3805)
`IntegrationManifest.uninstall()` guards every tracked-file `path.unlink()` with `except OSError: skipped.append(path)`, but the manifest's own `manifest.unlink()` is bare. The manifest is deleted *last*, so an undeletable manifest (read-only file, a directory left at the path, a Windows lock) raises after the tracked files are already gone. The caller loses the `(removed, skipped)` result and never runs its post-uninstall bookkeeping — reassigning the default integration, rewriting/removing `integration.json`, clearing init options — leaving a removed integration still recorded as installed. Report it in `skipped` like any other file we could not remove, mirroring the `path.unlink()` guard above and the same `except OSError: skipped.append(...)` pattern in kimi's legacy-directory cleanup. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -400,7 +400,19 @@ class IntegrationManifest:
|
||||
# Remove the manifest file itself
|
||||
manifest = root / ".specify" / "integrations" / f"{self.key}.manifest.json"
|
||||
if remove_manifest and manifest.exists():
|
||||
manifest.unlink()
|
||||
try:
|
||||
manifest.unlink()
|
||||
except OSError:
|
||||
# An undeletable manifest (read-only file, a directory left at
|
||||
# the path, a Windows lock) must not abort the uninstall after
|
||||
# the tracked files were already removed: the caller would lose
|
||||
# the (removed, skipped) result and never run its post-uninstall
|
||||
# bookkeeping. Report it like any other file we could not
|
||||
# remove, mirroring the path.unlink() guard above. The
|
||||
# empty-parent cleanup below is left unconditional: with the
|
||||
# manifest still on disk its parent is non-empty, so the first
|
||||
# rmdir() raises and breaks immediately.
|
||||
skipped.append(manifest)
|
||||
parent = manifest.parent
|
||||
while parent != root:
|
||||
try:
|
||||
|
||||
@@ -242,6 +242,34 @@ class TestManifestUninstall:
|
||||
"remove_manifest=False must keep the manifest file on disk"
|
||||
)
|
||||
|
||||
def test_undeletable_manifest_is_skipped_not_raised(self, tmp_path):
|
||||
"""An undeletable manifest must not abort the whole uninstall.
|
||||
|
||||
The tracked files are removed *before* the manifest, so raising here
|
||||
loses the ``(removed, skipped)`` result the caller needs: the CLI's
|
||||
post-uninstall bookkeeping (reassigning the default integration,
|
||||
rewriting/removing ``integration.json``, clearing init options) never
|
||||
runs, leaving a removed integration still recorded as installed.
|
||||
|
||||
Leaving a directory at the manifest path is a portable way to make
|
||||
``unlink()`` fail with no chmod and no monkeypatch: it raises
|
||||
``IsADirectoryError`` on Linux and ``PermissionError`` on
|
||||
Windows/macOS, both ``OSError`` subclasses.
|
||||
"""
|
||||
m = IntegrationManifest("test", tmp_path, version="1.0")
|
||||
m.record_file("f.txt", "content")
|
||||
m.save()
|
||||
m.manifest_path.unlink()
|
||||
m.manifest_path.mkdir()
|
||||
|
||||
removed, skipped = m.uninstall()
|
||||
|
||||
assert removed == [tmp_path / "f.txt"]
|
||||
assert not (tmp_path / "f.txt").exists()
|
||||
assert m.manifest_path in skipped, (
|
||||
"an undeletable manifest must be reported in skipped"
|
||||
)
|
||||
|
||||
def test_cleans_empty_parent_dirs(self, tmp_path):
|
||||
m = IntegrationManifest("test", tmp_path)
|
||||
m.record_file("a/b/c/f.txt", "content")
|
||||
|
||||
Reference in New Issue
Block a user