From fc7dc25a0a01c0ffde3e6c3e9ce298eb4a5aa284 Mon Sep 17 00:00:00 2001 From: ayushcodes10 Date: Wed, 23 Sep 2026 12:23:32 +0530 Subject: [PATCH] Break manifest key collapse ties by seen time, not iteration order A manifest written across a mix of versions or call sites, some passing root to relativize keys and some not, an outdated installed skill runbook for one, can end up carrying both an absolute and a relative key for the same file, each with different data. Both load_manifest and the relativize step inside save_manifest already canonicalize every key through a comprehension, so the two forms do collapse into one entry, but a plain dict comprehension keeps whichever raw key happens to iterate last, an artifact of on disk JSON key order that has nothing to do with which entry is actually current. A fresher hash could be silently discarded in favor of a stale one purely because of how the keys happened to accumulate, letting the incremental change detector call a genuinely changed file unchanged. Reproduced directly: a manifest with a stale absolute entry and a fresh relative entry for the same file returns the stale one when it happens to be written second, and the fresh one when the order is reversed, confirming the collapse really is order dependent today. Both call sites now go through one small helper that breaks a collision by each entry's own seen timestamp when both sides have one and they disagree, since that field exists specifically to record when an entry was last confirmed current. A legacy or partial entry without that field falls back to the existing last one wins behavior unchanged. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_017qfdzgbA5KedGEjD1AayNh --- graphify/detect.py | 56 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 5 deletions(-) diff --git a/graphify/detect.py b/graphify/detect.py index 2c9eb01a4..7e03e419f 100644 --- a/graphify/detect.py +++ b/graphify/detect.py @@ -2166,6 +2166,45 @@ def _to_absolute_from_storage(key: str, root: Path) -> str: return _nfc(str(Path(root).resolve() / p)) +def _collapse_manifest_duplicates(items, key_fn) -> dict: + """Canonicalize each raw key via ``key_fn``, keeping the more recently + observed entry when two distinct raw keys collapse to the same + canonical one (#1964). + + A manifest written across a mix of graphify versions/call sites — some + passing ``root`` to relativize keys, some not (an outdated installed + skill runbook, for one) — can end up with both an absolute and a + relative key for the same file, each carrying different data. A plain + dict comprehension over such a manifest silently keeps whichever raw + key happens to iterate last, an artifact of on-disk JSON key order that + has nothing to do with which entry is actually current — so a fresher + hash could be discarded in favor of a stale one purely because of how + the keys happened to accumulate, letting detect_incremental() call a + genuinely changed file unchanged. + + Ties are broken by each entry's own ``seen`` timestamp (when present on + both sides and they disagree) rather than iteration order, since that + field exists specifically to record when an entry was last confirmed + current. Legacy scalar/partial entries (no ``seen``, or either side not + a dict) fall back to the historical last-wins behavior, unchanged. + """ + result: dict = {} + for k, v in items: + canonical = key_fn(k) + if canonical in result: + prev = result[canonical] + prev_seen = prev.get("seen") if isinstance(prev, dict) else None + new_seen = v.get("seen") if isinstance(v, dict) else None + if ( + isinstance(prev_seen, (int, float)) + and isinstance(new_seen, (int, float)) + and new_seen < prev_seen + ): + continue + result[canonical] = v + return result + + def load_manifest( manifest_path: str = _MANIFEST_PATH, *, @@ -2189,8 +2228,10 @@ def load_manifest( if not isinstance(raw, dict): return raw if root is None: - return {_nfc(k): v for k, v in raw.items()} - return {_nfc(_to_absolute_from_storage(k, root)): v for k, v in raw.items()} + return _collapse_manifest_duplicates(raw.items(), _nfc) + return _collapse_manifest_duplicates( + raw.items(), lambda k: _nfc(_to_absolute_from_storage(k, root)) + ) def save_manifest( @@ -2397,10 +2438,15 @@ def save_manifest( # their absolute form so the manifest round-trips on the saving # machine even when not every entry can be portably encoded. # NFC after relativize so on-disk keys match what load_manifest - # re-anchors and compares against (#2221). - manifest = {_nfc(_to_relative_for_storage(k, root)): v for k, v in manifest.items()} + # re-anchors and compares against (#2221). A seeded absolute key and + # a seeded relative key for the same file collapse here too (#1964) + # -- _collapse_manifest_duplicates keeps whichever is more recently + # seen instead of whichever the dict comprehension iterates last. + manifest = _collapse_manifest_duplicates( + manifest.items(), lambda k: _nfc(_to_relative_for_storage(k, root)) + ) else: - manifest = {_nfc(k): v for k, v in manifest.items()} + manifest = _collapse_manifest_duplicates(manifest.items(), _nfc) # Avoid rewriting manifest.json when the serialized payload is identical (#2838). manifest_p = Path(manifest_path)