Skip to content

feat(backup): a live SQLite store is staged as one consistent copy (#731, slice 3) - #971

Closed
cdeust wants to merge 5 commits into
mainfrom
fix/731-sqlite-snapshot
Closed

cdeust wants to merge 5 commits into
mainfrom
fix/731-sqlite-snapshot

Conversation

@cdeust

@cdeust cdeust commented Nov 10, 2026

Copy link
Copy Markdown
Owner

Summary

Refs #731 (slice 3, SQLite part; the issue stays open).

Root cause. A Cortex SQLite store runs in WAL mode (sqlite_connection_registry.py). Copying memory.db alone misses every transaction committed only in memory.db-wal. Copying memory.db, -wal and -shm one after the other does not capture one instant while the server writes. The manifest from #960 digested whatever tree it was given, so it could digest a torn store.

Change.

  • infrastructure/sqlite_snapshot.py: opens the source read-only (a missing source is refused, not created) and copies it with sqlite3.Connection.backup(pages=-1), i.e. one sqlite3_backup_step(-1), which is one read transaction. The copy is then switched to journal_mode=DELETE, because the backup carries the source's WAL header, so it ends up as one self-contained file. It is then checked with PRAGMA integrity_check. An existing target is never overwritten. A linked source is refused.
  • infrastructure/backup_stage.py: the staging root must be empty and outside the Cortex root. Files are copied by their relative POSIX paths.
  • handlers/backup_snapshot.py: copies every class A resource of core/backup_manifest.py into the staging root, the SQLite store by snapshot, then returns the manifest computed on the staged copy. An empty directory resource stays present. A managed install is refused explicitly, because its cluster still needs a stopped copy or a dump. A postgresql install stages its files only.

Type of change

  • New feature (non-breaking change which adds functionality)

Test plan

  • pytest tests_py/infrastructure/test_sqlite_snapshot.py tests_py/handlers/test_backup_snapshot.py plus the existing backup tests (test_backup_manifest*, test_backup_scan): 93 passed.
    • Rows committed only in the WAL (wal_autocheckpoint=0, writer still open) are in the copy. The copy is a single file in delete mode.
    • 20 snapshots taken while a thread commits two-row transactions in a loop: every count is even, so no transaction is torn.
    • Refused: missing source (and not created), linked source, existing target (left intact), corrupt source, non-empty staging, staging inside the root, managed backend (no staging created).
  • ruff check / ruff format --check on the changed files: clean. python scripts/check_craftsmanship.py: OK.

Not verified locally:

  • The diff mutation gate (scripts/mutation_check.sh) and the full suite: uv run needs torch, and the environment's proxy refuses download-r2.pytorch.org.
  • Windows. The symlink test is skipped there, as in test_backup_scan.py.
  • CI is the source of truth for all of these.

Audit notes

Coding-standards compliance

  • Layers: handler is the composition root. The infrastructure modules import stdlib only.
  • Files < 300 lines, functions < 40 lines.
  • No new numeric constant.

Reviewer checklist

  • CHANGELOG.md updated (Added).
  • docs/module-inventory.md and the audit §5 updated.
  • No secrets / credentials / PII in the diff.

Generated by Claude Code

, slice 3)

A file copy of memory.db misses what is committed only in the -wal file, and
copying memory.db, -wal and -shm one after the other is not one instant. The
store is now copied by SQLite's online backup API in a single step, sealed to
journal_mode=DELETE and integrity_check'ed; every other class A file is
copied into a fresh staging root outside the Cortex root, and the manifest is
computed on that staged copy. A managed PostgreSQL install is refused until its
cluster can be copied stopped or dumped.

Refs #731

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_018kyZYsLE4ZAVorxvEdFaQv
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-10T11:10:32.170934Z ffaa26d New commits
🔒 Security Review ✅ Completed 2026-11-10T11:14:24.747489Z ffaa26d New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Nov 10, 2026 •

Copy link
Copy Markdown

Claude finished @cdeust's task in 5m 39s —— View job


Review 1 of 3 is done. I found two Blockers. The full review is at #971 (comment).

  • Read the diff (3 modules, 2 test files)
  • Run five reviews (quality, docs, performance, security, tests)
  • Check CI and layer rules (the Pipeline run was still in progress when I posted)
  • Post findings and verdict

