From 5613ae97aaa6c6d84f3477598f5b1b0a3662242e Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Sun, 6 Sep 2026 21:27:03 -0400 Subject: [PATCH] test(namespace-request): stop asserting an undocumented last-error value `a_capture_failure_exposes_its_os_error_both_ways` required the capture failure to carry a present, non-zero Win32 code. That failure path takes `GetLastError` after `IsValidSecurityDescriptor`, which Windows does not document as setting last-error, so the assertion demanded something the platform never promised. Measured rather than reasoned about, because "not documented" and "does not happen" are different claims and this repository has been wrong about that distinction before. A probe poisoned last-error with 0x0DEFACED and then cleared it to 0, capturing a zeroed descriptor after each: round 0: raw_os_error=Some(1338) poisoned=false zero=false round 1: raw_os_error=Some(1338) poisoned=false zero=false round 2: raw_os_error=Some(1338) poisoned=false zero=false after SetLastError(0): raw_os_error=Some(1338) So `IsValidSecurityDescriptor` does set last-error, to 1338 (`ERROR_INVALID_SECURITY_DESCR`), and overwrites both a stale value and a cleared one. The assertion was not flaky in practice. It comes out anyway, and the measurement is why rather than a reason to keep it. An undocumented behaviour that happens to hold is exactly what this workspace's rule on depending on specified primitives says not to bind to: a test demanding it would assert Windows' incidental behaviour rather than this crate's contract, and would fail on a Windows build that stopped setting the value without anything in this crate being wrong. What the crate does promise is that its typed accessor and the standard `source` chain report the same thing, and that holds whatever Windows reports -- including nothing at all. Both sides are now compared as `Option`. Confirmed the weaker assertion is not a weaker test: the comment above it claims `raw_os_error` survived replacement by `None`, and removing the `.expect()` could have made that stale. Re-injected that mutant -- the test still fails, on `left: Some(1338), right: None`, because the comparison is between the crate's accessor and `std`'s, so only one side moves. Reported on #74 after it merged, so this is its own commit against main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/security/tests.rs | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/crates/windows-namespace-request-sys/src/security/tests.rs b/crates/windows-namespace-request-sys/src/security/tests.rs index 7e6f9f028..621787a91 100644 --- a/crates/windows-namespace-request-sys/src/security/tests.rs +++ b/crates/windows-namespace-request-sys/src/security/tests.rs @@ -531,21 +531,29 @@ fn a_capture_failure_exposes_its_os_error_both_ways() { 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" - ); - + // Compared as `Option` on both sides, and deliberately *not* asserted to + // be present or non-zero. This failure path takes `GetLastError` after + // `IsValidSecurityDescriptor`, which Windows does not document as setting it, + // so any particular value here is observed behaviour rather than a promise. + // + // Measured on this machine: it does set it, to 1338 + // (`ERROR_INVALID_SECURITY_DESCR`), and does so even when last-error is + // poisoned with an unrelated value or cleared to 0 beforehand -- so the code + // is neither stale nor absent in practice. That is exactly why it is not + // asserted: an undocumented behaviour that happens to hold is the kind of + // thing this crate binds to a specification instead, and a test demanding it + // would be asserting the platform's incidental behaviour rather than this + // crate's contract. + // + // What *is* this crate's contract is that both routes agree, which holds + // whatever Windows reports -- including nothing 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), + error.raw_os_error(), "the typed accessor and the source chain must report the same error" ); }