diff --git a/CHANGELOG.md b/CHANGELOG.md index dbfb22c7..2f769f8e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -114,6 +114,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **`drive sheets format-cells`/`update-borders`/etc. and the `structure` verbs no longer orphan a `pending` audit record when building the request fails after the lease gate** ([#1688](https://github.com/rust-works/omni-dev/issues/1688), [ADR-0080](docs/adrs/adr-0080.md) §11): `gate_leased_write` fsyncs the write-ahead `pending` record and only the mutating call's own `allowed`/`failed` outcome concludes it, but `format.rs`/`structure.rs` built their `batchUpdate` request (`parse_hex_color`, a resolved sheet's missing `sheetId`) *after* the gate — so a request that could never be issued (e.g. `update-borders --color ZZZZZZ` under an otherwise-valid `--lease`) still opened a `pending` record with nothing to close it, indistinguishable from a process that died mid-write. Both engines now build the request before the gate, matching `protection.rs`'s existing shape (`validation.rs`/`sheets/write.rs`/`docs/write.rs`/`content_edit.rs` were never affected — each either builds first already or has no fallible step between the gate and its mutating call). One accepted side effect: a malformed request under a bad or absent lease now reports the request-build refusal rather than the lease refusal, which is the ordering both engines already use ahead of the gate for every other pre-lease refusal. - **`drive lease acquire` no longer spends a Touch ID prompt and a real backup on a file that's already leased, and the backup this fixes** ([#1690](https://github.com/rust-works/omni-dev/issues/1690)): the only live-lease check used to live inside `insert_record`, *after* the prompt and the full backup — the normal outcome of any second `acquire` on an already-leased file (a "did I already lease this?" retry, a lost token), not merely a rare race, and the just-taken backup was neither deleted nor recorded in any ledger row, so `drive lease prune` ([#1678](https://github.com/rust-works/omni-dev/issues/1678), which only ever iterates ledger rows) could never reclaim it. A new lock-free `live_lease_for_file` pre-check now runs before authenticating at all, refusing the common case for free; the ledger lock itself is deliberately *not* held across the prompt (`LedgerLock::acquire` is a non-blocking, machine-wide `create_new` lock, so holding it across a 120s human prompt would fail every other concurrent `drive lease` operation), so `insert_record`'s own lock-held check remains the sole authoritative gate for the pre-check's narrow remaining race. Belt-and-braces for that race, and for the other outcomes that can still follow a real backup (a post-backup `files.get` failure, a missing post-backup `version`, or `insert_record` itself failing against a concurrently-held lock): the backup this attempt took is now reclaimed (deleted/trashed, via the same `clear_backup` `drive lease prune` uses) on every exit but `Acquired`, and a reclamation failure is both warned and audited with a `-backup-orphaned` verdict suffix naming the surviving backup's location, rather than silently orphaning it as before. - **`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. - **`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/Cargo.lock b/Cargo.lock index 62be3445..dd385baf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3034,6 +3034,7 @@ dependencies = [ "regex", "reqwest", "rmcp", + "same-file", "schemars", "serde", "serde_json", diff --git a/Cargo.toml b/Cargo.toml index fc783c28..0d292b87 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -40,6 +40,10 @@ termcolor = "1.1" anyhow = "1.0" chrono = { version = "0.4", features = ["serde"] } tempfile = "3.27" +# Cross-platform file-identity comparison (inode on unix, a file handle on +# Windows) for request_log.rs's audit-file collision guard (issue #1694) — +# already in the dependency graph transitively via `walkdir`. +same-file = "1.0" reqwest = { version = "0.13", features = ["form", "json", "multipart", "stream"] } tokio = { version = "1.52", features = ["full"] } axum = "0.8" diff --git a/docs/adrs/adr-0080.md b/docs/adrs/adr-0080.md index 6b805ece..746bf939 100644 --- a/docs/adrs/adr-0080.md +++ b/docs/adrs/adr-0080.md @@ -662,6 +662,24 @@ and `-f/--follow` all apply unchanged, so `omni-dev log --audit --query `--audit`, since exemption from pruning is the point. This is a CLI-surface change and lands under `update-snapshots` in Phase 1. +The exemption is enforced against the **resolved file**, not merely the +`--audit` flag or the default path spelling (issue #1694): `prune` and +size-capped rotation both refuse outright when the path they were handed +resolves to the same file as `audit_file_path()` — via the `same-file` crate +(inode on unix, a file handle on Windows) where both sides exist, else by a +lexically-`..`-collapsed, canonicalized comparison — so a `OMNI_DEV_LOG_FILE` +naming `audit.jsonl` directly, via a `..` segment, or via a symlink can't be +pruned or rotated away either. `record_audit`'s own +collision guard uses the same file-identity comparison rather than a raw +path equality, for the same reason. One direction is deliberately left open: +a colliding `OMNI_DEV_LOG_FILE` can still cause a best-effort record to land +*in* `audit.jsonl` — only the destructive operations are guarded, since +silently dropping the user's request logging would be its own surprise, and +the next `record_audit` call still fails loudly on the same collision. +Separately, a `OMNI_DEV_AUDIT_LOG_FILE` pointed at one of rotation's own +*sibling* paths (`.1`, `.lock`, …) is not covered by this guard — +a contrived misconfiguration distinct from the reported one. + **Fail-closed inverts this codebase's usual ordering.** `request_log`'s existing writers are deliberately best-effort and non-blocking — a logging failure must never change the caller's exit code (module doc, diff --git a/docs/log.md b/docs/log.md index c293ea17..a3d29896 100644 --- a/docs/log.md +++ b/docs/log.md @@ -445,8 +445,8 @@ described: |---|---|---| | On a write failure | Swallowed; the command's exit code is unaffected | Propagated; the caller aborts the operation it was about to audit | | `OMNI_DEV_LOG_DISABLE=1` | Suppresses all writes | No effect | -| `omni-dev log prune` | Trims by age/size | Refuses `--audit` outright (`log prune does not support --audit`) | -| `OMNI_DEV_LOG_MAX_SIZE` rotation | Applies | Never applies, regardless of the setting | +| `omni-dev log prune` | Trims by age/size | Refuses `--audit` outright (`log prune does not support --audit`), **and** refuses any resolved log path that names the audit file — e.g. `OMNI_DEV_LOG_FILE` pointed at `audit.jsonl` directly, via a `..` segment, or via a symlink | +| `OMNI_DEV_LOG_MAX_SIZE` rotation | Applies | Never applies, regardless of the setting or how `OMNI_DEV_LOG_FILE` is spelled | | Path override | `OMNI_DEV_LOG_FILE` | `OMNI_DEV_AUDIT_LOG_FILE` | | Location when unset | `/omni-dev/log.jsonl` | `/omni-dev/audit.jsonl` | @@ -455,10 +455,18 @@ separately, on `prune` (`omni-dev log prune --audit`, always refused, above); placed before a subcommand name instead — `omni-dev log --audit prune ...` — it is refused rather than silently ignored, since a subcommand never consults it. `record_audit` also refuses to write if `OMNI_DEV_LOG_FILE` and -`OMNI_DEV_AUDIT_LOG_FILE` are configured to resolve to the same path: the two -sinks are siblings by default but not otherwise mutually exclusive by +`OMNI_DEV_AUDIT_LOG_FILE` are configured to resolve to the **same file** — +compared by file identity (the `same-file` crate — inode on unix, a file +handle on Windows — falling back to a normalized path when one side doesn't +exist yet), not merely by identical spelling, so a `..` segment, +a relative-vs-absolute spelling, or a symlink can't slip past the check: the +two sinks are siblings by default but not otherwise mutually exclusive by construction, so misconfiguration is caught at write time instead of -silently blending the fail-closed sink into the best-effort one. +silently blending the fail-closed sink into the best-effort one. That guard +only stops writes *into* `log.jsonl`; a colliding `OMNI_DEV_LOG_FILE` can +still cause a best-effort record to *land in* `audit.jsonl` (it just can no +longer be pruned or rotated away, per the table above) — the next +`record_audit` call still fails loudly, surfacing the misconfiguration. Every record carries `kind: "audit"` and, in `context`, an `integration` key (`"drive"` today, so a later integration's audit trail is additive rather diff --git a/src/request_log.rs b/src/request_log.rs index 28911bf2..6a823bc8 100644 --- a/src/request_log.rs +++ b/src/request_log.rs @@ -408,6 +408,138 @@ pub fn audit_file_path() -> Option { .or_else(default_audit_file_path) } +/// Collapses `.`/`..` components **syntactically**, with no filesystem +/// access — so `a/sub/../b` becomes `a/b` even when `sub` does not exist on +/// disk. [`normalize`] needs this pass before it ever calls +/// [`std::fs::canonicalize`]: canonicalizing a parent directory (its +/// fallback for a path that doesn't exist yet) itself requires that parent +/// to exist, and a `..` segment routed through a nonexistent intermediate +/// directory would otherwise never resolve — exactly the `..`-spelling +/// [`same_file()`] exists to catch. A leading `..` (or one immediately after +/// a root/prefix) has nothing to pop, so it is kept as-is. +fn lexically_normalize(path: &Path) -> PathBuf { + use std::path::Component; + + let mut out = PathBuf::new(); + for component in path.components() { + match component { + Component::CurDir => {} + Component::ParentDir => match out.components().next_back() { + Some(Component::Normal(_)) => { + out.pop(); + } + _ => out.push(".."), + }, + other => out.push(other.as_os_str()), + } + } + out +} + +/// Bounds how many symlinks [`normalize`] follows by hand, purely so a +/// symlink cycle can't loop forever; a real cycle already fails +/// `canonicalize` and is vanishingly unlikely to matter here, but the +/// budget makes termination structural rather than assumed. +const MAX_HAND_FOLLOWED_SYMLINK_HOPS: u32 = 8; + +/// Resolves `path` as close to a canonical form as possible without +/// requiring it to exist. Lexically collapses `.`/`..` first +/// ([`lexically_normalize`]), then tries [`std::fs::canonicalize`] (an +/// existing file, with every symlink in the chain followed); a target that +/// does not exist yet fails that outright, so a symlink is then followed by +/// hand — load-bearing for a `OMNI_DEV_LOG_FILE` symlink whose target +/// `audit.jsonl` has not been created yet, since a plain "canonicalize the +/// parent" would otherwise compare the link's own name against the target's +/// and answer "different" right up until the first write creates the target +/// through the link. Falls back to canonicalizing the parent and rejoining +/// the file name, and finally to the lexically-normalized path unchanged +/// when even that has nothing to canonicalize (`file_name()` is `None` for +/// `/`, `.`, a bare `..`, …). +/// +/// This only exists for [`same_file()`]'s not-yet-existing-target fallback — +/// when both paths already exist, `same_file::is_same_file` handles +/// symlinks (and Windows file identity) itself and this is never reached. +fn normalize(path: &Path) -> PathBuf { + normalize_with_budget(&lexically_normalize(path), MAX_HAND_FOLLOWED_SYMLINK_HOPS) +} + +/// The recursive body of [`normalize`]. `path` is always already lexically +/// normalized on entry (both the initial call and the recursive one below +/// maintain that). Each `if let Ok(..)` below falls through to the next, +/// weaker resolution strategy on any error — expected and unremarkable here +/// (`NotFound` is the common case, since this only runs once +/// `same_file::is_same_file` has already failed to resolve one side), so +/// none of them are worth logging. +fn normalize_with_budget(path: &Path, hops: u32) -> PathBuf { + if let Ok(canon) = std::fs::canonicalize(path) { + return canon; + } + if hops > 0 { + if let Ok(meta) = std::fs::symlink_metadata(path) { + if meta.file_type().is_symlink() { + if let Ok(target) = std::fs::read_link(path) { + let resolved = if target.is_absolute() { + target + } else { + path.parent() + .filter(|p| !p.as_os_str().is_empty()) + .unwrap_or_else(|| Path::new(".")) + .join(target) + }; + return normalize_with_budget(&lexically_normalize(&resolved), hops - 1); + } + } + } + } + let Some(file_name) = path.file_name() else { + return path.to_path_buf(); + }; + let parent = path + .parent() + .filter(|p| !p.as_os_str().is_empty()) + .unwrap_or_else(|| Path::new(".")); + match std::fs::canonicalize(parent) { + Ok(canon_parent) => canon_parent.join(file_name), + Err(_) => path.to_path_buf(), + } +} + +/// Whether `a` and `b` name the same file, robust to `..` segments, +/// relative-vs-absolute spellings, and symlinks — unlike a raw `PathBuf` +/// compare, which [`record_audit`]'s collision guard used to rely on and +/// which all three pass straight through. +/// +/// Delegates to the [`same_file`](mod@same_file) crate's `is_same_file` +/// first — real file-identity comparison (inode on unix, a file handle on +/// Windows), rather than a hand-rolled `(dev, ino)` check that would have +/// no non-unix equivalent. That call requires both paths to exist, though, +/// so it fails for the case that actually motivated this function: a +/// `OMNI_DEV_LOG_FILE` symlink whose target `audit.jsonl` has not been +/// created yet. [`normalize`] is the fallback for exactly that gap. +fn same_file(a: &Path, b: &Path) -> bool { + if let Ok(same) = same_file::is_same_file(a, b) { + return same; + } + normalize(a) == normalize(b) +} + +/// Whether `path` resolves to the same file as [`audit_file_path`] — the +/// check [`prune`] and [`append_with_rotation`] refuse on, so neither +/// destructive operation can be pointed at the fail-closed audit sink by a +/// `..`-spelled, relative, or symlinked `OMNI_DEV_LOG_FILE` (ADR-0080 §11). +/// +/// Both callers check this once, before reading/rewriting `path`, not +/// through a handle held across the whole operation — a symlink at `path` +/// repointed to the audit file in the narrow window between this check and +/// the subsequent read still slips through (an attacker able to time that +/// swap already needs write access to the log directory, i.e. this same +/// user's own account). This narrows that window, it does not close it — +/// the same "narrows, does not close" shape as +/// [`crate::daemon::paths::try_lock_file_exclusive`]'s own inode re-check. +fn resolves_to_audit_file(path: &Path) -> bool { + audit_file_path().is_some_and(|audit| same_file(path, &audit)) +} + /// Release builds have no per-thread override. #[cfg(not(test))] fn test_audit_file_override() -> Option { @@ -548,9 +680,15 @@ fn try_record(entry: &LogRecord) -> anyhow::Result<()> { /// keeps. /// /// Refuses outright if `entry.kind` isn't [`RecordKind::Audit`], or if -/// [`audit_file_path`] resolves to the same path as [`log_file_path`] (an env +/// [`audit_file_path`] resolves to the same file as [`log_file_path`] (an env /// override misconfiguration that would otherwise silently blend the -/// fail-closed sink into the best-effort, prunable one). +/// fail-closed sink into the best-effort, prunable one) — compared via +/// [`same_file()`], not a raw path equality, so a `..` segment, a +/// relative-vs-absolute spelling, or a symlink can't slip past the check. +/// This only stops the write *into* the request log; it does not stop the +/// reverse (`OMNI_DEV_LOG_FILE` naming the audit file lets best-effort +/// records land there too) — see [`prune`] and [`append_with_rotation`] for +/// the guards that protect the audit file itself. /// /// The appended line is `fsync`ed (`sync_data`, then the parent directory /// on unix so a freshly created file's entry is durable too) before this @@ -572,7 +710,7 @@ pub fn record_audit(entry: &LogRecord) -> anyhow::Result<()> { let path = audit_file_path().context("could not resolve the audit log file path")?; if let Some(log_path) = log_file_path() { ensure!( - path != log_path, + !same_file(&path, &log_path), "the audit log path resolves to the same file as the request log ({}); set \ OMNI_DEV_AUDIT_LOG_FILE and/or OMNI_DEV_LOG_FILE to distinct paths", path.display() @@ -844,10 +982,21 @@ fn rotate(path: &Path, keep_files: u32) -> anyhow::Result<()> { /// itself rotated — stat the log, rotate if this line would push a non-empty /// file past the cap, then append to the (possibly fresh) file. A rotation /// failure is logged at debug and the line is still appended (best effort). +/// +/// Never rotates when `path` [resolves to the audit +/// file](resolves_to_audit_file) — checked first, before the `.lock` +/// sibling below is created next to it — falling back to the plain +/// never-rotates append instead (ADR-0080 §11). The line is still appended: +/// this only withholds rotation, matching [`record`]'s best-effort, always +/// try to write it contract. #[cfg(unix)] fn append_with_rotation(path: &Path, line: &str, cfg: &RotationConfig) -> anyhow::Result<()> { use std::os::unix::fs::OpenOptionsExt; + if resolves_to_audit_file(path) { + return append_line_no_rotation(path, line); + } + let lock_path = sibling(path, ".lock"); let lock_file = std::fs::OpenOptions::new() .create(true) @@ -910,9 +1059,24 @@ pub struct PruneOutcome { /// atomically renamed over the original, so a reader never sees a half-written /// file. A missing log is a no-op; a no-change prune skips the rewrite (leaving /// the file's inode — and any concurrent appends — untouched). +/// +/// Refuses outright when `path` [resolves to the audit file](resolves_to_audit_file) +/// — regardless of who computed `path`, since `--audit`'s own refusal +/// (`src/cli/log/prune.rs`) only catches that one flag, not a +/// `OMNI_DEV_LOG_FILE` override spelled to name `audit.jsonl` directly, via +/// `..`, or via a symlink. Exemption from pruning is the point (ADR-0080 +/// §11); this applies even under `dry_run`, since `omni-dev log --audit` is +/// the supported way to read the file. pub fn prune(path: &Path, opts: &PruneOptions) -> anyhow::Result { use anyhow::Context as _; + anyhow::ensure!( + !resolves_to_audit_file(path), + "refusing to prune {}: it resolves to the audit log, which is exempt from pruning by \ + design (ADR-0080 §11); read it with `omni-dev log --audit` instead", + path.display() + ); + let data = match std::fs::read(path) { Ok(data) => data, Err(e) if e.kind() == std::io::ErrorKind::NotFound => { @@ -3552,6 +3716,108 @@ mod tests { assert!(parse_size(&"9".repeat(400)).is_err()); } + // --- same_file / normalize / lexically_normalize (issue #1694) --- + + #[test] + fn same_file_matches_identical_paths() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("audit.jsonl"); + assert!(same_file(&path, &path)); + } + + #[test] + fn same_file_matches_a_dot_dot_spelling_routed_through_a_nonexistent_directory() { + // `metadata` fails for the dotted spelling (it must traverse the + // nonexistent `sub` to apply `..`), forcing the fallback through + // `lexically_normalize`, which collapses the `..` with no filesystem + // access before canonicalizing what remains. + let dir = tempfile::tempdir().unwrap(); + let audit = dir.path().join("audit.jsonl"); + std::fs::write(&audit, "{}\n").unwrap(); + let dotted = dir.path().join("sub/../audit.jsonl"); + assert!(same_file(&audit, &dotted)); + } + + #[test] + fn same_file_matches_a_hard_link() { + let dir = tempfile::tempdir().unwrap(); + let original = dir.path().join("audit.jsonl"); + std::fs::write(&original, "{}\n").unwrap(); + let linked = dir.path().join("linked.jsonl"); + std::fs::hard_link(&original, &linked).unwrap(); + assert!(same_file(&original, &linked)); + } + + #[cfg(unix)] + #[test] + fn same_file_matches_a_symlink_to_an_existing_file() { + let dir = tempfile::tempdir().unwrap(); + let original = dir.path().join("audit.jsonl"); + std::fs::write(&original, "{}\n").unwrap(); + let link = dir.path().join("link.jsonl"); + std::os::unix::fs::symlink(&original, &link).unwrap(); + assert!(same_file(&original, &link)); + } + + #[cfg(unix)] + #[test] + fn same_file_matches_a_symlink_to_a_not_yet_existing_target() { + // `fs::metadata` follows symlinks, so a link whose target does not + // exist yet fails both `metadata` calls; the fallback must resolve + // the link by hand rather than comparing the link's own name against + // the target's, which would answer "different" right up until the + // first write creates the target through the link. + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("audit.jsonl"); // never created + let link = dir.path().join("link.jsonl"); + std::os::unix::fs::symlink(&target, &link).unwrap(); + assert!(same_file(&target, &link)); + } + + #[test] + fn same_file_rejects_two_distinct_existing_files() { + let dir = tempfile::tempdir().unwrap(); + let a = dir.path().join("a.jsonl"); + let b = dir.path().join("b.jsonl"); + std::fs::write(&a, "a").unwrap(); + std::fs::write(&b, "b").unwrap(); + assert!(!same_file(&a, &b)); + } + + #[test] + fn same_file_rejects_two_distinct_paths_that_both_do_not_exist() { + let dir = tempfile::tempdir().unwrap(); + let a = dir.path().join("a.jsonl"); + let b = dir.path().join("b.jsonl"); + assert!(!same_file(&a, &b)); + } + + #[test] + fn lexically_normalize_collapses_dot_dot_without_touching_disk() { + assert_eq!( + lexically_normalize(Path::new("a/sub/../b")), + PathBuf::from("a/b") + ); + assert_eq!( + lexically_normalize(Path::new("./a/./b")), + PathBuf::from("a/b") + ); + // A leading `..` has nothing to pop, and must be kept rather than + // dropped or turned into an error. + assert_eq!( + lexically_normalize(Path::new("../a")), + PathBuf::from("../a") + ); + } + + #[test] + fn normalize_returns_the_lexical_form_unchanged_when_there_is_nothing_left_to_canonicalize() { + // An empty path has no file name and does not canonicalize; the + // defensive fallback must return it unchanged rather than panicking + // on a `None` `file_name()`. + assert_eq!(normalize(Path::new("")), PathBuf::new()); + } + #[test] fn prune_surfaces_a_read_error() { // Reading a directory as the log yields an error other than NotFound, @@ -3568,6 +3834,106 @@ mod tests { assert!(result.is_err()); } + #[test] + fn prune_refuses_the_audit_file_by_direct_path() { + let dir = tempfile::tempdir().unwrap(); + let _audit = crate::test_support::AuditLogGuard::redirect(dir.path()); + let audit_path = dir.path().join("audit.jsonl"); + std::fs::write(&audit_path, "{}\n").unwrap(); + + let err = prune( + &audit_path, + &PruneOptions { + older_than: None, + max_size: Some(1), + dry_run: false, + }, + ) + .err() + .unwrap(); + + assert!(err.to_string().contains("audit log"), "{err}"); + assert_eq!(std::fs::read_to_string(&audit_path).unwrap(), "{}\n"); + } + + #[test] + fn prune_refuses_a_dot_dot_spelling_of_the_audit_file() { + let dir = tempfile::tempdir().unwrap(); + let _audit = crate::test_support::AuditLogGuard::redirect(dir.path()); + let audit_path = dir.path().join("audit.jsonl"); + std::fs::write(&audit_path, "{}\n").unwrap(); + let dotted = dir.path().join("sub/../audit.jsonl"); + + let err = prune( + &dotted, + &PruneOptions { + older_than: None, + max_size: Some(1), + dry_run: false, + }, + ) + .err() + .unwrap(); + + assert!(err.to_string().contains("audit log"), "{err}"); + assert_eq!(std::fs::read_to_string(&audit_path).unwrap(), "{}\n"); + } + + #[test] + fn prune_refuses_the_audit_file_even_under_dry_run() { + // Exemption from pruning is the point, and `omni-dev log --audit` is + // the supported way to read the file — a read-only dry run is not a + // carve-out. + let dir = tempfile::tempdir().unwrap(); + let _audit = crate::test_support::AuditLogGuard::redirect(dir.path()); + let audit_path = dir.path().join("audit.jsonl"); + std::fs::write(&audit_path, "{}\n").unwrap(); + + let err = prune( + &audit_path, + &PruneOptions { + older_than: None, + max_size: Some(1), + dry_run: true, + }, + ) + .err() + .unwrap(); + + assert!(err.to_string().contains("audit log"), "{err}"); + } + + #[cfg(unix)] + #[test] + fn append_with_rotation_refuses_to_rotate_the_audit_file() { + let dir = tempfile::tempdir().unwrap(); + let _audit = crate::test_support::AuditLogGuard::redirect(dir.path()); + let audit_path = dir.path().join("audit.jsonl"); + // Seed a file already over the cap so a request-log path would rotate. + std::fs::write(&audit_path, "0123456789012345\n").unwrap(); + let cfg = RotationConfig { + max_size: 5, + keep_files: 1, + }; + + append_with_rotation(&audit_path, "new-line\n", &cfg).unwrap(); + + assert!( + !sibling(&audit_path, ".1").exists(), + "the audit file must never be rotated" + ); + assert!( + !sibling(&audit_path, ".lock").exists(), + "a refused rotation must not create a lock file next to the audit file" + ); + assert!( + std::fs::read_to_string(&audit_path) + .unwrap() + .contains("new-line"), + "the line is still appended (best effort)" + ); + } + #[cfg(unix)] #[test] fn append_with_rotation_appends_even_when_rotate_fails() {