diff --git a/Makefile b/Makefile index c0844da19..4a85f0bd6 100644 --- a/Makefile +++ b/Makefile @@ -100,7 +100,7 @@ lint-clippy: ## Run rustdoc and Clippy with warnings denied RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings $(POLONIUS_FLAGS)" $(CARGO) clippy $(CLIPPY_FLAGS) lint-whitaker: ## Run the Whitaker Dylint suite with warnings denied - RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings $(POLONIUS_FLAGS)" $(WHITAKER) --all --no-deps --package netsuke-build -- --all-targets --all-features + DYLINT_TOML="$$(cat dylint.toml)" RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings $(POLONIUS_FLAGS)" $(WHITAKER) --all --no-deps --package netsuke-build -- --all-targets --all-features # Run from the crate directory as well so Whitaker loads the narrow # `test_support::fs` exemption from test_support/dylint.toml. cd test_support && DYLINT_TOML="$$(cat dylint.toml)" RUSTFLAGS="$${RUSTFLAGS:+$$RUSTFLAGS }-D warnings $(POLONIUS_FLAGS)" $(WHITAKER) --all --no-deps --package test_support -- --all-targets --all-features diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 3b4f21a24..1add5ea61 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1712,10 +1712,13 @@ is not obvious from the name: absent or unreadable path returns `false` rather than surfacing the underlying metadata error. Fixture code must use this wrapper for directory predicates rather than calling `std::fs::metadata(...).is_dir()` or - `Path::is_dir` directly. `test_support/src/manifest.rs` is an existing caller: - `ensure_manifest_exists` uses it both to reject a directory where a manifest - file is expected, and to accept a destination directory that is already - present. + `Path::is_dir` directly. +- `PathState` and `inspect_path(path) -> io::Result` provide a + fallible target-state probe. `PathState::Absent` means metadata returned + `NotFound`, `PathState::Directory` means the target is a directory, and + `PathState::NonDirectory` means it exists but is not a directory. The probe + follows symlinks, so a dangling symlink is `Absent` even when its directory + entry exists; metadata errors other than `NotFound` are propagated. - `try_is_file(path) -> io::Result` is the fallible counterpart to the boolean predicates: `Ok(true)` when the path is a regular file, `Ok(false)` when it is absent (`NotFound` is folded into the boolean result), and `Err` @@ -1749,6 +1752,22 @@ ambient boundary stays where the lint expects it. Prefer that shape — pass in what the operation needs and keep the handle here — over widening an exclusion to a module that wants a raw `File`. +### `test_support::ensure_manifest_exists` + +`test_support::ensure_manifest_exists` (`test_support/src/manifest.rs`) never +overwrites an existing non-directory target. If another actor creates a +non-directory target after the initial existence check but before persistence, +no-clobber persistence leaves that target unchanged and returns its path, which +satisfies the existence contract. An existing directory, including one created +at the controlled pre-persist point, returns `io::ErrorKind::IsADirectory`. + +When a manifest is missing, its generated contents are written to a temporary +file staged in the destination directory before persistence. The tests inject +the pre-persist action to cover controlled creation orderings; they do not claim +to model arbitrary scheduler or filesystem interleavings. The fallible +`test_support::fs::inspect_path` probe treats `NotFound` as absence and +propagates every other metadata error. + ### Shared Makefile contract helpers `tests/support/makefile.rs` is a shared module for integration tests that diff --git a/test_support/src/fs.rs b/test_support/src/fs.rs index 36009846a..a2afd5754 100644 --- a/test_support/src/fs.rs +++ b/test_support/src/fs.rs @@ -21,6 +21,17 @@ use std::io; use std::path::Path; use std::time::SystemTime; +/// The state observed when inspecting a filesystem path. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum PathState { + /// The path does not exist. + Absent, + /// The path exists and is a directory. + Directory, + /// The path exists and is not a directory. + NonDirectory, +} + /// Write `contents` to `path`, creating or truncating the file. /// /// # Errors @@ -117,6 +128,43 @@ pub fn is_dir(path: impl AsRef) -> bool { fs::metadata(path).is_ok_and(|metadata| metadata.is_dir()) } +/// Inspect whether `path` is absent, a directory, or another target. +/// +/// Only [`io::ErrorKind::NotFound`] becomes [`PathState::Absent`]. All other +/// metadata failures are returned so callers do not mistake an unreadable path +/// for an absent one. +/// [`std::fs::metadata`] follows symbolic links, so a dangling symbolic link is +/// reported as [`PathState::Absent`] even though its directory entry exists. +/// +/// # Errors +/// +/// Propagates the underlying metadata failure for any error other than +/// [`io::ErrorKind::NotFound`]. +/// +/// # Examples +/// +/// ``` +/// use test_support::fs::{PathState, inspect_path}; +/// +/// let dir = tempfile::tempdir().expect("create tempdir"); +/// let file = dir.path().join("file"); +/// test_support::fs::write(&file, "contents").expect("write file"); +/// assert_eq!(inspect_path(&file).expect("inspect file"), PathState::NonDirectory); +/// assert_eq!(inspect_path(dir.path()).expect("inspect directory"), PathState::Directory); +/// assert_eq!( +/// inspect_path(dir.path().join("absent")).expect("inspect absent path"), +/// PathState::Absent +/// ); +/// ``` +pub fn inspect_path(path: impl AsRef) -> io::Result { + match fs::metadata(path) { + Ok(metadata) if metadata.is_dir() => Ok(PathState::Directory), + Ok(_) => Ok(PathState::NonDirectory), + Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(PathState::Absent), + Err(error) => Err(error), + } +} + /// Return `true` when `path` is a regular file, surfacing unexpected errors. /// /// Unlike `Path::is_file`, an I/O failure other than `NotFound` is propagated diff --git a/test_support/src/fs_tests.rs b/test_support/src/fs_tests.rs index 3b5c9d793..471f1a55b 100644 --- a/test_support/src/fs_tests.rs +++ b/test_support/src/fs_tests.rs @@ -5,9 +5,10 @@ //! `module_max_lines` cap; included from there via `#[path]` so the tests //! stay a child module of `fs`. -use super::{create_dir_all, try_is_file, write}; +use super::{PathState, create_dir_all, inspect_path, try_is_file, write}; use rstest::{fixture, rstest}; use std::io; +use std::path::Path; /// Temporary workspace for the filesystem helper tests. /// @@ -21,6 +22,24 @@ fn temp_dir() -> TempDir { tempfile::tempdir() } +fn assert_traversal_through_file_propagates_non_not_found( + temp_dir: TempDir, + inspect: impl FnOnce(&Path) -> io::Result, +) -> anyhow::Result<()> { + let temp = temp_dir?; + let file = temp.path().join("regular-file"); + write(&file, b"fixture")?; + + let Err(error) = inspect(&file.join("child")) else { + anyhow::bail!("traversing through a regular file should fail"); + }; + anyhow::ensure!( + error.kind() != io::ErrorKind::NotFound, + "traversal through a file should not be reported as absence, got {error:?}" + ); + Ok(()) +} + #[rstest] fn try_is_file_reports_a_regular_file_as_a_file(temp_dir: TempDir) -> anyhow::Result<()> { let temp = temp_dir?; @@ -58,20 +77,28 @@ fn try_is_file_reports_a_directory_as_not_a_file(temp_dir: TempDir) -> anyhow::R #[rstest] fn try_is_file_propagates_errors_other_than_not_found(temp_dir: TempDir) -> anyhow::Result<()> { + assert_traversal_through_file_propagates_non_not_found(temp_dir, |path| try_is_file(path)) +} + +#[rstest] +fn inspect_path_distinguishes_absent_directory_and_non_directory( + temp_dir: TempDir, +) -> anyhow::Result<()> { let temp = temp_dir?; let file = temp.path().join("regular-file"); write(&file, b"fixture")?; - let Err(error) = try_is_file(file.join("child")) else { - anyhow::bail!("traversing through a regular file should fail"); - }; - anyhow::ensure!( - error.kind() != io::ErrorKind::NotFound, - "traversal through a file should not be reported as absence, got {error:?}" - ); + anyhow::ensure!(inspect_path(temp.path())? == PathState::Directory); + anyhow::ensure!(inspect_path(&file)? == PathState::NonDirectory); + anyhow::ensure!(inspect_path(temp.path().join("absent"))? == PathState::Absent); Ok(()) } +#[rstest] +fn inspect_path_propagates_errors_other_than_not_found(temp_dir: TempDir) -> anyhow::Result<()> { + assert_traversal_through_file_propagates_non_not_found(temp_dir, |path| inspect_path(path)) +} + #[rstest] fn create_dir_all_accepts_an_existing_directory(temp_dir: TempDir) -> io::Result<()> { let temp = temp_dir?; diff --git a/test_support/src/manifest.rs b/test_support/src/manifest.rs index c0b03f58d..f78e3a351 100644 --- a/test_support/src/manifest.rs +++ b/test_support/src/manifest.rs @@ -6,6 +6,9 @@ use cap_std::{ambient_authority, fs_utf8}; use std::io; use tempfile::NamedTempFile; +#[cfg(test)] +use std::cell::RefCell; + /// Prefix the provided manifest body with the standard Netsuke version header. #[must_use] pub fn manifest_yaml(body: &str) -> String { @@ -15,8 +18,16 @@ pub fn manifest_yaml(body: &str) -> String { /// Resolve `cli_file` relative to `temp_dir` and ensure it exists. /// /// When `cli_file` is relative, it is joined with `temp_dir` and the returned -/// path is absolute and UTF‑8. If the resulting path does not exist, a minimal -/// manifest is written to that location. +/// path is absolute and UTF‑8. If the resulting path already names a +/// non-directory target, that path is returned without modifying its contents. +/// If another actor creates a non-directory target after the initial existence +/// check but before persistence, that target is likewise returned unchanged. +/// Directory targets return an [`io::ErrorKind::IsADirectory`] error. When a +/// manifest must be created, staging occurs atomically in the target directory. +/// Target inspection is fallible: metadata errors other than `NotFound` +/// propagate to the caller. +/// These guarantees describe the controlled implementation ordering and do not +/// establish behaviour for every filesystem or scheduler. /// /// # Errors /// @@ -41,19 +52,58 @@ pub fn manifest_yaml(body: &str) -> String { pub fn ensure_manifest_exists(temp_dir: &Utf8Path, cli_file: &Utf8Path) -> io::Result { let manifest_path = resolve_manifest_path(temp_dir, cli_file)?; - if fs::is_dir(&manifest_path) { - return Err(io::Error::new( - io::ErrorKind::IsADirectory, - format!("Manifest path points to a directory, expected a file: {manifest_path}"), - )); + match inspect_manifest_target(&manifest_path)? { + fs::PathState::Absent => create_manifest_file(temp_dir, manifest_path.as_ref())?, + fs::PathState::Directory => return Err(manifest_path_is_directory_error(&manifest_path)), + fs::PathState::NonDirectory => return Ok(manifest_path), } - if fs::exists(&manifest_path) { - return Ok(manifest_path); + Ok(manifest_path) +} + +fn inspect_manifest_target(manifest_path: &Utf8Path) -> io::Result { + fs::inspect_path(manifest_path).map_err(|error| { + io::Error::new( + error.kind(), + format!("Failed to inspect manifest path {manifest_path}: {error}"), + ) + }) +} + +fn manifest_path_is_directory_error(manifest_path: &Utf8Path) -> io::Error { + io::Error::new( + io::ErrorKind::IsADirectory, + format!("Manifest path points to a directory, expected a file: {manifest_path}"), + ) +} + +fn handle_raced_manifest_target(manifest_path: &Utf8Path) -> io::Result<()> { + match inspect_manifest_target(manifest_path)? { + fs::PathState::Directory => Err(manifest_path_is_directory_error(manifest_path)), + fs::PathState::NonDirectory => Ok(()), + fs::PathState::Absent => Err(io::Error::new( + io::ErrorKind::AlreadyExists, + format!( + "Manifest target disappeared after no-clobber persistence reported it existed: {manifest_path}" + ), + )), } +} - create_manifest_file(temp_dir, manifest_path.as_ref())?; - Ok(manifest_path) +/// Install a deterministic action before a staged manifest is persisted. +/// +/// This test-only seam models controlled target creation after the initial +/// existence check; it does not introduce scheduling into production code. Its +/// action is one-shot: [`run_before_persist_hook`] removes it before execution, +/// so it runs at most once per guard even across multiple +/// [`ensure_manifest_exists`] calls. The guard restores the preceding hook when +/// it is dropped. +#[cfg(test)] +fn install_before_persist_hook( + hook: impl FnOnce(NamedTempFile, &Utf8Path) -> io::Result + 'static, +) -> BeforePersistHookGuard { + let previous = BEFORE_PERSIST_HOOK.with(|hook_slot| hook_slot.replace(Some(Box::new(hook)))); + BeforePersistHookGuard { previous } } fn resolve_manifest_path(temp_dir: &Utf8Path, cli_file: &Utf8Path) -> io::Result { @@ -78,7 +128,15 @@ fn create_manifest_file(temp_dir: &Utf8Path, manifest_path: &Utf8Path) -> io::Re ensure_parent_directory(manifest_path, dest_dir)?; let mut file = create_temp_file(dest_dir, manifest_path)?; write_manifest_content(&mut file, manifest_path)?; - persist_manifest_file(file, manifest_path) + #[cfg(test)] + { + persist_manifest_file(run_before_persist_hook(file, manifest_path)?, manifest_path) + } + + #[cfg(not(test))] + { + persist_manifest_file(file, manifest_path) + } } fn create_temp_file(dest_dir: &Utf8Path, manifest_path: &Utf8Path) -> io::Result { @@ -99,10 +157,16 @@ fn write_manifest_content(file: &mut NamedTempFile, manifest_path: &Utf8Path) -> }) } +/// Persist a staged manifest without overwriting an existing target. +/// +/// A concurrently created non-directory target is tolerated because it already +/// fulfils the manifest-exists contract. fn persist_manifest_file(file: NamedTempFile, manifest_path: &Utf8Path) -> io::Result<()> { - match file.persist(manifest_path.as_std_path()) { + match file.persist_noclobber(manifest_path.as_std_path()) { Ok(_) => Ok(()), - Err(e) if e.error.kind() == io::ErrorKind::AlreadyExists => Ok(()), + Err(e) if e.error.kind() == io::ErrorKind::AlreadyExists => { + handle_raced_manifest_target(manifest_path) + } Err(e) => Err(io::Error::new( e.error.kind(), format!( @@ -116,19 +180,22 @@ fn persist_manifest_file(file: NamedTempFile, manifest_path: &Utf8Path) -> io::R } fn ensure_parent_directory(manifest_path: &Utf8Path, dest_dir: &Utf8Path) -> io::Result<()> { - if fs::exists(dest_dir) { - // If the path exists but is not a directory, report a clear error that - // includes the final manifest path. Returning AlreadyExists mirrors the - // semantics that the desired directory “exists” but is unusable. - if fs::is_dir(dest_dir) { - return Ok(()); + match fs::inspect_path(dest_dir).map_err(|error| { + io::Error::new( + error.kind(), + format!("Failed to inspect manifest parent directory for {manifest_path}: {error}"), + ) + })? { + fs::PathState::Directory => return Ok(()), + fs::PathState::NonDirectory => { + return Err(io::Error::new( + io::ErrorKind::NotADirectory, + format!( + "Failed to create manifest parent directory for {manifest_path}: parent path exists and is not a directory", + ), + )); } - return Err(io::Error::new( - io::ErrorKind::AlreadyExists, - format!( - "Failed to create manifest parent directory for {manifest_path}: parent path exists and is not a directory", - ), - )); + fs::PathState::Absent => {} } let base = find_existing_ancestor(dest_dir, manifest_path)?; @@ -161,104 +228,67 @@ fn find_existing_ancestor<'a>( let mut ancestors = dest_dir.ancestors(); ancestors.next(); // Skip self - ancestors - .find(|candidate| fs::exists(candidate)) - .ok_or_else(|| { + for candidate in ancestors { + match fs::inspect_path(candidate).map_err(|error| { io::Error::new( - io::ErrorKind::NotFound, + error.kind(), format!( - "Failed to locate an existing ancestor for manifest directory {manifest_path}", + "Failed to inspect manifest ancestor {candidate} for {manifest_path}: {error}" ), ) - }) + })? { + fs::PathState::Directory => return Ok(candidate), + fs::PathState::NonDirectory => { + return Err(io::Error::new( + io::ErrorKind::NotADirectory, + format!( + "Failed to create manifest parent directory for {manifest_path}: ancestor {candidate} is not a directory", + ), + )); + } + fs::PathState::Absent => {} + } + } + + Err(io::Error::new( + io::ErrorKind::NotFound, + format!("Failed to locate an existing ancestor for manifest directory {manifest_path}"), + )) } #[cfg(test)] -mod tests { - //! Unit tests for manifest fixture creation. - - use super::*; - use anyhow::{Context, Result}; - use camino::Utf8Path; - use std::io; - use tempfile::TempDir; - - #[test] - fn existing_directory_manifest_path_is_rejected() -> Result<()> { - let temp = TempDir::new().context("create temp dir")?; - let temp_path = Utf8Path::from_path(temp.path()) - .ok_or_else(|| anyhow::anyhow!("temp path is not valid UTF-8"))?; - let dir = temp.path().join("dir"); - fs::create_dir(&dir).context("create directory placeholder")?; - - let Err(err) = ensure_manifest_exists(temp_path, Utf8Path::new("dir")) else { - anyhow::bail!("existing directory should be rejected"); - }; - anyhow::ensure!(err.kind() == io::ErrorKind::IsADirectory); - let msg = err.to_string(); - let dir_str = dir - .to_str() - .ok_or_else(|| anyhow::anyhow!("dir path is not valid UTF-8"))?; - anyhow::ensure!(msg.contains(dir_str), "message: {msg}"); - Ok(()) - } +type BeforePersistHook = Box io::Result>; - #[test] - fn read_only_parent_reports_target_path() -> Result<()> { - let temp = TempDir::new().context("create temp dir")?; - let temp_path = Utf8Path::from_path(temp.path()) - .ok_or_else(|| anyhow::anyhow!("temp path is not valid UTF-8"))?; - let parent = temp.path().join("parent"); - fs::write(&parent, b"file").context("write placeholder parent file")?; - let manifest = parent.join("manifest.yml"); - - let Err(err) = ensure_manifest_exists(temp_path, Utf8Path::new("parent/manifest.yml")) - else { - anyhow::bail!("non-directory parent should error"); - }; - anyhow::ensure!(err.kind() == io::ErrorKind::AlreadyExists); - let msg = err.to_string(); - let manifest_str = manifest - .to_str() - .ok_or_else(|| anyhow::anyhow!("manifest path is not valid UTF-8"))?; - anyhow::ensure!(msg.contains(manifest_str), "message: {msg}"); - Ok(()) +#[cfg(test)] +thread_local! { + static BEFORE_PERSIST_HOOK: RefCell> = const { RefCell::new(None) }; +} + +#[cfg(test)] +#[must_use] +struct BeforePersistHookGuard { + previous: Option, +} + +#[cfg(test)] +impl Drop for BeforePersistHookGuard { + fn drop(&mut self) { + BEFORE_PERSIST_HOOK.with(|hook_slot| *hook_slot.borrow_mut() = self.previous.take()); } +} - #[test] - fn creates_missing_parent_directory_and_manifest() -> Result<()> { - let temp = TempDir::new().context("create temp dir")?; - let temp_path = Utf8Path::from_path(temp.path()) - .ok_or_else(|| anyhow::anyhow!("temp path is not valid UTF-8"))?; - - // Parent directory does not exist beforehand. - let cli_file = Utf8Path::new("missing/subdir/manifest.yml"); - let expected_path = temp_path.join(cli_file); - anyhow::ensure!( - !fs::exists(&expected_path), - "precondition: path should not exist" - ); - - let manifest_path = - ensure_manifest_exists(temp_path, cli_file).context("create manifest when missing")?; - anyhow::ensure!(manifest_path == expected_path, "manifest path should match"); - anyhow::ensure!(fs::exists(&manifest_path), "manifest file should exist"); - anyhow::ensure!( - fs::exists( - manifest_path - .parent() - .ok_or_else(|| anyhow::anyhow!("manifest path missing parent"))? - ), - "parent directory should be created" - ); - - // Sanity check that content was written, not an empty file. - let contents = - fs::read_to_string(manifest_path.as_std_path()).context("read manifest contents")?; - anyhow::ensure!( - contents.contains("netsuke_version:"), - "unexpected manifest contents: {contents}" - ); - Ok(()) +#[cfg(test)] +fn run_before_persist_hook( + file: NamedTempFile, + manifest_path: &Utf8Path, +) -> io::Result { + let hook = BEFORE_PERSIST_HOOK.with(|hook_slot| hook_slot.borrow_mut().take()); + match hook { + Some(installed_hook) => installed_hook(file, manifest_path), + None => Ok(file), } } + +#[cfg(test)] +#[path = "manifest/tests.rs"] +mod tests; diff --git a/test_support/src/manifest/tests.rs b/test_support/src/manifest/tests.rs new file mode 100644 index 000000000..523ab6925 --- /dev/null +++ b/test_support/src/manifest/tests.rs @@ -0,0 +1,400 @@ +//! Unit tests for manifest fixture creation. + +use super::*; +use anyhow::{Context, Result}; +use camino::{Utf8Path, Utf8PathBuf}; +use proptest::{prelude::*, test_runner::TestCaseError}; +use rstest::{fixture, rstest}; +use std::io::{self, Write}; +use tempfile::TempDir; + +type TempManifestWorkspace = Result<(TempDir, Utf8PathBuf)>; + +#[fixture] +fn temp_manifest_workspace() -> TempManifestWorkspace { + let temp = TempDir::new().context("create temp dir")?; + let temp_path = Utf8Path::from_path(temp.path()) + .ok_or_else(|| anyhow::anyhow!("temp path is not valid UTF-8"))? + .to_owned(); + Ok((temp, temp_path)) +} + +fn assert_no_staged_manifest_files(temp_path: &Utf8Path, expected_path: &Utf8Path) -> Result<()> { + for entry_result in temp_path + .read_dir_utf8() + .context("inspect manifest workspace")? + { + let workspace_entry = entry_result.context("inspect manifest workspace entry")?; + anyhow::ensure!( + workspace_entry.path() == expected_path, + "leftover staged manifest file: {}", + workspace_entry.path() + ); + } + Ok(()) +} + +#[rstest] +fn existing_directory_manifest_path_is_rejected( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (temp, temp_path) = temp_manifest_workspace?; + let dir = temp.path().join("dir"); + fs::create_dir(&dir).context("create directory placeholder")?; + + let Err(err) = ensure_manifest_exists(&temp_path, Utf8Path::new("dir")) else { + anyhow::bail!("existing directory should be rejected"); + }; + anyhow::ensure!(err.kind() == io::ErrorKind::IsADirectory); + let msg = err.to_string(); + let dir_str = dir + .to_str() + .ok_or_else(|| anyhow::anyhow!("dir path is not valid UTF-8"))?; + anyhow::ensure!(msg.contains(dir_str), "message: {msg}"); + Ok(()) +} + +#[rstest] +fn non_directory_parent_propagates_target_inspection_error( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (temp, temp_path) = temp_manifest_workspace?; + let parent = temp.path().join("parent"); + fs::write(&parent, b"file").context("write placeholder parent file")?; + let manifest = parent.join("manifest.yml"); + + let Err(err) = ensure_manifest_exists(&temp_path, Utf8Path::new("parent/manifest.yml")) else { + anyhow::bail!("non-directory parent should error"); + }; + anyhow::ensure!( + err.kind() != io::ErrorKind::NotFound, + "target inspection error should not be treated as absence: {err}" + ); + let msg = err.to_string(); + let manifest_str = manifest + .to_str() + .ok_or_else(|| anyhow::anyhow!("manifest path is not valid UTF-8"))?; + anyhow::ensure!(msg.contains(manifest_str), "message: {msg}"); + Ok(()) +} + +#[rstest] +fn non_directory_parent_components_are_reported_as_not_a_directory( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (_temp, temp_path) = temp_manifest_workspace?; + let parent = temp_path.join("parent"); + fs::write(parent.as_std_path(), b"file").context("write placeholder parent file")?; + let manifest_path = parent.join("manifest.yml"); + + let Err(parent_err) = ensure_parent_directory(&manifest_path, &parent) else { + anyhow::bail!("non-directory parent should be rejected"); + }; + anyhow::ensure!(parent_err.kind() == io::ErrorKind::NotADirectory); + anyhow::ensure!(parent_err.to_string().contains(manifest_path.as_str())); + + let ancestor = temp_path.join("ancestor"); + fs::write(ancestor.as_std_path(), b"file").context("write placeholder ancestor file")?; + let ancestor_manifest_path = ancestor.join("child/manifest.yml"); + let dest_dir = ancestor_manifest_path + .parent() + .context("manifest path missing parent")?; + let Err(ancestor_err) = find_existing_ancestor(dest_dir, &ancestor_manifest_path) else { + anyhow::bail!("non-directory ancestor should be rejected"); + }; + anyhow::ensure!(ancestor_err.kind() == io::ErrorKind::NotADirectory); + let ancestor_message = ancestor_err.to_string(); + anyhow::ensure!(ancestor_message.contains(ancestor_manifest_path.as_str())); + Ok(()) +} + +#[rstest] +fn creates_missing_parent_directory_and_manifest( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (_temp, temp_path) = temp_manifest_workspace?; + + let cli_file = Utf8Path::new("missing/subdir/manifest.yml"); + let expected_path = temp_path.join(cli_file); + anyhow::ensure!( + !fs::exists(&expected_path), + "precondition: path should not exist" + ); + + let manifest_path = + ensure_manifest_exists(&temp_path, cli_file).context("create manifest when missing")?; + anyhow::ensure!(manifest_path == expected_path, "manifest path should match"); + anyhow::ensure!(fs::exists(&manifest_path), "manifest file should exist"); + anyhow::ensure!( + fs::exists( + manifest_path + .parent() + .ok_or_else(|| anyhow::anyhow!("manifest path missing parent"))? + ), + "parent directory should be created" + ); + + let contents = + fs::read_to_string(manifest_path.as_std_path()).context("read manifest contents")?; + anyhow::ensure!( + contents.contains("netsuke_version:"), + "unexpected manifest contents: {contents}" + ); + Ok(()) +} + +#[rstest] +fn existing_file_manifest_path_is_returned_unchanged( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (_temp, temp_path) = temp_manifest_workspace?; + let manifest_path = temp_path.join("manifest.yml"); + let existing_contents = b"existing manifest contents"; + fs::write(manifest_path.as_std_path(), existing_contents) + .context("create existing manifest")?; + + let returned_path = ensure_manifest_exists(&temp_path, Utf8Path::new("manifest.yml")) + .context("return existing manifest")?; + + anyhow::ensure!(returned_path == manifest_path, "manifest path should match"); + let contents = fs::read(manifest_path.as_std_path()).context("read existing manifest")?; + anyhow::ensure!(contents == existing_contents, "manifest contents changed"); + Ok(()) +} + +#[rstest] +fn raced_file_manifest_path_is_returned_unchanged( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (_temp, temp_path) = temp_manifest_workspace?; + let expected_path = temp_path.join("manifest.yml"); + let competing_contents = b"competing manifest contents"; + anyhow::ensure!( + fs::inspect_path(&expected_path)? == fs::PathState::Absent, + "precondition: manifest path should be absent" + ); + + let _hook = install_before_persist_hook(move |file, manifest_path| { + fs::write(manifest_path.as_std_path(), competing_contents)?; + Ok(file) + }); + let returned_path = ensure_manifest_exists(&temp_path, Utf8Path::new("manifest.yml")) + .context("tolerate manifest created before persistence")?; + + anyhow::ensure!(returned_path == expected_path, "manifest path should match"); + let contents = fs::read(expected_path.as_std_path()).context("read competing manifest")?; + anyhow::ensure!(contents == competing_contents, "manifest contents changed"); + assert_no_staged_manifest_files(&temp_path, &expected_path)?; + Ok(()) +} + +#[rstest] +fn raced_directory_manifest_path_is_rejected( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (_temp, temp_path) = temp_manifest_workspace?; + let expected_path = temp_path.join("manifest.yml"); + anyhow::ensure!( + fs::inspect_path(&expected_path)? == fs::PathState::Absent, + "precondition: manifest path should be absent" + ); + + let _hook = install_before_persist_hook(|file, manifest_path| { + fs::create_dir(manifest_path.as_std_path())?; + Ok(file) + }); + let Err(err) = ensure_manifest_exists(&temp_path, Utf8Path::new("manifest.yml")) else { + anyhow::bail!("directory created before persistence should be rejected"); + }; + + anyhow::ensure!(err.kind() == io::ErrorKind::IsADirectory); + anyhow::ensure!( + err.to_string().contains(expected_path.as_str()), + "message: {err}" + ); + assert_no_staged_manifest_files(&temp_path, &expected_path)?; + Ok(()) +} + +#[rstest] +fn before_persist_hook_does_not_escape_its_scope( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (_temp, temp_path) = temp_manifest_workspace?; + { + let _hook = install_before_persist_hook(|_, _| { + Err(io::Error::other( + "hook should be removed when its guard drops", + )) + }); + } + + let expected_path = temp_path.join("manifest.yml"); + let returned_path = ensure_manifest_exists(&temp_path, Utf8Path::new("manifest.yml")) + .context("create manifest after hook scope ends")?; + + anyhow::ensure!(returned_path == expected_path, "manifest path should match"); + Ok(()) +} + +#[rstest] +fn persisting_manifest_tolerates_existing_file_without_overwriting( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (temp, temp_path) = temp_manifest_workspace?; + let manifest_path = temp_path.join("manifest.yml"); + let existing_contents = b"existing manifest contents"; + fs::write(manifest_path.as_std_path(), existing_contents) + .context("create existing manifest")?; + + let mut staged_file = NamedTempFile::new_in(temp.path()).context("stage manifest")?; + staged_file + .write_all(b"replacement manifest contents") + .context("write staged manifest")?; + + persist_manifest_file(staged_file, &manifest_path) + .context("persist staged manifest without overwriting")?; + + let contents = + fs::read(manifest_path.as_std_path()).context("read existing manifest contents")?; + anyhow::ensure!( + contents == existing_contents, + "manifest contents changed: {contents:?}" + ); + Ok(()) +} + +#[rstest] +fn persisting_manifest_rejects_existing_directory( + temp_manifest_workspace: TempManifestWorkspace, +) -> Result<()> { + let (temp, temp_path) = temp_manifest_workspace?; + let manifest_path = temp_path.join("manifest.yml"); + fs::create_dir(manifest_path.as_std_path()).context("create manifest directory")?; + let staged_file = NamedTempFile::new_in(temp.path()).context("stage manifest")?; + + let Err(err) = persist_manifest_file(staged_file, &manifest_path) else { + anyhow::bail!("directory target should be rejected"); + }; + anyhow::ensure!(err.kind() == io::ErrorKind::IsADirectory); + anyhow::ensure!( + err.to_string().contains(manifest_path.as_str()), + "message: {err}" + ); + Ok(()) +} + +#[derive(Clone, Copy, Debug)] +enum TargetState { + Missing, + ExistingFile, + RacedFile, + ExistingDirectory, + RacedDirectory, +} + +fn target_state_strategy() -> impl Strategy { + prop_oneof![ + Just(TargetState::Missing), + Just(TargetState::ExistingFile), + Just(TargetState::RacedFile), + Just(TargetState::ExistingDirectory), + Just(TargetState::RacedDirectory), + ] +} +fn test_case_error(error: &io::Error) -> TestCaseError { + TestCaseError::fail(error.to_string()) +} +fn property_workspace() -> Result<(TempDir, Utf8PathBuf), TestCaseError> { + let temp = TempDir::new().map_err(|error| test_case_error(&error))?; + let temp_path = Utf8Path::from_path(temp.path()) + .ok_or_else(|| TestCaseError::fail("temporary path is not UTF-8"))? + .to_owned(); + Ok((temp, temp_path)) +} +fn replace_staged_manifest( + staged_file: NamedTempFile, + manifest_path: &Utf8Path, + contents: &[u8], +) -> io::Result { + drop(staged_file); + let destination_directory = manifest_path.parent().ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidInput, + format!("Manifest path has no parent directory: {manifest_path}"), + ) + })?; + let mut replacement = NamedTempFile::new_in(destination_directory.as_std_path())?; + replacement.write_all(contents)?; + Ok(replacement) +} +// This property checks controlled creation orderings through the injected hook. +// It does not model arbitrary operating-system scheduling or filesystems. +proptest! { + #![proptest_config(ProptestConfig::with_cases(16))] + + #[test] + fn manifest_existence_contract_holds_for_bounded_target_states( + state in target_state_strategy(), + staged_name in "[a-z]{1,12}", + competing_contents in proptest::collection::vec(any::(), 1..25), + ) { + let (_temp, temp_path) = property_workspace()?; + let cli_file = Utf8Path::new("manifest.yml"); + let expected_path = temp_path.join(cli_file); + let staged_contents = manifest_yaml(&format!( + "targets:\n - name: {staged_name}\n command: \"echo hi\"\n" + )) + .into_bytes(); + + match state { + TargetState::ExistingFile => fs::write(expected_path.as_std_path(), &competing_contents) + .map_err(|error| test_case_error(&error))?, + TargetState::ExistingDirectory => fs::create_dir(expected_path.as_std_path()) + .map_err(|error| test_case_error(&error))?, + TargetState::Missing | TargetState::RacedFile | TargetState::RacedDirectory => {} + } + + let expected_staged_contents = staged_contents.clone(); + let hook_contents = competing_contents.clone(); + let _hook = install_before_persist_hook(move |file, manifest_path| { + let replacement = replace_staged_manifest(file, manifest_path, &staged_contents)?; + match state { + TargetState::RacedFile => { + fs::write(manifest_path.as_std_path(), hook_contents)?; + } + TargetState::RacedDirectory => fs::create_dir(manifest_path.as_std_path())?, + TargetState::Missing + | TargetState::ExistingFile + | TargetState::ExistingDirectory => {} + } + Ok(replacement) + }); + let result = ensure_manifest_exists(&temp_path, cli_file); + + match state { + TargetState::Missing => { + let returned_path = result.map_err(|error| test_case_error(&error))?; + prop_assert_eq!(&returned_path, &expected_path); + let contents = fs::read(expected_path.as_std_path()) + .map_err(|error| test_case_error(&error))?; + prop_assert_eq!(contents, expected_staged_contents); + } + TargetState::ExistingFile | TargetState::RacedFile => { + let returned_path = result.map_err(|error| test_case_error(&error))?; + prop_assert_eq!(&returned_path, &expected_path); + let contents = fs::read(expected_path.as_std_path()) + .map_err(|error| test_case_error(&error))?; + prop_assert_eq!(contents, competing_contents); + } + TargetState::ExistingDirectory | TargetState::RacedDirectory => { + prop_assert!(result.is_err(), "directory target should be rejected"); + let error = result + .err() + .ok_or_else(|| TestCaseError::fail("directory target should be rejected"))?; + prop_assert_eq!(error.kind(), io::ErrorKind::IsADirectory); + prop_assert!(error.to_string().contains(expected_path.as_str())); + } + } + } +}