Fix twelve findings from an independent review, and release v3.12.0 - #78
Merged
Conversation
ConversionPolicy compared the detected charset's WebName against whatever the caller typed, so every accepted alias for the same 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. Measured: bytes identical in each case, and every modification time reset. With -Backup that also leaves a .bak and an .ecmeta.json beside each unchanged file. 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. Decide now takes the resolved code pages. A zero code page means the label did not resolve and proves nothing, so it is never treated as a match. ASCII to UTF-8 is deliberately still a conversion: they are different code pages, and a test says so with the reasoning attached. Mutation: reverting to the label comparison fails 7 of the 11 new tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Nothing about Math.Min(ProcessorCount, 8) gave the number an identity a document could be checked against, so raising it left two prose copies behind with nothing to notice. It is now ScanEngine.MaxParallelismCap. DocumentedParallelismDefaultTests pins all three statements together. It 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. Both also assert the search found exactly one line, so the check cannot pass by looking in the wrong place. Mutation, all three killed: CLI.md back to 4 fails 1; the help text back to 4 fails 1; raising the cap to 16 without touching the docs fails 2, which is what proves the tests track the code rather than a hardcoded 8. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The diagnostic reported DecoderFallbackException.Index as "offset N within the failing read chunk". That 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" - a position no file has, in a message naming a frame it did not describe. The exception already carries the offending bytes, and they mean the same thing wherever the failure happened: "invalid byte sequence 0xE4B8." An absolute file offset would need the streaming loop restructured to keep each chunk's base position in scope; the bytes identify the fault without it. DecodeFailureDiagnosticTests also records something non-obvious found while writing it: reaching the decoder with malformed UTF-8 needs -From utf-8. Automatic detection relabels those bytes as windows-1252 and the policy refuses them before any decoding happens. Mutation: restoring the index message fails 5 of 5. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. The filter is now "not OperationCanceledException and not OutOfMemoryException". Cancellation is the user asking to stop; carrying on after the other would be pretending to process rather than processing. RunParallel became internal so the isolation could be asserted at all. No file can be made to throw the exceptions that mattered, which is precisely what made them dangerous, so the only way to prove one failure stays one row is to hand it a processItem that throws. Five exception types now assert the failing file becomes a row while the other two complete untouched, and two assert that stopping the run is still allowed out. Mutation: restoring the four-type filter fails 5 of 8. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
After attaching to the parent console, both writers were rebuilt with StreamWriter's default encoding, which is UTF-8 whatever the console is. On the machine this was found on, Console.OutputEncoding is ibm437: the per-file CSV rendered "Grüße aus München" as "Gr├╝├ƒe aus M├╝nchen". A tool whose subject is that failure was producing it in its own output. A redirected stream is a file or a pipe, so it stays UTF-8, matching the -Report file apart from its BOM. A console gets Console.OutputEncoding instead, so characters it cannot represent become "?" - visibly lossy rather than quietly wrong. No global console state is mutated, so the parent shell keeps the code page it had. Tests pin both branches, that neither writer emits a preamble, and that ASCII is byte-identical either way. Mutation, both killed: always UTF-8 fails 1; always the console encoding fails 1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Twelve directory names are skipped deliberately, and that is documented. Attribute-excluded folders were counted; these were not. A scan of a tree whose only content sat under build/ reported one file, zero exclusions and no warning. In a tool that reports coverage precisely so a clean result cannot stand in for complete coverage, this was the one hole the coverage report could not see. docs/CLI.md promised "EC reports how many files each exclusion skipped" and this exclusion reported nothing. DirectoriesExcludedByName is separate from the attribute counter, because folding them together would make that message's "(hidden, system, or reparse point)" untrue. What is scanned is deliberately unchanged. Letting an explicit include reach into these folders was considered and not done: CLI.md and the built-in help both state they are skipped, so that is a product decision rather than a fix. The documentation now also distinguishes the two kinds of count, since for a skipped folder EC reports the folder and not its contents - it does not walk it to find out. Mutation: removing the increment fails 14 of 15. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An unreadable file becomes a row with ScanFailed and drives exit code 3. An unreadable directory produced a warning on stderr and nothing else: no row, no counter, exit 0. The window passes no warning callback, so a GUI user was told nothing whatever. Measured against a real 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, which is the one thing the coverage report exists to prevent. DirectoriesUnreadable is counted at both catch blocks and kept apart from the two exclusion counters: those record folders EC chose not to enter, this records a failure. The exit code is unchanged. CLI.md states that coverage counts do not affect it, and that sentence was written about folders skipped on purpose rather than ones that could not be read; the documentation now states the consequence plainly, so a script that must not pass over unexamined content reads the counts rather than the exit code alone. Whether this should instead be exit 3 is a product decision. Mutation: removing both increments fails 2 of 6. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 when the ambiguous BOM-less case was added. ConversionPolicy.ReasonCodeFor sits beside Decide because it answers the same question. Verified equivalent across all 256 reachable combinations of Decide's inputs - the other 256 are unreachable, since a conflict needs an explicit source and the automatic ambiguity needs the absence of one. The test that matters for next time asserts that any Refuse or Skip whose interpretation is unmapped produces no reason at all, and fails. That is the failure mode the old shape had no way to detect. Mutation, both killed: dropping the ExplicitSource case fails 3 of 4; collapsing two refusal reasons fails 2 of 4. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
-Validate had four ways to return Invalid and explained only two. A file whose whole contents fail strict decoding says StrictValidationFailed, and one whose BOM-less byte order cannot be proven says AmbiguousBomlessUtf16. The other two arrived as a bare Invalid with an empty reason column. They are not the same situation. A charset outside the allowed list means widen the list or convert the file; an unidentifiable one means EC could not tell what it is, which -DetectOnly already calls UnknownEncoding. Both were merged by one boolean covering two unlike conditions. CharsetNotAllowed is new. UnknownEncoding is deliberately reused rather than given a -Validate-specific name, 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 script could always classify these rows by comparing the encoding column against the list it had passed in - it just had to do the work twice. Mutation, both killed: removing the branch fails 3 of 6; collapsing the two situations into one code fails 1 of 6. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Detection reads at most 64 KiB. When the source codec matched the target, Unchanged was reported and nothing read further, so a file whose first 64 KiB is clean and whose later bytes are not valid in the codec EC had just named was reported as already correct. Whether EC noticed depended only on which target the caller typed: the same corrupt file came back Error under -Target utf-16 and Unchanged under -Target utf-8. -Validate always read the whole file. The Convert path simply never reached that check once it had decided it had nothing to do. Reported as the pre-check it is: Action stays Unchanged, nothing was attempted, and the reason code is the one -Validate already uses. Eleven tests, including the matrix as a theory and a clean 70 KiB file staying Unchanged under every target, so the check cannot start refusing what it was added to describe honestly. Cost, measured: near zero, and not for the reason first assumed. Convert already reads every byte of every file, because CaptureSourceSnapshot hashes the whole stream before anything is decided. This adds a decode to a pass that already reads and hashes: 0.04 to 0.10 ms per MiB across 8 MiB and 120 MiB corpora, medians of five interleaved runs from a warm cache. This also retires a scoping decision made in the alias fix. ASCII to UTF-8 was kept as a conversion there on the grounds that folding it into Unchanged would skip the full-file decode; that reasoning was wrong, because Unchanged already skipped it for every same-codec case. Folding them together is now safe, and is worth doing separately. Mutation: removing the check fails 4 of 11. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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: one file converted, one failed. 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 marked Refuse rather than merely carrying an error. The plan records the action, so an error row still saying Convert would have put the file in front of a reviewer as approved work. -Plan now exits 3. The decode only, by decision. A source that reads cleanly can still fail on a target that cannot represent it - Cyrillic previewed to us-ascii still reports "would be converted" and still fails with TargetEncodeError - and predicting that means running the whole conversion into a discarded buffer. A test named for that case asserts it, so the gap is recorded rather than left to be filed as a defect later. Cost, measured: 22 to 39 percent of a preview measured in tens of milliseconds, and an upper bound, since the benchmark adds a read where the snapshot pass already has the bytes. Mutation, both killed: removing the check fails 3 of 8; reporting the error but still planning Convert fails 1 of 8. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
<file>.bak is a fixed name holding the version the most recent run replaced. A second conversion of the same file replaces it and removes its sidecar, so the original becomes unrecoverable. That is deliberate, pinned by BackupIntegrityTests.Backup_OverwritesAnyPreviousBackupFile since the first commit of the test suite, and the contract is stated in BackupRecordPairingTests: .bak is "the version this run replaced". No document said so. CLI.md said "Save every replaced original as <file>.bak" and SAFETY.md called it a recovery artifact and stopped there, so a user converting twice lost the original with nothing having warned them. This was raised during 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. Naming backups by conversion ID was proposed once before and declined, because <file>.bak is load-bearing beyond the backup itself: HasReservedArtifactSuffix, the -Include "*.bak" coverage reporting, GUI smoke phase A, and both documents key off that name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
EC-08 was filed "not reproduced" against a pathological include mask that completed inside a 10 s budget. The entry names the reason it did not reproduce: the attempt used a matching filename, which is the one input class that cannot show it, because the engine stops at the first success. Against a non-matching name the same masks grow 38, 193, 758, 2699, 8855 ms for six through ten wildcards, with no ceiling. The status moves to open: the cited code is still there, emitting RegexOptions.Compiled with an infinite MatchTimeout. Scored Medium impact, Theoretical reach, on the same pattern as the BOM-less UTF-32 row. The blow-up needs both halves built on purpose - a mask of about ten or more wildcards separated by one character, and a filename carrying about twenty-four or more mostly consecutive repeats of that same character. At twelve wildcards every realistic filename measured answers in 0 to 5 ms; forty consecutive "a" takes over 20 s. The mask comes from the operator's own command line, so no untrusted path supplies it. The fix was written, measured and reverted, and is recorded with the numbers so it is not re-derived: NonBacktracking answers the same mask in 5 ms, constructs faster, and agreed with Compiled on all 18,000 comparisons measured. Three dead ends are recorded with it, including that "*" crossing directory separators is the intended subtree semantic and not a Windows-wildcard bug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…racked An independent review of 74d5b3d raised three defects this project had no record of, all now fixed with their own commits: a folder EC could not read leaving no trace a machine could see, "already in the target encoding" being a whole-file claim made from a 64 KiB sample, and a preview promising conversions that would fail while a plan recorded them as approved. The second and third are worth this file existing for. Both are cases where EC reported an outcome it had not checked, and both were reachable in ordinary use: the same corrupt file came back Error or Unchanged depending only on which target was typed, and -Plan exited 0 over a file -Apply then failed on, after approval and part-way through the batch. The section also names four findings the same review left open that are recorded nowhere else here, because a finding that is written down but not tracked is the thing this file was created to prevent. The first of them - a BOM-less UTF-16 file detected as UTF-32 and converted silently, which output verification cannot catch because both sides of the comparison use the same wrong codec - is the largest defect the review found and is still open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit added three findings and said the rest were "in the git history under their own commits". True, and useless: this file is the index, so a fix it does not name is one you would have to already know about to find. That is the failure this file was created to prevent. EC-06 was recorded in a summary that got lost, reported as closed, shipped broken in two releases and rediscovered from scratch. Recording three of twelve fixes and pointing at the git log for the others reproduces the same shape at smaller scale. The nine now added are the alias comparison, the per-file catch filter, the name-excluded folder counter, the -Validate rejections without reasons, the reason code re-derived instead of read, the decode diagnostic's impossible offset, standard output encoded for the wrong reader, the stale parallelism default, and the undocumented lifetime of <file>.bak - the last of these a finding raised in review and withdrawn, recorded because a rejected proposal is worth as much to the next reviewer as an accepted one. The section is now ordered by what a reader needs first: what EC did to files, then what it reported, then what it documented. The four findings the review left open and that nothing else here tracks keep their own subsection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Assembly version and README heading; --version and the built-in help both report 3.12.0 from the Release build. Minor rather than patch. Exit codes move in both directions across four modes - an alias target drops -FailOnChanges from 2 to 0, a file invalid past 64 KiB and a plan over an undecodable source each move from 0 to 3 - and the CSV gains a reason code a script may not know. A reader told only "patch" would have been misled. No schema or semantics bump. These changes make EC refuse more and convert less, which the approved-decision ceiling already permits, so a plan written by an earlier build still means what it meant. Semantics stay at 6, the plan schema at 5, the journal schema at 4, and the notes say so rather than leaving a reader to work it out. The notes also carry what the verification does not cover. Unlike v3.11.2, this release cannot claim the corpus exemption: detection is untouched, no detector file changed, but ConversionPolicy did change, and the checklist asks for a corpus run on exactly that. None was made. The notes state it plainly instead of quoting the v3.11.0 figures as though they were evidence about this build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Twelve findings from an independent review of
74d5b3d, run against the sourcerather than against the backlog. One commit each, carrying that commit's
measurement and mutation result, plus the v3.12.0 bump.
What EC did to files
ConversionPolicycompared the detected charset's label against whatever the caller typed, so
every accepted alias for one code page failed the "already in the target" test.
-Target unicodeon a tree already in UTF-16LE decoded, re-encoded, verifiedand reinstalled every file to produce identical bytes — resetting every
modification time, and with
-Backupleaving a.bakand an.ecmeta.jsonbeside each. Identity is now the resolved code page.
sample. A file clean for 64 KiB and invalid afterwards was reported
Unchanged, and whether EC noticed depended only on which target was named.Costs 0.04-0.10 ms per MiB, because Convert already reads every byte to hash it.
approved.
-PlansetsWhatIfand nothing decoded the file, so a planrecorded
Action=Convertwith no reason for a source that cannot be read,exited 0, and showed the reviewer nothing. Decode only; the entry is now
Refuse, because a plan records the action.What EC reported
exception types; anything else escaped
Parallel.ForEachas anAggregateExceptionand took every unreached file with it.against a deny ACE: a scan whose entire base directory was unreadable printed
a header-only CSV and exited 0.
docs/CLI.mdpromised EC reports how many files each exclusion skipped.
-Validaterejections could carry no reason at all.CharsetNotAllowedis new;
UnknownEncodingis reused deliberately.ConversionPolicy.ReasonCodeFor, verified equivalent across all 256 reachableinput combinations.
failing read chunk". The offending bytes are reported instead.
ibm437console the CSV rendered "Gruesse aus Muenchen" as mojibake: the tool
producing in its own output the failure it exists to detect.
What EC documented
The cap is now a named constant a test can assert the documents against.
<file>.bakis a fixed name and a second conversion replaces it.docs/SAFETY.mdnow says so.docs/DEFECT-BACKLOG.mdrecords all twelve, one finding withdrawn once itsfix was shown to break four existing tests, and four left open.
Compatibility
Semantics stay at 6, plan schema 5, journal schema 4. These changes
make EC refuse more and convert less, which the approved-decision ceiling
already permits.
Exit codes move in both directions, which is why this is a minor and not a patch:
-FailOnChangeson a tree already in an alias of the targetUnchanged, 0Error, 3-Planover a source that cannot be decodedConvert, 0Refuse, 3-Validaterow outside the allowed listCharsetNotAllowedVerification
required to fail, the file restored byte-identical and confirmed by hash
Detection is untouched — no detector file changed — but
ConversionPolicydid change, so the exemption v3.11.2 claimed is unavailable here. The release
notes state this rather than quoting the v3.11.0 figures as though they were
evidence about this build.
Release notes
🤖 Generated with Claude Code