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..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] @@ -426,8 +453,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 +463,41 @@ 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] + 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] @@ -447,8 +510,9 @@ public void AFileChangedAfterTheConfirmation_StopsTheWholeRun() byte[] stableBefore = File.ReadAllBytes(stable); + List entries = View(); OrchestrationResult result = Convert( - View(), + entries, Proceed, betweenPlanAndWrite: plan => { @@ -466,6 +530,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..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( @@ -158,6 +191,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 +206,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 +223,8 @@ internal OrchestrationResult Run( if (!ApplyChosenSource( response.SourceEncoding, response.Files, entries)) { + ConversionReportEntry.MarkUnattempted(entries, []); + return new OrchestrationResult { Outcome = OrchestrationOutcome.Cancelled, @@ -201,6 +244,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,