Skip to content

fix(drive,docs): guard the audit log's exemption against a resolved OMNI_DEV_LOG_FILE - #1717

Merged
newhoggy merged 1 commit into
mainfrom
issue-1694-audit-log-path-param
Sep 17, 2026
Merged

newhoggy merged 1 commit into
mainfrom
issue-1694-audit-log-path-param

Conversation

@newhoggy

Copy link
Copy Markdown
Contributor

Summary

Test plan

  • cargo build
  • cargo test --lib (9505 passed)
  • cargo clippy --all-targets -- -D warnings
  • cargo doc --no-deps --document-private-items (the CI Docs job's exact command)
  • Reviewed via /code-review high. The original hand-rolled (dev, ino) file-identity check had no real equivalent on non-unix, silently weakening this exact guard on the one non-unix platform this project ships binaries for (Windows) — switched to the same-file crate (already in the dependency graph transitively via walkdir), which does real cross-platform file identity (inode on unix, a file handle on Windows), keeping the hand-rolled symlink-following logic only as a fallback for the not-yet-existing-target case same-file can't handle. Also extracted a magic literal to a named constant and documented the residual TOCTOU window between the check and the destructive read/rewrite (narrows it, does not close it — same shape as an existing inode re-check elsewhere in this codebase).

…MNI_DEV_LOG_FILE

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 Section 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 via the
same-file crate (inode on unix, a file handle on Windows) where both
sides exist, else a lexically-..-collapsed canonicalized comparison so a
not-yet-created symlink target is still caught. record_audit's own
collision guard is upgraded to the same file-identity comparison.

Fixes #1694.
@github-actions

Copy link
Copy Markdown

Coverage

Total: 97.27% ⚪ 0 pp vs main

Comparing 7312477..f373fb4 (merge-base → PR head)

File Before After Δ
src/request_log.rs 97.85% 98% 🟢 0.14 pp
🔇 5 ignored region(s), 0 tolerated region(s)

ignore removes the lines from both reports; tolerate keeps them in the reported percentage but scores them against the baseline, so a cross-run flip cannot move a delta. Regions are read from each revision's own source.

File Kind Lines Rev Reason
src/cli/drive/lease.rs ignore 408-412 both a sheet restore's fresh backup is always a DriveCopy — restore.rs's own recheck refuses unless the target is still a spreadsheet, and acquire() only ever takes a Bytes backup of a non-native target; this arm exists solely for exhaustiveness over the shared LeaseBackup enum
src/cli/drive/lease.rs ignore 928-931 both guards this test helper against misuse; every call site below passes an acquire subcommand
src/cli/drive/lease.rs ignore 942-945 both guards this test helper against misuse; every call site below passes a prune subcommand
src/drive/lease/ledger.rs ignore 812-816 both mutate_locked refuses before ever calling the closure, so its body never runs — a hit here is a regression, not a coverage gap
src/drive/lease/restore.rs ignore 1137-1141 both every test using this double refuses before authenticating; a hit here is a regression, not a coverage gap

Patch coverage

Patch: 96.39% (187/194 new lines covered)

File Patch Uncovered new lines
src/request_log.rs 96.39% (187/194) 484-487, 490-491, 493
Uncovered new lines (7)
  • src/request_log.rs:484
  • src/request_log.rs:485
  • src/request_log.rs:486
  • src/request_log.rs:487
  • src/request_log.rs:490
  • src/request_log.rs:491
  • src/request_log.rs:493

Indirect coverage changes

🔴 0 lines lost coverage, 🟢 6 lines gained coverage on unchanged code.

Indirect changes
  • src/request_log.rs:545 🟢 uncovered → covered
  • src/request_log.rs:546 🟢 uncovered → covered
  • src/request_log.rs:547 🟢 uncovered → covered
  • src/request_log.rs:599 🟢 uncovered → covered
  • src/request_log.rs:600 🟢 uncovered → covered
  • src/request_log.rs:601 🟢 uncovered → covered

📦 Full per-file coverage summary · run summary

@newhoggy
newhoggy merged commit 5a81ba4 into main Sep 17, 2026
17 checks passed
@newhoggy
newhoggy deleted the issue-1694-audit-log-path-param branch September 17, 2026 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(request-log): audit log can be pruned/rotated via OMNI_DEV_LOG_FILE; collision guard is a raw PathBuf compare

1 participant