Blockers

  • B1. CI fails. test_storage_redaction_gate.py:144 fails on the two sqlite3.connect calls in sqlite_snapshot.py and on the shutil.copyfile call in backup_stage.py. None of them is in _AUDITED, and neither connection goes through db_connect.
  • B2. Security. The staging tree and the copied store are created with the umask (0644/0755), not the owner-only 0600/0700 that ADR-1112 requires.

I could not run pytest locally because the sandbox would not allow it.

Verdict: block

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 019d13c0ec

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

raise StagingError(f"{staging} is not a directory")
if staging.exists() and any(staging.iterdir()):
raise StagingError(f"{staging} is not empty")
staging.mkdir(parents=True, exist_ok=True)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restrict access to the staging directory before copying data

On POSIX systems with a typical 022 umask, staging under a traversable shared parent creates a 0755 directory, while copyfile and SQLite create backup files with mode 0644. This exposes previously private data to other local users: for example, capture_spool.py explicitly protects captured data with 0700 directories and 0600 files, but this backup removes those protections. A private 0700 source root and 0600 SQLite database likewise become a readable staged copy. Create the staging root with owner-only permissions before writing any data, and enforce equivalent protection when accepting an existing empty directory.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmé : sous umask 022 le staging était créé en 0755 et recopiait le spool 0700/0600 lisible par les autres utilisateurs. Corrigé dans 449f183. prepare_staging crée le répertoire en 0o700 et applique chmod aussi à un répertoire vide réutilisé. Test POSIX : test_the_staging_root_is_owner_only.


Generated by Claude Code

Comment thread mcp_server/handlers/backup_snapshot.py Outdated
Comment on lines +50 to +54
for spec in specs:
if spec.name == _SQLITE_STORE:
_stage_sqlite(root, staging, spec.path)
continue
records = scan_resource(root, spec.path, spec.kind)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Coordinate the capture spool with the SQLite snapshot

When a capture drainer is running, a payload queued before the backup can disappear from both backed-up resources: SQLite is snapshotted first, then capture_drain.py:65-72 can store the payload and delete its spool file before this loop reaches capture_spool. The resulting backup successfully returns a valid manifest but contains neither the queued payload nor its stored memory. This is silent loss of class A data, rather than merely differing wiki versions. Coordinate the spool copy and database snapshot with the drainer, for example by holding its drain lock across both operations, so a pending capture cannot fall between them.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmé : drain_pending enregistre puis supprime le fichier du spool. Copié après l'instantané SQLite, un payload en attente pouvait donc n'être dans aucune des deux copies. Corrigé dans 449f183 par l'ordre plutôt que par le verrou : toutes les ressources fichiers, spool compris, sont copiées avant l'instantané SQLite. Un payload drainé entre-temps est alors dans le spool copié, et peut-être aussi dans le store ; ce rejeu est absorbé par le gate d'écriture (livraison at-least-once). Le verrou du drainer est non bloquant, il aurait fallu attendre ou échouer. Test : test_the_spool_is_staged_before_the_store.


Generated by Claude Code

@claude

claude Bot commented Nov 10, 2026

Copy link
Copy Markdown

Review 1 of 3: PR #971 (019d13c)

I ran five reviews: quality, security, tests, docs and performance. Two Blockers came out of them, and I checked both against the code myself.

Blockers

B1. CI fails: the storage redaction gate (ADR-1110) does not know the three new storage calls.
tests_py/architecture/test_storage_redaction_gate.py:144 (test_every_storage_primitive_is_an_audited_site) walks all of mcp_server/. It fails on any storage call that is missing from _AUDITED. This PR adds three:

  • mcp_server/infrastructure/sqlite_snapshot.py:35, sqlite3.connect(...) (db-connect)
  • mcp_server/infrastructure/sqlite_snapshot.py:56, sqlite3.connect(target) (db-connect)
  • mcp_server/infrastructure/backup_stage.py:44, shutil.copyfile (shutil-copy)

The gate also says infrastructure/db_connect.py is the only place allowed to build a connection.

Fix:

  • Open both connections with db_connect.sqlite_connect(..., uri=True). GatedConnection subclasses sqlite3.Connection, so .backup() still works.
  • Add an _AUDITED entry with a reason for the shutil-copy site.

