Fix four findings from a review of the released v3.12.0, and release v3.12.1 - #80
Merged
Conversation
The journal and the report are written once scanning has returned, so a destination that could never be written was discovered only after conversion had rewritten the files. 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 sits in RunConsoleMode, after option validation and before either mode dispatches, so one call site covers -Apply and every scan mode. Exit 3, not 1. docs/CLI.md assigns 3 to a report failure and ExitCodeContractTests pins it, with the reasoning stated in the test: a report that cannot be written is a processing failure, not a usage error. Putting the check in TryValidateOptions 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. Verified: exit 3, source SHA-256 unchanged, and an actionable message naming the missing directory. Mutation: with the guard reverted, 4 of the 5 new tests fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…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 ConversionPolicy.ToRowResult, whose fallback arm was Converted. Measured before the fix: -Apply exited 0, printed "1 selected, 1 converted, 0 failed", wrote a journal recording Status=Converted and PlannedAction=99 - and the source SHA-256 was unchanged. The journal asserted work that never happened. That 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: - ConversionPlan.Load rejects an undefined Action or SourceInterpretation, before any source is touched, with a message telling the user to re-run -Plan. - ToRowResult 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. Verified after: exit 1, source unchanged, no journal written. Mutation: reverting the Load check fails the Action case; restoring the Converted fallback fails the mapping test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…he source Both found by an independent review checking the file's own claims against the code, which is the check this file exists to make possible. 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 - a user may well want to convert them. What is actually excluded is .bak, .ecmeta.json, temporary conversion files, and the output paths of the running command. An old-plan.json in the scan root is detected as ASCII and scanned, matching the documentation and contradicting the row. This is the same failure as the drive-root defect: a record trusting a summary instead of the source. The row now says the record was wrong rather than quietly restating the behaviour. CSV formula injection is withdrawn: it does not reproduce. DirectoryTraversal resolves every file through Path.GetFullPath, so the File column always begins with a drive letter or a UNC prefix and can never begin with =, +, - or @. A source named "=1+1.txt" produces a cell reading C:\...\=1+1.txt. Reopen only with a demonstrated reachable field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 - 454 lines of it - and four of its own artifacts did not use it. What each one cost: - The plan is the worst. A truncated plan destroys the reviewed plan a user was about to apply, and re-running -Plan produces one nobody has reviewed. - The journal is this product's audit trail; a truncated one means a conversion happened with no readable record of it. - Settings is EC-16, and its backlog note records that it has already produced a smoke-test failure that read as a product bug. - The report is regenerable, and is included because a fifth way to save a file is the thing worth avoiding. AtomicArtifactFile writes to a temporary file beside the destination, under the suffix scans already exclude so a leftover cannot become a scan candidate, then reuses EncodingConverter.AtomicReplaceForBackup. The recovery sidecar keeps its own writer: it also reads back and verifies what it wrote, which is more than this does and should not be reduced to this. Flush(flushToDisk: true) before the rename, because renaming a file whose bytes are still only in the page cache makes the install atomic and the artifact empty. Measured over 2,000 files and a 402 KB report: 152-153 ms with the flush, 153-154 ms without. No measurable cost at this scale. The report is byte-identical to the one the previous writer produced, BOM included, checked against a report generated before the change. Mutation: with the truncate-in-place write restored, 3 of the 6 new tests fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ad of carrying it Three findings from the review of the released 518a844, plus the two rows that did not survive being re-derived from the source - which is the kind this file exists to catch, and the second time it has caught one. EC-16 is marked fixed, with the note that it was closed by generalising a mechanism rather than by anyone deciding that row was urgent. That is worth recording: its scored row says it had already caused a smoke-test failure that looked like a product bug, and it still was not what prompted the change. The summary line no longer states a running total. Its arithmetic had stopped reconciling with the table it summarises - it claimed twelve open against eleven open rows - so it now states a figure counted from the rows, says it was counted, and names the drift as the thing this file exists to prevent. Verified against the file: seven open rows carrying an EC- or CX- number, nine open rows in total, four further findings kept as prose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Select exact combo items even when WinForms reports them below the popup viewport, and target the supplied modal for keyboard fallback. Add phase J for the out-of-directory source-choice refusal, including byte-level evidence and mutation coverage, and update the ten-phase records.
Phase J closed the automation-driver defect, and its two records were correctly removed - the scored-table row and the sentence saying it stayed open on its own account. The summary line was then decremented as though a ninth open row had gone, and it had not: the driver defect never appeared as an "open" row in the chronological tables at all. The nine are unchanged: EC-08, EC-15, EC-17, EC-18, EC-20, EC-23, CX-06, ambiguous BOM-less UTF-32, and the force-close race. Third time this line has drifted from the rows it summarises, and the first time it drifted in a commit made after reading the paragraph warning about it. Two sets that overlap but are not the same - rows marked open, and rows carrying a score - are easy to decrement together and wrong to. A re-derivation script is the fix, and is already asked for separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Keep off-screen selection for the combo-scoped search, but name and document the process-wide fallback as visible-only because an off-screen match may belong to another collapsed combo with the same label.
The script parsed only under pwsh 7. It is UTF-8 without a BOM and contained three em-dashes; Windows PowerShell reads a BOM-less .ps1 as the system ANSI codepage, so each one became three bytes and the quote inside the summary string terminated it early. The reported error named a missing string terminator, which points at neither the character nor the encoding. It failed on this machine for a second reason: pwsh is not installed system-wide, so the documented command cannot run and the obvious substitution gives that parse error. The em-dash is now built from its code point and used in both places. One of them mattered more than the parse failure: the ledger writes an em-dash where a finding carries no score, and the check for a missing score compared against a literal. Repairing only the summary string would have left that comparison matching mojibake - a check that passes while testing the wrong thing, in the file whose purpose is being checkable. The script is now pure ASCII, so it needs no BOM and no shell assumption. Verified: passes under Windows PowerShell 5.1 and pwsh 7, reporting the same 61 findings. Mutation: changing the header's fixed count to 43 makes it fail with "Count mismatch for fixed: header 43, ledger 44", so it can still tell the two apart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…orks Three defects, each found only by running the command the file tells a reader to run rather than one that happened to be convenient while writing it. The header documented `pwsh`, which is not installed on a stock Windows machine. Windows PowerShell then failed for a second reason: $PSScriptRoot is not populated while parameter defaults are evaluated under -File, so the default path resolved to nothing and Join-Path threw before the script began. It is now resolved in the body, with $MyInvocation as a fallback. The third only appears outside this session. A stock machine has every execution policy scope Undefined, which means Restricted, and refuses to run an unsigned script at all. The documented command now passes -ExecutionPolicy Bypass, which applies to that one process and changes nothing on the machine. No machine setting was altered to make this pass. Each defect hid behind the way the previous check was run. The first two checks used the call operator against a full path, which populates $PSScriptRoot. Every check ran inside a session carrying PSExecutionPolicyPreference=Bypass at process scope, which child powershell.exe processes inherit - so the execution policy could not fail here however it was invoked. Reproducing it needed that variable cleared first, which is now how it was verified. Verified with the environment variable cleared, so a stock console is simulated rather than assumed: the previously documented command fails exactly as reported, and the documented one passes. Also verified under pwsh 7, and by four invocation forms under Windows PowerShell. Mutation: changing the header's fixed count to 43 still fails with "Count mismatch for fixed", and the file restores byte-identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Assembly version and README heading; --version and the built-in help both report 3.12.1 from the Release build. Patch, not minor. Every change corrects behaviour that was wrong: no new reason codes, no new report columns, no schema change, and nothing about detection or conversion policy. What EC converts, refuses and reports for a valid input is exactly what v3.12.0 did. One exit code moves, for one case: -Apply on a plan carrying an undefined action was 0 and "1 converted", and is now a refusal with 1. That 0 was a false success, so the notes state the change rather than treating a corrected lie as a compatible outcome. No schema or semantics bump. Semantics stay at 6, plan schema 5, journal schema 4, and a valid plan written by v3.12.0 applies exactly as before. Beyond the code, the notes cover three records. This release removes the project's only "covered by a unit test instead of a smoke phase" entry, and says that no production code changed to make the dialog drivable - the trade this suite exists to avoid, which a reader cannot otherwise tell from outside. The defect backlog is re-derived from source into a checkable ledger, after a third drift in its hand-maintained counts. And the comments added by this release were cut roughly in half, because several retold a story already held in the commit message, these notes, and the backlog - a fourth copy nothing keeps in sync, which is the same drift in a different place. They also carry what the verification does not cover. This release needs no corpus run, because it changes neither detection nor conversion policy - but the notes say plainly that the gap v3.12.0 recorded, a release that did change ConversionPolicy without one, is untouched by this release and still stands. Two clean releases in a row must not read as the gap having closed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
amrali-eg
force-pushed
the
fix/codex-review-findings
branch
from
September 7, 2026 22:59
6ea6df3 to
62b70da
Compare
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
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.
Two defects from an independent review of the released v3.12.0, one refactor taken because
the inconsistency it describes was the finding, one smoke phase that had been blocked by a
defect in the test driver, and a backlog rebuilt into something a script can check.
What EC did wrong
An unwritable output destination was discovered only after conversion. The journal and
report are written once scanning has returned, so a destination that could never be
written was found after the files had been rewritten — a user who asked for a journal
ended with changed files and no record of the change. Measured on all four shapes of the
mistake (
-Journalor-Report, under a missing directory or onto an existing one); allfour converted first. Now checked before either mode dispatches.
The exit code is deliberately unchanged at 3.
docs/CLI.mdcalls a report failure aprocessing failure and a test pins it; finding the failure earlier must change when it
is reported, not what it is called. The first attempt put the check among the usage
validators, which made it exit 1 and broke that contract — the existing test caught it.
A plan action no build ever wrote was reported as a conversion.
System.Text.Jsonaccepts a number for any enum, so a damaged plan could carry
"Action": 99and loadwithout complaint, then reach
ToRowResultwhose fallback arm wasConverted. Measuredbefore the fix:
-Applyexited 0, printed "1 selected, 1 converted, 0 failed", andwrote a journal recording
Status=Converted— with the source hash unchanged. The journalasserted work that never happened.
EC wrote its own artifacts less carefully than everyone else's. It installs a
converted file through a temporary file and a replacement, and writes its recovery sidecar
the same way with a read-back check — while its plan, journal, report and settings each
truncated their destination and wrote into it. The mechanism already existed and shipped;
four of its own artifacts did not use it. A truncated plan destroys the reviewed plan a
user was about to apply. Closes EC-16.
What the records got wrong
CX-07 recorded a fix that was never made, and should not be:
docs/CLI.mdstates theopposite behaviour deliberately. CSV formula injection is withdrawn — every path is
resolved through
Path.GetFullPath, so theFilecolumn always begins with a driveletter or UNC prefix.
The whole backlog was re-derived from source, not carried forward: 61 unique findings —
44 fixed, 13 open, 1 not reproduced, 1 withdrawn, 1 not a defect, 1 design decision. It is
organised by status rather than discovery date, every finding has a stable id including
the thirty-three that had none, and
docs/Test-DefectBacklog.ps1now recomputes everyfigure in the header and checks ids, statuses, scores and links.
That recheck found a third drift in the hand-maintained count — one introduced while
correcting the second — which is why the count is no longer hand-maintained.
What was untested and now is not
The source-choice refusal was this project's only fix covered by a unit test instead of a
smoke phase, because
SelectCombotimed out in that dialog state while the identical callin another phase succeeded. 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 exact-item
search rejected it, and the keyboard fallback then fronted the main window while a modal
review was open.
The suite is now ten phases. Phase J requires the review to stay open, name the file
outside its directory, and leave every byte unchanged. No production code changed to make
it drivable — the driver was fixed, EC was not reshaped, which is the trade this suite
exists to avoid making.
Compatibility
Semantics stay at 6, plan schema 5, journal schema 4. No detection or
conversion policy change. One exit code moves:
-Applyon a plan carrying an undefined actionConverted, 0That 0 was a false success. A valid plan written by v3.12.0 applies exactly as before.
Verification
restored byte-identical and confirmed by SHA-256 — including phase J, checked by
reverting the guard it covers
with, 153–154 ms without
docs/Test-DefectBacklog.ps1passes under Windows PowerShell and pwsh 7, and was itselfchecked by changing a header count and requiring it to fail
policy, so the checklist does not require one — but the gap v3.12.0 recorded, a release
that did change
ConversionPolicywithout one, is untouched by this and still stands.Release notes · Defect ledger
🤖 Generated with Claude Code