Delete legacy forensic audit logs on app startup - #1782
Conversation
erskingardner
left a comment
erskingardner
left a comment
There was a problem hiding this comment.
| Review metadata | Value |
|---|---|
| Reviewed at (UTC) | 2026-09-10T13:02:00Z |
| Commit reviewed | 5f1a8830b7352cb0dbe7b256c85481770845dc65 |
| Model | claude-opus-5[1m] |
| Reasoning level | Not exposed by runtime |
| Recommended action | Resolve serious concerns before merge |
The implementation is careful and I found no correctness defect. I verified against the code rather than the description:
- The matcher covers the naming MDK actually produced. The unversioned
audit-<engine>.jsonlform really was v1 (audit_recorder_writes_a_new_v4_file_and_leaves_legacy_files_untouchedderivesv1_paththe same way), and segments have always been{index:06}(segment_path, introduced in #1642), so the>= 6digit rule cannot miss a real segment. - No other writer can be hit: only
marmot-appwrites into account dirs, and only throughdefault_jsonl_path→-v4.jsonl. The CLI writes no audit JSONL. - Symlink safety holds:
symlink_metadataon the container,DirEntry::file_type(d_type/lstat, never following) at both levels, no recursion, and the name check runs beforefile_type()so the scan does not stat every file. - Only
try_with_relays_and_account_home_and_configacquires the lease, and UniFFI (and therefore the C ABI, viamarmot_uniffi::Marmot::open) opens through it, so the "no additional host call" claim checks out.agent-connectorand the conformance simulator also inherit it — harmless, but worth knowing. - Preserving
.tmpandaudit-key-reveal.jsonlleaks nothing:staged_swap_pathfiles are always freshly created and empty, andKeyRevealAuditEntrycarries only a hashed account ref.
The concern worth answering before merge
This is unconditional, silent, permanent deletion of files in the user's data root on every startup, and it reverses a contract documented one commit earlier (#1779: "Keep legacy files available locally for inspection/deletion"). There is no linked issue, no host opt-out, and no signal to a host that files it previously listed have disappeared.
The stated rationale also does not fully hold: the account label survives cleanup as the accounts/<label> directory name in the same root, so "account/device labels remain on the device" is only partly addressed. The genuinely strong arguments are absent from the description — legacy JSONL can hold decrypted content (FullData mode, see mdk#1014) in plaintext outside the SQLCipher database, and legacy files under orphaned account directories were never reachable through audit_log_files() (it iterates known accounts only), so hosts could not offer the user a delete for them at all. If those are the real reasons, please say so; they justify the change far better than the label framing, and they set the boundary correctly.
Non-blocking
See inline comments: accounts layout duplicated instead of asking AccountHome, silent no-op when accounts is not a directory, matcher slightly broader than the documented boundary, and ~160 lines of filesystem/app-open tests for what is mostly a pure string predicate.
Docs, AGENTS.md, and tracing all match repo conventions (aggregate counts, marmot_app::audit_log target), and no versions were touched.
| ) -> Result<Self, AppError> { | ||
| let root = root.as_ref().to_path_buf(); | ||
| let lease = MarmotRootRuntimeLease::try_acquire(&root)?; | ||
| audit_log::cleanup_legacy_audit_logs(&root); |
There was a problem hiding this comment.
Cleanup is unconditional here: every exclusive-root open of every host deletes matching files, with no config flag, no host callback, and no record that anything was removed beyond an aggregate count in the log.
Two asks:
- Link the issue that requested this (I could not find one), so the decision to destroy user-local data has a reviewable home.
- Consider whether this deserves a sunset. It is a one-time migration, but it costs one
read_dirper account directory on every open forever and the code has no expiry. A follow-up issue to remove it after adoption would keep this from becoming permanent startup work for a condition that stops occurring.
| /// independently of audit consent. Failed deletions are retried on the next open. | ||
| pub(crate) fn cleanup_legacy_audit_logs(root: &Path) { | ||
| let mut counts = CleanupCounts::default(); | ||
| if clean_accounts(&root.join("accounts"), &mut counts).is_err() { |
There was a problem hiding this comment.
This hard-codes AccountHome's private on-disk layout (accounts_dir() is self.root.join("accounts")) as a fourth copy of the "accounts" literal in the workspace.
The constructor already holds the account_home argument at this point, so it could pass it in and ask it for the directory (exposing AccountHome::accounts_dir() would be a two-line change). That matters beyond DRY: a host that passes an AccountHome rooted somewhere other than root gets a cleanup that silently scans the wrong tree. It fails safe today (nothing deleted), but the coupling is invisible and nothing pins it — no test would catch the layouts drifting apart.
| // The constructor already verified and exclusively leased the root. Do not | ||
| // follow links in the accounts container or either level beneath it. | ||
| match fs::symlink_metadata(accounts) { | ||
| Ok(metadata) if !metadata.is_dir() => return Ok(()), |
There was a problem hiding this comment.
This branch (and the NotFound one below) makes cleanup a permanent silent no-op when accounts exists but is not a directory — including the case where it is a symlink to a directory, which symlink_metadata().is_dir() reports as false.
A fresh root with no accounts yet is the expected quiet case, but a symlinked or file-shadowed accounts is a config the rest of the app happily uses (AccountHome does not reject it), so cleanup would never run and nothing would ever say so: counts.failed stays 0, so neither the warn nor the info fires.
Counting the non-directory case as failed (or a debug! with a reason) would make a permanently skipped cleanup observable without leaking anything.
| .or_else(|| stem.strip_suffix("-v3")) | ||
| .unwrap_or(stem); | ||
| engine_id.len() == super::AUDIT_ID_BYTES * 2 | ||
| && engine_id.bytes().all(|byte| byte.is_ascii_hexdigit()) |
There was a problem hiding this comment.
Two small ways the matcher is broader than the documented boundary:
is_ascii_hexdigit()accepts uppercase, but engine ids come fromhex::encodeand are always lowercase, so this matches names MDK has never written. The test asserts"g".repeat(32)is preserved but never checks uppercase.- The docs and README say custom names are preserved; a host file named
audit-<32 uppercase-or-lowercase hex>.jsonlis deleted, because that is exactly the reserved v1 form. That is the intended trade, but "custom names are preserved" reads as a stronger guarantee than the code gives.
Either tighten to is_ascii_digit() || ('a'..='f'), or reword the boundary docs to say a custom name that collides with a reserved form is not preserved. Not worth much either way, but the file is about deleting user data, so the stated boundary should be exact.
| } | ||
|
|
||
| #[test] | ||
| fn legacy_audit_cleanup_deletes_only_reserved_old_names_at_startup() { |
There was a problem hiding this comment.
The safety-critical part of this change is is_legacy_audit_file_name, a pure &str -> bool. This test spends ~55 lines building a filesystem and opening a full MarmotApp to exercise it, and the module is #[cfg(all(test, unix))], so the classification matrix is not checked on non-unix builds at all.
A table-driven test over the predicate itself would cover more forms (uppercase hex, -v10, -v1-v1, -seg boundaries, non-UTF8-adjacent shapes) in a fraction of the lines, run everywhere, and fail with a name rather than a missing file. The three remaining tests — exclusive ownership, symlink isolation, failure/retry — genuinely need the filesystem and the app; this one mostly does not.
(The #[cfg(all(test, unix))] gate itself matches root_runtime_lease.rs, so no objection to the gate — only to what sits behind it.)
| Rejected bodies must not be stored as quarantined artifacts by Goggles. Client adoption and server deployment are | ||
| separate from this SDK change. | ||
|
|
||
| ### Legacy local cleanup |
There was a problem hiding this comment.
This section documents the mechanism well but not the reason, which is where I think the change is weakest as written.
Worth stating explicitly here:
- What cleanup does not achieve. The account label persists as the
accounts/<label>directory name in this very root, so a reader who takes the PR description's "account/device labels remain on the device" at face value will overestimate the remediation. - What it does achieve, which is stronger: legacy JSONL can contain decrypted content (
FullDatamode, mdk#1014) as plaintext outside the SQLCipher database, and legacy files in orphaned account directories were unreachable throughaudit_log_files()(it iteratesaccount_home().accounts()), so no host-driven delete could ever have removed them.
That second point is the real justification for doing this automatically rather than leaving it to the existing list/delete API, and it belongs in the doc.
erskingardner
left a comment
erskingardner
left a comment
There was a problem hiding this comment.
| Review metadata | Value |
|---|---|
| Reviewed at (UTC) | 2026-09-10T13:05:53Z |
| Commit reviewed | 5f1a8830b7352cb0dbe7b256c85481770845dc65 |
| Model | Cursor Grok 4.6 |
| Reasoning level | Not exposed by runtime |
| Recommended action | Resolve serious concerns before merge |
Independent review of the same SHA. I agree the implementation is careful and I also found no correctness bug in the matcher, lease hook, or symlink isolation.
Verified against the code:
- Historical names match what MDK actually wrote: unversioned
audit-<engine>.jsonlis v1 (audit_recorder_writes_a_new_v4_file_and_leaves_legacy_files_untouched), and segments have always been{index:06}(segment_pathfrom #1642). The>= 6digit +u32rule cannot miss a real segment, and-v4/-v5/-v30stay 35-character stems after the v1–v3 strip. - Production hosts all go through the leased constructor: UniFFI
Marmot::open, C ABI via that same open, andagent-connector. Cleanup runs aftertry_acquireand before the app is exposed, so no recorder can be appending to a file being unlinked. DirEntry::file_type()does not follow links;remove_fileunlinks the directory entry. Combined with the name check beforefile_type(), this does not walk or delete through symlinks.read_diron a replacedaccountssymlink is a hostile TOCTOU, not a realistic same-process race under the lease.
Concern to answer before merge
This is silent, unconditional, permanent deletion of user-local files on every exclusive-root open, and it reverses the contract #1779 shipped one commit earlier ("Keep legacy files available locally for inspection/deletion"). There is no linked issue, no host opt-out, and no signal that files a host previously listed have disappeared.
The PR frames this as removing leftover account/device labels. That is only partly true: the account label remains as accounts/<label>/ in the same root. The stronger reasons are not written down:
- Pre-#1580 v1–v3 JSONL can still hold
FullDataplaintext outside SQLCipher (mdk#1014). audit_log_files()only iteratesaccount_home().accounts(), so legacy files under orphaned/unreadable account directories were never host-deletable.
If those are the real reasons, say so in the PR and in ### Legacy local cleanup. They justify automatic deletion and they set the boundary (reserved names, including orphans; not a general v4 retention policy).
A smaller completeness hole in that same framing: failed remove_account wipes leave the same reserved filenames under .wipe-tombstones/, which this scan never touches. Only worth expanding if the goal is actually "get leftover plaintext off the device."
Non-blocking
The earlier review's notes on the hardcoded accounts literal, silent no-op when accounts is not a directory, and the heavy filesystem test for a pure string predicate are all fair; I will not repeat them. One additional drift in a file this PR already touched: open_audit_recorder still says legacy files remain enumerable.
| use crate::{MarmotApp, config}; | ||
|
|
||
| mod legacy_cleanup; | ||
| pub(crate) use legacy_cleanup::cleanup_legacy_audit_logs; |
There was a problem hiding this comment.
This file still tells the opposite story ~700 lines later in open_audit_recorder:
// Start a distinct v4 file. Legacy files remain enumerable for local
// inspection/deletion, but the upload gate rejects their contents.
That was the #1779 contract. After this change it is true only for the unleased test constructors. Please update that comment (and any nearby "legacy files remain" wording) so the production path is not documented as leaving v1–v3 files in place.
erskingardner
left a comment
erskingardner
left a comment
There was a problem hiding this comment.
| Review metadata | Value |
|---|---|
| Reviewed at (UTC) | 2026-09-10T13:07:25Z |
| Commit reviewed | 5f1a8830b7352cb0dbe7b256c85481770845dc65 |
| Model | claude-opus-5[1m] |
| Reasoning level | Not exposed by runtime |
| Recommended action | Merge |
Posted as a comment rather than an approval: GitHub blocks approving your own pull request. The recommended action is still Merge.
Clean, well-scoped follow-through on #1779. ~110 lines of implementation, no new abstraction, no new dependency, no public API, one call site. The premise holds: after the v4-only cutoff those v1-v3 files can never be uploaded again, so they are dead weight that still carries the account/device labels #1779 removed. Deleting them at the one moment the process provably owns the root is the right seam.
What I verified against the tree rather than the description
- The matcher is complete for every name this app ever wrote.
audit-<engine>.jsonl(initial import),-v2(pre-#1580),-v3(#1580) are the only historical app forms, anddefault_jsonl_pathonly moved to-v4in #1779. Segments have always been-seg{index:06}, introduced in #1642 (post-v3), so the six-digit floor cannot miss a real segment. .tmppreservation is safe, not a PII leak:swap_to_fresh_filestages an empty file and renames it over the live path, so a crashed rotate leaves no legacy payload at the.tmpname.- Symlink isolation is real at both levels.
clean_accountslstats the container, andDirEntry::file_typedoes not follow links, so a symlinked account dir failsis_dir()and a symlinked file failsis_file(). - Boundary cases behave:
-v4/-v10/-v30survive thestrip_suffixchain,-seg000001-seg000002fails the length check afterrsplit_once, a directory named like a legacy file is skipped by the type check, and non-UTF-8 names fall out atto_str(). - Constructor coverage is as claimed. UniFFI/MarmotKit (
marmot-uniffi/src/lib.rs:383, and C through it) andwn-agent(agent-connector/src/lib.rs:249) all open through the leased path. The CLI uses the unleased constructor, but it has no audit surface at all, so there are no legacy files in CLI-only roots — not a gap. - Repo invariants respected: counts-only tracing with
target/method, no new files created, idempotent with per-entry failure containment and retry on next open.
Two non-blocking notes inline, both about the same failure mode — the cleanup's knowledge of layout/naming is coupled to constants that could drift out from under it while the tests stay green. Neither changes behavior today.
One product question, not a code objection. This reverses the previously documented contract (Keep legacy files available locally for inspection/deletion) with no host-visible signal: no count is returned, no callback fires, and a user mid-investigation loses their local v3 evidence on the next app open. The docs record the new behavior and the PII argument is the stronger one, so I would not block on it — but if any host UI currently lists legacy files for the user, it is worth confirming that surface degrades gracefully rather than showing a stale list.
Docs, AGENTS.md, README, and the architecture note all track the code accurately. CI is green on the reviewed SHA.
| .or_else(|| stem.strip_suffix("-v2")) | ||
| .or_else(|| stem.strip_suffix("-v3")) | ||
| .unwrap_or(stem); | ||
| engine_id.len() == super::AUDIT_ID_BYTES * 2 |
There was a problem hiding this comment.
Non-blocking: this matcher describes a frozen historical filename convention (audit-<32-hex>.jsonl, -v1/-v2/-v3, verified back to the initial import and through #1580), but it derives the expected length from the live AUDIT_ID_BYTES. If that constant is ever changed for new v4+ ids, this silently stops matching files already on disk and the cleanup quietly becomes a no-op for exactly the accounts it exists to clean.
A local frozen constant would express the intent and decouple the two:
/// v1-v3 engine ids were always 16 bytes; this is history, not the current id width.
const LEGACY_ENGINE_ID_HEX_LEN: usize = 32;| /// independently of audit consent. Failed deletions are retried on the next open. | ||
| pub(crate) fn cleanup_legacy_audit_logs(root: &Path) { | ||
| let mut counts = CleanupCounts::default(); | ||
| if clean_accounts(&root.join("accounts"), &mut counts).is_err() { |
There was a problem hiding this comment.
Non-blocking, but the two halves of this compound: the scan root is a hardcoded "accounts" literal that duplicates AccountHome's layout (home.rs:1282), while try_with_relays_and_account_home_and_config is separately handed the AccountHome that actually owns that layout. Every in-repo caller passes a home rooted at the same path, so this is correct today.
What makes it worth a line: neither new test would catch drift. Both build accounts/<label> by hand (root.path().join("accounts/alice"), and the blocked/healthy dirs), so if the account layout ever changed, cleanup would scan an empty/absent directory, delete nothing, and both tests would still pass green while real devices kept their labelled v3 files.
Cheapest fix is to make one of the two ends authoritative: create the account through AccountHome::create_account and write the legacy fixtures into app.account_home().account_dir(label) (test 2 already does this for the v4 recorder assertion), so the test fails if the impl's literal ever diverges.
|
Warning Review limit reachedNext included review available in 15 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 72 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (7)
Comment |
|
Addressed the earlier Claude review (5167394846) together with this round's Cursor and Claude reviews (5167441736, 5167462532) in 59ea427. Both new reviewers finished before the address pass began.
Validation: This completes the single review/address round. The follow-up commit has not had another external review round; GitHub CI will validate the updated head. |
Datawav
left a comment
Datawav
left a comment
There was a problem hiding this comment.
No actionable findings.
Not approved on this head: failed CI: Simulator and vectors (failure), Required CI (failure). A corrected head is re-audited automatically.
2 participants
After #1779, MDK writes and uploads only v4 forensic audit logs. Legacy files no longer have a supported upload path, but can retain account/device labels and, in older FullData-mode logs, decrypted content as plaintext outside SQLCipher. Legacy files in orphaned account directories or failed-wipe remnants are also absent from the host-facing audit listing. This change permanently deletes recognized v1-v3 forensic files when an app acquires exclusive ownership of its data root, before any recorder opens.
Cleanup runs even with audit recording disabled or uploads unconfigured. Swift/Kotlin and C app constructors already use this open path, so adopting the updated MDK requires no additional cleanup call.
Retention decision
Automatic retirement is the requested policy for this follow-up, replacing #1779's interim preservation of legacy files for manual inspection. There is no per-host opt-out or per-file callback; deletion is reported through aggregate logs. This removes obsolete diagnostic artifacts, not account identities or other local state: account directory names and ordinary account data remain. Hosts should refresh any cached audit listing after a new runtime opens.
There is no date-based sunset: users can upgrade directly from older builds, restore old backups, or retry previously failed cleanup. Removing this scan later requires an explicit change to those upgrade/restore guarantees.
Deletion boundary
<root>/accounts/<account-directory>/and<root>/.wipe-tombstones/<account-remnant>/, including signed-out accounts, failed wipes, and directories with missing/unreadable account records. Keep the historical namespaces fixed inside the leased root; never follow a mismatchedAccountHomeinto an unleased root.audit-<32-lowercase-hex-engine-id>.jsonlandaudit-<32-lowercase-hex-engine-id>-v1/v2/v3.jsonlfilename forms, including their padded numeric-seg<index>.jsonlsegments. Match the historical naming convention without reading sensitive payloads, so corrupted or truncated legacy files can also be removed.audit-key-reveal.jsonl, databases, device IDs, checkpoint sidecars, filenames outside the reserved legacy forms, and temporary files. A custom file that collides with a reserved legacy name is also deleted. Skip symlinks at every scan level and do not recurse into account subdirectories.The v4 upload gate remains unchanged and blocks any legacy files that could not be removed. This does not introduce general v4 retention, change the schema/bindings, bump a release version, or delete server data.
Validation
just fast-ci.The permission-failure test runs on unprivileged hosts; it skips that scenario if the test process can bypass directory permissions. It exercised the failure/retry path locally.