Skip to content

Commit 21dc310

Browse files
committed
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.
1 parent 7666030 commit 21dc310

2 files changed

Lines changed: 28 additions & 0 deletions

File tree

src/specify_cli/integrations/manifest.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,12 @@ def record_existing(self, rel_path: str | Path, *, recovered: bool = False) -> N
169169
non-file path cannot be silently recorded — its hash would
170170
be meaningless and ``check_modified``/``uninstall`` would
171171
treat the entry as permanently broken.
172+
OSError: if the underlying filesystem call (``is_symlink``,
173+
``is_file``, or the file-read used to compute the hash)
174+
fails — for example a ``PermissionError`` on the path.
175+
Callers should be prepared to handle ``OSError`` (and its
176+
subclasses such as ``PermissionError``) in addition to
177+
``ValueError``.
172178
"""
173179
rel = Path(rel_path)
174180
# Cheap lexical pre-check first so absolute / parent-traversal paths
@@ -423,6 +429,10 @@ def load(cls, key: str, project_root: Path) -> IntegrationManifest:
423429
"list of string paths"
424430
)
425431
inst._recovered_files = set(recovered)
432+
# Drop any recovered_files entries that don't correspond to tracked
433+
# files — defensive against externally-edited or partially-corrupted
434+
# manifests. Inconsistent state self-corrects on next save().
435+
inst._recovered_files &= set(inst._files.keys())
426436

427437
stored_key = data.get("integration", "")
428438
if stored_key and stored_key != key:

tests/integrations/test_manifest.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,24 @@ def test_load_invalid_json_raises(self, tmp_path):
297297
with pytest.raises(ValueError, match="invalid JSON"):
298298
IntegrationManifest.load("bad", tmp_path)
299299

300+
def test_load_filters_recovered_files_not_in_files(self, tmp_path):
301+
# Finding B (Round-9): a recovered_files entry referencing a path
302+
# not present in files indicates an internally-inconsistent manifest
303+
# (e.g. external edit). load() filters those entries silently so the
304+
# manifest self-heals on next save(); is_recovered then returns the
305+
# truthful False for the orphan.
306+
path = tmp_path / ".specify" / "integrations" / "test.manifest.json"
307+
path.parent.mkdir(parents=True)
308+
path.write_text(json.dumps({
309+
"integration": "test",
310+
"files": {"kept.txt": "abc123"},
311+
"recovered_files": ["kept.txt", "orphan.txt"],
312+
}), encoding="utf-8")
313+
m = IntegrationManifest.load("test", tmp_path)
314+
assert m.recovered_files == {"kept.txt"}
315+
assert m.is_recovered("kept.txt") is True
316+
assert m.is_recovered("orphan.txt") is False
317+
300318

301319
class TestManifestRecoveredFiles:
302320
"""Coverage for the ``recovered_files`` channel added in #2483.

0 commit comments

Comments
 (0)