Stop asserting an undocumented last-error value in the security capture test - #76
Merged
Merged
Conversation
`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<i32>`. 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>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change correctly narrows the test assertion to the crate’s documented contract without affecting production behavior.
Pull request overview
This PR updates a windows-namespace-request-sys security capture test to stop asserting a specific/non-zero Win32 last-error value on a failure path where Windows does not document last-error semantics, and instead asserts only the crate’s contract: the typed accessor and the source chain agree.
Changes:
- Remove the test’s requirement that the capture failure must carry a present, non-zero Win32 error code.
- Compare the error code via
Option<i32>betweenSecurityCaptureError::raw_os_error()and thestd::error::Error::source()chain, without asserting a particular value.
File summaries
| File | Description |
|---|---|
| crates/windows-namespace-request-sys/src/security/tests.rs | Adjusts the security capture failure test to avoid asserting an undocumented last-error invariant and instead assert accessor/source consistency. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+548
to
+549
| // What *is* this crate's contract is that both routes agree, which holds | ||
| // whatever Windows reports -- including nothing at all. |
MikeGrier
pushed a commit
that referenced
this pull request
Sep 7, 2026
Brings in everything peeled and reviewed since the last merge: the namespace-request tests (#74), the Unicode terminology sweep (#75), the security last-error correction (#76), the flush-barrier instruments (#77), and the dangling-anchor repairs (#78). Two conflicts, both in files this branch originated and main has since improved through review. Took main's side in every hunk, which is the whole point of the peel-and-review cycle -- the branch holds the drafts, main holds what survived: path/tests.rs the branch still measured path lengths in bytes and scalars; main's version measures UTF-16 units, guards the helper against an underflowing length, and adds the case that separates the units security/tests.rs the branch still asserted a non-zero last-error from `IsValidSecurityDescriptor`, which Windows does not document as setting it; main's version compares the typed accessor against the source chain instead Verified beyond the conflicts: - ci.yml auto-merged with no conflict; enumerated its job keys -- 20, no duplicates. An earlier merge on this branch produced a duplicate job exactly this way, and it is invisible to conflict markers. - No conflict markers anywhere in the tree. - cargo check --all-targets clean. - cargo test --workspace --all-features: 3000 passed, 0 failed, 24 ignored. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the review comment on #74 that arrived after it merged, so it lands as its own PR.
The finding
a_capture_failure_exposes_its_os_error_both_waysrequired the capture failure to carry a present, non-zero Win32 code. That failure path takesGetLastErrorafterIsValidSecurityDescriptor, which Windows does not document as setting last-error -- so the assertion demanded something the platform never promised.Measured, because "not documented" and "does not happen" are different claims
A probe poisoned last-error with
0x0DEFACED, then cleared it to0, capturing a zeroed descriptor after each:So
IsValidSecurityDescriptordoes set last-error, to 1338 (ERROR_INVALID_SECURITY_DESCR), overwriting both a stale value and a cleared one. The assertion was not flaky in practice.It comes out anyway -- and the measurement is why, not a reason to keep it
An undocumented behaviour that happens to hold is exactly what this workspace's rule about depending on specified primitives says not to bind to. A test demanding it asserts 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
sourcechain report the same thing -- which holds whatever Windows reports, including nothing at all. Both sides are now compared asOption<i32>.Checked that the weaker assertion is not a weaker test
The comment above this test claims
raw_os_errorsurvived replacement byNone, and removing the.expect()could have made that claim stale. I re-injected that mutant: the test still fails, onleft: Some(1338), right: None, because the comparison is between the crate's accessor andstd's -- only one side moves under mutation.Verification
286 tests pass, clippy
--all-targets --all-featuresclean, fmt clean, encoding clean. Typedtest, so no release.