From b0de9376d780d148ee3e51156b8efde4fa00d8c0 Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Fri, 4 Sep 2026 09:18:21 +0300 Subject: [PATCH 01/11] Apply a plan rooted at a drive, and stop capping parallelism at 4 Two unrelated changes, both small. ResolvePath built its containment prefix by appending a separator to the plan's root. TrimEndingDirectorySeparator leaves a drive or share root alone, so the prefix became "C:\\", which no resolved path can start with. Every file in a plan rooted at a drive was reported as resolving outside the plan's own directory and the run was refused whole - blaming the file paths rather than the root, which is the wrong place to look. The prefix now only gains a separator when it lacks one, and the match additionally requires the path to be longer than the prefix so the root directory itself is still not treated as a file. Widening the prefix must not widen what a plan will touch: the tests keep the escaping and empty-path cases rejected, and those four kept passing while the two drive-root cases failed against the old code. Verified end to end on a virtual drive as well as in unit tests: -Plan then -Apply with -BasePath X:\ now converts, where it previously refused. DefaultMaxParallelism was Math.Min(ProcessorCount, 4). Conversion is bound by per-file I/O latency, not CPU, so the cap bit well before core count did. Measured over 2,000 files: without backups 4,207 ms at 4 against 2,511 ms at 8; with backups 10,516 against 7,305. Past 8 the curve flattens and backup runs stop improving at all, so 8 rather than something larger. Re-measured against this build: 2,521 ms and 6,885 ms. ProcessorCount still binds first on small machines, and -MaxParallelism still overrides. 645 tests pass, none skipped, build warning-free. Co-Authored-By: Claude Opus 5 --- .../ConversionPlanTests.cs | 59 +++++++++++++++++++ sources/EncodingChecker/ConversionPlan.cs | 14 ++++- sources/EncodingChecker/ScanEngine.cs | 6 +- 3 files changed, 75 insertions(+), 4 deletions(-) diff --git a/sources/EncodingChecker.Tests/ConversionPlanTests.cs b/sources/EncodingChecker.Tests/ConversionPlanTests.cs index 354800b..9fc56b1 100644 --- a/sources/EncodingChecker.Tests/ConversionPlanTests.cs +++ b/sources/EncodingChecker.Tests/ConversionPlanTests.cs @@ -139,6 +139,65 @@ public void ThePlanKeepsNonAsciiNamesAndApostrophesReadable() Assert.DoesNotContain("\\u", json, StringComparison.Ordinal); } + private static ConversionPlan PlanRootedAt(string baseDirectory, string relativePath) => + new() + { + CreatedUtc = "2026-01-01T00:00:00.0000000Z", + EcVersion = "test", + BaseDirectory = baseDirectory, + TargetEncoding = "utf-8", + TargetHasBom = false, + BackupEnabled = false, + Files = + [ + new PlannedFile + { + RelativePath = relativePath, + Size = 1, + Sha256 = new string('0', 64), + Action = PlannedAction.Convert, + SourceEncoding = "windows-1252", + SourceCodePage = 1252, + SourceHasBom = false, + SourceWasSpecified = true, + SourceInterpretation = SourceInterpretation.ExplicitSource, + }, + ], + }; + + [Theory] + [InlineData(@"C:\", @"file.txt", @"C:\file.txt")] + [InlineData(@"C:\", @"sub\file.txt", @"C:\sub\file.txt")] + [InlineData(@"C:\data", @"file.txt", @"C:\data\file.txt")] + [InlineData(@"C:\data\", @"file.txt", @"C:\data\file.txt")] + public void APlanResolvesItsFilesWhateverItsRootLooksLike( + string baseDirectory, string relativePath, string expected) + { + // A drive root is left alone by TrimEndingDirectorySeparator, so the old + // containment prefix became "C:\\" and matched nothing. Every file in a plan + // rooted at a drive was then reported as resolving outside the plan's own + // directory, and the whole run was refused - with a message blaming the file + // paths rather than the root. + ConversionPlan plan = PlanRootedAt(baseDirectory, relativePath); + + Assert.Equal(expected, plan.ResolvePath(plan.Files[0])); + } + + [Theory] + [InlineData(@"C:\data", @"..\outside.txt")] + [InlineData(@"C:\data", @"..\data-sibling\file.txt")] + [InlineData(@"C:\data", @"")] + [InlineData(@"C:\", @"")] + public void APathThatIsNotAFileBeneathTheRootIsStillRejected( + string baseDirectory, string relativePath) + { + // Widening the prefix must not widen what the plan will touch. The empty cases + // resolve to the root directory itself, which is not a file in the plan. + ConversionPlan plan = PlanRootedAt(baseDirectory, relativePath); + + Assert.Null(plan.ResolvePath(plan.Files[0])); + } + [Fact] public void PlanningWritesNothing() { diff --git a/sources/EncodingChecker/ConversionPlan.cs b/sources/EncodingChecker/ConversionPlan.cs index 4de3448..adb119d 100644 --- a/sources/EncodingChecker/ConversionPlan.cs +++ b/sources/EncodingChecker/ConversionPlan.cs @@ -390,9 +390,17 @@ internal static ConversionPlan FromEntries( return null; } - return full.StartsWith( - root + Path.DirectorySeparatorChar, - StringComparison.OrdinalIgnoreCase) + // A drive or share root already ends with the separator and is left alone by + // TrimEndingDirectorySeparator, so appending another produced "C:\\" - a prefix + // no resolved path can start with, which reported every file in such a plan as + // escaping its own directory. + string prefix = root.EndsWith(Path.DirectorySeparatorChar) + ? root + : root + Path.DirectorySeparatorChar; + + // Longer than the prefix, so the root directory itself is still not a file. + return full.Length > prefix.Length && + full.StartsWith(prefix, StringComparison.OrdinalIgnoreCase) ? full : null; } diff --git a/sources/EncodingChecker/ScanEngine.cs b/sources/EncodingChecker/ScanEngine.cs index 8aa80c2..ded41e7 100644 --- a/sources/EncodingChecker/ScanEngine.cs +++ b/sources/EncodingChecker/ScanEngine.cs @@ -96,8 +96,12 @@ internal sealed class ScanDirectoryOptions /// internal static class ScanEngine { + // Conversion is bound by per-file I/O latency rather than CPU, so the cap is set + // 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. internal static readonly int DefaultMaxParallelism = - Math.Min(Environment.ProcessorCount, 4); + Math.Min(Environment.ProcessorCount, 8); /// Charset label used when the source encoding cannot be established. internal const string UnknownCharset = "(Unknown)"; From 2789b4a0a7cdc0eb90e9475d5edf5902532df37e Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Fri, 4 Sep 2026 09:33:49 +0300 Subject: [PATCH 02/11] Track the thirty-five findings where they can be found again The two pre-v3.11.0 reviews produced a generated HTML report and a chat transcript. Neither reached the repository, so the only way to answer "what is still open?" was to trust a summary. The summary was wrong. EC-06 - a drive-root base path making every plan unusable - was recorded as Confirmed before v3.11.0, reported as closed, and shipped broken in both v3.11.0 and v3.11.1. It was rediscovered from scratch during an unrelated review and filed as a new finding before anyone recognised it as a known one. That is what an untracked list costs. Every status here was re-derived from the source rather than carried over. Twenty-five are fixed, nine are open, and one could not be reproduced - which the table says plainly instead of calling it fixed, because a failed reproduction is not evidence of a repair. The nine that remain are recorded with what a reader would notice first: a valid BOM losing to the entropy gate, settings written truncate-in-place, a detection read that permits concurrent writes, a redundant decode per pass, then five contract and clarity issues. None writes to a file nobody approved, which is why none of them blocked a release. Co-Authored-By: Claude Opus 5 --- docs/DEFECT-BACKLOG.md | 96 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 docs/DEFECT-BACKLOG.md diff --git a/docs/DEFECT-BACKLOG.md b/docs/DEFECT-BACKLOG.md new file mode 100644 index 0000000..b9a2254 --- /dev/null +++ b/docs/DEFECT-BACKLOG.md @@ -0,0 +1,96 @@ +# Defect backlog + +Status of the thirty-five findings from the two independent reviews that +preceded v3.11.0, plus what has been found since. + +**25 fixed, 9 open, 1 could not be reproduced.** + +## Why this file exists + +The original review lived in a generated HTML report and in a chat transcript. +Neither survived into the repository, so weeks later the only way to answer +"what is still open?" was to trust a summary — and the summary was wrong. + +EC-06 is the demonstration. It was found before v3.11.0, recorded as Medium and +Confirmed, reported as closed, and shipped broken in **both** v3.11.0 and +v3.11.1. It was rediscovered from scratch during an unrelated review, filed as a +new finding, and only then recognised as a known one. + +A finding that is written down but not tracked is a finding that gets found +twice and fixed late. Hence this file. + +## How each status was reached + +`fixed` means the code that caused it is demonstrably gone — a named +replacement, a test that pins the behaviour, or a check re-run against the +current build. `open` means the cited code is still present and was read again +on 2026-09-04. `not reproduced` means an attempt to trigger it failed, which is +weaker than either. + +Statuses were re-derived from the source, not carried over from the earlier +report. + +## The thirty-five + +| ID | Finding | Status | Evidence | +|---|---|---|---| +| EC-01 | Applying a plan converts a file the plan refused | fixed | `PlannedFile.HasReliableUnicodeDetection` carries the flag the plan boundary was dropping. | +| EC-02 | `-Validate` marks an unprovable file valid; `-Target` refuses it | fixed | v3.11.0 reports unprovable BOM-less UTF-16 as `Invalid`. | +| EC-03 | The GUI never shows the advisory v3.10.1 added | fixed | GUI smoke phase H asserts on the rendered advisory text. | +| EC-04 | `-Plan` exits 0 when files failed the scan | fixed | The plan branch returns 3 before considering 2. | +| 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-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`. | +| EC-12 | The GUI status line counts skipped files as unchanged | fixed | Pinned by a test naming EC-12. | +| EC-13 | The plan's explicit-source field can name one encoding for a run that used several | fixed | `DescribeSourceChoice` reports per-file choices. | +| EC-14 | `OutputTextSha256` is a copy of `SourceTextSha256` | fixed | The record takes the digest verification computed, and throws if absent. | +| EC-15 | The five conversion-semantics booleans are written everywhere and read nowhere | **open** | Only `SemanticsVersion` is enforced on load. | +| EC-16 | Settings.xml is written with truncate-in-place | **open** | `MainForm.Settings.cs:100` still opens `FileMode.Create` and serialises into it. | +| EC-17 | The text-validation comment contradicts its code | **open** | Control characters are penalised, not ignored. Behaviour is right, comment is wrong — and the file must stay byte-identical across three repos, so the fix is a synchronised change. | +| EC-18 | Ambiguity is recomputed on every pass over a BOM-less UTF-16 file | **open** | The or-expression short-circuits only when the flag is already true. | +| EC-19 | The double-BOM guard's reach depends on which object supplied the codec | **open** | `HasMultipleLeadingPreambles` returns false for an empty preamble. | +| EC-20 | `DetectFromFile` opens with looser sharing than every other read path | **open** | Still permits concurrent writes and deletes. | +| EC-21 | Three save-dialog instances are never disposed | fixed | All three use `using var`. | +| EC-22 | Plan serialisation and deserialisation use different options objects | **open** | `ConversionPlan.Load` still deserialises without options. | +| EC-23 | `ApplyPlan` dereferences `ResolvePath` with a null-forgiving operator | **open** | `Program.CliExecution.cs:90`. EC-06 was what happens when that invariant breaks. | +| CX-01 | An empty option value is silently ignored | fixed | Blank values are rejected with exit 1. | +| CX-02 | A failed second conversion destroys the first backup | fixed | `RemoveBeforeBackupReplacement` runs before the backup is replaced. | +| CX-03 | `-Apply` follows a plan root replaced by a junction | fixed | `HasReparsePointInPath` checks the whole path. | +| CX-05 | The journal cannot represent a post-install failure | fixed | `ConvertedWithWarning` and `InstallationUnknown` added. | +| CX-06 | The entropy gate outranks a valid BOM | **open** | `TextEncoding.cs:175` returns null before `UnicodeDetector.DetectFromBuffer` at line 182 reads the BOM. | +| CX-07 | Older plans, journals and reports are ordinary scan candidates | fixed | Reserved suffixes are excluded and rejected as output paths. | +| CX-08 | Documentation and validation disagree about `-DetectOnly` | fixed | Conflicting option combinations are rejected. | +| CX-09 | Cancelling a partly completed GUI run produces no journal | fixed | GUI smoke phase I covers it. | +| CX-10 | Plan summaries say "detection bypassed" when detection still ran | fixed | Now "chosen by you; detection still ran and is recorded". | +| CX-11 | GUI startup can fail if the settings directory cannot be created | fixed | `GetSettingsFileName()` moved inside the `try`. | +| CX-12 | A saved window position is not validated against current monitors | fixed | `WindowPosition.IsReachable` tests the title bar against attached monitors. | +| CX-13 | Detector parity is not a pull-request check or a release gate | fixed | Parity runs on pull requests, and the release workflow declares `needs: parity`. | + +## Found after v3.11.1 + +| Finding | Status | Note | +|---|---|---| +| Ambiguous BOM-less UTF-32 converts silently | **open** | The ambiguity guard covers only code pages 1200 and 1201, so the UTF-32 detector's prefer-little-endian wins with no refusal. Demonstrated end to end; reaching it needs every scalar to be a multiple of 0x100, so real text is unlikely to trigger it. | +| CSV report does not neutralise leading formula characters | **open** | A filename beginning with an equals, plus, minus or at sign becomes a live formula in a spreadsheet. | +| Conversion parallelism was capped at 4 | fixed | Raised to 8 on 2026-09-04; measured 1.5–1.7x faster. | + +## The nine that are open + +None writes to a file nobody approved, which is why none blocked a release. In +rough order of what a user could notice: + +1. **CX-06** — a valid BOM loses to the entropy gate. The one open finding that + changes what EC reports about a file. +2. **EC-16** — Settings.xml truncate-in-place. Already caused one smoke-test + failure that looked like a product bug. +3. **EC-20** — detection reads with sharing that permits concurrent writes. +4. **EC-18** — a redundant full-file decode per pass over BOM-less UTF-16. +5. **EC-15**, **EC-22**, **EC-23**, **EC-19**, **EC-17** — contract and clarity + issues, each a latent trap rather than a live defect. + +EC-08 sits outside that list because it could not be reproduced, and the absence +of a reproduction is not evidence of a fix. From 610dcf89bed3a968db8a1c6f3dc5f6bcef953583 Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Fri, 4 Sep 2026 09:51:18 +0300 Subject: [PATCH 03/11] Record what the deeper GUI pass found The orchestrator, the window's conversion path and the review dialog had a lighter read than the conversion core during the earlier review. Reading them properly turned up two things, neither of them severe. A ticked file can be dropped from a source choice in silence. The review's refused list carries each row's resolved path, and the ticked-file collector filters out rows whose path is null without saying so. That was live until today: with a drive-root base directory every row resolved to null, so choosing an encoding and clicking the button reported "Conversion cancelled. No files were modified." The user's choice was discarded and the message blamed a cancellation nobody made. That is EC-06 reaching the window, which neither the original finding nor this review had connected. The trigger is fixed; the silent drop is the part worth keeping written down. Force-closing during a run can throw on the way out. Abandoning the run on a second close is deliberate and correct, but the worker may then marshal a confirmation to a form that no longer exists. An error dialog at exit, not lost work. Reasoned from the code rather than reproduced, and labelled that way. The pass also settled several suspicions as unfounded, which is why they are not listed: an interrupted run cannot mark a written file as not attempted, the source-choice loop cannot spin, and the plan binding cannot throw on duplicate paths. Each was checked rather than assumed. Counts corrected: nine of the original thirty-five remain open, five findings have been raised since v3.11.1 with one already fixed, thirteen open in total. Co-Authored-By: Claude Opus 5 --- docs/DEFECT-BACKLOG.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/DEFECT-BACKLOG.md b/docs/DEFECT-BACKLOG.md index b9a2254..a3c5cc5 100644 --- a/docs/DEFECT-BACKLOG.md +++ b/docs/DEFECT-BACKLOG.md @@ -3,7 +3,9 @@ Status of the thirty-five findings from the two independent reviews that preceded v3.11.0, plus what has been found since. -**25 fixed, 9 open, 1 could not be reproduced.** +**Of the original thirty-five: 25 fixed, 9 open, 1 could not be reproduced.** +Five further findings have been raised since v3.11.1, one of them already fixed. +**Thirteen open in total.** ## Why this file exists @@ -77,8 +79,10 @@ report. | Ambiguous BOM-less UTF-32 converts silently | **open** | The ambiguity guard covers only code pages 1200 and 1201, so the UTF-32 detector's prefer-little-endian wins with no refusal. Demonstrated end to end; reaching it needs every scalar to be a multiple of 0x100, so real text is unlikely to trigger it. | | CSV report does not neutralise leading formula characters | **open** | A filename beginning with an equals, plus, minus or at sign becomes a live formula in a spreadsheet. | | Conversion parallelism was capped at 4 | fixed | Raised to 8 on 2026-09-04; measured 1.5–1.7x faster. | +| A ticked file can be dropped from a source choice in silence | **open** | 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. | -## The nine that are open +## The nine open from the original thirty-five None writes to a file nobody approved, which is why none blocked a release. In rough order of what a user could notice: From f98fff615585f8d143832a0416ed87b299c13937 Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Fri, 4 Sep 2026 10:23:41 +0300 Subject: [PATCH 04/11] Record why the hashing optimisations were rejected Conversion appeared to hash the same bytes several times over. Three variants were built on throwaway branches and measured interleaved against one baseline, because an earlier non-interleaved comparison had drifted enough to change the answer. Only one was faster, and it was the one that costs the most: digesting the backup while copying saves 15% by never reading the .bak back, which is the only thing proving the restore point on disk is intact. The headline result is the algorithm swap. XxHash128 is 6.8 times faster than SHA-256 in isolation and made EC very slightly slower, because at eight-way parallelism the hashing hides behind the I/O it accompanies. SHA-256 is also the fastest algorithm available here - hardware acceleration puts it ahead of SHA-1, MD5 and SHA-512 - so the lighter cryptographic options are slower as well as weaker. A test that verifies a recorded hash independently failed under the swap, which is the cost stated as an assertion rather than an opinion. Also recorded: LEN splits its hashing by whether the value is durable, using XxHash3 for a content digest it discards and SHA-256 for what it writes down, and compares backup hashes with FixedTimeEquals where EC compares hex strings with OrdinalIgnoreCase. Neither is wrong for accidental corruption. The drift is the finding, and nothing checks it the way detector parity checks the shared detector. Co-Authored-By: Claude Opus 5 --- docs/DEFECT-BACKLOG.md | 54 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/docs/DEFECT-BACKLOG.md b/docs/DEFECT-BACKLOG.md index a3c5cc5..11c3130 100644 --- a/docs/DEFECT-BACKLOG.md +++ b/docs/DEFECT-BACKLOG.md @@ -82,6 +82,60 @@ report. | A ticked file can be dropped from a source choice in silence | **open** | 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. | +## Hashing: three optimisations measured and rejected + +Conversion looked as though it hashed the same bytes several times over. Three +variants were built on throwaway branches and measured against the same +baseline, interleaved to cancel machine drift (292 MiB, 60 large files, backup +and journal enabled). + +| Variant | Median | vs baseline | What it costs | +|---|---|---|---| +| Baseline | 1030 ms | — | — | +| Digest the backup while copying | 872 ms | −15.3% | The `.bak` is no longer read back, so nothing proves the restore point on disk is intact. | +| Hash source and output while streaming | −3.5% (own batch) | −3.5% | Two independent measurements become values derived from what EC intended to write. | +| XxHash128 in place of SHA-256 | 1078 ms | **+4.7%, slower** | Recorded hashes stop being verifiable with `Get-FileHash`, and lose collision resistance. | + +**The reads are not redundant.** Each is an independent measurement: the source +re-read proves the file still matches what was approved, the backup re-read +proves the restore point is real, the output re-read proves what landed on disk. +Removing them is the same defect class as EC-14, which this project fixed +deliberately. + +**Hashing is not the bottleneck.** In isolation XxHash128 runs at 16,447 MiB/s +against SHA-256's 2,429 — 6.8x — yet replacing it made no difference at all, +because at eight-way parallelism the hashing hides behind the I/O it accompanies. +SHA-256 is also the fastest algorithm available here: hardware acceleration puts +it ahead of SHA-1 (981 MiB/s), MD5 (754) and SHA-512 (805), so every "lighter" +cryptographic option is slower as well as weaker. + +**What this means for future work.** Conversion is bound by cold reads, not by +CPU. The only variant that helped removed a read of a file that had just been +flushed to disk. Optimise reads, and treat the hashes as the verifications they +are. + +## Hash handling differs from LineEndingNormalizer + +LEN uses two algorithms, split by whether the value is durable: SHA-256 for the +raw source bytes and the backup check, XxHash3 for the normalised-content digest +that lives in a private record and is discarded after the run. + +EC uses SHA-256 for both, and persists its content digests as `SourceTextSha256` +and `OutputTextSha256`. That is defensible — EC-14 exists precisely to keep those +two independent — but the two tools now justify the same safety claim by +different means, and nothing checks that they agree: + +| | EC | LEN | +|---|---|---| +| Raw file / backup hash | SHA-256 | SHA-256 | +| Content digest | SHA-256, persisted | XxHash3, discarded | +| Backup comparison | `string.Equals(..., OrdinalIgnoreCase)` on hex | `CryptographicOperations.FixedTimeEquals` on bytes | + +Neither comparison is wrong for an accidental-corruption model. The point is the +drift: the detector-parity job exists to stop exactly this happening to the +shared detector, and nothing plays that role for the safety machinery around it. +**Open** — decide whether the two should converge, and on which. + ## The nine open from the original thirty-five None writes to a file nobody approved, which is why none blocked a release. In From 79ea91e06e86e7bee8bec243ee109c4757f8045e Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Fri, 4 Sep 2026 10:26:50 +0300 Subject: [PATCH 05/11] Answer the reviewer who arrives with this idea again The rejected optimisations look obviously right from the source: the same bytes are read up to seven times per converted file. Recording only the verdict invites someone to re-derive the whole experiment, so the section now states the conditions the numbers depend on and why they favour leaving it alone. Three points a future reviewer needs and would otherwise have to rediscover. The ceiling is 15% and it is the variant that costs the restore-point proof. The measurements are conditional on a fast local disk, a warm cache and eight-way parallelism, and changing those mostly raises the value of the checks rather than the value of removing them. And the safety argument does not rest on the measurement at all - the re-reads would still be the only proof that the file matches what was approved and that the backup exists, however fast a future machine made the alternative. The section ends by naming the honest target if throughput ever does matter: the backup read taken immediately after its own flush to disk, made cheaper rather than deleted. Co-Authored-By: Claude Opus 5 --- docs/DEFECT-BACKLOG.md | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/docs/DEFECT-BACKLOG.md b/docs/DEFECT-BACKLOG.md index 11c3130..a6eb980 100644 --- a/docs/DEFECT-BACKLOG.md +++ b/docs/DEFECT-BACKLOG.md @@ -114,6 +114,35 @@ CPU. The only variant that helped removed a read of a file that had just been flushed to disk. Optimise reads, and treat the hashes as the verifications they are. +### If you are reading this because you want to try again + +This idea looks obviously right from the source: the same bytes are read up to +seven times per converted file, and one of the hashes is computed twice over +data already in memory. It reads like waste. It is not, and the window in which +it would pay is narrower than it appears. + +**The ceiling is 15%, and it is the expensive 15%.** Every variant was measured, +not estimated. The two that preserve safety bought 3.5% and nothing at all. The +one worth having costs the only check that proves the restore point on disk is +intact — on a tool whose entire proposition is that it can undo what it did. + +**These numbers are conditional, and the conditions favour the status quo.** They +were taken on a 24-core machine with a fast local disk, a warm cache, and +eight-way parallelism. Change those and the results move, but mostly in ways that +do not help: on cold or network storage the read-elimination wins grow, yet so +does the value of verifying what actually landed there. Only a single-worker run +on a slow CPU would make the hashing itself visible, and that is not how EC runs. + +**The safety argument does not depend on the measurement.** Even if a future +machine made these changes worth 40%, the source re-read would still be the only +thing proving the file matches what was approved, and the backup re-read the only +thing proving the restore point exists. Speed is not the reason to decline; it is +merely the reason not to have to argue about it. + +If you still want the throughput, the honest target is the read that costs most — +the `.bak` read immediately after its `Flush(flushToDisk: true)` — and the honest +approach is to make that read cheaper, not to delete it. + ## Hash handling differs from LineEndingNormalizer LEN uses two algorithms, split by whether the value is durable: SHA-256 for the From ccddaf6a6fb163b84cc156c7a9ae50a298881ed9 Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Fri, 4 Sep 2026 10:57:01 +0300 Subject: [PATCH 06/11] Refuse a source choice that cannot be applied, instead of dropping it The review's refused rows each carry the path resolved for them, and the ticked set was built by filtering out the ones that resolved to nothing. When every row resolved to nothing the set came out empty, the orchestrator found no file matching the scope, and the run reported "Conversion cancelled. No files were modified." - a cancellation the user never asked for, after they had ticked files and chosen an encoding. The drive-root defect was one way in and is fixed, but not the only one. The results list is cleared when a scan starts and never when the directory box changes, and that box takes typing, a recent entry, or a dragged folder. Scan one folder, point the box at another, and every row in the next review resolves outside the plan's root. The Proceed path already handles this: it runs the staleness check and reports files that resolve outside the directory. Only the source-choice path failed silently, because it re-plans before that check runs. The dialog now says which ticked files it cannot act on and what to do about it, and leaves itself open. Verified by reading rather than by driving. A smoke phase was attempted and abandoned; the diagnosis and what remains are recorded in the backlog, and the nine existing phases still pass. Co-Authored-By: Claude Opus 5 --- docs/DEFECT-BACKLOG.md | 20 +++++++- .../ConversionConfirmationForm.cs | 48 +++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/docs/DEFECT-BACKLOG.md b/docs/DEFECT-BACKLOG.md index a6eb980..767734d 100644 --- a/docs/DEFECT-BACKLOG.md +++ b/docs/DEFECT-BACKLOG.md @@ -79,7 +79,7 @@ report. | Ambiguous BOM-less UTF-32 converts silently | **open** | The ambiguity guard covers only code pages 1200 and 1201, so the UTF-32 detector's prefer-little-endian wins with no refusal. Demonstrated end to end; reaching it needs every scalar to be a multiple of 0x100, so real text is unlikely to trigger it. | | CSV report does not neutralise leading formula characters | **open** | A filename beginning with an equals, plus, minus or at sign becomes a live formula in a spreadsheet. | | Conversion parallelism was capped at 4 | fixed | Raised to 8 on 2026-09-04; measured 1.5–1.7x faster. | -| A ticked file can be dropped from a source choice in silence | **open** | 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. | +| 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. | ## Hashing: three optimisations measured and rejected @@ -165,6 +165,24 @@ drift: the detector-parity job exists to stop exactly this happening to the shared detector, and nothing plays that role for the safety machinery around it. **Open** — decide whether the two should converge, and on which. +## The source-choice refusal has no GUI coverage + +The review now refuses a source choice it cannot apply, instead of closing on an +emptied scope. A smoke phase to drive that sequence was attempted and abandoned. + +The setup reproduces correctly - scan one directory, retarget the window at +another, and the refused row appears labelled `..\scanned\french.txt`, which only +happens when the plan's root does not contain the file. What does not work is +driving the source-encoding combo in that dialog state: `SelectCombo` times out +waiting for the selection to take, while the identical call in phase C succeeds. +Activating the review first and ticking the row first were both tried; neither +changed it. The cause is not known. + +Worth recording because the diagnosis is most of the work, and because the fix +is currently verified by reading rather than by driving. **Open** - either finish +the phase, or cover the refusal with a unit test that constructs the form +directly, as `InteractiveControlsExposeStableAutomationIds` already does. + ## The nine open from the original thirty-five None writes to a file nobody approved, which is why none blocked a release. In diff --git a/sources/EncodingChecker/ConversionConfirmationForm.cs b/sources/EncodingChecker/ConversionConfirmationForm.cs index d5522f6..3e257be 100644 --- a/sources/EncodingChecker/ConversionConfirmationForm.cs +++ b/sources/EncodingChecker/ConversionConfirmationForm.cs @@ -23,6 +23,20 @@ internal sealed class ConversionConfirmationForm : Form private readonly ConversionPlan _plan; private readonly ComboBox _sourceChoice = new() { Name = "lstSourceEncoding" }; private readonly Button _resolve = new() { Name = "btnConfirmSourceEncoding" }; + + /// + /// Shown in place when the ticked rows cannot be acted on. A silent no-op here + /// resolves to "Conversion cancelled. No files were modified." - a cancellation + /// the user never asked for, blamed on them. + /// + private readonly Label _scopeProblem = new() + { + Name = "lblSourceChoiceProblem", + AutoSize = true, + MaximumSize = new Size(660, 0), + Visible = false, + ForeColor = Color.FromArgb(0xB0, 0x28, 0x28), + }; private ListView? _refusedList; /// @@ -259,6 +273,32 @@ private Panel BuildRefusalPanel(List refused) _resolve.MinimumSize = new Size(230, 0); _resolve.Click += (_, _) => { + ListViewItem[] ticked = + [.. _refusedList?.CheckedItems.Cast() ?? []]; + + // A row carries the path this review resolved for it. A null one cannot be + // matched to an entry later, so acting on it would drop the file from the + // scope without saying so. + int unresolvable = ticked.Count(item => item.Tag is not string); + + if (ticked.Length == 0) + { + ShowScopeProblem( + "Tick at least one file to use this encoding for."); + return; + } + + if (unresolvable > 0) + { + ShowScopeProblem( + $"{unresolvable} of the {ticked.Length} ticked file(s) are no longer " + + "inside this review's directory, so the chosen encoding cannot be " + + "applied to them. Run View again for the directory these files are " + + "actually in, then choose the encoding."); + return; + } + + _scopeProblem.Visible = false; ChosenSourceEncoding = (string)_sourceChoice.SelectedItem!; ChosenFiles = TickedFiles(); DialogResult = DialogResult.Retry; @@ -269,6 +309,7 @@ private Panel BuildRefusalPanel(List refused) chooser.Controls.Add(_sourceChoice); chooser.Controls.Add(_resolve); + chooser.Controls.Add(_scopeProblem); var note = new Label { @@ -305,6 +346,13 @@ private List TickedFiles() => .Select(p => p!) ]; + /// Says why the button did nothing, instead of doing nothing. + private void ShowScopeProblem(string message) + { + _scopeProblem.Text = message; + _scopeProblem.Visible = true; + } + /// /// Keeps the button scope explicit. /// From c602264a4f722087e4c7454dd0d72e6341a4c71d Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Sun, 6 Sep 2026 07:36:00 +0300 Subject: [PATCH 07/11] Record the manual verification, and blame the right layer Both the defect and the fix were reproduced by hand. Before: the review closed and the status bar reported a cancellation nobody asked for, after the user had ticked a file and chosen an encoding. After: it stays open and says which ticked files it cannot act on. Until this run the defect existed only as a reading of the code. That also settles where the abandoned smoke phase went wrong. The dropdown works perfectly by hand, so SelectCombo timing out is a defect in the automation driver rather than in EC - most likely its keyboard fallback calling SetForegroundWindow on the main window while a modal review is open. The note said the cause was unknown; it is now narrowed to the layer it belongs to, which is most of what the next person needs. Co-Authored-By: Claude Opus 5 --- docs/DEFECT-BACKLOG.md | 43 +++++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/docs/DEFECT-BACKLOG.md b/docs/DEFECT-BACKLOG.md index 767734d..9fc8590 100644 --- a/docs/DEFECT-BACKLOG.md +++ b/docs/DEFECT-BACKLOG.md @@ -165,23 +165,32 @@ drift: the detector-parity job exists to stop exactly this happening to the shared detector, and nothing plays that role for the safety machinery around it. **Open** — decide whether the two should converge, and on which. -## The source-choice refusal has no GUI coverage - -The review now refuses a source choice it cannot apply, instead of closing on an -emptied scope. A smoke phase to drive that sequence was attempted and abandoned. - -The setup reproduces correctly - scan one directory, retarget the window at -another, and the refused row appears labelled `..\scanned\french.txt`, which only -happens when the plan's root does not contain the file. What does not work is -driving the source-encoding combo in that dialog state: `SelectCombo` times out -waiting for the selection to take, while the identical call in phase C succeeds. -Activating the review first and ticking the row first were both tried; neither -changed it. The cause is not known. - -Worth recording because the diagnosis is most of the work, and because the fix -is currently verified by reading rather than by driving. **Open** - either finish -the phase, or cover the refusal with a unit test that constructs the form -directly, as `InteractiveControlsExposeStableAutomationIds` already does. +## The source-choice refusal is verified by hand, not by the suite + +Both the defect and its fix were reproduced manually. That is how the defect was +finally confirmed at all — until then it existed only as a reading of the code. + +**Before the fix:** scan a directory, point the window at a different one without +scanning again, tick the refused file, choose an encoding, press confirm. The +review closes and the status bar reads "Conversion cancelled. No files were +modified." The choice is discarded and blamed on a cancellation nobody made. + +**After the fix:** the review stays open and says which ticked files are no +longer inside its directory, and what to do about it. + +A smoke phase for the sequence was attempted and abandoned. The setup drives +correctly — the refused row appears labelled `..\scanned\french.txt`, which only +happens when the plan's root does not contain the file — but `SelectCombo` times +out on the source-encoding dropdown in that dialog state, while the identical +call in phase C succeeds. Driving the review to the foreground and ticking the +row first were both tried; neither changed it. + +**The dropdown works perfectly by hand**, so this is a defect in the automation +driver, not in EC. One candidate: `SelectCombo`'s keyboard fallback calls +`SetForegroundWindow` on the *main* window, which is the wrong target while a +modal review is open. **Open** — finish the phase, or cover the refusal with a +unit test that constructs the form directly, as +`InteractiveControlsExposeStableAutomationIds` already does. ## The nine open from the original thirty-five From 816ad4cf2d0a8e7caba0cdd718ff5449b5f68e1e Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Sun, 6 Sep 2026 07:41:22 +0300 Subject: [PATCH 08/11] Cover the source-choice refusal with a test, not a manual step The refusal had no automated coverage because the smoke phase for it could not be made to work. A manual procedure is not the fallback: the whole point of the nine phases was retiring one. The decision moves out of the click handler into DescribeUnusableScope, which returns why the ticked rows cannot be acted on or null when they can. That is a seam a test can reach. PerformClick does nothing on a control that is not effectively visible, and a form this test never shows makes every child invisible, so a test driving the button would have to show a window - and the unit suite must not need an interactive desktop. The test builds a plan rooted outside its own files, which is what the window produces whenever the directory box changes after a scan, ticks the row, confirms the resolved path really is null, and asserts the refusal explains itself and reports no choice. Mutation-checked: with the unresolvable count forced to zero the test fails, and the file was restored byte-identical. 646 tests pass. The driver defect behind the abandoned phase stays open on its own account. It will bite any future phase that touches a combo inside a dialog, and the backlog now says where to look rather than calling the cause unknown. Co-Authored-By: Claude Opus 5 --- docs/DEFECT-BACKLOG.md | 16 +++- .../ConversionConfirmationFormTests.cs | 84 ++++++++++++++++++- .../ConversionConfirmationForm.cs | 52 +++++++----- 3 files changed, 126 insertions(+), 26 deletions(-) diff --git a/docs/DEFECT-BACKLOG.md b/docs/DEFECT-BACKLOG.md index 9fc8590..cd86ad0 100644 --- a/docs/DEFECT-BACKLOG.md +++ b/docs/DEFECT-BACKLOG.md @@ -165,7 +165,7 @@ drift: the detector-parity job exists to stop exactly this happening to the shared detector, and nothing plays that role for the safety machinery around it. **Open** — decide whether the two should converge, and on which. -## The source-choice refusal is verified by hand, not by the suite +## The source-choice refusal is covered by a unit test, not a smoke phase Both the defect and its fix were reproduced manually. That is how the defect was finally confirmed at all — until then it existed only as a reading of the code. @@ -188,9 +188,17 @@ row first were both tried; neither changed it. **The dropdown works perfectly by hand**, so this is a defect in the automation driver, not in EC. One candidate: `SelectCombo`'s keyboard fallback calls `SetForegroundWindow` on the *main* window, which is the wrong target while a -modal review is open. **Open** — finish the phase, or cover the refusal with a -unit test that constructs the form directly, as -`InteractiveControlsExposeStableAutomationIds` already does. +modal review is open. + +**The refusal is covered instead by a unit test**, not by a manual step. The +decision is now a method on the form — `DescribeUnusableScope` — so a test can +build a plan rooted outside its own files, tick the row, and assert on the +refusal without showing a window. `PerformClick` does nothing on a control that +is not effectively visible, and a test that had to show one would need an +interactive desktop, which is exactly what the unit suite must not require. + +The driver defect stays **open** on its own account: it will bite any future +phase that touches a combo inside a dialog. The refusal itself is closed. ## The nine open from the original thirty-five diff --git a/sources/EncodingChecker.Tests/ConversionConfirmationFormTests.cs b/sources/EncodingChecker.Tests/ConversionConfirmationFormTests.cs index 4e6bb52..f56b66e 100644 --- a/sources/EncodingChecker.Tests/ConversionConfirmationFormTests.cs +++ b/sources/EncodingChecker.Tests/ConversionConfirmationFormTests.cs @@ -1,4 +1,4 @@ -using System.Text; +using System.Text; using System.Windows.Forms; namespace EncodingChecker.Tests; @@ -110,6 +110,88 @@ public void ItBuildsWhenNothingIsRefused() }); } + /// + /// A plan whose root does not contain its own files. The GUI reaches this whenever the + /// directory box changes after a scan - by typing, by picking a recent entry, or by + /// dropping a folder on it - because the results list is only cleared when a scan + /// starts. + /// + private ConversionPlan PlanRootedElsewhere(string target = "utf-8") + { + var entries = new EntrySink(); + + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + IncludeSubdirectories = true, + IncludePatterns = ["*"], + Action = ScanAction.Convert, + TargetCharset = target, + TargetWriteBom = false, + WhatIf = true, + }, + entries.Add, + CancellationToken.None); + + string elsewhere = Directory.CreateTempSubdirectory("ec_elsewhere_").FullName; + + return ConversionPlan.FromEntries( + entries, elsewhere, target, targetHasBom: false, + backupEnabled: true, explicitSource: null); + } + + private static T Find(Control root, string name) where T : Control => + Descendants(root).OfType().Single(c => c.Name == name); + + [Fact] + public void ASourceChoiceThatCannotBeAppliedIsRefusedRatherThanDropped() + { + // Each refused row carries the path this review resolved for it. When the plan's + // root does not contain the file that path is null, and the ticked set used to be + // built by filtering those rows out - so the button did nothing, and the run + // reported "Conversion cancelled. No files were modified." The user had ticked a + // file and chosen an encoding; the cancellation was neither theirs nor explained. + Write("legacy.txt", "Le café était déjà prêt", "windows-1252"); + ConversionPlan plan = PlanRootedElsewhere(); + + UiTest.OnStaThread(() => + { + using var form = new ConversionConfirmationForm(plan); + form.CreateControl(); + + var refused = Find(form, "lstRefusedFiles"); + var chooser = Find(form, "lstSourceEncoding"); + var confirm = Find