Repository navigation
Conversation
, 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
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
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).
Blockers
I could not run pytest locally because the sandbox would not allow it. Verdict: block |
There was a problem hiding this comment.
💡 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) |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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
| for spec in specs: | ||
| if spec.name == _SQLITE_STORE: | ||
| _stage_sqlite(root, staging, spec.path) | ||
| continue | ||
| records = scan_resource(root, spec.path, spec.kind) |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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
|
Review 1 of 3: PR #971 ( 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.
The gate also says Fix:
B2. Security: the backup copy of personal data is world-readable (ADR-1112).
All of these take the process umask. With the usual umask 022, the staged Fix:
Notes, no action before merge
Checked and fine:
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 |
There was a problem hiding this comment.
💡 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".
| 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]) |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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 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).
Blockers
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 |
|
Review 2 of 3: PR #971 ( I first checked the two Blockers from review 1, then reviewed only the fixup Earlier Blockers
Fixup The new copy order is correct. The drainer commits a payload to the store and only then deletes its spool file ( Notes, no action before merge
Verdict: block |
|
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).
Accepted: the whole PR, every file and all three commits. There are three Notes, no action before merge:
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 |
There was a problem hiding this comment.
💡 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".
| if not staging.exists(): | ||
| staging.mkdir(mode=PRIVATE_DIR_MODE) | ||
| staging.chmod(PRIVATE_DIR_MODE) # an empty directory given by the caller |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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
| create_private_file(target) | ||
| with closing(sqlite_connect(str(target))) as dst: | ||
| src.backup(dst, pages=-1) |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
💡 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) |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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
|
Review 3 of 3: PR #971 ( Earlier Blockers: both are fixed in
New Blockers in Notes, no action before merge
Accepted: the whole PR, every file and all three commits. No file or commit carries an open Blocker. CI: when I posted this, the 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
|
CI autopilot :
Generated by Claude Code |
…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]>
|
Continued in #979. Generated by Claude Code |
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). Copyingmemory.dbalone misses every transaction committed only inmemory.db-wal. Copyingmemory.db,-waland-shmone 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 withsqlite3.Connection.backup(pages=-1), i.e. onesqlite3_backup_step(-1), which is one read transaction. The copy is then switched tojournal_mode=DELETE, because the backup carries the source's WAL header, so it ends up as one self-contained file. It is then checked withPRAGMA 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 ofcore/backup_manifest.pyinto the staging root, the SQLite store by snapshot, then returns the manifest computed on the staged copy. An empty directory resource stays present. Amanagedinstall is refused explicitly, because its cluster still needs a stopped copy or a dump. Apostgresqlinstall stages its files only.Type of change
Test plan
pytest tests_py/infrastructure/test_sqlite_snapshot.py tests_py/handlers/test_backup_snapshot.pyplus the existing backup tests (test_backup_manifest*,test_backup_scan): 93 passed.wal_autocheckpoint=0, writer still open) are in the copy. The copy is a single file indeletemode.ruff check/ruff format --checkon the changed files: clean.python scripts/check_craftsmanship.py: OK.Not verified locally:
scripts/mutation_check.sh) and the full suite:uv runneeds torch, and the environment's proxy refusesdownload-r2.pytorch.org.test_backup_scan.py.Audit notes
CORTEX_CLAUDE_DIRwith digest check;Coding-standards compliance
Reviewer checklist
Generated by Claude Code