fix(app): M6 recheck-1 backend - atomic add_source rollback, preview token, fake picker, fatal creds, boot config, overlap

R1-P1-1: add_source now splits master-key prep (generate keychain key +
encode phrase, no account stamp) from the atomic DB write; on a DB
failure when a key was just generated it deletes the keychain master key
so the account stays unprovisioned and a retry re-reveals the phrase.
R1-P1-2: preview_exclusions no longer walks a raw webview path - the DTO
takes a dialog token (resolved via a new non-consuming
AppState::peek_dialog_token so add_source keeps its single-use TAKE) XOR
a source_id (path resolved from SQLite); neither/bad-token is rejected.
R1-P1-3: pick_drive_folder honours remote_mode via select_picker_store -
Fake builds an InMemoryRemoteStore (no real store, no keychain), Real
builds the live GoogleDriveStore.
R1-P1-4: store_client_creds is FATAL (returns CommandResult); fresh-add
rolls back the stored refresh token on creds failure, reauth persists
creds before flipping the account to ok - no account that cannot refresh.
R1-P2-1: build_account loads the persisted OrchestratorConfig at cold
start instead of ::default(), so saved settings apply without a live edit.
R1-P2-2: add_source rejects a root overlapping (ancestor/descendant/
identical to) any existing source root per DESIGN s5.2.2; siblings allowed.

Tests: peek non-consuming + TTL, overlap nested/ancestor/identical
rejected + sibling allowed, fake picker lists without creds, cold-start
config reflects persisted non-default settings.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012CyiRqk2DVwmJjEu5gcD1m

Author: pmaxhogan

src-tauri/src/app_state.rs

@@ -316,6 +316,22 @@ impl AppState {
         Some(binding.path)
     }
 
+    /// C1 / R1-P1-2: PEEK (non-consuming) the path bound to `token`, if it
+    /// exists and has not expired. Unlike [`Self::take_dialog_token`] this does
+    /// NOT consume the token, so a read-only, idempotent, repeatable command
+    /// (`preview_exclusions`, which the user re-runs as they tweak globs) can
+    /// resolve the dialog-derived path without spending the single use the
+    /// subsequent `add_source` write needs. The TTL still bounds replay; only a
+    /// path-bearing WRITE consumes the token.
+    pub fn peek_dialog_token(&self, token: &str) -> Option<std::path::PathBuf> {
+        let map = self.lock_dialog_tokens();
+        let binding = map.get(token)?;
+        if std::time::Instant::now().duration_since(binding.minted_at) >= DIALOG_TOKEN_TTL {
+            return None;
+        }
+        Some(binding.path.clone())
+    }
+
     /// Lock the dialog-token map, recovering a poisoned lock.
     fn lock_dialog_tokens(&self) -> std::sync::MutexGuard<'_, HashMap<String, DialogTokenBinding>> {
         self.dialog_tokens.lock().unwrap_or_else(|e| e.into_inner())
@@ -862,6 +878,32 @@ mod tests {
         let _ = std::fs::remove_dir_all(dir);
     }
 
