Skip to content

Fix case-sensitivity probe corrupting directory path in MergeOutputFileHelper#10014

Merged
Evangelink merged 3 commits into
mainfrom
dev/amauryleve/fix-case-sensitivity-probe
Jul 16, 2026
Merged

Fix case-sensitivity probe corrupting directory path in MergeOutputFileHelper#10014
Evangelink merged 3 commits into
mainfrom
dev/amauryleve/fix-case-sensitivity-probe

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Summary

Follow-up bug fix for the post-merge review comment on #9805: #9805 (comment)

MergeOutputFileHelper.IsDirectoryCaseSensitive(string directory) probed case sensitivity by lower-casing the entire candidate path:

string upper = Path.Combine(directory, "CASESENSITIVEPROBE" + Guid.NewGuid().ToString("N"));
using (new FileStream(upper, ...))
{
    return !File.Exists(upper.ToLowerInvariant());
}

upper.ToLowerInvariant() also lower-cases the directory portion. For a case-insensitive/case-folded child directory (e.g. an ext4 casefold dir named with UPPERCASE chars) sitting beneath a case-sensitive parent, the lower-cased parent no longer exists, so File.Exists returns false and the method wrongly reports the directory as case-sensitive (Ordinal).

That makes EnsureOutputDoesNotAliasInput use StringComparison.Ordinal, so it can miss that a.trx and A.trx are the same file, and the subsequent ReplaceFile can overwrite an input that RFC 018 requires to stay read-only (data loss).

Fix

Lower-case only the generated probe file name, preserving the real directory path, and expand the comment to explain why.

Test

Adds a regression test MergeToFileAsync_WhenOutputDiffersFromInputOnlyByCase_IsRejectedOnCaseInsensitiveDirectory in CtrfReportMergerTests.cs that goes through the public MergeToFileAsync surface: on a case-insensitive directory a case-only output (report.json input vs REPORT.json output) must be rejected as an alias; on a genuinely case-sensitive directory the two names are distinct and the merge proceeds. The directory's real case sensitivity is probed independently (correctly, lower-casing only the file name).

Validation (Windows)

  • 3 src projects build 0 warnings / 0 errors with default analyzer enforcement: Microsoft.Testing.Extensions.TrxReport, Microsoft.Testing.Extensions.CtrfReport, Microsoft.Testing.Extensions.JUnitReport.
  • Test project builds 0/0 with /p:TreatWarningsAsErrors=true.
  • Merge tests pass on net9.0 (79/79) and net462 (76/76).

…leHelper

IsDirectoryCaseSensitive lower-cased the entire probe path, corrupting the
directory portion. A case-insensitive child directory beneath a case-sensitive
parent was then probed at a non-existent lowercased parent, so File.Exists
returned false and the location was misreported as case-sensitive (Ordinal).
That let EnsureOutputDoesNotAliasInput miss that a.trx and A.trx are the same
file, risking overwrite of a read-only input. Now only the generated probe file
name is lower-cased, keeping the real directory path intact.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 34f1a49b-b1e0-4f38-b8a1-3870dd0842d2
Copilot AI review requested due to automatic review settings July 16, 2026 14:23

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

Fixes case-sensitivity detection to prevent report mergers from overwriting aliased input files.

Changes:

  • Preserves directory casing when probing filesystem sensitivity.
  • Adds a case-only output alias test, though it does not reproduce the mixed-sensitivity regression.
Show a summary per file
File Description
src/Platform/SharedExtensionHelpers/MergeOutputFileHelper.cs Lowercases only the probe filename.
test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportMergerTests.cs Adds filesystem case-sensitivity coverage.

Review details

  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Medium

@github-actions

This comment has been minimized.

@Evangelink
Evangelink enabled auto-merge (squash) July 16, 2026 14:33
… regression test

Addresses review feedback: the previous black-box test could not distinguish the
fix from the bug on ordinary filesystems (the regression only manifests on a
mixed-sensitivity topology). Extract the probe-path construction into an internal
BuildCaseFoldedProbePath seam and assert directly that only the generated file name
is case-folded while the directory path is preserved. Reverting the production fix
now fails this test on every platform.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 34f1a49b-b1e0-4f38-b8a1-3870dd0842d2
Copilot AI review requested due to automatic review settings July 16, 2026 14:51

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.

Review details

  • Files reviewed: 5/5 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

… tests

Addresses the automated test-quality grade: the end-to-end case-only alias test
used an if/else on filesystem sensitivity to pick which assertions ran. Split it
into two focused tests (one asserting rejection on a case-insensitive filesystem,
one asserting the merge is allowed on a case-sensitive filesystem), each skipping
via Assert.Inconclusive on the non-matching host so exactly one scenario drives the
assertions.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 34f1a49b-b1e0-4f38-b8a1-3870dd0842d2
Copilot AI review requested due to automatic review settings July 16, 2026 14:57

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.

Review details

  • Files reviewed: 5/5 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

@github-actions

Copy link
Copy Markdown
Contributor

🧪 Test quality grade — PR #10014

GradeTestNotes
C (70–79) new CtrfReportMergerTests.
BuildCaseFoldedProbePath_
LowerCasesOnlyFileName_
PreservingCaseSensitiveDirectory
Three solid equality assertions, but testing via BindingFlags.NonPublic reflection couples the test to the private implementation — consider exposing via internal + InternalsVisibleTo.
B (80–89) new CtrfReportMergerTests.
MergeToFileAsync_
WhenOutputAliasesInputByCaseOnly_
IsRejectedOnCaseInsensitiveFilesystem
Good end-to-end coverage with exception + file-preservation assertions; idiomatic Assert.Inconclusive skip for platform gating.
B (80–89) new CtrfReportMergerTests.
MergeToFileAsync_
WhenOutputDiffersByCaseOnly_
IsAllowedOnCaseSensitiveFilesystem
Clean complementary sibling; both-files-exist assertions verify the key postcondition.

This advisory comment was generated automatically. Grades are heuristic
and informational — they do not block merging. Re-run with
/grade-tests.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

🤖 Automated content by GitHub Copilot. Generated by the Grade Tests on PR (on open / sync) workflow. · 42.4 AIC · ⌖ 6.43 AIC · ⊞ 8.9K · [◷]( · )

@Evangelink Evangelink added the state/needs-review Awaiting review from the team. label Jul 16, 2026
@Evangelink
Evangelink merged commit 613bf2a into main Jul 16, 2026
33 checks passed
@Evangelink
Evangelink deleted the dev/amauryleve/fix-case-sensitivity-probe branch July 16, 2026 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state/needs-review Awaiting review from the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants