diff --git a/README.md b/README.md index b8a0bd5..413d3d6 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.1 +# EncodingChecker v3.11.2 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/DEFECT-BACKLOG.md b/docs/DEFECT-BACKLOG.md new file mode 100644 index 0000000..d780bf5 --- /dev/null +++ b/docs/DEFECT-BACKLOG.md @@ -0,0 +1,233 @@ +# Defect backlog + +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.** +Six further findings have been raised since v3.11.1, two of them already fixed. +**Eleven open in total.** + +## 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 | *not reproduced* | The detector's BOM-less instance never reaches the guard; both paths refuse. Tested. | +| 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 | fixed | Reader and writer now share one options object, in the plan store and the metadata store. | +| 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. | +| 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 + +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. + +### 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 +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 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. + +**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. + +**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. + +## What is still open, scored + +Two axes, because one number hides the thing that matters. **Impact** is what a +user loses when it happens — the original review's rule, severity by consequence +and not by how hard the fix is. **Reach** is how easily it happens at all. A +critical impact nobody can trigger is not a crisis, and a low impact everyone +trips over is not noise. + +| | Finding | Impact | Reach | Note | +|---|---|---|---|---| +| 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. | +| — | 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. | +| EC-15 | Semantics booleans written as a contract, enforced nowhere | Low | Common | Cannot weaken behaviour — EC ignores the claim and always does the strict thing. The risk is a reader treating `OutputVerification: true` as evidence a check ran. | +| EC-17 | Text-validation comment contradicts its code | Low | Common | Behaviour is right, comment is wrong, and the file must stay byte-identical across three repos. A maintainer "correcting" it the wrong way would weaken binary rejection. | +| EC-23 | Null-forgiving dereference of `ResolvePath` | Low | Rare | Correct today only because `FindStaleFiles` runs 29 lines earlier. EC-06 is what happened when that invariant broke. | +| EC-18 | Ambiguity recomputed per pass | Low | Common | Measured: no cost. The probe aborts at the first invalid sequence, so a provable file settles in the first buffer. Untidy, not slow. | +| — | Automation driver cannot select a combo inside a dialog | Low | — | Blocks a smoke phase, not a user. Suspect: `SelectCombo`'s keyboard fallback targets the main window while a modal review is open. | +| — | Hash handling has drifted from LineEndingNormalizer | — | — | A decision, not a defect. See above. | + +Nothing here writes to a file nobody approved, which is why none of it blocked a +release. + +### 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`. | diff --git a/docs/RELEASE-NOTES-v3.11.2.md b/docs/RELEASE-NOTES-v3.11.2.md new file mode 100644 index 0000000..ef34a0a --- /dev/null +++ b/docs/RELEASE-NOTES-v3.11.2.md @@ -0,0 +1,103 @@ +# EncodingChecker v3.11.2 + +Two silent failures made loud, a measured speedup, and the first backlog this +project has kept. + +## A plan rooted at a drive can be applied + +`-BasePath D:\` produced a normal-looking plan that `-Apply` then refused +entirely, reporting every file as resolving outside the plan's own directory — +blaming the paths rather than the root. The containment check appended a +separator to a root that already ended in one, so the prefix became `D:\\` and +nothing could ever match it. + +This shipped broken in **v3.11.0 and v3.11.1**. It was found before v3.11.0, +recorded, reported as closed, and rediscovered from scratch during an unrelated +review. That is why this release also adds a backlog. + +## A source choice that cannot be applied is refused, not dropped + +In the review dialog, each refused row carries the path resolved for it. Rows +that resolved to nothing were filtered out of the ticked set in silence, so the +confirm button did nothing and the run reported **"Conversion cancelled. No +files were modified."** — a cancellation nobody asked for, after the user had +ticked files and chosen an encoding. + +The drive-root defect was one way in. It is not the only one: the results list +is cleared when a scan *starts* and never when the directory box changes, and +that box accepts 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 review now says which ticked files it cannot act on and what to do about it, +and stays open. The *Proceed* path already handled this correctly; only the +source-choice path failed silently. + +## Conversion is faster + +The parallelism cap was `min(ProcessorCount, 4)`. Conversion is bound by +per-file I/O latency rather than CPU, so the cap bit well before core count did. +It is now `min(ProcessorCount, 8)`. + +Measured over 2,000 files: + +| | Before (4) | After (8) | +|---|---|---| +| Without backups | 4,207 ms | 2,511 ms | +| With backups | 10,516 ms | 7,305 ms | + +Past 8 the curve flattens and backup runs stop improving, which is why 8 rather +than something larger. `ProcessorCount` still binds first on small machines, and +`-MaxParallelism` still overrides. + +## Recovery files are read with the options they were written with + +The plan and sidecar stores passed a configured options object when writing and +none when reading. Nothing observable changes today — both settings affect +writing only — but a setting added later for the writer would have altered every +file EC produces without altering what EC accepts, and each one would have +stopped loading silently. + +## A defect backlog + +[`docs/DEFECT-BACKLOG.md`](DEFECT-BACKLOG.md) records the status of all +thirty-five findings from the two independent reviews that preceded v3.11.0, +re-derived from the source rather than carried over from a summary, plus +everything found since. Open items are scored on two axes — what a user loses, +and how easily it happens — because one number hides the difference between a +critical impact nobody can trigger and a low impact everyone trips over. + +It also records three hashing optimisations that were built, measured and +rejected, so the next person does not have to re-derive them. + +## Release engineering + +- The GUI smoke suite now **gates the release**. All nine phases run against the + signed, published executable, after signing and before packaging, and a + failure stops publication. +- The release workflow can be **rehearsed without publishing**. A manual run does + everything a release does except create the release. Publishing requires a tag + push, not merely a tag reference, so a run started by hand cannot publish + whatever reference it was given. + +## Compatibility + +No conversion or classification behaviour changes. Conversion semantics stay at +**6**, the plan schema at **5**, the journal schema at **4**, and exit codes are +unchanged. + +The drive-root fix does change what happens when such a plan is applied — it +converts where it previously refused — but the decisions a plan records mean +exactly what they meant before. Only the resolution of the paths was wrong, so +the semantics version is deliberately left alone. + +## Verification + +- 646 tests pass, none skipped; release build with no warnings +- The nine-phase GUI smoke suite passes +- Each fix was mutation-checked — the change reverted, the intended test required + to fail, the file restored byte-identical — **except** the options-object + change, which no test can demonstrate without adding a setting to production + code purely to make one fail. That is stated rather than papered over. +- No four-corpus audit was run. The checklist requires one for a release that + changes detection or conversion policy; this changes neither. 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