From 25051ff38feb56b51d0b650a304ac0c6acecb33f Mon Sep 17 00:00:00 2001
From: eldar702 <eldarshlomi7@gmail.com>
Date: Thu, 21 May 2026 17:54:48 +0300
Subject: [PATCH] fix(manifest): normalize is_recovered input through
 _validate_rel_path
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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).
---
 src/specify_cli/integrations/manifest.py | 17 ++++++++++++++---
 tests/integrations/test_manifest.py      | 21 +++++++++++++++++++++
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/src/specify_cli/integrations/manifest.py b/src/specify_cli/integrations/manifest.py
index 7c2b75095..e6be895e8 100644
--- a/src/specify_cli/integrations/manifest.py
+++ b/src/specify_cli/integrations/manifest.py
@@ -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]:
diff --git a/tests/integrations/test_manifest.py b/tests/integrations/test_manifest.py
index e4d7c5ce7..a469a2fa5 100644
--- a/tests/integrations/test_manifest.py
+++ b/tests/integrations/test_manifest.py
@@ -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."""