fix(startup) [BRNS-DESK-025]: the data guard sees the wipe, so settings.json is restored - #77
fix(startup) [BRNS-DESK-025]: the data guard sees the wipe, so settings.json is restored#77sebastian-ssvlabs wants to merge 1 commit into
Conversation
…gs.json is restored startup_guard computed `existed` itself, but the logger's file target creates ~/.brains/logs/ ~320 lines earlier in run(), so the check always said true and the `if !existed` settings auto-restore branch was unreachable dead code. run() now samples data_dir().exists() as its first filesystem touch and threads the boot fact into startup_guard(); the guard's decision logic moves to run_guard(), which takes the data dir, the backups root and the boot facts as arguments so it is testable without a real home dir (home_dir() reads getpwuid, not $HOME). Wipe detection semantics are unchanged.
nir-ssvlabs
left a comment
There was a problem hiding this comment.
The fix is real rather than nominal, which was the thing worth checking: let data_dir_existed = storage::data_dir().exists() sits at lib.rs:141, genuinely ahead of the logger init at 145 that creates the dir, and data_dir() is a pure path join with no create_dir anywhere in it — so the capture observes the pre-boot state instead of manufacturing it. The description's "keeps the logger first" reads as if the capture came after; it doesn't, and the comment at the capture site states the invariant it depends on.
Taking the parameter with no fallback is the right call and worth saying so: exactly one call site exists (lib.rs:468), and a silent dir.exists() default would have re-created the original bug for the next caller while looking defensive. Extracting run_guard to take paths as arguments is what makes any of this testable given home_dir() resolves through getpwuid() and ignores $HOME.
Checked: the capture's position against the logger, data_dir() for side effects, the signature for a fallback, and that startup_guard has a single call site. Not read: the restore path itself — unchanged here; this PR only fixes whether it is reached.
Merge: ✅ into main once Rust (Windows) lands — it was still pending at review time.
stefan-ssv-labs
left a comment
There was a problem hiding this comment.
✅ review-pr: clean against BRNS-DESK-025 — the pre-logger existence capture, restore path, regression tests, and normal-boot guard are present.
Summary
~/.brains(app cleaner, DMG swap — the 2026-07-12 incident), the guard's second defence now actually fires:settings.jsonis restored from the newest~/.brains-backupssnapshot instead of the user silently starting over with defaults.if !existedrestore branch was unreachable dead code:startup_guardsampleddir.exists()itself, and by then the logger had already created~/.brains/logs/. Soexistedwas alwaystrue— which is why the repro log readsexisted=trueon a dir that had just been moved away.run()now takes the dir-existed observation as its first filesystem touch and threads it intostartup_guard(); the guard's logic moved into arun_guard()that takes the paths and boot facts as arguments, which is what makes it testable at all.Boot ordering — why
existedwas always truesrc-tauri/src/lib.rs~/.brainsTeeLog { file: open_log_file() }(run(), top)create_dir_all(data_dir()/logs)— creates the dir.setup()→startup_guard(version)let existed = dir.exists()→ alwaystrueOnly the
!existedterm was dead. Wipe detection still worked through the marker/runs half offresh(!marker.exists() && runs_count <= 1), which is why the recovery prompt kept appearing while the settings restore never ran — the log line in the ticket is both symptoms in one message.The fix keeps the logger first (boot diagnostics need it) and keeps the marker/runs term (dropping it would change wipe detection for partial wipes that leave the marker). It only moves when the fact is observed:
let data_dir_existed = storage::data_dir().exists();as the first statement ofrun(), with a comment stating that invariant at the capture site.startup_guardgained the parameter with no local fallback — the plan allowed one "if some call site genuinely boots without the early capture", and there is exactly one call site (main.rs→run()→setup()). A silentdir.exists()fallback would just reintroduce the bug for any future caller that forgot.Two smaller things the ticket did not ask for, both needed to land the tests it did ask for:
run_guard(dir, backups, existed, prior_evidence, app_version) -> GuardReport— pure w.r.t. paths. Necessary becausestorage::home_dir()resolves throughgetpwuid()and ignores$HOME, so a test cannot point the guard at a temp dir by env var. The home-dependent work (claude_home_sessions_exist, the recoverable-session count) stays instartup_guard, so the tests touch nothing outside their temp dir.GuardReport.existedis now reported (and surfaces in thedata_guard_statuscommand /RUST_LOG=debugdiagnostics). The ticket's test asks to assert "the reported existed is false", and the report had no such field; it is also the value you want visible when reproducing this class of bug. Purely additive for the frontend, which reads the report asany.Verification
Ran locally:
cargo fmt --manifest-path src-tauri/Cargo.toml— clean, and the pre-commit hook's Rust format check passed.git diffreviewed line by line; no frontend file is touched (src/routes/+page.sveltealready consumes the report untyped), so prettier/eslint had nothing to run against.std::env::temp_dir()+process::id()prefix, 20 ms sleep for mtime ordering, sameremove_dir_allteardown) and checked by inspection.Only CI can prove:
cargo testwas NOT run locally — this checkout has no Rust build environment (no cargo target dir, no.svelte-kit), socargo test --manifest-path src-tauri/Cargo.tomlin CI is the first real compile of these two tests:wiped_data_dir_restores_settings_from_newest_backup— dir absent + two backup snapshots →existed == false,settings_restored == true,fresh_but_prior_use == true, and the restored file is the newest snapshot's.normal_boot_does_not_restore_settings— dir present with marker + matching.app-version→ nothing restored, livesettings.jsonuntouched, no false recovery flag.cargo clippyandnpm run checkwere not run (clippy is warn-mode inci.yml).mv ~/.brains away, launch,grep data-guard,ls -a ~/.brains) was not performed — it needs a GUI app build. The acceptance criterion is exercised at the unit level instead; the one link the tests do not cover isrun()passing its capture through tosetup(), which is the three-line change inlib.rs.CI result (added after opening)
All checks green on
f144011. Both new tests pass on ubuntu, macOS and Windows, so the compile-by-inspection above is now confirmed by a real build on all three targets.The first Windows run failed on
commands::printing::tests::stash_then_take_returns_the_document_once, which this PR does not touch — those tests share a globalPRINT_JOBand race under the parallel test runner. A rerun of that job alone passed with no code change.Note
No conflict with PR #74 (
fix/desk-053-portable-backup-names, targetingdev): different branch and tree, and none of its backup-naming changes are imported here. This branch touches the restore path and the boot ordering only — backup directory names are read by mtime, not parsed.