mirror of
https://github.com/lahfir/agent-desktop.git
synced 2026-07-27 01:22:16 +00:00
fix: make private-file locking and stub permission reporting honest
Three defects the new Windows and Linux lanes exposed on first run. Windows failed 31 trace tests with "Access is denied". open_private_append opened the trace file append-only, and trace.rs locks that handle before every write. Windows LockFileEx rejects an append-only handle: the std docs state locking "will fail if the file is opened only for append; it must be opened with read or write access to succeed". Requesting read alongside append satisfies it and is inert on unix. Adds explicit lock-contract regression tests so the append handle cannot silently lose read access again. Linux failed two refs_lock tests because they placed the lock file directly in the system temp directory. /tmp is root-owned and world-writable, so the private-parent check correctly refused it; macOS never caught this because its per-user temp_dir is already 0700-safe. The tests now nest the lock in their own private directory. The FFI C-ABI passthrough test asserted ErrPermDenied for the stub adapter and its doc comment called that "the documented exception". It encoded the same dishonesty the core default carried: an adapter that never probes a permission has not observed a denial. All three predate this branch and were unreachable until the platform lanes existed.
This commit is contained in:
parent
5904a36b2a
commit
b67b0564ff
5 changed files with 65 additions and 19 deletions
|
|
@ -20,9 +20,12 @@ pub(crate) fn open_private_lock(path: &Path, create: bool) -> std::io::Result<Fi
|
|||
Ok(file)
|
||||
}
|
||||
|
||||
/// Read access is requested alongside append because callers lock the returned
|
||||
/// handle, and Windows `LockFileEx` fails with `ERROR_ACCESS_DENIED` on a handle
|
||||
/// opened for append only.
|
||||
pub(crate) fn open_private_append(path: &Path) -> std::io::Result<File> {
|
||||
let mut options = OpenOptions::new();
|
||||
options.create(true).append(true);
|
||||
options.read(true).create(true).append(true);
|
||||
configure_unix(&mut options, 0o600);
|
||||
let file = options.open(path)?;
|
||||
validate_private_regular(&file)?;
|
||||
|
|
@ -299,3 +302,7 @@ pub(super) fn permission_denied(message: &'static str) -> std::io::Error {
|
|||
#[cfg(test)]
|
||||
#[path = "private_file_tests.rs"]
|
||||
mod tests;
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "private_file_lock_tests.rs"]
|
||||
mod lock_tests;
|
||||
|
|
|
|||
37
crates/core/src/private_file_lock_tests.rs
Normal file
37
crates/core/src/private_file_lock_tests.rs
Normal file
|
|
@ -0,0 +1,37 @@
|
|||
use super::*;
|
||||
|
||||
fn scratch_dir(name: &str) -> PathBuf {
|
||||
let dir = std::env::temp_dir().join(format!(
|
||||
"agent-desktop-{name}-{}-{}",
|
||||
std::process::id(),
|
||||
crate::refs::new_snapshot_id()
|
||||
));
|
||||
crate::private_file_parent::ensure_private(&dir).expect("private scratch directory");
|
||||
dir
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn appended_private_files_are_lockable() {
|
||||
let path = scratch_dir("append-lock").join("trace.jsonl");
|
||||
|
||||
let file = open_private_append(&path).expect("append handle");
|
||||
file.lock()
|
||||
.expect("append handles must be lockable on every platform");
|
||||
file.unlock().expect("unlock");
|
||||
|
||||
drop(file);
|
||||
let _ = std::fs::remove_file(&path);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn append_handles_reopen_and_relock_after_close() {
|
||||
let path = scratch_dir("append-relock").join("trace.jsonl");
|
||||
|
||||
for _ in 0..2 {
|
||||
let file = open_private_append(&path).expect("append handle");
|
||||
file.lock().expect("lock");
|
||||
file.unlock().expect("unlock");
|
||||
}
|
||||
|
||||
let _ = std::fs::remove_file(&path);
|
||||
}
|
||||
|
|
@ -32,11 +32,13 @@ mod tests {
|
|||
use super::*;
|
||||
|
||||
fn lock_path(name: &str) -> std::path::PathBuf {
|
||||
std::env::temp_dir().join(format!(
|
||||
"agent-desktop-{name}-{}-{}.lock",
|
||||
std::process::id(),
|
||||
crate::refs::new_snapshot_id()
|
||||
))
|
||||
std::env::temp_dir()
|
||||
.join(format!(
|
||||
"agent-desktop-{name}-{}-{}",
|
||||
std::process::id(),
|
||||
crate::refs::new_snapshot_id()
|
||||
))
|
||||
.join("store.lock")
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
|
|
|||
|
|
@ -81,9 +81,9 @@ pub(crate) use acquire_adapter;
|
|||
///
|
||||
/// Every method delegates to the `PlatformAdapter` trait defaults, all of
|
||||
/// which return `not_supported()` errors. `permission_report()` returns
|
||||
/// `Denied` via the trait default, so `ad_check_permissions` yields
|
||||
/// `ErrPermDenied` on a stub build — the expected signal for a CI runner
|
||||
/// that has no OS accessibility permission.
|
||||
/// `Unknown` via the trait default, so `ad_check_permissions` yields
|
||||
/// `ErrPlatformNotSupported` on a stub build — the honest signal that the
|
||||
/// adapter never probed permissions, rather than that one was denied.
|
||||
#[cfg(feature = "stub-adapter")]
|
||||
struct StubAdapter;
|
||||
|
||||
|
|
|
|||
|
|
@ -93,27 +93,27 @@ fn stub_ad_version_always_succeeds() {
|
|||
}
|
||||
}
|
||||
|
||||
/// `ad_check_permissions` maps the stub adapter's `Denied` permission state
|
||||
/// to `ErrPermDenied (-1)`, not `ErrPlatformNotSupported (-8)`. This is the
|
||||
/// documented exception. Cross-platform callers must treat both codes as
|
||||
/// "adapter not operational here".
|
||||
/// `ad_check_permissions` reports `ErrPlatformNotSupported (-8)` on an adapter
|
||||
/// that never probes permissions. Not having measured a permission is a
|
||||
/// different fact from a user having denied one, and `ErrPermDenied (-1)` sends
|
||||
/// callers looking for a grant that does not exist on that platform.
|
||||
#[cfg(feature = "stub-adapter")]
|
||||
#[test]
|
||||
fn stub_ad_check_permissions_returns_err_perm_denied() {
|
||||
fn stub_ad_check_permissions_returns_err_platform_not_supported() {
|
||||
with_adapter(|adapter| unsafe {
|
||||
let rc = ad_check_permissions(adapter);
|
||||
assert_eq!(
|
||||
rc,
|
||||
AdResult::ErrPermDenied,
|
||||
"stub adapter permission_report() returns Denied → ErrPermDenied (-1), \
|
||||
not ErrPlatformNotSupported (-8). Both mean the adapter is not operational."
|
||||
AdResult::ErrPlatformNotSupported,
|
||||
"an adapter that does not probe permissions reports Unknown, which must \
|
||||
surface as ErrPlatformNotSupported (-8), never ErrPermDenied (-1)."
|
||||
);
|
||||
let msg = ad_last_error_message();
|
||||
assert!(
|
||||
!msg.is_null(),
|
||||
"last-error message must be set on ErrPermDenied"
|
||||
"last-error message must be set on ErrPlatformNotSupported"
|
||||
);
|
||||
assert_eq!(ad_last_error_code(), AdResult::ErrPermDenied);
|
||||
assert_eq!(ad_last_error_code(), AdResult::ErrPlatformNotSupported);
|
||||
});
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue