Repository navigation
feat(backup): a versioned manifest digests every class A file of an install (#731, slice 2) - #960
Conversation
…nstall (#731, slice 2) The manifest names the class A resources of docs/audits/owned-resources-backup-2026-11-05.md by backend and gives a SHA-256 digest and a size per file. A required store that is absent, a symbolic link or a special file is refused, never skipped. Refs #731 Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018kyZYt9sk9gsvUQZCwMbuF
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cc4524072
ℹ️ 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".
| target = root / relative | ||
| if not target.exists() and not target.is_symlink(): | ||
| return None | ||
| is_dir = stat.S_ISDIR(_checked_mode(target)) |
There was a problem hiding this comment.
When methodology or .capture-worker is a symlink, checking only the final resource with lstat() follows that ancestor link without detecting it. For example, with root/methodology pointing to an external directory containing memory.db, backup_manifest(root, "sqlite") successfully hashes the external database and labels it methodology/memory.db. This violates the scanner's explicit refusal to follow links outside the root; a dangling ancestor can also silently turn optional resources into absent entries. Check each path component below the supplied root before reading the resource.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in the next commit (fixup of 5cc4524). scan_resource now walks every directory between the root and the resource (_check_ancestors). A symbolic link there is refused, dangling or not, and so is a file standing where a directory is expected. The walk stops at the first absent directory, which makes the resource absent. Tests: test_a_symlinked_ancestor_is_refused (methodology -> outside/ holding a memory.db), test_a_dangling_symlinked_ancestor_is_refused_not_absent, test_a_deep_ancestor_that_is_a_file_is_refused, plus the two absent-ancestor cases.
Generated by Claude Code
| names = [entry.get("name") for entry in entries] | ||
| if names != [spec.name for spec in specs]: | ||
| raise ManifestError( | ||
| f"entries {names} do not match the {manifest['backend']} table" | ||
| ) | ||
| return manifest |
There was a problem hiding this comment.
A manifest with the expected entry names but different resource definitions passes this check unchanged. For example, changing the SQLite entry's path to /outside/other.db and its kind to dir is accepted by parse_manifest(), even though neither matches the versioned table. Entries containing only their names are accepted too. Consequently, the advertised rejection of another resource table does not work beyond checking names and order. Validate each entry's fixed metadata against its corresponding ResourceSpec before returning it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in the next commit (fixup of 5cc4524). After the name check, parse_manifest rebuilds each entry from its spec and from its own file records, then requires an exact match. This refuses: a different path, kind or secret; an extra key; files on an absent entry; a required entry marked absent; unsorted files; a record outside its directory; and malformed present/files/record types. Tests: test_parse_refuses_an_entry_that_differs_from_its_spec_or_files (your /outside/other.db + kind: dir example among them), test_parse_refuses_malformed_presence_or_files, test_parse_refuses_unsorted_files, test_parse_refuses_a_required_entry_marked_absent.
Generated by Claude Code
…of an install (#731, slice 2)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d8e1b8fc5
ℹ️ 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 spec.kind == "dir" and not path.startswith(spec.path + "/"): | ||
| raise ManifestError(f"{path} is not inside {spec.path}") |
There was a problem hiding this comment.
When a directory entry contains a path such as methodology/wiki/../../outside.txt, the string-prefix check accepts it even though it resolves outside that resource. I verified that both build_manifest() and parse_manifest() accept this record unchanged with a valid digest and size; rebuilding the entry in _check_entry() does not catch it because it uses this same check. This violates the manifest's containment invariant and lets malformed manifests pass validation. Reject parent-traversal components and require canonical relative paths before checking resource membership.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in the next commit (fixup of 5cc4524). Before the containment check, every record path must now be a canonical relative POSIX path: no empty, . or .. component and no backslash (_is_canonical). Because this runs in _checked_files, build_manifest and parse_manifest both refuse methodology/wiki/../../outside.txt. Names that merely contain dots (..a.md, a..b/.c) stay valid. Tests: test_a_path_that_is_not_canonical_is_refused (7 cases), test_dots_inside_a_name_are_canonical and test_parse_refuses_a_parent_traversal_in_a_file_path.
Generated by Claude Code
…of an install (#731, slice 2)
|
Claude finished @cdeust's task in 2m 15s —— View job Review 1 of 3: PR #960
The review is posted here: #960 (comment) I found no Blockers. The three earlier Codex findings are all fixed. The review lists three Notes, none of which need action before merge:
I could not run the tests or git fetch here: running commands needed approval this job does not have. The Notes come from reading the code. Verdict: approve |
Review 1 of 3: PR #960 (commit
|
Co-Authored-By: Claude Fable 5.2 <[email protected]> Claude-Session: https://claude.ai/code/session_018kyZYsWnKSjpX4oNV19PCF Signed-off-by: Claude <[email protected]>
|
Claude finished @cdeust's task in 2m 4s —— View job Review 2 of 3: PR #960
There are no Blockers. What the merge changed for this PR:
Notes, no action before merge
I could not run pytest here. The checks come from git diffs and from reading the code. Verdict: approve |
Review 2 of 3: PR #960 (commit
|
Summary
Second slice of #731 (first remaining slice listed in
docs/audits/owned-resources-backup-2026-11-05.md§5): a versioned manifest naming the class A resources of an install, with a SHA-256 digest and a size for every file.Refs #731 (not
Closes: the consistent snapshot, the restore check, the refusal of an incompatible archive and the recovery documentation remain).Root cause of the gap. No code defined what a Cortex backup carries. Slice 1 (#892) inventoried the files in prose only. A snapshot or a restore check has nothing to work from until the set of entries and their digests exist as data.
What this adds
mcp_server/core/backup_manifest.py(pure): the resource table, taken from the audit's class A set by backend:sqlite:memory.db, required;managed: cluster and password, both required, the password markedsecret;postgresql: no store entry, since the server is the user's (class X);It also provides
build_manifest, a canonicalto_json, and aparse_manifestthat refuses another format, another version or another table.mcp_server/infrastructure/backup_scan.py: reads one resource under the root and digests each file (hashlib.file_digest). A symbolic link (dangling or not) and any special file are refused, not skipped: following a link would digest data outside the root, and skipping it would drop a file silently.mcp_server/handlers/backup_manifest.py: the composition rootbackup_manifest(root, backend). No MCP tool is registered; later slices call it.CORTEX_MEMORY_DB_PATHis therefore refused, not left out silently.-wal/-shmfiles are not entries. The manifest describes the tree it is given; producing a consistent copy is the next slice.Type of change
Test plan
tests_py/core/test_backup_manifest.py,tests_py/infrastructure/test_backup_scan.pyandtests_py/handlers/test_backup_manifest.py, 48 passed (pytest -qon the three files, Python 3.11.18, Linux).GITHUB_BASE_REF=main python scripts/ci_mutation_diff.py(mutmut 3.8.1):3 file(s), 0 failed, 0 untested. A first run left two survivors, both equivalent:ensure_ascii=Nonebehaves likeFalse, and"SHA256"is accepted like"sha256". I removed their cause (default ASCII JSON; thehashlib.sha256constructor instead of a name) rather than registering them.python scripts/check_craftsmanship.py: OK.python scripts/check_doc_claims.py: OK.tests_py/architecture+tests_py/scripts/test_craftsmanship_layer_table.py: 19 passed.ruff check/ruff format --checkon the changed files: clean.Not verified locally
uv.lock: torch cannot be downloaded in this environment, so I ran a minimal venv (pytest, pydantic, numpy, mutmut).AF_UNIX.Audit notes
fc13eb1:memory_config.py:51,profile_store.py:19-21,config.py:38-42,backend_marker.py:48,managed_pg.py:64-70andcapture_spool.py:60-62.VERSION = 1carries a# source:comment (first format of this slice, bumped on any change to the table or the fields), on the precedent ofmemory_portability.VERSION.Coding-standards compliance
coreimports stdlib only (noos/pathlib);infrastructureimports stdlib only; the handler composes both.Breaking changes
None. These are new modules; nothing existing calls them.
Reviewer checklist
docs/module-inventory.md, and audit §5 marks the slice delivered.Generated by Claude Code