feat(backup): restore a staged backup into a fresh root, verified against its manifest (#731) - #983

Claude finished @cdeust's task in 2m 45s —— View job

Review of PR #983 (review 1 of 3)

• Gather context (diff, CLAUDE.md, /review-pr instructions)
• Read the diff myself
• Run the 5 /review-pr reviewers (code quality, performance, test coverage, documentation accuracy, security)
• Check their findings against the code
• Post the review (review comment)

Result: I found no Blockers. The review comment lists 3 Notes, none of which needs action before merge:

• The SQLite integrity check reads the backup, not the copy that becomes the install.
• The rename at the end can silently replace an empty directory created at the target during the restore.
• Two docstrings say more than the code does: the tree is said to carry "exactly the manifest's files", and backup_verify.py is credited with failures that other modules report.

I didn't run the tests or the linters; this review comes from reading the code.

Verdict: approve

Review 1 of 3: PR #983 (restore a staged backup, #731 slice 4)

I found no Blockers. Five reviewers looked at this PR (code quality, performance, test coverage, documentation accuracy, security), and I traced their findings in the code myself.

Checked and found safe

• Path traversal through the manifest: parse_manifest rebuilds each entry from the resource table. Every file path must be canonical and must sit under its resource's path, so copy_files and private_dirs only receive checked paths.
• Symlinks: a symlink in the backup tree is refused by the scan, and a manifest that is a symlink is refused by read_text_file.
• Writes: output files are opened O_EXCL|O_NOFOLLOW, so nothing is written through a link or over an existing file.
• The restored copy: it is verified against the manifest digests again before the rename, so content swapped during the copy is caught.
• Layers and limits: the layer rule, the file and method size limits and the # source: rule all hold.

Notes, no action before merge

• mcp_server/handlers/backup_restore.py:97 runs check_snapshot on the backup, not on the copy. If memory.db is swapped between the integrity check and the copy, the copy is never integrity-checked. Running it on partial / entry["path"] after line 105 closes that gap.
• backup_restore.py:106: on POSIX, partial.rename(target) silently replaces an empty directory created at target while the restore runs. The "never writes over an install" check happens only once, at line 98.
• The docstrings at backup_restore.py:10-12 and core/backup_verify.py:7-8 say the tree carries "exactly the manifest's files" and that core/backup_verify.py names each failure. In the code:
  • extra files are detected only inside the resource paths in the table;
  • symlinks are refused by backup_scan, and a missing store by build_manifest.

View job run
Verdict: approve

ci-autopilot — tentative 1

Diagnostic : le check unit (run 42990175181, image ubuntu-26.04 20261101.186.1) échoue sur tests_py/architecture/test_storage_redaction_gate.py::test_every_storage_primitive_is_an_audited_site ; merge n'échoue que parce que unit est rouge. La cause est dans la PR : le nouveau write_new_file de infrastructure/backup_stage.py ouvrait sa propre paire os.open/os.fdopen, soit deux sites d'écriture là où l'audit ADR-1110 en compte un. Au lieu d'augmenter le compteur de l'audit, les deux écrivains (copy_files et write_new_file) passent désormais par un seul _new_private_file, de sorte que le site audité reste unique et que la raison inscrite reste exacte.

Commit poussé : 58f0621 (fixup! de d975f92, absorbé au prochain rebase autosquash).

Vérifié localement : le test de gate échoue sur d975f92 (1 failed) et passe sur 58f0621 ; test_storage_redaction_gate, test_backup_stage, test_sqlite_snapshot, test_backup_verify, test_backup_restore, test_backup_restore_target : 57 passed ; ruff check, ruff format --check et scripts/check_craftsmanship.py OK.
Non vérifié : la suite complète (environnement sans torch ni PostgreSQL ; les tests PostgreSQL ont été sautés) ; la CI sur 58f0621 est en cours, aucun check vert n'est affirmé ici.

Generated by Claude Code

Claude finished @cdeust's task in 2m 3s —— View job

Review 2 of 3: PR #983

• Check earlier Blockers: review 1 had none.
• Review the only commit pushed after d975f92: 58f0621 (backup_stage.py, which now has one audited write site).
• Run the 5 /review-pr reviewers (code quality, performance, test coverage, documentation accuracy, security).
• Post the review (review comment).

Result: I found no Blockers. The refactor keeps the flags (O_EXCL|O_NOFOLLOW) and the 0600 mode, and it brings the ADR-1110 gate counts back to the audited 1/1/1. The review comment lists 3 Notes, none of which needs action before merge:

• The audit reason in the gate test doesn't mention the manifest write.
• A descriptor can leak if os.fdopen fails.
• Nothing tests a symlink at the write target directly.

I didn't run the tests or the linters, because pytest needs approval in this job. This review comes from reading the code.

Verdict: approve

Review 2 of 3: PR #983 (restore a staged backup, #731 slice 4)

Earlier Blockers: none. Review 1 approved d975f92 with Notes only.

Scope: only the fixup commit pushed after it, 58f0621 (mcp_server/infrastructure/backup_stage.py). Unchanged code was not re-audited. Five reviewers (code quality, performance, test coverage, documentation accuracy, security) looked at the commit, and I checked their findings against the code.

What the commit does: copy_files and write_new_file both open their file through _new_private_file now. Nothing else changes:

• The flags are the same: _CREATE keeps O_EXCL|O_NOFOLLOW and adds no O_TRUNC or O_APPEND.
• The mode is still 0600, and exceptions still pass through.
• In with source, _new_private_file(target), source is still closed if the open raises.

This brings the ADR-1110 gate's counts for this module back to the audited 1/1/1 (os.open-write, open-write, shutil-copy, tests_py/architecture/test_storage_redaction_gate.py:98-110). The method and file size limits still hold, and no new constant needs a # source:.

I found no Blockers.

Notes, no action before merge

• The audit reason at tests_py/architecture/test_storage_redaction_gate.py:100-101 says the bytes written at this site are "a copy of class A files already written through the gate". Since this commit, the site also writes the manifest (handlers/backup_snapshot.py:96), which holds paths, digests and sizes but no memory content. Naming both writers there would keep the audit reason accurate.
• backup_stage.py:49: if os.fdopen raises, the descriptor from os.open is never closed, and the empty 0600 file stays behind. That needs an async exception in practice.
• Nothing tests that a symlink placed at the write target is refused, so a regression that drops O_EXCL from _CREATE would only be caught indirectly (tests_py/infrastructure/test_backup_stage.py).

I didn't run the tests or the linters: pytest needs approval in this job. This review comes from reading the code.

View job run
Verdict: approve