Add thread-ambient and namespace-request crates, plus executable platform probes - #46
Merged
Conversation
This branch extracts the captured-context composite into its own crate ahead of the namespace facility that motivated it, so it needs that facility's measured platform background as its justification: a pool worker inherits no impersonation token and runs with SEM_FAILCRITICALERRORS clear. That background lives on mikegrier/pseudo-async-file-ops, which is not an ancestor of this branch. Rather than restate the measurements -- the restatement-drift failure this repository has already paid for -- the supporting files are copied here byte-identical, so a later merge of the two branches resolves them automatically and only this branch's deliberate corrections require a human decision. The workspace and windows-ioring-sys checklist items come along deliberately: the imported decisions state that the work they imply is queued in CHECKLIST.md, and landing the decisions without the items would break that promise on main. Nothing here is modified. Corrections and the new crates' plan follow in later commits. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds M22 through M24 to the workspace checklist, plus a parked M24+ item for the merge reconciliation this branch's byte-identical import creates. M22/M23 extract the captured-context composite into windows-thread-ambient-sys. The trigger is the one the imported decision itself named: an independent consumer now needs to carry a caller's ambient state onto another thread with none of the namespace facility around it. The crate stays a level platform -- every aspect is offered for capture and for explicit declaration, and the facility's dialog-suppression policy is composed from those primitives rather than baked in here. M24 adds windows-namespace-request-sys as a sibling, not a layer above: a request carries no ambient context, and a context is useful to work that never opens a file. It is the catalogue-plus-faithful-execution layer, with CreateFileW first and one entry per Win32 call thereafter. Two corrections are queued rather than made silently. WOW64 filesystem redirection is reclassified from transplanted to declared, because Wow64DisableWow64FsRedirection has no getter and so the transplanted classification was never implementable. And the accepted SEM_ bit set is to be measured rather than read off the documentation, since it decides which bits this crate can offer at all. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…n crate Creates windows-thread-ambient-sys and records the decision that put it in its own crate rather than inside the namespace facility that first designed it. The earlier statement said the composite was "not extracted preemptively" and would move out once it became a genuine cross-crate contract. That reasoning is unchanged; its precondition simply arrived early and from an unanticipated direction -- an independent consumer needs to carry a caller's ambient state onto another thread with none of that facility around it. So the crate is a level platform: every aspect is offered for capture and for explicit declaration, and the facility's dialog-suppression policy is composed from those primitives rather than baked in. Also corrects the classification of WOW64 filesystem redirection from transplanted to declared. This was not a preference: Wow64DisableWow64FsRedirection yields an OldValue only as a side effect of disabling redirection and has no getter, so the calling thread's value cannot be read and an aspect that cannot be read cannot be transplanted. The session's open question on WOW64 capturability is struck in the same change rather than left standing against a decided answer. Swept "wow64|extracted preemptively|facility's crate|transplanted" across the repository: 7 sites outside this checklist, all 7 updated -- 5 in DESIGN-NOTES.md (the extraction paragraph, both affected table rows, and a new consequence bullet) and 2 in the design session, which keeps its original text as a Tier-3 record and carries supersedence markers pointing at Tier 1 instead. Completed items: M22.1, M22.3 Completed item: M22.1: Record the extraction decision and the WOW64 correction in DESIGN-NOTES.md, sweeping every statement of each rather than the one site a reader happens to notice. Completed item: M22.3: Create the crate: Cargo.toml, workspace membership, README.md, a CHANGELOG.md baseline, a row in PLANS.md, and a crate DESIGN-NOTES.md recording the shape decisions before any of them are implemented. The two landed in one commit because M22.1's authoritative statement links to M22.3's DESIGN-NOTES.md, so writing it first would have created a broken cross-reference. That is a defect in the plan's sequencing, recorded in the checklist rather than disguised by splitting the commit after the fact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Measures which SEM_ bits SetThreadErrorMode accepts, because that decides which bits the crate can offer as a declarable aspect at all. Measured on Windows 11 Enterprise 10.0.28000, aarch64-pc-windows-msvc, Snapdragon X2 Elite, 12 logical processors, rustc 1.98.0. Settable and reading back: SEM_FAILCRITICALERRORS, SEM_NOGPFAULTERRORBOX, SEM_NOOPENFILEERRORBOX. SEM_NOALIGNMENTFAULTEXCEPT is rejected with ERROR_INVALID_PARAMETER. The probe reads every value back with GetThreadErrorMode rather than trusting the return code, because the outcome that would actually hurt is not rejection but silent acceptance of a bit that is not installed -- that would let this crate report installing a value it had not installed. It does not happen: the rejection is loud. Two findings the documentation does not give. An invalid bit fails the *whole* call: setting all four at once installed nothing, not the three valid ones. So the declarable type must be unable to represent the invalid bit rather than rejecting it at runtime, or a caller combining it with valid bits silently loses the entire error-mode change. And M21.2 is narrowed rather than closed -- SEM_NOALIGNMENTFAULTEXCEPT drops out of it entirely and needs no ARM64/x64 pair, while SEM_NOGPFAULTERRORBOX is settable and remains a genuine policy question. Completed item: M22.2: Measure which SEM_ bits SetThreadErrorMode actually accepts, because it decides which bits this crate can offer as declarable. Reason it from measurement, not from the documentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
M22.2 concluded that the declarable error-mode type should be unable to represent SEM_NOALIGNMENTFAULTEXCEPT. That conclusion had a hole: capture reads the thread mode back, and the process error mode is a separate setting reachable through SetErrorMode. If the process bit showed through GetThreadErrorMode, capture could observe a value the type cannot hold -- and a type that cannot represent a state the platform can produce is a bug, not a safeguard. Measured: it does not happen. With the process mode set to 0x0005 including the alignment bit, GetThreadErrorMode still returned 0x0000, and a thread-scoped set worked normally alongside it. The thread error mode is independent storage rather than a view of the process mode, so a captured value can only contain settable bits and the exclusion is safe. The same probe confirmed the stickiness the documentation asserts and this repository had so far taken on trust: restoring the process mode to its prior 0x8001 left it reading 0x8005. The alignment bit cannot be cleared once set at process scope. Also records an incidental observation that would otherwise read as a bug in a future test: the process error mode is not zero at entry under a normal Rust binary. It was 0x8001 here. Follow-up measurement for M22.2, which remains complete; this validates its conclusion rather than reopening it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The import commit put M22-M24 into the same CHECKLIST.md as the deferred namespace-facility work it had just imported (M19-M21, M-inf), leaving one file serving two independent branches with nothing to say which items belong to which. Importing those items was right -- the imported design decisions state that the work they imply is queued in CHECKLIST.md, so landing the decisions without the items would have broken that promise on main. Co-locating them was not. M22-M24 and M24+ move to a feature-scoped CHECKLIST-thread-ambient.md at the workspace root, which is the two new crates' lowest common source-component, and which is deleted outright when the work completes. CHECKLIST.md and PLANS.md's row for it revert to the byte-identical import, so the merge conflict surface against mikegrier/pseudo-async-file-ops shrinks to the three files carrying this branch's deliberate corrections: DESIGN-NOTES.md, the design session, and two added PLANS.md lines. No pointer is added inside CHECKLIST.md, precisely to keep it identical; PLANS.md is the repository's designated tracker and carries the new row instead. Reverting CHECKLIST.md also reverted the in-place narrowing of M21.2 that the M22.2 measurement produced. That is not dropped: it becomes M24+.2, so main cannot end up carrying a request to measure something already measured. Queuing it as a discrete item rather than a sentence in a design note is the point -- prose nobody is obliged to act on is how this kind of correction gets lost. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
M24 was written around a path-based CreateFileW and assumed a request owns everything it names. Auditing three real consumers -- windows-file-watcher and windows-file-enumeration-sys in this repository, and MikeGrier/Globazog-rs at 55a0b1ae -- falsified both assumptions, so the plan is restructured rather than worked around. The round-one list is nine entries, not one. Five of them take a handle rather than a path, so handle ownership is promoted from an hTemplateFile detail to a shared foundation: a request owns a duplicate taken with DuplicateHandle at capture, and is therefore self-contained and cannot be left referencing a handle its originator has closed. Two entries the first draft did not know about are distinct Win32 calls rather than variants: OpenFileById, which has no creation disposition, and the non-Ex GetFileInformationByHandle. M24 becomes foundations (crate, owned handle duplication, security attributes, path preparation, the faithful-execution contract), M25 the handle-producing entries, and M26 the query entries. The parked milestone is renumbered M26+ so its name stops implying it follows M24. Three findings are recorded in the milestone text because each contradicts something the plan previously assumed. No audited consumer passes a security descriptor or a template file, and none creates a file -- those parts of the CreateFileW entry are kept anyway, because an entry that cannot express two of its own parameters is a narrowed CreateFileW, and that is the platform-integrity anti-pattern rather than thrift; recorded so a later reader cannot mistake the absence of a consumer for an oversight. The strongest offload evidence is not an open at all but GetFinalPathNameByHandleW, which Globazog performs on its submitting thread once per root. And Globazog uses no ambient thread state whatsoever, which is evidence that the two crates are genuinely siblings rather than a stack. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
M26.1 claimed the design problem was that the five audited info classes have two result shapes -- fixed-size out-params versus variable-length batches. That was wrong. This crate returns bytes and the unaltered outcome and does not parse, so both shapes collapse to one owned aligned buffer and per-class parsing stays with the consumer that already owns it. As a marshaling problem the entry is the easiest in the catalogue: a handle, a scalar class, and a buffer size, with no pointer into caller memory anywhere. The real difficulty was unrecorded, and it falls straight out of the ownership decision made in M24.2. The directory classes are stateful on the file object -- FileIdExtdDirectoryRestartInfo rewinds the scan and FileIdExtdDirectoryInfo continues it -- and DuplicateHandle yields a second handle to the same file object. An owned duplicate therefore gives a request self-containment but not isolation: it shares the enumeration cursor with the handle it came from, and a consumer still enumerating on the original interleaves with it. This does not overturn the decision, since the alternative leaves a request able to outlive its handle, but the contract must say that a duplicate is not an independent enumeration and that an independent traversal needs a fresh open. It is also where the unresolved ordering question binds hardest. Two non-negotiable constraints are recorded with their in-repo precedent rather than left to be rediscovered: the buffer must be 8-byte aligned, because a Vec<u8> fails the very first query with ERROR_NOACCESS, and the call reports no written length, so the completion returns the whole buffer and the consumer bounds its own reads. The alignment requirement is shared with M24.3's security descriptor, so it is built once as an aligned buffer primitive. Also records that this entry needs no ambient context at all -- access was checked at the open, which is why the enumeration crate applies impersonation only around CreateFileW. It is the clearest illustration that a request and a context are paired at submission rather than fused. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e needing no marshaling Records the membership test the entry rests on, after it was challenged directly: a catalogue entry belongs because a blocking namespace call needs performing off the caller's thread, not because it is awkward to marshal. The latter test selects for our implementation convenience rather than consumer need, and would leave the most-called namespace operation across all three audited consumers outside the catalogue -- the same call whose lack of an overlapped form is why an unassociated handle is a first-class destination at all. Also closes a real gap the challenge exposed. An entry covering the two directory classes otherwise reads as a second implementation of windows-file-enumeration-sys's shipped streaming engine. It is not: this entry is single-shot and the client sequences the next call, while that crate owns the cursor, refill loop, quanta, and backpressure over the same shape. All five audited classes stay reachable, since restricting them would narrow the entry for a no-consumer reason -- the move already refused for lpSecurityAttributes -- but the documentation must point streaming consumers at that crate rather than let them rebuild the loop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The plan asserted that DuplicateHandle shares the enumeration cursor, reasoned from "a duplicate names the same FILE_OBJECT" rather than measured. Handles are usually just refcounted references to a kernel object, but not always, and an unmeasured claim load-bearing for a design decision is what this repository has repeatedly paid for. Measured on Windows 11 Enterprise 10.0.28000, aarch64-pc-windows-msvc, against a real directory with a deliberately small buffer so the cursor questions actually arise, and with a control that can tell a continuation from a restart. The assertion holds: the source read ".", "..", "f00" and the duplicate returned "f01", "f02", "f03" -- a clean continuation -- while the control, two separate opens, restarted from the beginning. The more valuable result answers a question the plan did not ask. Interleaving a non-enumeration query does NOT disturb the cursor: FileBasicInfo, FileIdInfo, and the non-Ex GetFileInformationByHandle all leave an in-progress enumeration intact, on the same handle and on a duplicate. So the contract is narrower than drafted -- handle-taking entries are not hazardous in general, only the two directory-enumeration classes mutate shared state, and every other query is a pure read that composes freely. A third property the ownership decision silently depended on is now asserted rather than assumed: closing the duplicate does not disturb the source, so a request owning a duplicate and dropping it cannot damage the handle its caller kept. M24.2 gains the distinction a caller reasoning in value semantics will get wrong, stated as a documentation requirement on the type: a path is a value and is copied, a handle is a reference and duplicating it shares the object rather than cloning it -- so a request is self-contained with respect to lifetime and not isolated with respect to state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Several decisions in this workspace rest on measurements of undocumented Windows behaviour. Recorded only in prose, a measurement decays silently: the claim stays in the design note while the platform, or our reading of it, moves. This crate gives them a home an ordinary build keeps alive. Each probe's logic is a library function that RETURNS its observation; the binaries print it and the tests assert it. Writing the check twice -- once to print, once to assert -- would make the test a check of the copy rather than of the platform, which is the restatement failure this repository has already paid for. Three tiers, because "run all the probes" is not a safe instruction. Asserted probes are fast, deterministic, and leave nothing behind. Ignored ones are assertable but slow, heavy, or environment-dependent. Binary-only ones cannot be tests at all -- the CancelSynchronousIo measurement never returns, which IS the finding. Every tier is compiled by an ordinary build, which is the floor: a probe that no longer compiles has already rotted. The tier rule changed a probe's design rather than merely classifying it. The natural demonstration that the thread error mode is independent of the process mode sets SEM_NOALIGNMENTFAULTEXCEPT at process scope -- but that bit is sticky, so the test process would be permanently altered and every other test in the binary would inherit it. The asserted version proves the same property with a reversible bit; the stickiness is real and worth recording, so it is measured binary-only and documented as irreversible at the call site. Ten assertions cover this session's findings, including their controls: "a duplicate continued the enumeration" says nothing unless two separate opens demonstrably do not, so that control is its own test rather than a reader's inference. A fixture that cannot exhibit the behaviour panics instead of passing quietly, because a probe that measures nothing must not read as a pass. The binding was verified by sabotage -- changing the expected settable mask failed the test, so it is checking the platform rather than itself. The nine earlier measurements' probes are NOT migrated here yet. They currently exist only in a git-ignored scratch directory and a previous session's private state, which is one machine failure away from losing the evidence for most of the workspace's platform claims. That is queued as M27.4, deliberately after establishing the scheme on two cheap probes first. Completed items: M27.1, M27.2, M27.3 Completed item: M27.1: Create windows-platform-probes as an unpublished workspace member, with each probe's logic in a library function that returns its observation. Completed item: M27.2: Adopt three tiers -- asserted, ignored, binary only -- with every tier compiled by an ordinary build. Completed item: M27.3: Migrate this session's measurements into the crate as asserted tests, including the controls, and verify the binding by sabotage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Capture via GetThreadErrorMode, declaration of an explicit value, and scoped application that restores the thread's entry value on every path including unwind. The type cannot represent SEM_NOALIGNMENTFAULTEXCEPT, and that is the shape the M22.2 measurement dictated rather than a simplification. The bit is rejected per thread, and an invalid bit fails the WHOLE SetThreadErrorMode call -- so a type that could hold it would let a caller combine it with valid bits and silently lose the entire change. from_bits therefore rejects rather than masks, and reports only the offending bits; masking would report installing a value that was not installed, which is the failure this type exists to prevent. The guard keeps the raw previous value rather than the crate's own type, so restoration round-trips exactly what Windows reported whether or not this crate can name it. capture() still returns a Result: measured, the thread error mode is independent storage and a process-scope bit cannot show through, but a type unable to represent a state the platform can produce would be a bug and this is where it would first be observable. The possibility is surfaced rather than assumed away. Release is explicit and reports failure; Drop restores best-effort and discards the failure, because a destructor has no caller to report to. That is right while unwinding and wrong on the ordinary path, so the guard is #[must_use] and documents the distinction. Restore failure is not fatal here -- contrast impersonation, whose fail-fast semantics exist because returning a shared worker under an unknown identity is a process-wide security failure. Imposing the strictest aspect's semantics on every aspect is what the composite exists to avoid. The aspect is offered for both capture and declaration, deliberately. It is the one aspect in both halves of the decomposition, and offering only one would bake a consumer's policy into a platform layer: a consumer on shared threads forces the dialog-suppressing bits, while one owning a private thread is entitled to the opposite choice. Eighteen tests, including the ones that matter beyond the happy path: an unsupported bit rejected when mixed with valid ones, nesting that restores through each intermediate state in exact reverse, a mode carried to another thread and applied there without leaving it contaminated, and the platform fact the aspect exists for -- a fresh worker thread has no bits set, so the critical-error handler is enabled on it. Completed item: M22.4: Implement the thread error mode aspect: capture via GetThreadErrorMode, declaration of an explicit value, and scoped application restoring the worker's entry value on every path including unwind. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e value Adds the impersonation aspect by consuming windows-impersonation-token-sys, and the Captured<T> three-state value the crate's design calls for -- this is the first aspect that needs it. The module deliberately implements almost nothing. Capture, transport, thread-bound application, and exact restoration belong to the dependency, which is independently published and is the platform layer for this Windows concept. This adapts it to the three-state shape and to subset application, and it does not soften the dependency's semantics: restore failure remains fail-fast, inherited rather than chosen, because returning a shared worker to a pool under an unknown identity is a process-wide security failure. That is a different order of hazard from the other aspects and is the reason this crate composes per-aspect guards rather than one guard with one policy. Captured<T> is not Option<T>, and the reason is a real hazard rather than a taste for precision. "Not captured" and "captured, and the thread had none" both leave the worker on the process identity: identical outcome, different meaning, and only one of them is a decision about what the work should run as. Collapsing them makes an omission indistinguishable from a deliberate choice, and no later reader can recover which happened. The shape is uniform across aspects even where Absent is unreachable, because a per-aspect shape would oblige every consumer to remember which aspects can be absent. Absent is unreachable for this aspect specifically: the dependency's capture snapshots the process identity when the thread has no token rather than reporting absence. That is asserted rather than merely documented, so a change in the dependency's contract surfaces here. Sixteen new tests. They assert the adaptation this module performs -- subset application, the three-state shape, and that a context survives a thread boundary -- rather than re-testing the dependency's own contract, which would be a second copy of it. The ones worth naming: a fresh worker inherits no token and is left uncontaminated after use; a panicking operation still leaves the thread restored, since the unwind path only ever runs when something has already gone wrong; and applying Absent is defined behaviour rather than whatever present() happens to do. Completed item: M22.5: Implement the impersonation aspect by consuming windows-impersonation-token-sys rather than reimplementing capture, transport, or restoration, retaining the three-state shape for uniformity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Captures the calling thread's current transaction as an owned duplicate, applies it around an operation, and restores the thread's entry transaction afterwards. The documented entry points do not exist as exports, and a failing test found that rather than a reading of the documentation. ktmw32.h documents GetCurrentTransaction and SetCurrentTransaction and MSDN names Ktmw32.dll as their library; neither is exported from it. The header declares them FORCEINLINE over RtlGetCurrentTransaction and RtlSetCurrentTransaction in ntdll.dll, which is what this binds. The first implementation resolved the documented names from ktmw32 and every transaction test failed at symbol resolution; the DLL's export table settled it. Two consequences are recorded rather than left to be rediscovered. The aspect rests on an Rtl-prefixed ntdll export, which is weaker footing than the rest of this crate -- unavoidable, since no documented export exists, and the reason binding is lazy and an unresolvable symbol is a typed Unsupported failure rather than a process that will not start. And RtlSetCurrentTransaction returns BOOLEAN, a single byte, not the four-byte BOOL its documented wrapper returns; reading it as BOOL would test three bytes of whatever was in the register, which is the kind of defect that passes for years and then does not. This is the first aspect where Captured::Absent is reachable, so the distinction between the two empty states finally has teeth. NotCaptured leaves the running thread's transaction alone because the caller never asked; Absent installs "no transaction" because the caller did ask and the answer was none -- a worker that happened to carry one would otherwise silently enlist the caller's work in it. The hazard the aspect cannot remove is stated on the module: a transaction handle references a shared kernel object, so owning a duplicate fixes the lifetime problem and not the state problem. The caller may commit or roll back while the worker is still inside it. Eleven tests, exercising the Present path against a real transaction created through the same lazy loader, including that the duplicate is a distinct handle value and that it outlives the originating handle being closed. A raw HANDLE is not Send, so the cross-thread test reports predicates rather than handles -- this crate's own rule applied to its tests. Completed item: M22.6: Implement the TxF transaction aspect: capture the calling thread's current transaction, carry an owned duplicate, apply it around the callback, bind lazily rather than linking, and state the hazard the aspect cannot remove. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
WOW64 filesystem redirection, memory priority, and background processing mode. Each is unspecified by default, meaning the running thread's own value is left untouched -- which is not the same as declaring a default, since a declared default would overwrite whatever the thread had. The reason each aspect is declared rather than captured is recorded per aspect, because the reasons differ and only one is absolute. Redirection has no getter at all: Wow64DisableWow64FsRedirection yields an OldValue only as a side effect of disabling, so the current state cannot be observed without changing it, and an aspect that cannot be read cannot be transplanted. Memory priority IS readable, so its declared status is a choice -- priority is a policy about how work competes for resources rather than something a caller implicitly consents to having remoted, and MemoryPriority::current() is exposed so a consumer that decides otherwise can read and declare it. I/O priority has no documented getter and does not move independently at all. That last point shapes the type. Background mode is named BackgroundMode rather than an I/O-priority setting, because entering it lowers CPU, I/O and memory priority together. The test asserts that coupling by observing memory priority change when background mode is entered, so the documented behaviour is measured here rather than repeated from MSDN. Redirection failure is reported rather than swallowed. In a 64-bit process there is no redirector to disable and the call fails; a caller that asked for redirection to be disabled and silently did not get it would be reading a different filesystem than it believes. Its tests assert the reported failure rather than pretending to measure a disable that did not happen. Guards apply in a fixed order and release in exact reverse, so the thread passes back through each intermediate state. Two tests cover what only runs when something has already gone wrong: an aspect that fails to install releases the ones already installed, and the operation does not run at all in that case. Sixteen tests. Completes M22. Completed item: M22.7: Implement the declared aspects -- WOW64 filesystem redirection, memory priority, and I/O priority -- each unspecified by default, recording why each is declared rather than captured, per aspect rather than as one blanket statement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… doctests
M22.7 landed the aspect surface with zero doctests. That was a planning error
rather than a deferral: M23.4 had scheduled all documentation at the end of the
composite, so the aspects would have shipped a whole milestone with no compiled
examples at all. The plan is corrected rather than worked around -- M22.8 is
added for the aspect documentation and M23.4 now explicitly retains only the
composite's.
Seven in-source examples cover the crate surface and each aspect, and the README
gains three, wired with #[cfg(doctest)] #[doc = include_str!("../README.md")].
Per this repository's rule that prose containing code must compile, that is what
stops the README teaching an answer the API no longer gives: an example nothing
executes can only rot.
Verified by sabotage rather than by counting. Changing an expected value in a
README example failed the doctest run and named ReadmeDoctests as the failing
target, so the file is genuinely executed rather than merely parsed or silently
skipped.
The examples were chosen to carry the decisions rather than to demonstrate
syntax: the crate-level and README examples show the three relationships to the
caller side by side and label which is which; the Captured example shows the two
empty states staying distinguishable where Option would collapse them; the
from_bits example shows an invalid error-mode bit refused beside a valid one,
which is the whole reason the type cannot represent it; and the transaction
example shows Absent installing "no transaction" rather than leaving the running
thread alone.
61 unit tests and 10 doctests pass.
Completed item: M22.8: Give the aspect surface runnable examples, and compile
the README as doctests, verifying by sabotage that the README examples are
genuinely executed rather than merely parsed.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Names which capturable aspects a capture should collect. Declared aspects are absent from this vocabulary by construction: there is nothing to collect for them, so they are stated by the caller instead. There is deliberately no Default implementation. The default set is the named constant CaptureSet::DEFAULT, which a caller must name to get. That is not ceremony: the decision that this composite is exhaustively enumerated rests on its field list being contract surface, and an implicit default has the same property in a worse form, because growing it changes behaviour for callers who never named it and have no diff to review. CaptureSet::ALL is DERIVED from CapturableAspect::EVERY by a const fn rather than restated, so an aspect added to that list joins ALL automatically instead of leaving it quietly stale. This is the "prefer a derived fact to a restated one" rule applied where restatement would be easiest and least visible. The binding was verified by sabotage: adding a variant to the enum fails compilation in three places including the test that asserts the list is complete, so the guard is real rather than aspirational. ALL and DEFAULT are documented as opposites with respect to growth, because they are. ALL grows -- membership is its meaning, so a later version's aspect appears there without notice, and a caller needing a fixed set should name its aspects. Adding to DEFAULT is a breaking change. TxF is deliberately outside DEFAULT. It is deprecated, it costs a lazy ntdll binding a caller may never need, and -- the reason that actually decides it -- a captured transaction enlists remoted work in a transaction the caller may commit or roll back while that work is still running. That is a hazard to opt into, not one to acquire by taking a default. Nineteen tests. The ones carrying weight rather than covering syntax: that containment fails on a partial overlap rather than succeeding on any shared aspect; that iteration order follows the aspect list rather than construction order, so two equal sets cannot render differently; and that Debug names the aspects, since the whole reason the default is named is so its contents can be read. Completed item: M23.1: Implement the capture set and its named default, covering only the capturable aspects. The default set is a named constant whose growth is a breaking change, so a caller who wants stability can name aspects explicitly and a caller who takes the default can see what it contains. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
AmbientState collects every aspect into one value a caller carries to a worker, rather than making it remember which pieces it gathered. Capture is synchronous and happens on the calling thread, so a context that cannot be captured is an admission failure rather than a deferred one. A worker discovering it later has nobody to report it to who can act, and by then the caller has moved on. The error names the failing aspect, because "capture failed" is not actionable when three aspects could have caused it. Two facts are derived rather than stored, in both cases because storing them would create a second copy that could drift from the first. CaptureError::aspect is computed from the failure variant instead of being recorded beside it. AmbientState::captured_set is computed from the aspects themselves, so it cannot claim something the state does not hold -- which matters precisely because the NotCaptured/Absent distinction is what a caller reads back from it. Declared aspects are attached with with_declared rather than collected, since they are read from nothing. A test asserts that declaring an aspect does not make captured_set report it: the two sets are separate vocabularies and blurring them at composite level would undo the decomposition. Sixteen tests. The ones doing real work: capture reads the calling thread at the moment of the call rather than later, verified by installing a distinctive error mode around the capture and checking the captured value is the installed one and the thread is left as found; a whole state moves to another thread, which is the composite's entire purpose and which a single non-Send field would silently break; and an omitted aspect reports NotCaptured rather than Absent even when both look empty. 95 unit tests and 12 doctests pass. Completed item: M23.2: Implement composite capture, failing synchronously on the calling thread. A capture that cannot be performed is an admission failure, not a deferred one, and the error names which aspect failed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…o equality The question arises naturally -- a consumer wants assert_eq! rather than matches! on a captured token -- so it is answered once rather than re-litigated. All three routes are declined, and the decision schedules no work. Introspecting the captured handle is closed by D-6: it carries TOKEN_IMPERSONATE only, and every question a token can answer about itself needs TOKEN_QUERY. Object identity is implementable via Arc::ptr_eq or CompareObjectHandles without any rights, but DuplicateTokenEx mints a new token object, so two captures of one context are interchangeable yet compare unequal -- an Eq that fails there misleads in the direction a reader will not check. Capture-time metadata is feasible and the most dangerous of the three: a restricted token derived from the same logon has the same user and LUID while differing in privileges, integrity level, restricting SIDs and AppContainer, so the comparison would return true exactly when the rights differ. The rule recorded: == on a security principal is a predicate consumers use to skip work, so it must not exist unless it means what they will assume. If a real need appears, the answer is a narrowly named is_same_object_as, noted so the option is found rather than rediscovered. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…not DuplicateHandle D-6 asserts the captured token is immutable without saying what buys that, and DuplicateHandle is both the cheaper and the more obvious call. Two reasons are recorded, the first absolute. A primary token cannot be applied to a thread. The no-thread-token fallback opens the process token, which is primary; SetThreadToken refuses a primary token, and DuplicateHandle cannot change a token's type. Only DuplicateTokenEx with TokenImpersonation converts it, so the documented fallback is inexpressible with DuplicateHandle. DuplicateHandle would make a capture a live alias rather than a snapshot. Duplicating a handle shares the object's mutable state, and a token object is mutable through AdjustTokenPrivileges, AdjustTokenGroups and SetTokenInformation. This crate's handle cannot perform those, but that is not what protects the caller: the submitting thread can reopen its own token with TOKEN_ADJUST_PRIVILEGES, so a caller could capture, disable a privilege, and have the worker run without it -- silently. Copying is what makes D-3's synchronous capture and D-6's immutability true rather than aspirational. The cost is real and is why D-3 notes a consumer may capture once and reuse: DuplicateTokenEx allocates a whole token object where DuplicateHandle is a refcount. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tion AmbientState::with_applied installs every aspect outermost-first and releases in exact reverse: error mode, declared aspects, transaction, impersonation. The error mode is outermost so hard-error suppression is already in force while the rest is being installed; impersonation is innermost because its window is the narrowest and its restoration must not be delayed. Implementing it exposed a defect in the aspects it composes. Declared and transaction application both DISCARDED the operation's value when restoration failed, because Result had no room for both. That is worse than the silence the design rejected: an opened handle would be thrown away because a memory priority could not be put back. Both aspects now expose install/release guards, with the closure form kept as a convenience over them and documented as discarding the value in that case. The composite uses the guards, so a restore failure is reported through Applied::restore while the value survives. Applying a subset stays expressible, which the aspects' differing application windows require: an uncaptured or unspecified aspect is skipped entirely rather than being given a default, and a test asserts that a mode the state knows nothing about is left untouched rather than overwritten. Install failure and restore failure are deliberately different. An install failure releases everything already installed and returns without running the operation. A restore failure does not fail the call. Impersonation keeps its inherited fail-fast restore, so it never reaches a report -- RestoreReport is exhaustively enumerated and impersonation is absent from it by construction. The release sequence is written as separate statements rather than a struct literal, against clippy's suggestion. Inside field initialisers the release order would be the field order, and a later reader reordering fields for tidiness would silently reorder the releases. The crate-level and README examples now use the composite. Leaving them composing aspects by hand would have taught a path the composite supersedes, and they are compiled as doctests precisely so that cannot drift. Fourteen composition tests. 109 unit tests and 12 doctests pass. Completed item: M23.3: Implement application as a composition of per-aspect guards, applied outermost-first and released in exact reverse, with the impersonation guard innermost. Applying a subset stays expressible. Restore failure is fail-fast for impersonation and reported rather than fatal for the rest, reaching the caller instead of being dropped on the floor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The unit tests apply state on ordinary spawned threads, which checks the mechanics. This adds an acceptance test on the process-default Windows thread pool, because that is where the facts the crate was built on were measured and because a pool worker is process-shared: contamination left behind is not confined to a thread the test owns. A private pool would have been a gentler test of exactly the thing that must not go wrong. Five tests. The founding measurement is restated first -- a bare pool worker inherits neither the submitter's error mode nor a token -- because every other assertion depends on it: if a worker DID inherit the context, the crate would have no reason to exist and the positive tests would pass for the wrong reason. The negative the crate rests on is the one that took work to make honest. A suite that only ever watches capture succeed cannot distinguish "the context was transported" from "the worker happened to have it already". So the submitter holds a distinctive error mode that is deliberately NOT captured, and the worker must fail to see it while still receiving the aspect that WAS captured -- the second half is what stops the test passing vacuously when nothing arrives at all. Verified by sabotage: adding ERROR_MODE to the capture set fails it with "an aspect that was never captured arrived on the worker anyway". The thread pool is a dev-dependency only. The crate deliberately does not depend on it -- it is a platform layer for thread state, not for scheduling. Also completes the crate documentation: the README gains a table of which aspects are captured, declarable, or declared and why, and its status now reflects that the aspects and composite are both complete. 109 unit tests, 5 pool tests, and 12 doctests pass. Debug and release builds of the default workspace are clean. Completes M23. Completed item: M23.4: Prove the composite across a real thread boundary -- capture on one thread, apply on a thread-pool worker, and assert each aspect took effect there and was restored afterwards, including the negative that an uncaptured aspect does not arrive. Complete the API documentation, the README examples, and the changelog baseline. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…t implies The M24 audit notes recorded that Globazog "uses no ambient thread state at all" and drew a structural conclusion from it -- that the two crates are siblings rather than a stack. The observation is accurate about the code as it stands; the inference was wrong. A consumer that is still synchronous-on-worker-threads has not needed ambient state yet, which says nothing about whether it will, and Globazog's own notes schedule the async follow-up (NtQueryDirectoryFile plus IOCP) that puts its work on pool workers and obliges marshaling the caller's identity to reach it. Read correctly, Globazog is a prospective consumer of windows-thread-ambient-sys rather than evidence against it, and every aspect this workspace carries is plausibly live for it: impersonation for identity, the error mode because a traversal is exactly what meets a dead network path or an empty removable drive on a shared pool thread, WOW64 redirection for a 32-bit host, and priority for a background scan. The sibling claim still holds, but on its own footing -- a request needs no context and a context needs no request -- not on this evidence. The larger miss is that the audit had two purposes and only one was discharged. Establishing the operation set was done; establishing that the scenario is adequately served was not. Globazog's shape makes that concrete and more demanding than the in-repository consumers: one capture taken at submit(), shared across up to 64 concurrent workers, applied repeatedly for the length of a traversal. M23.5 is added for what that requires and nothing currently tests. AmbientState must be Sync and Arc-shareable, not merely Send -- it is Sync, verified, but only Send was ever asserted, and Send alone would let the design pass its own suite and then fail to compile in the consumer that motivated it. The granularity choice between applying once around a batch and applying per operation is the consumer's to make and costs a SetThreadToken per operation, which the crate should state rather than leave to be discovered. And an impersonation restore failure is fail-fast, which on a shared pool is a process abort rather than one failed operation -- inherited and correct, but better learned from documentation than from an incident. M26.6 is split into operation coverage and scenario coverage for the same reason, since checking only the first is how this was missed once already. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The audit's second purpose -- does the design serve the scenario, not just the call list -- applied to the shape a traversal engine actually has: capture once at submission, share it, run it on every worker for the length of the job. AmbientState is now asserted Sync, not merely Send. It already was, but only Send was asserted, and Send alone would have let the crate pass its entire suite and then fail to compile in the consumer that motivated extracting it. That is the class of gap a call-list audit cannot find and a scenario question does. Three tests. Eight threads hold one shared Arc<AmbientState> inside the applied region simultaneously, each observing the captured context and restoring itself. Thirty-two pool callbacks share one capture at volume. And two different states applied concurrently on separate workers must not blend -- without that cross-check, a per-thread implementation and a process-wide one would both pass, since each worker sees *a* captured mode and only the comparison shows it is the right one. The overlap test had a race of its own, found because it took exactly the ten seconds of its own timeout. It waited on a gauge each worker decremented on the way out, so the last worker to arrive could decrement before the others observed the peak; they then spun to the deadline while the assertion still passed on the single sample that worker recorded. Slow and weak at once. The wait now keys off a monotonic arrival count, which cannot regress, so a full count means genuine overlap. Ten seconds became zero. Also records what a consumer of that shape must know and could not previously learn from the crate. Sharing is the cheap option, since capture duplicates a kernel token object and re-capturing per unit of work re-pays for a snapshot already held. Granularity is the caller's choice and costs a SetThreadToken per application, but the narrowest window is sometimes right for a security reason rather than a performance one, as the enumeration crate's open-only impersonation shows. And impersonation's fail-fast restore aborts the process from a pool callback, which on sixty-four impersonated workers is not one failed operation -- an intended trade that should be a decision rather than a discovery. 110 unit tests, 8 pool tests, and 12 doctests pass. Completed item: M23.5: Prove the composite against a many-worker consumer's shape -- assert Sync, share one Arc across concurrent workers applying and restoring independently, and document the granularity cost and the fail-fast blast radius. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…isions Adds `windows-namespace-request-sys` as a workspace member with its design notes written before any implementation: a request excludes ambient context (D-1), performs its call faithfully but chooses no delivery model so an opened handle comes back plain and unassociated (D-2), the family grows one entry per Win32 call (D-3), a request owns duplicates of any handle it names (D-4), the round-one entry list is audited from three real consumers with its omissions deliberate and written down (D-5), and membership is decided by whether a call blocks rather than by how awkward it is to marshal (D-6). Also records two open items inherited rather than introduced: path resolution under a captured identity, and that the enumeration crate's shipped `prepare` is `pub(crate)` -- so M24.4's "bind to it" needs it published or extracted first. That correction is folded into the M24.4 item text. Completed item: M24.1: Create the crate, with a `DESIGN-NOTES.md` recording the boundary decisions before implementation: a request excludes ambient context; a request captures parameters and performs the call faithfully but does not choose a delivery model, so the handle-destination fork stays out and an opened handle comes back plain and unassociated; the family grows one entry per Win32 call; and a request owns duplicates of any handle it names. Record the audited entry list above as the round-one scope, with its provenance, so a later reader can tell a deliberate omission from an unexamined one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds `CapturedHandle`, the shared primitive behind `hTemplateFile` and the five handle-taking entries: it duplicates a caller's handle with `DuplicateHandle` at capture, owns the duplicate for its life, and closes it on drop. The duplicate carries `DUPLICATE_SAME_ACCESS` and is non-inheritable. Capture validates before duplicating, and one of those checks is load-bearing rather than defensive: `INVALID_HANDLE_VALUE` and the current-process pseudo-handle are the same value, so `DuplicateHandle` would happily accept an unchecked `CreateFileW` failure and return a process handle where a file handle was meant. Null, `INVALID_HANDLE_VALUE`, and the remaining pseudo-handles are therefore refused by name; everything else reaches Win32, where an already-closed handle fails with `ERROR_INVALID_HANDLE`. Every failure is a construction error, raised on the calling thread where the caller still holds the source handle and can act on it. Recorded as D-7. The type's documentation states the distinction a caller reasoning in value semantics gets wrong -- a path is copied, a handle is duplicated and so shares its kernel object -- and the tests assert the property the design rests on: dropping the duplicate leaves the source usable, and the duplicate outlives its source. Duplication is fallible, so the type offers `try_clone`, not `Clone`. The closed-handle test needs its handle value to stay unallocated across the close, so handle-opening tests take a shared lock and that one takes it exclusively rather than depending on timing or on a process-per-test runner. Completed item: M24.2: Implement owned handle references: duplicate at capture with `DuplicateHandle`, own the duplicate for the request's life, and close it with the request. This is the shared primitive behind both `hTemplateFile` and the five handle-taking entries, so it lands before any of them. Cover the case the audit makes unavoidable -- a source handle that is already closed, or is a pseudo-handle -- and decide whether duplication failure is a construction error (it is: capture fails on the caller's thread, where the caller can still do something about it). State plainly, in the type's own documentation, what a duplicate is and is not. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-relative blob A `SECURITY_ATTRIBUTES` is not a value: it points at a descriptor, and an absolute descriptor is a struct of raw pointers to an owner SID, a group SID, a DACL and a SACL that are quite possibly on the caller's stack. Copying the struct would carry those pointers to a worker that reads a dead stack frame, so capture converts to the self-relative form and owns the resulting contiguous blob. Validation runs at capture, on the calling thread, for the same reason handle capture does. Two traps are handled rather than left to be discovered. A self-relative descriptor requires DWORD-aligned storage, which `Box<[u8]>` (alignment 1) does not give; the directory-information classes will need 8-byte alignment for the same underlying reason, so this lands as one `AlignedBuffer` primitive rather than two local tricks. And a null `lpSecurityAttributes`, attributes with a null descriptor, and a descriptor with a DACL are three different grants, as are an absent, NULL, and empty DACL -- NULL and empty being opposites, so collapsing them would invert the grant rather than merely lose detail. `AclState` keeps all four states and covers the SACL too. Recorded as D-8. Completed item: M24.3: Capture the security attributes. A caller's descriptor may be absolute, holding raw pointers to owner SID, group SID, DACL and SACL that are quite possibly on the caller's stack, so capture normalises to self-relative and owns the resulting contiguous blob. Two traps must be handled rather than discovered: a self-relative descriptor requires DWORD alignment, which a plain boxed byte slice does not guarantee; and no descriptor, a descriptor with a NULL DACL, and a descriptor with an empty DACL are three different security outcomes the type must keep distinct. Validate on capture, so an invalid descriptor fails at the caller rather than on the worker. The alignment requirement is not peculiar to descriptors -- M26.1 needs an 8-byte-aligned buffer for the same underlying reason -- so build it once as an owned aligned buffer primitive rather than twice. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ation crate Resolves a caller's path on the calling thread when the request is built, because the process current directory is shared mutable state any thread can change between submission and execution. `PreparedPath` is the result type, and it documents the hazard preparation does NOT close: GetFullPathNameW is lexical and never expands a drive letter, and a drive letter resolves against the logon session of the token in effect, so a path prepared on a submitter and opened on a worker under a captured token from another session can still name a different device. M24.4 called for binding to the shipped precedent in windows-file-enumeration-sys rather than writing a second preparation, but that crate's `prepare` is `pub(crate)`. Neither published option was taken: that crate is released and this one is not, so making it depend here would make it unpublishable, and this branch exists to reach publication with minimal impact on what already ships; extracting a third shared crate buys a new published member before a consumer justifies it. So the module was copied verbatim and adapted only where this crate's error taxonomy differs (PathError / PathFailure carrying io::Error rather than the enumeration crate's Win32Error). This is duplicate-then-decide working as intended -- the released path stays untouched while this one is proven -- and it is not allowed to become permanent by default: path.rs carries a provenance comment naming its source and commit, D-9 records the reasoning, and the merge-or-delete decision is queued as a checklist item (M26+.3) gated on this crate's first release, not parked in a design note. Completed item: M24.4: Implement path preparation: resolve on the calling thread at construction, because the process current directory is mutable by any thread. Bind to the shipped precedent in crates/windows-file-enumeration-sys/src/path.rs rather than writing a second path preparation, deciding first how to make it reachable and treating that decision as part of this item. The result inherits M20.1: a session-relative drive letter is a documented hazard on these types, and the documentation must say so rather than imply the resolution is complete. Split-Source: crates/windows-file-enumeration-sys/src/path.rs Split-Into: crates/windows-namespace-request-sys/src/path.rs Split-Into: crates/windows-namespace-request-sys/src/path/tests.rs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ule each entry restates Establishes the contract every entry will follow: report what Windows reported, raw code unaltered, never normalised or reclassified. ERROR_FILE_NOT_FOUND means a missing directory from an open, an empty directory from a first query, and a genuine failure from a later one -- only a consumer holding that context can tell them apart, so interpreting it here destroys information no layer above can rebuild. The harder half is when the code is read. GetLastError is volatile thread state that almost any subsequent call overwrites, including cleanup nobody thinks of as a call: a CloseHandle in a Drop, a buffer release, a restoration guard unwinding. Rather than asking each entry to remember to read it first, `outcome::perform` takes the call as a closure and snapshots the code in the statement after it returns, with `perform_bool`, `perform_handle`, `perform_nonnull_handle`, and `perform_nonzero` layered on it. An entry binds to that function instead of re-implementing the rule, which is what makes the guarantee structural. The load-bearing test demonstrates the trap by reproducing it -- cleanup clobbers the code when it is read late -- and then shows the same sequence through `perform` keeps it, so the guarantee is proven rather than merely observed to pass. Recorded as D-10; D-2's restatement of the same fact now points at it rather than saying it a second time. Completed item: M24.5: Establish the faithful-execution contract that every entry then follows: an entry returns its result or the raw Win32 code unaltered, and `GetLastError` is captured before any restoration runs so nothing in between overwrites it. Preserving the code is a constraint from a real consumer rather than a stylistic choice -- `ERROR_FILE_NOT_FOUND` means a missing directory from an open, an empty directory from a first query, and a genuine failure from a later one, and only the consumer can disambiguate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e crate's docs Each foundation already proves its own independence in its own module: a duplicated handle outlives its source, a captured descriptor outlives the absolute one it was built from, a prepared path outlives its input string. What none of them shows is the composite -- one value holding a prepared path, two captured handles, and captured security attributes, outliving every input at once and still working on a thread that saw none of them. That is the property an entry will depend on, so it is asserted rather than inferred from three separate results, and Send/Sync are asserted rather than assumed. The cross-module tests reuse the handle module's fixture and the security module's absolute-descriptor builder rather than standing up second copies, so both are made pub(crate) under cfg(test). Also completes the crate's API documentation: a crate-level example showing capture-on-the-submitter, use-on-a-worker, and the same example in the README compiled as a doctest so a contract change breaks the build instead of quietly teaching the old answer. The changelog baseline already matched its siblings. Note on scope: the per-case tests this item enumerated were not deferred to it -- each landed with the item that introduced the behaviour, which is what the one-item-then-commit loop produces. The item text records that re-plan rather than implying the cases were skipped. Completed item: M24.6: Test the foundations: security descriptors that are absolute, self-relative, null, empty-DACL, and invalid; handle duplication against a live handle, a closed handle, and a pseudo-handle; and the property that binds the whole crate together -- a captured request survives the caller dropping every input it was built from, including the source handle. Complete the API documentation and the changelog baseline. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
|
Addressed the three suppressed findings from reviews 5056717914 and 5056737841. All three were correct. They created no review threads, so replying here. 1.
|
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces multiple new crates plus new CI execution behavior, and needs a human end-to-end validation pass despite only minor actionable review items.
Review details
Suppressed comments (1)
.github/workflows/ci.yml:139
- Same issue here as the previous step: multiple
cargo runcommands in a single PowerShellrun: |block can hide earlier failures (only the last command's exit code is reliably returned). Make each probe its own step so any one failure fails the job and is clearly attributed.
- Files reviewed: 101/117 changed files
- Comments generated: 2
- Review effort level: Lite
The module documentation was updated to say two entries carry their own error, but the `type Error` doc comment a few lines below still said "all of them but one". Same fact, two statements, one corrected -- the restatement drift the repository's contract-integrity rule exists to catch, and introduced by the very commit that fixed the module text. Names both entries explicitly rather than giving a count, so the next entry to grow its own error type makes this read wrong rather than merely stale. Swept the crate for count-based claims about the error type; this was the only remaining one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two defects in worker_context, both of which turn a probe failure into something worse than a failed test. observe_on_worker documented that it panics if the worker "never reports", and then waited on a plain recv. That wait is unbounded in the strict sense: the callback owns the only sender, so a pool that never runs it leaves no sender to drop and no disconnect to observe. A wedged pool therefore hung cargo test rather than failing it -- and CI runs this suite with --include-ignored, so the job would sit until its step timeout with nothing said about why. Now a bounded recv_timeout against a new REPORT_TIMEOUT (30s; a healthy pool answers in milliseconds), so the documented panic is one that can actually happen. On timeout the boxed sender is deliberately left to the callback, which may still run later; its send then finds the receiver gone and is discarded, which report already tolerates. Reclaiming it would free a box the pool may be about to dereference. Impersonation::drop ignored the result of RevertToSelf. A failed revert leaves the thread impersonating, and under --test-threads=1 libtest runs later tests inline on that same thread, so every one of them inherits the identity and a_thread_pool_worker_starts_with_no_impersonation_token asserts the opposite as its premise. The real failure would have surfaced as a cascade of unrelated ones, which is precisely what this guard exists to prevent. It now fails loudly -- except while an unwind is already in progress, where panicking from Drop would abort the process and replace a diagnosable failure with one that explains nothing; there it reports and lets the original panic carry the diagnosis. The handle is closed on every path, before the check. All 33 probe tests pass including the ignored tier, among them the existing the_impersonation_guard_reverts_even_while_unwinding, and probe-worker-context still runs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The two probe steps each listed several cargo run commands in one `run: |` block. These steps take the Windows runner's default shell, pwsh, where a failing native command neither aborts the script nor sets the step's exit code: Actions ends a pwsh step with `exit $LASTEXITCODE`, so only the last command in a block is reported. A failing probe-error-mode followed by a passing probe-worker-context exited 0 and the job went green. Measured rather than assumed. Simulating the Actions wrapper on pwsh 7.6.5 -- `$ErrorActionPreference = 'stop'`, a command exiting 3, a command exiting 0, then `exit $LASTEXITCODE` -- the script continues past the failure and the step exits 0. `$ErrorActionPreference` does not change this, because it governs cmdlet errors rather than native exit codes. Each probe is now its own step, so any one failure fails the job and the Actions UI names which probe failed -- worth having for a job whose output is per-probe magnitudes. Swept every `run: |` block in the workflows. The only other multi-command blocks are in publish-crate.yml, which declares `shell: bash` explicitly; bash runs Actions steps with `set -e`, so a failing command aborts there and those blocks are unaffected. probe-cancel-io remains deliberately absent, and the comment saying why is kept. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces multiple new crates and CI/release automation changes whose correctness depends on Windows runtime behavior and comprehensive validation beyond what can be safely verified here.
Review details
- Files reviewed: 101/117 changed files
- Comments generated: 0 new
- Review effort level: Lite
The Cargo section asserted "This workspace contains a `.config/nextest.toml`, so prefer `cargo_nextest_run` over `cargo_test`". None of that is true here: there is no `.config` directory, `.config/nextest.toml` has never been tracked in this repository's history, cargo-nextest is not installed, and all fifteen test invocations across the workflows are `cargo test`. It was generic guidance that never matched this workspace. Corrected rather than satisfied by adding the file, because the difference is not cosmetic. The runners isolate differently -- nextest gives each test its own process, `cargo test` runs tests as threads in one -- and DESIGN-NOTES already records a case written specifically for the latter: a close routine is a bare `extern "system"` pointer that cannot capture, so its observation state lives in a `static`, declared inside each test function rather than at module scope precisely because a module-scope counter would be corrupted by another test closing a handle concurrently. That note names `cargo test` as what CI runs. Adopting nextest is therefore a deliberate change to CI and to the model the tests are written against, so the section now says so instead of inviting someone to close the "gap" by creating the missing file. Also fixes a second, subtler error the same claim had propagated. Both the milestone-boundary step and the Rust instructions said doctests need a separate run because "cargo-nextest does not run doc tests" -- true of nextest, and misleading here, where the reason is different. Verified both halves rather than reasoning about them: an unfiltered `cargo test -p windows-thread-ambient-sys` runs 111 + 8 unit/integration tests *and* 13 + 3 doctests, emitting `Doc-tests windows_thread_ambient_sys`; a `test_filter` run of the same package enumerates only the lib and test binaries and emits no `Doc-tests` line at all. So an unfiltered run already covers doctests, and it is specifically a *filtered* run that needs the separate `doc: true` pass. Swept both instruction files: every remaining mention of nextest is either tool-table documentation of the MCP tools themselves (correctly annotated "requires cargo-nextest") or the corrected text. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The checklist rule said a pre-existing failure "must be recorded in `UNRESOLVED-TEST-FAILURES.md`" and gave no location, then deferred the convention to "language-specific instructions" -- which, until the Rust instructions file landed yesterday, did not exist. With no path and nothing to defer to, the natural reading is a repository-root file, and there is none: the only pair in the repository is under crates/windows-file-watcher. The rule now states the location outright. The pair is per-component and located by the same nearest-ancestor rule as DESIGN-NOTES.md, created beside that component's CHECKLIST.md when absent, with no root-level pair by design -- a failure belongs next to the component that owns it, where the person working there will see it. Both existing files are linked as the worked example, which also satisfies the repository's own rule that a reference meant to be opened is a clickable link rather than an inline-code filename. Stated once rather than twice: the Rust instructions carried a full second copy of the same convention, which is the restatement drift this repository has a rule against, so that copy is reduced to a pointer at the authority. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces multiple new crates plus new CI/probing behavior and is explicitly awaiting interpretation of probe outputs, which warrants final human review.
Review details
- Files reviewed: 100/117 changed files
- Comments generated: 0 new
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces multiple new crates plus new CI behavior and probe-based assertions about undocumented platform semantics, which warrants final human review despite only minor localized issues found.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
crates/windows-file-watcher/src/watcher/tests.rs:517
- The comment describes
recvreturningNoneonly at end-of-stream, but this test usesreceiver.recv_timeout(...), which (perqueue::Receiver::recv_timeout) returnsNoneon timeout as well as teardown. Consider rewording so the drain logic is described as: drain untilrecv_timeoutyieldsNone, then assertis_disconnected()to prove it was end-of-stream rather than a timeout.
- Files reviewed: 100/117 changed files
- Comments generated: 0 new
- Review effort level: Lite
thread_mode_independent_of_process calls SetErrorMode, which is process-scoped: for the length of that call the whole process carries a mode it did not ask for, and every other thread in it sees that. A review proposed hardening it -- set `previous | bit` so unrelated process bits are not briefly cleared, and serialize the mutation behind a lock. Both suggestions are technically right, and the second addresses a real defect rather than a hypothetical one: two overlapping calls can interleave so the second saves a value the first had already installed, losing the entry mode and leaving the probe's bit set for the life of the process. The code is nonetheless left as it stands, because hardening it would answer the wrong question. A locked, non-clobbering version looks like something safe to call, and the objection is not that the mutation is untidy -- it is that a component has no business changing process-wide state at all. Process-wide state belongs to whoever owns the process; a library that changes it unilaterally decides on behalf of code it has never heard of, and restoring it afterwards narrows the window without acquiring the right. What makes it acceptable here is that this crate is not a component. It is an experiment for discovering platform behaviour -- the only way to learn whether the thread mode is a view of the process mode is to move the process mode and look -- and `publish = false` at version 0.0.0 is the enforcement. So the answer is scope rather than hardening, documented in three places a reader might arrive at: the crate docs gain an "Experiments, not components" section stating that none of this is for production use and that a technique here must not be lifted into a crate that ships; the function's own rustdoc says it mutates process-wide state, must not be called from production code, and is knowingly not concurrency-hardened; and DESIGN-NOTES records the decision with the interleaving table, so the suggestion is not re-raised and nobody "fixes" it into something that looks reusable. No behaviour change: 33 probe tests pass including the ignored tier, and probe-error-mode still runs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
There are malformed rustdoc fenced code blocks / truncated doc text in windows-namespace-request-sys docs (outcome.rs, path.rs) that can break documentation rendering and potentially doctest extraction.
Review details
Suppressed comments (5)
Previously missed (2) — in code that hasn't changed since the last review.
crates/windows-namespace-request-sys/src/path.rs:253
- The
# Errorssection is currently truncated mid-sentence (it ends at “exceedsMAX_PATHbefore”), leaving the documented error conditions incomplete.
This issue also appears on line 278 of the same file.
crates/windows-namespace-request-sys/src/outcome.rs:125
- The doc comment sentence ends prematurely (“Use this”), which reads like a truncated thought. This looks like it was meant to explain when to use the general
performhelper versus the convention-specific helpers.
This issue also appears in the following locations of the same file:
- line 156
- line 209
crates/windows-namespace-request-sys/src/outcome.rs:156
- The Markdown code fence closing the example is malformed: the closing
is immediately followed by prose on the same line (`/// general form ...`). In rustdoc Markdown, a fenced code block closer must be on its own line; as written this can cause the rest of the doc comment to be treated as code and can break doctests/rendering.
crates/windows-namespace-request-sys/src/outcome.rs:209 - Same doc-comment formatting issue here: the closing code fence is immediately followed by prose on the same line (
```/// Returns ...). This is not a valid fenced-block terminator in Markdown and can corrupt the rendered docs and doctest extraction.
crates/windows-namespace-request-sys/src/path.rs:278 - The fenced code block terminator is malformed (
```/// ...), which can cause rustdoc to treat the rest of the comment as code (breaking rendering and potentially doctests). The closing ``` needs to be on its own line (optionally followed by a blank doc line).
- Files reviewed: 100/117 changed files
- Comments generated: 0 new
- Review effort level: Lite
The "experiment, not a component" framing existed only in the crate docs and on the one function a review had questioned, which left it looking like a caveat about that function rather than the standing scope of the whole crate. A reader opening any other probe module, or any of the eight binaries, saw nothing. Every probe module and every binary now carries the same four-line banner as the second paragraph of its own documentation -- byte-identical in all sixteen files, so it is greppable and obviously a standard notice rather than a local remark. It sits after each file's one-line summary so rustdoc listings keep their real summaries. The crate README gains the same statement. The per-function copy on thread_mode_independent_of_process is trimmed to what is actually specific to it -- that it mutates the process error mode, and that overlapping calls can interleave their save and restore -- with the general framing left to the banner now above it, rather than stated twice. The banner deliberately carries no intra-doc link. `[windows_platform_probes]` resolves in the binaries, where the library is an extern crate, but not inside the library's own modules, and `[crate]` would resolve in both while pointing at the wrong root from a binary. A self-contained banner naming DESIGN-NOTES.md in prose stays identical everywhere and cannot rot into a wrong target; verified with `cargo doc` under `-D rustdoc::broken_intra_doc_links`. No behaviour change: 33 probe tests pass including the ignored tier, and the binaries still run. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces multiple new crates and CI probe behavior tied to nuanced Win32 semantics, and warrants final human review of contracts, unsafe/FFI boundaries, and probe assertions across architectures.
Review details
- Files reviewed: 100/117 changed files
- Comments generated: 0 new
- Review effort level: Lite
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.
Adds two new crates and makes this workspace's measured platform claims executable. Draft so the CI probe job runs and we can read its x64 output before review.
windows-thread-ambient-sys(M22-M23)Captures a thread's ambient state -- impersonation, error mode, WOW64 redirection, memory/IO priority -- and applies it on another thread. A pool worker inherits none of it, which is measured rather than assumed.
Aspects split into captured (readable from the calling thread) and declared (WOW64 redirection has no getter at all), because the two do not relate to the caller the same way. The crate holds no policy: every aspect is offered for capture and explicit declaration, so a consumer on shared threads and one with a private thread can each compose the choice they need.
windows-namespace-request-sys(M24-M26)Owned, marshalable parameter sets for the nine synchronous Win32 namespace calls three real consumers actually make -- audited from this repo's watcher and enumeration crates plus
MikeGrier/Globazog-rs, not chosen by taste.CreateFileWOpenFileOpenFileByIdOpenFileByIdentifierFindFirstChangeNotificationWWatchDirectoryCloseHandle+ variantsCloseRequestGetFileInformationByHandleExQueryFileInformationGetFileInformationByHandleQueryFileInformationByHandleGetFinalPathNameByHandleWQueryFinalPathGetVolumeInformationByHandleWQueryVolumeInformationGetFullPathNameWResolveFullPathLoad-bearing decisions:
windows-thread-ambient-sysis a dev-dependency only.IoRinguse irreversibly, so that choice belongs to a layer that knows the handle's destination. Now verified rather than asserted -- see below.FindCloseChangeNotificationcloses a change notification andCloseHandleis silently wrong for it. Closing exactly once is enforced structurally, not by discipline.ERROR_FILE_NOT_FOUNDmeans three different things depending on the call, so nothing is reclassified, and the code is snapshotted before cleanup can clobber it.RequestorConsumingRequest, so a consumer can exercise its own code with a fake and no filesystem.Path preparation is duplicated from
windows-file-enumeration-syson purpose: that crate is released and this one is not. The de-duplication is scheduled asM26+.3, gated on this merge, with a provenance comment inpath.rs-- not left to be rediscovered.windows-platform-probes(M27)Several designs here rest on undocumented Windows behaviour. Recorded only in prose, a measurement decays silently. These probes make each claim re-runnable, in three tiers: asserted (real tests), ignored (correct to assert but slow or environment-dependent), and binary only (cannot be a test -- it hangs by design).
Two findings worth calling out:
ERROR_INVALID_PARAMETERand its byte count zero, because it checked where the completion arrived rather than whether the operation succeeded. The corrected reading holds, both negative controls pass, andCreateThreadpoolIoforecloses it the same way.windows-ioring-sys's replace-not-append assumption is no longer unverified. The probe calls Win32 directly, because that crate refuses a second registration because of the assumption being measured -- probing through its guard would confirm our own belief by consulting it.Why draft
The new
platform probes (x64, ...)CI job runs the ignored tier and the magnitude-printing binaries onwindows-latest. Every measurement in this workspace was taken on ARM64, so this is the first x64 run.A failure in that job is a result, not a build break to patch away. Those tests assert the shape of a measured platform fact; red means the fact differs on x64 and the design note resting on it needs revisiting.
probe-cancel-iois deliberately excluded -- it is binary-only becauseCancelSynchronousIocan block indefinitely, so running it in CI would hang the job by design.Marking ready once that output has been read and
M27.5's diffs recorded.