fix(app,ui): M6 codex round-1 - account/OAuth lifecycle, live crypto + recovery-phrase reveal, dialog-token path validation + diagnostics

Fixes all 8 P1 + 3 P2 findings from .claude/codex-reviews/M6-20260624-011401.md.
CI/Chaos were green but the wizard/account/source/crypto lifecycle had real
end-to-end gaps the mocked unit tests did not exercise.

Theme A - account / OAuth lifecycle:
- A1: persist per-account BYO OAuth client creds in the keychain
  (ClientCredsStore) so refresh works after restart; load everywhere a
  RefreshingTokenSource is built; delete on remove_account.
- A2: AppState.accounts behind a sync Mutex with insert/remove; assembly
  spawn_account hot-spawns the orchestrator after finish_add_account so the
  wizard's initial sync_now finds a live handle (no restart).
- A3: reauth_account returns { sessionId, authUrl }; the UI completes re-consent
  onto the existing account (no duplicate) and hot-spawns it.
- A4: only the frontend opens the consent URL (backend no longer double-opens).
- A5: request userinfo scopes + fetch the real Google email/display name.

Theme B - source / crypto / recovery-phrase:
- B1: pick_drive_folder returns the concrete root id "root" so setup can select
  a destination (incl. My Drive root).
- B2: KeystoreCryptoProvider resolves LIVE (refreshable source map on
  AccountHandle.crypto, refreshed by reconfigure_account); fail-closed preserved.
- B3: the recovery phrase is a one-time RETURN VALUE on AddSourceResult, shown
  once via RecoveryPhraseReveal AFTER the source/key exists, with Finish/Done
  gated on an explicit ack; never an unrestorable encrypted backup.

Theme C - settings / diagnostics / path security:
- C1: backend-owned native dialogs (pick_folder_dialog / pick_save_zip_dialog)
  mint one-shot path tokens (SPEC s11.6.1); add_source + export validate the
  token -> path binding and reject any untrusted path.
- C2: export writes a real .zip FILE at the save-dialog path.
- C3: diagnostic bundle now includes activity_last_30d.csv, logs/, crashes/,
  the redaction pipeline, and the real PRAGMA user_version (new
  StateRepo::schema_version).

Tests EXERCISE each fix: backend #[cfg(test)] (crypto refresh, dialog tokens,
client-creds round-trip, userinfo parse, activity CSV redaction, schema
version), src-tauri/tests/ipc_path_validation.rs (SPEC s11.6.1), and the vitest
wizard walk completes end-to-end against the fake remote (root selectable,
running-orchestrator sync_now, phrase-gated Finish, reauth sequence).

design/CODEX_NOTES.md M6 section records the per-finding table + the
Playwright-deferred-to-local note.

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

Author: pmaxhogan

crates/driven-core/src/orchestrator.rs

