docs(codex): log M6 recheck-2 fixes (R2-P1-1..R2-P2-4; round-3 user-granted, closes after recheck-3)

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

Author: pmaxhogan

design/CODEX_NOTES.md

@@ -779,3 +779,27 @@ This is the FINAL fix round (recheck cap = 2); all 8 are fixed below.
 | R1-P2-2 (`sources.rs`, DESIGN s5.2.2) | Overlapping / nested source roots were not rejected; `add_source` canonicalised the new path but never compared it to existing roots. | New `reject_overlapping_root` canonicalises every existing `backup_sources.local_path` and rejects (stable `local.io_error`) when the candidate is an ancestor of, descendant of, or identical to any existing root (applied GLOBALLY per DESIGN, which does not scope it per-account); siblings are allowed. Checked BEFORE master-key generation so an overlap never provisions a key. Test: nested + ancestor + identical rejected, sibling allowed. |
 | R1-P2-3 (`stores/setup.ts`) | Leaving the encryption step always called `createFirstSource()`; going Back from confirm then Next again re-called it, but the one-shot folder token was already consumed -> the wizard wedged. | `createFirstSource` is now idempotent: it short-circuits when `sourceId` is already set (preserving the staged phrase + ack). Test: a second `createFirstSource` does NOT re-call `add_source` and does not error. |
 | R1-P2-4 (`CredentialsWalkthrough.vue`, DESIGN s6.1) | The UI required a non-empty client secret, but the backend + DESIGN allow an empty secret for PKCE installed-app clients. | `canSubmit` now requires only a non-empty client ID; the (possibly empty) secret is passed through. Tests: submit allowed + the empty secret forwarded with a client ID; still blocked with no client ID. |