B2. Security: the backup copy of personal data is world-readable (ADR-1112).
shared/private_paths.py:6-10 requires every file of personal data to be 0600 and every directory 0700. handlers/memory_portability.py:60 already exports with 0o600. This PR does not:

  • prepare_staging creates the staging directory with staging.mkdir (backup_stage.py:35).
  • copy_files creates subdirectories with target.parent.mkdir and copies with shutil.copyfile (backup_stage.py:43-44).
  • sqlite_snapshot.py:55-56 creates the database copy with mkdir and sqlite3.connect(target).

All of these take the process umask. With the usual umask 022, the staged memory.db, wiki, profiles, session log and capture spool come out 0644, and the directories 0755. Any local user can then read the full memory store, even though the sources are 0600/0700.

Fix:

  • Create the staging tree 0700: os.mkdir(..., 0o700), without exist_ok on the root.
  • Create each copied file with O_CREAT|O_EXCL|O_NOFOLLOW and mode 0o600.
  • Pre-create the SQLite target the same way before connecting to it.
  • Add a test that checks the modes.

Notes, no action before merge

  • A symlinked methodology/ is followed for the store: _stage_sqlite skips _check_ancestors. The store is copied into staging before the next resource's scan refuses the run, and nothing is cleaned up on failure (backup_snapshot.py:32-36, :49).
  • The quote attributed to sqlite.org/backup.html in sqlite_snapshot.py:16-18 is not on that page. What the page says is that step(-1) "copies the entire source database in one call" under one read lock. That supports the claim, but the wording should be the page's own.
  • scan_resource hashes every source file only for its paths, and the hashes are thrown away (backup_snapshot.py:54). That is one wasted full read per file.

Checked and fine:

  • A handler importing a handler is allowed as a sibling import (scripts/craftsmanship_imports.py:71-72).
  • as_uri() percent-encodes ? and #, so a path cannot inject URI parameters.
  • backup(pages=-1) captures rows committed only in the WAL.

When I posted this, the Pipeline CI run was still in progress. I could not run pytest locally because the sandbox would not allow it.

Verdict: block

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 449f1831ef

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread mcp_server/handlers/backup_snapshot.py Outdated
return
if spec.kind == "dir": # an empty directory is still present
(staging / spec.path).mkdir(parents=True, exist_ok=True)
copy_files(root, staging, [record[0] for record in records])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle spool files disappearing between scan and copy

When a capture drainer is running, it can commit a payload and unlink its spool file (capture_drain.py:65-72) after scan_resource returns but before copy_files opens that file. This raises an uncaught FileNotFoundError, aborting the backup before SQLite is staged and leaving a nonempty staging directory that subsequent attempts reject. The new spool-before-SQLite ordering fixes the previously reported silent omission, but a deterministic reproduction that drains immediately after the spool scan confirms this separate failure remains. Coordinate spool staging with the drain lock, or explicitly handle files consumed during the scan/copy interval.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmé : un fichier drainé entre scan_resource et la copie levait FileNotFoundError. Corrigé dans be865d5 : copy_files(..., consumed_ok=True), uniquement pour capture_spool, laisse de côté un fichier disparu après son listage. Ce fichier a soit été stocké, et il se retrouve alors dans l'instantané SQLite pris ensuite, soit été renommé *.rejected. Pour toute autre ressource, la disparition reste une erreur. Tests : test_a_spool_file_drained_after_its_listing_is_left_out et test_any_other_file_vanishing_after_its_listing_fails_the_backup.


Generated by Claude Code

@claude

claude Bot commented Nov 10, 2026 •

Copy link
Copy Markdown

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


Review 2 of 3 is done. One Blocker is still open. The full review is at #971 (comment).

  • Read the commit pushed since 019d13c (449f183)
  • Check B1 and B2 from review 1
  • Run five foreground reviews on 449f183 (quality, security, tests, docs, performance)
  • Check CI: the Pipeline run was still in progress when I posted
  • Post findings and verdict

Blockers

  • B1 is still open. test_storage_redaction_gate.py:144 will fail:
    • The two sqlite3.connect calls in sqlite_snapshot.py:35,56 are not in _AUDITED, and neither goes through db_connect.
    • shutil.copyfile in backup_stage.py:52 is not in _AUDITED either.
  • B2 is fixed. The staging root is now 0o700.

The new copy order (spool first, then the store) is correct, and its test would fail if the order were reverted. I posted 3 Notes, none needed before merge. I could not run pytest because the sandbox needs approval for it, so I checked B1 by reading the code.

