fix(dr): the full restore-verify opens the snapshot under the real store keyring (BACKLOG #1718) by wshallwshall · Pull Request #1126 · MEFORORG/MessageFoundry

wshallwshall · 2026-09-15T00:22:41Z

Closes BACKLOG #1718. _full_open_check in messagefoundry/pipeline/dr_backup.py opened the
restore-verify snapshot with a bare StoreSettings, which carries no key, so on a keyed store the
snapshot opened under the identity cipher. It passed a corrupted store and failed a good one holding a
state or reference row.

THIS PR CARRIES TWO AUTHORS' WORK, AND THE OLDER HALF HAD NEVER BEEN REVIEWED

The lower commit is 9c1efdaaf1a5bf477c1479d659fbd171037edac6, rebased onto current main as
dc2f5f8e2. It was written and pushed on 2026-09-14 by an agent session that has since died, on a
branch that had no pull request and cited no backlog number in its subject, so it was invisible
to both a PR listing and a citation grep. It was one commit ahead of main and fourteen behind.
git patch-id --stable on the original and on the rebased commit is f654977d684ad5b60b0d44eb8f66de0593d70382
for both, so the rebase preserved it byte-for-byte; there were no conflicts.

I am the first reviewer it has ever had. I read it at hunk granularity and judged it sound: it threads
the live StoreSettings into _verify_archive_blocking and on into _full_open_check
(model_copy substituting only path and backend), adds _decrypt_check reading the snapshot's
cipher-covered cells back through build_store_cipher under the same cell_aad binding, adds
VerifyResult.decrypted_cells, and amends ADR 0049. Whoever lands this should know they are getting
that work as well as mine.

Its commit message overclaims one thing, which I left standing rather than rewrite another author's
text: change 3 says it binds the store before the try in _full_open_check so an open that raises
surfaces its own cause. The store was already bound before the try in the shipped code — the leak is
inside MessageStore.open, one frame down, which is item step 4 and is my commit.

WHAT I ADDED ON TOP (`d34676f55`)

Step 4 of the item: a failed open closes its handle. MessageStore.open left its aiosqlite
connection open when a warm-up raised, so the verify's temp-directory cleanup was refused on Windows
and its PermissionError replaced the real cause. Two guards now close what they own and re-raise;
the close is itself wrapped so a failing cleanup cannot mask the open's error either.
Step 2's status split. A snapshot holding sealed cells that the settings resolve no key for was
FAIL; it is now KEY_MISMATCH, raised from both places the condition surfaces — the decrypt pass,
and the StoreKeylessError the store's eager state/reference warm-ups raise first. A failed AEAD
tag stays FAIL: CipherError cannot separate bit rot from a key that was never supplied (its own
docstring says so), and bit rot is the reading that must not be softened. ADR 0049 AC-13 amended,
AC-14 added for the close, and the docs/CONFIGURATION.md row for full_restore_verify corrected —
that row is the false promise the item quotes.
Step 3's missing case. No test held a state or reference row. Those are the rows
MessageStore.open decrypts eagerly, and so the ones that turned every scheduled backup of a keyed
store into a failing verify. Two new tests:
test_full_verify_passes_on_a_snapshot_holding_state_and_reference_rows and
test_full_verify_on_a_failed_open_reports_the_open_error_not_a_cleanup_error.
The crypto-inventory gate (ASVS 11.1.3). dr_backup.py gains a messagefoundry.store.crypto
import in the lower commit, and that trips the gate. Without the INVENTORY row this branch reds a
required leg. This is evidence the lower commit never ran through pre-commit or CI.

AND A SECOND RED THE INHERITED COMMIT LEFT BEHIND, CAUGHT BY CI (`460d21948`)

tests/test_phi_at_rest_inventory.py::test_pl1_encryption_rule_carves_out_the_backup_codec reddened
both the ubuntu-latest and windows-2022 test legs on the first push. It asserted
"build_store_cipher" not in dr_backup.py, pinning a sentence in docs/PHI.md §3 PL-1:
"Because the key is resolved by resolve_active_key and not build_store_cipher, cipher_provider = vault_transit never applies to a backup."

That sentence is now half wrong, and the half that is wrong matters. The archive's own seal is
unchanged — backup_codec, keyed by resolve_active_key — so vault_transit still never applies to
sealing or unsealing a .mfbak. But full_restore_verify now opens the snapshot's cipher-covered
CELLS through build_store_cipher, and under vault_transit that read runs in Transit. I corrected
docs/PHI.md to state the distinction (the archive versus the cells inside it) and re-pointed the
guard at the claim instead of at the import: if the module builds the store cipher, §3 has to say which
cipher governs which read. Its other assertions are untouched.

This is the second gate the inherited commit had never been run against, and it is the substantive one:
a security document asserting a control that does not hold is exactly what CLAUDE.md §11 / SDS-3.7
forbids.

POSITIVE CONTROL, measured on this box. With the store.py guard reverted and everything else
unchanged, test_full_verify_on_a_failed_open_reports_the_open_error_not_a_cleanup_error fails with
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: '...\mefor-verify-1igwxz1e\extracted_store.db', reported as FAIL. That is the item's
measured symptom reproduced, and it is what the guard fixes. 9 of 10 passed in that control run, so the
instrument is not failing everything.

FILE TYPES, not the subject

      3 md
      6 py

docs/CONFIGURATION.md
docs/PHI.md
docs/adr/0049-turnkey-dr-backup-restore-verify.md
messagefoundry/__main__.py
messagefoundry/pipeline/dr_backup.py
messagefoundry/store/store.py
scripts/security/crypto_inventory_check.py
tests/test_phi_at_rest_inventory.py
tests/test_restore_verify.py

Six .py files, two of them engine modules and one a CI gate script. Do not read this as a docs PR.

messagefoundry/store/store.py touches MessageStore.open, which every SQLite-backed test in the
repository runs through. The change is two try/except BaseException guards that close and re-raise;
the success path is unchanged. That breadth is the main review risk here and is why the whole-suite legs
matter more than usual.

CHECKS RUN

Check	Result
`ruff format --check .`	pass (1292 files)
`ruff check .`	pass
`mypy messagefoundry` (strict)	pass, 274 source files
`pytest tests/test_restore_verify.py`	10 passed
`pytest tests/test_backup_restore_atleastonce.py test_backup_runner.py test_backup_crypto.py test_store_aad_binding.py`	67 passed with the above
`pre-commit` (full hook set, at commit)	pass, including the crypto-inventory gate
`pytest tests/test_phi_at_rest_inventory.py`	33 passed after the fix
`pytest tests/test_asvs_gcm_invocation_bound.py`	32 passed
whole suite, `-n 4`, `test_connscale_smoke.py` deselected	STARTED, NOT FINISHED before my process exits — see below

WHAT MUST BE READ AFTER I AM GONE

The whole-suite legs on all three OS rows. A local whole-suite run reached 34 percent and then
reported one failure I caused myself — I edited store.py while it was running, which stales
linecache under inspect.getsource and made
test_asvs_gcm_invocation_bound.py::test_every_batched_encrypt_burst_charges_the_bound_on_every_backend
raise tokenize.TokenError: unterminated string literal. That file passes 32/32 on a stable tree; the
failure is an artifact of mid-run editing, not a defect. I restarted the run on the stable tree and it
did not finish before my process exited. It is not a result and I am not reporting it as one.
Because MessageStore.open is on every SQLite test's path, the Linux and Windows pytest legs are
the real gate on this PR, not the DR suite I ran. The first push's CI already ran the whole suite:
13121 passed with the single test_phi_at_rest_inventory failure that 460d21948 fixes.
windows-service-smoke and any other hosted-runner-only leg — a Builder never sees these.
repo harness tests (windows-2025) is RED and it is not mine.
tests/test_worktree_gate_control_plane.py::test_a_carried_env_at_an_UNRELATED_repo_allows_from_a_GOVERNED_cwd
reports PWSH LAUNCH TIMED OUT after 45s (BACKLOG #1304) — the known slow-Windows-runner pwsh-launch
timeout the test itself cites. 3210 passed beside it, the ubuntu-latest harness row passed, and
worktree_gate.ps1 is not in this diff. This leg is NOT in the required set (read below). Do not
inherit it into the next PR.
The "expected red" my brief warned about did not fire. tests/test_required_contexts.py passed on
all three test rows. Read branch protection for the live set rather than either that file or this
paragraph; measured at 2026-09-15 it is 15 contexts: CI gate, a PR that implements BACKLOG #N must update BACKLOG.md, bandit (Python SAST), cla, crypto-inventory (ASVS 11.1.3 discovery gate), dependency-and-secret-scan, forbidden-content, gitleaks, npm-audit, pip-audit,
repo-scan, semgrep, and the three test (ubuntu-latest | windows-2022 | windows-2025, py3.14) rows.

NON-REQUIRED legs that cover these file types, so green-required is not sufficient here: the
ide leg does not touch this, but the coverage/lint-extras and any docs-link legs do cover the two .md
files, and the ASVS anchor-report workflow reads scripts/security/. The required set is the merge bar,
not the correctness bar.

QUESTIONS I COULD NOT RESOLVE

Should the keyless case be KEY_MISMATCH at all? The item's "what closing looks like" step 2 says
report KEY_MISMATCH when the keyring cannot, and I implemented that. The inherited commit's AC-13
deliberately said FAIL instead. Both are defensible and I chose the item's wording. If the owner
prefers the narrower reading — that KEY_MISMATCH names only the ARCHIVE header fingerprint compare
and nothing else — the change is one status string and one AC sentence.
_decrypt_check reads MessageStore._CIPHER_COLUMNS, a private class attribute, from
pipeline/. That is the inherited commit's choice, documented in its docstring, and it is the only
way to avoid copying the store's coverage list into the DR module. It is still a pipeline/ module
reading a store/ private. The clean fix is for the store to publish that declaration; I did not do
it, because it widens the change into a store API addition.
The decrypt pass is unbounded — every mfenc: cell in the covered columns. On a multi-GB store
that is a full scan plus a decrypt per row inside a scheduled backup. full_restore_verify is
opt-in and documented as heavier, so this may be intended, but nobody has measured it at scale.

FOOTPRINT DEVIATIONS

My brief's footprint was dr_backup.py, store.py, test_restore_verify.py,
test_backup_restore_atleastonce.py, docs/CONFIGURATION.md and docs/adr/0049-*.md. Two files fall
outside it:

scripts/security/crypto_inventory_check.py — forced by the crypto-inventory gate, mine.
docs/PHI.md and tests/test_phi_at_rest_inventory.py — forced by the required test leg above, mine.
messagefoundry/__main__.py — arrives with the inherited commit (the restore-verify CLI prints
decrypted_cells), not mine.

Neither is in any sibling builder's stated footprint. tests/test_backup_restore_atleastonce.py needed
no edit: store_settings is keyword-with-default, so its positional-by-keyword calls still compile.

CI STATE WHEN I EXITED

All 14 non-aggregate required contexts green on 460d21948, including all three test OS rows
(ubuntu 7m39s, windows-2022 8m25s, windows-2025 10m52s), both SQL Server rows, postgres store, and
all three web-console rows. CI gate was still pending as the aggregator. The one red anywhere on the
PR is the non-required repo harness tests (windows-2025) flake described above.

BACKLOG #1718 is CLAIMED by this worktree (scripts/coord/claim.ps1 -Take 1718). Release it after the
merge: pwsh -NoProfile -File scripts\coord\claim.ps1 -Release 1718.

Not enqueued and auto-merge not armed — both belong to the Lander.

…s PHI Full restore verification built a bare StoreSettings to open the extracted snapshot, so an encrypted snapshot opened under the identity cipher: no key, no keyring, no provider. PRAGMA quick_check passed anyway, and the verify reported PASS having proved nothing about whether a single PHI cell was readable. Adding the decrypt pass without the settings fix turns that into the opposite and worse failure -- a perfectly good encrypted archive reported FAIL, which would teach a deploying operator that their backup is bad when it is fine, and a disaster-recovery check that cries wolf gets switched off. Four changes: 1. Thread the live StoreSettings into _verify_archive_blocking and on into _full_open_check, substituting only the path (and the backend, which the archive member is by construction). model_copy, not a rebuilt object, so a field added to StoreSettings later rides along instead of being dropped. 2. Add _decrypt_check: decrypt AND authenticate the snapshot's cipher-covered cells under the store's own cipher, with the same cell-bound AAD the store writes. A bit-flipped AEAD cell passes quick_check and every row count, so this is the half that makes the check worth running. VerifyResult now carries decrypted_cells -- a count, never a plaintext. 3. Bind the store before the try in _full_open_check, so an open that raises surfaces its own cause rather than a cleanup error over the top of it. 4. ADR 0049: correct the full_restore_verify prose, which claimed the open ran through the cipher, and add AC-13. Scope of the decrypt pass, stated rather than implied: it covers the store's own id-keyed _CIPHER_COLUMNS declaration. The cipher-covered tables whose AAD binds to a composite or natural key are enumerated only as code inside the store's bespoke passes, so widening this means giving the store one declaration to publish -- a separate change.

…open closes its handle (BACKLOG #1718) Builds on the commit beneath it, which threaded the live store settings into the full verify and added the decrypt pass. Three things it left open. 1. The status. A snapshot holding sealed cells that the settings resolve no key for was FAIL, which sends an operator looking for a bad archive when the archive is fine and the key configuration is not. It is now KEY_MISMATCH, raised from both places the condition surfaces: the decrypt pass, and the StoreKeylessError the store's own eager state/reference warm-ups raise first. A failed AEAD tag stays FAIL -- CipherError cannot separate bit rot from a key that was never supplied, and bit rot is the reading that must not be softened. 2. The leaked handle. MessageStore.open left its aiosqlite connection open when a warm-up raised, so the verify's temp-directory cleanup was refused and its PermissionError replaced the real cause. Measured here: without the guard the new test reports WinError 32 on the extracted snapshot instead of the missing key. 3. The case the item was filed for. No test held a state or reference row -- the rows the store decrypts eagerly at open, and so the ones that turned every scheduled backup of a keyed store into a failing verify. ADR 0049 AC-13 is amended for the status split and AC-14 added for the close; the CONFIGURATION.md row for full_restore_verify now says what the leg proves. The crypto-inventory gate (ASVS 11.1.3) also had to be told that dr_backup.py now imports the store-cipher seam. That import arrives with the commit below, so this branch reds that required leg without the row.

…ow it half does (BACKLOG #1718) The full restore-verify opens the extracted snapshot's cipher-covered cells through the STORE cipher (build_store_cipher, ADR 0049 AC-13). That is the same cipher that wrote them, so under cipher_provider = vault_transit the read runs in Transit. PHI.md's PL-1 rule said vault_transit never applies to a backup, full stop, and that sentence is now a false premise -- the kind §11 forbids a control from resting on. The archive's own seal is unchanged: backup_codec, keyed by resolve_active_key, so vault_transit still never applies to sealing or unsealing a .mfbak. The distinction is the archive versus the cells inside it, and the doc now states it. The guard that caught this asserted "build_store_cipher not in dr_backup.py", which pinned "one cipher in this module" rather than the claim itself. It now pins the distinction: if the module builds the store cipher, §3 has to say which cipher governs which read. Its other assertions are untouched. This is a required CI leg, red on both the ubuntu and windows-2022 test rows of PR 1126 before this commit.

wshallwshall · 2026-09-15T01:38:02Z

LANDER: PR 1126 and PR 1138 ARE THE SAME FEATURE, built twice under two item numbers. Neither should merge until one is withdrawn. Posting this on both.

THE EVIDENCE, measured rather than inferred from the titles.

The messagefoundry/__main__.py hunk is BYTE-IDENTICAL between them. Both add the same --full restore-verify wording and the same decrypted_cells reporting; a diff of the two patches, with index and hunk headers excluded, is empty.

PR 1126   BACKLOG #1718   head 460d2194   base: current main
PR 1138   BACKLOG #1561   head 9c1efdaa   base: older

Overlap on the rest:

messagefoundry/pipeline/dr_backup.py          1126 adds 160 lines   1138 adds 130
docs/adr/0049-turnkey-dr-backup-restore-verify.md   1126 adds 43    1138 adds 30
tests/test_restore_verify.py                  1126 adds 223         1138 adds 130

113 of the 126 added lines in PR 1138's dr_backup.py also appear in PR 1126's. The 13 that do not are a return-shape difference: PR 1138 returns (ok, message, decrypted_cells) as a three-tuple from _full_open_check.

PR 1126 is the superset. It additionally changes messagefoundry/store/store.py, scripts/security/crypto_inventory_check.py and tests/test_phi_at_rest_inventory.py, which PR 1138 does not touch at all.

WHY THIS NEEDED SAYING NOW rather than at merge time. Two PRs implementing one feature against different bases do not necessarily conflict textually. Whichever lands second can merge clean and double-apply, and the merge queue will not report anything. This is the failure mode that does not announce itself, which is why I am recording it while both are still unmergeable rather than discovering it on the second merge.

WHAT I AM NOT DOING YET. I am not closing PR 1138, because PR 1126 has not proved it can land and I will not discard the fallback before the preferred one is green. PR 1126 currently reads BLOCKED: CI gate is REQUIRED and ABSENT, so the roll-up never ran.

ON PR 1126's OTHER RED, so it is not mistaken for a reason to prefer PR 1138: repo harness tests (windows-2025) is FAILURE but is NOT required, and the failure is environmental rather than the diff. Attempt 1, so this is the original evidence and not a re-run's. The run reports 1 failed, 3210 passed and the failing test says so itself:

This is a PROCESS LAUNCH that never returned. It is NOT an assertion failure
and NOT evidence that the gate's behaviour changed: no gate logic ran.

That leg intermittently times out starting pwsh. Worth noting that the test was deliberately written to tell that apart from a real failure, which is why this took one log read rather than a bisect.

NEITHER DIFF HAS BEEN READ AT HUNK GRANULARITY YET. This comment establishes the relationship only. A read verdict follows separately, and neither will be enqueued before one.

wshallwshall · 2026-09-15T01:38:47Z

Cross-PR finding from the Builder on #1147, verified here before posting. Not a blocker for review, but it should not land unnoticed.

The two cleanup paths added here are unshielded, and they catch BaseException.

MessageStore.open now has except BaseException: at store.py:2307 and :2341, each awaiting cleanup inside the handler (await db.close(), await store.close()). BaseException catches asyncio.CancelledError, and an await inside a cancellation handler can itself be cancelled immediately — so on the cancellation path the cleanup may never complete, which is the failure this change exists to prevent.

Measured: asyncio.shield appears 0 times in messagefoundry/store/store.py at 460d21948. Control: the _writer_txn work on #1147 uses shield 4 times for exactly this reason.

The non-cancellation path — a StoreKeylessError or CipherError from the eager warm-ups, which is the case this PR was built for — is unaffected and works as described. So this is a narrower residual than it first sounds: it is the cancellation arm only.

Two options, and I am not asserting which is right:

Shield the cleanup awaits, matching fix(store): unwind the SQLite writer transaction on cancellation, not only on Exception #1147's idiom so the two changes to this module agree.
Narrow the catch to Exception plus an explicit CancelledError arm, if unwinding on cancellation is deliberately out of scope here.

#1147 and this PR merge clean and use no competing locking idiom; this is the one interaction worth a decision rather than a silent resolution.

wshallwshall · 2026-09-15T02:06:36Z

Triage of the two red rows, and a structural note that is worth more than either.

repo harness tests (windows-2025) is the known flake, and the test says so itself. Captured at run 34914411854 attempt 1, head 460d21948:

tests/test_worktree_gate_control_plane.py::test_a_carried_env_at_an_UNRELATED_repo_allows_from_a_GOVERNED_cwd
subprocess.TimeoutExpired: ['pwsh', '-NoProfile', '-NonInteractive', '-File', ...worktree_gate.ps1...]
    timed out after 45 seconds
AssertionError: PWSH LAUNCH TIMED OUT after 45s (BACKLOG #1304).

No test logic ran — pwsh never launched. That is the slow-runner failure the test cites by number, not a defect in this diff.

The structural note: a NOT-required leg is blocking a REQUIRED context here.

The only two failed jobs in that run are repo harness tests (windows-2025) and CI gate. CI gate is in the live required set of 15; repo harness tests (windows-2025) is not. So the aggregator turns a non-required failure into a required-context failure, and the "not required" label on that leg does not mean what a reader would take it to mean — it blocks the merge anyway.

That matters beyond this PR. Anyone classifying rows against the required set, as they should, will read repo harness tests as safe to ignore and then be unable to explain why CI gate is red. Worth an item; I am not filing one from here.

All other required contexts on this PR are green, including all three test OS rows, both SQL Server rows, postgres store, and all three web-console rows.

I have re-run the failed jobs on the same head. The attempt-1 evidence is quoted above so the re-run does not erase the failure that was actually acted on.

…ACKLOG #1718) store.py resolves to main's content. This PR's step 4 closed the connection on a failed MessageStore.open so a keyless open surfaces KEY_MISMATCH, not a Windows PermissionError from temp-dir cleanup. BACKLOG #1670 (1fbcf78) landed independently doing the same thing, and its try/except around the whole region after aiosqlite.connect subsumes this branch's two narrower guards: it closes the constructed store, which owns the read pool, and the writer either way. Verified, not asserted. This PR's own regression test test_full_verify_on_a_failed_open_reports_the_open_error_not_a_cleanup_error passes against main's code. Control: deleting main's cleanup block reds it with WinError 32 masking the expected KEY_MISMATCH, the exact symptom step 4 was written against. The PR keeps its DR-side value: _full_open_check under the real store keyring, _decrypt_check, KEY_MISMATCH, ADR 0049 and the docs.

wshallwshall · 2026-09-15T13:52:07Z

Conflict resolved: store.py takes main's content, this PR's store.py hunk is dropped

Head moved 260dffd93 -> 428f0a523. mergeable is now MERGEABLE.

How, and why

messagefoundry/store/store.py was the only conflicting file. It now matches origin/main byte for byte (git diff origin/main -- messagefoundry/store/store.py is empty).

Main's BACKLOG #1670 already covers this PR's step 4. Step 4 added a connection-close on a failed MessageStore.open, so a keyless open surfaces its real cause instead of a Windows PermissionError raised while a caller unwinds a tempfile.TemporaryDirectory around the failure. 1fbcf7833 (#1670) then landed independently doing the same thing.

Main's version is a strict superset, not merely an equivalent:

	this PR	main (#1670)
guarded region	two blocks, with the `store = cls(...)` line between them unguarded	one block covering everything after `aiosqlite.connect`
closes the constructed store	yes	yes
closes the read pool	via `store.close()`	via `store.close()`
closes the writer connection	only in the pre-construct block	always, after `store.close()`

MessageStore.close closes every tracked read-pool connection and then self._db, so both versions release the handles that hold the Windows file lock. Main additionally closes the raw connection unconditionally, and covers the gap between the two blocks.

Verified, not asserted

This PR's own regression test is the experiment, run against main's implementation:

tests/test_restore_verify.py::test_full_verify_on_a_failed_open_reports_the_open_error_not_a_cleanup_error  PASSED

A passing test that examines nothing is indistinguishable from a real pass, so I ran a positive control: deleting main's except BaseException: cleanup block reds that same test with

AssertionError: PermissionError: [WinError 32] The process cannot access the file
because it is being used by another process: '...\extracted_store.db'
assert 'FAIL' == 'KEY_MISMATCH'

That is the exact symptom step 4 was written against. The control was reverted; store.py is identical to origin/main.

Nothing landed was reverted

suite	pins	result
`test_restore_verify.py`, `test_backup_restore_atleastonce.py`, `test_store.py`	this PR + #1670	72 passed
`test_backlog1548_writer_txn_cancel_unwind.py`	#1147 writer-transaction unwind on cancellation	passed
`test_replay_purged_body.py`, `test_replay_erased_body_scope.py`	#1560 replay refusal on erased body	passed
`test_cli.py`	#1670 CLI exit-2 translation	passed

The last four ran together: 138 passed, 4 skipped. Also green: test_phi_at_rest_inventory.py, test_tooling_partition.py, test_security_doc_drift.py (86 passed), ruff format --check (1299 files), ruff check, mypy messagefoundry strict (274 files, no issues).

What this PR still delivers

Dropping the store.py hunk costs the PR nothing it uniquely offered. Its DR-side work has no counterpart on main: _full_open_check opening under the real store keyring, _decrypt_check reading cipher-covered cells back, the KEY_MISMATCH status and decrypted_cells count, plus the ADR 0049 and docs/CONFIGURATION.md corrections. The PR is not redundant.

Remaining diff vs main is 8 files: docs/CONFIGURATION.md, docs/PHI.md, docs/adr/0049-turnkey-dr-backup-restore-verify.md, messagefoundry/__main__.py, messagefoundry/pipeline/dr_backup.py, scripts/security/crypto_inventory_check.py, tests/test_phi_at_rest_inventory.py, tests/test_restore_verify.py.

One note for whoever lands this

The __main__.py auto-merge is semantically safe. #1670 routed store-opening subcommands through a new exit-2 helper and deliberately excluded restore-verify, whose only open_store call sits inside _full_open_check and is already caught there. This PR touches only _restore_verify's docstring and its decrypted_cells reporting, so the two changes do not overlap.

Not enqueued, auto-merge not armed.

wshallwshall · 2026-09-15T13:56:16Z

LANDER review. Read against head 428f0a5, base 0f9206a (current main). Merge.

THE DEFECT IS THE GATE-THAT-EXAMINED-NOTHING FAMILY IN THE HIGHEST-STAKES PLACE THERE IS. A full restore-verify opened the snapshot and reported PASS without ever proving a single PHI cell was readable. The docstring puts it better than I would:

a bit-flipped AEAD cell passes PRAGMA quick_check and every row count, so without this pass a full verify would report PASS on an archive whose bodies no longer decrypt.

A disaster-recovery check that goes green on an unrecoverable archive is the worst false green in the product, because the one moment its answer matters is the moment nobody can go back and re-check it.

THE CONTROL IS IN THE TEST AND IT IS THE RIGHT ONE:

assert res.decrypted_cells >= 1, "a full verify that decrypted nothing proves nothing"

paired with its mirror on the unencrypted archive:

assert res.decrypted_cells == 0

That pairing is what makes the count meaningful rather than decorative. Zero is accepted ONLY where it is provably correct, and demanded to be non-zero everywhere else. test_full_verify_fails_on_a_corrupted_aead_cell then asserts the lightweight verify still says PASS on the same archive, which is the control proving the OLD path was blind rather than merely improvable.

THE SUBTLEST CORRECT THING IS THE IDENTITY-CIPHER TRAP. if cipher.is_encrypted(plain) catches the case where a keyless open hands an mfenc: value straight back. Without that branch a keyless open would "decrypt" every sealed cell successfully, count them, and report PASS with a healthy-looking cell count -- a false green that is worse than the one being fixed, because it comes with a number attached. Catching it and routing it to KEY_MISMATCH rather than PASS is the whole fix in one branch.

THE KEY_MISMATCH / FAIL DISCRIMINATION IS ARGUED IN THE RIGHT DIRECTION. A CipherError stays FAIL because it cannot separate a corrupted ciphertext from a key that was never supplied -- "corruption is the reading that must not be talked down." KEY_MISMATCH is reserved for the one cause the keyring is unambiguously to blame for. An operator reading FAIL goes looking for a bad archive; reading KEY_MISMATCH they fix their keys. Getting that backwards would send someone hunting the wrong thing during a restore.

model_copy(update={...}) rather than a rebuilt StoreSettings is the right call and the reason is stated: a field added later "rides along instead of being silently dropped". That is characterise-rather-than-enumerate applied to config, and substituting only path and backend keeps every field that governs HOW the bytes are read.

Binding store = await open_store(...) BEFORE the try is the same class of care: a raising open surfaces its own cause instead of a NameError from a finally, or the Windows temp-directory cleanup error stacked over the top of it. There is a test for exactly that -- ..._reports_the_open_error_not_a_cleanup_error, asserting "another process" and "WinError" are ABSENT from the reason.

I VERIFIED THE TWO LOAD-BEARING CLAIMS RATHER THAN ACCEPTING THEM.

MessageStore._CIPHER_COLUMNS is a real declaration on main at store.py:2789, so the cell list is the store's own rather than one invented here.
The out-of-scope tables this PR names -- message_events, connection_event, alert_instance, and the response/state/reference family -- match the store's OWN NB: comments at store.py:2803-2812, which explain that their cell_aad binds to natural columns because their id is AUTOINCREMENT. So the scope boundary is the store's, not this module's invention, and the stated rule for widening it (publish one declaration from the store, do not copy its private passes here) is the right one.

ON THE MERGE RESOLUTION, WHICH I ALSO CHECKED. The Manager reports store.py was resolved to MAIN's content and this PR's step-4 hunk dropped, because #1670 is a strict superset. Verified at origin/main: there is ONE try: at store.py:2472 covering everything after aiosqlite.connect at :2464 -- including the store = cls(...) construction at :2495 -- with except BaseException: at :2523 closing the store and then the connection. The file's own comment at :2465 says "Everything past connect runs under the cleanup below (#1670)." This PR's version guarded two narrower blocks with that construction unguarded between them. Main's is strictly wider, so dropping the hunk is correct and store.py is rightly absent from the 8 changed files.

AND THE BUILDER PROVED IT INSTEAD OF READING IT, which is the part worth repeating: it DELETED main's cleanup block and re-ran this PR's own regression test, which went red with PermissionError: [WinError 32] and assert 'FAIL' == 'KEY_MISMATCH' -- the exact symptom step 4 was written against -- then reverted and confirmed byte-identity with origin/main. A green test on main's code alone would have been indistinguishable from a test that examined nothing.

THIS IS THE THIRD TIME TONIGHT TWO LEDGER NUMBERS TURNED OUT TO NAME ONE DEFECT (#1718's step 4 and #1670). That is worth a ledger item of its own rather than three separate rediscoveries.

Verdict: merge.

wshallwshall · 2026-09-15T14:58:40Z

LANDER: both reds are known flakes, neither is yours, and I am NOT re-running yet — re-running now would be wasted. My merge verdict above stands.

RED 1 -- the store thread race, and it has now reached a THIRD platform.

test (windows-2022, py3.14)
FAILED tests/test_store.py::test_a_failed_open_closes_the_connection_and_lets_the_process_exit
AssertionError: a failed open left live thread(s): Thread-1 (_connection_worker_thread)
1 failed, 13417 passed

That arrived on main with #1152 and has now been seen on windows-2025, ubuntu-latest and windows-2022. It has evicted four pull requests. PR #1174 fixes it and is at merge-queue position 1.

This is why I am not re-running this leg now. Your PR's own CI builds against main, and main still carries the broken test, so a re-run is a fresh roll of the same race rather than a new experiment. I will re-run it once #1174 has landed, when the re-run is actually testing something different.

The evidence that #1174 works is a paired control CI produced on its own: two queue branches in the same batch, one second apart, differing by exactly that diff — pr-1164-... (no fix) FAILED on this assertion, pr-1174-... (fix) SUCCEEDED.

RED 2 -- the pwsh launch ceiling, BACKLOG #1304.

repo harness tests (windows-2025)
AssertionError: PWSH LAUNCH TIMED OUT after 45s (BACKLOG #1304)   x2
2 failed, 3209 passed

Different tests again (test_a_LITERAL_backtick_in_a_cmd_payload_does_not_hide_a_gated_command and test_an_assignment_MENTIONED_before_the_write_does_not_earn_a_refusal), same 45-second ceiling. That leg fails roughly one execution in three on main's own push runs with no PR diff present at all — measured at 33.3%, 31.1% and 28.1% across three independent censuses. So two failures here is the base rate, not a signal, and nothing in your eight files can affect how long pwsh takes to start.

I want to flag my own error on that second one, because it is the more useful half. I recently escalated #1168 to the owner over four failures of this exact leg, having never measured the leg's failure RATE. Four-in-a-row at a one-in-three rate is unremarkable. A count is not evidence until you know the rate it should be compared against, and I had run only the sibling control — which answers "is it broken everywhere?", not "is four surprising?".

WHAT HAPPENS NEXT, so nothing is owed from you: #1174 lands, I re-run this PR's failed legs, and enqueue when they are green. 28 SUCCESS already, and the only two reds are the two flakes above.

wshallwshall added 3 commits September 14, 2026 19:07

github-actions Bot added the ci-red A required check went red. Attribute it before retrying. label Sep 15, 2026

wshallwshall and others added 2 commits September 14, 2026 23:08

Merge branch 'main' into claude/builder-1718-dr-keyed-verify

260dffd

Merge branch 'main' into claude/builder-1718-dr-keyed-verify

bd754ad

wshallwshall added this pull request to the merge queue Sep 15, 2026

wshallwshall mentioned this pull request Sep 15, 2026

fix(dr): open the restore snapshot under the live store settings (BACKLOG #1561) #1138

Open

Merged via the queue into main with commit f4d3577 Sep 15, 2026
43 checks passed

wshallwshall deleted the claude/builder-1718-dr-keyed-verify branch September 15, 2026 17:50

Navigation Menu

Uh oh!

fix(dr): the full restore-verify opens the snapshot under the real store keyring (BACKLOG #1718) - #1126

fix(dr): the full restore-verify opens the snapshot under the real store keyring (BACKLOG #1718)#1126
wshallwshall merged 6 commits into
mainfrom
claude/builder-1718-dr-keyed-verify

wshallwshall commented Sep 15, 2026 •

edited

Loading

Uh oh!

wshallwshall commented Sep 15, 2026

Uh oh!

wshallwshall commented Sep 15, 2026

Uh oh!

wshallwshall commented Sep 15, 2026

Uh oh!

wshallwshall commented Sep 15, 2026

Uh oh!

wshallwshall commented Sep 15, 2026

Uh oh!

wshallwshall commented Sep 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

wshallwshall commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

THIS PR CARRIES TWO AUTHORS' WORK, AND THE OLDER HALF HAD NEVER BEEN REVIEWED

WHAT I ADDED ON TOP (d34676f55)

AND A SECOND RED THE INHERITED COMMIT LEFT BEHIND, CAUGHT BY CI (460d21948)

FILE TYPES, not the subject

CHECKS RUN

WHAT MUST BE READ AFTER I AM GONE

QUESTIONS I COULD NOT RESOLVE

FOOTPRINT DEVIATIONS

CI STATE WHEN I EXITED

Uh oh!

wshallwshall commented Sep 15, 2026

Uh oh!

wshallwshall commented Sep 15, 2026

Uh oh!

wshallwshall commented Sep 15, 2026

Uh oh!

wshallwshall commented Sep 15, 2026

Conflict resolved: store.py takes main's content, this PR's store.py hunk is dropped

How, and why

Verified, not asserted

Nothing landed was reverted

What this PR still delivers

One note for whoever lands this

Uh oh!

wshallwshall commented Sep 15, 2026

Uh oh!

wshallwshall commented Sep 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

wshallwshall commented Sep 15, 2026 •

edited

Loading

WHAT I ADDED ON TOP (`d34676f55`)

AND A SECOND RED THE INHERITED COMMIT LEFT BEHIND, CAUGHT BY CI (`460d21948`)