Fix two latent bugs: merge-chunks output and manifest data loss#1207
Conversation
1. graphify merge-chunks dumped the entire node list to the terminal instead of the node count. __main__.py concatenated merged['nodes'] (a list of dicts) into an f-string where it clearly meant len(merged['nodes']) -- the other two values in the same line use len() correctly. 2. global_graph._load_manifest silently returned a fresh empty manifest on any JSON parse error. That is reachable through normal interrupted writes (the manifest is rewritten in full on every global_add / global_remove, with no fsync or atomic rename), and the failure mode is total data loss: every tracked repo disappears from ~/.graphify/global-manifest.json on the next read. Back the corrupt file up to <path>.corrupt.<unix_ts> and print to stderr before returning the empty default. Users can then recover manually and the failure is visible rather than silent.
safishamsi
left a comment
There was a problem hiding this comment.
Thanks for tracking down both of these — the manifest data-loss bug in particular is nasty and I've confirmed the root cause independently.
#1 merge-chunks display bug ✓
Fix is correct — len(merged['nodes']) is the right call; interpolating the whole list was clearly unintentional. No concerns here.
#2 manifest corruption recovery ✓
The backup-on-corrupt approach is sound and the rename logic is correct. A few things before this lands:
Missing test coverage. tests/test_global_graph.py has 16 tests but none cover the new corruption-recovery branch. Please add a test that:
- Writes a corrupt JSON string to the manifest path
- Calls
_load_manifest() - Asserts a
.corrupt.<timestamp>backup was created - Asserts the return value is the empty default
Without this, the next refactor of the error path has no safety net.
Follow-up worth filing (not blocking this PR): the real root cause is that _save_manifest does a non-atomic write_text — a crash mid-write produces a truncated file that triggers the corruption branch. The fix here makes corruption recoverable, but a follow-up that changes the write to temp-file + os.replace would prevent it entirely. Happy to review that separately.
merge-chunks has no test at all — also not blocking, but worth a note since this is the other half of the PR.
Please add the manifest recovery test and I'll approve.
safishamsi
left a comment
There was a problem hiding this comment.
Review: Needs test before merge
Both fixes are correct and the logic is clean — happy to approve once a test lands.
Bug A (merge-chunks): cosmetic, trivially correct
__main__.py:4523 interpolates merged['nodes'] (the full list) instead of len(merged['nodes']). Clearly a typo given the sibling len(merged['edges']) on the same line. Fix is correct.
Bug B (manifest data loss): correct and well-scoped
global_graph.py:15-21 silent except Exception: pass on parse failure returns an empty default, which callers then save — silently overwriting all other tracked repos. The rename-to-.corrupt.<ts> approach is the right call. All required symbols (datetime, timezone, sys) are already imported. Behavior on the happy path is byte-for-byte unchanged.
Missing test (the owner already flagged this)
The corruption-recovery branch is the most critical new path and has zero coverage. Please add to tests/test_global_graph.py:
def test_load_manifest_backs_up_corrupt_file(tmp_path, monkeypatch):
manifest_path = tmp_path / "global-manifest.json"
manifest_path.write_text("not valid json", encoding="utf-8")
monkeypatch.setattr("graphify.global_graph._GLOBAL_MANIFEST", manifest_path)
result = _load_manifest()
assert result == {"version": 1, "repos": {}}
assert not manifest_path.exists()
backups = list(tmp_path.glob("global-manifest.json.corrupt.*"))
assert len(backups) == 1Follow-up (non-blocking, file separately)
_save_manifest at :24-26 is a non-atomic write_text — the root cause of the corruption scenario. A tmp_path + os.replace atomic write would close the gap completely. This PR makes corruption recoverable; a follow-up should make it preventable.
Two small, independent bug fixes spotted during a review pass. Both have the same shape: a defensive coding mistake whose worst case is reachable through normal use.
1.
graphify merge-chunksdumps the full node list to the terminal__main__.py:4523concatenatesmerged['nodes'](a list of dicts) into an f-string where it clearly intends the node count -- the other two values in the same line uselen()correctly:Fix:
len(merged['nodes']).2.
global_graph._load_manifestsilently wipes~/.graphify/global-manifest.jsonon any parse errorThe manifest is rewritten in full on every
global_add/global_removewith no atomic rename or fsync, so a partial-write from an interrupted command or a disk-full event is enough to corrupt it. On the nextglobal *invocation the parse fails, the empty default is returned, and_save_manifestoverwrites the corrupt file with{}-- every tracked repo gone, silently.Fix: on parse failure, rename the corrupt file to
<path>.corrupt.<unix_ts>, print a message to stderr explaining what happened, and then fall through to the empty default. The user keeps a recoverable backup and the failure is visible.Test plan
graphify merge-chunksagainst any chunked output; confirm the summary line shows a count, not a JSON blob.~/.graphify/global-manifest.json(truncate to{or similar); confirm nextgraphify global listprints the backup notice, creates a.corrupt.<ts>file, and starts from an empty manifest.🤖 Generated with Claude Code