From f3bb0583fee15a52e462f82f40d280dc6389cb3c Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Sun, 6 Sep 2026 19:28:19 -0400 Subject: [PATCH 1/5] test(namespace-request): 104 cases across the six request families Peeled from mikegrier/deferred-namespace-ops, where these were written alongside work that is not ready. They stand on their own: no source change comes with them, and nothing outside the crate is touched. Coverage by module: path 134 lines -- the largest gap. Path handling is where a namespace crate is most exposed to odd input open_by_id 78 lines -- identifier-based opens open 66 lines volume 50 lines watch 48 lines security 40 lines The only non-additive lines are two `use` statements widened to import `SecurityAttributes` and `VolumeInformation` for the new cases. Typed `test` rather than `feat` or `fix` deliberately: no behavior changes, and release-please must not cut a version for a test-only commit. Verified against this branch rather than assumed portable: the patch was first applied to a scratch worktree checked out at origin/main with no other branch content present, and the suite passed there before the branch was cut. It also carries no cross-references -- no links to DESIGN-NOTES, checklists, or design sessions -- so it cannot leave a dangling reference behind on the source branch, which three earlier peels each did. 285 tests pass (217 unit, 68 across the two integration targets), clippy --all-targets --all-features clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/open/tests.rs | 66 +++++++++ .../src/open_by_id/tests.rs | 78 +++++++++- .../src/path/tests.rs | 134 ++++++++++++++++++ .../src/security/tests.rs | 40 ++++++ .../src/volume/tests.rs | 50 ++++++- .../src/watch/tests.rs | 48 +++++++ 6 files changed, 414 insertions(+), 2 deletions(-) diff --git a/crates/windows-namespace-request-sys/src/open/tests.rs b/crates/windows-namespace-request-sys/src/open/tests.rs index 252a5a3d..5d49d3bb 100644 --- a/crates/windows-namespace-request-sys/src/open/tests.rs +++ b/crates/windows-namespace-request-sys/src/open/tests.rs @@ -385,3 +385,69 @@ fn a_copy_duplicates_the_template_rather_than_sharing_the_owner() { copy.perform() .expect("the copy's template outlived the original"); } + +#[test] +fn every_configured_parameter_reads_back_through_its_own_accessor() { + // A mutation run replaced `desired_access`, `share_mode`, and + // `creation_disposition` with constants and nothing failed. The tests above + // build requests with `with_*` and then *open* them, so they exercise the + // fields through Win32 -- which is exactly what cannot distinguish an + // accessor reporting the truth from one reporting a constant, because the + // open path reads the struct's fields directly rather than through them. + // + // Every value here is deliberately non-zero and pairwise distinct. + // `OpenFile::new` starts every parameter at zero, so a test that configured + // a zero -- or reused one value twice -- would be satisfied by + // `-> Default::default()` and by an accessor reading a neighbour's field. + let fixture = Fixture::new("open-accessors"); + let request = request_for(fixture.directory()) + .with_desired_access(FILE_GENERIC_READ) + .with_share_mode(FILE_SHARE_READ) + .with_creation_disposition(OPEN_EXISTING) + .with_flags_and_attributes(FILE_FLAG_BACKUP_SEMANTICS); + + assert_eq!(request.desired_access(), FILE_GENERIC_READ); + assert_eq!(request.share_mode(), FILE_SHARE_READ); + assert_eq!(request.creation_disposition(), OPEN_EXISTING); + assert_eq!(request.flags_and_attributes(), FILE_FLAG_BACKUP_SEMANTICS); + + // The property the four assertions above rest on, stated rather than left + // to the reader's eye: these are Win32 constants and their values are not + // obvious, so a collision between two of them would silently weaken the + // test into one that cannot tell those accessors apart. + let configured = [ + FILE_GENERIC_READ, + FILE_SHARE_READ, + OPEN_EXISTING, + FILE_FLAG_BACKUP_SEMANTICS, + ]; + for (index, value) in configured.iter().enumerate() { + assert_ne!( + *value, 0, + "a zero is indistinguishable from the unset default" + ); + for other in &configured[index + 1..] { + assert_ne!( + value, other, + "two parameters share a value, so this test cannot tell their \ + accessors apart" + ); + } + } +} + +#[test] +fn an_unset_parameter_reads_back_as_nothing_rather_than_as_a_plausible_open() { + // The contract `OpenFile::new` states: every parameter starts at "the + // caller said nothing" rather than at a plausible-looking open, because a + // plausible default is exactly what a caller cannot see they got. An + // accessor that invented one would hide that from them. + let fixture = Fixture::new("open-unset"); + let request = request_for(fixture.directory()); + + assert_eq!(request.desired_access(), 0); + assert_eq!(request.share_mode(), 0); + assert_eq!(request.creation_disposition(), 0); + assert_eq!(request.flags_and_attributes(), 0); + assert!(request.security().is_none()); +} diff --git a/crates/windows-namespace-request-sys/src/open_by_id/tests.rs b/crates/windows-namespace-request-sys/src/open_by_id/tests.rs index b0ddb68a..7e81a81f 100644 --- a/crates/windows-namespace-request-sys/src/open_by_id/tests.rs +++ b/crates/windows-namespace-request-sys/src/open_by_id/tests.rs @@ -18,8 +18,8 @@ use windows_sys::Win32::Storage::FileSystem::{ }; use super::{FileIdentifier, OpenFileByIdentifier}; -use crate::CapturedHandle; use crate::handle::tests::{FILE_CONTENTS, Fixture, handle_allocation}; +use crate::{CapturedHandle, SecurityAttributes}; const AUDITED_SHARE: u32 = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE; @@ -275,3 +275,79 @@ fn a_request_performs_the_same_way_on_another_thread() { assert_eq!(length, FILE_CONTENTS.len() as u64); } + +#[test] +fn every_configured_parameter_reads_back_through_its_own_accessor() { + // Four accessors -- `desired_access`, `share_mode`, `security`, and + // `flags_and_attributes` -- all survived replacement by constants in a + // mutation run. Every test above builds a request and then *performs* it, + // and the perform path reads the struct's fields directly, so nothing + // distinguished an accessor that reports the truth from one that does not. + // + // `OpenFileByIdentifier::new` starts every parameter at zero or `None`, so + // the values here are deliberately non-zero and pairwise distinct: a zero + // would be indistinguishable from the default, and a repeated value would + // let one accessor read a neighbour's field undetected. + let _allocating = handle_allocation() + .read() + .expect("the lock is not poisoned"); + let fixture = Fixture::new("byid-accessors"); + let file = fixture.open_file(); + let id = file_id_of(&file); + let hint = open_directory_for_hint(&fixture); + + let request = OpenFileByIdentifier::new( + CapturedHandle::capture(hint.as_handle()).expect("capture the volume hint"), + FileIdentifier::FileId(id), + ) + .with_desired_access(FILE_GENERIC_READ) + .with_share_mode(FILE_SHARE_READ) + .with_flags_and_attributes(FILE_FLAG_BACKUP_SEMANTICS); + + assert_eq!(request.desired_access(), FILE_GENERIC_READ); + assert_eq!(request.share_mode(), FILE_SHARE_READ); + assert_eq!(request.flags_and_attributes(), FILE_FLAG_BACKUP_SEMANTICS); + assert!( + request.security().is_none(), + "nothing was supplied, so nothing must be reported" + ); + + let configured = [ + FILE_GENERIC_READ, + FILE_SHARE_READ, + FILE_FLAG_BACKUP_SEMANTICS, + ]; + for (index, value) in configured.iter().enumerate() { + assert_ne!(*value, 0, "a zero is indistinguishable from the default"); + for other in &configured[index + 1..] { + assert_ne!( + value, other, + "two parameters share a value, so this test cannot tell their \ + accessors apart" + ); + } + } +} + +#[test] +fn supplied_security_attributes_read_back_rather_than_reporting_none() { + // `security -> None` is the one accessor whose default is already `None`, + // so it needs the opposite case: attributes that were supplied must be + // visible to a caller inspecting the request, not only to the open that + // consumes it. + let _allocating = handle_allocation() + .read() + .expect("the lock is not poisoned"); + let fixture = Fixture::new("byid-security"); + let file = fixture.open_file(); + let id = file_id_of(&file); + let hint = open_directory_for_hint(&fixture); + + let request = OpenFileByIdentifier::new( + CapturedHandle::capture(hint.as_handle()).expect("capture the volume hint"), + FileIdentifier::FileId(id), + ) + .with_security(Some(SecurityAttributes::new(None, false))); + + assert!(request.security().is_some()); +} diff --git a/crates/windows-namespace-request-sys/src/path/tests.rs b/crates/windows-namespace-request-sys/src/path/tests.rs index eec60ded..7538862d 100644 --- a/crates/windows-namespace-request-sys/src/path/tests.rs +++ b/crates/windows-namespace-request-sys/src/path/tests.rs @@ -236,3 +236,137 @@ fn an_error_without_an_os_code_renders_only_its_description() { assert_eq!(error.to_string(), PathFailure::EmptyPath.description()); assert!(std::error::Error::source(&error).is_none()); } + +// --------------------------------------------------------------------------- +// Boundaries. +// +// A mutation sweep moved the ordinary path limit by one in both directions and +// changed `>` to `>=` and `==`, and every one of those survived: the tests +// above use comfortably-wrong lengths, which prove a check exists but not that +// it sits at the right unit. +// +// This is the same block, and for the same reason, as the one in +// `windows-file-enumeration-sys`'s path module -- the two crates carry +// near-identical path contracts, and the sweep found the same gap in both. +// --------------------------------------------------------------------------- + +/// An absolute path of exactly `units` UTF-16 units, already in normal form so +/// `GetFullPathNameW` returns it unchanged and the resolved length equals the +/// input length. +fn absolute_path_of_length(units: usize) -> String { + let prefix = r"C:\"; + format!("{prefix}{}", "a".repeat(units - prefix.len())) +} + +#[test] +fn an_ordinary_path_of_exactly_max_path_content_is_accepted() { + // 259 = MAX_PATH - 1, the longest path that leaves room for the terminator. + // Rejecting it is the off-by-one a "much too long" test cannot see, and it + // is the expensive direction: it refuses a path Windows would have opened. + let path = absolute_path_of_length(259); + assert_eq!(path.chars().count(), 259); + + let prepared = prepare_str(&path).expect("259 units is within the ordinary limit"); + assert_eq!(text(&prepared), path); +} + +#[test] +fn an_ordinary_path_one_unit_past_max_path_content_is_rejected() { + let path = absolute_path_of_length(260); + assert_eq!(path.chars().count(), 260); + + let error = prepare_str(&path).expect_err("260 units leaves no room for the terminator"); + assert_eq!(error.failure(), PathFailure::PathTooLong); +} + +#[test] +fn the_ordinary_limit_is_one_less_than_max_path() { + // The relationship the two tests above rest on, stated directly so a change + // to the constant fails here with its reason rather than only as a puzzling + // length assertion elsewhere. + assert_eq!(MAX_PATH_CONTENT, MAX_PATH - 1); + assert_eq!(MAX_PATH_CONTENT, 259); +} + +#[test] +fn a_verbatim_drive_relative_path_with_a_separator_is_rejected() { + // `\\?\C:foo` has no separator at all, so it is refused before the root is + // ever inspected and never reaches the drive-designator check. This form + // does reach it: the root is `C:foo`, which contains a colon but is not a + // drive. Without it, the check could report every root as a drive and + // nothing would notice. + let error = prepare_str(r"\\?\C:foo\bar").expect_err("drive-relative, not fully qualified"); + assert_eq!(error.failure(), PathFailure::NotFullyQualified); +} + +#[test] +fn a_verbatim_root_needs_a_letter_before_its_colon_not_merely_a_colon() { + // Both halves of the drive-designator rule are load-bearing, and only a + // root that satisfies one but not the other separates them. `1:` has the + // colon in the right place and is still not a drive, so a check accepting + // *either* condition would wave it through. + for path in [r"\\?\1:\", r"\\?\1:\dir"] { + let error = prepare_str(path).expect_err("a digit is not a drive letter"); + assert_eq!( + error.failure(), + PathFailure::NotFullyQualified, + "for {path}" + ); + } + + // Deliberately no companion case for "second unit is not a colon": the + // check is guarded by `root.contains(&COLON)`, so a colonless root -- a + // volume GUID, say -- never reaches it and is accepted on its own terms. + let prepared = prepare_str(r"\\?\Ca\dir").expect("a colonless root is not a drive at all"); + assert_eq!(text(&prepared), r"\\?\Ca\dir"); +} + +#[test] +fn every_path_failure_describes_itself_distinctly() { + // `PathFailure::description -> "xyzzy"` survived: the tests above assert + // which *failure* was reported, never what it says, so a description that + // collapsed every variant onto one string would go unnoticed. + // + // Distinctness is the assertion that matters. A description exists to tell + // one failure from another, so it catches every constant substitution at + // once rather than one string at a time -- and non-emptiness alone would + // not, because a constant is non-empty too. + let cases = [ + ("EmptyPath", PathFailure::EmptyPath), + ("InteriorNul", PathFailure::InteriorNul), + ("PathTooLong", PathFailure::PathTooLong), + ("NotFullyQualified", PathFailure::NotFullyQualified), + ("PathResolution", PathFailure::PathResolution), + ]; + + for (name, failure) in cases { + assert!( + !failure.description().is_empty(), + "{name} has no description, so a reader learns nothing from it" + ); + } + for (index, (name, failure)) in cases.iter().enumerate() { + for (other_name, other) in &cases[index + 1..] { + assert_ne!( + failure.description(), + other.description(), + "{name} and {other_name} describe themselves identically, so the \ + description cannot tell them apart" + ); + } + } +} + +#[test] +fn a_failure_decided_here_carries_no_os_error_and_renders_as_its_description() { + // The half of `PathError` that has no Win32 call behind it. Asserting the + // exact rendering -- rather than merely that it is non-empty -- is what + // binds `Display` to `description`: without it the formatter could drop the + // description entirely and nothing would fail. + let error = prepare_str("").expect_err("an empty path names nothing"); + + assert_eq!(error.failure(), PathFailure::EmptyPath); + assert_eq!(error.raw_os_error(), None); + assert!(std::error::Error::source(&error).is_none()); + assert_eq!(error.to_string(), PathFailure::EmptyPath.description()); +} diff --git a/crates/windows-namespace-request-sys/src/security/tests.rs b/crates/windows-namespace-request-sys/src/security/tests.rs index a23f8f59..7e6f9f02 100644 --- a/crates/windows-namespace-request-sys/src/security/tests.rs +++ b/crates/windows-namespace-request-sys/src/security/tests.rs @@ -509,3 +509,43 @@ fn a_capture_moves_and_shares_across_threads() { assert_eq!(observed, AclState::Populated(1)); } + +#[test] +fn a_capture_failure_exposes_its_os_error_both_ways() { + // `raw_os_error` survived replacement by `None`, `Some(0)`, `Some(1)`, and + // `Some(-1)`, and `source` survived replacement by `None`. The test above + // asserts the failure stage and the rendered message, neither of which + // touches either accessor. + // + // The two routes are asserted against each other rather than against a + // literal code: which error Windows reports for a zeroed descriptor is its + // business, but whatever it is must reach a caller identically through the + // typed accessor and through the standard `source` chain. That also rules + // out every constant the sweep tried, including the plausible-looking ones. + use std::error::Error as _; + + let zeroed = AlignedBuffer::zeroed(size_of::(), SELF_RELATIVE_ALIGNMENT); + + // SAFETY: the buffer outlives the call; its contents are not a valid + // descriptor, which is what makes this fail. + let error = unsafe { SecurityDescriptor::capture(zeroed.as_ptr().cast::()) } + .expect_err("a zeroed descriptor has revision 0 and cannot be valid"); + + let code = error + .raw_os_error() + .expect("this failure came from a Win32 call, so it carries a code"); + assert_ne!( + code, 0, + "a success code would mean the capture had not failed at all" + ); + + let source = error.source().expect("the OS error is the source"); + assert_eq!( + source + .downcast_ref::() + .expect("the source is the io::Error behind the failure") + .raw_os_error(), + Some(code), + "the typed accessor and the source chain must report the same error" + ); +} diff --git a/crates/windows-namespace-request-sys/src/volume/tests.rs b/crates/windows-namespace-request-sys/src/volume/tests.rs index c5af630b..2efcfda9 100644 --- a/crates/windows-namespace-request-sys/src/volume/tests.rs +++ b/crates/windows-namespace-request-sys/src/volume/tests.rs @@ -5,9 +5,10 @@ use std::fs::File; use std::os::windows::io::{AsHandle, AsRawHandle}; -use super::QueryVolumeInformation; +use super::{QueryVolumeInformation, VolumeInformation}; use crate::CapturedHandle; use crate::handle::tests::{Fixture, handle_allocation}; +use wtf_string::Wtf16String; fn request_for(file: &File) -> QueryVolumeInformation { QueryVolumeInformation::new( @@ -189,3 +190,50 @@ fn a_query_performs_the_same_way_on_another_thread() { assert_eq!(observed, expected); } + +#[test] +fn each_accessor_reports_its_own_field() { + // Four accessors -- `label`, `serial_number`, `maximum_component_length`, + // and `flags` -- all survived replacement by constants in a mutation run. + // The tests above query a *real* volume, so they cannot assert exact + // values: whatever this machine reports is what they get, and a constant is + // as plausible as the truth. + // + // Built directly rather than queried, because what is under test is the + // wiring between four same-typed fields and their accessors, not the query. + // Three of them are `u32` and nothing but distinct values can tell a + // transposition apart -- the failure this catches is an accessor returning + // its neighbour, which every real query would hide behind plausible numbers. + let information = VolumeInformation { + label: Wtf16String::from("LABEL"), + serial_number: 0x1111_1111, + maximum_component_length: 0x2222_2222, + flags: 0x3333_3333, + filesystem_name: Wtf16String::from("NTFS"), + }; + + assert_eq!(information.label().to_string_lossy(), "LABEL"); + assert_eq!(information.serial_number(), 0x1111_1111); + assert_eq!(information.maximum_component_length(), 0x2222_2222); + assert_eq!(information.flags(), 0x3333_3333); + assert_eq!(information.filesystem_name().to_string_lossy(), "NTFS"); + + // The property the three numeric assertions rest on. Stated rather than + // eyeballed, so a later edit that reused a value would fail here instead of + // silently weakening the test into one that cannot detect a transposition. + let numeric = [ + information.serial_number(), + information.maximum_component_length(), + information.flags(), + ]; + for (index, value) in numeric.iter().enumerate() { + for other in &numeric[index + 1..] { + assert_ne!(value, other, "two numeric fields share a value"); + } + } + assert_ne!( + information.label(), + information.filesystem_name(), + "the two string fields must differ, or one accessor could serve both" + ); +} diff --git a/crates/windows-namespace-request-sys/src/watch/tests.rs b/crates/windows-namespace-request-sys/src/watch/tests.rs index 1544f131..5f550b9f 100644 --- a/crates/windows-namespace-request-sys/src/watch/tests.rs +++ b/crates/windows-namespace-request-sys/src/watch/tests.rs @@ -289,3 +289,51 @@ fn a_watch_moves_to_another_thread_and_still_signals() { "a notification handle is usable from a thread that did not create it" ); } + +#[test] +fn the_configured_subtree_and_filter_read_back_through_their_accessors() { + // `subtree -> false` and `filter -> Default::default()` both survived a + // mutation run. Every test above builds a watch and then *performs* it, and + // the perform path reads the fields directly -- so nothing distinguished an + // accessor reporting the truth from one reporting a constant. + // + // `WatchDirectory::new` defaults `subtree` to false and the filter to an + // empty set, so both values here are deliberately the opposite: a `true` + // subtree, and a filter with bits set. A test that watched the default + // shape would pass against either constant. + let fixture = Fixture::new("watch-accessors"); + let filter = NotifyFilter::FILE_NAME | NotifyFilter::LAST_WRITE; + let request = watch_for(&fixture, filter).with_subtree(true); + + assert!( + request.subtree(), + "a caller that asked to watch the subtree must be able to see that it did" + ); + assert_eq!(request.filter(), filter); + assert_ne!( + filter, + NotifyFilter::default(), + "the filter under test must differ from the default, or a constant \ + accessor passes" + ); +} + +#[test] +fn an_unconfigured_watch_reads_back_as_the_narrowest_one() { + // The other half: the defaults are what `new` documents rather than what an + // accessor invents, and asserting them is what stops the test above from + // being satisfied by an accessor that always reports the configured shape. + let fixture = Fixture::new("watch-defaults"); + let text = fixture + .directory() + .to_str() + .expect("the fixture path is valid UTF-8"); + let request = + WatchDirectory::new(prepare(&Wtf16String::from(text)).expect("prepare the fixture path")); + + assert!( + !request.subtree(), + "watching a whole tree is the expensive choice and must be asked for" + ); + assert_eq!(request.filter(), NotifyFilter::default()); +} From d97778d942c787b649495dc7b08b8195cf265a7a Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Sun, 6 Sep 2026 19:55:33 -0400 Subject: [PATCH 2/5] docs: state what the `-sys` suffix means in this workspace Prompted by a direct question -- should `windows-namespace-request-sys` be `-sys` at all, given it wraps unsafe rather than declaring FFI? -- which turned out to be unanswerable from anything a reader can reach. By the usual ecosystem meaning the doubt is well founded: a `-sys` crate is normally raw FFI declarations, and this one takes its declarations from `windows-sys` and exposes 112 safe `pub fn` against 4 unsafe ones. By this workspace's meaning it is a textbook fit, because here the suffix marks a *layer* -- makes an existing Win32 API memory-safe without adding policy -- and this crate refuses policy explicitly: it schedules nothing, chooses no delivery model, and reports raw Win32 outcomes without normalising them. The two readings disagree, and nothing published said which applies. The convention existed only as a subordinate clause inside a decision about a *different* crate (why `windows-waitable-queues` is not `-sys`), and that decision is not on main at all -- see below. So: - The root README gains a `Crate naming` section, and is the statement of record. The convention governs published crate names, so it belongs where a reader meets the crates. - The crate README gains a short section answering the question for the one crate whose name provokes it, pointing at the root README. Kept deliberately brief so there is one statement and one pointer rather than two statements that can drift apart. Also corrects a stale line in the crate README: it said "Not yet released to crates.io" while the crate has been published since 2026-08-29 (0.2.0, verified against the crates.io API rather than assumed). Found while doing this, and NOT fixed here because it needs its own change: `crates/windows-waitable-queues/DESIGN-NOTES.md` links to `../../DESIGN-NOTES.md#the-waitable-queues-crate-is-named-plural-and-carries-no-sys-suffix`, and that section does not exist on main. The decision is 88 lines and lives only on mikegrier/deferred-namespace-ops, so the crate shipped with a dangling link to reasoning that never landed. Peeling it needs its own verification pass -- six cross-references, one of which (CHECKLIST-io-domains.md) is also branch-only -- so this commit deliberately does not cite that anchor rather than adding a seventh reference to a missing referent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- README.md | 26 +++++++++++++++++++ .../windows-namespace-request-sys/README.md | 24 +++++++++++++++-- 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index d9580120..c67acec9 100644 --- a/README.md +++ b/README.md @@ -27,6 +27,32 @@ code lives behind `cfg(windows)`. The workspace's `wtf-string` crate is the exception -- its portable core has no `cfg(windows)` gating, so CI additionally builds, tests, and lints it on Linux and macOS. +## Crate naming + +The `-sys` suffix here does **not** carry its usual ecosystem meaning. A `-sys` +crate is normally raw FFI declarations with no abstraction over them; every +`windows-*-sys` crate in this workspace instead wraps a great deal of `unsafe`, +and takes its declarations from +[`windows-sys`](https://crates.io/crates/windows-sys) rather than making its own. + +What the suffix marks is a **layer**: + +- **`windows-*-sys`** -- makes an existing Win32 API memory-safe *without adding + policy*. It schedules nothing, picks no delivery model, and reports the + platform's outcomes unnormalised. +- **no suffix** -- decides something Win32 has no equivalent of. + `windows-waitable-queues` is the worked example: it chooses a slot protocol, an + overflow policy, and a signalling discipline, so calling it `-sys` would + misdescribe how much it decides on the caller's behalf. + +So the presence of `unsafe` wrappers is not evidence either way -- it is the job +description of the first kind. The distinction is *policy*, and the suffix is the +only signal a reader has for how much a crate decides for them. + +This section is the statement of record. The convention governs published crate +names, so it belongs where a reader meets the crates rather than only in the +design notes. + ## Build Requires Rust `1.98` or newer. diff --git a/crates/windows-namespace-request-sys/README.md b/crates/windows-namespace-request-sys/README.md index 6851d262..8688aebb 100644 --- a/crates/windows-namespace-request-sys/README.md +++ b/crates/windows-namespace-request-sys/README.md @@ -32,6 +32,25 @@ and unassociated, because associating it with a completion port irreversibly forecloses `IoRing` use of it, and that choice belongs to a layer that knows the handle's destination. +## Why the `-sys` suffix + +Not in the usual sense. A `-sys` crate elsewhere in the ecosystem is normally +raw FFI declarations with no abstraction over them, and by that reading this +crate is misnamed: it declares almost no FFI of its own and takes its +declarations from [`windows-sys`](https://crates.io/crates/windows-sys). + +In this workspace the suffix marks a **layer**, not a linking strategy: a +`windows-*-sys` crate makes an existing Win32 API memory-safe **without adding +policy**, and a crate that decides something Win32 has no equivalent of drops +the suffix. That is why `windows-waitable-queues` carries no `-sys` -- it picks +a slot protocol and an overflow policy -- while this crate does, despite +wrapping a great deal of `unsafe`. Everything above is the suffix being earned: +it schedules nothing, chooses no delivery model, and reports raw Win32 outcomes +without normalising them. + +The convention is stated for the whole workspace in the repository's +[README](https://github.com/MikeGrier/windows-threadpool-sys#crate-naming). + ## A path is copied; a handle is duplicated Several entries take a handle rather than a path, and a request owns a @@ -127,5 +146,6 @@ operation coverage (every audited call site re-expressed) and scenario coverage context, many requests across concurrent workers from one shared capture, and a handle opened by one request carried into a later one). -Design decisions are recorded in [DESIGN-NOTES.md](DESIGN-NOTES.md). Not yet -released to crates.io. +Design decisions are recorded in [DESIGN-NOTES.md](DESIGN-NOTES.md). Published +on crates.io as +[`windows-namespace-request-sys`](https://crates.io/crates/windows-namespace-request-sys). From f8ce06215bab8bab214bee0d458c6501bbf00f16 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Sun, 6 Sep 2026 20:07:44 -0400 Subject: [PATCH 3/5] test: measure path lengths in the unit Win32 actually uses `absolute_path_of_length` documents itself as producing "exactly `units` UTF-16 units" and then computed the suffix from `prefix.len()` (UTF-8 bytes) while its callers checked the result with `chars().count()` (scalars). Three units in one helper. They coincide for the ASCII it builds, so it passed -- and would have gone on passing while measuring the wrong thing the moment a case used a character that is not one byte, one scalar, and one UTF-16 unit at once. UTF-16 is the unit that matters here: `MAX_PATH` is a count of `WCHAR`, and the implementation is already correct -- `units.len()` on a `Wtf16Str` is a UTF-16 count. Only the tests were loose. Corrected in both crates that carry the helper. It is duplicated verbatim, including the doc comment that misdescribed it, and the test file's own header notes the two crates carry near-identical path contracts. Fixing one copy would have left the workspace with two versions of the same helper disagreeing about what it measures, which is worse than the original defect. windows-namespace-request-sys helper, both length assertions, and the `> 260` fixture checks in final_path and full_path, which compared a WCHAR limit against a scalar count and said "chars" in the message windows-file-enumeration-sys the duplicated helper and its two assertions Adds a test that separates the units, since a fix nothing can fail is not worth much: a path of `C:\` plus 128 astral characters is 259 UTF-16 units but only 131 `char`s, so an implementation counting scalars would admit the 260-unit version -- the expensive direction, since the caller is told the open may proceed. What that test pins is stated carefully, because sabotage showed the obvious claim was wrong. Disabling the pre-check alone leaves it passing: `GetFullPathNameW` reports the resolved length in UTF-16 units and a second guard refuses on that, so the contract survives a scalar-counting pre-check. It takes disabling both to make the test fail. So it is named for the contract it covers rather than for arithmetic at any one site, and the comment records that the post-check is the stronger of the two -- its count comes from Windows and therefore cannot be in the wrong unit -- which is worth knowing before anyone removes the pre-check as redundant. 286 tests pass in windows-namespace-request-sys, 377 in windows-file-enumeration-sys, workspace clippy --all-targets --all-features clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/path/tests.rs | 15 ++++-- .../windows-namespace-request-sys/README.md | 2 +- .../src/final_path/tests.rs | 6 +-- .../src/full_path/tests.rs | 6 +-- .../src/path/tests.rs | 54 +++++++++++++++++-- 5 files changed, 70 insertions(+), 13 deletions(-) diff --git a/crates/windows-file-enumeration-sys/src/path/tests.rs b/crates/windows-file-enumeration-sys/src/path/tests.rs index 0e5013da..be4a7ac6 100644 --- a/crates/windows-file-enumeration-sys/src/path/tests.rs +++ b/crates/windows-file-enumeration-sys/src/path/tests.rs @@ -181,9 +181,18 @@ fn a_device_namespace_path_is_resolved_rather_than_kept_verbatim() { /// An absolute path of exactly `units` UTF-16 units, already in normal form so /// `GetFullPathNameW` returns it unchanged and the resolved length is the input /// length. +/// +/// UTF-16 units and not bytes or `char`s, because that is the unit Win32 +/// measures a path in: `MAX_PATH` is a count of `WCHAR`. The three coincide for +/// the ASCII this builds, so counting the prefix any other way would pass today +/// and quietly measure the wrong thing the moment a case uses a character that +/// is not one byte, one scalar, and one unit at once. fn absolute_path_of_length(units: usize) -> String { let prefix = r"C:\"; - format!("{prefix}{}", "a".repeat(units - prefix.len())) + format!( + "{prefix}{}", + "a".repeat(units - prefix.encode_utf16().count()) + ) } #[test] @@ -193,7 +202,7 @@ fn an_ordinary_path_of_exactly_max_path_content_is_accepted() { // cannot see, and it is the expensive direction: it refuses a path Windows // would have opened. let path = absolute_path_of_length(259); - assert_eq!(path.chars().count(), 259); + assert_eq!(path.encode_utf16().count(), 259); let prepared = prepare_str(&path).expect("259 units is within the ordinary limit"); assert_eq!(text(&prepared), path); @@ -203,7 +212,7 @@ fn an_ordinary_path_of_exactly_max_path_content_is_accepted() { fn an_ordinary_path_one_unit_past_max_path_content_is_rejected() { // 260 counts the terminator, so 260 content units do not fit. let path = absolute_path_of_length(260); - assert_eq!(path.chars().count(), 260); + assert_eq!(path.encode_utf16().count(), 260); let error = prepare_str(&path).expect_err("260 units leaves no room for the terminator"); assert_eq!(error.failure(), RequestFailure::PathTooLong); diff --git a/crates/windows-namespace-request-sys/README.md b/crates/windows-namespace-request-sys/README.md index 8688aebb..a1a5e075 100644 --- a/crates/windows-namespace-request-sys/README.md +++ b/crates/windows-namespace-request-sys/README.md @@ -49,7 +49,7 @@ it schedules nothing, chooses no delivery model, and reports raw Win32 outcomes without normalising them. The convention is stated for the whole workspace in the repository's -[README](https://github.com/MikeGrier/windows-threadpool-sys#crate-naming). +[README](../../README.md#crate-naming). ## A path is copied; a handle is duplicated diff --git a/crates/windows-namespace-request-sys/src/final_path/tests.rs b/crates/windows-namespace-request-sys/src/final_path/tests.rs index 4b700004..6a1d1636 100644 --- a/crates/windows-namespace-request-sys/src/final_path/tests.rs +++ b/crates/windows-namespace-request-sys/src/final_path/tests.rs @@ -117,9 +117,9 @@ fn the_buffer_grows_for_a_path_longer_than_the_first_attempt() { .to_string_lossy(); assert!( - resolved.chars().count() > 260, - "the fixture must actually exceed the first attempt: {} chars", - resolved.chars().count() + resolved.encode_utf16().count() > 260, + "the fixture must actually exceed the first attempt: {} UTF-16 units", + resolved.encode_utf16().count() ); assert!(resolved.ends_with("f.t"), "unexpected: {resolved}"); } diff --git a/crates/windows-namespace-request-sys/src/full_path/tests.rs b/crates/windows-namespace-request-sys/src/full_path/tests.rs index 7997dd5f..a8d734ea 100644 --- a/crates/windows-namespace-request-sys/src/full_path/tests.rs +++ b/crates/windows-namespace-request-sys/src/full_path/tests.rs @@ -102,9 +102,9 @@ fn a_deeply_nested_path_beyond_the_first_attempt_still_resolves() { let resolved = resolve(&path); assert!( - resolved.chars().count() > 260, - "the fixture must exceed the first attempt: {} chars", - resolved.chars().count() + resolved.encode_utf16().count() > 260, + "the fixture must exceed the first attempt: {} UTF-16 units", + resolved.encode_utf16().count() ); assert!(resolved.ends_with("file.txt"), "unexpected: {resolved}"); } diff --git a/crates/windows-namespace-request-sys/src/path/tests.rs b/crates/windows-namespace-request-sys/src/path/tests.rs index 7538862d..85ec7d59 100644 --- a/crates/windows-namespace-request-sys/src/path/tests.rs +++ b/crates/windows-namespace-request-sys/src/path/tests.rs @@ -253,9 +253,18 @@ fn an_error_without_an_os_code_renders_only_its_description() { /// An absolute path of exactly `units` UTF-16 units, already in normal form so /// `GetFullPathNameW` returns it unchanged and the resolved length equals the /// input length. +/// +/// UTF-16 units and not bytes or `char`s, because that is the unit Win32 +/// measures a path in: `MAX_PATH` is a count of `WCHAR`. The three coincide for +/// the ASCII this builds, so counting the prefix any other way would pass today +/// and quietly measure the wrong thing the moment a case uses a character that +/// is not one byte, one scalar, and one unit at once. fn absolute_path_of_length(units: usize) -> String { let prefix = r"C:\"; - format!("{prefix}{}", "a".repeat(units - prefix.len())) + format!( + "{prefix}{}", + "a".repeat(units - prefix.encode_utf16().count()) + ) } #[test] @@ -264,7 +273,7 @@ fn an_ordinary_path_of_exactly_max_path_content_is_accepted() { // Rejecting it is the off-by-one a "much too long" test cannot see, and it // is the expensive direction: it refuses a path Windows would have opened. let path = absolute_path_of_length(259); - assert_eq!(path.chars().count(), 259); + assert_eq!(path.encode_utf16().count(), 259); let prepared = prepare_str(&path).expect("259 units is within the ordinary limit"); assert_eq!(text(&prepared), path); @@ -273,7 +282,7 @@ fn an_ordinary_path_of_exactly_max_path_content_is_accepted() { #[test] fn an_ordinary_path_one_unit_past_max_path_content_is_rejected() { let path = absolute_path_of_length(260); - assert_eq!(path.chars().count(), 260); + assert_eq!(path.encode_utf16().count(), 260); let error = prepare_str(&path).expect_err("260 units leaves no room for the terminator"); assert_eq!(error.failure(), PathFailure::PathTooLong); @@ -288,6 +297,45 @@ fn the_ordinary_limit_is_one_less_than_max_path() { assert_eq!(MAX_PATH_CONTENT, 259); } +#[test] +fn a_path_whose_character_count_hides_its_utf16_length_is_still_refused() { + // The two tests above build ASCII, where bytes, `char`s and UTF-16 units are + // the same number, so a path they accept or refuse says nothing about which + // unit was counted. This one separates them: every astral character is one + // `char` and *two* UTF-16 units, so a path measured in scalars looks about + // half as long as Windows considers it. + // + // What this pins is the **contract** -- such a path is still refused -- and + // not any single site's arithmetic. Measured, because the distinction is not + // obvious: `prepare` checks the length twice, and sabotaging only the + // pre-check leaves this test passing, because `GetFullPathNameW` reports the + // resolved length in UTF-16 units and the post-check refuses on that. It + // takes disabling *both* to make this test fail. That second guard is the + // stronger one -- its count comes from Windows and so cannot be in the wrong + // unit -- which is worth knowing before anyone "simplifies" the pre-check + // away as redundant. + let astral = '\u{1F600}'; + assert_eq!(astral.len_utf16(), 2, "the premise of this test"); + + // 3 units of `C:\` plus 128 astral characters is exactly the limit. + let accepted = format!(r"C:\{}", astral.to_string().repeat(128)); + assert_eq!(accepted.encode_utf16().count(), 259); + assert_eq!(accepted.chars().count(), 131); + + let prepared = prepare_str(&accepted).expect("259 UTF-16 units is within the limit"); + assert_eq!(text(&prepared), accepted); + + // One more unit is over it. A limit enforced on `chars().count()` would see + // 132 against a bound of 259 and admit a path Windows refuses -- which is + // the expensive direction, since the caller is told the open may proceed. + let rejected = format!("{accepted}a"); + assert_eq!(rejected.encode_utf16().count(), 260); + assert_eq!(rejected.chars().count(), 132); + + let error = prepare_str(&rejected).expect_err("260 UTF-16 units is past the limit"); + assert_eq!(error.failure(), PathFailure::PathTooLong); +} + #[test] fn a_verbatim_drive_relative_path_with_a_separator_is_rejected() { // `\\?\C:foo` has no separator at all, so it is refused before the root is From 4e178cabe3a47f05f7eeb866b065294804e7b603 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Sun, 6 Sep 2026 20:46:06 -0400 Subject: [PATCH 4/5] test(namespace-request): say "above U+FFFF" instead of "astral" "Astral plane" is real jargon -- it is what the early Perl/Unicode community called Unicode's supplementary planes -- but it is obscure enough to cost a reader, which is the opposite of what a test about precise unit counting should do. Raised by a reviewer with decades of Unicode work who had not met the term. The test now says what it means: a character above `U+FFFF` is one `char` but two UTF-16 units, because UTF-16 encodes it as a surrogate pair. That is the whole property the test depends on, and stating it plainly is shorter than the jargon plus the explanation the jargon needs. `supplementary` is the Unicode standard's own word and is used for the binding. Scope: this corrects only the site added by this branch. The term is also used in 19 places that predate it -- `wtf-string` (test names, and matrix case keys such as `astral_min` / `astral_max`), and one comment in `windows-file-watcher` -- and those are left alone here rather than folded into a test-coverage PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/path/tests.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/crates/windows-namespace-request-sys/src/path/tests.rs b/crates/windows-namespace-request-sys/src/path/tests.rs index 85ec7d59..29e12d05 100644 --- a/crates/windows-namespace-request-sys/src/path/tests.rs +++ b/crates/windows-namespace-request-sys/src/path/tests.rs @@ -301,9 +301,10 @@ fn the_ordinary_limit_is_one_less_than_max_path() { fn a_path_whose_character_count_hides_its_utf16_length_is_still_refused() { // The two tests above build ASCII, where bytes, `char`s and UTF-16 units are // the same number, so a path they accept or refuse says nothing about which - // unit was counted. This one separates them: every astral character is one - // `char` and *two* UTF-16 units, so a path measured in scalars looks about - // half as long as Windows considers it. + // unit was counted. This one separates them: a character above `U+FFFF` (a + // supplementary character) is one `char` but *two* UTF-16 units, because + // UTF-16 encodes it as a surrogate pair. A path built from those measured in + // scalars looks about half as long as Windows considers it. // // What this pins is the **contract** -- such a path is still refused -- and // not any single site's arithmetic. Measured, because the distinction is not @@ -314,11 +315,11 @@ fn a_path_whose_character_count_hides_its_utf16_length_is_still_refused() { // stronger one -- its count comes from Windows and so cannot be in the wrong // unit -- which is worth knowing before anyone "simplifies" the pre-check // away as redundant. - let astral = '\u{1F600}'; - assert_eq!(astral.len_utf16(), 2, "the premise of this test"); + let supplementary = '\u{1F600}'; + assert_eq!(supplementary.len_utf16(), 2, "the premise of this test"); - // 3 units of `C:\` plus 128 astral characters is exactly the limit. - let accepted = format!(r"C:\{}", astral.to_string().repeat(128)); + // 3 units of `C:\` plus 128 two-unit characters is exactly the limit. + let accepted = format!(r"C:\{}", supplementary.to_string().repeat(128)); assert_eq!(accepted.encode_utf16().count(), 259); assert_eq!(accepted.chars().count(), 131); From 2bcc9326127caf1c56c0714c7be50c9058b65914 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Sun, 6 Sep 2026 21:14:33 -0400 Subject: [PATCH 5/5] test: guard absolute_path_of_length against an underflowing length `units - prefix_units` underflows for any `units` below 3, so a future case asking for a 2-unit path would get "attempt to subtract with overflow" from a line that does not mention paths or prefixes. The helper now checks first and says which two numbers disagree. Hoists `prefix.encode_utf16().count()` into `prefix_units` so the guard and the subtraction cannot drift apart -- the failure this replaces is only reachable if they do. Applied to both copies of the helper, in windows-namespace-request-sys and windows-file-enumeration-sys, for the same reason the previous commit corrected both: leaving one guarded and one not is how two copies of the same code start to differ. Verified by execution rather than by reading: a temporary `#[should_panic(expected = "the C:\\ prefix is already 3 units")]` case calling `absolute_path_of_length(2)` passed, confirming the guard fires and that the message names the prefix and its width. Removed again -- a helper's guard is not worth a permanent test, but is worth having actually run once. 286 tests pass in windows-namespace-request-sys, 377 in windows-file-enumeration-sys, workspace clippy --all-targets --all-features clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../windows-file-enumeration-sys/src/path/tests.rs | 12 ++++++++---- .../windows-namespace-request-sys/src/path/tests.rs | 12 ++++++++---- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/crates/windows-file-enumeration-sys/src/path/tests.rs b/crates/windows-file-enumeration-sys/src/path/tests.rs index be4a7ac6..fa68f16c 100644 --- a/crates/windows-file-enumeration-sys/src/path/tests.rs +++ b/crates/windows-file-enumeration-sys/src/path/tests.rs @@ -189,10 +189,14 @@ fn a_device_namespace_path_is_resolved_rather_than_kept_verbatim() { /// is not one byte, one scalar, and one unit at once. fn absolute_path_of_length(units: usize) -> String { let prefix = r"C:\"; - format!( - "{prefix}{}", - "a".repeat(units - prefix.encode_utf16().count()) - ) + let prefix_units = prefix.encode_utf16().count(); + assert!( + units >= prefix_units, + "asked for a {units}-unit path, but the {prefix} prefix is already \ + {prefix_units} units; the subtraction below would underflow and panic \ + without saying why" + ); + format!("{prefix}{}", "a".repeat(units - prefix_units)) } #[test] diff --git a/crates/windows-namespace-request-sys/src/path/tests.rs b/crates/windows-namespace-request-sys/src/path/tests.rs index 29e12d05..dbb4aac2 100644 --- a/crates/windows-namespace-request-sys/src/path/tests.rs +++ b/crates/windows-namespace-request-sys/src/path/tests.rs @@ -261,10 +261,14 @@ fn an_error_without_an_os_code_renders_only_its_description() { /// is not one byte, one scalar, and one unit at once. fn absolute_path_of_length(units: usize) -> String { let prefix = r"C:\"; - format!( - "{prefix}{}", - "a".repeat(units - prefix.encode_utf16().count()) - ) + let prefix_units = prefix.encode_utf16().count(); + assert!( + units >= prefix_units, + "asked for a {units}-unit path, but the {prefix} prefix is already \ + {prefix_units} units; the subtraction below would underflow and panic \ + without saying why" + ); + format!("{prefix}{}", "a".repeat(units - prefix_units)) } #[test]