diff --git a/FINDINGS.md b/FINDINGS.md new file mode 100644 index 0000000..ada6d1c --- /dev/null +++ b/FINDINGS.md @@ -0,0 +1,101 @@ +# Implementation Findings + +Confirmed, high-impact work only. Keep changes inside `crates/storage` unless a +finding explicitly names another crate. Prefer the smallest shared fix and add +one focused regression test per behavior change. + +## 1. Preserve operation and rollback failures + +Rollback code can replace the original failure with a restoration failure: + +- `Repository::rollback_created` in `crates/storage/src/repository.rs` +- `TreeMutator::remove` and `TreeMutator::transfer` in + `crates/storage/src/mutation.rs` + +Return an error containing both failures, following the existing +`WriteError::RollbackFailed { operation, rollback }` pattern. Cover failures +from reopening/syncing directories and from all source/destination restoration +steps. Never report successful rollback if any restoration step failed. + +Acceptance: focused tests prove that the original operation error and rollback +error are both retained. + +## 2. Use one snapshot per tree mutation + +Directory remove/move/copy currently scan the repository up to five times via +`snapshot`, `reject_unmanaged`, `source_entries`, `source_policies`, and +`source_directories` in `crates/storage/src/mutation.rs`. + +Take one snapshot at operation start and pass it to those helpers. All +resolution, validation, and source selection for the transaction must use that +same snapshot. + +Acceptance: existing mutation tests pass and a focused test or instrumentation +proves one snapshot is taken for a directory operation. + +## 3. Remove string-based HTTPS Git error classification + +`map_reqwest_error` and `fetch_embedded_https` in +`crates/storage/src/git.rs` classify authentication, TLS, and network failures +using substrings from third-party error messages. + +Use structured `reqwest` status/connect/timeout information and matchable `gix` +error variants wherever available. Keep a bounded textual diagnostic only as a +fallback; it must not drive a more specific classification unless the library +exposes no structured alternative. + +Acceptance: tests classify HTTP 401/403, TLS failure, connection/timeout, and an +unknown protocol failure without depending on English error text. + +## 4. Make config replacement durable + +`Config::persist` and `persist_new` in `crates/storage/src/config.rs` sync the +file but not the parent directory after replacement/creation. + +Sync the parent directory after the atomic rename, reusing the repository's +existing durability pattern where practical. Return a distinct durability error +if the rename succeeds but the directory sync fails. + +Acceptance: focused tests cover the post-rename sync failure boundary. Do not +add config locking unless concurrent writers are an explicit supported use case. + +## 5. Do not hold secret-store state across backend I/O + +`SecretStore` in `crates/storage/src/secret_store.rs` holds its mutex while +calling potentially blocking backend operations (`unlock`, `lock`, `create`, +`retrieve`, `replace`, and `delete`). + +Reduce the lock scope around backend calls while preserving atomic lock-state +and cache behavior. Do not weaken biometric/keychain protection or allow a +secret operation to complete as unlocked after a concurrent `lock()`. + +Acceptance: concurrency tests cover `lock()` racing with at least retrieval and +mutation, and prove no stale secret is inserted into the cache. + +## 6. Reap completed TUI task handles + +`AsyncExecutor::submit` in `apps/tui/src/runtime.rs` retains every `JoinHandle` +until executor drop. + +Remove and join finished handles during normal executor activity. Keep the +current thread-per-task design; do not add a thread pool without measured need. + +Acceptance: a focused test submits and completes repeated tasks and verifies +the retained handle count does not grow without bound. + +## Required checks + +```sh +cargo fmt --all -- --check +RUSTFLAGS="-D warnings" cargo check --workspace --all-targets +cargo clippy --workspace --all-targets -- -D warnings +cargo test --workspace +``` + +## Explicitly deferred + +Do not spend this pass on `thiserror`, broad error-enum redesign, Git options +structs, frontend file splitting, generic generation tracking, public struct +fields, UniFFI mirror removal, Git performance caching, config locking, or a +thread pool. Revisit only with a concrete bug, compatibility need, or measured +performance problem.