Verdict: block

@claude

claude Bot commented Nov 10, 2026

Copy link
Copy Markdown

Review 2 of 3: PR #971 (449f183)

I first checked the two Blockers from review 1, then reviewed only the fixup 449f183. I ran five reviews (quality, security, tests, docs, performance) and checked each finding myself against the code.

Earlier Blockers

  • B1 is still open: CI fails on the storage redaction gate (ADR-1110). The fixup does not change sqlite_snapshot.py, and it does not add anything to _AUDITED. tests_py/architecture/test_storage_redaction_gate.py:144 will still report two sites that are not audited:
    • ("infrastructure/sqlite_snapshot.py", "db-connect"), count 2, at sqlite_snapshot.py:35 and :56. The scanner reads import sqlite3 as sqlite3.connect (_storage_primitives.py:45, :180). These connections also do not come from infrastructure/db_connect.py, which the gate requires.
    • ("infrastructure/backup_stage.py", "shutil-copy"), count 1, at backup_stage.py:52 (_storage_primitives.py:61, :192).
    • Fix: open both connections through db_connect.sqlite_connect(..., uri=True). Add an _AUDITED entry, with a reason, for the shutil.copyfile site.
    • The Pipeline run on 449f183 was still running when I posted this. I could not run pytest because the sandbox needs approval for it, so I traced this by reading the code.
  • B2 is fixed. prepare_staging now creates the staging root 0o700, and the chmod also tightens a reused empty directory (backup_stage.py:42-43). Since no other user can enter the root, they can no longer reach the staged files. test_the_staging_root_is_owner_only covers this and would fail if the change were reverted.

Fixup 449f183

The new copy order is correct. The drainer commits a payload to the store and only then deletes its spool file (hooks/capture_drain.py:65-72). Because the spool is now copied before the store, a pending payload ends up in at least one of the two copies. test_the_spool_is_staged_before_the_store patches the right name and would fail if the order were reverted. No new Blocker.

Notes, no action before merge

  • A spool file that is drained or rejected between scan_resource and shutil.copyfile raises FileNotFoundError and aborts the backup (backup_snapshot.py:50-56, backup_stage.py:52). Nothing is lost, but backing up a root while capture is running can fail.
  • Two citations in the fixup are wrong. backup_stage.py:16,25 cite ADR-0488, but capture_spool.py:34 sources PRIVATE_DIR from ADR-1084. backup_snapshot.py:13 says the write gate absorbs a replay, but it is find_exact, and only when the first run inserted an exact row. A merged payload is not absorbed.
  • Files and subdirectories inside staging are still created with the umask (0644/0755), and archiving or restoring keeps those modes. Give them 0600/0700 before the slice-4 restore writes them back into the private root.

Verdict: block

@claude

claude Bot commented Nov 10, 2026 •

Copy link
Copy Markdown

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


Review 3 of 3 is done. There are no Blockers. The full review is at #971 (comment).

  • Check that the earlier Blockers B1 and B2 are fixed (both are fixed in 760b826)
  • Review 760b826, the commit pushed after 449f183
  • Run the five reviews (quality, security, tests, docs, performance)
  • Check CI: the PR #971 run passed, Pipeline was still running when I posted
  • Post the verdict

Accepted: the whole PR, every file and all three commits. There are three Notes, no action before merge:

  • A spool file the drainer deletes between scan and copy aborts the backup with FileNotFoundError.
  • The "whatever the umask" comments overstate. The modes can only come out narrower, never wider.
  • create_private_file opens without O_EXCL, and copy_files opens each source without O_NOFOLLOW.

I could not run pytest locally because the sandbox would not allow it. I checked the redaction gate counts by reading the scanner's rules, not by running the test.

Verdict: approve

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 760b826914

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +55 to +57
if not staging.exists():
staging.mkdir(mode=PRIVATE_DIR_MODE)
staging.chmod(PRIVATE_DIR_MODE) # an empty directory given by the caller

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restrict the staging ACL on Windows before copying data

On Windows, accepting an existing empty staging directory under a shared parent leaves its inherited read permissions intact: chmod(0o700) does not establish an owner-only Windows ACL. Newly created directories also lack this protection on supported older Python versions. Consequently, copying a protected Cortex store into that directory can expose its contents to other local users. Unlike the previously reported POSIX issue, the current code still omits the Windows protection implemented by shared/private_paths.py::restrict_windows_acl; apply that protection to both new and caller-provided staging roots before writing any backup data.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmé, mais ce point n'est pas traité dans #971. Cette PR a épuisé son budget de trois revues (ADR-1119), et la revue 3 a accepté le contenu tel quel. Il passera dans une PR de suivi de #731, avec les autres points restants : coordination de la copie du spool et de l'instantané SQLite avec le verrou du drainer, ACL Windows du staging (restrict_windows_acl), réservation O_EXCL de la cible SQLite.


Generated by Claude Code

Comment on lines +63 to +65
create_private_file(target)
with closing(sqlite_connect(str(target))) as dst:
src.backup(dst, pages=-1)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reserve the SQLite snapshot target exclusively

When two snapshot calls target the same initially absent path, one can finish after the other's existence check but before its create_private_file call. That helper uses O_CREAT without O_EXCL, so the delayed caller opens the completed backup and src.backup overwrites it instead of refusing the existing target. Interleaving a second completed snapshot at _open_source reproduces this overwrite. Atomically reserve the destination with exclusive creation before opening it through SQLite, as copy_files already does for other staged files.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmé, mais ce point n'est pas traité dans #971. Cette PR a épuisé son budget de trois revues (ADR-1119), et la revue 3 a accepté le contenu tel quel. Il passera dans une PR de suivi de #731, avec les autres points restants : coordination de la copie du spool et de l'instantané SQLite avec le verrou du drainer, ACL Windows du staging (restrict_windows_acl), réservation O_EXCL de la cible SQLite.


Generated by Claude Code

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: be865d5ef8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".



def _stage_files(root: Path, staging: Path, spec: ResourceSpec) -> None:
records = scan_resource(root, spec.path, spec.kind)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle consumed spool files during the initial scan

With a capture drainer running, a spool file can disappear during scan_resource, before the consumed_ok handling is reached. That scan lists children and then calls _checked_mode and _record, both of which can raise FileNotFoundError after capture_drain.py deletes a stored payload. This aborts the backup before the SQLite snapshot and can leave a nonempty staging tree that cannot be reused. Unlike the previously reported scan-to-copy race, fresh reproduction deleting a file immediately before _record opens it confirms that the initial scan still fails. Make the spool scan tolerate individually consumed files, or coordinate the scan and copy with the drainer; catching the exception around the entire scan would incorrectly discard other pending payloads.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmé, mais ce point n'est pas traité dans #971. Cette PR a épuisé son budget de trois revues (ADR-1119), et la revue 3 a accepté le contenu tel quel. Il passera dans une PR de suivi de #731, avec les autres points restants : coordination de la copie du spool et de l'instantané SQLite avec le verrou du drainer, ACL Windows du staging (restrict_windows_acl), réservation O_EXCL de la cible SQLite.


Generated by Claude Code

@claude

claude Bot commented Nov 10, 2026

Copy link
Copy Markdown

Review 3 of 3: PR #971 (760b826)

Earlier Blockers: both are fixed in 760b826.

  • B1 (redaction gate, ADR-1110): fixed.
    • Both connections in sqlite_snapshot.py now come from db_connect.sqlite_connect; no raw sqlite3.connect is left.
    • backup_stage.py has three storage calls: os.open at :76, os.fdopen(.., "wb") at :76 and shutil.copyfileobj at :77.
    • Each one has an _AUDITED entry with count 1 (test_storage_redaction_gate.py:98-110).
    • I checked these against the scanner rules in _storage_primitives.py, and the counts match exactly.
    • GatedConnection lets PRAGMA statements through untouched, and backup() accepts it as the target.
  • B2 (owner-only modes, ADR-1112): fixed.
    • The staging root is set to 0o700 with chmod.
    • Each new directory level is created 0o700 (private_dirs).
    • Each copied file is created 0o600 with O_CREAT|O_EXCL|O_NOFOLLOW.
    • The SQLite target is pre-created 0600 by create_private_file. SQLite gives its -journal/-wal/-shm files the same mode as the database.
    • A test checks the modes under umask 022.
  • Note from review 1, linked methodology/: also fixed. _stage_sqlite now goes through scan_resource, and a new test covers the case.

New Blockers in 760b826: none.

Notes, no action before merge

  • A spool file the drainer deletes between scan_resource and copy_files raises FileNotFoundError (backup_stage.py:75). The backup then aborts and leaves a non-empty staging root, so a retry with the same staging root is refused (the Codex finding).
  • The comments "0o600/0o700 whatever the umask" (backup_stage.py:8-10, sqlite_snapshot.py:14-15) overstate. Only the staging root is set with chmod; the other modes are still reduced by the umask. That can only make them narrower, never wider.
  • create_private_file opens without O_EXCL (private_paths.py:122), so "never overwrites" rests on the earlier exists() check alone. copy_files opens each source without O_NOFOLLOW (backup_stage.py:75). Both are races inside the owner-only 0700 trees.

Accepted: the whole PR, every file and all three commits. No file or commit carries an open Blocker.

CI: when I posted this, the PR #971 run had passed and Pipeline was still running. I could not run pytest locally because the sandbox would not allow it. The CI result above comes from reading the gate scanner's rules, not from running the test.

Verdict: approve

…copy (#731, slice 3)

fix(backup): kill the 26 surviving mutants of the unit check's mutation gate

Pipeline run 42801544562, job unit: the diff mutation gate kept 26
survivors in sqlite_snapshot.py, backup_stage.py and backup_snapshot.py.
The messages, the read-only URI, the staging refusals and created_at are
now pinned by tests; pages=-1 (Python's default) and the default of
consumed_ok are gone, so no mutant can alter them. Local mutmut 3.8.1:
0 survivors in all three files.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_018kyZYsLE4ZAVorxvEdFaQv

cdeust commented Nov 10, 2026

Copy link
Copy Markdown
Owner Author

CI autopilot : unit (et donc merge) rouge sur be865d5, run 42801544562.

  • Cause : le gate de mutation du diff laissait 26 mutants survivants dans sqlite_snapshot.py, backup_stage.py et backup_snapshot.py (« GENUINE surviving mutant(s) »). Les tests ne fixaient ni les messages d'erreur, ni l'URI en lecture seule, ni les refus du staging, ni created_at.
  • Correctif ffaa26d :
    • Des tests fixent désormais ces comportements.
    • pages=-1, qui est la valeur par défaut de Python, et la valeur par défaut de consumed_ok sont supprimés, ce qui élimine les mutants qui ne pouvaient rien changer.
    • _seal ne prend plus de chemin et rapporte toutes les erreurs de integrity_check.
    • Aucun équivalent n'a été enregistré dans le registre.
  • Vérifié localement : mutmut 3.8.1, 0 survivant sur les trois fichiers (42, 57 et 82 mutants tués) ; 114 tests réussis ; ruff et le gate de craftsmanship OK.
  • Non vérifié : la suite complète et Windows (torch inaccessible ici). La CI fera foi.

Generated by Claude Code

github-actions Bot pushed a commit that referenced this pull request Nov 10, 2026
…973)

* docs(agents): state the CI contract every agent and routine follows

The scheduled routines were written before the review contract: on
2026-11-10 they measured branches against the main branch and rebased
them with a forced push, commented on approved pull requests (#968) and
opened pull requests while another was in flight (#971, #972). Their
prompt tells them to read CLAUDE.md and docs/ and to apply the
repository's standards, so the rules "Review rounds" does not hold yet
are written next to it, and both are said to win over a less
restrictive routine prompt.

Co-Authored-By: Claude Fable 5.2 <[email protected]>
Claude-Session: https://claude.ai/code/session_018kyZYsWnKSjpX4oNV19PCF
Signed-off-by: Claude <[email protected]>

* docs(agents): rule 1 names the two merges the verdict does not decide

Notes of review 1 of #973, taken in one push because that review's
verdict cannot be read by the resolver (its comment does not link the
run, fixed for later reviews by ADR-1130, #975): the low-risk Dependabot
merge and the "do not merge" title, and what a pull request cut down
after its third review waits for.

Co-Authored-By: Claude Fable 5.2 <[email protected]>
Claude-Session: https://claude.ai/code/session_018kyZYsWnKSjpX4oNV19PCF
Signed-off-by: Claude <[email protected]>

---------

Signed-off-by: Claude <[email protected]>
Co-authored-by: Claude <[email protected]>

cdeust commented Nov 10, 2026

Copy link
Copy Markdown
Owner Author

Continued in #979.


Generated by Claude Code

@cdeust cdeust closed this Nov 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants