From 69396798b2bca274f4eab534649632e8a2a2299d Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Mon, 14 Sep 2026 01:04:20 +0300 Subject: [PATCH 1/2] Fix false-success CSV reporting on cancel and stale plans ConversionOrchestrator's decide pass marks eligible entries "would convert" before the user is ever asked. Cancelling, hitting a stale plan, or failing to plan returned without correcting that, so the same entries later exported to CSV as Converted even though nothing was written. Mark them NotAttempted at each no-write return, the same idiom already used for an interrupted run. Also corrects SAFETY.md/CONVERSION-WORKFLOW.md, which said an already-matching file is not decoded (it is now strictly validated before being reported Unchanged), and strengthens WhatIf_NeverModifiesAnything, which was silently refused before reaching the dry-run branch it meant to test. Co-Authored-By: Claude Sonnet 5 --- docs/CONVERSION-WORKFLOW.md | 5 +++-- docs/SAFETY.md | 4 +++- .../ConversionOrchestrationTests.cs | 22 +++++++++++++++++-- .../ConversionSafetyInvariantTests.cs | 8 +++++++ .../EncodingChecker/ConversionOrchestrator.cs | 14 ++++++++++++ 5 files changed, 48 insertions(+), 5 deletions(-) diff --git a/docs/CONVERSION-WORKFLOW.md b/docs/CONVERSION-WORKFLOW.md index c145cd0..aa53c4f 100644 --- a/docs/CONVERSION-WORKFLOW.md +++ b/docs/CONVERSION-WORKFLOW.md @@ -21,8 +21,9 @@ For a file that needs conversion, EC applies this policy: | ASCII, Unicode with a BOM, or text whose encoding EC can prove from its bytes | Convert automatically | | Legacy text or BOM-less Unicode whose encoding cannot be proven safely | Do not convert; ask you to choose the original encoding | -A file that already matches the target encoding and BOM is reported as **Unchanged** and -is not decoded or rewritten. No source choice is needed because no conversion occurs. +A file that already matches the target encoding and BOM is still strictly validated in +full. A valid file is reported as **Unchanged** and is not rewritten; an invalid file is +reported as an error instead. No source choice is needed because no conversion occurs. If you choose a source encoding, EC uses it only to read the original bytes. It does not disable strict decoding, output verification, backup verification, or safe installation. diff --git a/docs/SAFETY.md b/docs/SAFETY.md index 790064c..4cdf0a9 100644 --- a/docs/SAFETY.md +++ b/docs/SAFETY.md @@ -34,7 +34,9 @@ both the output and backup have verified. | ASCII, Unicode with a BOM, or text whose encoding EC can prove from its bytes | Convert automatically. | | Legacy text or BOM-less Unicode whose encoding cannot be proven safely | Do not convert; ask you to choose the original encoding. | -An unchanged file is reported as `Unchanged` and is not decoded or rewritten. +A file already matching the target is strictly validated in full. A valid file +is reported as `Unchanged` and is not rewritten; an invalid file is reported as +an error instead. A source encoding chosen by the user controls only how EC reads the original bytes. It does not bypass any safety check. diff --git a/sources/EncodingChecker.Tests/ConversionOrchestrationTests.cs b/sources/EncodingChecker.Tests/ConversionOrchestrationTests.cs index 13c9f5a..a09576f 100644 --- a/sources/EncodingChecker.Tests/ConversionOrchestrationTests.cs +++ b/sources/EncodingChecker.Tests/ConversionOrchestrationTests.cs @@ -426,8 +426,9 @@ public void CancellingTheConfirmation_ModifiesNothing() byte[] jpBefore = File.ReadAllBytes(jp); byte[] plainBefore = File.ReadAllBytes(plain); + List entries = View(); OrchestrationResult result = Convert( - View(), _ => ConfirmationResponse.Cancel, backup: true); + entries, _ => ConfirmationResponse.Cancel, backup: true); Assert.Equal(OrchestrationOutcome.Cancelled, result.Outcome); Assert.Equal(jpBefore, File.ReadAllBytes(jp)); @@ -435,6 +436,14 @@ public void CancellingTheConfirmation_ModifiesNothing() // Not even a backup, which would be a modification of the directory. Assert.Empty(Directory.GetFiles(_root, "*.bak")); + + // The decide pass marks plain.txt "would convert" before the user is even asked. + // Cancelling must not let that leak into the exported report as completed work. + Assert.All(entries, entry => Assert.True(entry.NotAttempted)); + + using var csv = new StringWriter(); + ConversionReport.WriteCsv(entries, csv); + Assert.DoesNotContain("Converted", csv.ToString()); } [Fact] @@ -447,8 +456,9 @@ public void AFileChangedAfterTheConfirmation_StopsTheWholeRun() byte[] stableBefore = File.ReadAllBytes(stable); + List entries = View(); OrchestrationResult result = Convert( - View(), + entries, Proceed, betweenPlanAndWrite: plan => { @@ -466,6 +476,14 @@ public void AFileChangedAfterTheConfirmation_StopsTheWholeRun() // Neither file, not just the one that moved. Assert.Equal(stableBefore, File.ReadAllBytes(stable)); Assert.Equal("changed underneath", File.ReadAllText(moving)); + + // Both entries were marked "would convert" by the decide pass before the plan was + // shown; going stale must not let that leak into the exported report as done work. + Assert.All(entries, entry => Assert.True(entry.NotAttempted)); + + using var csv = new StringWriter(); + ConversionReport.WriteCsv(entries, csv); + Assert.DoesNotContain("Converted", csv.ToString()); } [Fact] diff --git a/sources/EncodingChecker.Tests/ConversionSafetyInvariantTests.cs b/sources/EncodingChecker.Tests/ConversionSafetyInvariantTests.cs index 8df43ea..b8c9b6c 100644 --- a/sources/EncodingChecker.Tests/ConversionSafetyInvariantTests.cs +++ b/sources/EncodingChecker.Tests/ConversionSafetyInvariantTests.cs @@ -248,6 +248,11 @@ public void WhatIf_NeverModifiesAnything() SourceHasBom = false, TargetEncoding = "windows-1252", TargetHasBom = false, + + // Without this, policy refuses the file before the dry-run branch is even + // reached (windows-1252 needs an explicit source), and the assertions below + // would hold trivially without exercising -WhatIf at all. + SourceEncodingWasSpecified = true, }; var completed = new EntrySink(); @@ -256,6 +261,9 @@ public void WhatIf_NeverModifiesAnything() ScanEngine.DefaultMaxParallelism, whatIf: true, backup: false, completed.Add, CancellationToken.None); + // Confirms the file was actually eligible to convert, so preservation below is + // proof of dry-run behavior rather than an artifact of an earlier refusal. + Assert.Equal(ConversionRowResult.Converted, Assert.Single(completed).Result); Assert.Equal(original, File.ReadAllBytes(path)); Assert.False(File.Exists(path + ".bak")); } diff --git a/sources/EncodingChecker/ConversionOrchestrator.cs b/sources/EncodingChecker/ConversionOrchestrator.cs index 4941195..eed57ee 100644 --- a/sources/EncodingChecker/ConversionOrchestrator.cs +++ b/sources/EncodingChecker/ConversionOrchestrator.cs @@ -158,6 +158,10 @@ internal OrchestrationResult Run( catch (InvalidOperationException ex) { // An undecided entry means the caller failed to provide a complete plan. + // The decide pass above already marked some entries "would convert"; no + // write pass will ever reach them now. + ConversionReportEntry.MarkUnattempted(entries, []); + return new OrchestrationResult { Outcome = OrchestrationOutcome.CouldNotPlan, @@ -169,6 +173,10 @@ internal OrchestrationResult Run( if (response.Choice == ConfirmationChoice.Cancel) { + // The decide pass already marked some entries "would convert"; the write + // pass that would have made that true is never going to run. + ConversionReportEntry.MarkUnattempted(entries, []); + return new OrchestrationResult { Outcome = OrchestrationOutcome.Cancelled, @@ -182,6 +190,8 @@ internal OrchestrationResult Run( if (!ApplyChosenSource( response.SourceEncoding, response.Files, entries)) { + ConversionReportEntry.MarkUnattempted(entries, []); + return new OrchestrationResult { Outcome = OrchestrationOutcome.Cancelled, @@ -201,6 +211,10 @@ internal OrchestrationResult Run( if (stale.Count > 0) { + // The decide pass already marked some entries "would convert"; the write + // pass that would have made that true is never going to run. + ConversionReportEntry.MarkUnattempted(entries, []); + return new OrchestrationResult { Outcome = OrchestrationOutcome.PlanWentStale, From 5fb26d42e683f1a122b5e7002fd94b6a443a5828 Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Mon, 14 Sep 2026 01:27:28 +0300 Subject: [PATCH 2/2] Reconcile stale conversion results when cancelled during the decide pass The previous fix marked entries NotAttempted at the confirmation loop's no-write return points, but cancellation could also escape earlier: from RefreshSourceSnapshots, from either whatIf decide-pass RunPass call, or from the explicit ThrowIfCancellationRequested check, none of which were guarded. Any of those left the decide pass's "would convert" marks on entries with no reconciliation, reproducing the same false-success CSV export the prior fix closed. Run() now wraps the whole decide/confirm/write sequence (extracted to DecideConfirmAndRun) in one cancellation handler that marks every entry NotAttempted before rethrowing. The write pass's own cancellation handling is untouched: it returns an Interrupted result rather than throwing, so it never reaches the new outer catch. Also covers the ApplyChosenSource-fails-to-Cancelled branch with a test using a mixed batch, since the existing test for it used a fixture that was never marked Converted in the first place. Co-Authored-By: Claude Sonnet 5 --- .../ConversionOrchestrationTests.cs | 54 +++++++++++++++++++ .../EncodingChecker/ConversionOrchestrator.cs | 33 ++++++++++++ 2 files changed, 87 insertions(+) diff --git a/sources/EncodingChecker.Tests/ConversionOrchestrationTests.cs b/sources/EncodingChecker.Tests/ConversionOrchestrationTests.cs index a09576f..7a29fa7 100644 --- a/sources/EncodingChecker.Tests/ConversionOrchestrationTests.cs +++ b/sources/EncodingChecker.Tests/ConversionOrchestrationTests.cs @@ -415,6 +415,33 @@ public void ChoosingAnEncodingForNoFilesChangesNothing() Assert.Equal(before, File.ReadAllBytes(path)); } + [Fact] + public void ChoosingAnEncodingForNoFilesChangesNothing_EvenForFilesAlreadyMarkedConverted() + { + // The scope check above uses a batch where the only file is ambiguous, so its + // Result was never "would convert" to begin with. A mixed batch is needed to + // prove this same no-op scope doesn't leak a stale Converted for an eligible + // file the decide pass already marked before this response was scripted. + string ambiguous = Write("ambiguous.txt", "Le café était déjà prêt", "windows-1252"); + string eligible = Write("eligible.txt", "plain ascii here", "ascii"); + byte[] ambiguousBefore = File.ReadAllBytes(ambiguous); + byte[] eligibleBefore = File.ReadAllBytes(eligible); + + List entries = View(); + OrchestrationResult result = Convert(entries, _ => new ConfirmationResponse( + ConfirmationChoice.ChooseSourceEncoding, "windows-1252", [])); + + Assert.Equal(OrchestrationOutcome.Cancelled, result.Outcome); + Assert.Equal(ambiguousBefore, File.ReadAllBytes(ambiguous)); + Assert.Equal(eligibleBefore, File.ReadAllBytes(eligible)); + + Assert.All(entries, entry => Assert.True(entry.NotAttempted)); + + using var csv = new StringWriter(); + ConversionReport.WriteCsv(entries, csv); + Assert.DoesNotContain("Converted", csv.ToString()); + } + // ------------------------------------------------------- nothing gets modified [Fact] @@ -446,6 +473,33 @@ public void CancellingTheConfirmation_ModifiesNothing() Assert.DoesNotContain("Converted", csv.ToString()); } + [Fact] + public void CancellingDuringTheDecidePass_MarksEntriesUnattemptedBeforePropagating() + { + // The decide pass runs before the user is ever asked and can itself be + // cancelled. Whatever it already marked "would convert" must not be left + // looking like completed work once the exception propagates. + Write("plain.txt", "plain ascii here", "ascii"); + + List entries = View(); + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + Assert.ThrowsAny(() => + new ConversionOrchestrator(Proceed).Run( + entries, _root, "utf-8", targetWriteBom: false, + backup: false, preview: false, + ScanEngine.DefaultMaxParallelism, + _ => { }, + cts.Token)); + + Assert.All(entries, entry => Assert.True(entry.NotAttempted)); + + using var csv = new StringWriter(); + ConversionReport.WriteCsv(entries, csv); + Assert.DoesNotContain("Converted", csv.ToString()); + } + [Fact] public void AFileChangedAfterTheConfirmation_StopsTheWholeRun() { diff --git a/sources/EncodingChecker/ConversionOrchestrator.cs b/sources/EncodingChecker/ConversionOrchestrator.cs index eed57ee..4155077 100644 --- a/sources/EncodingChecker/ConversionOrchestrator.cs +++ b/sources/EncodingChecker/ConversionOrchestrator.cs @@ -123,6 +123,39 @@ internal OrchestrationResult Run( foreach (ConversionReportEntry entry in entries) entry.ResetAttemptEvidence(); + try + { + return DecideConfirmAndRun( + entries, baseDirectory, targetCharset, targetWriteBom, backup, preview, + maxParallelism, onEntry, cancellationToken, startedUtc); + } + catch (OperationCanceledException) + { + // Cancellation anywhere before a write pass starts can leave entries the + // decide pass already marked "would convert". The write pass below has its + // own cancellation handling that returns normally instead of throwing, so it + // never reaches this catch; only a decide-phase cancellation does. + ConversionReportEntry.MarkUnattempted(entries, []); + throw; + } + } + + /// + /// The decide/confirm/write sequence, split out so can wrap all of + /// it in one cancellation handler. + /// + private OrchestrationResult DecideConfirmAndRun( + IReadOnlyList entries, + string baseDirectory, + string targetCharset, + bool targetWriteBom, + bool backup, + bool preview, + int maxParallelism, + Action onEntry, + CancellationToken cancellationToken, + DateTime startedUtc) + { // Detection and hashing share one read, so the plan cannot mix different bytes. // Detection still runs for explicit choices to preserve provenance and safety vetoes. ScanEngine.RefreshSourceSnapshots(