Skip to content

test(filesystem_dacl): broaden network_path_rejected_e2e matcher#401

Merged
MGudgin merged 1 commit into
mainfrom
user/gudge/fix-unc-test-flake
May 23, 2026
Merged

test(filesystem_dacl): broaden network_path_rejected_e2e matcher#401
MGudgin merged 1 commit into
mainfrom
user/gudge/fix-unc-test-flake

Conversation

@MGudgin

@MGudgin MGudgin commented May 22, 2026

Copy link
Copy Markdown
Member

Summary

Broadens the matcher in filesystem_dacl::tests::network_path_rejected_e2e (added in #295 / Phase 3) so the test no longer flakes on CI agents whose network environment causes fs::canonicalize("\\someserver\\share\\foo") to error with a non-NotFound Win32 code such as ERROR_BAD_NETPATH or ERROR_LOGON_FAILURE.

Why

The test asserts that grant_appcontainer_access on a UNC path errors out cleanly. It previously accepted only NetworkPathRejected | PathNotFound:

  • PathNotFound lands when fs::canonicalize returns io::ErrorKind::NotFound — the common case on dev boxes / agents where someserver does not resolve.
  • NetworkPathRejected lands when canonicalize succeeds and our ensure_local_canonical_prefix rejects the UNC namespace.
  • Win32 { reason: "canonicalize: …" } is the third reachable variant: on agents in a corporate network, DNS may resolve someserver to an unreachable host, or the Win32 layer may return ERROR_BAD_NETPATH / ERROR_LOGON_FAILURE, neither of which maps to io::ErrorKind::NotFound. This is what an MXC Phase 3.5 CI run hit while Phase 3.5: --prepare-system-drive flag for AppContainer host setup #389 was in flight (build 147648700).

The deterministic verification of the UNC classifier itself lives in ensure_local_canonical_prefix_rejects_unc_namespace and ensure_local_canonical_prefix_rejects_raw_unc (both unaffected and unchanged).

What changed

  • network_path_rejected_e2e now accepts Win32 { .. } in addition to the prior two variants — the test's intent ("we never silently succeed on a UNC path") is satisfied by any well-typed error variant.
  • The assertion form changes from assert!(matches!(...)) to a match arm that prints the actual error variant on mismatch, so future flakes are diagnostic.
  • A comment in the test enumerates the three accepted variants and the host conditions under which each lands.

No production code paths change.

Validation

cargo +1.93 fmt --all -- --check
cargo +1.93 clippy --target x86_64-pc-windows-msvc -p wxc_common --all-targets --all-features --profile release --locked -- -D warnings
cargo +1.93 test --target x86_64-pc-windows-msvc --all-features --profile release --locked -p wxc_common --lib -- network_path --test-threads=1

All clean on the CI-matched toolchain (1.93.1; once #399 lands, this will be the default for everyone). Test passes locally — it falls into the PathNotFound branch on a dev box (no DNS resolution for someserver).

Microsoft Reviewers: Open in CodeFlow

Copilot AI review requested due to automatic review settings May 22, 2026 23:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Broadens the filesystem_dacl end-to-end test matcher for UNC paths to avoid CI flakes caused by environment-dependent fs::canonicalize failures, without changing production behavior.

Changes:

  • Updated network_path_rejected_e2e to accept DaclError::Win32 { .. } in addition to NetworkPathRejected and PathNotFound.
  • Replaced matches! assertion with a match that panics with the observed variant for better diagnostics.
  • Added an explanatory comment documenting why different error variants can occur on different hosts.
Show a summary per file
File Description
src/wxc_common/src/filesystem_dacl.rs Broadens the UNC-path E2E test’s accepted error variants and improves mismatch diagnostics to reduce CI flakiness.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 1

Comment thread src/wxc_common/src/filesystem_dacl.rs
The `network_path_rejected_e2e` test (added in #295 / Phase 3) calls
`grant_appcontainer_access` on `\\someserver\share\foo` and previously
asserted the error was `NetworkPathRejected` or `PathNotFound`. On CI
agents whose DNS resolves `someserver` (or returns a non-`NotFound`
Win32 error such as `ERROR_BAD_NETPATH` or `ERROR_LOGON_FAILURE`),
`fs::canonicalize` lands in our `Win32 { reason: "canonicalize: …" }`
fallback, which the matcher did not accept — flaking the test.

Broaden the matcher to accept `Win32 { .. }` as well, and convert the
assertion into a `match` that prints the actual variant on failure so
future flakes are diagnostic. The test's intent ("we never silently
succeed on a UNC path") is preserved — the deterministic verification
of the classifier itself lives in
`ensure_local_canonical_prefix_rejects_unc_namespace` and
`ensure_local_canonical_prefix_rejects_raw_unc` above, both
unaffected.

No production behavior change. Discovered while shepherding #389
(Phase 3.5) through CI on an agent that hit this code path
differently than the dev-box agents used during Phase 3's CI run.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@MGudgin MGudgin force-pushed the user/gudge/fix-unc-test-flake branch from 6ff6eb8 to 1924883 Compare May 23, 2026 20:27
@alexsnitkovskiy alexsnitkovskiy self-assigned this May 23, 2026

@alexsnitkovskiy alexsnitkovskiy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely

@MGudgin MGudgin merged commit 2678df3 into main May 23, 2026
18 checks passed
@MGudgin MGudgin deleted the user/gudge/fix-unc-test-flake branch May 23, 2026 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants