[ssevera1]

What

Add validation and error handling to manifest.json read/write operations to catch malformed, missing, or inaccessible files early.

Why

Silent manifest corruption could lead to incomplete or incorrect rollback behavior, potentially leaving the system in an inconsistent state.

[claude]

Reviewed the full diff against vulntester/backup.py and its only caller (vulntester/__main__.py:175), plus the existing restore coverage in tests/test_privacy.py. The direction is right and the restore-loop restructuring is correct (no double counting — the inner except Exception handles restore failures, the outer except TypeError only covers the BackupEntry(**entry_data) construction). Two things should be fixed before merge.

1. backup.py:177 — entries is never validated, so malformed manifests still escape the new ManifestError contract.

Line 171 validates that the root is an object, but data.get("entries", []) is used unchecked:

• If entries is a number, for entry_data in ... raises a raw TypeError: int object is not iterable out of restore(), bypassing ManifestError entirely and crashing vulntester --rollback with a traceback.
• If entries is an object or a string, the result is worse than a crash: iteration yields dict keys / individual characters, each one fails the isinstance(entry_data, dict) check on line 178, and the function returns (0, N). A structurally broken manifest is then reported as "Rollback complete: 0 restored, N failed" — indistinguishable from a real partial rollback failure over N genuine entries.

Suggested fix, hoisting the lookup:

entries = data.get("entries", [])
if not isinstance(entries, list):
    raise ManifestError("Manifest entries must be a JSON array")

2. backup.py:152 — the write path still corrupts the manifest it is meant to protect.

The PR title is "prevent data corruption", but open(self._manifest_path, "w") truncates the existing manifest before anything is written. If json.dump fails partway (disk full, caught on line 154; serialization error, caught on line 156) the previously valid manifest is already destroyed, and every backup recorded before this call becomes un-rollbackable — not just the current entry. The new ManifestError reports the failure but does not prevent the loss, so the write half of this PR is detection only. Writing to a sibling temp file and then os.replace()-ing it over the manifest would actually deliver the stated intent.

Smaller points

• __main__.py:175: restore() now raises ManifestError and the only caller does not catch it, so a missing or unreadable manifest still exits with a stack trace rather than the clean early failure the description promises. Worth catching it there and returning a non-zero exit with the message.
• backup.py:79-81 and 143-145: these except ManifestError blocks are behaviourally dead — the except Exception immediately below returns the same value, so only the log wording differs. Both paths also leave the failed entry appended to self.manifest.entries (lines 75 and 139), so a later successful save silently persists an entry whose backup was reported as failed. Pre-existing, but squarely within this PR scope.
• backup.py:154 and 164: IOError is an alias of OSError on Python 3, so the tuple is redundant.
• No tests added. tests/test_privacy.py:589-622 already exercises restore(), so the new branches (unreadable file, malformed JSON, non-object root, non-object entry, missing fields) are cheap to cover and would have caught point 1.

[claude]

Verified both main points against the code and both held up; fixed them plus the uncaught-exception point:

Fixed

1. backup.py restore() — entries was used unchecked. Confirmed: an int entries raised a raw TypeError straight out of restore(), and a dict entries iterated its keys (each failing the isinstance(entry_data, dict) check) and silently reported (0, N) as if N real entries had failed. Added an explicit isinstance(entries, list) check that raises ManifestError.
2. backup.py _save_manifest() — confirmed open(self._manifest_path, "w") truncates before writing, so a mid-write OSError/serialization failure destroyed the previously valid manifest, not just the current entry. Now writes to a sibling .tmp file and os.replace()s it into place, so a failed write/serialize leaves the existing manifest untouched.
3. __main__.py — restore() can now raise ManifestError, and the only caller didn't catch it, so a bad --rollback manifest would exit with a stack trace. Added a catch that prints the error and returns 1.
4. Dropped the redundant IOError in the exception tuples — confirmed it's just an alias of OSError on Python 3, no behavior change.

Verified each new test fails on the pre-fix code and passes after the fix (checked by reverting backup.py alone and re-running).

Not changed

• The "behaviourally dead except ManifestError" point and the "failed entry still appended to self.manifest.entries" point: both true, but as the review itself noted, pre-existing and only tangential to this PR's manifest-corruption contract — the dead branch only affects log wording, not behavior. Left alone to keep this fix focused.

Checks

pytest -q: 157 passed (6 new regression tests covering non-list/non-dict entries, non-object root, malformed JSON, missing manifest, and write-failure corruption). No mypy step in this repo's CI.

[claude]

Re-reviewed the full diff; all three substantive items from my previous review are now fixed. backup.py:180-182 hoists and type-checks entries so a non-list no longer escapes as a raw TypeError or gets mis-counted as N genuine restore failures; _save_manifest (backup.py:151-163) now writes to a sibling manifest.tmp and os.replace-es it into place, so a mid-write OSError/serialization failure leaves the previous manifest intact (same directory, so the replace is atomic); and __main__.py:175-179 catches ManifestError and exits 1 with a message instead of a traceback. I also checked that the restructured restore loop does not double-count — the inner except Exception covers restore failures and the outer except TypeError only covers BackupEntry(**entry_data) construction — that restore() is the only public entry point affected and has no other callers, and that the five new tests plus the write-failure test match the branches they claim to cover. Scope is limited to backup.py, its one caller, and tests, with no unrelated changes.

Two non-blocking notes for a follow-up: a manifest with invalid UTF-8 raises UnicodeDecodeError (a ValueError, not an OSError) and still bypasses the ManifestError contract at backup.py:167-175; and the except ManifestError branches at backup.py:80 and 144 remain behaviourally identical to the except Exception below them, with the failed entry still left in self.manifest.entries so a later successful save persists it.