Peel namespace-request tests, plus the -sys naming docs and a UTF-16 path-length fix

[MikeGrier]

Started as a peel of 104 test cases off mikegrier/deferred-namespace-ops (#56). Review then turned up two things that could not honestly be left out, so the scope is wider than the title suggests and the description below is written to the actual changed files rather than the original intent.

No source behaviour changes. Everything here is tests or documentation, typed test / docs, so release-please cuts no version for any crate.

What is in this PR

1. The peel -- 104 test cases for windows-namespace-request-sys

module	lines
path	134	the largest gap -- path handling is where a namespace crate is most exposed to odd input
open_by_id	78	identifier-based opens
open	66
volume	50
watch	48
security	40

Verified portable before the branch was cut, not after: the patch was applied to a scratch worktree checked out at origin/main with no other branch content, and the suite passed there. It carries no cross-references, so it cannot leave a dangling link on the source branch -- which three earlier peels each did.

2. Documentation: what the -sys suffix means (root README + crate README)

Prompted by a direct question -- should this crate 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: this crate 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. The convention existed only as a subordinate clause inside a decision about a different crate, so it is now stated in the root README (the record) with a short pointer from the crate README.

Also corrects a stale line: the crate README claimed "Not yet released to crates.io" while it has been published since 2026-08-29 (checked against the crates.io API).

3. Path lengths measured in the unit Win32 uses (windows-namespace-request-sys and windows-file-enumeration-sys)

absolute_path_of_length documented itself as producing UTF-16 units, computed the suffix from UTF-8 bytes, and had its result checked in scalars -- three units in one helper. They coincide for ASCII, so it passed while measuring the wrong thing.

The helper is duplicated verbatim into windows-file-enumeration-sys, doc comment included. Fixing one copy would leave two versions disagreeing about what they measure, so both are corrected. That is why this PR touches a second crate.

Adds a test that separates the units, and a guard against an underflowing length in both copies.

Why the scope grew, stated plainly

An earlier version of this description said "Tests only" and "nothing outside the crate touched". That was true when written and false by the time the review caught it -- the reviewer was right to flag it, and it is the same defect as overclaiming a sweep. The scope grew twice, each time for a reason recorded above, and this description is now written from git diff --name-only origin/main...HEAD rather than from memory.

Verification

286 tests pass in windows-namespace-request-sys, 377 in windows-file-enumeration-sys. Workspace clippy --all-targets --all-features clean, cargo fmt clean, encoding check clean, check-commit-scope confirms no release-triggering commit.

[Copilot]

🟢 Approval recommended

The changes are isolated to tests, are internally consistent, and add concrete coverage for previously-surviving mutation scenarios without altering production behavior.

Pull request overview

Adds a set of new, mutation-sweep-driven test cases in windows-namespace-request-sys to harden accessor/default wiring and boundary behavior across the six request families, without changing production code.

Changes:

• Add targeted tests that verify accessor methods report their configured fields (and defaults) rather than returning constants.
• Add path-boundary tests around the ordinary MAX_PATH content limit and a few additional contract edge cases.
• Widen a couple of use imports to support the new test cases (VolumeInformation, SecurityAttributes, Wtf16String).

File summaries

File	Description
crates/windows-namespace-request-sys/src/watch/tests.rs	Adds tests asserting WatchDirectory subtree/filter accessors reflect both configured and default values.
crates/windows-namespace-request-sys/src/volume/tests.rs	Adds an accessor-wiring test for VolumeInformation using constructed, distinct field values.
crates/windows-namespace-request-sys/src/security/tests.rs	Adds a test that cross-checks raw_os_error() against the standard source() chain on capture failure.
crates/windows-namespace-request-sys/src/path/tests.rs	Adds boundary/contract tests for ordinary path length limits, drive-designator rejection, and distinct failure descriptions.
crates/windows-namespace-request-sys/src/open/tests.rs	Adds tests asserting configured parameters read back through accessors and that defaults remain “unset” (zero/None).
crates/windows-namespace-request-sys/src/open_by_id/tests.rs	Adds accessor/default tests for the open-by-id request, including supplied SecurityAttributes visibility.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Minor but concrete documentation/test-correctness issues were found (relative link robustness and UTF-16 unit accounting in a path-length helper).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

crates/windows-namespace-request-sys/src/path/tests.rs:277

• absolute_path_of_length is documented as producing a path of N UTF-16 units, but it computes the suffix length using prefix.len() (UTF-8 bytes) and the tests verify length via chars().count() (Unicode scalar values). This happens to work for ASCII, but it makes the helper/doc misleading and risks future edits accidentally testing the wrong unit; use encode_utf16().count() for both the prefix and the assertions so the test matches the stated contract.

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Lite

[MikeGrier]

The UTF-16 unit accounting (suppressed comment on path/tests.rs:277)

Right, and worse than stated: the helper used three different units in one function. Documented as UTF-16 units, computed the suffix from prefix.len() (UTF-8 bytes), and its callers asserted with chars().count() (scalars). All three are 3 for C:\, so it passed and would have kept passing while measuring the wrong thing.

UTF-16 is the unit that matters -- MAX_PATH is a count of WCHAR -- and the implementation was already correct: units.len() on a Wtf16Str is a UTF-16 count. Only the tests were loose.

Swept the class rather than the site

The helper is duplicated verbatim into windows-file-enumeration-sys, doc comment and all, and this test file's own header says the two crates carry near-identical path contracts. Fixing one copy would have left two versions of the same helper disagreeing about what they measure -- worse than the original defect -- so both are corrected. That makes this PR span two crates; both changes are test-only and typed test, so no release is triggered.

Also corrected two same-crate siblings the sweep turned up: final_path and full_path asserted resolved.chars().count() > 260 against a WCHAR limit, and reported it as "chars" in the failure message.

The new test, and a correction to what I first claimed about it

A fix nothing can fail is not worth much, so I added a case that separates the units: C:\ plus 128 astral characters is 259 UTF-16 units but only 131 chars, so a scalar-counting implementation would admit the 260-unit version -- the expensive direction, since the caller is told the open may proceed.

I then sabotaged the implementation to check the test was load-bearing, and it was not, in the way I had assumed. Disabling the pre-check alone leaves the test passing: GetFullPathNameW reports the resolved length in UTF-16 units and a second guard refuses on that. It takes disabling both to make the test fail.

So I renamed it from the_ordinary_limit_counts_utf16_units_and_not_characters -- which claimed to pin one site's arithmetic -- to a_path_whose_character_count_hides_its_utf16_length_is_still_refused, which is the contract it actually covers. The comment records the two-guard finding, including that the post-check is the stronger one (its count comes from Windows, so it cannot be in the wrong unit) -- worth knowing before someone removes the pre-check as redundant.

Had I not run the sabotage I would have shipped a test whose name asserted something it does not establish.

286 tests pass in windows-namespace-request-sys, 377 in windows-file-enumeration-sys, workspace clippy --all-targets --all-features clean.

[Copilot]

🔵 Needs a closer look

The PR’s scope/description claims (“tests only” / “nothing outside the crate touched”) don’t match the actual changed files, so the PR metadata or split needs to be corrected.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

README.md:34

• The PR description says "Tests only" / "nothing outside the crate touched", but this change set also updates the workspace root README (crate naming guidance) and touches crates/windows-file-enumeration-sys/src/path/tests.rs. Please either (a) update the PR description/scope statements (and implied test:-only framing) to match what is actually changing, or (b) split the non-namespace-request changes into a separate PR.

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The PR’s stated scope in the description (“tests only”, “nothing outside the crate touched”) does not match the actual changed files, and the new path-length helper should avoid underflow panics with a clear guard.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

README.md:54

• The PR description says "Tests only" and "nothing outside the crate touched", but this PR also changes workspace documentation (root README + crate README) and updates tests in a different crate (crates/windows-file-enumeration-sys). Please either update the PR title/description to match the actual scope, or split the non-namespace-request changes into separate PRs so the peel stays as advertised.
  crates/windows-file-enumeration-sys/src/path/tests.rs:195
• absolute_path_of_length will panic if called with units shorter than the C:\ prefix (the subtraction underflows). Adding an explicit assertion makes the helper safer and produces a clearer failure if a future test accidentally passes a too-small value.
  crates/windows-namespace-request-sys/src/path/tests.rs:267
• absolute_path_of_length will panic if called with units shorter than the C:\ prefix (the subtraction underflows). It’s only a test helper, but adding an explicit assertion makes failures clearer and prevents accidental misuse in future cases.

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Lite

[MikeGrier]

All three addressed in 2bcc932 plus a rewritten description.

Scope claim -- you are right, and it is the same defect twice

The description said "Tests only" and "nothing outside the crate touched". That was true when written and false by the time you read it: the scope grew twice, and each time I appended a new section rather than fixing the claim at the top. That is exactly the overclaiming I was pulled up for on #69, in a different shape -- a stale assertion left standing above accurate detail.

I chose to widen the description rather than split, and the reason is in the finding itself: the second crate is there because absolute_path_of_length is duplicated verbatim into windows-file-enumeration-sys, doc comment and all. Splitting it would ship one guarded, unit-correct copy and one unguarded copy still measuring bytes -- two versions of the same helper disagreeing about what it measures, which is worse than either the original defect or the wider PR.

The title and description are rewritten from git diff --name-only origin/main...HEAD rather than from memory, and the description now says up front that the scope is wider than the title, why it grew, and that the earlier claim was wrong.

The underflow guard, in both copies

units - prefix_units underflows below 3, and the panic would have been "attempt to subtract with overflow" from a line that mentions neither paths nor prefixes. Now:

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"
);

