From 0f2cd7a5507c1c27b4998d160d541f0bfdec91a4 Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Mon, 7 Sep 2026 00:19:15 +0300 Subject: [PATCH 01/16] Decide "already in the target encoding" by codec, not by label ConversionPolicy compared the detected charset's WebName against whatever the caller typed, so every accepted alias for the same code page failed the test. -Target unicode, ucs-2 or utf-16le on a tree already in UTF-16LE decoded, re-encoded, verified and reinstalled every file to produce identical bytes. Measured: bytes identical in each case, and every modification time reset. With -Backup that also leaves a .bak and an .ecmeta.json beside each unchanged file. Under -FailOnChanges the same tree exits 0 for -Target utf-16 and 2 for -Target unicode; on BOM-less UTF-16 the alias reaches the ambiguity guard and exits 5, so the spelling alone moved a clean run to a refusal. Decide now takes the resolved code pages. A zero code page means the label did not resolve and proves nothing, so it is never treated as a match. ASCII to UTF-8 is deliberately still a conversion: they are different code pages, and a test says so with the reasoning attached. Mutation: reverting to the label comparison fails 7 of the 11 new tests. Co-Authored-By: Claude Opus 5 --- .../ConversionPolicyTests.cs | 20 +- .../TargetAliasIdentityTests.cs | 171 ++++++++++++++++++ sources/EncodingChecker/ConversionPolicy.cs | 17 +- sources/EncodingChecker/ScanEngine.cs | 2 + 4 files changed, 199 insertions(+), 11 deletions(-) create mode 100644 sources/EncodingChecker.Tests/TargetAliasIdentityTests.cs diff --git a/sources/EncodingChecker.Tests/ConversionPolicyTests.cs b/sources/EncodingChecker.Tests/ConversionPolicyTests.cs index c6484a0..abddc23 100644 --- a/sources/EncodingChecker.Tests/ConversionPolicyTests.cs +++ b/sources/EncodingChecker.Tests/ConversionPolicyTests.cs @@ -140,8 +140,8 @@ public void BothSurfacesReachTheSameDecisionForTheSameFile() public void DetectedLegacyTextRequiresAnExplicitSourceChoice() { PlannedAction action = ConversionPolicy.Decide( - "windows-1252", sourceHasBom: false, - "utf-16le", targetHasBom: false, + "windows-1252", sourceCodePage: 1252, sourceHasBom: false, + "utf-16le", targetCodePage: 1200, targetHasBom: false, sourceWasSpecified: false, isUnicodeOrAscii: false, explicitSourceConflictsWithReliableDetection: false, automaticBomlessUtf16IsAmbiguous: false, @@ -158,8 +158,8 @@ public void UnicodeAndExplicitSourcesCanConvert() Assert.Equal( PlannedAction.Convert, ConversionPolicy.Decide( - "utf-8", sourceHasBom: false, - "utf-16le", targetHasBom: false, + "utf-8", sourceCodePage: 65001, sourceHasBom: false, + "utf-16le", targetCodePage: 1200, targetHasBom: false, sourceWasSpecified: false, isUnicodeOrAscii: true, explicitSourceConflictsWithReliableDetection: false, automaticBomlessUtf16IsAmbiguous: false, @@ -169,8 +169,8 @@ public void UnicodeAndExplicitSourcesCanConvert() Assert.Equal( PlannedAction.Convert, ConversionPolicy.Decide( - "windows-1252", sourceHasBom: false, - "utf-8", targetHasBom: false, + "windows-1252", sourceCodePage: 1252, sourceHasBom: false, + "utf-8", targetCodePage: 65001, targetHasBom: false, sourceWasSpecified: true, isUnicodeOrAscii: false, explicitSourceConflictsWithReliableDetection: false, automaticBomlessUtf16IsAmbiguous: false, @@ -186,8 +186,8 @@ public void AFileAlreadyInTheTargetEncodingIsNotRefusedForLegacyDetection() Assert.Equal( PlannedAction.Unchanged, ConversionPolicy.Decide( - "utf-8", sourceHasBom: false, - "utf-8", targetHasBom: false, + "utf-8", sourceCodePage: 65001, sourceHasBom: false, + "utf-8", targetCodePage: 65001, targetHasBom: false, sourceWasSpecified: false, isUnicodeOrAscii: true, explicitSourceConflictsWithReliableDetection: false, automaticBomlessUtf16IsAmbiguous: false, @@ -203,8 +203,8 @@ public void AnUnidentifiedSourceIsSkippedByBothTheGuardAndThePolicy() Assert.Equal( PlannedAction.Skip, ConversionPolicy.Decide( - ScanEngine.UnknownCharset, sourceHasBom: false, - "utf-8", targetHasBom: false, + ScanEngine.UnknownCharset, sourceCodePage: 0, sourceHasBom: false, + "utf-8", targetCodePage: 65001, targetHasBom: false, sourceWasSpecified: false, isUnicodeOrAscii: false, explicitSourceConflictsWithReliableDetection: false, automaticBomlessUtf16IsAmbiguous: false, diff --git a/sources/EncodingChecker.Tests/TargetAliasIdentityTests.cs b/sources/EncodingChecker.Tests/TargetAliasIdentityTests.cs new file mode 100644 index 0000000..2897dbc --- /dev/null +++ b/sources/EncodingChecker.Tests/TargetAliasIdentityTests.cs @@ -0,0 +1,171 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Text; +using System.Threading; + +namespace EncodingChecker.Tests; + +/// +/// A file already in the target encoding must be left alone however the target was +/// spelled. Codec identity is the code page: "utf-16", "unicode", "ucs-2" and "utf-16le" +/// all name code page 1200, and comparing the labels instead rewrote every such file to +/// identical bytes, discarding its timestamp and creating a backup on the way. +/// +public sealed class TargetAliasIdentityTests : IDisposable +{ + private readonly string _root = + Directory.CreateTempSubdirectory("ec-alias-").FullName; + + private readonly string _path; + + public TargetAliasIdentityTests() => _path = Path.Combine(_root, "file.txt"); + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // A leftover temp directory cannot make a passing test wrong. + } + } + + private ConversionReportEntry Convert(string targetLabel) + { + ScanEngine.ParseCharsetLabel(targetLabel, out string charset, out bool writeBom); + + var entries = new EntrySink(); + + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + Action = ScanAction.Convert, + TargetCharset = charset, + TargetWriteBom = writeBom, + Backup = true, + MaxParallelism = 1, + }, + entries.Add, + CancellationToken.None); + + return entries.Single(); + } + + private static byte[] Utf16LeWithBom(string text) => + [ + .. new UnicodeEncoding(bigEndian: false, byteOrderMark: true).GetPreamble(), + .. new UnicodeEncoding(bigEndian: false, byteOrderMark: false).GetBytes(text), + ]; + + [Theory] + [InlineData("utf-16-bom")] + [InlineData("unicode-bom")] + [InlineData("ucs-2-bom")] + [InlineData("utf-16le-bom")] + public void EveryAliasOfTheSourceCodecLeavesTheFileAlone(string targetLabel) + { + byte[] original = Utf16LeWithBom("hello alias world\n"); + File.WriteAllBytes(_path, original); + DateTime written = File.GetLastWriteTimeUtc(_path); + + ConversionReportEntry entry = Convert(targetLabel); + + Assert.Equal(PlannedAction.Unchanged, entry.Action); + Assert.Equal(ConversionRowResult.Unchanged, entry.Result); + Assert.Equal(original, File.ReadAllBytes(_path)); + Assert.Equal(written, File.GetLastWriteTimeUtc(_path)); + + // An unchanged file is never read or rewritten, so it earns no recovery artifacts. + Assert.False(File.Exists(_path + ".bak")); + Assert.Empty(Directory.GetFiles(_root, "*" + ConversionMetadataStore.Suffix)); + } + + [Theory] + [InlineData("us-ascii")] + [InlineData("ascii")] + [InlineData("ANSI_X3.4-1968")] + public void EveryAliasOfAsciiLeavesAnAsciiFileAlone(string targetLabel) + { + byte[] original = Encoding.ASCII.GetBytes("plain ascii content\n"); + File.WriteAllBytes(_path, original); + DateTime written = File.GetLastWriteTimeUtc(_path); + + ConversionReportEntry entry = Convert(targetLabel); + + Assert.Equal(PlannedAction.Unchanged, entry.Action); + Assert.Equal(original, File.ReadAllBytes(_path)); + Assert.Equal(written, File.GetLastWriteTimeUtc(_path)); + } + + [Fact] + public void ABomlessUtf16FileIsUnchangedRatherThanRefusedUnderAnAliasTarget() + { + // Spelling the same codec differently used to route this file past the unchanged + // test and into the BOM-less ambiguity refusal, so an alias moved the CLI from + // exit 0 to exit 5 without any file differing. + byte[] original = new UnicodeEncoding(bigEndian: false, byteOrderMark: false) + .GetBytes("a list of ordinary words\n"); + + File.WriteAllBytes(_path, original); + + ConversionReportEntry entry = Convert("utf-16le"); + + Assert.Equal("utf-16", entry.SourceEncoding); + Assert.False(entry.SourceHasBom); + Assert.Equal(PlannedAction.Unchanged, entry.Action); + Assert.Null(entry.ReasonCode); + Assert.Equal(original, File.ReadAllBytes(_path)); + } + + [Fact] + public void AsciiIsStillConvertedToUtf8BecauseTheyAreDifferentCodecs() + { + // Deliberately not folded into the unchanged test. Detection samples only the + // first 64 KiB, so accepting an "us-ascii" label as already being UTF-8 would + // pass silently over a file whose later bytes are neither. + byte[] original = Encoding.ASCII.GetBytes("plain ascii content\n"); + File.WriteAllBytes(_path, original); + + ConversionReportEntry entry = Convert("utf-8"); + + Assert.Equal(PlannedAction.Convert, entry.Action); + Assert.Equal(ConversionRowResult.Converted, entry.Result); + } + + [Fact] + public void AnUnresolvedCodePageIsNotEvidenceOfSameness() + { + // 0 is the "did not resolve" sentinel plans and journals already use. Two of + // them describe two unknowns, not one shared codec. + Assert.NotEqual( + PlannedAction.Unchanged, + ConversionPolicy.Decide( + "mystery", sourceCodePage: 0, sourceHasBom: false, + "mystery", targetCodePage: 0, targetHasBom: false, + sourceWasSpecified: false, isUnicodeOrAscii: false, + explicitSourceConflictsWithReliableDetection: false, + automaticBomlessUtf16IsAmbiguous: false, + out _, out _)); + } + + [Fact] + public void ADifferingBomPolicyStillConvertsTheSameCodec() + { + // The code page matches, so only the BOM decision separates these. It must still + // be honoured, or "utf-8" and "utf-8-bom" would become the same request. + byte[] original = Encoding.ASCII.GetBytes("plain ascii content\n"); + File.WriteAllBytes(_path, original); + + ConversionReportEntry entry = Convert("utf-8-bom"); + + Assert.Equal(PlannedAction.Convert, entry.Action); + Assert.Equal( + new UTF8Encoding(encoderShouldEmitUTF8Identifier: true).GetPreamble(), + File.ReadAllBytes(_path).Take(3)); + } +} diff --git a/sources/EncodingChecker/ConversionPolicy.cs b/sources/EncodingChecker/ConversionPolicy.cs index 763a132..51d2c71 100644 --- a/sources/EncodingChecker/ConversionPolicy.cs +++ b/sources/EncodingChecker/ConversionPolicy.cs @@ -19,8 +19,14 @@ internal static class ConversionPolicy /// Why, in words a user can act on, when the answer is not a plain conversion. /// /// The character set of the source file. + /// + /// The source codec's canonical code page, or 0 when the label did not resolve. + /// /// Whether the source file has a BOM. /// The character set to convert to. + /// + /// The target codec's canonical code page, or 0 when the label did not resolve. + /// /// Whether to write a BOM when converting to the target charset. /// Whether the source encoding was explicitly specified by the user. /// Whether the source encoding is Unicode or ASCII. @@ -31,8 +37,10 @@ internal static class ConversionPolicy /// The planned action for the file. internal static PlannedAction Decide( string sourceCharset, + int sourceCodePage, bool sourceHasBom, string targetCharset, + int targetCodePage, bool targetHasBom, bool sourceWasSpecified, bool isUnicodeOrAscii, @@ -52,7 +60,14 @@ internal static PlannedAction Decide( } // An unchanged file is not read or rewritten, so no source choice is needed. - if (string.Equals(sourceCharset, targetCharset, StringComparison.OrdinalIgnoreCase) + // + // Codec identity is the code page, not the label. "utf-16", "unicode", "ucs-2" + // and "utf-16le" all name code page 1200, so comparing the strings reported a + // file already in the target as needing conversion, then rewrote it to identical + // bytes - discarding its timestamp, and making -FailOnChanges fail forever. A + // zero code page means the label did not resolve, so it proves nothing. + if (sourceCodePage != 0 + && sourceCodePage == targetCodePage && sourceHasBom == targetHasBom) { sourceInterpretation = SourceInterpretation.NotApplicable; diff --git a/sources/EncodingChecker/ScanEngine.cs b/sources/EncodingChecker/ScanEngine.cs index ded41e7..509b415 100644 --- a/sources/EncodingChecker/ScanEngine.cs +++ b/sources/EncodingChecker/ScanEngine.cs @@ -823,8 +823,10 @@ private static void ApplyConversion( PlannedAction action = ConversionPolicy.Decide( sourceCharset, + sourceEncoding.CodePage, sourceHasBom, targetCharset, + targetEncoding.CodePage, targetWriteBom, entry.SourceEncodingWasSpecified, TextEncoding.IsUnicodeOrAscii(sourceEncoding), From 19e01af51bbce93f23a9cab275be6dc9679b3604 Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Mon, 7 Sep 2026 00:20:55 +0300 Subject: [PATCH 02/16] Say the parallelism default the code actually uses DefaultMaxParallelism was raised from min(CPU, 4) to min(CPU, 8) with the measurement recorded beside it, and both statements of it were left saying 4: the built-in help and docs/CLI.md, which are the two places someone tuning -MaxParallelism against a slow share would look. The cause was an unnamed literal. Nothing about Math.Min(ProcessorCount, 8) gave the number an identity a document could be checked against, so raising it left two prose copies behind with nothing to notice. It is now ScanEngine.MaxParallelismCap. DocumentedParallelismDefaultTests pins all three statements together. It finds the one line in each document that states the default, extracts every run of digits from it, and asserts the set equals the cap, so a stale number cannot hide beside a fresh one. Both also assert the search found exactly one line, so the check cannot pass by looking in the wrong place. Mutation, all three killed: CLI.md back to 4 fails 1; the help text back to 4 fails 1; raising the cap to 16 without touching the docs fails 2, which is what proves the tests track the code rather than a hardcoded 8. Co-Authored-By: Claude Opus 5 --- docs/CLI.md | 2 +- .../DocumentedParallelismDefaultTests.cs | 105 ++++++++++++++++++ sources/EncodingChecker/Program.cs | 2 +- sources/EncodingChecker/ScanEngine.cs | 6 +- 4 files changed, 112 insertions(+), 3 deletions(-) create mode 100644 sources/EncodingChecker.Tests/DocumentedParallelismDefaultTests.cs diff --git a/docs/CLI.md b/docs/CLI.md index 1c3f7d7..2f2b92d 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -101,7 +101,7 @@ combined with conversion options. | `-Journal ` | Write a JSON record of the conversion decision and final result for every file. Convert mode only. | | `-Quiet` | Suppress per-file CSV and normal summaries. Errors and coverage warnings still go to stderr. | | `-Verbose` | Include error details and a result breakdown. | -| `-MaxParallelism ` | Maximum simultaneous files. Default: the smaller of CPU count and 4. | +| `-MaxParallelism ` | Maximum simultaneous files. Default: the smaller of CPU count and 8. | `-Quiet` and `-Verbose` cannot be combined. diff --git a/sources/EncodingChecker.Tests/DocumentedParallelismDefaultTests.cs b/sources/EncodingChecker.Tests/DocumentedParallelismDefaultTests.cs new file mode 100644 index 0000000..16d1349 --- /dev/null +++ b/sources/EncodingChecker.Tests/DocumentedParallelismDefaultTests.cs @@ -0,0 +1,105 @@ +using System.Runtime.CompilerServices; +using System.Text.RegularExpressions; + +namespace EncodingChecker.Tests; + +/// +/// The documented default parallelism must be the one the code actually uses. +/// +/// +/// was raised from 4 to 8 with a measurement +/// recorded beside it, and both statements of it — the built-in help and +/// docs/CLI.md — were left saying 4. Nothing failed, which is the problem: someone +/// tuning -MaxParallelism against a slow share was being given the wrong baseline +/// by the two places they would look. +/// +public sealed class DocumentedParallelismDefaultTests +{ + private static string RepositoryRoot([CallerFilePath] string thisFile = "") + { + string? directory = Path.GetDirectoryName(thisFile); + + while (directory is not null && + !File.Exists(Path.Combine(directory, "docs", "CLI.md"))) + { + directory = Path.GetDirectoryName(directory); + } + + Assert.True( + directory is not null, + "docs/CLI.md was not found above the test sources; this check reads it, so it " + + "has to run from a source checkout"); + + return directory!; + } + + /// The one line in a document that states the option's default. + private static string TheLineStatingTheDefault(string text, string source) + { + string[] candidates = + [ + .. text.Split('\n') + .Select(line => line.TrimEnd('\r')) + .Where(line => + line.Contains("-MaxParallelism", StringComparison.Ordinal) && + line.Contains("efault", StringComparison.Ordinal)) + ]; + + Assert.True( + candidates.Length == 1, + $"expected exactly one line in {source} stating the -MaxParallelism default, " + + $"found {candidates.Length}; the check is not looking where it thinks it is"); + + return candidates[0]; + } + + /// Every run of digits in the line, so a stale number cannot hide beside a fresh one. + private static string[] NumbersIn(string line) => + [.. Regex.Matches(line, @"\d+").Select(m => m.Value)]; + + private static string HelpText() + { + TextWriter originalOut = Console.Out; + + try + { + using var captured = new StringWriter(); + Console.SetOut(captured); + Assert.Equal(0, Program.RunConsoleMode(["--help"])); + return captured.ToString(); + } + finally + { + Console.SetOut(originalOut); + } + } + + [Fact] + public void TheBuiltInHelpStatesTheCapTheCodeUses() + { + string line = TheLineStatingTheDefault(HelpText(), "the built-in help"); + + Assert.Equal([ScanEngine.MaxParallelismCap.ToString()], NumbersIn(line)); + } + + [Fact] + public void TheCommandLineReferenceStatesTheCapTheCodeUses() + { + string reference = File.ReadAllText( + Path.Combine(RepositoryRoot(), "docs", "CLI.md")); + + string line = TheLineStatingTheDefault(reference, "docs/CLI.md"); + + Assert.Equal([ScanEngine.MaxParallelismCap.ToString()], NumbersIn(line)); + } + + [Fact] + public void TheCapIsWhatBoundsTheDefault() + { + // Ties the documented number to the value actually handed to Parallel.ForEach, + // so documenting the cap correctly cannot drift from applying it. + Assert.Equal( + Math.Min(Environment.ProcessorCount, ScanEngine.MaxParallelismCap), + ScanEngine.DefaultMaxParallelism); + } +} diff --git a/sources/EncodingChecker/Program.cs b/sources/EncodingChecker/Program.cs index eaec018..1f61bad 100644 --- a/sources/EncodingChecker/Program.cs +++ b/sources/EncodingChecker/Program.cs @@ -198,7 +198,7 @@ A pattern with a separator matches relative paths. -Quiet Suppress per-file CSV and normal summaries. Errors and coverage warnings still go to stderr. -Verbose Include error details and a result breakdown. - -MaxParallelism Maximum simultaneous files; default is min(CPU count, 4). + -MaxParallelism Maximum simultaneous files; default is min(CPU count, 8). -FailOnChanges Return exit code 2 if files need conversion (or fail validation). Useful for CI. diff --git a/sources/EncodingChecker/ScanEngine.cs b/sources/EncodingChecker/ScanEngine.cs index 509b415..2273698 100644 --- a/sources/EncodingChecker/ScanEngine.cs +++ b/sources/EncodingChecker/ScanEngine.cs @@ -100,8 +100,12 @@ internal static class ScanEngine // above the point where CPU count would matter. Measured on 2,000 files: 4 gave // 4,207 ms and 8 gave 2,511 ms without backups, 10,516 and 7,305 with them. Past 8 // the curve flattens, and backup runs stop improving entirely. + // Named rather than inlined so the documentation stating it can be asserted against + // it. Both the help text and CLI.md still said 4 a release after this became 8. + internal const int MaxParallelismCap = 8; + internal static readonly int DefaultMaxParallelism = - Math.Min(Environment.ProcessorCount, 8); + Math.Min(Environment.ProcessorCount, MaxParallelismCap); /// Charset label used when the source encoding cannot be established. internal const string UnknownCharset = "(Unknown)"; From 3106a59c03093fadbda1fd7ef60acac389c235ac Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Mon, 7 Sep 2026 00:21:04 +0300 Subject: [PATCH 03/16] Name the bytes a decode failed on, not a position it cannot know The diagnostic reported DecoderFallbackException.Index as "offset N within the failing read chunk". That index is relative to the decoder call, not the file, and goes negative when the bad sequence began in bytes carried over from the previous call. A UTF-8 file ending in a truncated three-byte sequence produced "offset -2" - a position no file has, in a message naming a frame it did not describe. The exception already carries the offending bytes, and they mean the same thing wherever the failure happened: "invalid byte sequence 0xE4B8." An absolute file offset would need the streaming loop restructured to keep each chunk's base position in scope; the bytes identify the fault without it. DecodeFailureDiagnosticTests also records something non-obvious found while writing it: reaching the decoder with malformed UTF-8 needs -From utf-8. Automatic detection relabels those bytes as windows-1252 and the policy refuses them before any decoding happens. Mutation: restoring the index message fails 5 of 5. Co-Authored-By: Claude Opus 5 --- .../DecodeFailureDiagnosticTests.cs | 111 ++++++++++++++++++ sources/EncodingChecker/EncodingConverter.cs | 10 +- 2 files changed, 118 insertions(+), 3 deletions(-) create mode 100644 sources/EncodingChecker.Tests/DecodeFailureDiagnosticTests.cs diff --git a/sources/EncodingChecker.Tests/DecodeFailureDiagnosticTests.cs b/sources/EncodingChecker.Tests/DecodeFailureDiagnosticTests.cs new file mode 100644 index 0000000..8fb7e14 --- /dev/null +++ b/sources/EncodingChecker.Tests/DecodeFailureDiagnosticTests.cs @@ -0,0 +1,111 @@ +using System.Text; +using System.Threading; + +namespace EncodingChecker.Tests; + +/// +/// A decode failure has to describe something the reader can find. +/// +/// +/// The diagnostic reported DecoderFallbackException.Index as "offset N within the +/// failing read chunk". That index is relative to the decoder call, not the file, and is +/// negative when the bad sequence began in bytes carried over from the previous call, so a +/// truncated tail produced "offset -2" — a position no file has, in a message naming a +/// frame it did not describe. The offending bytes are reported instead. +/// +public sealed class DecodeFailureDiagnosticTests : IDisposable +{ + private readonly string _root = + Directory.CreateTempSubdirectory("ec-decode-").FullName; + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup. + } + } + + /// + /// Converts one file, naming the source codec so the decoder is what fails. + /// + /// + /// Malformed UTF-8 is valid windows-1252, so automatic detection relabels these files + /// as legacy text and the policy refuses them before any decoding happens. Naming the + /// source is how a caller reaches the decoder with bytes it cannot accept. + /// + private ConversionReportEntry Convert(byte[] contents, string? from = null) + { + File.WriteAllBytes(Path.Combine(_root, "f.txt"), contents); + + var entries = new EntrySink(); + + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + Action = ScanAction.Convert, + TargetCharset = "utf-16", + SourceCharset = from, + MaxParallelism = 1, + }, + entries.Add, + CancellationToken.None); + + return entries.Single(); + } + + private static byte[] Cjk(int repeats) => + new UTF8Encoding(false).GetBytes( + string.Concat(Enumerable.Repeat("中文内容测试", repeats))); + + [Fact] + public void ATruncatedTrailingSequenceNamesTheBytes() + { + // Begins in one decoder call and fails in the flush: the case that produced the + // negative index, and the one automatic detection still calls UTF-8. + ConversionReportEntry entry = Convert([.. Cjk(3), 0xE4, 0xB8]); + + Assert.Equal(ConversionRowResult.Error, entry.Result); + Assert.Equal(nameof(ConversionErrorCode.SourceDecodeError), entry.ReasonCode); + Assert.Contains("0xE4B8", entry.Diagnostic); + } + + [Theory] + [InlineData(new byte[] { 0xC0, 0xAF }, "0xC0")] // overlong "/" + [InlineData(new byte[] { 0x80 }, "0x80")] // lone continuation byte + [InlineData(new byte[] { 0xED, 0xA0, 0x80 }, "0xED")] // CESU-8 surrogate + public void AnInvalidSequenceMidFileNamesTheBytes(byte[] bad, string expected) + { + ConversionReportEntry entry = + Convert([.. Cjk(3), .. bad, .. Cjk(3)], from: "utf-8"); + + Assert.Equal(ConversionRowResult.Error, entry.Result); + Assert.Equal(nameof(ConversionErrorCode.SourceDecodeError), entry.ReasonCode); + Assert.Contains(expected, entry.Diagnostic); + } + + [Fact] + public void NoDecodeDiagnosticDescribesAPositionItCannotKnow() + { + // The wording is the regression: an index relative to a decoder call was being + // presented as an offset within a frame the reader cannot locate. + foreach ((byte[] contents, string? from) in new (byte[], string?)[] + { + ([.. Cjk(3), 0xE4, 0xB8], null), + ([.. Cjk(3), 0xC0, 0xAF, .. Cjk(3)], "utf-8"), + ([.. Cjk(3), 0x80, .. Cjk(3)], "utf-8"), + }) + { + string diagnostic = Convert(contents, from).Diagnostic ?? string.Empty; + + Assert.Contains("invalid byte sequence", diagnostic); + Assert.DoesNotContain("offset", diagnostic, StringComparison.OrdinalIgnoreCase); + Assert.DoesNotContain("chunk", diagnostic, StringComparison.OrdinalIgnoreCase); + } + } +} diff --git a/sources/EncodingChecker/EncodingConverter.cs b/sources/EncodingChecker/EncodingConverter.cs index c22de0f..3bf85c6 100644 --- a/sources/EncodingChecker/EncodingConverter.cs +++ b/sources/EncodingChecker/EncodingConverter.cs @@ -961,9 +961,13 @@ private static ConversionResult Failure( private static string DescribeDecoderFailure(DecoderFallbackException ex) { - // The index is relative to the failing read chunk. - return - $"invalid byte sequence (offset {ex.Index} within the failing read chunk)."; + // Index is relative to the decoder call that failed, not to the file, and goes + // negative when the sequence began in bytes carried over from the previous call - + // so reporting it as a position produced "offset -2", a place no file has. The + // bytes themselves identify the fault and mean the same thing wherever it happened. + return ex.BytesUnknown is { Length: > 0 } bytes + ? $"invalid byte sequence 0x{System.Convert.ToHexString(bytes)}." + : "invalid byte sequence."; } private static string DescribeEncoderFailure(EncoderFallbackException ex) From 96b99a5af9539995d99848e8babdc2b2f776b8a7 Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Mon, 7 Sep 2026 00:21:13 +0300 Subject: [PATCH 04/16] Let one file's failure be one row, whatever it threw The per-item catch in RunParallel named four exception types. Anything else - a SecurityException from an ACL the enumerator did not surface, a regex timeout, a defect in EC itself - escaped Parallel.ForEach as an AggregateException and took every file the run had not reached with it. The CLI's outer catch names the same four, so it would have surfaced as a crash rather than exit 3. The filter is now "not OperationCanceledException and not OutOfMemoryException". Cancellation is the user asking to stop; carrying on after the other would be pretending to process rather than processing. RunParallel became internal so the isolation could be asserted at all. No file can be made to throw the exceptions that mattered, which is precisely what made them dangerous, so the only way to prove one failure stays one row is to hand it a processItem that throws. Five exception types now assert the failing file becomes a row while the other two complete untouched, and two assert that stopping the run is still allowed out. Mutation: restoring the four-type filter fails 5 of 8. Co-Authored-By: Claude Opus 5 --- .../ScanFailureIsolationTests.cs | 100 ++++++++++++++++++ sources/EncodingChecker/ScanEngine.cs | 20 +++- 2 files changed, 115 insertions(+), 5 deletions(-) create mode 100644 sources/EncodingChecker.Tests/ScanFailureIsolationTests.cs diff --git a/sources/EncodingChecker.Tests/ScanFailureIsolationTests.cs b/sources/EncodingChecker.Tests/ScanFailureIsolationTests.cs new file mode 100644 index 0000000..8f22628 --- /dev/null +++ b/sources/EncodingChecker.Tests/ScanFailureIsolationTests.cs @@ -0,0 +1,100 @@ +using System.Threading; + +namespace EncodingChecker.Tests; + +/// +/// One file's failure must be one row, not the end of the run. +/// +/// +/// The per-item catch named four exception types. Anything outside that list — a +/// SecurityException from an ACL the enumerator did not surface, a regex timeout, a +/// defect in EC itself — escaped Parallel.ForEach as an AggregateException +/// and took every file the run had not reached with it. The CLI's outer catch names the +/// same four, so it would have surfaced as a crash rather than exit 3. +/// +/// Cancellation and are deliberately still allowed out: +/// one is the user asking to stop, and continuing after the other would be pretending to +/// process rather than processing. +/// +/// +public sealed class ScanFailureIsolationTests +{ + private static ConversionReportEntry Entry(string name) => new() + { + FilePath = @"C:\scan\" + name, + SourceEncoding = "utf-8", + TargetEncoding = "utf-8", + }; + + private static EntrySink Run(Func processItem) + { + var seen = new EntrySink(); + + ScanEngine.RunParallel( + [Entry("a.txt"), Entry("b.txt"), Entry("c.txt")], + maxParallelism: 1, + getPath: entry => entry.FilePath, + processItem: processItem, + onEntry: seen.Add, + CancellationToken.None); + + return seen; + } + + [Theory] + [InlineData(typeof(InvalidOperationException))] + [InlineData(typeof(NullReferenceException))] + [InlineData(typeof(System.Security.SecurityException))] + [InlineData(typeof(System.Text.RegularExpressions.RegexMatchTimeoutException))] + [InlineData(typeof(FormatException))] + public void AnUnlistedFailureBecomesOneRowAndTheRunContinues(Type thrown) + { + EntrySink seen = Run(entry => + entry.FilePath.EndsWith("b.txt", StringComparison.Ordinal) + ? throw (Exception)Activator.CreateInstance(thrown, "boom")! + : entry); + + Assert.Equal(3, seen.Count); + + ConversionReportEntry failed = seen.Single( + e => e.FilePath.EndsWith("b.txt", StringComparison.Ordinal)); + + Assert.Equal(ConversionRowResult.Error, failed.Result); + Assert.Equal(ConversionReasonCodes.ScanFailed, failed.ReasonCode); + Assert.Equal(PlannedAction.Refuse, failed.Action); + Assert.False(failed.ReplacementCommitted); + Assert.Contains("boom", failed.Diagnostic); + + // The other two are untouched, which is the point of isolating the failure. + Assert.All( + seen.Where(e => !e.FilePath.EndsWith("b.txt", StringComparison.Ordinal)), + e => Assert.Null(e.ReasonCode)); + } + + [Theory] + [InlineData(typeof(OperationCanceledException))] + [InlineData(typeof(OutOfMemoryException))] + public void StoppingTheRunIsStillAllowedOut(Type thrown) + { + var seen = new EntrySink(); + + Assert.ThrowsAny(() => + ScanEngine.RunParallel( + [Entry("a.txt")], + maxParallelism: 1, + getPath: entry => entry.FilePath, + processItem: _ => throw (Exception)Activator.CreateInstance(thrown)!, + onEntry: seen.Add, + CancellationToken.None)); + + // Turned into a row, it would read as one file failing while the run carried on. + Assert.Equal(0, seen.Count); + } + + [Fact] + public void AnEntryThatProcessesToNullIsSimplyNotReported() + { + // RefreshSourceSnapshots relies on this, so widening the catch must not change it. + Assert.Equal(0, Run(_ => null).Count); + } +} diff --git a/sources/EncodingChecker/ScanEngine.cs b/sources/EncodingChecker/ScanEngine.cs index 2273698..692bad1 100644 --- a/sources/EncodingChecker/ScanEngine.cs +++ b/sources/EncodingChecker/ScanEngine.cs @@ -1222,7 +1222,13 @@ private static void CreateBackup(string path) /// Processes items with bounded parallelism and isolates per-file errors. /// Cancellation propagates normally. /// - private static void RunParallel( + /// + /// Internal rather than private so the isolation itself can be tested. No file can be + /// made to throw the exceptions this has to survive - that is what makes them the + /// dangerous ones - so the only way to prove one file's failure stays one row is to + /// hand it a that throws. + /// + internal static void RunParallel( IEnumerable items, int maxParallelism, Func getPath, @@ -1250,11 +1256,15 @@ private static void RunParallel( { entry = processItem(item); } + // One file's failure is one row. This used to name four exception types, + // so anything else - a SecurityException the enumerator did not surface, a + // regex timeout, a defect in this code - escaped Parallel.ForEach as an + // AggregateException and took every file the run had not reached yet with + // it. Cancellation still propagates, and OutOfMemoryException is left alone + // because carrying on after it would be pretending to process, not + // processing. catch (Exception ex) when ( - ex is IOException or - UnauthorizedAccessException or - ArgumentException or - NotSupportedException) + ex is not OperationCanceledException and not OutOfMemoryException) { if (item is ConversionReportEntry existing) { From 26a6e1c84b521367f373d630721be912099ae72e Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Mon, 7 Sep 2026 00:21:22 +0300 Subject: [PATCH 05/16] Encode standard output for whoever is going to read it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After attaching to the parent console, both writers were rebuilt with StreamWriter's default encoding, which is UTF-8 whatever the console is. On the machine this was found on, Console.OutputEncoding is ibm437: the per-file CSV rendered "Grüße aus München" as "Gr├╝├ƒe aus M├╝nchen". A tool whose subject is that failure was producing it in its own output. A redirected stream is a file or a pipe, so it stays UTF-8, matching the -Report file apart from its BOM. A console gets Console.OutputEncoding instead, so characters it cannot represent become "?" - visibly lossy rather than quietly wrong. No global console state is mutated, so the parent shell keeps the code page it had. Tests pin both branches, that neither writer emits a preamble, and that ASCII is byte-identical either way. Mutation, both killed: always UTF-8 fails 1; always the console encoding fails 1. Co-Authored-By: Claude Opus 5 --- .../ConsoleOutputEncodingTests.cs | 62 +++++++++++++++++++ sources/EncodingChecker/Program.cs | 31 +++++++--- 2 files changed, 83 insertions(+), 10 deletions(-) create mode 100644 sources/EncodingChecker.Tests/ConsoleOutputEncodingTests.cs diff --git a/sources/EncodingChecker.Tests/ConsoleOutputEncodingTests.cs b/sources/EncodingChecker.Tests/ConsoleOutputEncodingTests.cs new file mode 100644 index 0000000..2c9f9b1 --- /dev/null +++ b/sources/EncodingChecker.Tests/ConsoleOutputEncodingTests.cs @@ -0,0 +1,62 @@ +using System.Text; + +namespace EncodingChecker.Tests; + +/// +/// Standard output must be encoded for whoever is going to read it. +/// +/// +/// After attaching to the parent console, both writers were rebuilt with +/// StreamWriter's default encoding, which is UTF-8 whatever the console is. On a +/// CP437 console — the default on the machine this was found on — the per-file CSV rendered +/// "Grüße aus München" as "Gr├╝├ƒe aus M├╝nchen": the tool's own output exhibiting the +/// defect it exists to detect. +/// +public sealed class ConsoleOutputEncodingTests +{ + private static byte[] Write(bool redirected, string text) + { + using var buffer = new MemoryStream(); + + using (StreamWriter writer = Program.OpenStandardWriter(buffer, redirected)) + writer.Write(text); + + return buffer.ToArray(); + } + + [Fact] + public void RedirectedOutputIsUtf8WithoutABom() + { + // A file or a pipe wants the encoding the -Report file uses, minus its BOM. + byte[] written = Write(redirected: true, "Gr\u00fc\u00dfe"); + + Assert.Equal(new UTF8Encoding(false).GetBytes("Gr\u00fc\u00dfe"), written); + } + + [Fact] + public void ConsoleOutputUsesTheConsoleEncoding() + { + // Whatever the console decodes with is what it must be handed. + byte[] written = Write(redirected: false, "Gr\u00fc\u00dfe"); + + Assert.Equal(Console.OutputEncoding.GetBytes("Gr\u00fc\u00dfe"), written); + } + + [Fact] + public void NeitherWriterEmitsAPreamble() + { + // A BOM in the middle of a shell session, or at the head of a piped CSV, would be + // a new defect rather than a fix. + Assert.Empty(Write(redirected: true, string.Empty)); + Assert.Empty(Write(redirected: false, string.Empty)); + } + + [Fact] + public void AsciiIsIdenticalEitherWay() + { + // The common case must not depend on which branch ran. + Assert.Equal( + Write(redirected: true, "File,Encoding,BOM"), + Write(redirected: false, "File,Encoding,BOM")); + } +} diff --git a/sources/EncodingChecker/Program.cs b/sources/EncodingChecker/Program.cs index 1f61bad..c2ecb55 100644 --- a/sources/EncodingChecker/Program.cs +++ b/sources/EncodingChecker/Program.cs @@ -67,21 +67,32 @@ private static bool TryAttachToParentConsole() if (!AttachConsole(AttachParentProcess)) return false; - var stdout = new StreamWriter(Console.OpenStandardOutput()) - { - AutoFlush = true - }; - Console.SetOut(stdout); + Console.SetOut( + OpenStandardWriter(Console.OpenStandardOutput(), Console.IsOutputRedirected)); - var stderr = new StreamWriter(Console.OpenStandardError()) - { - AutoFlush = true - }; - Console.SetError(stderr); + Console.SetError( + OpenStandardWriter(Console.OpenStandardError(), Console.IsErrorRedirected)); return true; } + /// + /// A writer encoded for whatever is going to read it. + /// + /// + /// A redirected stream is a file or a pipe, so UTF-8 is the useful answer and matches + /// the -Report file apart from its BOM. A console decodes with its own code page + /// instead, and these writers were UTF-8 either way: handing CP437 the UTF-8 for + /// "Grüße" displays "Gr├╝├ƒe", which is precisely the failure this program + /// exists to find. Characters the console cannot represent become "?", which is visibly + /// lossy rather than quietly wrong. + /// + internal static StreamWriter OpenStandardWriter(Stream stream, bool redirected) => + new(stream, redirected ? new UTF8Encoding(false) : Console.OutputEncoding) + { + AutoFlush = true, + }; + #endregion #region Console mode From 8dc375864050af61d98ffe43ace8764c2b11a3f7 Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Mon, 7 Sep 2026 00:21:31 +0300 Subject: [PATCH 06/16] Count the folders skipped by name, as the attribute ones already are Twelve directory names are skipped deliberately, and that is documented. Attribute-excluded folders were counted; these were not. A scan of a tree whose only content sat under build/ reported one file, zero exclusions and no warning. In a tool that reports coverage precisely so a clean result cannot stand in for complete coverage, this was the one hole the coverage report could not see. docs/CLI.md promised "EC reports how many files each exclusion skipped" and this exclusion reported nothing. DirectoriesExcludedByName is separate from the attribute counter, because folding them together would make that message's "(hidden, system, or reparse point)" untrue. What is scanned is deliberately unchanged. Letting an explicit include reach into these folders was considered and not done: CLI.md and the built-in help both state they are skipped, so that is a product decision rather than a fix. The documentation now also distinguishes the two kinds of count, since for a skipped folder EC reports the folder and not its contents - it does not walk it to find out. Mutation: removing the increment fails 14 of 15. Co-Authored-By: Claude Opus 5 --- docs/CLI.md | 17 ++- .../NameExcludedFolderCoverageTests.cs | 137 ++++++++++++++++++ sources/EncodingChecker/DirectoryTraversal.cs | 23 ++- sources/EncodingChecker/MainForm.Execution.cs | 8 +- .../EncodingChecker/Program.CliExecution.cs | 7 + 5 files changed, 182 insertions(+), 10 deletions(-) create mode 100644 sources/EncodingChecker.Tests/NameExcludedFolderCoverageTests.cs diff --git a/docs/CLI.md b/docs/CLI.md index 2f2b92d..1a920f0 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -40,13 +40,16 @@ conversion of the same folder will rewrite them like anything else. Keep exporte plans, journals, and reports outside the folder you scan. Common metadata and build folders such as `.git`, `bin`, `obj`, and -`node_modules` are skipped. Hidden, system, and reparse-point files are left -alone, and hidden, system, and reparse-point folders are not entered. - -EC reports how many files each exclusion skipped, counting only files your -patterns actually selected — so `-Include "*.bak"` reports that they were -skipped instead of returning nothing at all. These counts do not change the -exit code. +`node_modules` are skipped, and no pattern reaches into them. Hidden, system, +and reparse-point files are left alone, and hidden, system, and reparse-point +folders are not entered. + +EC reports what each exclusion skipped, so a clean result cannot stand in for +complete coverage. For files it reports counts, limited to files your patterns +actually selected — so `-Include "*.bak"` reports that they were skipped instead +of returning nothing at all. For skipped folders it reports the folders, not +their contents, because it does not walk them to find out. These counts do not +change the exit code. ## Conversion diff --git a/sources/EncodingChecker.Tests/NameExcludedFolderCoverageTests.cs b/sources/EncodingChecker.Tests/NameExcludedFolderCoverageTests.cs new file mode 100644 index 0000000..716ab1a --- /dev/null +++ b/sources/EncodingChecker.Tests/NameExcludedFolderCoverageTests.cs @@ -0,0 +1,137 @@ +using System.Text; +using System.Threading; + +namespace EncodingChecker.Tests; + +/// +/// Folders skipped by name must appear in the coverage report. +/// +/// +/// Eleven directory names — .git .svn .hg .vs .idea bin obj node_modules packages dist +/// build target — are skipped deliberately, and that is documented. Attribute-excluded +/// folders were counted; these were not, so a scan of a tree whose only content sat under +/// build/ reported one file, zero exclusions and no warning. In a tool that reports +/// coverage precisely so a clean result cannot stand in for complete coverage, this was the +/// one hole the coverage report could not see. +/// +/// What is scanned is unchanged: these folders are still skipped, and no pattern reaches +/// into one. Only the accounting is fixed. +/// +/// +public sealed class NameExcludedFolderCoverageTests : IDisposable +{ + private readonly string _root = + Directory.CreateTempSubdirectory("ec-namedirs-").FullName; + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup. + } + } + + private void Populate(params string[] folders) + { + foreach (string folder in folders) + { + Directory.CreateDirectory(Path.Combine(_root, folder)); + File.WriteAllText( + Path.Combine(_root, folder, "a.txt"), "content", new UTF8Encoding(false)); + } + } + + private (List Entries, DirectoryTraversal.TraversalCounters Counters) + Scan() + { + var counters = new DirectoryTraversal.TraversalCounters(); + var entries = new EntrySink(); + + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + Action = ScanAction.Detect, + Counters = counters, + MaxParallelism = 1, + }, + entries.Add, + CancellationToken.None); + + return (entries.ToList(), counters); + } + + [Fact] + public void EverySkippedFolderIsCounted() + { + Populate("build", "target", "dist", "packages", "src"); + + var (entries, counters) = Scan(); + + // Unchanged: only src is walked. + Assert.Equal("src", Path.GetFileName(Path.GetDirectoryName(Assert.Single(entries).FilePath))); + + // Fixed: the other four are accounted for rather than invisible. + Assert.Equal(4, counters.DirectoriesExcludedByName); + Assert.Equal(0, counters.DirectoriesExcludedByAttribute); + } + + [Theory] + [InlineData(".git")] + [InlineData(".svn")] + [InlineData(".hg")] + [InlineData(".vs")] + [InlineData(".idea")] + [InlineData("bin")] + [InlineData("obj")] + [InlineData("node_modules")] + [InlineData("packages")] + [InlineData("dist")] + [InlineData("build")] + [InlineData("target")] + public void EachDocumentedNameIsCounted(string folder) + { + // The list in docs/CLI.md and the built-in help names all twelve; none may be + // skipped without saying so. + Populate(folder); + + Assert.Equal(1, Scan().Counters.DirectoriesExcludedByName); + } + + [Fact] + public void ANameExclusionIsReportedSeparatelyFromAnAttributeOne() + { + // Two different reasons a folder was not entered, and the report says which. + Populate("build", "hidden"); + File.SetAttributes( + Path.Combine(_root, "hidden"), + File.GetAttributes(Path.Combine(_root, "hidden")) | FileAttributes.Hidden); + + var (_, counters) = Scan(); + + Assert.Equal(1, counters.DirectoriesExcludedByName); + Assert.Equal(1, counters.DirectoriesExcludedByAttribute); + + string coverage = MainForm.FormatCoverage(counters); + + Assert.Contains("1 folder(s) not entered", coverage); + Assert.Contains("1 build/metadata folder(s) not entered", coverage); + } + + [Fact] + public void ACleanTreeStillReportsNothing() + { + // The counters must stay silent when there is genuinely nothing to disclose, + // or the report becomes noise people learn to skip. + Populate("src"); + + var (_, counters) = Scan(); + + Assert.Equal(0, counters.DirectoriesExcludedByName); + Assert.Equal(string.Empty, MainForm.FormatCoverage(counters)); + } +} diff --git a/sources/EncodingChecker/DirectoryTraversal.cs b/sources/EncodingChecker/DirectoryTraversal.cs index 4f69fcd..52c891e 100644 --- a/sources/EncodingChecker/DirectoryTraversal.cs +++ b/sources/EncodingChecker/DirectoryTraversal.cs @@ -57,6 +57,7 @@ internal sealed class TraversalCounters { private int _filesExcludedByAttribute; private int _directoriesExcludedByAttribute; + private int _directoriesExcludedByName; private int _filesExcludedAsEcArtifact; /// Matching files skipped for being hidden, system, or reparse points. @@ -66,6 +67,19 @@ internal sealed class TraversalCounters internal int DirectoriesExcludedByAttribute => Volatile.Read(ref _directoriesExcludedByAttribute); + /// + /// Directories not entered because their name is a build or metadata convention. + /// + /// + /// Counted for the same reason the attribute exclusions are. Skipping these is + /// deliberate and documented, but leaving them out of the coverage report let a + /// clean result stand in for complete coverage - the one thing this report exists + /// to prevent - and "build" and "target" are ordinary content directory names + /// outside the conventions they were chosen for. + /// + internal int DirectoriesExcludedByName => + Volatile.Read(ref _directoriesExcludedByName); + /// /// Matching files skipped for being EC's own backups, sidecars, or temporaries. /// @@ -79,6 +93,9 @@ internal void CountFileExcludedAsEcArtifact() => internal void CountDirectoryExcludedByAttribute() => Interlocked.Increment(ref _directoriesExcludedByAttribute); + + internal void CountDirectoryExcludedByName() => + Interlocked.Increment(ref _directoriesExcludedByName); } /// @@ -243,9 +260,11 @@ internal static IEnumerable EnumerateFiles( foreach (DirectoryInfo subdirectory in subdirectories) { - if (ExcludedDirectoryNames.Contains( - subdirectory.Name)) + if (ExcludedDirectoryNames.Contains(subdirectory.Name)) + { + counters?.CountDirectoryExcludedByName(); continue; + } // Do not traverse excluded directories merely to count their contents. // Reporting the directory itself is honest about the unknown scope. diff --git a/sources/EncodingChecker/MainForm.Execution.cs b/sources/EncodingChecker/MainForm.Execution.cs index 8ddc48c..365cfe2 100644 --- a/sources/EncodingChecker/MainForm.Execution.cs +++ b/sources/EncodingChecker/MainForm.Execution.cs @@ -372,7 +372,7 @@ internal static string FormatCoverage(DirectoryTraversal.TraversalCounters? coun if (counters is null) return string.Empty; - var parts = new List(3); + var parts = new List(4); if (counters.FilesExcludedByAttribute > 0) { @@ -392,6 +392,12 @@ internal static string FormatCoverage(DirectoryTraversal.TraversalCounters? coun $"{counters.DirectoriesExcludedByAttribute} folder(s) not entered"); } + if (counters.DirectoriesExcludedByName > 0) + { + parts.Add( + $"{counters.DirectoriesExcludedByName} build/metadata folder(s) not entered"); + } + return string.Join(", ", parts); } diff --git a/sources/EncodingChecker/Program.CliExecution.cs b/sources/EncodingChecker/Program.CliExecution.cs index 691573e..c6ba242 100644 --- a/sources/EncodingChecker/Program.CliExecution.cs +++ b/sources/EncodingChecker/Program.CliExecution.cs @@ -381,6 +381,13 @@ .. collectedEntries.OrderBy( + "(hidden, system, or reparse point); their contents were not counted."); } + if (traversalCounters.DirectoriesExcludedByName > 0) + { + Console.Error.WriteLine( + $"{traversalCounters.DirectoriesExcludedByName} folder(s) not entered " + + "(build or metadata name); their contents were not counted."); + } + // Quiet mode must not hide the reason for exit code 3. foreach (ConversionReportEntry entry in entries .Where(e => e.Result == ConversionRowResult.Error)) From 864ab35075c9fff630809cf41f78f40885750add Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Mon, 7 Sep 2026 00:21:39 +0300 Subject: [PATCH 07/16] Say when a folder could not be read at all An unreadable file becomes a row with ScanFailed and drives exit code 3. An unreadable directory produced a warning on stderr and nothing else: no row, no counter, exit 0. The window passes no warning callback, so a GUI user was told nothing whatever. Measured against a real deny ACE: -Validate -FailOnChanges over a tree with one denied folder reported "1 file(s) processed" and exited 0, and a scan whose entire base directory was unreadable printed a header-only CSV and exited 0. A run that examined none of the tree could report success, which is the one thing the coverage report exists to prevent. DirectoriesUnreadable is counted at both catch blocks and kept apart from the two exclusion counters: those record folders EC chose not to enter, this records a failure. The exit code is unchanged. CLI.md states that coverage counts do not affect it, and that sentence was written about folders skipped on purpose rather than ones that could not be read; the documentation now states the consequence plainly, so a script that must not pass over unexamined content reads the counts rather than the exit code alone. Whether this should instead be exit 3 is a product decision. Mutation: removing both increments fails 2 of 6. Co-Authored-By: Claude Opus 5 --- docs/CLI.md | 10 +- .../UnreadableFolderCoverageTests.cs | 188 ++++++++++++++++++ sources/EncodingChecker/DirectoryTraversal.cs | 20 ++ sources/EncodingChecker/MainForm.Execution.cs | 10 +- .../EncodingChecker/Program.CliExecution.cs | 7 + 5 files changed, 232 insertions(+), 3 deletions(-) create mode 100644 sources/EncodingChecker.Tests/UnreadableFolderCoverageTests.cs diff --git a/docs/CLI.md b/docs/CLI.md index 1a920f0..38ccc44 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -48,8 +48,14 @@ EC reports what each exclusion skipped, so a clean result cannot stand in for complete coverage. For files it reports counts, limited to files your patterns actually selected — so `-Include "*.bak"` reports that they were skipped instead of returning nothing at all. For skipped folders it reports the folders, not -their contents, because it does not walk them to find out. These counts do not -change the exit code. +their contents, because it does not walk them to find out. + +A folder EC tried to read and could not — a permission denial, or one removed +mid-scan — is reported the same way, counted separately from the folders it +skipped on purpose. **These counts do not change the exit code.** A run that +could not read part of the tree still exits 0, so a script that must not pass +over unexamined content has to read the counts on stderr rather than the exit +code alone. ## Conversion diff --git a/sources/EncodingChecker.Tests/UnreadableFolderCoverageTests.cs b/sources/EncodingChecker.Tests/UnreadableFolderCoverageTests.cs new file mode 100644 index 0000000..eb04536 --- /dev/null +++ b/sources/EncodingChecker.Tests/UnreadableFolderCoverageTests.cs @@ -0,0 +1,188 @@ +using System.Diagnostics; +using System.Text; +using System.Text.RegularExpressions; +using System.Threading; + +namespace EncodingChecker.Tests; + +/// +/// A folder EC could not read must leave a trace, the way an unreadable file does. +/// +/// +/// An unreadable file becomes a row with ScanFailed and drives exit code 3. An +/// unreadable directory produced a warning on stderr and nothing else: no row, no counter, +/// exit 0 — and nothing whatever in the GUI, which passes no warning callback. Measured +/// against a denied folder: -Validate -FailOnChanges reported "1 file(s) processed" +/// and exited 0, and a scan whose entire base directory was unreadable printed a +/// header-only CSV and exited 0. A run that examined none of the tree could report success, +/// which is the one thing the coverage report exists to prevent. +/// +/// The exit code is deliberately unchanged; docs/CLI.md states that coverage counts +/// do not affect it, and now also states what that means for a script. +/// +/// +public sealed class UnreadableFolderCoverageTests : IDisposable +{ + private readonly string _root = + Directory.CreateTempSubdirectory("ec-unreadable-").FullName; + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup. + } + } + + private static (List Files, DirectoryTraversal.TraversalCounters Counters) + Walk(string baseDirectory) + { + var counters = new DirectoryTraversal.TraversalCounters(); + + List files = + [ + .. DirectoryTraversal.EnumerateFiles( + baseDirectory, + includeSubdirectories: true, + DirectoryTraversal.CompilePatterns(["*"], defaultToMatchAll: true), + [], + excludedFullPaths: null, + onWarning: null, + counters: counters) + ]; + + return (files, counters); + } + + [Fact] + public void AnUnlistableBaseDirectoryIsCounted() + { + // Never created, so listing it fails with DirectoryNotFoundException. This is the + // shape that used to print a header-only CSV and exit 0 with nothing else said. + string ghost = Path.Combine(_root, "never-created-" + Guid.NewGuid().ToString("N")); + + var (files, counters) = Walk(ghost); + + Assert.Empty(files); + Assert.Equal(1, counters.DirectoriesUnreadable); + } + + [Fact] + public void AnUnreadableFolderIsSaidOutLoudInTheCoverageText() + { + // The GUI has no other channel: it passes no warning callback, so this string is + // the whole of what a window user is told. + var counters = new DirectoryTraversal.TraversalCounters(); + counters.CountDirectoryUnreadable(); + + Assert.Equal("1 folder(s) could not be read", MainForm.FormatCoverage(counters)); + } + + [Fact] + public void ItIsCountedApartFromFoldersSkippedOnPurpose() + { + // Three different reasons a folder was not walked, and the report keeps them apart: + // two are policy, one is a failure. + var counters = new DirectoryTraversal.TraversalCounters(); + counters.CountDirectoryExcludedByAttribute(); + counters.CountDirectoryExcludedByName(); + counters.CountDirectoryUnreadable(); + + string coverage = MainForm.FormatCoverage(counters); + + Assert.Contains("1 folder(s) not entered", coverage); + Assert.Contains("1 build/metadata folder(s) not entered", coverage); + Assert.Contains("1 folder(s) could not be read", coverage); + } + + [Fact] + public void AnUnreadableFileStillBecomesARowInstead() + { + // The contrast that made the gap visible: files fail loudly, folders did not. + string path = Path.Combine(_root, "held.txt"); + File.WriteAllText(path, "content", new UTF8Encoding(false)); + + using FileStream exclusive = new(path, FileMode.Open, FileAccess.Read, FileShare.None); + + var entries = new EntrySink(); + var counters = new DirectoryTraversal.TraversalCounters(); + + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + Action = ScanAction.Detect, + Counters = counters, + MaxParallelism = 1, + }, + entries.Add, + CancellationToken.None); + + ConversionReportEntry row = entries.Single(); + + Assert.Equal(ConversionRowResult.Error, row.Result); + Assert.Equal(ConversionReasonCodes.ScanFailed, row.ReasonCode); + Assert.Equal(0, counters.DirectoriesUnreadable); + } + + [Fact] + public void ATreeThatReadsCleanlyReportsNothing() + { + File.WriteAllText(Path.Combine(_root, "a.txt"), "x", new UTF8Encoding(false)); + + var (files, counters) = Walk(_root); + + Assert.Single(files); + Assert.Equal(0, counters.DirectoriesUnreadable); + Assert.Equal(string.Empty, MainForm.FormatCoverage(counters)); + } + + [Fact] + public void ADeniedSubdirectoryIsCountedAndTheScanContinues() + { + string denied = Path.Combine(_root, "denied"); + Directory.CreateDirectory(denied); + File.WriteAllText(Path.Combine(denied, "secret.txt"), "x", new UTF8Encoding(false)); + File.WriteAllText(Path.Combine(_root, "visible.txt"), "x", new UTF8Encoding(false)); + + string user = $"{Environment.UserDomainName}\\{Environment.UserName}"; + + using (Process? deny = Process.Start(new ProcessStartInfo( + "icacls", $"\"{denied}\" /deny \"{user}:(OI)(CI)(RX)\"") + { + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + })) + { + deny?.WaitForExit(10000); + + if (deny is null || deny.ExitCode != 0) + return; // icacls unavailable here; the ghost-root test still covers the counter. + } + + try + { + var (files, counters) = Walk(_root); + + Assert.Equal("visible.txt", Path.GetFileName(Assert.Single(files))); + Assert.Equal(1, counters.DirectoriesUnreadable); + } + finally + { + using Process? restore = Process.Start(new ProcessStartInfo( + "icacls", $"\"{denied}\" /remove:d \"{user}\"") + { + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + }); + + restore?.WaitForExit(10000); + } + } +} diff --git a/sources/EncodingChecker/DirectoryTraversal.cs b/sources/EncodingChecker/DirectoryTraversal.cs index 52c891e..5838737 100644 --- a/sources/EncodingChecker/DirectoryTraversal.cs +++ b/sources/EncodingChecker/DirectoryTraversal.cs @@ -58,6 +58,7 @@ internal sealed class TraversalCounters private int _filesExcludedByAttribute; private int _directoriesExcludedByAttribute; private int _directoriesExcludedByName; + private int _directoriesUnreadable; private int _filesExcludedAsEcArtifact; /// Matching files skipped for being hidden, system, or reparse points. @@ -80,6 +81,18 @@ internal sealed class TraversalCounters internal int DirectoriesExcludedByName => Volatile.Read(ref _directoriesExcludedByName); + /// Directories EC tried to list and could not. + /// + /// Distinct from the two exclusion counters above, which record directories EC + /// chose not to enter. This one records a failure, and it is the one that used to + /// be invisible: an unreadable file becomes a row and drives exit code 3, while an + /// unreadable directory produced a warning on stderr and nothing else - no row, no + /// count, exit 0. A GUI scan reported nothing at all, because the window passes no + /// warning callback. A run that examined none of the tree could report success. + /// + internal int DirectoriesUnreadable => + Volatile.Read(ref _directoriesUnreadable); + /// /// Matching files skipped for being EC's own backups, sidecars, or temporaries. /// @@ -96,6 +109,9 @@ internal void CountDirectoryExcludedByAttribute() => internal void CountDirectoryExcludedByName() => Interlocked.Increment(ref _directoriesExcludedByName); + + internal void CountDirectoryUnreadable() => + Interlocked.Increment(ref _directoriesUnreadable); } /// @@ -191,6 +207,8 @@ internal static IEnumerable EnumerateFiles( catch (Exception ex) when ( ex is IOException or UnauthorizedAccessException) { + counters?.CountDirectoryUnreadable(); + onWarning?.Invoke( $"Skipping directory (cannot list): {dir}{Environment.NewLine} {ex.Message}"); @@ -252,6 +270,8 @@ internal static IEnumerable EnumerateFiles( catch (Exception ex) when ( ex is IOException or UnauthorizedAccessException) { + counters?.CountDirectoryUnreadable(); + onWarning?.Invoke( $"Skipping directory (cannot list): {dir}{Environment.NewLine} {ex.Message}"); diff --git a/sources/EncodingChecker/MainForm.Execution.cs b/sources/EncodingChecker/MainForm.Execution.cs index 365cfe2..3cd0864 100644 --- a/sources/EncodingChecker/MainForm.Execution.cs +++ b/sources/EncodingChecker/MainForm.Execution.cs @@ -372,7 +372,7 @@ internal static string FormatCoverage(DirectoryTraversal.TraversalCounters? coun if (counters is null) return string.Empty; - var parts = new List(4); + var parts = new List(5); if (counters.FilesExcludedByAttribute > 0) { @@ -398,6 +398,14 @@ internal static string FormatCoverage(DirectoryTraversal.TraversalCounters? coun $"{counters.DirectoriesExcludedByName} build/metadata folder(s) not entered"); } + // The window passes no warning callback, so before this counter a directory the + // scan could not read left no trace in the GUI at all. + if (counters.DirectoriesUnreadable > 0) + { + parts.Add( + $"{counters.DirectoriesUnreadable} folder(s) could not be read"); + } + return string.Join(", ", parts); } diff --git a/sources/EncodingChecker/Program.CliExecution.cs b/sources/EncodingChecker/Program.CliExecution.cs index c6ba242..5a18469 100644 --- a/sources/EncodingChecker/Program.CliExecution.cs +++ b/sources/EncodingChecker/Program.CliExecution.cs @@ -388,6 +388,13 @@ .. collectedEntries.OrderBy( + "(build or metadata name); their contents were not counted."); } + if (traversalCounters.DirectoriesUnreadable > 0) + { + Console.Error.WriteLine( + $"{traversalCounters.DirectoriesUnreadable} folder(s) could not be read; " + + "their contents were not examined."); + } + // Quiet mode must not hide the reason for exit code 3. foreach (ConversionReportEntry entry in entries .Where(e => e.Result == ConversionRowResult.Error)) From 40aca2972a57a784ddfa5099d6647addf943c892 Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Mon, 7 Sep 2026 00:21:48 +0300 Subject: [PATCH 08/16] Read a refusal's reason from the decision that made it ApplyConversion worked the reason code out again from the four raw facts Decide had already reduced to a SourceInterpretation. The two copies were textually identical and their operands never changed between them, so they could not disagree - but nothing tied them together, and a fourth refusal reason added to the policy would have fallen through to LegacySourceRequired at the call site: a correct refusal carrying the wrong explanation, with nothing to fail. That shape had already needed one bolt-on "when" guard when the ambiguous BOM-less case was added. ConversionPolicy.ReasonCodeFor sits beside Decide because it answers the same question. Verified equivalent across all 256 reachable combinations of Decide's inputs - the other 256 are unreachable, since a conflict needs an explicit source and the automatic ambiguity needs the absence of one. The test that matters for next time asserts that any Refuse or Skip whose interpretation is unmapped produces no reason at all, and fails. That is the failure mode the old shape had no way to detect. Mutation, both killed: dropping the ExplicitSource case fails 3 of 4; collapsing two refusal reasons fails 2 of 4. Co-Authored-By: Claude Opus 5 --- .../RefusalReasonCodeTests.cs | 143 ++++++++++++++++++ sources/EncodingChecker/ConversionPolicy.cs | 32 ++++ sources/EncodingChecker/ScanEngine.cs | 13 +- 3 files changed, 176 insertions(+), 12 deletions(-) create mode 100644 sources/EncodingChecker.Tests/RefusalReasonCodeTests.cs diff --git a/sources/EncodingChecker.Tests/RefusalReasonCodeTests.cs b/sources/EncodingChecker.Tests/RefusalReasonCodeTests.cs new file mode 100644 index 0000000..9bb69ef --- /dev/null +++ b/sources/EncodingChecker.Tests/RefusalReasonCodeTests.cs @@ -0,0 +1,143 @@ +namespace EncodingChecker.Tests; + +/// +/// The reason EC gives for a decision must come from the decision. +/// +/// +/// ApplyConversion used to work the reason out again from the four raw facts +/// had already reduced to a +/// . The two copies could not disagree — the expressions +/// were identical and their operands never changed between them — but nothing tied them +/// together, and a refusal reason added to the policy would have fallen through to +/// LegacySourceRequired at the call site: a correct refusal carrying the wrong +/// explanation, with no test able to notice. These tests are what now ties them together. +/// +public sealed class RefusalReasonCodeTests +{ + private static readonly (string Charset, int CodePage)[] Sources = + [ + (ScanEngine.UnknownCharset, 0), + ("windows-1252", 1252), + ("utf-8", 65001), + ("utf-16", 1200), + ]; + + private static readonly (string Charset, int CodePage)[] Targets = + [ + ("utf-16", 1200), + ("utf-8", 65001), + ]; + + /// + /// Every combination of Decide's inputs that ApplyConversion can actually + /// produce, with the decision it reaches. + /// + /// + /// The two flags are mutually exclusive by construction at the call site: a conflict + /// needs an explicit source, and the automatic ambiguity needs the absence of one. + /// + private static IEnumerable<(PlannedAction Action, SourceInterpretation Interpretation, + bool Ambiguous, bool Conflict)> Reachable() + { + foreach ((string sc, int scp) in Sources) + foreach ((string tc, int tcp) in Targets) + foreach (bool sourceHasBom in new[] { false, true }) + foreach (bool targetHasBom in new[] { false, true }) + foreach (bool specified in new[] { false, true }) + foreach (bool unicodeOrAscii in new[] { false, true }) + foreach (bool conflict in new[] { false, true }) + foreach (bool ambiguous in new[] { false, true }) + { + if (conflict && !specified) continue; + if (ambiguous && specified) continue; + + PlannedAction action = ConversionPolicy.Decide( + sc, scp, sourceHasBom, tc, tcp, targetHasBom, + specified, unicodeOrAscii, conflict, ambiguous, + out SourceInterpretation interpretation, out _); + + yield return (action, interpretation, ambiguous, conflict); + } + } + + /// The formula this replaced, kept as the oracle for the change itself. + private static string? PreviousFormula( + PlannedAction action, bool ambiguous, bool conflict) => action switch + { + PlannedAction.Skip => ConversionReasonCodes.UnknownEncoding, + PlannedAction.Refuse when ambiguous => ConversionReasonCodes.AmbiguousBomlessUtf16, + PlannedAction.Refuse => conflict + ? ConversionReasonCodes.ExplicitSourceConflictsWithDetection + : ConversionReasonCodes.LegacySourceRequired, + _ => null, + }; + + [Fact] + public void TheReasonIsUnchangedFromTheFormulaItReplaced() + { + var checkedCombinations = 0; + + foreach ((PlannedAction action, SourceInterpretation interpretation, + bool ambiguous, bool conflict) in Reachable()) + { + checkedCombinations++; + + Assert.Equal( + PreviousFormula(action, ambiguous, conflict), + ConversionPolicy.ReasonCodeFor(action, interpretation)); + } + + // A rule that silently matched nothing would look identical to one that passed. + Assert.Equal(256, checkedCombinations); + } + + [Fact] + public void EveryOutcomeThatLeavesAFileAloneCarriesAReason() + { + // The guard that matters for the next refusal reason someone adds: a decision not + // to convert has to explain itself, and an unmapped interpretation returns null. + foreach ((PlannedAction action, SourceInterpretation interpretation, _, _) in Reachable()) + { + if (action is not (PlannedAction.Refuse or PlannedAction.Skip)) + continue; + + Assert.False( + string.IsNullOrEmpty(ConversionPolicy.ReasonCodeFor(action, interpretation)), + $"{action}/{interpretation} produced no reason code"); + } + } + + [Fact] + public void EachRefusalPathHasItsOwnReason() + { + // Three ways to refuse, three distinct codes: collapsing any two would tell a user + // to do something that cannot resolve their case. + string?[] codes = + [ + ConversionPolicy.ReasonCodeFor( + PlannedAction.Refuse, SourceInterpretation.ExplicitSource), + ConversionPolicy.ReasonCodeFor( + PlannedAction.Refuse, SourceInterpretation.AutomaticUnicodeOrAscii), + ConversionPolicy.ReasonCodeFor( + PlannedAction.Refuse, SourceInterpretation.LegacyNeedsSourceChoice), + ]; + + Assert.Equal(3, codes.Distinct(StringComparer.Ordinal).Count()); + Assert.All(codes, code => Assert.False(string.IsNullOrEmpty(code))); + } + + [Fact] + public void ADecisionToWriteOrLeaveAloneCarriesNoReason() + { + // A reason code on a plain conversion would read as a problem in the report. + // Written as one test rather than a theory because PlannedAction is internal and + // an InlineData parameter would have to be public. + foreach (PlannedAction action in + new[] { PlannedAction.Convert, PlannedAction.Unchanged }) + foreach (SourceInterpretation interpretation in + Enum.GetValues()) + { + Assert.Null(ConversionPolicy.ReasonCodeFor(action, interpretation)); + } + } +} diff --git a/sources/EncodingChecker/ConversionPolicy.cs b/sources/EncodingChecker/ConversionPolicy.cs index 51d2c71..09bd5aa 100644 --- a/sources/EncodingChecker/ConversionPolicy.cs +++ b/sources/EncodingChecker/ConversionPolicy.cs @@ -119,6 +119,38 @@ internal static PlannedAction Decide( _ => ConversionRowResult.Converted, }; + /// + /// The machine-readable reason for a decision, read from the decision itself. + /// + /// + /// Beside because it answers the same question. The caller used to + /// work the reason out again from the raw inputs, re-deriving the distinction + /// had already been handed back to express. The two + /// could not disagree - the expressions were identical and their operands never changed + /// between them - but a refusal reason added to would have fallen + /// through to at the call site: + /// a correct refusal carrying the wrong explanation, with nothing to fail. That already + /// happened once, when the ambiguous BOM-less case had to be bolted on as a guard rather + /// than added as a case. + /// + internal static string? ReasonCodeFor( + PlannedAction action, + SourceInterpretation sourceInterpretation) => (action, sourceInterpretation) switch + { + (PlannedAction.Skip, _) => ConversionReasonCodes.UnknownEncoding, + + (PlannedAction.Refuse, SourceInterpretation.AutomaticUnicodeOrAscii) => + ConversionReasonCodes.AmbiguousBomlessUtf16, + + (PlannedAction.Refuse, SourceInterpretation.ExplicitSource) => + ConversionReasonCodes.ExplicitSourceConflictsWithDetection, + + (PlannedAction.Refuse, SourceInterpretation.LegacyNeedsSourceChoice) => + ConversionReasonCodes.LegacySourceRequired, + + _ => null, + }; + /// /// Whether a refusal can be resolved by the user identifying the original source /// encoding. Kept here so the GUI, plans, and CLI describe the same policy. diff --git a/sources/EncodingChecker/ScanEngine.cs b/sources/EncodingChecker/ScanEngine.cs index 692bad1..ad78d9d 100644 --- a/sources/EncodingChecker/ScanEngine.cs +++ b/sources/EncodingChecker/ScanEngine.cs @@ -857,18 +857,7 @@ approved.Action is not PlannedAction.Convert && entry.Action = action; entry.SourceInterpretation = sourceInterpretation; - entry.ReasonCode = action switch - { - PlannedAction.Skip => ConversionReasonCodes.UnknownEncoding, - PlannedAction.Refuse when automaticBomlessUtf16IsAmbiguous => - ConversionReasonCodes.AmbiguousBomlessUtf16, - PlannedAction.Refuse => entry.SourceEncodingWasSpecified && - entry.HasReliableUnicodeDetection && automaticallyDetected is not null && - automaticallyDetected.CodePage != sourceEncoding.CodePage - ? ConversionReasonCodes.ExplicitSourceConflictsWithDetection - : ConversionReasonCodes.LegacySourceRequired, - _ => null, - }; + entry.ReasonCode = ConversionPolicy.ReasonCodeFor(action, sourceInterpretation); // A retry must not carry a diagnostic from an earlier failed attempt. // The optional BOM-less Unicode advisory below is added back for this pass. From 9e829f86bc00a1d16ff144f87fe5a0cc77a9cfe7 Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Mon, 7 Sep 2026 00:21:57 +0300 Subject: [PATCH 09/16] Give every -Validate rejection a reason -Validate had four ways to return Invalid and explained only two. A file whose whole contents fail strict decoding says StrictValidationFailed, and one whose BOM-less byte order cannot be proven says AmbiguousBomlessUtf16. The other two arrived as a bare Invalid with an empty reason column. They are not the same situation. A charset outside the allowed list means widen the list or convert the file; an unidentifiable one means EC could not tell what it is, which -DetectOnly already calls UnknownEncoding. Both were merged by one boolean covering two unlike conditions. CharsetNotAllowed is new. UnknownEncoding is deliberately reused rather than given a -Validate-specific name, because two names for one condition depending on which mode ran would be its own defect. This was the only outcome in the product where the reader had to re-derive a reason the producer already knew. A script could always classify these rows by comparing the encoding column against the list it had passed in - it just had to do the work twice. Mutation, both killed: removing the branch fails 3 of 6; collapsing the two situations into one code fails 1 of 6. Co-Authored-By: Claude Opus 5 --- .../ValidateReasonCodeTests.cs | 143 ++++++++++++++++++ sources/EncodingChecker/ConversionReport.cs | 11 ++ sources/EncodingChecker/ScanEngine.cs | 22 ++- 3 files changed, 173 insertions(+), 3 deletions(-) create mode 100644 sources/EncodingChecker.Tests/ValidateReasonCodeTests.cs diff --git a/sources/EncodingChecker.Tests/ValidateReasonCodeTests.cs b/sources/EncodingChecker.Tests/ValidateReasonCodeTests.cs new file mode 100644 index 0000000..3121b67 --- /dev/null +++ b/sources/EncodingChecker.Tests/ValidateReasonCodeTests.cs @@ -0,0 +1,143 @@ +using System.Text; +using System.Threading; + +namespace EncodingChecker.Tests; + +/// +/// Every way -Validate can reject a file has to say which way it was. +/// +/// +/// There are four. Two explained themselves — a file whose whole contents fail strict +/// decoding, and one whose BOM-less byte order cannot be proven. The other two arrived as a +/// bare Invalid with an empty reason column, and they are not the same situation: a +/// charset the caller did not allow means widen the list or convert the file, while an +/// unidentifiable one means EC could not tell what it is. That made -Validate the +/// only outcome in the product whose reason the reader had to reconstruct, from the encoding +/// column and the list they had passed in. +/// +public sealed class ValidateReasonCodeTests : IDisposable +{ + private readonly string _root = + Directory.CreateTempSubdirectory("ec-validate-").FullName; + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup. + } + } + + private ConversionReportEntry Validate(byte[] contents, params string[] allowed) + { + File.WriteAllBytes(Path.Combine(_root, "f.txt"), contents); + + var entries = new EntrySink(); + + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + Action = ScanAction.Validate, + ValidCharsets = allowed, + MaxParallelism = 1, + }, + entries.Add, + CancellationToken.None); + + return entries.Single(); + } + + private static byte[] Cjk(int repeats) => + new UTF8Encoding(false).GetBytes( + string.Concat(Enumerable.Repeat("中文内容测试", repeats))); + + [Fact] + public void AnAllowedCharsetThatDecodesCleanlyPasses() + { + ConversionReportEntry entry = Validate( + new UTF8Encoding(false).GetBytes("plain ascii content\n"), "us-ascii", "utf-8"); + + Assert.Equal(ConversionRowResult.Unchanged, entry.Result); + Assert.Null(entry.ReasonCode); + } + + [Fact] + public void ACharsetOutsideTheAllowedListSaysSo() + { + ConversionReportEntry entry = Validate( + Encoding.GetEncoding("windows-1251").GetBytes("Здравствуй, мир! Это русский текст.\n"), + "us-ascii", "utf-8"); + + Assert.Equal(ConversionRowResult.Invalid, entry.Result); + Assert.Equal(ConversionReasonCodes.CharsetNotAllowed, entry.ReasonCode); + Assert.Contains("windows-1251", entry.Diagnostic); + Assert.Contains("not in the allowed list", entry.Diagnostic); + } + + [Fact] + public void AFileEcCannotIdentifyIsNamedAsThatInstead() + { + // The distinction the empty reason column erased: this is not "you did not allow + // it", and -DetectOnly already calls the same condition UnknownEncoding. + ConversionReportEntry entry = Validate([], "us-ascii", "utf-8"); + + Assert.Equal(ConversionRowResult.Invalid, entry.Result); + Assert.Equal(ConversionReasonCodes.UnknownEncoding, entry.ReasonCode); + Assert.NotEqual(ConversionReasonCodes.CharsetNotAllowed, entry.ReasonCode); + } + + [Fact] + public void AnAllowedCharsetThatFailsStrictDecodingKeepsItsOwnReason() + { + ConversionReportEntry entry = Validate([.. Cjk(3), 0xE4, 0xB8], "utf-8"); + + Assert.Equal(ConversionRowResult.Invalid, entry.Result); + Assert.Equal(ConversionReasonCodes.StrictValidationFailed, entry.ReasonCode); + Assert.Contains("E4", entry.Diagnostic); + } + + [Fact] + public void AnAllowedCharsetWithAnUnprovableByteOrderKeepsItsOwnReason() + { + byte[] utf16 = new UnicodeEncoding(false, false).GetBytes( + string.Concat(Enumerable.Repeat("a quiet line of words\n", 4))); + + ConversionReportEntry entry = Validate(utf16, "utf-16"); + + Assert.Equal(ConversionRowResult.Invalid, entry.Result); + Assert.Equal(ConversionReasonCodes.AmbiguousBomlessUtf16, entry.ReasonCode); + } + + [Fact] + public void NoRejectionLeavesTheReaderWithoutAReason() + { + // The property, rather than the four cases: nothing -Validate rejects may arrive + // as a bare Invalid again. + (byte[] Contents, string[] Allowed)[] rejected = + [ + (Encoding.GetEncoding("windows-1251").GetBytes("Здравствуй, мир!\n"), ["utf-8"]), + ([], ["utf-8"]), + ([.. Cjk(3), 0xE4, 0xB8], ["utf-8"]), + (new UnicodeEncoding(false, false).GetBytes( + string.Concat(Enumerable.Repeat("a quiet line of words\n", 4))), ["utf-16"]), + ]; + + foreach ((byte[] contents, string[] allowed) in rejected) + { + ConversionReportEntry entry = Validate(contents, allowed); + + Assert.Equal(ConversionRowResult.Invalid, entry.Result); + Assert.False( + string.IsNullOrEmpty(entry.ReasonCode), + $"a rejected file carried no reason code (encoding was {entry.SourceEncoding})"); + Assert.False( + string.IsNullOrEmpty(entry.Diagnostic), + $"a rejected file carried no diagnostic (encoding was {entry.SourceEncoding})"); + } + } +} diff --git a/sources/EncodingChecker/ConversionReport.cs b/sources/EncodingChecker/ConversionReport.cs index 55dc41c..cf69c60 100644 --- a/sources/EncodingChecker/ConversionReport.cs +++ b/sources/EncodingChecker/ConversionReport.cs @@ -225,6 +225,17 @@ internal static class ConversionReasonCodes nameof(ExplicitSourceOnUnprovableBomlessUnicode); internal const string AmbiguousBomlessUtf16 = BomlessUnicodeSafety.AmbiguousReasonCode; internal const string StrictValidationFailed = nameof(StrictValidationFailed); + + /// + /// -Validate identified the file, and its charset is not in the allowed list. + /// + /// + /// Distinct from , which these rows used to be + /// indistinguishable from: one says widen the list or convert the file, the other says + /// EC could not tell what the file is. Both arrived as a bare Invalid with an + /// empty reason, the only outcome in the product that did not explain itself. + /// + internal const string CharsetNotAllowed = nameof(CharsetNotAllowed); internal const string SourceSnapshotFailed = nameof(SourceSnapshotFailed); internal const string BackupFailed = nameof(BackupFailed); internal const string MultipleLeadingByteOrderMarks = nameof(MultipleLeadingByteOrderMarks); diff --git a/sources/EncodingChecker/ScanEngine.cs b/sources/EncodingChecker/ScanEngine.cs index ad78d9d..355a375 100644 --- a/sources/EncodingChecker/ScanEngine.cs +++ b/sources/EncodingChecker/ScanEngine.cs @@ -556,13 +556,29 @@ options.ValidCharsets is not null && ? ConversionRowResult.Unchanged : ConversionRowResult.Invalid; - if (isValid && entry.Result == ConversionRowResult.Invalid) + if (!isValid) + { + // Two unlike situations used to arrive here as the same bare Invalid: + // a file EC could not identify, and one it identified as something the + // caller did not allow. Every other mode names both. Leaving these + // blank made -Validate the only outcome whose reason the reader had to + // reconstruct from the encoding column and the list they passed in. + bool identified = sourceCharset != UnknownCharset; + + entry.ReasonCode = identified + ? ConversionReasonCodes.CharsetNotAllowed + : ConversionReasonCodes.UnknownEncoding; + + entry.Diagnostic = identified + ? $"The file is {label}, which is not in the allowed list." + : "The file's encoding could not be identified from its contents."; + } + else if (entry.Result == ConversionRowResult.Invalid) { entry.ReasonCode = ConversionReasonCodes.StrictValidationFailed; entry.Diagnostic = validationDiagnostic; } - else if (entry.Result == ConversionRowResult.Unchanged && - entry.HasAmbiguousBomlessUtf16) + else if (entry.HasAmbiguousBomlessUtf16) { // The two byte orders are separate entries in the allowed set; the // label matched only because .NET names both "utf-16". Passing the From 892caeeeb6c1187421426472358884051fa891cb Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Mon, 7 Sep 2026 00:22:06 +0300 Subject: [PATCH 10/16] Verify a file before calling it already in the target encoding Detection reads at most 64 KiB. When the source codec matched the target, Unchanged was reported and nothing read further, so a file whose first 64 KiB is clean and whose later bytes are not valid in the codec EC had just named was reported as already correct. Whether EC noticed depended only on which target the caller typed: the same corrupt file came back Error under -Target utf-16 and Unchanged under -Target utf-8. -Validate always read the whole file. The Convert path simply never reached that check once it had decided it had nothing to do. Reported as the pre-check it is: Action stays Unchanged, nothing was attempted, and the reason code is the one -Validate already uses. Eleven tests, including the matrix as a theory and a clean 70 KiB file staying Unchanged under every target, so the check cannot start refusing what it was added to describe honestly. Cost, measured: near zero, and not for the reason first assumed. Convert already reads every byte of every file, because CaptureSourceSnapshot hashes the whole stream before anything is decided. This adds a decode to a pass that already reads and hashes: 0.04 to 0.10 ms per MiB across 8 MiB and 120 MiB corpora, medians of five interleaved runs from a warm cache. This also retires a scoping decision made in the alias fix. ASCII to UTF-8 was kept as a conversion there on the grounds that folding it into Unchanged would skip the full-file decode; that reasoning was wrong, because Unchanged already skipped it for every same-codec case. Folding them together is now safe, and is worth doing separately. Mutation: removing the check fails 4 of 11. Co-Authored-By: Claude Opus 5 --- .../UnchangedVerificationTests.cs | 159 ++++++++++++++++++ sources/EncodingChecker/ScanEngine.cs | 20 +++ 2 files changed, 179 insertions(+) create mode 100644 sources/EncodingChecker.Tests/UnchangedVerificationTests.cs diff --git a/sources/EncodingChecker.Tests/UnchangedVerificationTests.cs b/sources/EncodingChecker.Tests/UnchangedVerificationTests.cs new file mode 100644 index 0000000..3c930d8 --- /dev/null +++ b/sources/EncodingChecker.Tests/UnchangedVerificationTests.cs @@ -0,0 +1,159 @@ +using System.Text; +using System.Threading; + +namespace EncodingChecker.Tests; + +/// +/// "Already in the target encoding" has to be true of the whole file. +/// +/// +/// Detection reads at most 64 KiB, and when the source codec matched the target nothing read +/// further. A file whose first 64 KiB was clean and whose later bytes were not valid in the +/// codec EC had just named was reported Unchanged — and whether EC noticed depended +/// only on which target the caller typed: the same corrupt file came back Error under +/// -Target utf-16 and Unchanged under -Target utf-8. -Validate +/// always read the whole file; the Convert path simply never reached that check once it had +/// decided it had nothing to do. +/// +public sealed class UnchangedVerificationTests : IDisposable +{ + private readonly string _root = + Directory.CreateTempSubdirectory("ec-unchanged-").FullName; + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup. + } + } + + /// Clean text well past the detection sample, optionally spoiled at the end. + private static byte[] PastTheSample(bool ascii, bool corrupt) + { + string unit = ascii ? "the quick brown fox jumps over the lazy dog. " : "中文内容测试 "; + var text = new StringBuilder(); + + while (Encoding.UTF8.GetByteCount(text.ToString()) < 70 * 1024) + text.Append(unit); + + byte[] clean = new UTF8Encoding(false).GetBytes(text.ToString()); + + // An overlong "/" — invalid in UTF-8 and outside us-ascii alike. + return corrupt ? [.. clean, 0xC0, 0xAF] : clean; + } + + private ConversionReportEntry Convert(byte[] contents, string target) + { + File.WriteAllBytes(Path.Combine(_root, "f.txt"), contents); + + var entries = new EntrySink(); + + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + Action = ScanAction.Convert, + TargetCharset = target, + MaxParallelism = 1, + }, + entries.Add, + CancellationToken.None); + + return entries.Single(); + } + + [Theory] + [InlineData(true, "us-ascii")] + [InlineData(true, "utf-8")] + [InlineData(true, "utf-16")] + [InlineData(false, "us-ascii")] + [InlineData(false, "utf-8")] + [InlineData(false, "utf-16")] + public void TheSameCorruptFileFailsUnderEveryTarget(bool ascii, string target) + { + // The matrix is the finding: the outcome must not depend on which target is named. + byte[] corrupt = PastTheSample(ascii, corrupt: true); + + ConversionReportEntry entry = Convert(corrupt, target); + + Assert.Equal(ConversionRowResult.Error, entry.Result); + Assert.False(string.IsNullOrEmpty(entry.ReasonCode)); + Assert.Equal(corrupt, File.ReadAllBytes(Path.Combine(_root, "f.txt"))); + } + + [Fact] + public void TheUnchangedPathNamesTheCheckThatFailed() + { + // Reported as the pre-check it is, not as a conversion that went wrong: no write + // was attempted at all. + ConversionReportEntry entry = Convert(PastTheSample(ascii: true, corrupt: true), "us-ascii"); + + Assert.Equal(PlannedAction.Unchanged, entry.Action); + Assert.Equal(ConversionRowResult.Error, entry.Result); + Assert.Equal(ConversionReasonCodes.StrictValidationFailed, entry.ReasonCode); + Assert.False(entry.ReplacementCommitted); + Assert.False(string.IsNullOrEmpty(entry.Diagnostic)); + } + + [Theory] + [InlineData(true, "us-ascii")] + [InlineData(false, "utf-8")] + public void AFileThatReallyIsInTheTargetEncodingIsStillUnchanged(bool ascii, string target) + { + // The check must not start refusing the ordinary case it was added to describe + // honestly — including well past the 64 KiB sample. + byte[] clean = PastTheSample(ascii, corrupt: false); + + ConversionReportEntry entry = Convert(clean, target); + + Assert.Equal(PlannedAction.Unchanged, entry.Action); + Assert.Equal(ConversionRowResult.Unchanged, entry.Result); + Assert.Null(entry.ReasonCode); + Assert.Equal(clean, File.ReadAllBytes(Path.Combine(_root, "f.txt"))); + } + + [Fact] + public void AShortCleanFileIsStillUnchanged() + { + ConversionReportEntry entry = Convert( + new UTF8Encoding(false).GetBytes("plain ascii content\n"), "us-ascii"); + + Assert.Equal(ConversionRowResult.Unchanged, entry.Result); + Assert.Null(entry.ReasonCode); + } + + [Fact] + public void ConvertAndValidateNowAgreeAboutTheSameFile() + { + // -Validate always caught this. The two modes disagreeing about one file was the + // shape of the defect. + byte[] corrupt = PastTheSample(ascii: true, corrupt: true); + File.WriteAllBytes(Path.Combine(_root, "f.txt"), corrupt); + + var validated = new EntrySink(); + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + Action = ScanAction.Validate, + ValidCharsets = ["us-ascii", "utf-8"], + MaxParallelism = 1, + }, + validated.Add, + CancellationToken.None); + + Assert.Equal(ConversionRowResult.Invalid, validated.Single().Result); + Assert.Equal( + ConversionReasonCodes.StrictValidationFailed, validated.Single().ReasonCode); + + // Same file, same reason, from the Convert path. + Assert.Equal( + ConversionReasonCodes.StrictValidationFailed, + Convert(corrupt, "us-ascii").ReasonCode); + } +} diff --git a/sources/EncodingChecker/ScanEngine.cs b/sources/EncodingChecker/ScanEngine.cs index 355a375..6d41a27 100644 --- a/sources/EncodingChecker/ScanEngine.cs +++ b/sources/EncodingChecker/ScanEngine.cs @@ -905,6 +905,26 @@ automaticallyDetected is not null && + "your explicit selection and kept all strict conversion checks enabled."; } + // "Already in the target encoding" is a claim about the whole file, and detection + // saw at most the first 64 KiB of it. Without this check a file whose later bytes + // are not valid in the codec just named was reported as already correct, and + // whether EC noticed depended only on which target the caller happened to type: + // the same corrupt file was an Error under -Target utf-16 and Unchanged under + // -Target utf-8. -Validate has always read the whole file; this is the same check, + // reached from the one path that had decided it had nothing to do. + if (action == PlannedAction.Unchanged && + !StrictFileValidation.TryValidateFile( + path, sourceEncoding, out string? unchangedDiagnostic)) + { + entry.Result = ConversionRowResult.Error; + entry.ReasonCode = ConversionReasonCodes.StrictValidationFailed; + entry.Diagnostic = unchangedDiagnostic; + + // Nothing was written, and nothing was going to be. + entry.ReplacementCommitted = false; + return; + } + if (action != PlannedAction.Convert) { entry.Result = ConversionPolicy.ToRowResult(action); From 75eb87c808fa4286eb129ce4e40a24391603116e Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Mon, 7 Sep 2026 00:22:16 +0300 Subject: [PATCH 11/16] Decode what a preview promises it would convert ApplyConversion returned at the whatIf branch before the converter ran, so nothing decoded the file. -Plan sets WhatIf, so a plan recorded Action=Convert with no reason for a source that cannot be read, exited 0, and showed the reviewer nothing. The failure surfaced at -Apply, after approval and part-way through the batch: one file converted, one failed. FindStaleFiles can prove the bytes have not changed since review and cannot prove they are readable, because that needs a decode nobody performed. The entry is marked Refuse rather than merely carrying an error. The plan records the action, so an error row still saying Convert would have put the file in front of a reviewer as approved work. -Plan now exits 3. The decode only, by decision. A source that reads cleanly can still fail on a target that cannot represent it - Cyrillic previewed to us-ascii still reports "would be converted" and still fails with TargetEncodeError - and predicting that means running the whole conversion into a discarded buffer. A test named for that case asserts it, so the gap is recorded rather than left to be filed as a defect later. Cost, measured: 22 to 39 percent of a preview measured in tens of milliseconds, and an upper bound, since the benchmark adds a read where the snapshot pass already has the bytes. Mutation, both killed: removing the check fails 3 of 8; reporting the error but still planning Convert fails 1 of 8. Co-Authored-By: Claude Opus 5 --- .../PreviewVerificationTests.cs | 162 ++++++++++++++++++ sources/EncodingChecker/ScanEngine.cs | 21 +++ 2 files changed, 183 insertions(+) create mode 100644 sources/EncodingChecker.Tests/PreviewVerificationTests.cs diff --git a/sources/EncodingChecker.Tests/PreviewVerificationTests.cs b/sources/EncodingChecker.Tests/PreviewVerificationTests.cs new file mode 100644 index 0000000..e086319 --- /dev/null +++ b/sources/EncodingChecker.Tests/PreviewVerificationTests.cs @@ -0,0 +1,162 @@ +using System.Text; +using System.Threading; + +namespace EncodingChecker.Tests; + +/// +/// A preview must not promise a conversion that would fail. +/// +/// +/// ApplyConversion returned at the whatIf branch before the converter ran, so +/// nothing decoded the file. -Plan sets WhatIf, so a plan recorded +/// Action = Convert with no reason for a source that cannot be read, exited 0, and +/// showed the reviewer nothing; the failure surfaced at -Apply, after approval and +/// part-way through the batch. FindStaleFiles can prove the bytes have not changed +/// since review and cannot prove they are readable, because that needs a decode nobody +/// performed. +/// +/// The decode only. A source that reads cleanly can still fail on a target that cannot +/// represent it — TargetEncodeError — and previewing that means running the whole +/// conversion into a discarded buffer. +/// records that limit rather than leaving it to be discovered. +/// +/// +public sealed class PreviewVerificationTests : IDisposable +{ + private readonly string _root = + Directory.CreateTempSubdirectory("ec-preview-").FullName; + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup. + } + } + + /// Clean text past the 64 KiB detection sample, optionally spoiled at the end. + private static byte[] PastTheSample(bool corrupt) + { + var text = new StringBuilder(); + + while (Encoding.UTF8.GetByteCount(text.ToString()) < 70 * 1024) + text.Append("the quick brown fox jumps over the lazy dog. "); + + byte[] clean = new UTF8Encoding(false).GetBytes(text.ToString()); + + return corrupt ? [.. clean, 0xC0, 0xAF] : clean; + } + + private ConversionReportEntry Preview(byte[] contents, string target, string? from = null) + { + File.WriteAllBytes(Path.Combine(_root, "f.txt"), contents); + + var entries = new EntrySink(); + + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + Action = ScanAction.Convert, + TargetCharset = target, + SourceCharset = from, + WhatIf = true, + MaxParallelism = 1, + }, + entries.Add, + CancellationToken.None); + + return entries.Single(); + } + + [Theory] + [InlineData("utf-8")] + [InlineData("utf-16")] + [InlineData("us-ascii")] + public void APreviewRefusesWhatARealRunWouldRefuse(string target) + { + ConversionReportEntry entry = Preview(PastTheSample(corrupt: true), target); + + Assert.Equal(ConversionRowResult.Error, entry.Result); + Assert.Equal(ConversionReasonCodes.StrictValidationFailed, entry.ReasonCode); + Assert.False(string.IsNullOrEmpty(entry.Diagnostic)); + } + + [Fact] + public void APlanDoesNotScheduleAFileItCannotCarryOut() + { + // The action is what the plan records, so a preview that merely reported an error + // while still saying Convert would put it in front of a reviewer as approved work. + ConversionReportEntry entry = Preview(PastTheSample(corrupt: true), "utf-16"); + + Assert.Equal(PlannedAction.Refuse, entry.Action); + Assert.False(entry.ReplacementCommitted); + + ConversionPlan plan = ConversionPlan.FromEntries( + [entry], _root, "utf-16", targetHasBom: false, + backupEnabled: false, explicitSource: null); + + PlannedFile planned = Assert.Single(plan.Files); + + Assert.Equal(PlannedAction.Refuse, planned.Action); + Assert.False(planned.NeedsSourceChoice); + Assert.Equal(0, plan.Summary.ReadyToConvert); + } + + [Theory] + [InlineData("utf-8")] + [InlineData("utf-16")] + public void AnOrdinaryFileStillPreviewsAsConvertible(string target) + { + // The check must not start refusing the files a preview exists to describe, and + // 70 KiB puts the clean tail well past the detection sample. + ConversionReportEntry entry = Preview(PastTheSample(corrupt: false), target); + + Assert.Equal(PlannedAction.Convert, entry.Action); + Assert.Equal(ConversionRowResult.Converted, entry.Result); + Assert.Null(entry.ReasonCode); + } + + [Fact] + public void APreviewStillWritesNothing() + { + byte[] contents = PastTheSample(corrupt: false); + + Preview(contents, "utf-16"); + + Assert.Equal(contents, File.ReadAllBytes(Path.Combine(_root, "f.txt"))); + Assert.Empty(Directory.GetFiles(_root, "*.bak")); + Assert.Empty(Directory.GetFiles(_root, "*" + ConversionMetadataStore.Suffix)); + } + + [Fact] + public void ATargetThatCannotRepresentTheTextIsStillNotPredicted() + { + // The deliberate limit of this fix, pinned so it is a known gap rather than a + // surprise: the source decodes cleanly, so the preview says it would convert, and + // the real run fails encoding it into a target that has no Cyrillic. + byte[] cyrillic = new UTF8Encoding(false).GetBytes("Здравствуй, мир!\n"); + + Assert.Equal(ConversionRowResult.Converted, Preview(cyrillic, "us-ascii").Result); + + var real = new EntrySink(); + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + Action = ScanAction.Convert, + TargetCharset = "us-ascii", + MaxParallelism = 1, + }, + real.Add, + CancellationToken.None); + + Assert.Equal(ConversionRowResult.Error, real.Single().Result); + Assert.Equal( + nameof(ConversionErrorCode.TargetEncodeError), real.Single().ReasonCode); + } +} diff --git a/sources/EncodingChecker/ScanEngine.cs b/sources/EncodingChecker/ScanEngine.cs index 6d41a27..712e252 100644 --- a/sources/EncodingChecker/ScanEngine.cs +++ b/sources/EncodingChecker/ScanEngine.cs @@ -947,6 +947,27 @@ automaticallyDetected is not null && if (whatIf) { + // A preview saying "would be converted" has to have read what it is promising + // about. Nothing here decoded the file, so -Plan - which sets WhatIf - recorded + // Action=Convert with no reason for a source that cannot be read, exited 0, and + // showed the reviewer nothing; the failure surfaced at -Apply, after approval + // and part-way through the batch. Refuse rather than schedule it, so the plan + // never carries a file it cannot carry out. + // + // The decode only. A source that reads cleanly can still fail on a target that + // cannot represent it, and no amount of reading the source predicts that; + // closing that half means running the whole conversion into a discarded buffer. + if (!StrictFileValidation.TryValidateFile( + path, sourceEncoding, out string? previewDiagnostic)) + { + entry.Action = PlannedAction.Refuse; + entry.Result = ConversionRowResult.Error; + entry.ReasonCode = ConversionReasonCodes.StrictValidationFailed; + entry.Diagnostic = previewDiagnostic; + entry.ReplacementCommitted = false; + return; + } + entry.Result = ConversionRowResult.Converted; // "would be converted" return; } From 7a19a95502e73bb0bc4cdd1226281584c0187a66 Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Mon, 7 Sep 2026 00:22:25 +0300 Subject: [PATCH 12/16] Say that a second conversion replaces the first backup .bak is a fixed name holding the version the most recent run replaced. A second conversion of the same file replaces it and removes its sidecar, so the original becomes unrecoverable. That is deliberate, pinned by BackupIntegrityTests.Backup_OverwritesAnyPreviousBackupFile since the first commit of the test suite, and the contract is stated in BackupRecordPairingTests: .bak is "the version this run replaced". No document said so. CLI.md said "Save every replaced original as .bak" and SAFETY.md called it a recovery artifact and stopped there, so a user converting twice lost the original with nothing having warned them. This was raised during review as a defect and withdrawn. Refusing to overwrite a non-matching .bak breaks four existing tests and would block an ordinary "wrong target, convert again" run until the user deleted the backups by hand. Naming backups by conversion ID was proposed once before and declined, because .bak is load-bearing beyond the backup itself: HasReservedArtifactSuffix, the -Include "*.bak" coverage reporting, GUI smoke phase A, and both documents key off that name. Co-Authored-By: Claude Opus 5 --- docs/CLI.md | 2 +- docs/SAFETY.md | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/CLI.md b/docs/CLI.md index 38ccc44..2cc4ed6 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -63,7 +63,7 @@ code alone. |---|---| | `-Target ` | Target encoding, for example `utf-8` or `utf-8-bom`. Required for conversion. | | `-From ` | Explicit original encoding for every selected file. Use when you know a legacy source encoding. | -| `-Backup` | Save every replaced original as `.bak`. | +| `-Backup` | Save the replaced original as `.bak`. The name is fixed, so converting the same file again replaces that backup. | | `-WhatIf` | Show a one-time preview without writing files. | | `-Plan ` | Write a reviewable conversion plan; do not modify files. | | `-Apply ` | Execute a saved plan. Its scope and conversion settings are fixed. | diff --git a/docs/SAFETY.md b/docs/SAFETY.md index 3e57b6b..704114d 100644 --- a/docs/SAFETY.md +++ b/docs/SAFETY.md @@ -68,6 +68,12 @@ whether installation was prepared or completed. The `.bak` and sidecar provide independently verifiable recovery information. EC does not currently provide a built-in restore command. +`.bak` is a fixed name holding the version the most recent run replaced, so +it is one level of undo rather than a history. Converting the same file again +replaces the backup, and removes its sidecar with it. If you may want the original +of a file you are about to convert a second time, copy the existing `.bak` +elsewhere first. + `-Journal` creates the batch-level record: detection, chosen source, decision, reason code, final result, and before/after hashes for every file. From 36cd14d6598b34cbe7813cf2cebfff29f95644bf Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Mon, 7 Sep 2026 00:22:34 +0300 Subject: [PATCH 13/16] Record EC-08 as reproduced, and why the first attempt missed it EC-08 was filed "not reproduced" against a pathological include mask that completed inside a 10 s budget. The entry names the reason it did not reproduce: the attempt used a matching filename, which is the one input class that cannot show it, because the engine stops at the first success. Against a non-matching name the same masks grow 38, 193, 758, 2699, 8855 ms for six through ten wildcards, with no ceiling. The status moves to open: the cited code is still there, emitting RegexOptions.Compiled with an infinite MatchTimeout. Scored Medium impact, Theoretical reach, on the same pattern as the BOM-less UTF-32 row. The blow-up needs both halves built on purpose - a mask of about ten or more wildcards separated by one character, and a filename carrying about twenty-four or more mostly consecutive repeats of that same character. At twelve wildcards every realistic filename measured answers in 0 to 5 ms; forty consecutive "a" takes over 20 s. The mask comes from the operator's own command line, so no untrusted path supplies it. The fix was written, measured and reverted, and is recorded with the numbers so it is not re-derived: NonBacktracking answers the same mask in 5 ms, constructs faster, and agreed with Compiled on all 18,000 comparisons measured. Three dead ends are recorded with it, including that "*" crossing directory separators is the intended subtree semantic and not a Windows-wildcard bug. Co-Authored-By: Claude Opus 5 --- docs/DEFECT-BACKLOG.md | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/docs/DEFECT-BACKLOG.md b/docs/DEFECT-BACKLOG.md index d780bf5..c197e15 100644 --- a/docs/DEFECT-BACKLOG.md +++ b/docs/DEFECT-BACKLOG.md @@ -3,9 +3,9 @@ Status of the thirty-five findings from the two independent reviews that preceded v3.11.0, plus what has been found since. -**Of the original thirty-five: 26 fixed, 7 open, 2 could not be reproduced.** +**Of the original thirty-five: 26 fixed, 8 open, 1 could not be reproduced.** Six further findings have been raised since v3.11.1, two of them already fixed. -**Eleven open in total.** +**Twelve open in total.** ## Why this file exists @@ -43,7 +43,7 @@ report. | EC-05 | A plan holding an unreadable file can never be applied | fixed | A hash is required only for `Convert` entries. | | EC-06 | A drive-root base path makes every plan unusable | fixed | Fixed 2026-09-04. **Shipped broken in v3.11.0 and v3.11.1.** | | EC-07 | The refusal advises the very encoding it cannot justify | fixed | `DescribeRefusal` offers both byte orders. | -| EC-08 | An include pattern can hang the scan indefinitely | *not reproduced* | A pathological mask against a matching filename completed inside a 10 s budget. No match timeout was added, so this is not *proven fixed*. | +| EC-08 | An include pattern can hang the scan indefinitely | **open** | Reproduced 2026-09-06, but only against inputs built for it. `CompilePatterns` still emits `RegexOptions.Compiled` with `Regex.InfiniteMatchTimeout`. The 2026-09-04 attempt used a *matching* filename, which stops at the first success and cannot show it. Scored below. | | EC-09 | `.bak` files are excluded, uncounted, and unreported | fixed | `TraversalCounters.FilesExcludedAsEcArtifact`. | | EC-10 | A scan failure is journaled as a refusal | fixed | A failed snapshot is recorded as `Error`, not `Refused`. | | EC-11 | `ApplyPlan` leaks its Ctrl+C handler onto a disposed token source | fixed | Both handler sites unsubscribe in a `finally`. | @@ -212,6 +212,7 @@ trips over is not noise. |---|---|---|---|---| | CX-06 | Entropy gate outranks a valid BOM | Medium | Occasional | EC reports the wrong encoding for a file that says what it is. The only open item that changes what EC tells you. | | — | Ambiguous BOM-less UTF-32 converts silently | **Critical** | **Theoretical** | Rewrites on an unproven byte order — the exact thing this release line exists to prevent. Needs every scalar to be a multiple of 0x100, so real text will not reach it. Scored high on impact and dismissed on reach, deliberately. | +| EC-08 | An include pattern can hang the scan indefinitely | Medium | **Theoretical** | Availability, not data: a scan no token can cancel, and if it hangs partway through a conversion the tree is left partly converted with no journal. Needs *both* halves built on purpose — a mask of ~10+ wildcards separated by one character, and a filename carrying ~24+ mostly consecutive repeats of that same character. Measured at twelve wildcards: every realistic name answered in 0–5 ms; forty consecutive `a` took >20 s. The mask comes from the operator's own command line, so there is no untrusted path. Fix measured and not taken: see below. | | — | CSV report does not neutralise leading formula characters | Medium | Rare | Needs an attacker-influenced filename and a reader who opens the report in a spreadsheet. | | EC-16 | Settings.xml written truncate-in-place | Low | Occasional | Loses preferences, not data, and reverts toward safer defaults. Already caused one smoke-test failure that looked like a product bug. | | EC-20 | Detection reads with looser file sharing | Low | Rare | Detect and validate only; nothing is written. Can describe bytes another process is changing. | @@ -225,9 +226,25 @@ trips over is not noise. Nothing here writes to a file nobody approved, which is why none of it blocked a release. +### EC-08: the fix that was measured and not taken + +`RegexOptions.NonBacktracking` in place of `Compiled` removes the blow-up +entirely, agrees with the current engine on every mask tested, and costs nothing +measurable beside per-file I/O. It was implemented with tests and then reverted +deliberately: the defect needs a mask *and* a filename both built for it, and +neither arrives from anywhere but the operator's own hands. + +Three dead ends, recorded so nobody walks them again. Testing a pathological +mask against a **matching** filename proves nothing, because the engine stops at +the first success — that is how this was first recorded "not reproduced". `*` +crossing directory separators is deliberate rather than a Windows-wildcard bug: +`src/*.cs` is meant to scope a subtree, pinned by +`PathAwarePatternTests.PathQualifiedPattern_MatchesOnlyTheIntendedSubtree`. And +`[^/]*` in place of `.*` does not reduce the backtracking, because a filename +contains no separator for it to bound. + ### Not reproduced | | Finding | Why it is not listed as open | |---|---|---| -| EC-08 | An include pattern can hang the scan indefinitely | A pathological mask completed inside a 10 s budget. No match timeout was added, so this is not *proven fixed* either. | | EC-19 | The double-BOM guard's reach depends on which object supplied the codec | An inspection-only finding that traced the wrong object. `ConvertFiles` re-resolves the codec by name through `Encoding.GetEncoding`, which carries a 3-byte preamble, so the detector's BOM-less instance never reaches the guard. Tested against a file beginning with two BOMs: both the automatic path and `-From utf-8` refuse with `MultipleLeadingByteOrderMarks`. | From d49319bcda329379e7a9f2430246fd89f6c40cd9 Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Mon, 7 Sep 2026 00:27:15 +0300 Subject: [PATCH 14/16] Track the three findings this review added, and name what it left untracked An independent review of 74d5b3d raised three defects this project had no record of, all now fixed with their own commits: a folder EC could not read leaving no trace a machine could see, "already in the target encoding" being a whole-file claim made from a 64 KiB sample, and a preview promising conversions that would fail while a plan recorded them as approved. The second and third are worth this file existing for. Both are cases where EC reported an outcome it had not checked, and both were reachable in ordinary use: the same corrupt file came back Error or Unchanged depending only on which target was typed, and -Plan exited 0 over a file -Apply then failed on, after approval and part-way through the batch. The section also names four findings the same review left open that are recorded nowhere else here, because a finding that is written down but not tracked is the thing this file was created to prevent. The first of them - a BOM-less UTF-16 file detected as UTF-32 and converted silently, which output verification cannot catch because both sides of the comparison use the same wrong codec - is the largest defect the review found and is still open. Co-Authored-By: Claude Opus 5 --- docs/DEFECT-BACKLOG.md | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/docs/DEFECT-BACKLOG.md b/docs/DEFECT-BACKLOG.md index c197e15..1e144f6 100644 --- a/docs/DEFECT-BACKLOG.md +++ b/docs/DEFECT-BACKLOG.md @@ -5,6 +5,7 @@ preceded v3.11.0, plus what has been found since. **Of the original thirty-five: 26 fixed, 8 open, 1 could not be reproduced.** Six further findings have been raised since v3.11.1, two of them already fixed. +Three more since v3.11.2, all three fixed. **Twelve open in total.** ## Why this file exists @@ -82,6 +83,40 @@ report. | A ticked file can be dropped from a source choice in silence | fixed | Each row in the review's refused list carries its resolved path. `TickedFiles()` filters out rows whose path is null and says nothing, so a file the user ticked is left refused with no message. This was live until EC-06 was fixed: with a drive-root base directory every row resolved to null, so choosing an encoding reported "Conversion cancelled. No files were modified." The trigger is gone; the silent drop is not. | | Force-closing during a run can throw on the way out | **open** | The second close request abandons a run deliberately, which is correct. But the worker may then marshal its next confirmation to a form that no longer exists, and the completion handler runs against disposed controls. An error dialog at exit rather than lost work — finished files are installed and the one in flight is untouched. Reasoned from the code, not reproduced: it needs precise timing. | +## Found after v3.11.2 + +Three findings from an independent review of 74d5b3d, run from the source rather +than from this file, and new to the project. All three are fixed; each has its +own commit carrying the measurement and the mutation result. + +| Finding | Status | Note | +|---|---|---| +| A folder EC could not read left no trace a machine could see | fixed | An unreadable *file* becomes a row with `ScanFailed` and drives exit 3. An unreadable *directory* produced a warning on stderr and nothing else: no row, no counter, exit 0 — and nothing whatever in the window, which passes no warning callback. Measured against a deny ACE: `-Validate -FailOnChanges` over a tree with one denied folder reported "1 file(s) processed" and exited 0, and a scan whose entire base directory was unreadable printed a header-only CSV and exited 0. A run that examined none of the tree could report success. `DirectoriesUnreadable` is now counted at both catch blocks, apart from the two exclusion counters, which record folders EC *chose* not to enter. The exit code is deliberately unchanged and the documentation now says what that means for a script. | +| "Already in the target encoding" was a whole-file claim made from a 64 KiB sample | fixed | Detection reads at most 64 KiB, and when the source codec matched the target nothing read further. A file clean for 64 KiB and invalid afterwards was reported `Unchanged`, and whether EC noticed depended only on which target was named: the same corrupt file was `Error` under `-Target utf-16` and `Unchanged` under `-Target utf-8`. `-Validate` always read the whole file; Convert never reached that check once it had decided it had nothing to do. Costs almost nothing, and not for the expected reason — Convert already reads every byte, because `CaptureSourceSnapshot` hashes the whole stream before anything is decided. Measured at 0.04–0.10 ms per MiB. | +| A preview promised conversions that would fail, and a plan recorded them as approved | fixed | `ApplyConversion` returned at the `whatIf` branch before the converter ran, so nothing decoded the file. `-Plan` sets `WhatIf`, so a plan recorded `Action=Convert` with no reason for a source that cannot be read, exited 0, and showed the reviewer nothing; the failure surfaced at `-Apply`, after approval and part-way through the batch. `FindStaleFiles` can prove the bytes have not changed since review and cannot prove they are readable, because that needs a decode nobody performed. The entry is now marked `Refuse`, not merely given an error, because the plan records the *action*. Decode only: a target that cannot represent the text still fails at conversion time, which reading the source cannot predict, and a test named for that case pins the limit. | + +### From the same review, and not tracked here + +Its fixed findings are in the git history under their own commits. Four of the +ones it left open are recorded nowhere in this file, and the first is the one +worth reading: + +- **A BOM-less UTF-16 file can be detected as UTF-32 and converted.** Silent, and + output verification cannot catch it, because both sides of the comparison use + the same wrong codec. It needs a file in which every other UTF-16 code unit is + a C0 control — one character per line with LF endings, say. Measured over + nineteen realistic file shapes: 41 of 44 detect correctly, and the three that + do not are the same degenerate shape. Scored Critical impact, low reach. +- ASCII text with 2.3% or more NUL bytes is labelled `utf-16`. Conversion is + refused by the ambiguity guard in every case constructed, so the wrong label + reaches `-DetectOnly` and `-Validate` only. +- Two hard links to one file are converted twice, once per name. Both runs + succeeded when tested, because `File.Replace` breaks the link; the `File.Move` + fallback would not. +- Detection accepts a truncated trailing sequence, because it decodes without + flushing, while conversion flushes and rejects it. `-DetectOnly` can therefore + bless a file conversion refuses. + ## Hashing: three optimisations measured and rejected Conversion looked as though it hashed the same bytes several times over. Three From 0f27399e69c106e4df4f7a92625c3a7c961a65af Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Mon, 7 Sep 2026 00:33:33 +0300 Subject: [PATCH 15/16] Record the other nine findings this review fixed The previous commit added three findings and said the rest were "in the git history under their own commits". True, and useless: this file is the index, so a fix it does not name is one you would have to already know about to find. That is the failure this file was created to prevent. EC-06 was recorded in a summary that got lost, reported as closed, shipped broken in two releases and rediscovered from scratch. Recording three of twelve fixes and pointing at the git log for the others reproduces the same shape at smaller scale. The nine now added are the alias comparison, the per-file catch filter, the name-excluded folder counter, the -Validate rejections without reasons, the reason code re-derived instead of read, the decode diagnostic's impossible offset, standard output encoded for the wrong reader, the stale parallelism default, and the undocumented lifetime of .bak - the last of these a finding raised in review and withdrawn, recorded because a rejected proposal is worth as much to the next reviewer as an accepted one. The section is now ordered by what a reader needs first: what EC did to files, then what it reported, then what it documented. The four findings the review left open and that nothing else here tracks keep their own subsection. Co-Authored-By: Claude Opus 5 --- docs/DEFECT-BACKLOG.md | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/docs/DEFECT-BACKLOG.md b/docs/DEFECT-BACKLOG.md index 1e144f6..6c73f1f 100644 --- a/docs/DEFECT-BACKLOG.md +++ b/docs/DEFECT-BACKLOG.md @@ -5,7 +5,7 @@ preceded v3.11.0, plus what has been found since. **Of the original thirty-five: 26 fixed, 8 open, 1 could not be reproduced.** Six further findings have been raised since v3.11.1, two of them already fixed. -Three more since v3.11.2, all three fixed. +Twelve more since v3.11.2, all twelve fixed. **Twelve open in total.** ## Why this file exists @@ -85,21 +85,30 @@ report. ## Found after v3.11.2 -Three findings from an independent review of 74d5b3d, run from the source rather -than from this file, and new to the project. All three are fixed; each has its -own commit carrying the measurement and the mutation result. +An independent review of 74d5b3d, run from the source rather than from this file. +Everything it found and fixed is below, one commit each, carrying that commit's +measurement and mutation result. Ordered by what a reader needs first: what EC +did to files, then what it reported, then what it documented. | Finding | Status | Note | |---|---|---| -| A folder EC could not read left no trace a machine could see | fixed | An unreadable *file* becomes a row with `ScanFailed` and drives exit 3. An unreadable *directory* produced a warning on stderr and nothing else: no row, no counter, exit 0 — and nothing whatever in the window, which passes no warning callback. Measured against a deny ACE: `-Validate -FailOnChanges` over a tree with one denied folder reported "1 file(s) processed" and exited 0, and a scan whose entire base directory was unreadable printed a header-only CSV and exited 0. A run that examined none of the tree could report success. `DirectoriesUnreadable` is now counted at both catch blocks, apart from the two exclusion counters, which record folders EC *chose* not to enter. The exit code is deliberately unchanged and the documentation now says what that means for a script. | +| "Already in the target encoding" was decided by label, not by codec | fixed | `Decide` compared the detected charset's `WebName` against whatever the caller typed, so every accepted alias for one code page failed the test. `-Target unicode`, `ucs-2` or `utf-16le` on a tree already in UTF-16LE decoded, re-encoded, verified and reinstalled every file to produce **identical bytes**, resetting every modification time and, with `-Backup`, leaving a `.bak` and an `.ecmeta.json` beside each. Under `-FailOnChanges` the same tree exits 0 for `-Target utf-16` and 2 for `-Target unicode`; on BOM-less UTF-16 the alias reaches the ambiguity guard and exits 5, so the spelling alone moved a clean run to a refusal. Now compares resolved code pages; a zero code page proves nothing and never matches. ASCII to UTF-8 was deliberately left a conversion, on reasoning the 64 KiB finding below then disproved — folding them together is now safe and is not yet done. | | "Already in the target encoding" was a whole-file claim made from a 64 KiB sample | fixed | Detection reads at most 64 KiB, and when the source codec matched the target nothing read further. A file clean for 64 KiB and invalid afterwards was reported `Unchanged`, and whether EC noticed depended only on which target was named: the same corrupt file was `Error` under `-Target utf-16` and `Unchanged` under `-Target utf-8`. `-Validate` always read the whole file; Convert never reached that check once it had decided it had nothing to do. Costs almost nothing, and not for the expected reason — Convert already reads every byte, because `CaptureSourceSnapshot` hashes the whole stream before anything is decided. Measured at 0.04–0.10 ms per MiB. | | A preview promised conversions that would fail, and a plan recorded them as approved | fixed | `ApplyConversion` returned at the `whatIf` branch before the converter ran, so nothing decoded the file. `-Plan` sets `WhatIf`, so a plan recorded `Action=Convert` with no reason for a source that cannot be read, exited 0, and showed the reviewer nothing; the failure surfaced at `-Apply`, after approval and part-way through the batch. `FindStaleFiles` can prove the bytes have not changed since review and cannot prove they are readable, because that needs a decode nobody performed. The entry is now marked `Refuse`, not merely given an error, because the plan records the *action*. Decode only: a target that cannot represent the text still fails at conversion time, which reading the source cannot predict, and a test named for that case pins the limit. | +| One file's failure could end the whole run | fixed | The per-item catch in `RunParallel` named four exception types. Anything else — a `SecurityException` from an ACL the enumerator did not surface, a regex timeout, a defect in EC itself — escaped `Parallel.ForEach` as an `AggregateException` and took every file the run had not reached with it. The CLI's outer catch names the same four, so it would have surfaced as a crash rather than exit 3. Now everything except cancellation and `OutOfMemoryException`, since carrying on after the latter would be pretending to process. `RunParallel` became internal so the isolation could be tested at all: no file can be made to throw the exceptions that mattered, which is exactly what made them dangerous. | +| A folder EC could not read left no trace a machine could see | fixed | An unreadable *file* becomes a row with `ScanFailed` and drives exit 3. An unreadable *directory* produced a warning on stderr and nothing else: no row, no counter, exit 0 — and nothing whatever in the window, which passes no warning callback. Measured against a deny ACE: `-Validate -FailOnChanges` over a tree with one denied folder reported "1 file(s) processed" and exited 0, and a scan whose entire base directory was unreadable printed a header-only CSV and exited 0. A run that examined none of the tree could report success. `DirectoriesUnreadable` is now counted at both catch blocks, apart from the two exclusion counters, which record folders EC *chose* not to enter. The exit code is deliberately unchanged and the documentation now says what that means for a script. | +| Folders skipped by name were counted nowhere | fixed | Twelve directory names are skipped deliberately and that is documented, but unlike attribute-excluded folders they incremented no counter. A scan of a tree whose only content sat under `build/` reported one file, zero exclusions and no warning — while `docs/CLI.md` promised that EC reports how many files each exclusion skipped. Counted separately from the attribute exclusions, whose message says "(hidden, system, or reparse point)" and would become untrue if the two were merged. What is scanned is unchanged: letting an explicit include reach into these folders was considered and declined, because both documents state they are skipped. | +| `-Validate` rejections could carry no reason at all | fixed | Four ways to return `Invalid`, two of them explained. A charset outside the allowed list and a file EC could not identify both arrived as a bare `Invalid` with an empty reason, though they are not the same situation: one means widen the list or convert the file, the other means EC could not tell what it is, which `-DetectOnly` already calls `UnknownEncoding`. `CharsetNotAllowed` is new; `UnknownEncoding` is reused deliberately, because two names for one condition depending on which mode ran would be its own defect. This was the only outcome in the product where the reader had to re-derive a reason the producer already knew. | +| A refusal's reason was re-derived instead of read from the decision | fixed | `ApplyConversion` worked the reason code out again from the four raw facts `Decide` had already reduced to a `SourceInterpretation`. The two copies were textually identical and their operands never changed between them, so they could not disagree — but nothing tied them together, and a fourth refusal reason added to the policy would have fallen through to `LegacySourceRequired` at the call site: a correct refusal carrying the wrong explanation, with nothing to fail. That shape had already needed one bolt-on `when` guard. Now `ConversionPolicy.ReasonCodeFor`, verified equivalent across all 256 reachable combinations of `Decide`'s inputs, with a test that fails if any refusal ever produces no reason. | +| A decode failure reported a position no file has | fixed | `DecoderFallbackException.Index` is relative to the decoder call, not the file, and goes negative when the bad sequence began in bytes carried over from the previous call. A UTF-8 file ending in a truncated three-byte sequence produced "offset -2 within the failing read chunk", a message naming a frame it did not describe. The offending bytes are reported instead, which mean the same thing wherever the failure happened. An absolute file offset would need the streaming loop restructured to keep each chunk's base position in scope. | +| Standard output was UTF-8 whatever the console was | fixed | After attaching to the parent console, both writers were rebuilt with `StreamWriter`'s default encoding. On the machine this was found on `Console.OutputEncoding` is `ibm437`, and the per-file CSV rendered "Grüße aus München" as "Gr├╝├ƒe aus M├╝nchen" — the tool producing in its own output the failure it exists to detect. A redirected stream stays UTF-8, matching the `-Report` file apart from its BOM; a console gets its own encoding, so characters it cannot represent become "?", which is visibly lossy rather than quietly wrong. No global console state is mutated. | +| The documented parallelism default was the old one | fixed | `DefaultMaxParallelism` was raised from `min(CPU, 4)` to `min(CPU, 8)` with the measurement recorded beside it, and both statements of it were left saying 4: the built-in help and `docs/CLI.md`, which are the two places someone tuning `-MaxParallelism` against a slow share would look. The cause was an unnamed literal, with no identity a document could be checked against; it is now `ScanEngine.MaxParallelismCap`. A test finds the one line in each document that states the default, extracts every run of digits from it, and asserts the set equals the cap, so a stale number cannot hide beside a fresh one. | +| The lifetime of `.bak` was undocumented | fixed | `.bak` is a fixed name holding the version the most recent run replaced, so converting the same file again replaces it and removes its sidecar. That is deliberate, and pinned by `BackupIntegrityTests.Backup_OverwritesAnyPreviousBackupFile` since the first commit of the test suite — but no document said so, and a user converting twice lost the original with nothing having warned them. Raised in review as a defect and **withdrawn**: refusing to overwrite a non-matching `.bak` breaks four existing tests and would block an ordinary "wrong target, convert again" run until the user deleted the backups by hand. See CX-02, whose fix accepted the replacement and removed the stale record instead. | ### From the same review, and not tracked here -Its fixed findings are in the git history under their own commits. Four of the -ones it left open are recorded nowhere in this file, and the first is the one -worth reading: +Four findings the same review left open are recorded nowhere else in this file. +The first is the one worth reading: - **A BOM-less UTF-16 file can be detected as UTF-32 and converted.** Silent, and output verification cannot catch it, because both sides of the comparison use From 876353b24dc3a131631e9aa951740816c4ec2915 Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Mon, 7 Sep 2026 00:48:21 +0300 Subject: [PATCH 16/16] Bump to v3.12.0 and write the release notes Assembly version and README heading; --version and the built-in help both report 3.12.0 from the Release build. Minor rather than patch. Exit codes move in both directions across four modes - an alias target drops -FailOnChanges from 2 to 0, a file invalid past 64 KiB and a plan over an undecodable source each move from 0 to 3 - and the CSV gains a reason code a script may not know. A reader told only "patch" would have been misled. No schema or semantics bump. These changes make EC refuse more and convert less, which the approved-decision ceiling already permits, so a plan written by an earlier build still means what it meant. Semantics stay at 6, the plan schema at 5, the journal schema at 4, and the notes say so rather than leaving a reader to work it out. The notes also carry what the verification does not cover. Unlike v3.11.2, this release cannot claim the corpus exemption: detection is untouched, no detector file changed, but ConversionPolicy did change, and the checklist asks for a corpus run on exactly that. None was made. The notes state it plainly instead of quoting the v3.11.0 figures as though they were evidence about this build. Co-Authored-By: Claude Opus 5 --- README.md | 2 +- docs/RELEASE-NOTES-v3.12.0.md | 204 ++++++++++++++++++ .../Properties/AssemblyInfo.cs | 4 +- 3 files changed, 207 insertions(+), 3 deletions(-) create mode 100644 docs/RELEASE-NOTES-v3.12.0.md diff --git a/README.md b/README.md index 413d3d6..99f047b 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ [![CI](https://github.com/amrali-eg/EncodingChecker/actions/workflows/ci.yml/badge.svg)](https://github.com/amrali-eg/EncodingChecker/actions/workflows/ci.yml) -# EncodingChecker v3.11.2 +# EncodingChecker v3.12.0 EncodingChecker is a Windows tool for finding, checking, and safely converting text-file encodings. Use the GUI for everyday work or the command line for repeatable jobs. diff --git a/docs/RELEASE-NOTES-v3.12.0.md b/docs/RELEASE-NOTES-v3.12.0.md new file mode 100644 index 0000000..e22ab36 --- /dev/null +++ b/docs/RELEASE-NOTES-v3.12.0.md @@ -0,0 +1,204 @@ +# EncodingChecker v3.12.0 + +Twelve findings from an independent review, run against the source rather than +against the backlog: a target's spelling could rewrite an entire tree, two +claims were made without reading the file, and three failures were reported to +nobody. + +## A file already in the target encoding is left alone + +`ConversionPolicy` compared the detected charset's label against whatever the +caller typed. "utf-16", "unicode", "ucs-2" and "utf-16le" all name code page +1200, so every accepted alias but one failed the test: `-Target unicode` on a +tree already in UTF-16LE decoded, re-encoded, verified and reinstalled every +file to produce **identical bytes**. + +Nothing was corrupted. What was lost was every modification time — and with +`-Backup`, a `.bak` and an `.ecmeta.json` appeared beside each unchanged file. +Under `-FailOnChanges` the same clean tree exits 0 for `-Target utf-16` and 2 +for `-Target unicode`; on BOM-less UTF-16 the alias reaches the ambiguity guard +and exits 5, so the spelling alone turned a clean run into a refusal. + +Identity is now the resolved code page. A zero code page means the label did not +resolve, so it proves nothing and never matches. + +## "Already in the target encoding" is a whole-file claim again + +Detection reads at most 64 KiB. When the source codec matched the target, +nothing read any further — so a file clean for 64 KiB and invalid afterwards was +reported `Unchanged`, and whether EC noticed depended only on which target was +named. The same corrupt file was `Error` under `-Target utf-16` and `Unchanged` +under `-Target utf-8`. `-Validate` always read the whole file; Convert never +reached that check once it had decided it had nothing to do. + +Reading the rest costs almost nothing, and not for the reason expected: Convert +already reads every byte, because the source snapshot hashes the whole stream +before anything is decided. Measured at 0.04-0.10 ms per MiB. + +## A preview no longer promises a conversion that would fail + +`-Plan` sets `WhatIf`, and conversion returned at the `whatIf` branch before the +converter ran, so nothing decoded the file. A plan therefore recorded +`Action=Convert`, with no reason, for a source that cannot be read; the run +exited 0 and showed the reviewer nothing. The failure surfaced at `-Apply`, +after approval and part-way through the batch. + +A staleness check cannot substitute. It proves the bytes have not changed since +review and cannot prove they are readable, because that needs a decode nobody +performed. The entry is now marked `Refuse` rather than merely given an error, +because a plan records the *action*. + +Decode only. A target that cannot represent the text still fails at conversion +time, which reading the source cannot predict, and a test named for that case +pins the limit. + +## One file's failure is one row, whatever it threw + +The per-item catch named four exception types. Anything else — a +`SecurityException` from an ACL the enumerator did not surface, a regex timeout, +a defect in EC itself — escaped `Parallel.ForEach` as an `AggregateException` +and took every file the run had not yet reached with it. The CLI's outer catch +names the same four, so it would have surfaced as a crash rather than exit 3. + +It now catches everything except cancellation and `OutOfMemoryException`, since +carrying on after the latter would be pretending to process rather than +processing. + +## A folder EC could not read leaves a trace + +An unreadable *file* becomes a row with `ScanFailed` and drives exit 3. An +unreadable *directory* produced a warning on stderr and nothing else: no row, no +counter, exit 0 — and nothing whatever in the window, which passes no warning +callback. + +Measured against a deny ACE: `-Validate -FailOnChanges` over a tree with one +denied folder reported "1 file(s) processed" and exited 0, and a scan whose +entire base directory was unreadable printed a header-only CSV and exited 0. A +run that examined none of the tree could report success. + +Unreadable directories are now counted, separately from the two exclusion +counters, which record folders EC *chose* not to enter. The exit code is +deliberately unchanged, and the documentation now says what that means for a +script. + +## Folders skipped by name are counted + +Twelve directory names are skipped deliberately and that is documented, but +unlike attribute-excluded folders they incremented no counter. A scan of a tree +whose only content sat under `build/` reported one file, zero exclusions and no +warning, while `docs/CLI.md` promised that EC reports how many files each +exclusion skipped. + +They are counted apart from the attribute exclusions, whose message says +"(hidden, system, or reparse point)" and would become untrue if the two were +merged. What is scanned is unchanged. + +## Every `-Validate` rejection carries a reason + +There were four ways to return `Invalid` and two of them were explained. A +charset outside the allowed list and a file EC could not identify both arrived +as a bare `Invalid` with an empty reason, though they are not the same +situation: one means widen the list or convert the file, the other means EC +could not tell what the file is. + +`CharsetNotAllowed` is new. `UnknownEncoding` is reused deliberately, because +two names for one condition depending on which mode ran would be its own defect. +This was the only outcome left in the product where the reader had to re-derive +a reason the producer already knew. + +## A refusal's reason comes from the decision that made it + +The conversion path worked the reason code out again from the four raw facts the +policy had already reduced to a single interpretation. The two copies were +textually identical and their operands never changed between them, so they could +not disagree — but nothing tied them together, and a fourth refusal reason added +to the policy would have fallen through to `LegacySourceRequired` at the call +site: a correct refusal carrying the wrong explanation, with nothing to fail. + +It is now `ConversionPolicy.ReasonCodeFor`, verified equivalent across all 256 +reachable combinations of the policy's inputs, with a test that fails if any +refusal ever produces no reason. + +## A decode failure names the bytes, not a position no file has + +`DecoderFallbackException.Index` is relative to the decoder call, not to the +file, and goes negative when the bad sequence began in bytes carried over from +the previous call. A UTF-8 file ending in a truncated three-byte sequence +produced "offset -2 within the failing read chunk" — a message naming a frame it +did not describe. + +The offending bytes are reported instead, which mean the same thing wherever the +failure happened. + +## Standard output is encoded for whoever is going to read it + +After attaching to the parent console, both writers were rebuilt with +`StreamWriter`'s default encoding, which is UTF-8 whatever the console is. On +the machine this was found on, `Console.OutputEncoding` is `ibm437`, and the +per-file CSV rendered "Grüße aus München" as "Gr├╝├ƒe aus M├╝nchen" — the tool +producing in its own output the failure it exists to detect. + +A redirected stream stays UTF-8, matching the `-Report` file apart from its BOM. +A console gets its own encoding, so characters it cannot represent become "?", +which is visibly lossy rather than quietly wrong. No global console state is +mutated. + +## The documented parallelism default is the one the code uses + +v3.11.2 raised the cap from `min(CPU, 4)` to `min(CPU, 8)` and left both +statements of it saying 4: the built-in help and `docs/CLI.md`, which are the +two places someone tuning `-MaxParallelism` against a slow share would look. The +cause was an unnamed literal, with no identity a document could be checked +against; it is now `ScanEngine.MaxParallelismCap`. A test finds the one line in +each document that states the default, extracts every run of digits from it, and +asserts the set equals the cap, so a stale number cannot hide beside a fresh one. + +## Documentation + +`.bak` is a fixed name holding the version the most recent run replaced, +so converting the same file twice replaces it. That is deliberate and has been +pinned by a test since the suite's first commit, but no document said so, and a +user converting twice lost the original with nothing having warned them. +[`docs/SAFETY.md`](SAFETY.md) now says it. + +[`docs/DEFECT-BACKLOG.md`](DEFECT-BACKLOG.md) records every finding of this +review: the twelve fixed here, one withdrawn once its fix was shown to break +four existing tests, and four left open — including a BOM-less UTF-16 file that +can be detected as UTF-32 and converted silently, which output verification +cannot catch because both sides of the comparison use the same wrong codec. It +also records EC-08 — an include pattern that can hang a scan — as reproduced +against inputs built for it, and why the first attempt to reproduce it missed. + +## Compatibility + +Conversion semantics stay at **6**, the plan schema at **5**, the journal schema +at **4**. A plan written by an earlier build still means what it meant: these +changes make EC refuse more and convert less, which the approved-decision +ceiling already permits. + +Exit codes and report contents do change, in both directions: + +| Situation | Before | After | +|---|---|---| +| `-FailOnChanges` on a tree already in an alias of the target | 2 | 0 | +| A file valid for 64 KiB and invalid afterwards | `Unchanged`, 0 | `Error`, 3 | +| `-Plan` over a source that cannot be decoded | `Convert`, 0 | `Refuse`, 3 | +| An unexpected exception during a scan | run-ending crash | one row, 3 | +| A `-Validate` row outside the allowed list | empty reason | `CharsetNotAllowed` | + +Coverage output gains two lines, for folders skipped by name and folders that +could not be read. Standard output written to a console now uses that console's +encoding rather than UTF-8; redirected output is unchanged. + +## Verification + +- 727 tests pass, none skipped; release build with no warnings +- The nine-phase GUI smoke suite gates the release, as it has since v3.11.2 +- Every fix was mutation-checked: the change reverted, the intended test + required to fail, the file restored byte-identical and confirmed by hash +- **No four-corpus audit was run, and this release is one that asks for one.** + The checklist requires a corpus run for a release changing detection or + conversion policy. Detection is untouched — no detector file changed — but + `ConversionPolicy` is not, so the exemption v3.11.2 claimed is unavailable + here. What supports this release is the unit suite, the GUI suite, the + detector parity check, and a mutation check on each fix. diff --git a/sources/EncodingChecker/Properties/AssemblyInfo.cs b/sources/EncodingChecker/Properties/AssemblyInfo.cs index 26c7f1a..a36b31e 100644 --- a/sources/EncodingChecker/Properties/AssemblyInfo.cs +++ b/sources/EncodingChecker/Properties/AssemblyInfo.cs @@ -41,5 +41,5 @@ // You can specify all the values, or you can default the Build and Revision Numbers // by using the '*' as shown below: // [assembly: AssemblyVersion("1.0.*")] -[assembly: AssemblyVersion("3.11.2.0")] -[assembly: AssemblyFileVersion("3.11.2.0")] +[assembly: AssemblyVersion("3.12.0.0")] +[assembly: AssemblyFileVersion("3.12.0.0")]