Harden the file watcher and remove the reopen-by-id fast path

[MikeGrier]

Split out of #56, which had grown to 248 files across nine crates and was churning under review. This crate's work is a separate deliverable that happened to travel in the same branch.

Why this separates cleanly

Three facts, each verified rather than assumed:

• windows-file-watcher's Cargo.toml is not in Add waitable queues and the placement probe, and reshape the topology model #56's diff at all -- its dependencies never changed.
• It depends on none of windows-topology-sys, windows-waitable-queues or windows-placement-probe, which is where most of that PR's churn lives. Its only path dependencies are windows-overlapped-io-sys, windows-threadpool-sys and wtf-string.
• The changes those three received in Add waitable queues and the placement probe, and reshape the topology model #56 are unrelated to this work -- 186 purely additive lines, and the lib.rs parts are the same cross-cutting change (compiling READMEs as doctests) repeated in each.

Squashed deliberately

The 22 file-watcher-only commits are not reproduced here. Eight further commits in #56 mixed this crate with others, so reconstructing per-commit history would fabricate something that never existed. The original commits remain on mikegrier/deferred-namespace-ops for archaeology.

Breaking change

The reopen-by-id fast path is removed. It was root-caused as impossible rather than merely unused -- a handle reopened by file id rejects the watcher's own read -- so the path could not have worked, and removing it takes away nothing a caller could have relied on. tests/reopen_by_id_cannot_be_watched.rs is the evidence, and it is why the removal is safe rather than merely convenient.

What is in here

• A use-after-free in the cancellation path. The watcher freed a cancelled read's buffer without waiting for the kernel to finish with it. It now waits.
• StandingHold::drop's dead release path replaced with a tripwire, after a mutation run showed nothing could reach it.
• Bounded waits throughout the queue tests, so a broken wake fails the suite instead of hanging it -- with NOTIFY_TIMEOUT measured rather than guessed.
• D-85: paths are the caller's, verbatim. Recorded as a decision, with the write-only canonical_path dropped and the query kept, plus a test guarding the pass-through against a helpfully-added prefix.
• Mutation-gap coverage across queue.rs, directory.rs, monitor.rs and the notification categories -- including which survivors are uncatchable and why, rather than leaving them as unexplained gaps.

Verification

Run on this branch, against main's dependencies rather than #56's -- which is the point of the split, so it is what was tested:

• 392 tests pass
• cargo fmt --check -- clean
• cargo clippy --all-targets --all-features -- -D warnings -- clean
• cargo doc --no-deps --all-features under the CI deny flags (-D warnings -D rustdoc::broken_intra_doc_links) -- clean
• No inline #[cfg(test)] mod tests { } blocks in the diff
• The five markdown files reference only crates that exist on main -- no dangling cross-references

CI has not run yet: ci.yml triggers on pull_request, so this PR is what starts it.

[Copilot]

🟡 Changes recommended

crates/windows-file-watcher/src/directory/tests.rs constructs “UTF-16 unit”-sized path fixtures using .len() on formatted strings (UTF-8 bytes), which can make the new boundary tests flaky or incorrect on non-ASCII paths.

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

Pull request overview

This PR hardens windows-file-watcher by removing the (now proven impossible) reopen-by-file-id fast path, tightening correctness around overlapped I/O cancellation/arming, and strengthening the test suite to fail fast (bounded waits + mutation-gap coverage) rather than hanging.

Changes:

• Removed reopen-by-id (OpenFileById) re-establishment and updated design/docs + tests to assert the OS limitation directly.
• Extracted and tested the ReadDirectoryChangesW submission classification seam to prevent buffer lifetime misreads (use-after-free class).
• Expanded test coverage and reliability: bounded waits, regression guards for multi-watch ordering, and focused unit tests for mutation survivors (queue/monitor/directory helpers + path pass-through decision D-85).

File summaries

File	Description
crates/windows-file-watcher/tests/watched_paths.rs	Lowers NOTIFY_TIMEOUT and points to measured justification.
crates/windows-file-watcher/tests/reopen_by_id_cannot_be_watched.rs	New integration test asserting ReadDirectoryChangesW rejects by-id directory handles.
crates/windows-file-watcher/tests/fault_detail.rs	Aligns NOTIFY_TIMEOUT with measured 5s budget.
crates/windows-file-watcher/tests/consumer_test_surface.rs	Bounds a blocking drain-loop assertion via thread + deadline.
crates/windows-file-watcher/src/watcher/tests.rs	Lowers NOTIFY_TIMEOUT to 5s; adds mutation-gap tests and bounded drain-loop assertions.
crates/windows-file-watcher/src/watcher.rs	Removes reopen-by-id logic; factors classify_submission; improves volume-change warning context; adds test-only seams.
crates/windows-file-watcher/src/watch/tests.rs	Introduces Drain helper to avoid discarding notifications across subscriptions; adds regression test.
crates/windows-file-watcher/src/session/tests.rs	Bounds blocking recv() disconnect behavior with thread + deadline.
crates/windows-file-watcher/src/session.rs	Documents D-85 “paths are caller’s verbatim” semantics and long-path implications.
crates/windows-file-watcher/src/queue/tests.rs	Reworks tests to avoid suite hangs (bounded waits + explicit end-of-stream assertions); adds tripwire test.
crates/windows-file-watcher/src/queue.rs	Replaces StandingHold::drop release logic with tripwire; updates reservation-transfer docs and invariants.
crates/windows-file-watcher/src/monitor/tests.rs	Adds focused tests for monitor predicates/state projection, cleanup invariants, and mutation survivors.
crates/windows-file-watcher/src/monitor.rs	Factors duplicated boolean rules into predicates used by both production and tests.
crates/windows-file-watcher/src/directory/tests.rs	Replaces reopen-by-id characterization tests with canonical-path + D-85 pass-through + helper/mutation-gap coverage.
crates/windows-file-watcher/src/directory.rs	Removes OpenFileById reopen plumbing; documents verbatim path encoding (wide_path) and canonical-path semantics.
crates/windows-file-watcher/PLANS.md	Updates “no active plan” text for milestones through M16.
crates/windows-file-watcher/DESIGN-NOTES.md	Updates D-80 and records D-85; revises reopen-by-id rationale with executable evidence.
crates/windows-file-watcher/COMPLETED-PLANS.md	Archives M16 monitor mutation closure.
crates/windows-file-watcher/CHECKLIST.md	Updates milestone status, records M15 items, and closes the horizon item for reopen-by-id.

Review details

Suppressed comments (2)

crates/windows-file-watcher/src/directory/tests.rs:714

• deep_dir_of_prefixed_len claims to size a path to an exact target in UTF-16 units, but it uses format!("\\\\?\\{}", path.display()).len() which counts UTF-8 bytes of a display string. On non-ASCII paths this mis-sizes the fixture and can break the boundary walk tests unexpectedly.

    loop {
        let current = format!(r"\\?\{}", path.display()).len();
        assert!(
            current + 2 <= target,
            "the base path is already too long to hit {target}"

crates/windows-file-watcher/src/directory/tests.rs:739

• In canonical_path_is_exact_on_both_sides_of_its_first_buffer, assert_eq!(prefixed.len(), target, ...) compares UTF-8 byte length, not the advertised UTF-16 unit length. This assertion can fail (or pass without meaning what it says) depending on non-ASCII characters in the base path; use encode_wide().count() for the check.

        let prefixed = format!(r"\\?\{}", deep.display());
        assert_eq!(prefixed.len(), target, "the fixture must be exactly sized");
        std::fs::create_dir_all(&prefixed).expect("create the sized directory");

• Files reviewed: 20/20 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.

[MikeGrier]

Fixed in bf51474, and the finding was right on both counts.

There were three sites, not two. The suppressed comments named deep_dir_of_prefixed_len and the exactness assertion; the third is the loop that grows a path until it passes 560 units, which was also counting bytes and so could stop short of the overflow it exists to cause. All three now go through one verbatim helper.

That helper also closes a second defect none of the comments named. Every site built its prefixed path with format!("{}", p.display()), and Path::display is documented to replace invalid sequences with U+FFFD -- so the string handed to create_dir_all could differ from the path that was measured. Composing OsStrings keeps the caller's bytes intact, which is also what this crate promises callers under D-85.

Measured with wtf_string::Wtf16String rather than encode_wide().count(): it is already a dependency of this crate for the lossless OsStr interop D-8 requires, and it holds the string in the encoding Windows uses, so its len is the number under test rather than a conversion of one.

Guarded by a test, because nothing else here could catch a regression. Every fixture is built under a temp directory, and on an ASCII temp path both counts agree exactly -- so reverting the helper would leave the whole suite green on this machine and on CI, and mis-size the boundary fixtures only for someone whose %TEMP% has a non-ASCII character. verbatim_counts_utf16_units_rather_than_bytes asserts the property directly, and is sabotage-verified: restoring str::len fails it with café must expose the divergence: 17 units vs 17 bytes.

393 tests pass; fmt, clippy -D warnings and rustdoc under the CI deny flags are clean.

[Copilot]

🔵 Needs a closer look

A few newly-added/modified tests have verified correctness issues that can weaken assertions or risk hangs, and they should be fixed before merge.

Review details

Suppressed comments (3)

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

crates/windows-file-watcher/src/queue/tests.rs:216

• This drain loop uses recv_timeout, which returns None for timeout as well as for teardown. That means the loop can terminate after 5s even if the receiver was never woken on the final sender drop, weakening the assertion and potentially masking a broken wake. Since this test knows exactly how many notifications should arrive, drain by count with next(..) and then assert the stream ended with assert_stream_ended(..).

This issue also appears on line 249 of the same file.
crates/windows-file-watcher/tests/reopen_by_id_cannot_be_watched.rs:171

• CancelIo(handle)'s return value is ignored. If cancellation fails (e.g. the IRP already completed, or another unexpected error), the subsequent GetOverlappedResult(..., bWait=TRUE) can block indefinitely and hang this test. Capture the return value and handle ERROR_NOT_FOUND (already complete) explicitly; treat any other failure as a hard test failure.

crates/windows-file-watcher/src/queue/tests.rs:252

• Same issue as above: recv_timeout returns None on timeout as well as disconnect, so this loop can end after 5s without proving the stream actually ended (and without proving the last-sender wake happened). Because the test knows it should receive exactly 2 * EACH notifications, drain by count using next(..) and then call assert_stream_ended(..) to make the end-of-stream condition explicit.

    let mut seen_a = Vec::new();
    let mut seen_b = Vec::new();
    while let Some(item) = receiver.recv_timeout(Duration::from_secs(5)) {
        let name = names(&item).remove(0);

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

[MikeGrier]

Review 5119017522 -- both findings fixed in b4a2407

Both were in tests this PR added, and both were right.

The recv_timeout drain loops

Correct, and sharper than the comment puts it: assert_stream_ended exists precisely to separate "the stream ended" from "nothing arrived in time", and its own documentation spells out that ambiguity -- so the rule was stated correctly in one place while two loops in the same file did not follow it.

Both now drain exactly the count they expect with next, then call assert_stream_ended. That is more precise as well as more honest: a lost notification fails at the index it went missing rather than after a 5s timeout with a count mismatch, and an extra one fails explicitly instead of being consumed silently.

One correction to the finding, from measuring rather than reasoning. The comment says the loop "can terminate after 5s even if the receiver was never woken on the final sender drop, weakening the assertion and potentially masking a broken wake". I sabotaged that wake to check -- let last = false in Drop for Sender -- and these two tests pass even after the rewrite, because by the time the loop reaches the end every item has been consumed and senders is already zero, so recv_timeout reports the end without ever blocking. No wake is required for them either way.

The wake is genuinely covered, by two dedicated tests that did fail under that sabotage: a_blocked_receiver_is_woken_by_the_last_sender_dropping and disconnection_signals_the_doorbell. So the rewrite is worth making for precision, and I verified it with a sabotage it actually detects -- leaving one notification unconsumed now fails assert_stream_ended with "a notification arrived where the stream should have ended".

I would rather say that than let the commit imply I had closed a hole that was already covered.

CancelIo's ignored return

Real, and fixed -- with one deliberate departure from the suggested remedy, which I want to be explicit about rather than quietly diverge.

The suggestion was to "treat any other failure as a hard test failure". Taken literally at the cancel site, that means returning or panicking there -- which would unwind the frame while an IRP may still be outstanding against overlapped and buffer, reintroducing the exact use-after-free this helper exists to prevent. The remedy would restore the defect it is guarding.

So the wait is now unconditional and the assertion happens after it, once nothing is outstanding. The two failure modes are not equal: waiting on an uncancelled read hangs, which the harness bounds and a log makes obvious, while freeing a buffer the kernel still owns corrupts whatever lands there next -- and this crate has already seen that as a STATUS_STACK_BUFFER_OVERRUN with no useful diagnosis. The safe failure is chosen over the fast one, and the reasoning is recorded at the site so it is not "simplified" later.

ERROR_NOT_FOUND is accepted as you suggested: the IRP had already completed, a race this helper is allowed to lose, and the wait then returns immediately.

Verification

337 lib tests plus the integration targets pass; cargo fmt --check, cargo clippy --all-targets --all-features -- -D warnings and rustdoc under the CI deny flags are clean.

[Copilot]

🔵 Needs a closer look

A few newly added/edited test fixtures and diagnostics still use lossy Path::display()/incorrect \\?\\ spelling in D-85 guard areas, and the “tripwire” in StandingHold::drop is currently compiled out in release builds.

Review details

Suppressed comments (4)

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

crates/windows-file-watcher/src/directory/tests.rs:524

• case_sensitivity_is_read_from_the_directory_rather_than_assumed passes the directory path to fsutil.exe via path.display().to_string(), which round-trips through UTF-8 (and can be lossy for non-Unicode paths). Since this file already calls out Path::display lossiness as part of the D-85 guards, it’s safer and more consistent to pass the path as an OsStr/Path directly to Command.

This issue also appears in the following locations of the same file:

• line 638
• line 679
  crates/windows-file-watcher/src/queue.rs:847

• StandingHold::drop is described as a “tripwire”, but it currently uses debug_assert!, which is compiled out in release builds. If this invariant is ever violated outside an unwind (e.g., a future discard path forgets to settle the reservation), release builds would silently proceed and could leave capacity accounting corrupted with no diagnostic. Using assert! keeps the “no extra panic during an unwind” property (because the predicate is std::thread::panicking()), while still failing fast in release when the invariant is violated.

crates/windows-file-watcher/src/directory/tests.rs:640

• a_caller_supplied_verbatim_prefix_is_forwarded_and_honoured builds its \\?\ path via format!(.., dir.path().display()), which can be lossy and undermines the test’s own D-85 guarantee that caller-supplied bytes are forwarded verbatim. Also, the expect message currently spells the prefix as \\?\\ instead of \\?\.

    let prefixed = format!(r"\\?\{}", dir.path().display());
    let handle = DirectoryHandle::open(Path::new(&prefixed))
        .expect("a caller's own `\\?\\` path must be forwarded unchanged and open");

crates/windows-file-watcher/src/directory/tests.rs:681

• The expect string in canonical_path_grows_its_buffer_when_the_path_does_not_fit spells the verbatim prefix as \\?\\; the actual prefix is \\?\ (as used elsewhere in this file and in the test setup). Keeping the spelling consistent matters because this message is part of diagnosing prefix/parsing-mode failures.

    let handle = DirectoryHandle::open(Path::new(&prefixed))
        .expect("a caller's own `\\?\\` path opens past MAX_PATH (D-85)");
    let reported = handle.canonical_path().expect("canonical path");

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

[MikeGrier]

Review 5119074568 -- all four fixed in deab234

The tripwire was compiled out of release

The sharpest of the four, and the reasoning in the comment is exactly right: assert! keeps the "no extra panic during an unwind" property because the predicate is std::thread::panicking(), so nothing is traded away by making it unconditional.

Two things fell out of fixing it that the comment did not mention. The test was gated #[cfg(debug_assertions)] -- correct while the assertion was a debug_assert!, since a test for an assertion the release build does not contain would fail there, and inverted the moment the assertion became unconditional: the gate would have left the release configuration, the one where a silent violation actually costs something, as the only one nothing checked. The gate is gone.

Verified in both directions rather than asserted:

• With assert!, cargo test --release -- trips_the_tripwire passes.
• Reverting to debug_assert! fails it under --release with test did not panic as expected, which is the release gap made visible.

Path::display() in test fixtures

Correct, and there were more sites than the comment named. It flagged the fsutil invocation; the same defect was also in mklink's arguments in monitor/tests.rs, and in the trailing-separator fixture in directory/tests.rs.

All three now pass OsStrs. It matters most for the two external commands: a formatted path could point fsutil or mklink at a different directory than the one the test then opens or watches, so the test would quietly be measuring something else rather than failing.

This is the same class the verbatim helper closed in bf51474 -- these are the sites that helper did not reach, which is a fair thing to have caught in a follow-up round.

The \\?\\ spelling

Right, and worth more than it looks. In a non-raw literal "\\?\\" renders \?\, not \\?\. Both messages are raw strings now. The spelling matters precisely because these are the messages someone reads when diagnosing a prefix or parsing-mode failure, and a wrong prefix in the diagnosis sends them looking at the wrong thing.

Verification

337 tests pass in both debug and release -- the same count in each, which is itself the check that the tripwire test is no longer gated out. cargo fmt --check, cargo clippy --all-targets --all-features -- -D warnings and rustdoc under the CI deny flags are clean.

[Copilot]

🔵 Needs a closer look

Two test-harness changes should be adjusted for correctness/consistency (watch test timeout alignment and assert_stream_ended documentation implying wake guarantees it does not enforce).

Review details

Suppressed comments (2)

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

crates/windows-file-watcher/src/watch/tests.rs:24

• src/watch/tests.rs still uses a 30s NOTIFY_TIMEOUT, but the new Drain::wait_for helper will now spend up to that bound per missing notification. That undermines the PR’s stated goal of keeping mutation-broken delivery runs inside cargo-mutants’ kill deadline (the same motivation used to lower the other NOTIFY_TIMEOUT copies to 5s). Consider aligning this timeout to 5s (and documenting that it matches the measured bound in src/watcher/tests.rs) so failures go red quickly instead of burning 30s per wait.
  crates/windows-file-watcher/src/queue/tests.rs:642
• The assert_stream_ended doc comment says pairing recv_timeout(..).is_none() with is_disconnected() makes a broken wake satisfy neither. That is not necessarily true: is_disconnected() is derived from the sender count under the mutex, while a missing wake/notify can still leave recv_timeout returning None only after its timeout; this helper doesn’t prove that a blocked recv() was woken promptly (it just distinguishes “stream still live” from “senders==0”). Please adjust the comment to match what the function actually asserts, so future changes don’t rely on an incorrect guarantee.

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

[MikeGrier]

Review 5119125249 -- both fixed in 34a7740

assert_stream_ended's doc claimed something I had already measured false

This one lands, and not gently. I proved this exact claim wrong in the previous round, sabotaging the last-sender wake and finding that every caller of this helper still passes -- then reported it in a PR comment and left the false sentence sitting in the code. You found the same thing by reading. A conclusion recorded only in a review thread is not recorded.

The comment now states what the helper actually asserts -- nothing more arrived, and the queue agrees there is nobody left to send -- and adds a # What this does not establish section naming the two tests that genuinely cover the wake, with an explicit "do not reach for this helper to cover one, that is how a gap gets filed as closed".

Re-verified against the current code rather than relying on last round's run: let last = false in Drop for Sender fails a_blocked_receiver_is_woken_by_the_last_sender_dropping and disconnection_signals_the_doorbell, and nothing else -- exactly the two the comment now names.

The same false diagnosis was in both assertion messages, which the comment did not mention: they blamed "the receiver was never woken" for conditions neither can distinguish, so a failure would have sent the reader after the wrong cause. Both now describe what was actually observed.

The 30s NOTIFY_TIMEOUT

Correct, including the reasoning about why this module is worse than a plain count of waits suggests -- Drain::wait_for spends the budget per notification it is still looking for.

Now 5s, with the measured evidence recorded beside it rather than only in the other module: 93.6s at 30s (killed by cargo-mutants' deadline and filed as timeout rather than caught) against 31.8s at 5s. The residual risk is stated in both places, and says to raise them together if it ever flakes.

Verification

337 tests pass; cargo fmt --check, cargo clippy --all-targets --all-features -- -D warnings and rustdoc under the CI deny flags are clean.

---

Both of these were the code describing itself inaccurately rather than behaving wrongly, which is the class this PR's last two rounds have been converging on. Worth noting for whoever reviews next: the same pattern -- a fact established once and restated wrongly elsewhere -- has now produced findings in three consecutive rounds here.

[Copilot]

🔵 Needs a closer look

A design note paragraph claims the StandingHold::drop tripwire is a debug_assert!, but the implementation is an assert!, so the documentation should be corrected to match shipped behavior.

Review details

Suppressed comments (1)

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

crates/windows-file-watcher/DESIGN-NOTES.md:864

• This paragraph says the tripwire is a debug_assert!(std::thread::panicking(), ...), but the implementation in src/queue.rs uses a non-debug assert! (and even documents "assert!, not debug_assert!"). The design note should match the shipped behavior.

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

[MikeGrier]

Review 5119153757 -- fixed in 9b12a48, and the sweep found a second site

Correct. DESIGN-NOTES.md -- which is this crate's authority for current behaviour -- still described the tripwire as a debug_assert! after deab234 made it an unconditional assert!.

This is the fourth round of this PR to find the same failure mode, and all four have been mine: a fact corrected in one place and left standing in another. The repository has a rule for exactly this ("blast-radius sweep before any contract correction": grep the distinguishing term across src/, tests/ and *.md, fix every hit or say why it is out of scope). I had not been running it. I ran it this time, and it found a second stale site the review did not name, in COMPLETED-CHECKLIST.md.

The two are treated differently, deliberately

DESIGN-NOTES.md is rewritten. It states what is true now, so a wrong tense there is simply wrong. It also gains the reasoning that had only existed at the call site: nothing is traded away by making the assertion unconditional, because the "do not raise a second panic during an unwind" property comes from the predicate being std::thread::panicking(), not from the assertion's flavour.

COMPLETED-CHECKLIST.md is annotated, not rewritten. It records what was done, and a debug_assert! is genuinely what that item landed -- editing the sentence would falsify the record it exists to hold. A dated note above it gives the current form and links to the design note, so the claim cannot be read as present tense while the history stays accurate.

I would rather explain that choice than have it look like an inconsistent fix.

Sweep result

debug_assert across the crate's markdown, src/ and tests/: 8 mentions, 2 corrected, 6 are the new explanations of the change. Also checked and fixed the anchor in the note I added -- the heading I first cited did not exist.

337 tests pass; fmt is clean.

[Copilot]

🔵 Needs a closer look

deep_dir_of_prefixed_len strips the \\?\ prefix via to_str(), which can double-prefix/mis-size fixtures on non-UTF-8 paths and should be made UTF-16/OsStr-based.

Review details

Suppressed comments (1)

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

crates/windows-file-watcher/src/directory/tests.rs:797

• deep_dir_of_prefixed_len strips the \\?\ prefix by converting the canonical path with to_str(). That makes the behavior depend on UTF-8 validity: on paths containing non-UTF-8 sequences (which is exactly why the surrounding code avoids display()), the prefix won’t be stripped and verbatim(&path) will double-prefix, mis-sizing the fixture and potentially making the boundary assertions flaky.

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

[MikeGrier]

Fixed in 2097ad7

Correct, and the location is the pointed part: this is in verbatim's own helper, added two commits ago to fix this exact class. I converted the measurement to UTF-16 units and left the to_str() strip feeding it, so the fixture could still be built at the wrong length -- by a different route, in the same function.

Stripping now compares encoded units, which answers the same question without ever asking whether the path is valid UTF-8. unprefixed is the inverse of verbatim, and the two are now named and documented as a pair so the next reader sees they must agree.

Verified rather than reasoned about

A round-trip test on a path containing 0xD800 -- an unpaired surrogate, legal on Windows and not valid UTF-8. Nothing else in the file could catch this: every fixture is built under a temp directory whose path is ASCII, so both implementations agree on every path the suite actually uses.

Sabotage-verified by restoring the to_str form, which fails it with the double prefix visible in the units:

left:  [92, 92, 63, 92, 92, 92, 63, 92, 67, 58, 92, 100, 105, 114, 92, 55296]
right: [92, 92, 63, 92, 67, 58, 92, 100, 105, 114, 92, 55296]

\\?\ twice, exactly as predicted.

339 tests pass; fmt, clippy -D warnings and rustdoc under the CI deny flags are clean.

---

Seven rounds on this PR, all at 20/20 files. Three of the last four findings have been in code I wrote while fixing the previous finding -- which is worth naming rather than glossing: correcting one expression of a rule and not sweeping for the others is the specific mistake, and it has now recurred often enough here to be the thing I check first rather than last.

[Copilot]

🔵 Needs a closer look

It combines a breaking behavioral removal with substantial OS-facing watcher/test refactoring, and should get a final human review pass despite the strong test/measurement support.

Review details

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