I hoisted prefix.encode_utf16().count() into prefix_units so the guard and the subtraction cannot drift apart -- the failure the guard replaces is only reachable if they do.

Verified by execution, not by reading. A temporary #[should_panic(expected = "the C:\\ prefix is already 3 units")] case calling absolute_path_of_length(2) passed, which confirms the guard fires and that the message names the prefix and its width. Removed again -- a test helper's guard is not worth a permanent test, but it was worth running once rather than assuming the assertion was well formed.

Applied to both copies, for the same reason both were corrected in the previous commit.

286 tests pass in windows-namespace-request-sys, 377 in windows-file-enumeration-sys, workspace clippy clean.

[Copilot]

🔵 Needs a closer look

One newly added security test asserts a non-zero last-error code for an API (IsValidSecurityDescriptor) that is not documented to set last-error, making the test potentially flaky.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

crates/windows-namespace-request-sys/src/security/tests.rs:550

• This test assumes the capture failure always carries a non-zero Win32 last-error code (and panics if it doesn't). The failure path begins with IsValidSecurityDescriptor, which is not documented to set the thread last-error value, so io::Error::last_os_error() may be 0 or a stale value depending on prior calls; that makes this assertion potentially flaky and not a reliable property to test here. Instead, compare the typed accessor result and the source chain directly as Option<i32> without requiring it to be non-zero/present.

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The changes are confined to tests and documentation, and the updated tests/documentation appear internally consistent and aligned with Win32 UTF-16 length semantics.

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Lite