@@ -2096,6 +2096,9 @@ mod tests {
         async fn delete_activity_by_source(&self, _source: SourceId) -> anyhow::Result<u64> {
             unimplemented!()
         }
+        async fn schema_version(&self) -> anyhow::Result<i64> {
+            Ok(0)
+        }
         async fn get_setting(&self, key: &str) -> anyhow::Result<Option<serde_json::Value>> {
             Ok(self.settings.lock().unwrap().get(key).cloned())
         }

crates/driven-core/src/planner.rs

@@ -346,6 +346,9 @@ mod tests {
         async fn delete_activity_by_source(&self, _source: SourceId) -> Result<u64> {
             unimplemented!()
         }
+        async fn schema_version(&self) -> Result<i64> {
+            unimplemented!()
+        }
         async fn get_setting(&self, _key: &str) -> Result<Option<serde_json::Value>> {
             unimplemented!()
         }

crates/driven-core/src/scanner.rs

@@ -784,6 +784,9 @@ mod tests {
         async fn delete_activity_by_source(&self, _source: SourceId) -> anyhow::Result<u64> {
             unimplemented!()
         }
+        async fn schema_version(&self) -> anyhow::Result<i64> {
+            unimplemented!()
+        }
         async fn get_setting(&self, _key: &str) -> anyhow::Result<Option<serde_json::Value>> {
             unimplemented!()
         }

crates/driven-core/src/state/mod.rs

@@ -550,6 +550,12 @@ pub trait StateRepo: Send + Sync {
 
     // --- settings -----------------------------------------------------------
 
+    /// The schema version recorded in SQLite's `PRAGMA user_version`
+    /// (SPEC s18 diagnostic bundle `schema.txt`). Exposed on the object-safe
+    /// trait so the diagnostic-bundle command (which holds only `dyn StateRepo`)
+    /// can record the REAL schema version rather than "not exposed".
+    async fn schema_version(&self) -> Result<i64>;
+
     /// Reads a setting value (SPEC s22). Returns `None` if the key is
     /// absent. Values are JSON-typed per the schema's TEXT column.
     async fn get_setting(&self, key: &str) -> Result<Option<serde_json::Value>>;

crates/driven-core/src/state/sqlite.rs

@@ -1348,6 +1348,16 @@ impl StateRepo for SqliteStateRepo {
             .await
     }
 
+    async fn schema_version(&self) -> Result<i64> {
+        // PRAGMA returns a non-standard row shape the `query!` macro cannot
+        // describe, so use the dynamic query API (mirrors the wal_checkpoint
+        // call). `user_version` is an i64-typed pragma column.
+        let row: (i64,) = sqlx::query_as("PRAGMA user_version;")
+            .fetch_one(&self.pool)
+            .await?;
+        Ok(row.0)
+    }
+
     // --- settings -----------------------------------------------------------
 
     async fn get_setting(&self, key: &str) -> Result<Option<Value>> {

crates/driven-drive/src/google/oauth.rs

@@ -46,6 +46,15 @@ const GOOGLE_TOKEN_URL: &str = "https://oauth2.googleapis.com/token";
 /// The Drive scope Driven requests (full Drive access; SPEC s4).
 const DRIVE_SCOPE: &str = "https://www.googleapis.com/auth/drive";
 
+/// The OpenID userinfo email scope (A5): lets Driven read the account's Google
+/// email via the userinfo endpoint so the Accounts UI / needs_reauth banner show
+/// the real address rather than a placeholder label.
+const USERINFO_EMAIL_SCOPE: &str = "https://www.googleapis.com/auth/userinfo.email";
+
+/// The OpenID userinfo profile scope (A5): lets Driven read the account's
+/// display name from the userinfo endpoint.
+const USERINFO_PROFILE_SCOPE: &str = "https://www.googleapis.com/auth/userinfo.profile";
+
 /// Connect timeout for the code-exchange client (DESIGN s5.8.4; codex V-A1).
 const EXCHANGE_CONNECT_TIMEOUT: Duration = Duration::from_secs(10);
 
@@ -138,6 +147,10 @@ pub async fn run_pkce_loopback_flow(
     let (auth_url, csrf_state) = client
         .authorize_url(CsrfToken::new_random)
         .add_scope(Scope::new(DRIVE_SCOPE.to_string()))
+        // A5: request the userinfo email + profile scopes so the account's real
+        // Google email + display name can be fetched from the userinfo endpoint.
+        .add_scope(Scope::new(USERINFO_EMAIL_SCOPE.to_string()))
+        .add_scope(Scope::new(USERINFO_PROFILE_SCOPE.to_string()))
         // `access_type=offline` + `prompt=consent` force Google to mint a
         // refresh token (otherwise re-auth yields only an access token).
         .add_extra_param("access_type", "offline")

crates/driven-drive/src/google/token_store.rs

@@ -47,6 +47,13 @@ const TARGET: &str = "driven::drive::token";
 /// keyring "service" namespace for Driven Google refresh tokens (SPEC s4.1).
 const KEYRING_SERVICE: &str = "driven.google.refresh_token";
 
+/// keyring "service" namespace for Driven per-account BYO OAuth client
+/// credentials (A1 / DESIGN s6.1). A refresh token is bound to the OAuth client
+/// that minted it, so the account's `client_id` + `client_secret` MUST persist
+/// alongside the refresh token - otherwise a restart falls back to the
+/// env/default client and every BYO-client refresh fails (`invalid_client`).
+const KEYRING_CLIENT_CREDS_SERVICE: &str = "driven.google.client_creds";
+
 /// Google's OAuth token endpoint (SPEC s4.1 refresh path).
 const GOOGLE_TOKEN_URL: &str = "https://oauth2.googleapis.com/token";
 
@@ -114,6 +121,91 @@ impl KeyringTokenStore {
     }
 }
 
+/// A persisted per-account BYO OAuth client credential pair (A1).
+///
+/// `client_secret` is empty for a PKCE installed-app client (no real secret).
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct ClientCreds {
+    /// The OAuth client id that minted the account's refresh token.
+    pub client_id: String,
+    /// The OAuth client secret (empty for a PKCE installed-app client).
+    pub client_secret: String,
+}
+
+/// Keychain wrapper for an account's BYO OAuth client credentials (A1 / DESIGN
+/// s6.1). One entry per account, in the [`KEYRING_CLIENT_CREDS_SERVICE`]
+/// namespace, keyed by account id. The two fields are stored as a single
+/// newline-separated record (`client_id\nclient_secret`) so one keychain entry
+/// holds the pair. The secret is NEVER logged.
+pub struct ClientCredsStore {
+    account: String,
+}
+
+impl ClientCredsStore {
+    /// Builds a client-creds store scoped to `account` (the keychain lookup key
+    /// within the Driven client-creds namespace).
+    pub fn new(account: impl Into<String>) -> Self {
+        Self {
+            account: account.into(),
+        }
+    }
+
+    /// Opens the keychain entry for this account's client creds.
+    fn entry(&self) -> anyhow::Result<Entry> {
+        Entry::new(KEYRING_CLIENT_CREDS_SERVICE, &self.account)
+            .map_err(|e| anyhow::anyhow!("keychain: failed to open client-creds entry: {e}"))
+    }
+
+    /// Persists `creds` for this account (A1). Stored as
+    /// `client_id\nclient_secret` in one keychain entry.
+    pub fn store(&self, creds: &ClientCreds) -> anyhow::Result<()> {
+        let record = encode_client_creds(creds);
+        self.entry()?
+            .set_password(&record)
+            .map_err(|e| anyhow::anyhow!("keychain: failed to store client creds: {e}"))
+    }
+
+    /// Loads the stored client creds, or `None` if the account never persisted
+    /// any (a default/env-client account). A `NoEntry` maps to `Ok(None)`.
+    pub fn load(&self) -> anyhow::Result<Option<ClientCreds>> {
+        match self.entry()?.get_password() {
+            Ok(record) => Ok(Some(decode_client_creds(&record))),
+            Err(keyring::Error::NoEntry) => Ok(None),
+            Err(e) => Err(anyhow::anyhow!(
+                "keychain: failed to load client creds: {e}"
+            )),
+        }
+    }
+
+    /// Deletes the stored client creds (e.g. on account removal). Absent entry
+    /// is a no-op.
+    pub fn delete(&self) -> anyhow::Result<()> {
+        map_delete_result(self.entry()?.delete_credential())
+    }
+}
+
+/// Encode a [`ClientCreds`] pair as the single keychain record
+/// `client_id\nclient_secret`. Pure so it is unit-testable without a keychain.
+fn encode_client_creds(creds: &ClientCreds) -> String {
+    format!("{}\n{}", creds.client_id, creds.client_secret)
+}
+
+/// Decode a keychain record (`client_id\nclient_secret`) into [`ClientCreds`].
+/// A record with no newline is treated as a bare client id (empty secret). Pure
+/// so it is unit-testable without a keychain.
+fn decode_client_creds(record: &str) -> ClientCreds {
+    match record.split_once('\n') {
+        Some((id, secret)) => ClientCreds {
+            client_id: id.to_string(),
+            client_secret: secret.to_string(),
+        },
+        None => ClientCreds {
+            client_id: record.to_string(),
+            client_secret: String::new(),
+        },
+    }
+}
+
 /// Maps a `keyring` `get_password` result to the load-token domain result:
 /// `Ok(pw) -> Ok(Some(pw))`, `Err(NoEntry) -> Ok(None)`, other `Err -> Err`.
 /// Pure so it is unit-testable without a keychain.
@@ -401,6 +493,38 @@ mod tests {
         assert_eq!(r, Some("tok".to_string()));
     }
 
+    #[test]
+    fn client_creds_encode_decode_round_trips() {
+        // A1: a BYO client id + secret round-trips through the single keychain
+        // record (`client_id\nclient_secret`).
+        let creds = ClientCreds {
+            client_id: "byo-id.apps.googleusercontent.com".to_string(),
+            client_secret: "byo-secret".to_string(),
+        };
+        let record = encode_client_creds(&creds);
+        assert_eq!(decode_client_creds(&record), creds);
+    }
+
+    #[test]
+    fn client_creds_decode_tolerates_pkce_empty_secret() {
+        // A PKCE installed-app client has an empty secret; the record is
+        // `id\n` and decodes to an empty secret.
+        let creds = ClientCreds {
+            client_id: "pkce-id".to_string(),
+            client_secret: String::new(),
+        };
+        let record = encode_client_creds(&creds);
+        assert_eq!(decode_client_creds(&record), creds);
+        // A bare record with no newline is treated as a client id, empty secret.
+        assert_eq!(
+            decode_client_creds("bare-id"),
+            ClientCreds {
+                client_id: "bare-id".to_string(),
+                client_secret: String::new(),
+            }
+        );
+    }
+
     #[test]
     fn map_load_no_entry_is_none() {
         let r = map_load_result(Err(keyring::Error::NoEntry)).unwrap();

design/CODEX_NOTES.md

@@ -729,3 +729,35 @@ yet. No deferral-by-typed-error was needed: both M6 commands have real bodies.
   Rust `preview_exclusions(req: ExclusionPreviewRequest)` signature). The test
   assertion was corrected to the real `{ req: { localPath } }` contract (a
   contract fix, not a weakening).
+
+## M6 codex review round-1 fixes (8 P1 + 3 P2)
+
+Source review `.claude/codex-reviews/M6-20260624-011401.md` (baseline 3af8fc8,
+M6 @ 80c2452): CI + Chaos were GREEN but the wizard/account/source/crypto
+lifecycle had real end-to-end gaps the mocked unit tests did not exercise. All
+11 findings fixed; new tests EXERCISE each fix (backend `#[cfg(test)]` +
+`src-tauri/tests/ipc_path_validation.rs` + vitest the wizard now completes
+end-to-end against the fake remote).
+
+| Finding | What was broken | How it was fixed |
+|---|---|---|
+| P1-1 (B1) | Setup could not pick a Drive destination - `pick_drive_folder` echoed `current_folder_id: None` at root, so `setup.driveFolderId` was never set. | `pick_drive_folder` now resolves `None` -> the concrete Drive root alias `"root"` AND echoes it back as `current_folder_id`, so the user can select the current folder (incl. My Drive root). `add_source` accepts `"root"`. Test: `pick_drive_folder` root-listing mock + the wizard walk select the root id. |
+| P1-2 (A2) | A newly added account had no running orchestrator until restart, so the wizard's initial `sync_now(sourceId)` failed "no running orchestrator". | `AppState.accounts` moved behind a sync `Mutex<HashMap<_, Arc<AccountHandle>>>` with `insert_account`/`remove_account_handle`; assembly's per-account build factored into `assembly::spawn_account(app, &AppState, id)`, called by `finish_add_account` to hot-spawn + insert the handle (mirroring the M5 no-orphan drain - a prior handle is shut down first). Tests: `dialog_token`/handle bookkeeping + the vitest wizard walk hits a running-orchestrator mock for `sync_now`. |
+| P1-3 (A1) | BYO `client_id`/`client_secret` lived only in the in-memory wizard session; only the refresh token persisted, so after restart refresh fell back to env/default creds and FAILED for every BYO account (silent broken-account data loss). | New `ClientCredsStore` (keychain namespace `driven.google.client_creds`) persists the per-account client creds on `finish_add_account`; loaded everywhere a `RefreshingTokenSource` is built (`assembly::resolve_account_oauth_creds` used by boot `build_remote` + `pick_drive_folder` + reauth); deleted on `remove_account`. Secret never logged. Tests: `ClientCreds` encode/decode round-trip. |
+| P1-4 (A3) | Reauth created a hidden session and expected `finish_add_account`, but the UI only received `authUrl` and never the session id, so reauth never completed. | `reauth_account` now returns `ReauthSession { sessionId, authUrl }` (seeded with the account's stored client creds, A1); the UI opens the URL, listens `oauth:complete`, then `completeReauth(sessionId)` -> `finish_add_account` re-stores the new token onto the EXISTING account (no duplicate) + flips it back to `ok` + hot-spawns it. Tests: accounts-store `reauth` + `completeReauth`. |
+| P1-5 (B3) | The BIP39 recovery phrase was emitted as a transient event the UI never subscribed to; setup rendered the reveal BEFORE the source (empty phrase) and the confirm checkbox could be ticked with no phrase shown - so the app could create ENCRYPTED BACKUPS THE USER CAN NEVER RESTORE. | The phrase is now a ONE-TIME RETURN VALUE: `add_source` returns `AddSourceResult { source, recoveryPhrase }` (Some only when this opt-in generated the master key). `ensure_master_key` encodes the phrase BEFORE stamping the row and HARD-ERRORS (rolling back the key) if it cannot encode - never an unrestorable source. The UI shows the phrase via `RecoveryPhraseReveal` AFTER the source/key exists (setup confirm step; add-source a post-confirm reveal step) and gates Finish/Done on an explicit ack that is only enableable once a real phrase was displayed. Tests: store + vitest assert phrase returned, displayed, Finish disabled until acked. |
+| P1-6 (B2) | The crypto provider snapshotted source rows at assembly; `reconfigure_account` only updated orchestrator config. So an encrypted source added/toggled while running failed CLOSED (no row -> Unavailable) until restart. | `KeystoreCryptoProvider.sources` moved behind a `Mutex` with `refresh(sources)` that swaps the live map AND invalidates cache entries whose crypto fields changed/vanished; the provider Arc is held on `AccountHandle.crypto`, and `reconfigure_account` reads the account's current rows and refreshes it after every source add/update/remove. Fail-closed preserved (missing key -> Unavailable, never plaintext). Tests: refresh picks up a new encrypted source (was unknown->Plaintext, now Unavailable), toggles invalidate cache, removal drops to Plaintext. |
+| P1-7 (C1) | SPEC s11.6.1 requires dialog-derived paths; the impl took raw webview strings and fabricated a token from the untrusted parent. | The BACKEND now OWNS the dialogs: `pick_folder_dialog` / `pick_save_zip_dialog` (tauri-plugin-dialog Rust API via a oneshot) return `{ path, token }`; `AppState` holds a one-shot, TTL-bounded `token -> path` binding (`mint_dialog_token`/`take_dialog_token`). `add_source` takes `localPathToken` and `export_diagnostic_bundle` takes `token`; each resolves the path from the token (single-use) and REJECTS a path with no matching token, then runs `validate_writable_dest` (canonicalize / no-dotdot / no-symlink-leaf / confine-to-dialog-root / atomic). Frontend calls the backend dialogs. Tests: `src-tauri/tests/ipc_path_validation.rs` (traversal, symlink-at-leaf, non-existent parent, outside-root reject, valid) + `dialog_token` single-use/TTL. |
+| P1-8 (C2) | About asked for a DIRECTORY and passed it as `dest`; the backend then renamed a temp ZIP over the directory path -> always failed. | `pick_save_zip_dialog` returns a concrete `.zip` FILE path (suggested name + zip filter); `export_diagnostic_bundle` resolves it from the token and `atomic_write`s the ZIP AT that file. Test: the path-validation IT writes + reads back a real archive at the confined dest; About uses `pickSaveZipDialog`. |
+| P2-1 (C3) | The diagnostic bundle omitted `activity_last_30d.csv`, `logs/`, `crashes/`, and wrote "user_version not exposed". | Added `StateRepo::schema_version()` (real `PRAGMA user_version`); `build_diagnostic_zip` now adds `activity_last_30d.csv` (30-day activity, message+source hashed), `logs/` + `crashes/` from `<config>/driven/logs` through a redaction pipeline (`redact_log_text`: tokens -> `<token-redacted>`, paths -> `<path:hash>`, emails -> `<email:hash>`, drive-id-shaped -> `<fileid:hash>`), and the real `user_version`. Tests: schema summary has real `user_version`, activity CSV header + redacts message, redaction-pipeline unit tests. |
+| P2-2 (A4) | The consent URL was opened twice (backend `start_oauth_signin` AND frontend). | The backend opener closure now ONLY captures the URL for the return value (no `open_system_browser`); the FRONTEND is the single owner that opens it (add-account + reauth). |
+| P2-3 (A5) | Account email was a user label / `account-<id>`, not the Google email. | OAuth now requests the `userinfo.email`+`userinfo.profile` scopes; `finish_add_account` fetches `oauth2/v3/userinfo` (text + serde_json, no `json` reqwest feature) with the fresh access token and persists the real email + display name (fallback to a label on failure, never a fabricated address). Tests: userinfo parse (with + without name). |
+
+### Playwright deferred-to-local (CI uses vitest for the wizard walk)
+
+SPEC's end-to-end wizard coverage is exercised in CI by the vitest jsdom walk
+(`setup-wizard.test.ts` drives welcome -> credentials -> source -> encryption ->
+confirm against the fake backend, including the B3 phrase-gated Finish and the
+C1 backend folder dialog). A real Playwright/WebDriver run against the built
+Tauri app is deferred to a local pre-release check (no headless Tauri WebDriver
+in the Windows-only PR gate); the vitest walk is the CI proxy.