diff --git a/CHANGELOG.md b/CHANGELOG.md index 12576cb6..7de576be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -117,6 +117,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **`drive lease restore` no longer duplicates a sheet it has already restored** ([#1689](https://github.com/rust-works/omni-dev/issues/1689), [ADR-0080](docs/adrs/adr-0080.md) §10): re-running restore on a token whose deleted-sheet restore had already succeeded silently copied the sheet in *again*, one "Copy of …" per run, each costing a Touch ID prompt and a fresh Drive backup copy. `spreadsheets.sheets.copyTo` assigns the destination a fresh `sheetId`, so the backup sheet's own id stayed missing from the live spreadsheet and the structural detection ADR-0080 §10 relies on kept firing; the pre-write recheck compared against that same original id rather than the one the previous restore created, so it passed too. Live spreadsheet state alone cannot settle it — a restored sheet is structurally indistinguishable from a live sheet the user happened to give the backup sheet's title, and §10 deliberately restores *through* the latter — so the ledger row now records `restored_sheet_id`, the live id each restore creates, and a repeat run is refused (`sheet-already-restored`) **before** the authentication prompt and the fresh backup copy, naming the sheet and the still-live lease the earlier restore minted. The guard keys on that id still being live rather than on `restored_at` being set, so re-deleting the restored sheet and re-running legitimately restores it again; the recheck immediately before the write applies the same test, for a concurrent restore landing during the up-to-two-minute prompt window. The `Bytes` restore path was already idempotent and is unchanged. `restored_sheet_id` is additive — ledgers written by earlier builds keep loading. - **The audit log's exemption from `log prune`/rotation now holds against the *resolved* file, not just the `--audit` flag or the default path spelling** ([#1694](https://github.com/rust-works/omni-dev/issues/1694), [ADR-0080](docs/adrs/adr-0080.md) §11): `omni-dev log prune` refused `--audit` outright, but nothing stopped `OMNI_DEV_LOG_FILE` from being pointed at `audit.jsonl` directly, via a `..` segment, or via a symlink — the obvious workaround, which let `log prune` or `OMNI_DEV_LOG_MAX_SIZE` rotation destroy the fail-closed audit trail ADR-0080 §11 promises is unprunable. `request_log::prune` and size-capped rotation now both refuse outright when the path they were handed resolves to the audit file, compared by **file identity** (the [`same-file`](https://docs.rs/same-file) crate — inode on unix, a file handle on Windows — where both sides exist, else a lexically-`..`-collapsed, canonicalized path comparison so a not-yet-created symlink target is still caught) rather than a raw path or spelling. `record_audit`'s own collision guard (refusing to write when `OMNI_DEV_AUDIT_LOG_FILE` and `OMNI_DEV_LOG_FILE` name the same file) is upgraded to the same file-identity comparison, closing the same `..`/symlink gap there. This only guards the destructive operations — a colliding `OMNI_DEV_LOG_FILE` can still cause a best-effort record to land in `audit.jsonl`, which the next `record_audit` call still catches and fails loudly on. - **A relative `drive lease` `--backup-dir` is now absolutized before it's stored in the ledger** ([#1692](https://github.com/rust-works/omni-dev/issues/1692)): a relative value from `--backup-dir`, `OMNI_DEV_DRIVE_LEASE_BACKUP_DIR`, or `settings.json`'s `lease.backup_dir` was stored **relative** in the ledger row, so a later `drive lease prune`/`restore` run from a different cwd would resolve it against the wrong directory. `prune`'s `clear_backup` would get `NotFound`, treat the backup as already gone, and drop the ledger row while reporting `bytes_freed` — leaving the real backup on disk with no row pointing at it; `restore` from the other cwd would fail outright with "Failed to read backup". `resolve_backup_dir` (`src/drive/lease/settings.rs`) now routes every non-default source through a new `absolutize()` helper (`std::path::absolute`, touching neither the filesystem nor requiring the directory to exist), so the ledger always stores an absolute path regardless of the caller's cwd. The hard-coded default (already absolute, under the state/data directory) is unaffected. +- **A `drive lease` command with a broken `settings.json` now warns instead of silently falling back to defaults** ([#1695](https://github.com/rust-works/omni-dev/issues/1695)): `LeaseFlags::resolve` called `Settings::load().unwrap_or_default()`, so a `settings.json` that failed to parse silently dropped every `lease.*` setting — including `biometrics_only`, whose default is the *less*-secure direction, so a broken config could silently downgrade the auth policy with no indication anything was wrong. A parse failure now logs a `tracing::warn!` naming the error and which settings are being ignored before falling back to defaults. - **`gmail sync-all`'s rate-limit retry notices no longer corrupt the live progress bars or go unattributed** ([#1651](https://github.com/rust-works/omni-dev/issues/1651)): the shared retry loop (`retry_if`, `src/utils/http.rs`) printed a raw `eprintln!` while waiting out a 429/403, so on a `sync-all` run — where every account's bars share one `indicatif::MultiProgress` ([#1504](https://github.com/rust-works/omni-dev/issues/1504)) — a rate limit on any account teared the render and named no account, leaving no way to tell which one was throttled. `retry_if`/`retry_429` gain an optional `notify` callback (`RetryNotifyFn`) invoked in place of the `eprintln!`; Atlassian, Datadog and Drive keep the default fallback unchanged (they pass `None`), while `GmailClient` exposes `set_retry_notify` so `gmail sync`/`sync-all` can register one once a progress channel exists. The notice now renders as a transient message on the throttled account's own fetch bar — already prefixed with the account label in `sync-all` — via a new `SyncProgressEvent::RateLimited` event, and clears itself once that fetch's retry resolves. - **`worktrees ui`'s embedded-terminal shutdown could still hang forever, this time on the kernel rather than a child process** ([#1611](https://github.com/rust-works/omni-dev/issues/1611)): [#1605](https://github.com/rust-works/omni-dev/issues/1605)/[#1610](https://github.com/rust-works/omni-dev/issues/1610) bounded the child-side wait (a SIGHUP-ignoring shell) by reaping the whole process group with escalation before joining the PTY thread — but `TerminalTab::shutdown` ran that join, and the `Pty` drop inside it, on `tokio::task::spawn_blocking`. Nobody ever awaited that task, yet `tokio::Runtime::drop()` still blocks until every outstanding blocking-pool task finishes, so anything that made the join-then-drop take unboundedly long could still wedge a shutdown — reintroducing the exact class of bug #1605 fixed, from a new cause. Live `lldb` debugging of a hung process (reading the actual `pid` argument off the blocked `wait4()` syscall, confirmed on two independent reproductions) found that cause: a `TabKind::Shell` tab's child is `/usr/bin/login` on macOS, and a killed `login` process can land in the kernel's own exit teardown (`ps` reports state `Es+`) and never finish it — the SIGKILL genuinely lands, but the reap that would collect the resulting zombie simply never completes. Nothing in-process can bound a stuck kernel-side teardown, so `shutdown` now joins the PTY thread and drops the `Pty` on a plain, detached `std::thread` instead of `spawn_blocking` — untracked by the runtime, so a stuck reap can no longer hold up `Runtime::drop`, whether in a test's per-test runtime or the real CLI's in `main.rs`. This was a real bug, not a test artifact: `shutdown` also runs on ordinary app quit, so a user's own Shell tab could in principle wedge `omni-dev worktrees ui` on exit the same way. Verified with a 20-run soak of `cargo test --lib worktrees::ui` at default parallelism (0 hangs, versus roughly 2 in 3 runs hanging beforehand); a new test pins the structural property the fix relies on — a detached thread that never finishes cannot block a runtime's drop, unlike a `spawn_blocking` task, which an adjoining `#[ignore]`d test documents by demonstrating the hang it *would* cause if run. diff --git a/src/cli/drive/lease.rs b/src/cli/drive/lease.rs index b280854e..1f97839c 100644 --- a/src/cli/drive/lease.rs +++ b/src/cli/drive/lease.rs @@ -122,8 +122,18 @@ impl LeaseFlags { // and `Settings::load_lease()` each independently re-read it; // `SettingsEnv::from_settings` was added for exactly this (issue // #1533), so both views come from the same parse (issue #1677 review - // finding). - let loaded = Settings::load().unwrap_or_default(); + // finding). A parse failure warns rather than silently falling back + // (issue #1695) — `biometrics_only`'s default is the less-secure + // direction, so a broken settings.json must not silently downgrade + // it. + let loaded = Settings::load().unwrap_or_else(|e| { + tracing::warn!( + "{e:#}; falling back to default settings for this invocation — any \ + `lease.*` config in settings.json (backup_dir, default_expiry_minutes, \ + biometrics_only, allow_headless) is being ignored" + ); + Settings::default() + }); let lease = loaded.lease.clone(); let profile = crate::utils::settings::active_profile_from(&crate::utils::env::SystemEnv); let env = SettingsEnv::from_settings(loaded, profile.as_deref()); @@ -868,6 +878,68 @@ mod tests { ); } + /// Thread-scoped log buffer, mirroring the `CaptureWriter`/`capture_info` + /// pattern in `src/gmail/chrome_profile.rs`. + #[derive(Clone, Default)] + struct CaptureWriter(std::sync::Arc>>); + + impl std::io::Write for CaptureWriter { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.0.lock().unwrap().extend_from_slice(buf); + Ok(buf.len()) + } + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } + } + + impl<'a> tracing_subscriber::fmt::MakeWriter<'a> for CaptureWriter { + type Writer = Self; + fn make_writer(&'a self) -> Self::Writer { + self.clone() + } + } + + /// Runs `f` under a thread-local WARN-level subscriber and returns + /// everything it logged. `f` must be fully synchronous on this thread. + fn capture_warn(f: impl FnOnce()) -> String { + let writer = CaptureWriter::default(); + let subscriber = tracing_subscriber::fmt() + .with_max_level(tracing::Level::WARN) + .with_ansi(false) + .with_writer(writer.clone()) + .finish(); + tracing::subscriber::with_default(subscriber, f); + let logs = String::from_utf8_lossy(&writer.0.lock().unwrap()).into_owned(); + logs + } + + #[test] + fn resolve_warns_and_falls_back_when_settings_json_fails_to_parse() { + // A missing settings.json resolves to defaults with no warning + // (the ordinary case, covered implicitly by every other test in + // this module) — this covers the "file exists but doesn't parse" + // case (issue #1695), which must warn rather than silently drop + // `lease.biometrics_only`/`backup_dir`/`default_expiry_minutes`. + let guard = crate::drive::test_support::EnvGuard::take(); + let dir = guard.clear_credentials(); + let settings_dir = dir.path().join(".omni-dev"); + std::fs::create_dir_all(&settings_dir).unwrap(); + std::fs::write(settings_dir.join("settings.json"), "{not valid json").unwrap(); + + let flags = LeaseFlags { + backup_dir: None, + expiry_minutes: None, + biometrics_only: false, + allow_headless: false, + }; + let logs = capture_warn(|| { + let resolved = flags.resolve().unwrap(); + assert_eq!(resolved.auth_policy, AuthPolicy::DeviceOwner); + }); + assert!(logs.contains("settings.json"), "{logs}"); + } + #[test] fn print_result_does_not_panic_for_any_variant() { for result in [