diff --git a/README.md b/README.md index 99f047b..8e93354 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.12.0 +# EncodingChecker v3.12.1 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 index 6c73f1f..7a7dd49 100644 --- a/docs/DEFECT-BACKLOG.md +++ b/docs/DEFECT-BACKLOG.md @@ -1,294 +1,850 @@ # 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, 8 open, 1 could not be reproduced.** -Six further findings have been raised since v3.11.1, two of them already fixed. -Twelve more since v3.11.2, all twelve fixed. -**Twelve 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 | **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`. | -| 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. | +This is the current ledger for defects and review findings in EncodingChecker. +It is organised by status, not discovery date, so the open work is visible in +one place. Longer evidence and history follow the ledger. + + + +**Derived count: 61 findings — 44 fixed, 13 open, 1 not reproduced, +1 withdrawn, 1 not a defect, and 1 design decision.** Recompute and validate +these figures with: + +```powershell +powershell -NoProfile -ExecutionPolicy Bypass -File docs/Test-DefectBacklog.ps1 +``` + +`powershell` rather than `pwsh` because it is present on every Windows machine; the script +is ASCII-only and needs no BOM, so either shell runs it. `-ExecutionPolicy Bypass` because +this repository ships no signed scripts and a stock machine refuses to run them at all. It +applies to that one process and changes nothing on the machine. + +The checker reads the canonical tables below, verifies unique IDs and known +statuses, and requires both impact and reach for every open finding. + +## Status and scoring rules + +- **Fixed** means the responsible code is gone and a current source location, + regression test, or current-build probe demonstrates the replacement. +- **Open** means the behavior remains reachable or a source path to it remains. +- **Not reproduced** means a current attempt did not trigger the proposed + behavior; that is weaker than proof that it cannot occur. +- **Withdrawn** means the proposed defect was disproved. +- **Not a defect** means the implementation matches an intentional contract. +- **Decision** records a deliberate design difference that needs a choice, not + a product correction. + +Impact asks what a user loses if a finding occurs. Reach asks how readily the +preconditions occur. They are kept separate because a severe but constructed +case and a harmless common case require different decisions. + +## Canonical ledger + +This is the only place that assigns a current status to an individual finding. +Existing `EC-nn` and `CX-nn` IDs are unchanged. `BL-nn` IDs were assigned during +the 2026-09-08 reformat to findings that previously had only a sentence. + + + +### Open findings + +| ID | Finding | Status | Impact | Reach | Details | +|---|---|---|---|---|---| +| EC-08 | An include pattern can hang a scan indefinitely | Open | Medium | Theoretical | [EC-08](#ec-08) | +| EC-15 | Serialized conversion guarantees are not enforced individually | Open | Low | Common | [EC-15](#ec-15) | +| EC-17 | A text-validation comment contradicts the calculation | Open | Low | Common | [EC-17](#ec-17) | +| EC-18 | A negative BOM-less UTF-16 ambiguity result is recomputed | Open | Low | Common | [EC-18](#ec-18) | +| EC-20 | Detection permits concurrent writes and deletes | Open | Low | Rare | [EC-20](#ec-20) | +| EC-23 | Plan application relies on a null-forgiving path dereference | Open | Low | Rare | [EC-23](#ec-23) | +| CX-06 | The entropy gate runs before BOM detection | Open | Medium | Occasional | [CX-06](#cx-06) | +| BL-01 | Ambiguous BOM-less UTF-32 can be converted under the wrong byte order | Open | Critical | Theoretical | [BL-01](#bl-01) | +| BL-05 | Force-closing during a run can race UI callbacks | Open | Low | Rare | [BL-05](#bl-05) | +| BL-18 | BOM-less UTF-16 can be detected and converted as UTF-32 | Open | Critical | Rare | [BL-18](#bl-18) | +| BL-19 | NUL-heavy ASCII can be reported as BOM-less UTF-16 | Open | Medium | Rare | [BL-19](#bl-19) | +| BL-20 | Hard-linked paths are processed independently | Open | Low | Rare | [BL-20](#bl-20) | +| BL-21 | Detection can accept a truncated trailing sequence that conversion rejects | Open | Low | Rare | [BL-21](#bl-21) | + +### Closed and other findings + +| ID | Finding | Status | Impact | Reach | Details | +|---|---|---|---|---|---| +| EC-01 | Applying a plan could convert a file the plan refused | Fixed | — | — | [EC-01](#ec-01) | +| EC-02 | Read-only validation disagreed with the BOM-less Unicode safety policy | Fixed | — | — | [EC-02](#ec-02) | +| EC-03 | The GUI omitted the source-choice advisory | Fixed | — | — | [EC-03](#ec-03) | +| EC-04 | `-Plan` could exit successfully after scan failures | Fixed | — | — | [EC-04](#ec-04) | +| EC-05 | An unreadable non-conversion plan entry made a plan unusable | Fixed | — | — | [EC-05](#ec-05) | +| EC-06 | A drive-root base path made every plan unusable | Fixed | — | — | [EC-06](#ec-06) | +| EC-07 | A refusal advised the same ambiguous encoding it rejected | Fixed | — | — | [EC-07](#ec-07) | +| EC-09 | Excluded EC artifacts were uncounted | Fixed | — | — | [EC-09](#ec-09) | +| EC-10 | A scan failure was journaled as a policy refusal | Fixed | — | — | [EC-10](#ec-10) | +| EC-11 | Plan application leaked a Ctrl+C handler | Fixed | — | — | [EC-11](#ec-11) | +| EC-12 | The GUI counted skipped files as unchanged | Fixed | — | — | [EC-12](#ec-12) | +| EC-13 | A mixed-source plan could report one source encoding for the run | Fixed | — | — | [EC-13](#ec-13) | +| EC-14 | The output-text hash copied the source-text hash | Fixed | — | — | [EC-14](#ec-14) | +| EC-16 | Settings were written by truncating the live file | Fixed | — | — | [EC-16](#ec-16) | +| EC-19 | Double-BOM handling depended on the encoding instance | Not reproduced | — | — | [EC-19](#ec-19) | +| EC-21 | Save dialogs were not disposed | Fixed | — | — | [EC-21](#ec-21) | +| EC-22 | Plan and metadata readers used different JSON options from writers | Fixed | — | — | [EC-22](#ec-22) | +| CX-01 | Empty option values were silently treated as absent | Fixed | — | — | [CX-01](#cx-01) | +| CX-02 | A failed second conversion could destroy the first recovery record | Fixed | — | — | [CX-02](#cx-02) | +| CX-03 | Applying a plan followed a root replaced by a junction | Fixed | — | — | [CX-03](#cx-03) | +| CX-05 | The journal could not represent uncertainty after installation | Fixed | — | — | [CX-05](#cx-05) | +| CX-07 | Old plans, journals, and reports should be excluded from scans | Not a defect | — | — | [CX-07](#cx-07) | +| CX-08 | Documentation and validation disagreed about `-DetectOnly` conflicts | Fixed | — | — | [CX-08](#cx-08) | +| CX-09 | Cancelling after writes produced no journal | Fixed | — | — | [CX-09](#cx-09) | +| CX-10 | Plan summaries said detection was bypassed when it ran | Fixed | — | — | [CX-10](#cx-10) | +| CX-11 | GUI startup could fail before settings error handling began | Fixed | — | — | [CX-11](#cx-11) | +| CX-12 | Saved window positions ignored the current monitor layout | Fixed | — | — | [CX-12](#cx-12) | +| CX-13 | Detector parity was not a pull-request or release gate | Fixed | — | — | [CX-13](#cx-13) | +| BL-02 | CSV cells need formula neutralization | Withdrawn | — | — | [BL-02](#bl-02) | +| BL-03 | Conversion parallelism was capped at four | Fixed | — | — | [BL-03](#bl-03) | +| BL-04 | A ticked source choice could be discarded without explanation | Fixed | — | — | [BL-04](#bl-04) | +| BL-06 | Target identity was compared by label rather than codec | Fixed | — | — | [BL-06](#bl-06) | +| BL-07 | A whole-file unchanged claim came from a 64 KiB sample | Fixed | — | — | [BL-07](#bl-07) | +| BL-08 | Preview could approve a source that conversion could not decode | Fixed | — | — | [BL-08](#bl-08) | +| BL-09 | One unexpected file exception could stop the whole run | Fixed | — | — | [BL-09](#bl-09) | +| BL-10 | An unreadable folder was invisible to machine output | Fixed | — | — | [BL-10](#bl-10) | +| BL-11 | Folders skipped by name were uncounted | Fixed | — | — | [BL-11](#bl-11) | +| BL-12 | Some validation failures had no reason code | Fixed | — | — | [BL-12](#bl-12) | +| BL-13 | Refusal reasons were re-derived after the policy decision | Fixed | — | — | [BL-13](#bl-13) | +| BL-14 | Decode failures could report a negative chunk offset | Fixed | — | — | [BL-14](#bl-14) | +| BL-15 | Console output ignored the console's encoding | Fixed | — | — | [BL-15](#bl-15) | +| BL-16 | Help and CLI documentation stated an old parallelism default | Fixed | — | — | [BL-16](#bl-16) | +| BL-17 | The lifetime of `.bak` was undocumented | Fixed | — | — | [BL-17](#bl-17) | +| BL-22 | An unwritable report or journal path was discovered after conversion | Fixed | — | — | [BL-22](#bl-22) | +| BL-23 | An undefined plan action was reported as a conversion | Fixed | — | — | [BL-23](#bl-23) | +| BL-24 | Plans, journals, reports, and settings used truncate-in-place writes | Fixed | — | — | [BL-24](#bl-24) | +| BL-25 | EC and LEN use different transient content-hash machinery | Decision | — | — | [BL-25](#bl-25) | +| BL-26 | The GUI smoke driver rejected an exact offscreen combo item | Fixed | — | — | [BL-26](#bl-26) | + + + +## Open-finding evidence + +### EC-08 + +**A hostile include mask can monopolize the regex engine.** +`DirectoryTraversal.CompilePatterns` still translates `*` to `.*`, creates a +regex with `RegexOptions.Compiled`, and therefore uses +`Regex.InfiniteMatchTimeout`. On 2026-09-08 a scan with +twelve separated wildcards against a nonmatching forty-character run of `a` +did not finish within three seconds and had to be terminated. + +The trigger requires both inputs to be deliberately hostile: roughly ten or +more separated wildcards and a filename with roughly twenty-four or more mostly +consecutive copies of the separating character. Earlier measurements found +realistic names answered in 0–5 ms, while the forty-`a` case exceeded 20 s. +The mask comes from the operator, not an untrusted file. + +`RegexOptions.NonBacktracking` removed the blow-up, matched the current engine on +the tested masks, and cost nothing measurable beside file I/O. It was measured +and deliberately reverted because of the constructed reach. Three dead ends are +worth preserving: a matching filename stops at the first success and proves +nothing; `*` crossing directory separators is deliberate and tested; replacing +`.*` with `[^/]*` does not prevent backtracking in a separator-free filename. +`PathAwarePatternTests.PathQualifiedPattern_MatchesOnlyTheIntendedSubtree` pins +the intended `src/*.cs` directory behavior. + +### EC-15 + +**The five serialized guarantee flags look enforceable but are descriptive.** +`ConversionSemantics` writes `StrictDecoding`, `StrictEncoding`, +`OutputVerification`, `AtomicInstall`, and `LegacyRequiresExplicitSource`. +`ConversionPlan.Load` enforces `SemanticsVersion` only. EC always executes its +current strict behavior, so this cannot weaken conversion, but a reader may +mistake a serialized `true` value for proof that the specific check ran. + +### EC-17 + +**The printable-ratio comment says the opposite of the calculation.** +`TextValidation.cs` increments the total rune count before its category switch. +Control and private-use scalars do not increment `printable`, so they lower the +ratio; they are not ignored. The behavior is the intended binary-rejection +behavior. The comment is shared byte-for-byte with LineEndingNormalizer and +CorpusTesters, so its correction must be synchronized. + +### EC-18 + +**Only a positive ambiguity result is cached.** In `ScanEngine`, +`entry.HasAmbiguousBomlessUtf16 || IsAmbiguousBomlessUtf16(...)` short-circuits +when the stored value is true. A stored false runs the full check again on each +pass. Measurement found no meaningful cost because a provable file normally +fails the opposite-order decode in its first buffer; the issue is redundant +work and unclear state, not observed slowness. + +### EC-20 + +**The detector has a looser sharing mode than the paths that rely on its result.** +`TextEncoding.DetectFromFile` opens with `FileShare.ReadWrite | +FileShare.Delete`; validation and source-snapshot paths use `FileShare.Read`. +Detection can therefore describe bytes while another process changes or deletes +them. This affects detection and validation output; conversion later takes its +own bound snapshot before writing. + +### EC-23 + +**Plan application still depends on ordering to make a nullable path non-null.** +At the current source location `Program.CliExecution.cs:90`, the code uses +`plan.ResolvePath(f)!`. `FindStaleFiles` validates the same paths 29 lines +earlier, so the dereference is safe under the present flow. EC-06 demonstrates +why leaving that invariant implicit is fragile. + +### CX-06 + +**High entropy can hide an otherwise valid BOM.** At current +`TextEncoding.cs:175`, the entropy guard returns before +`UnicodeDetector.DetectFromBuffer` examines the BOM at line 182. The result can +be an unknown or wrong encoding even when the file declares it. This changes +what EC reports; it is not evidence that conversion writes the file. + +### BL-01 + +**BOM-less UTF-32 byte order can be guessed and then treated as proven.** The +ambiguity guard covers code pages 1200 and 1201, not 12000 and 12001. A current +end-to-end fixture containing UTF-32BE `00 00 01 00` units was detected as +little-endian UTF-32 and converted from U+0100 to U+10000 with exit 0. Reaching +the case requires every scalar to have the special byte shape, so ordinary text +is unlikely to do it; its consequence is nevertheless silent text change. + +### BL-05 + +**A second close request can outlive the form while its worker still uses it.** +`OnFormClosing` deliberately allows a confirmed second close because +cancellation is cooperative. A worker can subsequently call the synchronous +confirmation `Invoke`, and completion accesses form controls. Finished files +have already been installed independently and the in-flight file remains +protected; the expected symptom is an exception during exit, not lost file +content. The timing-dependent path was source-confirmed but not reproduced. -## Found after v3.11.2 +### BL-18 -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. +**A degenerate BOM-less UTF-16 stream can look like UTF-32.** A current fixture +containing UTF-16LE `00 01 0A 00` units was detected as BOM-less UTF-32 and +rewritten to different Unicode with exit 0. Output verification cannot expose a +wrong source interpretation because both sides use that same interpretation. -| Finding | Status | Note | -|---|---|---| -| "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 - -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 - 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 -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 | +The original measurement used nineteen realistic shapes: 41 of 44 detections +were right, and all three misses had the same constructed shape—one character +per line, with every other UTF-16 code unit a C0 control. The reach is low; the +impact when reached is silent text change. + +### BL-19 + +**The UTF-16 structure heuristic can claim NUL-heavy ASCII.** The earlier +document said 2.3% of all bytes; that was wrong. A current 65,536-byte ASCII +fixture with a NUL every 100 bytes—1.001% overall—was reported as UTF-16BE. The +detector threshold is 2% in one putative UTF-16 byte channel, approximately 1% +of all bytes for this shape. Conversion refused with exit 5 and the source hash +did not change, so the wrong claim currently reaches detection and validation, +not installation. + +### BL-20 + +**Filesystem aliases are treated as separate selected paths.** Two hard links +to one UTF-8 file were both converted in a current probe. Both retained exact +text, and each received its own verified `.bak` and `.ecmeta.json`; the normal +Windows `File.Replace` path broke their link relationship. This can duplicate +work and does not preserve hard-link identity. The fallback +`File.Move(..., overwrite: true)` path could not be forced on this platform, so +no claim is made about that branch. + +### BL-21 + +**Sample detection and complete conversion intentionally use different flush +semantics.** A UTF-8 file ending in incomplete bytes `E2 82` was reported as +UTF-8 by `-DetectOnly`, because sample detection does not flush an incomplete +tail. Conversion flushed the strict decoder, returned `SourceDecodeError` and +exit 3, and left the source unchanged. The safety path is correct; the detector +can still bless a complete file that conversion rejects. + +## Evidence for the original review findings + +### EC-01 + +**A reviewed refusal is binding.** `PlannedFile.HasReliableUnicodeDetection` +carries the policy input that was formerly lost at the plan boundary. +`AppliedPlanFidelityTests.ThePlanCarriesTheDetectionReliabilityTheVetoDependsOn` +pins it. + +### EC-02 + +**Read-only modes use the conversion safety decision.** +`ReadOnlyModeAmbiguityTests` proves that an unprovable BOM-less Unicode source is +not reported valid when conversion would refuse it. + +### EC-03 + +**The v3.10.1 advisory reaches the real window.** GUI smoke phase H asserts on +the rendered source-choice text rather than only on an internal decision. + +### EC-04 + +**Plan failures control the exit code.** The plan branch returns processing +failure before considering `-FailOnChanges`; `PlanPreflightReportingTests` +covers the ordering. + +### EC-05 + +**Only scheduled conversions require a source hash.** Plan loading no longer +makes an unreadable skipped or refused entry render the whole plan unusable. + +### EC-06 + +**Drive roots resolve without manufacturing `C:\\`.** +`ConversionPlan.ResolvePath` now uses a root-aware containment check, with +theories for `C:\`, nested paths, and outside paths. + +This defect was found before v3.11.0, recorded as confirmed, reported as closed, +and shipped broken in both **v3.11.0 and v3.11.1**. It was rediscovered during an +unrelated review. This history is why status is now derived from a checkable +ledger rather than a summary. + +### EC-07 + +**The refusal gives two actionable choices.** +`BomlessUnicodeSafety.DescribeRefusal` offers both UTF-16 byte orders instead of +recommending the unproved estimate. + +### EC-09 + +**Selected EC artifacts are counted.** `.bak`, `.ecmeta.json`, and temporary +conversion files update `TraversalCounters.FilesExcludedAsEcArtifact`, pinned by +`ArtifactExclusionCoverageTests`. + +### EC-10 + +**A failed snapshot is an error, not a policy decision.** Journal outcome tests +pin `ScanFailed` to `Error` rather than `Refused`. + +### EC-11 + +**Both console cancellation subscriptions have bounded lifetimes.** Each Ctrl+C +handler is removed in `finally`, so it cannot retain a disposed token source. + +### EC-12 + +**Skipped and unchanged are separate GUI counts.** The tally is pinned by +`SkippedFilesAreNotCountedAsUnchanged`. + +### EC-13 + +**Mixed batches describe source choice per file.** `DescribeSourceChoice` no +longer presents one run-wide explicit encoding when several were used. + +### EC-14 + +**Source and output text hashes come from separate reads.** The conversion +record accepts the output digest produced by verification and rejects a missing +one; `RecordedProvenanceTests` compares the installed output independently. + +### EC-16 + +**Settings use the same atomic artifact writer as other records.** An +interruption before replacement leaves the previous preferences intact. This +was closed incidentally by the v3.12.1 artifact-writer refactor, not by a +settings-specific change. + +### EC-19 + +**The proposed encoding-instance gap did not reach conversion.** Conversion +re-resolves the codec name through `Encoding.GetEncoding`, whose UTF-8 instance +has the expected preamble. A current file beginning with two UTF-8 BOMs was +refused with `MultipleLeadingByteOrderMarks` and exit 5 through both automatic +detection and `-From utf-8`. + +### EC-21 + +**All three save dialogs have deterministic disposal.** Each construction site +uses `using var`. + +### EC-22 + +**Each JSON store shares its reader and writer options.** Plan and recovery +metadata no longer serialize and deserialize through mismatched option objects. + +### CX-01 + +**A present option must carry a usable value.** Empty values for all value-taking +flags are rejected with exit 1; `BlankOptionValueSafetyTests` verifies that +nothing changes. + +### CX-02 + +**A stale sidecar cannot survive backup replacement.** +`RemoveBeforeBackupReplacement` removes the old record before replacing the +backup, including a read-only record. + +### CX-03 + +**Applied plans re-check every path component.** +`HasReparsePointInPath` rejects a root or descendant replaced by a junction; +applied-plan integrity tests cover the final component and outside-root cases. + +### CX-05 + +**The journal can say what is and is not known after installation.** +`ConvertedWithWarning` distinguishes a completed install with a later warning; +`InstallationUnknown` represents a failure after the replacement outcome can no +longer be proved. + +### CX-07 + +**Old JSON and CSV artifacts are intentionally ordinary input.** The earlier +ledger claimed they were excluded and even described a correction that was +never made. `docs/CLI.md` deliberately says old plans, journals, and reports are +scanned because a user may wish to convert them. A current `old-plan.json` probe +was detected as ASCII. Only backups, sidecars, temporary files, and the current +command's output paths are excluded. + +This false correction was discovered by re-deriving the row from source rather +than trusting its own note. + +### CX-08 + +**CLI mode conflicts are executable documentation.** +`DocumentedOptionContractTests` pins the rejected combinations around +`-DetectOnly`, validation, conversion, plan, and apply. + +### CX-09 + +**An interrupted GUI write run still produces a journal.** Unit coverage and +GUI smoke phase I reconcile completed and unattempted entries. + +### CX-10 + +**An explicit choice does not erase detection history.** Plan summaries now say +“chosen by you; detection still ran and is recorded,” with provenance tests. + +### CX-11 + +**Settings-path creation is inside startup error handling.** A failure no longer +escapes before the guarded settings load begins. + +### CX-12 + +**Window restoration checks the monitors that exist now.** +`WindowPosition.IsReachable` requires a useful title-bar intersection, with +tests for removed, left-side, and secondary displays. + +### CX-13 + +**Detector parity is enforced before integration and release.** The parity +workflow runs on pull requests, and the release workflow declares it as a job +dependency. + +## Evidence for later findings + +### BL-02 + +**No reachable report field begins with a spreadsheet formula marker.** +`DirectoryTraversal` resolves the `File` value with `Path.GetFullPath`, so it +begins with a drive letter or UNC prefix. A current file named `=1+1.txt` +produced `C:\...\=1+1.txt`. Encoding, BOM, target, result, reason code, and +diagnostic are product-controlled values. Reopen this only if a reachable field +starting with `=`, `+`, `-`, or `@` is demonstrated. + +### BL-03 + +**The named default cap is eight.** `ScanEngine.MaxParallelismCap` and +`DocumentedParallelismDefaultTests` keep code, help, and `docs/CLI.md` aligned. +The change from four was measured on 2026-09-04 at 1.5–1.7x faster; that +historical timing was not rerun during the 2026-09-08 source recheck. + +### BL-04 + +**A source choice that cannot be scoped remains visible.** Each review row +carries its resolved path. `DescribeUnusableScope` detects a ticked row whose +path is unavailable, keeps the review open, names how many rows are affected, +and asks the user to run View again. The unit test +`ASourceChoiceThatCannotBeAppliedIsRefusedRatherThanDropped` and GUI smoke phase +J verify the message and unchanged bytes. + +Before this correction, choosing an encoding after the review's directory had +changed could close the dialog and report “Conversion cancelled. No files were +modified,” although the user had not cancelled. EC-06 made every row hit that +path when the review root was a drive root. + +### BL-06 + +**Already-target identity is canonical codec identity, not spelling.** The old +comparison used `WebName` against the caller's label. `-Target unicode`, +`ucs-2`, or `utf-16le` could therefore decode, re-encode, verify, and reinstall +files already in UTF-16LE with identical bytes, changing timestamps and creating +backups and sidecars. Under `-FailOnChanges`, spelling alone changed the exit +code; on BOM-less UTF-16 it could change a no-op into a refusal. The decision now +compares nonzero resolved code pages. ASCII-to-UTF-8 behavior was left separate +until full-file validation made folding it safe. + +### BL-07 + +**An unchanged decision validates the complete file.** Detection examines at +most 64 KiB. Previously, a matching source and target label skipped every later +byte, so a file valid for 64 KiB and invalid afterward was `Unchanged` under one +target and `Error` under another. Conversion already captures a whole-file +snapshot hash; the added validation measured at 0.04–0.10 ms per MiB. + +### BL-08 + +**A preview reads the source it promises to convert.** The old `WhatIf` branch +returned before decoding, so `-Plan` could approve an unreadable source and defer +failure until `-Apply`, after approval and potentially partway through a batch. +The source now receives strict full-file decode validation and an unreadable +entry is planned as `Refuse`. This remains source-only preflight: target +representability is tested by actual conversion, and a dedicated test pins that +limit. + +### BL-09 + +**Unexpected per-file exceptions are isolated.** The old `Parallel.ForEach` +worker caught four named exception types; a `SecurityException`, regex timeout, +or product defect escaped as `AggregateException` and ended work on files the +run had not reached. The CLI outer catch had the same four-name limit. The +worker now propagates only cancellation and `OutOfMemoryException`. +`RunParallel` is internal so a test can inject the otherwise difficult +exceptions and prove another file still runs. + +### BL-10 + +**Unreadable directories are visible as coverage loss.** An unreadable file +already produced a `ScanFailed` row and exit 3. An unreadable folder formerly +produced only an optional stderr warning—and no GUI trace because the GUI passes +no warning callback. In the original deny-ACE measurement, a tree with one +unreadable folder reported “1 file(s) processed” and exit 0; a wholly unreadable +root produced a header-only CSV and exit 0. `DirectoriesUnreadable` now counts +both traversal failure points separately from intentional exclusions. The exit +code deliberately remains unchanged, so strict automation must inspect coverage +output. + +### BL-11 + +**Folders skipped by reserved name have their own counter.** `.git`, `bin`, +`obj`, `build`, and the other documented names were skipped without appearing in +coverage. They are now counted separately from hidden, system, and reparse-point +folders, preserving the truth of both messages. An include pattern still cannot +override these exclusions, matching both user documents. + +### BL-12 + +**Every validation rejection names its cause.** A charset outside the allowed +list now uses `CharsetNotAllowed`; a file EC cannot identify uses +`UnknownEncoding`. Previously both reached `Invalid` with no reason, forcing a +consumer to re-derive information the producer already had. + +### BL-13 + +**The policy owns refusal reason codes.** `ApplyConversion` formerly repeated +the condition already reduced to `SourceInterpretation`. The copies were +textually identical, but nothing tied them together; a future refusal could +fall through the caller's bolt-on guard to `LegacySourceRequired`. +`ConversionPolicy.ReasonCodeFor` now owns the mapping. Tests cover all 256 +reachable input combinations and require every refusal to carry a reason. + +### BL-14 + +**Decode errors name offending bytes, not a misleading chunk offset.** +`DecoderFallbackException.Index` is relative to one decoder call and can be +negative when an invalid sequence began in carried bytes. A truncated UTF-8 +tail reported offset -2. The diagnostic now reports the byte sequence. An +absolute file offset would require restructuring the streaming loop and is not +claimed. + +### BL-15 + +**Interactive output follows the console; redirected output remains UTF-8.** +Reattaching to a parent console formerly rebuilt writers with UTF-8 even when +`Console.OutputEncoding` was IBM437, turning “Grüße aus München” into +“Gr├╝├ƒe aus M├╝nchen”. The console now receives its own encoding, where +unrepresentable characters become visibly lossy `?`; redirected CSV remains +UTF-8, and `-Report` is UTF-8 with BOM. EC does not mutate global console state. + +### BL-16 + +**The parallelism default has one code identity and checked documentation.** The +help and `docs/CLI.md` both used to say four after the implementation moved to +eight. A test reads the statement in each document and verifies its digits equal +`ScanEngine.MaxParallelismCap`. + +### BL-17 + +**A backup is the version replaced by the most recent run.** Re-converting a +file replaces `.bak` and removes its old sidecar. This long-standing +behavior was tested but undocumented. The documents now say it plainly. + +The review initially proposed refusing to overwrite a nonmatching backup. That +proposal was rejected after it broke four existing tests and would have blocked +an ordinary “wrong target, convert again” workflow until the user manually +deleted recovery files. CX-02 instead ensures stale metadata cannot describe a +new backup. + +### BL-22 + +**Requested report and journal destinations are checked before mode dispatch.** +Previously, a missing output directory or an existing directory used as the +output path was discovered after source files had been rewritten, leaving the +requested record absent. All four combinations were reproduced. Preflight now +returns processing exit code 3 before conversion. It does not create a probe +file, which would itself leave artifacts and still could not promise a later +write. + +### BL-23 + +**Undefined plan enums are rejected before a source is touched.** +`System.Text.Json` accepts any number for an enum. An action value of 99 formerly +fell through the result mapper as `Converted`: apply exited 0, reported one +conversion, and journaled action 99 even though the source hash was unchanged. +Plan loading now validates both `Action` and `SourceInterpretation`, while the +result mapper exhaustively names known actions and throws for anything else. + +### BL-24 + +**All durable EC artifacts use replacement writes.** Converted files and +recovery sidecars already used temporary files and replacement; plans, journals, +reports, and settings truncated their live destinations. A failed plan write +could destroy the reviewed plan immediately before use. `AtomicArtifactFile` +now handles all four. The sidecar retains its stronger dedicated writer and +read-back verification. This same refactor addressed EC-16. + +### BL-25 + +**EC and LineEndingNormalizer make the same safety argument with different +transient machinery.** Both use SHA-256 for source bytes and backup evidence. +EC also uses SHA-256 for content digests and persists them as +`SourceTextSha256` and `OutputTextSha256`; LEN uses XxHash3 for a private +normalized-content digest that is discarded. EC compares hexadecimal digest +strings with `string.Equals(..., OrdinalIgnoreCase)`; LEN compares digest bytes +with `CryptographicOperations.FixedTimeEquals`. + +| Evidence | EC | LEN | |---|---|---| -| Raw file / backup hash | SHA-256 | SHA-256 | +| Raw source and backup | 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. | -| 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. | -| 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. - -### 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 | +| Backup comparison | Case-insensitive hexadecimal strings | Fixed-time byte comparison | + +Neither comparison is wrong for accidental corruption. This is recorded so a +future maintainer can decide whether safety machinery should converge; detector +parity does not cover it. + +### BL-26 + +**The smoke driver now treats an exact combo item consistently even when UI +Automation calls it offscreen.** Phase J selected `windows-1252`, which was below +the visible part of the source dropdown. The combo-scoped search rejected it, +then a keyboard fallback foregrounded the disabled main form instead of the +modal review. Phase C had not exposed this because its `iso-8859-1` choice was +inside the visible part of the same dropdown. Both combo-scoped and process-wide +exact-name searches now follow the same documented rule, and fallback input +uses the supplied window. Phase J drives the source-choice refusal against the +built application. + +## Decisions and mistakes that must remain visible + +### A known defect shipped after being reported closed + +EC-06 was present before v3.11.0, recorded as confirmed, and reported as closed. +It shipped broken in v3.11.0 and v3.11.1, then was rediscovered from scratch +during unrelated work. Neither the corpus audit nor the GUI suite covered a +drive-root plan. The record failed because it trusted a summary instead of the +source. + +### A correction was recorded for code that should not change + +CX-07 said old plans, journals, and reports had been excluded. No such change +had been made, and `docs/CLI.md` intentionally promises the opposite. The false +record was corrected on 2026-09-07 only after the row was re-derived from code. + +### Aggregate counts drifted repeatedly + +The open count had already stopped reconciling with its rows three times. The +2026-09-08 recheck found a separate error: “27 fixed” counted CX-07 as fixed +although the ledger called its alleged behavior not a defect. The current header +is generated from the canonical rows, and `Test-DefectBacklog.ps1` fails if it +drifts again. + +### A later review over-rated four of its own findings + +The independent review of `74d5b3d` that produced BL-06 through BL-17 correctly +found mechanisms, then described their importance before checking realistic +product behavior. The user caught all four corrections: two findings were +downgraded after measurement, one proposed correction was rejected when it +broke four existing tests, and one finding was withdrawn after checking the +actual CSV. This is kept visible because a true mechanism does not automatically +justify the claimed product risk. + +### Three hashing optimisations were measured and rejected + +Conversion can read the same file several times, but each read answers a +different question: does the source still match approval, did the backup really +land, and does the installed output contain the verified text? Three throwaway +variants were interleaved against the same 292 MiB, 60-file workload with backup +and journal enabled: + +| Variant | Measurement | Safety or usability cost | +|---|---|---| +| Baseline | 1030 ms median | None | +| Digest backup while copying | 872 ms; 15.3% faster | Nothing independently re-reads the restore point on disk | +| Hash source and output while streaming | 3.5% faster in its own batch | Both values derive from intended I/O rather than independent reads | +| XxHash128 instead of SHA-256 | 1078 ms; 4.7% slower | Recorded hashes lose `Get-FileHash` interoperability and collision resistance | + +The source can be read up to seven times per converted file, and one hash is +computed twice over bytes already held in memory. Those reads are still not +redundant. The only materially faster variant removed the backup re-read after +`Flush(flushToDisk: true)`, which is the only independent check that the restore +point exists intact. XxHash128 itself measured 16,447 MiB/s against SHA-256 at +2,429 MiB/s—6.8x—but made the parallel I/O-bound workload slower. On that +machine SHA-256 also beat SHA-1 (981 MiB/s), MD5 (754 MiB/s), and SHA-512 +(805 MiB/s). + +These figures are conditional: a 24-core machine, fast local disk, warm cache, +and eight workers. Cold or network storage may increase the benefit of removing +a read while also increasing the value of verifying it. Even a different speed +result—even 40%—would not change what each check proves. The observed ceiling +was about 15%, and that was the variant which removed the strongest backup +evidence. Only a single-worker run on a slow CPU is likely to make hashing itself +visible. Future throughput work should make the backup re-read cheaper rather +than delete it. + +## The source-choice refusal is covered by both a unit test and smoke phase J + +Before BL-04 was addressed, a user could scan one directory, point the main +window at another without scanning again, tick a refused file, choose an +encoding, and confirm. The review closed and the status said “Conversion +cancelled. No files were modified.” The user's choice had been discarded. + +The unit test constructs that state directly. Smoke phase J drives it through +the built application: the review must remain open, name +`..\scanned\french.txt`, explain that it is no longer inside the review, and +leave every source byte unchanged. BL-26 records the automation-driver defect +found while making that phase reliable. + +## 2026-09-08 recheck record + +Every pre-reformat row was checked against source, a current regression test, a +current Release-build probe, or a clearly marked historical measurement. The +source inspection used `c071c10`; the factual recheck is preserved as local +commit `902f567` immediately before this reformat. + +For chronology, BL-06 through BL-17 came from the independent review of +`74d5b3d` after v3.11.2. BL-22 through BL-24 came from a later independent review +of released commit `518a844` after v3.12.0. These source identities are retained +because the observations were made against those builds, even though the +current statuses were rechecked against `c071c10`. + +Current behavioral probes established: + +- the EC-08 hostile regex exceeded a three-second child-process budget; +- automatic and explicit UTF-8 paths both refused a double BOM; +- an old plan in the scan root was scanned as ASCII; +- a filename beginning `=1+1` still produced an absolute-path CSV cell; +- BL-01 and BL-18 both changed Unicode under the wrong automatic interpretation; +- BL-19 and BL-21 were reported inconsistently but refused or failed before a + write; and +- both names of a hard-linked file preserved exact text and received separate + recovery artifacts through the normal Windows replacement path. + +No old row was skipped. Three portions remain only partly reproducible: + +- BL-05 requires precise force-close timing and was inspected rather than + triggered; +- BL-20's move fallback requires a platform where `File.Replace` is unsupported; + and +- the historical timing figures above were not rerun; their current code and + safety properties were checked. + +## Pre-reformat row mapping + +The task expected 78 rows, 45 with an existing ID and 33 without one. The exact +`c071c10` input had 79 data-shaped rows, 45 of which mentioned an existing ID. +The disjoint breakdown is 35 original canonical rows, 21 unique anonymous +finding or decision rows, 13 duplicate or correction rows, 7 benchmark or +comparison-evidence rows, and 3 malformed blank-cell headers. Four findings +existed only as prose, and one GUI-driver finding existed only in a narrative +section. That yields the 61 unique ledger entries above. The mapping below +accounts for every old location without pretending that a table header or +benchmark variant is a new defect. + +| Old location | Canonical finding or destination | Note | |---|---|---| -| 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`. | +| R001 | [EC-01](#ec-01) | Original ledger row | +| R002 | [EC-02](#ec-02) | Original ledger row | +| R003 | [EC-03](#ec-03) | Original ledger row | +| R004 | [EC-04](#ec-04) | Original ledger row | +| R005 | [EC-05](#ec-05) | Original ledger row | +| R006 | [EC-06](#ec-06) | Original ledger row | +| R007 | [EC-07](#ec-07) | Original ledger row | +| R008 | [EC-08](#ec-08) | Original ledger row | +| R009 | [EC-09](#ec-09) | Original ledger row | +| R010 | [EC-10](#ec-10) | Original ledger row | +| R011 | [EC-11](#ec-11) | Original ledger row | +| R012 | [EC-12](#ec-12) | Original ledger row | +| R013 | [EC-13](#ec-13) | Original ledger row | +| R014 | [EC-14](#ec-14) | Original ledger row | +| R015 | [EC-15](#ec-15) | Original ledger row | +| R016 | [EC-16](#ec-16) | Original ledger row | +| R017 | [EC-17](#ec-17) | Original ledger row | +| R018 | [EC-18](#ec-18) | Original ledger row | +| R019 | [EC-19](#ec-19) | Original ledger row | +| R020 | [EC-20](#ec-20) | Original ledger row | +| R021 | [EC-21](#ec-21) | Original ledger row | +| R022 | [EC-22](#ec-22) | Original ledger row | +| R023 | [EC-23](#ec-23) | Original ledger row | +| R024 | [CX-01](#cx-01) | Original ledger row | +| R025 | [CX-02](#cx-02) | Original ledger row | +| R026 | [CX-03](#cx-03) | Original ledger row | +| R027 | [CX-05](#cx-05) | Original ledger row | +| R028 | [CX-06](#cx-06) | Original ledger row | +| R029 | [CX-07](#cx-07) | Original ledger row | +| R030 | [CX-08](#cx-08) | Original ledger row | +| R031 | [CX-09](#cx-09) | Original ledger row | +| R032 | [CX-10](#cx-10) | Original ledger row | +| R033 | [CX-11](#cx-11) | Original ledger row | +| R034 | [CX-12](#cx-12) | Original ledger row | +| R035 | [CX-13](#cx-13) | Original ledger row | +| R036 | [BL-01](#bl-01) | Previously anonymous finding | +| R037 | [BL-02](#bl-02) | Previously anonymous finding | +| R038 | [BL-03](#bl-03) | Previously anonymous finding | +| R039 | [BL-04](#bl-04) | Previously anonymous finding | +| R040 | [BL-05](#bl-05) | Previously anonymous finding | +| R041 | [BL-06](#bl-06) | Previously anonymous finding | +| R042 | [BL-07](#bl-07) | Previously anonymous finding | +| R043 | [BL-08](#bl-08) | Previously anonymous finding | +| R044 | [BL-09](#bl-09) | Previously anonymous finding | +| R045 | [BL-10](#bl-10) | Previously anonymous finding | +| R046 | [BL-11](#bl-11) | Previously anonymous finding | +| R047 | [BL-12](#bl-12) | Previously anonymous finding | +| R048 | [BL-13](#bl-13) | Previously anonymous finding | +| R049 | [BL-14](#bl-14) | Previously anonymous finding | +| R050 | [BL-15](#bl-15) | Previously anonymous finding | +| R051 | [BL-16](#bl-16) | Previously anonymous finding | +| R052 | [BL-17](#bl-17) | Previously anonymous finding | +| R053 | [BL-22](#bl-22) | Previously anonymous finding | +| R054 | [BL-23](#bl-23) | Previously anonymous finding | +| R055 | [BL-24](#bl-24) | Previously anonymous finding | +| R056 | [CX-07](#cx-07) | Duplicate correction row | +| R057 | [BL-02](#bl-02) | Duplicate withdrawal row | +| R058 | [Hashing measurements](#three-hashing-optimisations-were-measured-and-rejected) | Baseline data, not a finding | +| R059 | [Hashing measurements](#three-hashing-optimisations-were-measured-and-rejected) | Backup-read variant, not a separate finding | +| R060 | [Hashing measurements](#three-hashing-optimisations-were-measured-and-rejected) | Streaming-hash variant, not a separate finding | +| R061 | [Hashing measurements](#three-hashing-optimisations-were-measured-and-rejected) | XxHash variant, not a separate finding | +| R062 | [BL-25](#bl-25) | Malformed comparison-table header, not a finding | +| R063 | [BL-25](#bl-25) | Raw-hash comparison evidence | +| R064 | [BL-25](#bl-25) | Content-digest comparison evidence | +| R065 | [BL-25](#bl-25) | Backup-comparison evidence | +| R066 | [Canonical ledger](#canonical-ledger) | Malformed score-table header, not a finding | +| R067 | [CX-06](#cx-06) | Duplicate score row | +| R068 | [BL-01](#bl-01) | Duplicate score row | +| R069 | [EC-08](#ec-08) | Duplicate score row | +| R070 | [BL-02](#bl-02) | Duplicate score row | +| R071 | [EC-16](#ec-16) | Duplicate score row | +| R072 | [EC-20](#ec-20) | Duplicate score row | +| R073 | [EC-15](#ec-15) | Duplicate score row | +| R074 | [EC-17](#ec-17) | Duplicate score row | +| R075 | [EC-23](#ec-23) | Duplicate score row | +| R076 | [EC-18](#ec-18) | Duplicate score row | +| R077 | [BL-25](#bl-25) | Design-decision row | +| R078 | [EC-19](#ec-19) | Malformed not-reproduced table header, not a finding | +| R079 | [EC-19](#ec-19) | Duplicate evidence row | +| P001 | [BL-18](#bl-18) | Former prose-only finding | +| P002 | [BL-19](#bl-19) | Former prose-only finding | +| P003 | [BL-20](#bl-20) | Former prose-only finding | +| P004 | [BL-21](#bl-21) | Former prose-only finding | +| S001 | [BL-26](#bl-26) | Former narrative-only GUI-driver finding | diff --git a/docs/GUI-SMOKE-TEST.md b/docs/GUI-SMOKE-TEST.md index 703ea82..441c1f7 100644 --- a/docs/GUI-SMOKE-TEST.md +++ b/docs/GUI-SMOKE-TEST.md @@ -1,6 +1,6 @@ # The GUI smoke test -Nine phases that drive a built `EncodingChecker.exe` through Windows UI Automation +Ten phases that drive a built `EncodingChecker.exe` through Windows UI Automation and check the bytes it leaves behind. Every phase creates its own disposable folder, performs a real sequence in the real window, and then verifies files — never status messages. @@ -13,7 +13,7 @@ sources/EncodingChecker.GuiSmoke/bin/Release/net10.0-windows/EncodingChecker.Gui ``` --app the executable to drive; defaults to the Release build --output where evidence is written; must be empty or new ---phase run one phase +--phase run one phase --keep-workspace keep the fixtures even when the run passes ``` @@ -52,6 +52,7 @@ because nothing ran the sequence. | **G** | When a backup cannot be created the source is untouched and no recovery record is written. | Converting without the restore point the run promised. | | **H** | A source choice that **agrees** with an unprovable byte order is still flagged, saying the order was taken on trust and *not* that it differs from your choice. | A warning that fires for the safer choice and stays silent for the riskier one. | | **I** | Cancelling a 400-file run mid-write reports what it actually wrote: the converted count equals the files whose BOM is gone, and unreached files are reported as not attempted. | A cancelled run claiming it changed nothing, or claiming the whole batch converted. | +| **J** | After View, changing the directory without rescanning leaves out-of-directory results visible. Confirming a source encoding keeps the review open, explains why it cannot apply the choice, and changes no bytes. | A ticked source choice being discarded as an unexplained cancellation, or the automation driver failing to reach that path. | ### Notes on two of them @@ -120,7 +121,7 @@ exits `0`, because every phase in an empty set passes. The evidence the run uplo is what settles it: one phase recorded, `A`, with five files hashed before and five after. Read the artifact, not the tick. -**It now gates the release.** `release.yml` runs all nine phases against the signed, +**It now gates the release.** `release.yml` runs all ten phases against the signed, published executable, after signing and before packaging, so what is verified is the bytes that ship rather than a rebuild of the same commit. A failure fails the job and no release is created. The report is uploaded as a `gui-smoke-evidence` artifact. diff --git a/docs/RELEASE-CHECKLIST.md b/docs/RELEASE-CHECKLIST.md index 6a010d7..0d1468a 100644 --- a/docs/RELEASE-CHECKLIST.md +++ b/docs/RELEASE-CHECKLIST.md @@ -47,7 +47,7 @@ description that turned out to be wrong. ### Running it -Nine phases drive the built executable through Windows UI Automation and verify the +Ten phases drive the built executable through Windows UI Automation and verify the resulting bytes. ```powershell @@ -59,7 +59,7 @@ Exit 0 is a pass. Each run writes `gui-smoke-report.json` and `gui-smoke-report. carrying the EC version, the executable and managed-assembly hashes, and every phase's before and after file hashes. -**[What each of the nine phases proves, and what it would catch →](GUI-SMOKE-TEST.md)** +**[What each of the ten phases proves, and what it would catch →](GUI-SMOKE-TEST.md)** **The release workflow runs this for you, and a failure stops the release.** It drives the signed, published executable — the bytes that ship, not a rebuild of the same @@ -86,7 +86,7 @@ checks the status line *against* the bytes on disk rather than trusting it. ### Record -The nine phases record themselves. `gui-smoke-report.md` and `gui-smoke-report.json` +The ten phases record themselves. `gui-smoke-report.md` and `gui-smoke-report.json` already carry the EC version, the executable and managed-assembly hashes, the OS and .NET versions, and every phase's before and after file hashes — better evidence than a transcribed letter, and not subject to a typo. Keep both files with the release. diff --git a/docs/RELEASE-NOTES-v3.12.1.md b/docs/RELEASE-NOTES-v3.12.1.md new file mode 100644 index 0000000..6e742c6 --- /dev/null +++ b/docs/RELEASE-NOTES-v3.12.1.md @@ -0,0 +1,199 @@ +# EncodingChecker v3.12.1 + +Two defects from an independent review of the released v3.12.0, one refactor taken +because the inconsistency it describes was the finding, and two rows of the backlog that +did not survive being re-derived from the source. + +## An unwritable output destination is refused before the run, not after + +The journal and the report are written once scanning has returned. A destination that +could never be written was therefore discovered *after* conversion had rewritten the +files — so a user who asked for a journal ended with changed files and no record of the +change, which is the one outcome a journal exists to prevent. + +Measured on all four shapes of the mistake: `-Journal` or `-Report`, under a missing +directory or onto an existing one. All four converted first and failed afterwards. + +The check now runs in `RunConsoleMode`, after option validation and before either mode +dispatches, so one place covers `-Apply` and every scan mode. + +**The exit code is deliberately unchanged.** `docs/CLI.md` assigns 3 to a report failure, +and the test that pins it states the reasoning: a report that cannot be written is a +processing failure, not a usage error. Putting the check among the usage validators made +it exit 1 and broke that contract for no benefit. The defect is *when* the failure is +found, not what it is called. + +It does not probe by creating a file. That would leave one behind on every path that then +fails, and it still could not promise the later write — the disk can fill and permissions +can change in between. It removes the two mistakes a caller can make before the run +starts, which is what the four cases were. + +## A plan action no build ever wrote is refused, not reported as converted + +`System.Text.Json` accepts a number for any enum, so a damaged or hand-edited plan could +carry `"Action": 99` and load without complaint. It then reached the mapping from planned +action to report result, whose fallback arm was `Converted`. + +Measured before the fix: `-Apply` exited **0**, printed "1 selected, 1 converted, 0 +failed", and wrote a journal recording `Status=Converted` and `PlannedAction=99` — while +the source file's hash was unchanged. The journal asserted work that never happened, which +is worse than a missing journal, because this project treats the journal as its audit +trail. + +Two changes, because either alone leaves the other half reachable. Loading a plan now +rejects an undefined action or source interpretation before any source is touched. The +mapping names every action and throws for anything else: a value that cannot arise from a +decision this build made has no report result, and guessing `Converted` is the guess that +makes the report lie. + +## EC writes its own artifacts the way it writes everyone else's + +EC installs a converted file by writing a temporary file beside it and replacing the +original, and it writes its recovery sidecar the same way, with a read-back check. Its +plan, journal, report and settings did not: each truncated its destination and then wrote +into it, so an interruption left a half-written artifact where a readable one had been. + +The inconsistency is the finding. EC had already decided this matters and had already +built the mechanism; four of its own artifacts did not use it. + +The plan is the worst case — a truncated plan destroys the reviewed plan a user was about +to apply, and re-running `-Plan` produces one nobody has reviewed. The settings file is +the case already known to have failed this way, recorded as EC-16 and closed by this +change rather than on its own account. + +All four now write through one shared writer, into a temporary file beside the destination +under the suffix scans already exclude, and then through the existing replacement. The +recovery sidecar keeps its own writer: it also reads back and verifies what it wrote, +which is more than the shared one does and should not be reduced to it. + +The bytes are unchanged. A report produced by this build is byte-identical to one produced +by v3.12.0, BOM included. + +## Two backlog rows corrected + +`CX-07` was recorded as fixed, with a note describing a fix that was never made and should +not be: `docs/CLI.md` states plainly that EC does not exclude plans, journals or reports +left by earlier runs, and that is deliberate. What is excluded is `.bak`, `.ecmeta.json`, +temporary conversion files, and the output paths of the running command. This is the same +failure as the drive-root defect — a record trusting a summary instead of the source — so +the row now says the record was wrong rather than quietly restating the behaviour. + +CSV formula injection is **withdrawn**: it does not reproduce. Every file is resolved +through `Path.GetFullPath`, so the `File` column always begins with a drive letter or a +UNC prefix and can never begin with `=`, `+`, `-` or `@`. + +The backlog's summary line also stopped reconciling with the table it summarises, so it +now states a figure counted from the rows and says that it was counted. + +## The backlog is a ledger you can check + +`docs/DEFECT-BACKLOG.md` exists because a status summary drifted from the code and a +defect recorded as closed shipped broken in two releases. It had since drifted twice more, +in smaller ways, and both were found by hand. + +Every entry has now been re-derived from the source rather than carried forward: **61 +unique findings — 44 fixed, 13 open, 1 not reproduced, 1 withdrawn, 1 not a defect, and 1 +design decision.** It is organised by status rather than by discovery date, so the open +work is in one place instead of spread across five chronological tables, and every finding +has a stable id — including the thirty-three that previously had none and could not be +cited at all. + +The recheck found one more error in the old summary, this one introduced while correcting +the previous one: the count of fixed findings included CX-07, which the same review had +just reclassified as *not a defect*. + +That is three drifts in a hand-maintained figure, so it is no longer hand-maintained. +`docs/Test-DefectBacklog.ps1` recomputes every number in the header from the rows, and +checks that ids are unique, statuses are known, every open finding carries both impact and +reach, and every detail link resolves. + +Three limits are recorded rather than papered over: one open finding needs force-close +timing too precise to trigger and was inspected instead, one needs a filesystem where +`File.Replace` is unsupported, and the historical performance figures were not re-measured +— their current code and safety properties were checked instead. + +The checker itself shipped with an encoding defect, which in this project is worth +recording rather than quietly fixing. It was UTF-8 without a BOM and contained em-dashes, +so Windows PowerShell read it in the system ANSI codepage and it would not parse. One of +those em-dashes was a *value* — the marker the ledger writes for an unscored finding — so +repairing only the line the error pointed at would have left that comparison matching +mojibake, in the file whose purpose is being checkable. It is now pure ASCII and runs +under both shells. + +The same reasoning was applied to the code comments this release added. Several of them +retold the story of the defect they sat beside - a story already recorded in the commit +message, in these notes, and in the backlog. That is a fourth copy that nothing keeps in +sync, which is the drift problem in a different place. The comments were cut roughly in +half, keeping only what a reader with the code in front of them could not derive: why the +output check returns exit 3 rather than 1, why the writer does not probe by creating a +file, why it flushes to disk, why the recovery sidecar keeps its own writer, and why an +unknown planned action throws instead of being reported as a conversion. + +## The source-choice refusal is covered by the smoke suite, not only a unit test + +One EC fix has been carried by a unit test rather than a smoke phase since v3.11.2, and +the reason was a defect in the smoke driver rather than in EC. + +The sequence it covers: scan a directory, point the window at a different one **without +scanning again**, tick the refused file, choose a source encoding, confirm. That used to +close the review and report *"Conversion cancelled. No files were modified."* — a +cancellation nobody asked for, after the user had ticked files and chosen an encoding. The +behaviour was fixed in v3.11.2. What was missing was a phase that would notice if it came +back. + +The phase could not be written, because selecting the source encoding timed out while the +identical call in another phase succeeded. The cause was two failures in sequence. The +item this phase needs sits below the visible part of the dropdown, and UI Automation +reports it offscreen, so the driver's exact-item search rejected it; control then fell +through to a keyboard fallback that brought the *main* window to the front, which is the +wrong target while a modal review is open. The phase that worked chose an encoding that +happened to be visible, so it never reached either failure. + +**The suite is now ten phases.** Phase J drives the sequence against the built +application and requires the review to stay open, to name the file that falls outside its +directory, and to leave every source byte unchanged. + +No production code changed to make this drivable. The driver was fixed; EC was not +reshaped to be easier to test, which is the trade this suite exists to avoid making. + +The unit test stays. It checks the form's decision without needing an interactive desktop, +which is a different question from whether the window behaves, and the unit suite must +keep running without one. + +## Compatibility + +Conversion semantics stay at **6**, the plan schema at **5**, the journal schema at **4**. +No detection or conversion policy changes: what EC converts, refuses, and reports for a +valid input is exactly what v3.12.0 did. + +One exit code changes, for one case: + +| Situation | Before | After | +|---|---|---| +| `-Apply` on a plan carrying an undefined action | `Converted`, 0 | refused, 1 | + +That 0 was a false success. Every other exit code, reason code, report column and journal +field is unchanged, and a valid plan written by v3.12.0 applies exactly as before. + +## Verification + +- 742 tests pass, none skipped (was 727); release build with no warnings +- `docs/Test-DefectBacklog.ps1` passes under both Windows PowerShell and pwsh 7, + and was itself checked by changing a header count and requiring it to fail +- The GUI smoke suite is now **ten phases**, all passing against the built executable. + The new phase was mutation-checked the same way as the code fixes: the production guard + it covers was reverted, the phase was required to fail — it did, reporting that the + review closed after refusing an unusable source choice — and the file was restored + byte-identical and confirmed by hash +- Each fix was mutation-checked — the change reverted, the intended tests required to + fail, the file restored byte-identical and confirmed by SHA-256 +- Both defects were reproduced end to end before the fix and re-run after: the output + destination case now exits 3 with the source hash unchanged, and the damaged plan is + refused with no journal written +- Report output was compared byte-for-byte against v3.12.0's, including the BOM +- Flushing each artifact to disk before installing it was measured over 2,000 files and a + 402 KB report: 152–153 ms with the flush, 153–154 ms without — no measurable cost +- **No four-corpus audit was run.** This release changes no detection or conversion + policy, so the checklist does not require one. The gap v3.12.0 recorded — a release that + did change `ConversionPolicy` without a corpus run — is unchanged by this one and still + stands. diff --git a/docs/Test-DefectBacklog.ps1 b/docs/Test-DefectBacklog.ps1 new file mode 100644 index 0000000..18a971d --- /dev/null +++ b/docs/Test-DefectBacklog.ps1 @@ -0,0 +1,183 @@ +[CmdletBinding()] +param( + [string] $Path +) + +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' + +# Resolved here rather than as a parameter default: Windows PowerShell does not +# populate $PSScriptRoot while evaluating defaults under -File, so the documented +# invocation failed on the only shell guaranteed to be installed. +if (-not $Path) { + $scriptDirectory = $PSScriptRoot + + if (-not $scriptDirectory) { + $scriptDirectory = Split-Path -Parent $MyInvocation.MyCommand.Path + } + + $Path = Join-Path $scriptDirectory 'DEFECT-BACKLOG.md' +} + +$resolvedPath = (Resolve-Path -LiteralPath $Path).Path +$content = [IO.File]::ReadAllText($resolvedPath) +$errors = [Collections.Generic.List[string]]::new() + +$countMarker = [regex]::Match( + $content, + '') + +if (-not $countMarker.Success) { + throw 'The backlog-counts marker is missing.' +} + +$expected = @{} + +foreach ($token in $countMarker.Groups['pairs'].Value.Split( + ' ', + [StringSplitOptions]::RemoveEmptyEntries)) { + $parts = $token.Split('=', 2) + $value = 0 + + if ($parts.Length -ne 2 -or + -not [int]::TryParse($parts[1], [ref] $value) -or + $value -lt 0) { + throw "Invalid count token '$token'." + } + + $expected[$parts[0]] = $value +} + +$startMarker = '' +$endMarker = '' +$start = $content.IndexOf($startMarker, [StringComparison]::Ordinal) +$end = $content.IndexOf($endMarker, [StringComparison]::Ordinal) + +if ($start -lt 0 -or $end -le $start) { + throw 'The canonical ledger markers are missing or out of order.' +} + +$ledger = $content.Substring( + $start + $startMarker.Length, + $end - ($start + $startMarker.Length)) + +$rows = foreach ($line in [regex]::Split($ledger, '\r?\n')) { + if ($line -notmatch '^\|\s*((?:EC|CX|BL)-\d{2})\s*\|') { + continue + } + + $cells = @( + $line.Trim().Trim('|').Split('|') | + ForEach-Object { $_.Trim() } + ) + + if ($cells.Count -ne 6) { + $errors.Add("$($Matches[1]) has $($cells.Count) cells; expected 6.") + continue + } + + [pscustomobject] @{ + Id = $cells[0] + Finding = $cells[1] + Status = $cells[2] + Impact = $cells[3] + Reach = $cells[4] + Details = $cells[5] + } +} + +# The ledger writes an em-dash where a finding carries no score. Built from its code +# point rather than typed: Windows PowerShell reads a BOM-less .ps1 as the system ANSI +# codepage, so a literal here parses as three bytes and breaks the string containing it. +$EmDash = [string][char]0x2014 + +$knownStatuses = @( + 'Fixed', + 'Open', + 'Not reproduced', + 'Withdrawn', + 'Not a defect', + 'Decision' +) + +foreach ($duplicate in $rows | Group-Object Id | Where-Object Count -gt 1) { + $errors.Add("Duplicate finding ID: $($duplicate.Name).") +} + +foreach ($row in $rows) { + if ($row.Status -notin $knownStatuses) { + $errors.Add("$($row.Id) has unknown status '$($row.Status)'.") + } + + if ($row.Status -eq 'Open' -and + ($row.Impact -in @('', '-', $EmDash) -or $row.Reach -in @('', '-', $EmDash))) { + $errors.Add("$($row.Id) is open but lacks impact or reach.") + } + + $expectedLink = "[$($row.Id)](#$($row.Id.ToLowerInvariant()))" + + if ($row.Details -ne $expectedLink) { + $errors.Add("$($row.Id) must link to its exact detail heading.") + } + + $headingPattern = '(?m)^###\s+' + [regex]::Escape($row.Id) + '\s*$' + + if (-not [regex]::IsMatch($content, $headingPattern)) { + $errors.Add("$($row.Id) has no exact detail heading.") + } +} + +$actual = @{ + total = @($rows).Count + fixed = @($rows | Where-Object Status -eq 'Fixed').Count + open = @($rows | Where-Object Status -eq 'Open').Count + 'not-reproduced' = @($rows | Where-Object Status -eq 'Not reproduced').Count + withdrawn = @($rows | Where-Object Status -eq 'Withdrawn').Count + 'not-a-defect' = @($rows | Where-Object Status -eq 'Not a defect').Count + decision = @($rows | Where-Object Status -eq 'Decision').Count +} + +foreach ($key in $expected.Keys) { + if (-not $actual.ContainsKey($key)) { + $errors.Add("The count marker has unknown key '$key'.") + } +} + +foreach ($key in $actual.Keys) { + if (-not $expected.ContainsKey($key)) { + $errors.Add("The count marker has no '$key' value.") + continue + } + + if ($actual[$key] -ne $expected[$key]) { + $errors.Add( + "Count mismatch for ${key}: header $($expected[$key]), ledger $($actual[$key]).") + } +} + +$summary = "**Derived count: $($actual.total) findings $EmDash $($actual.fixed) fixed, " + + "$($actual.open) open, $($actual['not-reproduced']) not reproduced, " + + "$($actual.withdrawn) withdrawn, $($actual['not-a-defect']) not a defect, " + + "and $($actual.decision) design decision.**" +$normalizedContent = [regex]::Replace($content, '\s+', ' ') + +if (-not $normalizedContent.Contains($summary)) { + $errors.Add('The human-readable derived count does not match the ledger.') +} + +Write-Output "Defect backlog: $($actual.total) finding(s)" + +foreach ($status in $knownStatuses) { + $count = @($rows | Where-Object Status -eq $status).Count + Write-Output (" {0,-16} {1}" -f ($status + ':'), $count) +} + +if ($errors.Count -gt 0) { + foreach ($message in $errors) { + Write-Error $message + } + + exit 1 +} + +Write-Output "PASS: counts, IDs, scores, and detail links reconcile." diff --git a/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs b/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs index 909b0d0..0326a91 100644 --- a/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs +++ b/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs @@ -37,6 +37,24 @@ internal EcGuiDriver(string executable) } internal AutomationElement OpenReview(string directory, int expectedFiles) + { + ConfigureScan(directory); + Scan(expectedFiles); + return OpenSelectedReview(expectedFiles); + } + + internal AutomationElement OpenReviewAfterRetarget( + string scannedDirectory, + string reviewDirectory, + int expectedFiles) + { + ConfigureScan(scannedDirectory); + Scan(expectedFiles); + SetText(MainWindow, "lstBaseDirectory", reviewDirectory); + return OpenSelectedReview(expectedFiles); + } + + private void ConfigureScan(string directory) { SetText(MainWindow, "lstBaseDirectory", directory); SetText(MainWindow, "txtFileMasks", "*"); @@ -44,12 +62,18 @@ internal AutomationElement OpenReview(string directory, int expectedFiles) SetToggle(MainWindow, "chkCreateBackup", true); SetToggle(MainWindow, "chkPreviewChanges", false); SelectCombo(MainWindow, "lstConvert", "utf-8"); + } + private void Scan(int expectedFiles) + { Invoke(MainWindow, "btnView"); WaitUntil( () => ResultCount() == expectedFiles && IsEnabled(MainWindow, "btnView"), $"View did not finish with {expectedFiles} result row(s)."); + } + private AutomationElement OpenSelectedReview(int expectedFiles) + { SetToggle(MainWindow, "chkSelectDeselectAll", true); WaitUntil( () => CheckedResultCount() == expectedFiles, @@ -61,6 +85,28 @@ internal AutomationElement OpenReview(string directory, int expectedFiles) return WaitForReview(); } + internal void TryConfirmSource( + AutomationElement review, + string sourceEncoding, + params string[] filesToCheck) + { + foreach (string file in filesToCheck) + SetRefusedFileChecked(review, file, true); + + SelectCombo(review, "lstSourceEncoding", sourceEncoding); + Invoke(review, "btnConfirmSourceEncoding"); + } + + internal bool ReviewIsOpen(AutomationElement review) => + FindReviewWindow() is { } current && + current.Current.NativeWindowHandle == review.Current.NativeWindowHandle; + + internal void WaitForReviewText(AutomationElement review, string expected) => + WaitUntil( + () => ReviewIsOpen(review) && + ReviewText(review).Contains(expected, StringComparison.OrdinalIgnoreCase), + $"The review did not show '{expected}'."); + internal AutomationElement ConfirmSource( AutomationElement review, string sourceEncoding, @@ -308,7 +354,7 @@ private void SelectCombo( } AutomationElement? item = WaitFor( - () => FindNamedItem(combo, value) ?? FindProcessItem(value), + () => FindNamedItem(combo, value) ?? FindVisibleProcessItem(value), TimeSpan.FromSeconds(5)); if (item is not null && @@ -318,7 +364,7 @@ private void SelectCombo( } else { - SetForegroundWindow(MainWindow.Current.NativeWindowHandle); + SetForegroundWindow(root.Current.NativeWindowHandle); combo.SetFocus(); System.Windows.Forms.SendKeys.SendWait(value); System.Windows.Forms.SendKeys.SendWait("{ENTER}"); @@ -336,13 +382,14 @@ private void SelectCombo( AutomationElementCollection items = root.FindAll( TreeScope.Descendants, Condition.TrueCondition); + // UIA can select WinForms items that sit below the popup viewport and are + // therefore reported offscreen. return items.Cast().FirstOrDefault(element => element.Current.ControlType == ControlType.ListItem && - element.Current.Name.Equals(value, StringComparison.OrdinalIgnoreCase) && - !element.Current.IsOffscreen); + element.Current.Name.Equals(value, StringComparison.OrdinalIgnoreCase)); } - private AutomationElement? FindProcessItem(string value) + private AutomationElement? FindVisibleProcessItem(string value) { var condition = new AndCondition( new PropertyCondition( @@ -355,6 +402,8 @@ private void SelectCombo( AutomationElementCollection items = AutomationElement.RootElement.FindAll( TreeScope.Descendants, condition); + // A hidden process-wide match may belong to another collapsed combo that + // contains the same encoding name, so only its visible popup is safe to use. return items.Cast().FirstOrDefault(element => element.Current.Name.Equals(value, StringComparison.OrdinalIgnoreCase) && !element.Current.IsOffscreen); diff --git a/sources/EncodingChecker.GuiSmoke/Program.cs b/sources/EncodingChecker.GuiSmoke/Program.cs index 3164e5b..2eadb23 100644 --- a/sources/EncodingChecker.GuiSmoke/Program.cs +++ b/sources/EncodingChecker.GuiSmoke/Program.cs @@ -169,7 +169,7 @@ private sealed record Options( { internal const string Usage = "Usage: EncodingChecker.GuiSmoke [--app ] " - + "[--output ] [--phase ] [--keep-workspace]"; + + "[--output ] [--phase ] [--keep-workspace]"; internal static Options Parse(string[] args) { @@ -198,8 +198,8 @@ internal static Options Parse(string[] args) phase = TakeValue(args, ref i, "--phase").ToUpperInvariant(); if (phase is not ("A" or "B" or "C" or "D" or "E" or "F" or "G" - or "H" or "I")) - throw new ArgumentException("--phase must be one letter from A to I."); + or "H" or "I" or "J")) + throw new ArgumentException("--phase must be one letter from A to J."); break; default: throw new ArgumentException($"Unknown option: {args[i]}"); diff --git a/sources/EncodingChecker.GuiSmoke/SmokeSuite.cs b/sources/EncodingChecker.GuiSmoke/SmokeSuite.cs index 378c66d..e471800 100644 --- a/sources/EncodingChecker.GuiSmoke/SmokeSuite.cs +++ b/sources/EncodingChecker.GuiSmoke/SmokeSuite.cs @@ -67,6 +67,7 @@ internal SmokeReport Run(string? onlyPhase = null) RunIf("G", "Backup failure leaves the source unchanged", PhaseG); RunIf("H", "A source choice matching an unprovable estimate is flagged", PhaseH); RunIf("I", "An interrupted run reports what it actually wrote", PhaseI); + RunIf("J", "An out-of-directory source choice is refused visibly", PhaseJ); return new SmokeReport { @@ -421,6 +422,51 @@ private void PhaseI(PhaseContext phase) } } + /// + /// A source choice that cannot be mapped back to the review's directory stays visible. + /// + /// + /// The main window can be retargeted after View without replacing its old results. + /// Those rows then appear outside the new directory. They used to be dropped when + /// the user confirmed an encoding, which closed the review as an unexplained + /// cancellation. This phase drives that exact sequence and checks the files rather + /// than trusting the dialog text alone. + /// + private void PhaseJ(PhaseContext phase) + { + string scanned = Directory.CreateDirectory( + Path.Combine(phase.Directory, "scanned")).FullName; + string retargeted = Directory.CreateDirectory( + Path.Combine(phase.Directory, "retargeted")).FullName; + Write(scanned, "french.txt", "Prix: 100€ pour le café", CodePage("windows-1252")); + + Dictionary before = phase.CaptureBefore(); + + using var gui = new EcGuiDriver(_app); + System.Windows.Automation.AutomationElement review = + gui.OpenReviewAfterRetarget(scanned, retargeted, 1); + + const string outsideFile = @"..\scanned\french.txt"; + Check( + gui.ReviewText(review).Contains(outsideFile, StringComparison.OrdinalIgnoreCase), + "The review did not identify the file outside its current directory."); + + gui.TryConfirmSource(review, "windows-1252", outsideFile); + Check(gui.ReviewIsOpen(review), + "The review closed after refusing an unusable source choice."); + gui.WaitForReviewText(review, "no longer inside this review"); + + Check(gui.ReviewIsOpen(review), + "The review closed after refusing an unusable source choice."); + Check( + gui.ReviewText(review).Contains(outsideFile, StringComparison.OrdinalIgnoreCase), + "The refusal no longer named the file outside the review directory."); + + gui.CancelReview(review); + AssertSameFiles(before, Snapshot(phase.Directory)); + AssertNoArtifacts(phase.Directory); + } + /// Files whose byte-order mark has been stripped, so they were written. private static int RewrittenCount(string directory) => Directory.EnumerateFiles(directory, "file-*.txt") @@ -549,7 +595,8 @@ private static void AssertSameFiles( private static void AssertNoArtifacts(string directory) { - string[] artifacts = Directory.GetFileSystemEntries(directory) + string[] artifacts = Directory.EnumerateFileSystemEntries( + directory, "*", SearchOption.AllDirectories) .Where(path => path.EndsWith(".bak", StringComparison.OrdinalIgnoreCase) || path.EndsWith(".ecmeta.json", StringComparison.OrdinalIgnoreCase) || @@ -561,10 +608,10 @@ private static void AssertNoArtifacts(string directory) } private static Dictionary Snapshot(string directory) => - Directory.EnumerateFiles(directory) + Directory.EnumerateFiles(directory, "*", SearchOption.AllDirectories) .OrderBy(path => path, StringComparer.OrdinalIgnoreCase) .ToDictionary( - path => Path.GetFileName(path)!, + path => Path.GetRelativePath(directory, path), Hash, StringComparer.OrdinalIgnoreCase); diff --git a/sources/EncodingChecker.Tests/AtomicArtifactFileTests.cs b/sources/EncodingChecker.Tests/AtomicArtifactFileTests.cs new file mode 100644 index 0000000..8159b06 --- /dev/null +++ b/sources/EncodingChecker.Tests/AtomicArtifactFileTests.cs @@ -0,0 +1,122 @@ +using System.Text; + +namespace EncodingChecker.Tests; + +/// +/// A failed artifact write must preserve the previous complete file. +/// +/// +/// An interrupted in-place write once left plans, journals, reports, and settings +/// incomplete. Losing a reviewed plan is especially unsafe because regenerating it creates +/// a different, unreviewed plan. These tests pin preservation, not the writer's mechanism. +/// +public sealed class AtomicArtifactFileTests : IDisposable +{ + private readonly string _root = + Directory.CreateTempSubdirectory("ec_atomic_").FullName; + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup. + } + } + + private string Existing(string name, string content) + { + string path = Path.Combine(_root, name); + File.WriteAllText(path, content, new UTF8Encoding(false)); + return path; + } + + private string[] TempArtifacts() => + Directory.GetFiles(_root, "*." + EncodingConverter.TempFileSuffix); + + [Fact] + public void AFailedWriteLeavesThePreviousArtifactIntact() + { + string path = Existing("plan.json", "{\"reviewed\":true}"); + byte[] before = File.ReadAllBytes(path); + + string? error = AtomicArtifactFile.Write( + path, _ => throw new InvalidOperationException("serialiser gave up")); + + Assert.NotNull(error); + Assert.Equal(before, File.ReadAllBytes(path)); + Assert.Empty(TempArtifacts()); + } + + /// An unexpected write failure must still preserve the previous version. + [Fact] + public void AnUnexpectedFailureAlsoLeavesThePreviousArtifactIntact() + { + string path = Existing("journal.json", "{\"runs\":1}"); + byte[] before = File.ReadAllBytes(path); + + Assert.Throws( + () => AtomicArtifactFile.Write(path, _ => throw new FormatException())); + + Assert.Equal(before, File.ReadAllBytes(path)); + Assert.Empty(TempArtifacts()); + } + + [Fact] + public void ASuccessfulWriteReplacesTheArtifactAndLeavesNoTemporaryFile() + { + string path = Existing("report.csv", "old,content\n"); + + string? error = AtomicArtifactFile.WriteText( + path, "new,content\n", new UTF8Encoding(false)); + + Assert.Null(error); + Assert.Equal("new,content\n", File.ReadAllText(path)); + Assert.Empty(TempArtifacts()); + } + + [Fact] + public void ADestinationThatDoesNotExistYetIsCreated() + { + string path = Path.Combine(_root, "fresh.json"); + + string? error = AtomicArtifactFile.WriteText( + path, "{}", new UTF8Encoding(false)); + + Assert.Null(error); + Assert.Equal("{}", File.ReadAllText(path)); + Assert.Empty(TempArtifacts()); + } + + /// Writing a report through a stream must preserve its UTF-8 BOM for Excel. + [Fact] + public void TheEncodingsPreambleIsStillWritten() + { + string path = Path.Combine(_root, "bom.csv"); + + Assert.Null(AtomicArtifactFile.WriteText( + path, "File,Encoding\n", ConversionReport.CsvFileEncoding)); + + Assert.Equal( + ConversionReport.CsvFileEncoding.GetPreamble(), + File.ReadAllBytes(path).Take(3)); + } + + /// A leftover temporary artifact must use the suffix excluded from scans. + [Fact] + public void TheTemporaryFileUsesTheSuffixScansAlreadyExclude() + { + string path = Path.Combine(_root, "observed.json"); + string? observed = null; + + AtomicArtifactFile.Write(path, _ => + observed = TempArtifacts().SingleOrDefault()); + + Assert.NotNull(observed); + Assert.True(DirectoryTraversal.HasReservedArtifactSuffix(observed)); + Assert.Empty(TempArtifacts()); + } +} diff --git a/sources/EncodingChecker.Tests/OutputDestinationPreflightTests.cs b/sources/EncodingChecker.Tests/OutputDestinationPreflightTests.cs new file mode 100644 index 0000000..aa503fa --- /dev/null +++ b/sources/EncodingChecker.Tests/OutputDestinationPreflightTests.cs @@ -0,0 +1,116 @@ +using System.Text; + +namespace EncodingChecker.Tests; + +/// +/// An unusable report or journal destination must stop the run before files change. +/// +/// +/// EC once found these failures after conversion, leaving changed files without the +/// requested record. Preflight changes when the failure is found, not its meaning: +/// docs/CLI.md and keep exit code 3. +/// +public sealed class OutputDestinationPreflightTests : IDisposable +{ + private const int ExpectedProcessingErrors = 3; + + private readonly string _root = + Directory.CreateTempSubdirectory("ec_preflight_").FullName; + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup. + } + } + + private static int Run(params string[] args) + { + TextWriter originalOut = Console.Out; + TextWriter originalError = Console.Error; + + try + { + Console.SetOut(new StringWriter()); + Console.SetError(new StringWriter()); + + return Program.RunConsoleMode(args); + } + finally + { + Console.SetOut(originalOut); + Console.SetError(originalError); + } + } + + private string WriteSource() + { + string path = Path.Combine(_root, "source.txt"); + File.WriteAllText(path, "hello world\n", new UTF8Encoding(false)); + return path; + } + + /// An unwritable destination must leave every selected file unchanged. + [Theory] + [InlineData("-Journal")] + [InlineData("-Report")] + public void AnOutputUnderAMissingDirectoryLeavesTheSourceAlone(string option) + { + string source = WriteSource(); + byte[] before = File.ReadAllBytes(source); + + int exitCode = Run( + "-BasePath", _root, + "-Include", "source.txt", + "-Target", "utf-16-bom", + option, Path.Combine(_root, "no-such-folder", "output.json")); + + Assert.Equal(ExpectedProcessingErrors, exitCode); + Assert.Equal(before, File.ReadAllBytes(source)); + } + + /// An existing directory is not a valid report-file destination. + [Theory] + [InlineData("-Journal")] + [InlineData("-Report")] + public void AnOutputOntoAnExistingDirectoryLeavesTheSourceAlone(string option) + { + string source = WriteSource(); + byte[] before = File.ReadAllBytes(source); + + string occupied = Path.Combine(_root, "occupied"); + Directory.CreateDirectory(occupied); + + int exitCode = Run( + "-BasePath", _root, + "-Include", "source.txt", + "-Target", "utf-16-bom", + option, occupied); + + Assert.Equal(ExpectedProcessingErrors, exitCode); + Assert.Equal(before, File.ReadAllBytes(source)); + } + + /// A usable output path must pass preflight and allow the run. + [Fact] + public void AUsableDestinationStillRuns() + { + WriteSource(); + + string report = Path.Combine(_root, "report.csv"); + + int exitCode = Run( + "-BasePath", _root, + "-Include", "source.txt", + "-DetectOnly", + "-Report", report); + + Assert.Equal(0, exitCode); + Assert.True(File.Exists(report)); + } +} diff --git a/sources/EncodingChecker.Tests/PlanEnumValidationTests.cs b/sources/EncodingChecker.Tests/PlanEnumValidationTests.cs new file mode 100644 index 0000000..2f10c85 --- /dev/null +++ b/sources/EncodingChecker.Tests/PlanEnumValidationTests.cs @@ -0,0 +1,108 @@ +using System.Text; +using System.Text.Json.Nodes; + +namespace EncodingChecker.Tests; + +/// +/// A plan carrying an action no build ever wrote must be refused, not reported as done. +/// +/// +/// JSON accepts undefined numeric enum values. The former fallback mapped one to +/// , producing a false success and journal +/// record. Plan validation and the mapping's throw now guard both boundaries. +/// +public sealed class PlanEnumValidationTests : IDisposable +{ + private readonly string _root = + Directory.CreateTempSubdirectory("ec_planenum_").FullName; + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup. + } + } + + private static int Run(params string[] args) + { + TextWriter originalOut = Console.Out; + TextWriter originalError = Console.Error; + + try + { + Console.SetOut(new StringWriter()); + Console.SetError(new StringWriter()); + + return Program.RunConsoleMode(args); + } + finally + { + Console.SetOut(originalOut); + Console.SetError(originalError); + } + } + + /// Writes a real plan, then replaces one field with an undefined value. + private string PlanWithField(string field, object value) + { + string source = Path.Combine(_root, "plain.txt"); + File.WriteAllText(source, "hello\n", new UTF8Encoding(false)); + + string planPath = Path.Combine(_root, $"plan-{field}.json"); + + Assert.Equal(0, Run( + "-BasePath", _root, + "-Include", "plain.txt", + "-Target", "utf-16-bom", + "-Plan", planPath, + "-Quiet")); + + JsonNode plan = JsonNode.Parse(File.ReadAllText(planPath))!; + plan["Files"]![0]![field] = JsonValue.Create(value); + + File.WriteAllText(planPath, plan.ToJsonString(), new UTF8Encoding(false)); + + return planPath; + } + + [Theory] + [InlineData("Action")] + [InlineData("SourceInterpretation")] + public void AnUndefinedEnumValueIsRefusedBeforeAnyFileIsTouched(string field) + { + string planPath = PlanWithField(field, 99); + string source = Path.Combine(_root, "plain.txt"); + byte[] before = File.ReadAllBytes(source); + + string journal = Path.Combine(_root, $"journal-{field}.json"); + + int exitCode = Run("-Apply", planPath, "-Journal", journal); + + Assert.NotEqual(0, exitCode); + Assert.Equal(before, File.ReadAllBytes(source)); + + // No journal may claim the rejected action ran. + Assert.False(File.Exists(journal)); + } + + /// The result mapping must reject every undefined planned action. + [Fact] + public void AnUndefinedActionHasNoReportResult() + { + Assert.Throws( + () => ConversionPolicy.ToRowResult((PlannedAction)99)); + } + + /// Every defined action still maps, or the throw would be a regression. + [Fact] + public void EveryDefinedActionStillMaps() + { + foreach (PlannedAction action in Enum.GetValues()) + _ = ConversionPolicy.ToRowResult(action); + } +} diff --git a/sources/EncodingChecker/AtomicArtifactFile.cs b/sources/EncodingChecker/AtomicArtifactFile.cs new file mode 100644 index 0000000..99f4648 --- /dev/null +++ b/sources/EncodingChecker/AtomicArtifactFile.cs @@ -0,0 +1,78 @@ +using System; +using System.IO; +using System.Text; + +namespace EncodingChecker; + +/// +/// Writes plans, journals, reports, and settings without replacing the previous +/// artifact until its successor is complete. +/// +/// +/// Recovery sidecars keep their own writer because it also reads back and validates +/// each record. +/// +internal static class AtomicArtifactFile +{ + /// Writes to a temporary file, then installs the completed artifact. + /// on success; otherwise, a diagnostic. + internal static string? Write(string path, Action writeContent) + { + ArgumentException.ThrowIfNullOrWhiteSpace(path); + ArgumentNullException.ThrowIfNull(writeContent); + + string fullPath = Path.GetFullPath(path); + + // Keep the temporary file on the destination volume and under a suffix scans ignore. + string tempPath = + $"{fullPath}.{Guid.NewGuid():N}.{EncodingConverter.TempFileSuffix}"; + + try + { + using (var stream = new FileStream( + tempPath, FileMode.CreateNew, FileAccess.Write, FileShare.None)) + { + writeContent(stream); + + // Flush to disk before installation so a power loss cannot expose an empty file. + stream.Flush(flushToDisk: true); + } + + EncodingConverter.AtomicReplaceForBackup(tempPath, fullPath); + return null; + } + catch (Exception ex) when ( + ex is IOException or UnauthorizedAccessException or ArgumentException + or NotSupportedException or InvalidOperationException) + { + return ex.Message; + } + finally + { + try + { + if (File.Exists(tempPath)) + File.Delete(tempPath); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + // Cleanup failure cannot invalidate an artifact already installed. + } + } + } + + /// + /// Writes text, including any preamble required by . + /// + internal static string? WriteText(string path, string content, Encoding encoding) + { + ArgumentNullException.ThrowIfNull(content); + ArgumentNullException.ThrowIfNull(encoding); + + return Write(path, stream => + { + using var writer = new StreamWriter(stream, encoding, leaveOpen: true); + writer.Write(content); + }); + } +} diff --git a/sources/EncodingChecker/ConversionJournal.cs b/sources/EncodingChecker/ConversionJournal.cs index a55141c..04dc388 100644 --- a/sources/EncodingChecker/ConversionJournal.cs +++ b/sources/EncodingChecker/ConversionJournal.cs @@ -354,9 +354,10 @@ private static string Hash(string path) { try { - File.WriteAllText( - path, JsonSerializer.Serialize(this, Options), new UTF8Encoding(false)); - return null; + // Serialize before opening the destination so failure preserves the previous journal. + string json = JsonSerializer.Serialize(this, Options); + + return AtomicArtifactFile.WriteText(path, json, new UTF8Encoding(false)); } catch (Exception ex) when ( ex is IOException or UnauthorizedAccessException or JsonException) diff --git a/sources/EncodingChecker/ConversionPlan.cs b/sources/EncodingChecker/ConversionPlan.cs index c95c079..b098193 100644 --- a/sources/EncodingChecker/ConversionPlan.cs +++ b/sources/EncodingChecker/ConversionPlan.cs @@ -288,9 +288,10 @@ internal static ConversionPlan FromEntries( { try { - File.WriteAllText( - path, JsonSerializer.Serialize(this, Options), new UTF8Encoding(false)); - return null; + // Serialize before opening the destination so failure preserves the previous plan. + string json = JsonSerializer.Serialize(this, Options); + + return AtomicArtifactFile.WriteText(path, json, new UTF8Encoding(false)); } catch (Exception ex) when ( ex is IOException or UnauthorizedAccessException or JsonException) @@ -352,6 +353,24 @@ internal static ConversionPlan FromEntries( return null; } + // A number deserializes into any enum, so a plan can name a value no build + // wrote. Reject it before touching a source. + if (!Enum.IsDefined(file.Action)) + { + error = "The plan records an unknown action for " + + $"'{file.RelativePath}'. Re-run -Plan to produce one this " + + "build can carry out."; + return null; + } + + if (!Enum.IsDefined(file.SourceInterpretation)) + { + error = "The plan records an unknown source interpretation for " + + $"'{file.RelativePath}'. Re-run -Plan to produce one this " + + "build can carry out."; + return null; + } + // Only writes need a hash; hashless refusals remain visible and harmless. if (file.Action == PlannedAction.Convert && string.IsNullOrWhiteSpace(file.Sha256)) diff --git a/sources/EncodingChecker/ConversionPolicy.cs b/sources/EncodingChecker/ConversionPolicy.cs index 09bd5aa..859193f 100644 --- a/sources/EncodingChecker/ConversionPolicy.cs +++ b/sources/EncodingChecker/ConversionPolicy.cs @@ -111,27 +111,26 @@ internal static PlannedAction Decide( /// /// Maps a planned action to its report result. /// + /// + /// JSON can deserialize undefined enum values. Throw rather than report an action no + /// build wrote as a completed conversion. + /// internal static ConversionRowResult ToRowResult(PlannedAction action) => action switch { + PlannedAction.Convert => ConversionRowResult.Converted, PlannedAction.Unchanged => ConversionRowResult.Unchanged, PlannedAction.Skip => ConversionRowResult.Skipped, PlannedAction.Refuse => ConversionRowResult.Refused, - _ => ConversionRowResult.Converted, + _ => throw new ArgumentOutOfRangeException( + nameof(action), action, "Unknown planned action."), }; /// /// 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. + /// Keep this beside so a new refusal cannot silently inherit an + /// unrelated fallback reason. /// internal static string? ReasonCodeFor( PlannedAction action, diff --git a/sources/EncodingChecker/MainForm.Settings.cs b/sources/EncodingChecker/MainForm.Settings.cs index f640545..e4e4f1e 100644 --- a/sources/EncodingChecker/MainForm.Settings.cs +++ b/sources/EncodingChecker/MainForm.Settings.cs @@ -96,10 +96,10 @@ private void SaveSettings() try { - using var settingsFile = new FileStream( - GetSettingsFileName(), FileMode.Create, FileAccess.Write, FileShare.None); - new XmlSerializer(typeof(Settings)).Serialize(settingsFile, _settings); - settingsFile.Flush(); + // EC-16: atomic replacement preserves the previous settings after a write failure. + _ = AtomicArtifactFile.Write( + GetSettingsFileName(), + stream => new XmlSerializer(typeof(Settings)).Serialize(stream, _settings)); } catch (Exception ex) when ( ex is IOException or UnauthorizedAccessException or InvalidOperationException diff --git a/sources/EncodingChecker/Program.CliExecution.cs b/sources/EncodingChecker/Program.CliExecution.cs index 5a18469..7b9d602 100644 --- a/sources/EncodingChecker/Program.CliExecution.cs +++ b/sources/EncodingChecker/Program.CliExecution.cs @@ -211,6 +211,52 @@ .. entries.OrderBy(e => e.FilePath, StringComparer.OrdinalIgnoreCase) } // Internal so tests can pin the published CLI exit-code contract. + /// Returns the first output path known to be unusable before the run. + /// + /// It does not probe by creating a file because a later failure would leave that probe + /// behind. The actual write can still fail. + /// + private static string? FindUnusableOutputDestination(CliOptions options) + { + (string Name, string? Path)[] outputs = + [ + ("-Plan", options.PlanPath), + ("-Journal", options.JournalPath), + ("-Report", options.ReportPath), + ]; + + foreach ((string name, string? path) in outputs) + { + if (string.IsNullOrWhiteSpace(path)) + continue; + + string fullPath; + + try + { + fullPath = Path.GetFullPath(path); + } + catch (Exception ex) when ( + ex is IOException or ArgumentException or NotSupportedException) + { + return $"{name} contains an invalid path: {ex.Message}"; + } + + if (Directory.Exists(fullPath)) + return $"{name} names an existing directory. Give it a file path."; + + string? parent = Path.GetDirectoryName(fullPath); + + if (!string.IsNullOrEmpty(parent) && !Directory.Exists(parent)) + { + return $"{name} is under '{parent}', which does not exist. Create it " + + "first, or choose a path whose folder already exists."; + } + } + + return null; + } + internal static int RunConsoleMode(string[] args) { if (args is ["--version"]) @@ -239,6 +285,16 @@ internal static int RunConsoleMode(string[] args) return 1; } + // Keep output preflight outside argument validation: docs/CLI.md assigns report + // failures exit 3, and ExitCodeContractTests pins that contract. + string? destinationError = FindUnusableOutputDestination(options); + + if (destinationError is not null) + { + Console.Error.WriteLine(destinationError); + return 3; + } + if (!string.IsNullOrWhiteSpace(options.ApplyPath)) return ApplyPlan(options); @@ -430,17 +486,16 @@ .. collectedEntries.OrderBy( if (!string.IsNullOrEmpty(options.ReportPath)) { - try + string? reportError = AtomicArtifactFile.Write(options.ReportPath, stream => { using var writer = new StreamWriter( - options.ReportPath, false, ConversionReport.CsvFileEncoding); + stream, ConversionReport.CsvFileEncoding, leaveOpen: true); ConversionReport.WriteCsv(entries, writer); - } - catch (Exception ex) when ( - ex is IOException or UnauthorizedAccessException) + }); + + if (reportError is not null) { - Console.Error.WriteLine( - $"Failed to write report file: {ex.Message}"); + Console.Error.WriteLine($"Failed to write report file: {reportError}"); return 3; } } diff --git a/sources/EncodingChecker/Properties/AssemblyInfo.cs b/sources/EncodingChecker/Properties/AssemblyInfo.cs index a36b31e..a96838d 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.12.0.0")] -[assembly: AssemblyFileVersion("3.12.0.0")] +[assembly: AssemblyVersion("3.12.1.0")] +[assembly: AssemblyFileVersion("3.12.1.0")]