+    #[tokio::test]
+    async fn peek_dialog_token_is_non_consuming() {
+        // R1-P1-2: `preview_exclusions` PEEKS the dialog token (non-consuming) so
+        // the user can re-run the preview as they tweak globs AND the subsequent
+        // `add_source` still has the single TAKE it needs. Peeking N times then
+        // taking once must all resolve the same path; a take after that is
+        // rejected.
+        let (state, dir) = temp_state().await;
+        let app_state = AppState::new(state, HashMap::new(), RemoteMode::Fake);
+        let path = std::path::PathBuf::from("/home/u/preview-root");
+        let token = app_state.mint_dialog_token(path.clone());
+
+        // Multiple peeks all resolve the path WITHOUT consuming the token.
+        assert_eq!(app_state.peek_dialog_token(&token), Some(path.clone()));
+        assert_eq!(app_state.peek_dialog_token(&token), Some(path.clone()));
+        // The single TAKE (what add_source uses) still works after the peeks.
+        assert_eq!(app_state.take_dialog_token(&token), Some(path));
+        // Now consumed: a further peek AND take both return None.
+        assert_eq!(app_state.peek_dialog_token(&token), None);
+        assert_eq!(app_state.take_dialog_token(&token), None);
+        // An unknown token never resolves.
+        assert_eq!(app_state.peek_dialog_token("nope"), None);
+
+        let _ = std::fs::remove_dir_all(dir);
+    }
+
     #[tokio::test]
     async fn dialog_tokens_are_distinct_per_mint() {
         // Two mints yield distinct tokens each bound to its own path.

src-tauri/src/assembly.rs

@@ -316,9 +316,15 @@ async fn build_account(
     };
 
     // --- pacer: real AIMD pacer seeded from the account's config -------------
-    // M5 uses the default orchestrator config (the persisted per-account
-    // settings UI is M6); the bandwidth cap is therefore unset here.
-    let config = OrchestratorConfig::default();
+    // R1-P2-1: load the PERSISTED SPEC s22 settings (scan cadence, bandwidth
+    // cap, metered/battery gates, VSS mode) so a cold start honours the user's
+    // saved settings, not the hard defaults. Before this fix the orchestrator
+    // always booted with `OrchestratorConfig::default()` and only picked up the
+    // persisted values after a live settings edit. A read/parse failure falls
+    // back to the conservative default (reconfigure-style best-effort).
+    let config = crate::commands::settings::load_orchestrator_config(state.as_ref())
+        .await
+        .unwrap_or_default();
     let pacer: Arc<dyn Pacer> = Arc::new(AimdPacer::with_ceilings(
         clock.clone(),
         config.bandwidth_cap_mbps.map(f64::from),
@@ -775,3 +781,66 @@ struct AccountSyncStatusEvent {
     account_id: String,
     state: driven_core::types::OrchestratorState,
 }
+
+#[cfg(test)]
+mod tests {
+    use driven_core::orchestrator::OrchestratorConfig;
+    use driven_core::state::sqlite::SqliteStateRepo;
+    use driven_core::state::StateRepo;
+
+    /// R1-P2-1: cold-start orchestrators must build their [`OrchestratorConfig`]
+    /// from the PERSISTED SPEC s22 settings, not the hard defaults. `build_account`
+    /// now reads `commands::settings::load_orchestrator_config` at assembly time
+    /// (replacing the old `OrchestratorConfig::default()`); this asserts that a
+    /// persisted NON-DEFAULT setting is reflected in the config that path yields,
+    /// so a fresh boot honours the user's saved settings without a live edit.
+    #[tokio::test]
+    async fn cold_start_config_reflects_persisted_non_default_setting() {
+        let nonce = std::time::SystemTime::now()
+            .duration_since(std::time::UNIX_EPOCH)
+            .map(|d| d.as_nanos())
+            .unwrap_or(0);
+        let dir = std::env::temp_dir().join(format!("driven-assembly-cfg-{nonce}"));
+        std::fs::create_dir_all(&dir).unwrap();
+        let repo = SqliteStateRepo::open(&dir.join("state.db"))
+            .await
+            .expect("open repo");
+
+        // The hard default scan cadence is 600s; persist a DISTINCT non-default
+        // value so a cold start that ignored persisted settings would fail this.
+        let default_cfg = OrchestratorConfig::default();
+        let persisted_scan_secs: u64 = 123;
+        assert_ne!(
+            default_cfg.scan_interval_secs, persisted_scan_secs,
+            "fixture must differ from the default to prove the persisted value wins"
+        );
+        let global = serde_json::json!({
+            "auto_start_on_login": false,
+            "default_concurrent_uploads": serde_json::Value::Null,
+            "bandwidth_cap_mbps": 7,
+            "skip_on_battery": false,
+            "skip_on_metered": false,
+            "scan_interval_secs": persisted_scan_secs,
+            "deep_verify_interval_secs": 604_800,
+            "io_priority": "low",
+            "log_level": "info",
+        });
+        repo.set_setting("global", &global)
+            .await
+            .expect("seed global");
+
+        // The EXACT function `build_account` reads at cold start.
+        let cfg = crate::commands::settings::load_orchestrator_config(&repo)
+            .await
+            .expect("load config");
+        assert_eq!(
+            cfg.scan_interval_secs, persisted_scan_secs,
+            "cold-start config must reflect the persisted scan cadence (R1-P2-1)"
+        );
+        assert_eq!(cfg.bandwidth_cap_mbps, Some(7));
+        assert!(!cfg.skip_on_battery);
+        assert!(!cfg.skip_on_metered);
+
+        let _ = std::fs::remove_dir_all(dir);
+    }
+}

src-tauri/src/commands/accounts.rs

@@ -415,10 +415,15 @@ pub async fn finish_add_account(
     let profile = fetch_google_userinfo(&tokens.access_token).await;
 
     let (account_id, dto) = if let Some(account_id) = reauth_account {
-        // Reauth path: re-store the refresh token + client creds + flip the
+        // Reauth path: re-store the refresh token + client creds, THEN flip the
         // existing account back to Ok, refreshing the profile if we got one.
+        // R1-P1-4: persisting the client creds is FATAL and happens BEFORE the
+        // account is flipped to Ok - if it fails the account stays in its prior
+        // (needs_reauth) state rather than being marked Ok with un-refreshable
+        // creds. The refresh token re-store is harmless to leave (the same
+        // account, same key) and is overwritten on the next successful reauth.
         store_refresh_token(account_id, &tokens.refresh_token)?;
-        store_client_creds(account_id, &creds);
+        store_client_creds(account_id, &creds)?;
 
         let rows = state
             .state()
@@ -452,9 +457,21 @@ pub async fn finish_add_account(
     } else {
         // Fresh add: allocate the id, store the token + client creds, write the
         // row with the real Google email/name (A5).
+        // R1-P1-4: the token AND client creds are persisted BEFORE the account
+        // row is written, and persisting the client creds is FATAL: an account
+        // whose BYO client creds did not persist could never refresh its own
+        // token (the refresh is bound to the minting client). If the creds store
+        // fails, roll back the just-stored refresh token so NO half-account
+        // (token without creds, or a row that cannot refresh) is left behind.
         let account_id = AccountId::new_v4();
         store_refresh_token(account_id, &tokens.refresh_token)?;
-        store_client_creds(account_id, &creds);
+        if let Err(err) = store_client_creds(account_id, &creds) {
+            if let Err(del) = KeyringTokenStore::new(account_id.to_string()).delete_refresh_token()
+            {
+                tracing::error!(target: TARGET, account_id = %account_id, error = %del, "failed to roll back refresh token after client-creds persist failure");
+            }
+            return Err(err);
+        }
 
         // A5: prefer the real Google email; else the user label; else a stable
         // fallback. The display name prefers the user-supplied label, else the
@@ -520,19 +537,29 @@ pub async fn finish_add_account(
     Ok(dto)
 }
 
-/// Persist the per-account BYO OAuth client creds in the keychain (A1).
-/// Best-effort: a keychain write failure is logged (NEVER the secret) but does
-/// not fail the finish - the account is already saved; a missing client-creds
-/// entry falls back to the env/default client on next refresh.
-fn store_client_creds(account_id: AccountId, creds: &(String, String)) {
+/// Persist the per-account BYO OAuth client creds in the keychain (A1; R1-P1-4).
+///
+/// FATAL, not best-effort: a refresh token is bound to the client that minted
+/// it, so an account whose client creds were NOT persisted will fail EVERY
+/// refresh after restart (it falls back to the env/default client, which did not
+/// mint the token -> `invalid_client`). `finish_add_account` therefore aborts +
+/// rolls the account back when this fails, rather than leaving behind an account
+/// that can never refresh its own token. The error maps to `crypto.key_missing`
+/// (the keychain-write failure class); the secret is NEVER logged or embedded.
+fn store_client_creds(account_id: AccountId, creds: &(String, String)) -> CommandResult<()> {
     use driven_drive::google::token_store::{ClientCreds, ClientCredsStore};
     let record = ClientCreds {
         client_id: creds.0.clone(),
         client_secret: creds.1.clone(),
     };
-    if let Err(e) = ClientCredsStore::new(account_id.to_string()).store(&record) {
-        tracing::warn!(target: TARGET, account_id = %account_id, error = %e, "failed to persist BYO client creds in keychain");
-    }
+    ClientCredsStore::new(account_id.to_string())
+        .store(&record)
+        .map_err(|e| {
+            CommandError::with_code(
+                ErrorCode::CryptoKeyMissing,
+                format!("failed to persist BYO OAuth client creds in keychain: {e}"),
+            )
+        })
 }
 
 /// The subset of the Google userinfo response Driven persists (A5).

src-tauri/src/commands/dtos.rs

@@ -246,14 +246,26 @@ pub struct DriveFolderListing {
 
 /// Request body for `preview_exclusions` (SPEC s11.2 `ExclusionPreviewRequest`).
 ///
-/// `local_path` MUST be a dialog-derived path (SPEC s11.6.1) - the preview
-/// walks the local tree, so the same untrusted-path rule applies as
-/// `add_source`.
-#[derive(Debug, Clone, Serialize, Deserialize)]
+/// R1-P1-2 (SPEC s11.6.1): the preview WALKS the local tree, so its root must
+/// never be a raw webview-supplied path (a compromised renderer could enumerate
+/// arbitrary readable directories). The root is resolved one of two safe ways:
+/// - a NEW candidate source: `local_path_token` is the one-shot dialog token
+///   `pick_folder_dialog` minted; the backend PEEKS (non-consuming, so the later
+///   `add_source` keeps its single use) the path bound to it;
+/// - an EXISTING source: `source_id` is the source's id; the backend resolves
+///   `backup_sources.local_path` from SQLite.
+///
+/// Exactly one of the two must be present; a request with neither (or a token
+/// that does not map to a backend dialog) is REJECTED.
+#[derive(Debug, Clone, Default, Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
 pub struct ExclusionPreviewRequest {
-    /// Dialog-derived absolute local path to preview (SPEC s11.6.1).
-    pub local_path: PathBuf,
+    /// One-shot dialog token for a NEW candidate source's folder (from
+    /// `pick_folder_dialog`); resolved via a non-consuming peek (R1-P1-2).
+    pub local_path_token: Option<String>,
+    /// An EXISTING source's id; its `local_path` is resolved from SQLite
+    /// (R1-P1-2). Mutually exclusive with `local_path_token`.
+    pub source_id: Option<String>,
     /// Whether `.gitignore` rules are honoured.
     pub respect_gitignore: bool,
     /// Candidate include globs.