mirror of
https://github.com/github/spec-kit.git
synced 2026-07-06 05:53:12 +08:00
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).
This commit is contained in:
@@ -222,9 +222,20 @@ class IntegrationManifest:
|
||||
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)``."""
|
||||
rel = Path(rel_path)
|
||||
normalized = rel.as_posix()
|
||||
"""Return True if *rel_path* was recorded via ``record_existing(recovered=True)``.
|
||||
|
||||
Input is normalized through the same ``_validate_rel_path`` pipeline that
|
||||
``record_existing`` uses for its stored keys, so the two methods agree
|
||||
on key format. Absolute paths and paths that escape the project root
|
||||
return ``False`` (they cannot match the relative POSIX keys we store) —
|
||||
consistent with Python's membership-predicate convention of not raising
|
||||
on a not-in-set query.
|
||||
"""
|
||||
try:
|
||||
abs_path = _validate_rel_path(Path(rel_path), 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]:
|
||||
|
||||
@@ -354,6 +354,27 @@ class TestManifestRecoveredFiles:
|
||||
with pytest.raises(ValueError, match="recovered_files"):
|
||||
IntegrationManifest.load("bad", tmp_path)
|
||||
|
||||
def test_is_recovered_absolute_path_returns_false(self, tmp_path):
|
||||
# Copilot round-5 finding: passing an absolute path silently returned
|
||||
# False because the stored keys are relative POSIX strings. Now the
|
||||
# call normalizes through ``_validate_rel_path`` which raises on
|
||||
# absolute inputs; we catch and return False so query semantics stay
|
||||
# exception-free.
|
||||
(tmp_path / "f.txt").write_text("x", encoding="utf-8")
|
||||
m = IntegrationManifest("test", tmp_path)
|
||||
m.record_existing("f.txt", recovered=True)
|
||||
import sys
|
||||
abs_input = "C:\\tmp\\f.txt" if sys.platform == "win32" else "/tmp/f.txt"
|
||||
assert m.is_recovered(abs_input) is False
|
||||
|
||||
def test_is_recovered_escaping_path_returns_false(self, tmp_path):
|
||||
# A relative path that resolves outside project_root cannot have been
|
||||
# recorded; ``_validate_rel_path`` raises and ``is_recovered`` returns
|
||||
# False rather than letting the ValueError propagate.
|
||||
m = IntegrationManifest("test", tmp_path)
|
||||
# Don't record anything — the path is impossible to record anyway.
|
||||
assert m.is_recovered("../escape.txt") is False
|
||||
|
||||
|
||||
class TestRecordExistingNewGuards:
|
||||
"""Coverage for the two new guards added by Copilot's 2026-05-18 review."""
|
||||
|
||||
Reference in New Issue
Block a user