v3.11.2: apply plans rooted at a drive, refuse source choices that cannot be applied - #76
Merged
Merged
Conversation
Two unrelated changes, both small. ResolvePath built its containment prefix by appending a separator to the plan's root. TrimEndingDirectorySeparator leaves a drive or share root alone, so the prefix became "C:\\", which no resolved path can start with. Every file in a plan rooted at a drive was reported as resolving outside the plan's own directory and the run was refused whole - blaming the file paths rather than the root, which is the wrong place to look. The prefix now only gains a separator when it lacks one, and the match additionally requires the path to be longer than the prefix so the root directory itself is still not treated as a file. Widening the prefix must not widen what a plan will touch: the tests keep the escaping and empty-path cases rejected, and those four kept passing while the two drive-root cases failed against the old code. Verified end to end on a virtual drive as well as in unit tests: -Plan then -Apply with -BasePath X:\ now converts, where it previously refused. DefaultMaxParallelism was Math.Min(ProcessorCount, 4). Conversion is bound by per-file I/O latency, not CPU, so the cap bit well before core count did. Measured over 2,000 files: without backups 4,207 ms at 4 against 2,511 ms at 8; with backups 10,516 against 7,305. Past 8 the curve flattens and backup runs stop improving at all, so 8 rather than something larger. Re-measured against this build: 2,521 ms and 6,885 ms. ProcessorCount still binds first on small machines, and -MaxParallelism still overrides. 645 tests pass, none skipped, build warning-free. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two pre-v3.11.0 reviews produced a generated HTML report and a chat transcript. Neither reached the repository, so the only way to answer "what is still open?" was to trust a summary. The summary was wrong. EC-06 - a drive-root base path making every plan unusable - was recorded as Confirmed before v3.11.0, reported as closed, and shipped broken in both v3.11.0 and v3.11.1. It was rediscovered from scratch during an unrelated review and filed as a new finding before anyone recognised it as a known one. That is what an untracked list costs. Every status here was re-derived from the source rather than carried over. Twenty-five are fixed, nine are open, and one could not be reproduced - which the table says plainly instead of calling it fixed, because a failed reproduction is not evidence of a repair. The nine that remain are recorded with what a reader would notice first: a valid BOM losing to the entropy gate, settings written truncate-in-place, a detection read that permits concurrent writes, a redundant decode per pass, then five contract and clarity issues. None writes to a file nobody approved, which is why none of them blocked a release. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The orchestrator, the window's conversion path and the review dialog had a lighter read than the conversion core during the earlier review. Reading them properly turned up two things, neither of them severe. A ticked file can be dropped from a source choice in silence. The review's refused list carries each row's resolved path, and the ticked-file collector filters out rows whose path is null without saying so. That was live until today: with a drive-root base directory every row resolved to null, so choosing an encoding and clicking the button reported "Conversion cancelled. No files were modified." The user's choice was discarded and the message blamed a cancellation nobody made. That is EC-06 reaching the window, which neither the original finding nor this review had connected. The trigger is fixed; the silent drop is the part worth keeping written down. Force-closing during a run can throw on the way out. Abandoning the run on a second close is deliberate and correct, but the worker may then marshal a confirmation to a form that no longer exists. An error dialog at exit, not lost work. Reasoned from the code rather than reproduced, and labelled that way. The pass also settled several suspicions as unfounded, which is why they are not listed: an interrupted run cannot mark a written file as not attempted, the source-choice loop cannot spin, and the plan binding cannot throw on duplicate paths. Each was checked rather than assumed. Counts corrected: nine of the original thirty-five remain open, five findings have been raised since v3.11.1 with one already fixed, thirteen open in total. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Conversion appeared to hash the same bytes several times over. Three variants were built on throwaway branches and measured interleaved against one baseline, because an earlier non-interleaved comparison had drifted enough to change the answer. Only one was faster, and it was the one that costs the most: digesting the backup while copying saves 15% by never reading the .bak back, which is the only thing proving the restore point on disk is intact. The headline result is the algorithm swap. XxHash128 is 6.8 times faster than SHA-256 in isolation and made EC very slightly slower, 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, MD5 and SHA-512 - so the lighter cryptographic options are slower as well as weaker. A test that verifies a recorded hash independently failed under the swap, which is the cost stated as an assertion rather than an opinion. Also recorded: LEN splits its hashing by whether the value is durable, using XxHash3 for a content digest it discards and SHA-256 for what it writes down, and compares backup hashes with FixedTimeEquals where EC compares hex strings with OrdinalIgnoreCase. Neither is wrong for accidental corruption. The drift is the finding, and nothing checks it the way detector parity checks the shared detector. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The rejected optimisations look obviously right from the source: the same bytes are read up to seven times per converted file. Recording only the verdict invites someone to re-derive the whole experiment, so the section now states the conditions the numbers depend on and why they favour leaving it alone. Three points a future reviewer needs and would otherwise have to rediscover. The ceiling is 15% and it is the variant that costs the restore-point proof. The measurements are conditional on a fast local disk, a warm cache and eight-way parallelism, and changing those mostly raises the value of the checks rather than the value of removing them. And the safety argument does not rest on the measurement at all - the re-reads would still be the only proof that the file matches what was approved and that the backup exists, however fast a future machine made the alternative. The section ends by naming the honest target if throughput ever does matter: the backup read taken immediately after its own flush to disk, made cheaper rather than deleted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The review's refused rows each carry the path resolved for them, and the ticked set was built by filtering out the ones that resolved to nothing. When every row resolved to nothing the set came out empty, the orchestrator found no file matching the scope, and the run reported "Conversion cancelled. No files were modified." - a cancellation the user never asked for, after they had ticked files and chosen an encoding. The drive-root defect was one way in and is fixed, but not the only one. The results list is cleared when a scan starts and never when the directory box changes, and that box takes typing, a recent entry, or a dragged folder. Scan one folder, point the box at another, and every row in the next review resolves outside the plan's root. The Proceed path already handles this: it runs the staleness check and reports files that resolve outside the directory. Only the source-choice path failed silently, because it re-plans before that check runs. The dialog now says which ticked files it cannot act on and what to do about it, and leaves itself open. Verified by reading rather than by driving. A smoke phase was attempted and abandoned; the diagnosis and what remains are recorded in the backlog, and the nine existing phases still pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both the defect and the fix were reproduced by hand. Before: the review closed and the status bar reported a cancellation nobody asked for, after the user had ticked a file and chosen an encoding. After: it stays open and says which ticked files it cannot act on. Until this run the defect existed only as a reading of the code. That also settles where the abandoned smoke phase went wrong. The dropdown works perfectly by hand, so SelectCombo timing out is a defect in the automation driver rather than in EC - most likely its keyboard fallback calling SetForegroundWindow on the main window while a modal review is open. The note said the cause was unknown; it is now narrowed to the layer it belongs to, which is most of what the next person needs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The refusal had no automated coverage because the smoke phase for it could not be made to work. A manual procedure is not the fallback: the whole point of the nine phases was retiring one. The decision moves out of the click handler into DescribeUnusableScope, which returns why the ticked rows cannot be acted on or null when they can. That is a seam a test can reach. PerformClick does nothing on a control that is not effectively visible, and a form this test never shows makes every child invisible, so a test driving the button would have to show a window - and the unit suite must not need an interactive desktop. The test builds a plan rooted outside its own files, which is what the window produces whenever the directory box changes after a scan, ticks the row, confirms the resolved path really is null, and asserts the refusal explains itself and reports no choice. Mutation-checked: with the unresolvable count forced to zero the test fails, and the file was restored byte-identical. 646 tests pass. The driver defect behind the abandoned phase stays open on its own account. It will bite any future phase that touches a combo inside a dialog, and the backlog now says where to look rather than calling the cause unknown. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The open list called EC-18 a redundant full-file decode per pass and put it fourth. Measuring it says otherwise: IsAmbiguousBomlessUtf16 aborts at the first invalid sequence, so a provable file usually settles inside the first buffer and never reads the rest. Built the worst case on purpose - megabytes of ASCII-range UTF-16, which decodes happily in either order, with one proving character at the very end so the probe has to read everything - and it still showed no measurable difference against the same content carrying a byte-order mark. It is untidy, not slow, and now sits last with the numbers attached. The first attempt measured the wrong thing. Cyrillic content turned out to be ambiguous, so the flag went true, the short-circuit fired and the files were refused rather than converted, which made the BOM-less set look faster. The report's AmbiguousBomlessUtf16 reason code is what caught it. EC-15 moves up in its place, with what is actually at stake spelled out: nothing can be weakened by editing those booleans, because EC ignores them and always does the strict thing. The risk is a reader treating them as evidence that a check ran, when they are constants that would keep saying true if a future build stopped running it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Writing passed a configured options object; reading passed none. Nothing observable changes today, because both settings on that object affect writing only. What changes is what a later edit can do: adding a naming policy to make plans prettier would have altered every file EC writes without altering what EC accepts, and each one would have stopped loading - silently, because a reader that matches no property yields defaults rather than an error. Reader and writer now share the one object, in the plan store and the metadata store. The journal has no production reader; only tests deserialise it. No test demonstrates this, and none can without adding a setting to production code purely to make a test fail. Every other fix on this branch was mutation-checked; this one is not, and saying so is better than inventing a test that proves something else. What it does have is the full suite still passing and a plan round-trip over paths with Arabic text and an apostrophe. The backlog now scores what is left on two axes rather than one. Impact is what a user loses when it happens; 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 - the BOM-less UTF-32 refusal gap and the semantics booleans sit at opposite corners of exactly that. EC-19 moves to not reproduced. It was inspection-only and traced the wrong object: ConvertFiles re-resolves the codec by name, so the detector's BOM-less instance never reaches the guard. A file beginning with two byte-order marks is refused on both the automatic and the explicit path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Assembly version and README heading; --version reports 3.11.2 from the Release build. No schema or semantics bump. The drive-root fix changes what happens when such a plan is applied - it converts where it previously refused - but the decisions a plan records mean what they always meant, and only the resolution of its paths was wrong. The notes say that rather than leaving a reader to wonder why semantics stayed at 6. The notes also carry what the verification does not cover: the options-object change is the one fix here no test can demonstrate, and the release has no corpus run because nothing in it touches detection or conversion policy. 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.
v3.11.2. Two silent failures made loud, a measured speedup, and the first
backlog this project has kept.
A plan rooted at a drive can be applied
-BasePath D:\produced a normal-looking plan that-Applythen refusedentirely, reporting every file as resolving outside the plan's own directory.
The containment check appended a separator to a root that already ended in one,
so the prefix became
D:\\and nothing could ever match it.This shipped broken in v3.11.0 and v3.11.1. It was found before v3.11.0,
recorded as Confirmed, reported as closed, and rediscovered from scratch during
an unrelated review — filed as a new finding before anyone recognised it. That
is why this release also adds a backlog.
A source choice that cannot be applied is refused, not dropped
Refused rows in the review each carry the path resolved for them. Rows resolving
to nothing were filtered out of the ticked set in silence, so the confirm button
did nothing and the run reported "Conversion cancelled. No files were
modified." — after the user had ticked files and chosen an encoding.
The drive-root defect was one way in, not the only one: the results list is
cleared when a scan starts and never when the directory box changes, and that
box takes typing, a recent entry, or a dragged folder.
Both the defect and the fix were reproduced by hand in the window before and
after. The Proceed path already handled this correctly — only the source-choice
path failed silently, because it re-plans before the staleness check runs.
Conversion is faster
min(ProcessorCount, 4)→min(ProcessorCount, 8). Conversion is bound byper-file I/O latency, not CPU, so the cap bit long before core count did.
Past 8 the curve flattens and backup runs stop improving.
Reading uses the writer's options
The plan and sidecar stores passed a configured options object when writing and
none when reading. Nothing observable changes today; a setting added later for
the writer would have altered every file EC produces without altering what EC
accepts, and each would have stopped loading silently.
A defect backlog
docs/DEFECT-BACKLOG.md— all thirty-five findings from the two reviews beforev3.11.0, re-derived from the source rather than trusted from a summary, plus
what has been found since. Open items are scored on two axes, what a user loses
and how easily it happens, because one number hides the difference between a
critical impact nobody can trigger and a low impact everyone trips over.
It also records three hashing optimisations built, measured and rejected — one
of which made EC slower despite using a hash 6.8× faster in isolation — so the
next person need not re-derive them.
Verification
restored byte-identical — except the options-object change, which no test
can demonstrate without adding a setting to production code purely to make one
fail. Stated rather than papered over.
detection or conversion policy; this changes neither.
Compatibility
Conversion semantics stay at 6, plan schema 5, journal schema 4, exit
codes unchanged. The drive-root fix does change what happens when such a plan is
applied, but the decisions a plan records mean what they always meant — only the
resolution of its paths was wrong, so the semantics version is deliberately left
alone.
🤖 Generated with Claude Code