+
+## M6 codex recheck-2 fixes (round 3: 4 P1 + 4 P2 - USER-GRANTED exception; M6 CLOSES after recheck-3)
+
+The codex recheck-2 (baseline 3af8fc8, M6 @ 71efe9c) raised 4 P1 + 4 P2. Round 3
+is a ONE-TIME exception past the normal recheck cap=2 (the user explicitly
+approved it for these specific findings, analogous to M5's recheck-3). After this
+push, codex RECHECK-3 runs and the M6 review CLOSES regardless - there is no
+recheck-4. All 8 are fixed below. A new SPEC s24 code `internal.invalid_input`
+(`ErrorCode::InvalidInput`, with its `en-US` i18n entry + the tray red-error
+classification) was added for backend-side input-validation rejections (R2-P1-3 +
+R2-P2-3).
+
+| Finding | What was broken | How it was fixed |
+|---|---|---|
+| R2-P1-1 (data-safety, `sources.rs` + `sqlite.rs`) | Two concurrent `add_source` on an account whose `encryption_master_key_id` was still NULL could BOTH generate DIFFERENT master keys into the same keychain slot and wrap different source keys; SQLite then unconditionally stamped -> one source permanently unrestorable (its `wrapped_source_key` under a master key no longer in keychain). | BOTH defenses, per the spec: (1) a per-account async `tokio::Mutex` in `AppState` (`ensure_master_key_lock(account)`) held across the ENTIRE first-encrypted critical section (ensure-master-key -> stamp -> insert) - and the account master-key state is RE-READ inside the lock, so a losing-race second add observes the key the winner installed and wraps under the SAME key (`newly_generated=false`). (2) The SQL stamp is now a COMPARE-AND-SET: `UPDATE accounts SET encryption_master_key_id=? WHERE id=? AND encryption_master_key_id IS NULL`; on 0 rows it reads the current value and treats a same-key stamp as idempotent but a DIFFERENT-key stamp as a hard error (transaction rolled back, source NOT inserted) so a divergent key can never be committed. Tests: AppState lock is shared per-account / distinct across accounts / serialises a critical section; sqlite CAS rejects a divergent concurrent key (first key preserved, divergent source not persisted) and is idempotent for the same key (both sources persist under one key). |
+| R2-P1-2 (regression from round-2, `sources.rs` + `assembly.rs` + `app_state.rs`) | The fake Drive picker and the fake orchestrator built DIFFERENT `InMemoryRemoteStore` instances, so a root folder id the picker minted was invisible to the uploader -> fake-mode setup made an unusable source. | `AppState` now holds a SHARED per-account fake-remote-store registry (`FakeRemoteStores = Arc<Mutex<HashMap<AccountId, InMemoryRemoteStore>>>`, get-or-create). `assembly::build_and_spawn` builds the registry BEFORE the account loop, threads it into `build_account`/`build_remote` (the orchestrator's fake store comes from it), then MOVES it into `AppState`; `spawn_account` (hot path) reads it from the running `AppState`; `select_picker_store` returns `AppState::fake_remote_store(account)`. `InMemoryRemoteStore` is `Clone` over a shared `Arc<Mutex>`, so every clone sees the same objects. Test: fake pick -> the uploader store creates a folder under the picker's root id -> the picker store lists it (round-trips the parent id in one shared store). |
+| R2-P1-3 (`sources.rs` + `exclude.rs`) | include/exclude patterns were persisted with NO backend validation (only `preview_exclusions` validated, which callers can skip or patch around); an invalid/oversized glob then failed at scan-setup and stopped that source's backups. | New `driven_core::exclude::validate_patterns(include, exclude)` enforces max count per side (`MAX_PATTERNS_PER_SIDE`), max length per pattern (`MAX_PATTERN_LEN`), non-empty, and COMPILES each with the SAME `GitignoreBuilder` the scanner uses (`exclude` verbatim, `include` as its `!`-re-include form). Wired into BOTH `add_source` (request patterns, before any key gen) and `update_source` (the post-patch EFFECTIVE patterns) via `validate_source_patterns`, mapped to `internal.invalid_input`. Tests (`exclude.rs`): valid accepted; over-count, over-length, blank, and an uncompilable glob (trailing `\`) rejected on both sides. |
+| R2-P1-4 (`settings.rs`, SPEC s18) | Diagnostic redaction was whitespace-token-based and only caught tokens that START with an absolute path, so `path=C:\Users\Pat Smith\Taxes\f.pdf`, quoted paths, paths with spaces, and UNC paths leaked user paths/filenames into the exported bundle. | Rewrote redaction as a `Redactor` (built once per bundle from the DB source roots + `USERPROFILE`/`HOME` + `USERNAME`). Per line: (1) EXACT case-insensitive substring scrub of known source roots (longest-first) + home dir + username (handles their spaces); (2) an ABSOLUTE-PATH-RUN scanner that detects Windows drive / UNC / Unix-abs starts at a left boundary and consumes embedded spaces when QUOTED (to the matching quote) or after `key=` (to the next `key=value` field), while a bare path stops at the first space (so trailing prose / an adjacent email is not swallowed); (3) the residual token scrub (OAuth tokens / emails / drive-ids). No new dep (hand-written scanner, not regex). Tests: `key=path with spaces`, quoted-with-spaces, UNC, and a configured source-root substring are all scrubbed; ordinary non-path text (incl. a lone `/`) is unchanged. |
+| R2-P2-1 (`accounts.rs` + `assembly.rs`, BYO-only, SPEC s11.1 / DESIGN s6.1) | The backend shipped + fell back to a baked-in default Google client id, so a direct IPC call could start OAuth with no submitted creds. | Removed the `DEFAULT_CLIENT_ID` fallback from BOTH the wizard session (`resolve_creds` now returns `CommandResult` and REJECTS with `auth.consent_required` when no BYO id is submitted AND no env override is set; `start_oauth_signin` requires it before marking the session started; `finish_add_account` requires it too) and the assembly refresh path (`resolve_oauth_creds` is env-only now). The `DRIVEN_OAUTH_CLIENT_ID`/`_SECRET` env vars are KEPT solely as the test/e2e injection seam (the `google_e2e` suite lives in `driven-drive` and injects creds directly, so it is unaffected). Test: `resolve_creds` rejects when no creds, resolves to the submitted BYO creds otherwise. |
+| R2-P2-2 (`accounts.rs`) | `finish_add_account` `take()`-consumed the session tokens before all persistence succeeded, and stored keychain creds before the account row; a DB insert failure made the session unreplayable and orphaned creds. | The session tokens are now READ by `clone` (not `take`) and the session is removed ONLY on full success, so a failed finish stays replayable. Fresh-add persistence is extracted into `persist_new_account` over an `AccountSecretStore` trait (real impl over the keychain): it stores token -> creds -> row, rolling back EVERY prior keychain write if a later step fails, so a forced row-insert failure leaves NO orphaned keychain entries. Tests: a forced row-insert failure rolls back both keychain entries (and returns the error); the happy path keeps both; a clone leaves the session's tokens intact (replayable). |
+| R2-P2-3 (`settings.rs`, SPEC s22) | Settings IPC accepted unchecked numeric/enum values; a buggy/compromised renderer could persist zero/huge intervals, invalid log level/channel/locale/vss_mode, etc. | Added backend validators run BEFORE `store_group`: numeric ranges for scan interval, deep-verify interval, bandwidth cap (when set), concurrency override (1..=32, SPEC s22), and update-check interval; enum checks for `io_priority`, `log_level`, updater `channel`, `color_mode`, `tray_left_click_opens`, `vss_mode`; and a BCP-47-shape check for `locale`. Out-of-range / invalid -> `internal.invalid_input`. Tests: out-of-range numeric + invalid enum + malformed locale rejected, valid accepted. |
+| R2-P2-4 (`settings.rs` + `state/mod.rs` + `sqlite.rs`, SPEC s18) | `schema.txt` only counted `accounts` + `backup_sources`. | New authoritative `KNOWN_STATE_TABLES` (every migration-defined table: accounts, backup_sources, file_state, file_state_fts, pending_ops, activity_log, settings, file_checksum_mismatch) + a `StateRepo::table_row_count(table)` method (allow-list guarded, since a table name cannot be a bound parameter). `build_schema_summary` now counts EVERY table. Tests: schema.txt contains a count line for every known table incl. file_state + pending_ops. |
+
+Cross-cutting: backend/frontend contracts stayed in sync (the only UI change is the new `errors.internal.invalid_input` `en-US` locale entry; the DTO shapes are unchanged). The new sqlx `query!` (CAS SELECT) regenerated the workspace `.sqlx` offline cache (0 drift). All gates green: `cargo build/clippy(-D warnings)/test --workspace`, `build -p driven-app`, `deny check`, `fmt --check`; `pnpm lint/test:unit/build` (vue-tsc clean). Anti-fake-green stub sweep on the M6 non-test surface: zero `todo!`/`unimplemented!`/`unreachable!` (the planner/scanner `unimplemented!()` are pre-existing `#[cfg(test)]` FakeStateRepo doubles).