Repository navigation
update-shape.py: apply puts every byte back under any OSError after its first write, says what it put back on stderr, and raises the error as it was, so a file gone between its stat and its read, a broken pipe or an EIO no longer leaves the tree half-written behind a traceback (#1173) - #1187
Conversation
…er its first write, says what it put back on stderr, and raises the error as it was, so a file gone between its `stat` and its read, a broken pipe or an `EIO` no longer leaves the tree half-written behind a traceback (#1173) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewer's GuideThe PR makes Sequence diagram for apply rollback after an OSErrorsequenceDiagram
participant Apply as cmd_apply
participant Ledger as Rollback
participant Stdout as stdout
participant Stderr as stderr
Apply->>Ledger: undo()
alt writes exist
Ledger-->>Apply: rollback message
Apply->>Stdout: flush()
Note over Stdout: OSError from flush is suppressed
Apply->>Stderr: print(undone)
Apply-->>Apply: raise original OSError
else no writes or commit landed
Ledger-->>Apply: None
Apply-->>Apply: raise original OSError
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
(lane openRepoShape-3, 2026-10-23T00:13:12Z) Independent review (RV1187, opus, ruling 8) of #1187 at head Verdict: READY. No MUST. Two SHOULDs, each a one-line test change measured below, are best taken in a fix round before the merge word, since this lands only after the v1.2.0 tag anyway. The fix is right. Re-measured with my own probe (a LOCAL
Added checks:
SHOULD 1,
SHOULD 2,
NIT 1, the body. The #1176 lines ("has no PR yet", "the one line it edits in update-shape.py is in NIT 2, Mutants (my own script, the whole file per mutant, restored,
Tests at the head, one pytest at a time:
Merges ( Sonar: gate OK, 0 new issues, 0.0% duplication on new code, no hotspots. CI on Body: a draft with no milestone and the follow-up sentence first. It ends Lane: openreposhape-3 (openRepoShape-3) |
…THONUNBUFFERED` so it passes whoever runs it, and the refused-read row pins that the rollback is said once, under the refusal and not above it (#1173) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
|
(lane openRepoShape-3, 2026-10-23T00:23:54Z) Delta review (RV1187, opus) of #1187's fix round: head Verdict: READY. No new finding. Land only once CI's
Lane: openreposhape-3 (openRepoShape-3) |
|
Sorry @brettheap, you've used your own review budget of 250,000 diff characters for the last 7 days. You can request another review in 4 hours and 9 minutes by commenting |



What changed
A follow-up outside v1.2.0 (Brett Heap, 2026-10-21T10:32:04Z). It lands only after the v1.2.0 tag, on a later merge word.
update-shape.py applyrolled back anOSErrorafter its first write only where the system had refused a read (#976). Any other one was raised as it was, with every copy, the pin and the manifest left written: a file gone between itsstatand its read, an interpreter or a working directory gone when a validator starts, an error that names no path (aBrokenPipeErrorfrom a flush into a pipe nobody reads any more, anEIO). The traceback sat over a half-written tree and said nothing of what was written, the outcomeRollback.writing(#285) and_rewrite_readme(#152) exist to prevent. It now puts every byte back first.update-shape.py:cmd_apply'sexcept OSErrorarm, whereunreadable_refusalanswers None, calls the new_roll_back_before_raising(ledger)and then re-raises with a bareraise. The three-arm shape is kept: aRefusalor aCommandFailedstill goes to_rolled_back, and a refused read is still madeupdate-unreadableand rolled back under it, with update-shape.py: a path in the root or the upstream this user cannot read isupdate-unreadableon every Python, so ascripts/it can list but not search is no longer fivecopy-missingand a validator it cannot stat is never SKIPPED #976's rows and messages as they were._roll_back_before_raising(ledger)asksledger.undo()and, when it returns None (no write had been made, or the commit landed and is the result), says nothing and returns. Otherwise it flushes stdout, letting anOSErrorfrom the flush go, and prints what was put back on stderr, in_rolled_back's own words.applyrolls back only on aRefusal(and aCommandFailed), which anOSErrorafter the first write is not (run_validator,Rollback.writing,_rewrite_readme), are put in the past tense, and_rewrite_readme's says what is still true: anOSErrorthe tool cannot name is a traceback, which names no step and no remedy.tests/test_update_shape_unreadable.py, besidetest_any_read_refused_after_the_writes_rolls_them_back(6 rows added, 54 in the file):test_any_oserror_after_the_writes_rolls_them_back_and_is_raised[a-file-gone | a-broken-pipe | EIO-naming-no-path]:_refuse_red_validatorsmade to raise aFileNotFoundErrornaming a path, aBrokenPipeErrorand anOSError(errno.EIO, ...)with no path. Each leaves the tree byte for byte as it was, says on stderr (and only there) what was put back, and raises the same error object.test_a_stdout_nobody_reads_is_rolled_back_under_its_broken_pipe: no stand-in. The tool runs as a subprocess with its stdout a pipe whose reader is gone, asapply --yes | head -1leaves it. The tree is as it was, and what went back is on stderr above the traceback.test_an_oserror_before_the_first_write_is_raised_and_says_nothingandtest_an_oserror_after_the_commit_puts_nothing_back: the two ends where there is nothing to put back. The error is raised as it always was, stderr is empty, and after the commit the commit and its tree stand.Decided where the Direction left it open
_rolled_back._rolled_backends inraiseof a refusal orreturn 2, and this arm has to re-raise the original error with its traceback. The helper does the first half (undo, say) and leaves the raise to the arm.OSErrorbefore the first write, or after the commit landed, is raised exactly as it always was (Rollback.undoreturns None in both)._rolled_backflushes stdout so thecopiedlines come before the rollback in a merged log. Here the error being answered may be stdout's own, a pipe nobody reads, and an unguarded flush would raise over what must still reach stderr.contextlib.suppress(OSError)lets it go, and the original error is the one raised. M3 puts the bare flush back._rolled_backdoes for a failed git command. M6 prints it on stdout.Left out of scope
_rolled_back's ownsys.stdout.flush()has the same closed-pipe weakness (found while measuring this; measured below). ARefusalafter the first write with stdout a pipe nobody reads restores the tree, sinceundo()runs before the flush, and then loses the refusal to aBrokenPipeErrorfrom that flush, exit 120. It is the same cure as the helper's (contextlib.suppress(OSError)on the flush), but it changes the answer to aRefusaland aCommandFailed, which this issue's Goal keeps as it is, so it wants an issue of its own.OSErrorexception after the first write (aKeyboardInterrupt, or a bug'sTypeError) still escapes with nothing put back. The Goal isOSError.bump-leg.py's own rollback arm (bump-leg.py_apply_rewriterolls back only onRefusalandCommandFailed, so aPermissionErroron a workflow file it rewrites, or theUnicodeEncodeErrorthat #230'serrors="replace"moves the decode crash to, leaves the pin rewritten and the leg on the new commit, with a traceback and no rollback #253): the issue did not read it, and it is not touched. A sibling, update-shape.py and bump-leg.py: the byte-order-mark rows (#1050) hold what each rewriter returns but not the tools' own read and write, nor that only U+FEFF is cut, so autf-8-sigread that drops the mark and a cut that also takes spaces both survive #1176, is now the PR tests/test_bom_tool_io.py: the byte-order-mark rows hold each tool's own read and write and that only U+FEFF is cut, so autf-8-sigread that drops the mark and a cut that also takes spaces no longer survive (#1176) #1188 (aa0275be), which touches onlytests/test_bom_tool_io.py; merge-tree with it is clean (see Merges).docs/release-notes/v1.2.0.md:1820states this as a v1.2.0 known limit. That file records v1.2.0's state and is left alone.Measured
A scratch probe (not in the repository) runs
update-shape.py apply --root <root> --yes --upstream <local clone> --at B, with a LOCAL--upstream, a stubghand no network. In process,_refuse_red_validatorsraises the namedOSErrorafter the writes, and once with no stand-in at all: the tool as a subprocess with its stdout a closed pipe. It prints whatmain()raised, what stderr held, and what the root holds afterwards.Before, at
961640c6(the first and the fourth of the four cases):BrokenPipeErrorandOSError(EIO)after the writes read the same as the first: stderr empty, pin at B, the copy written, three files modified.After, at
7a9d397b:BrokenPipeErrorandOSError(EIO)read the same as the first: the three files restored, the pin at A, the status clean. The error raised is the same in each; exit 120 is the interpreter's own code for a stdout that would not flush at exit, as before.The out-of-scope find, measured at
7a9d397bby a second probe: the pin made read-only so the re-pin is refusedupdate-write-failedafter the copy. With stdout read, stderr holdsREFUSED update-write-failed: ...andROLLED BACK: ... restored scripts/validate-manifest.py, exit 2. With stdout a closed pipe:The tree is back; the refusal's own
REFUSEDline and itsROLLED BACKlist never reach stderr.Tests
Each file runs alone, on Python 3.12.3, as
timeout 1500 /usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch>/bt-gate -o tmp_path_retention_policy=failed <file>. A stubghthat exits 97 is first on PATH, withGH_TOKEN=bogus, an emptyGH_CONFIG_DIR,GIT_SSH_COMMAND=false,GIT_CONFIG_GLOBALa scratch file that sets onlygc.autoandmaintenance.auto, andEMAILset. Every run exited 0.The guards the brief names come first, then the update-shape files, then the other files that name
update-shape.pyand read whatapplyraises or prints.tests/test_shape_doctor.pyis not run:shape-doctor.pyis not changed.tests/test_update_shape_unreadable.pytests/test_repo_hygiene.pytests/test_path_guard_hygiene.pytests/test_no_auto_maintenance.pytests/test_agents_md_codes.pytests/test_cli_reference.pytests/test_update_add_codes_documented.pytests/test_update_shape_partial_rollback.pytests/test_update_shape_messages.pytests/test_update_shape_apply_lines_and_headers.pytests/test_update_shape.pytests/test_update_shape_landing.pytests/test_update_shape_rewrite_symlink.pytests/test_update_shape_add_symlink.pytests/test_update_shape_add_not_a_directory.pytests/test_update_shape_add_ignored.pytests/test_update_shape_add.pytests/test_update_shape_branch_any_directory.pytests/test_update_shape_hardlink.pytests/test_update_shape_header_comments.pytests/test_update_shape_kept_helper.pytests/test_update_shape_merged_away.pytests/test_update_shape_next_quoting.pytests/test_update_shape_paste_lines.pytests/test_update_shape_pr_base.pytests/test_doctor_unreadable_paths.pytests/test_doctor_unreadable_pin.pytests/test_bom_reads.pytests/test_bom_rewrites.pytests/test_stdout_encoding.pytests/test_windows_paths.pytests/test_validator_child_encoding.pytests/test_pin_rewrite_layout.pytests/test_openreposhape_package.pytests/test_commit_trailers.pytests/test_refusal_default_remediation.pytests/test_chain_remedy_callers.pytests/test_pinned_copy_text.pyMutants
Each mutant breaks one behaviour of
update-shape.pyonce and runstests/test_update_shape_unreadable.py -k "oserror or nobody_reads"(the 6 new rows). The file is then restored from a saved copy and checked withcmp(exit 0 each time). Run twice, by the writer before the commit and again at the head, with the same verdicts.if refusal is None: raise, no rollbacktest_any_oserror_after_the_writes_rolls_them_back_and_is_raised[a-file-gone],[a-broken-pipe],[EIO-naming-no-path],test_a_stdout_nobody_reads_is_rolled_back_under_its_broken_pipeprint(undone, file=sys.stderr)removedsys.stdout.flush()test_a_stdout_nobody_reads_is_rolled_back_under_its_broken_pipe...is_raisedrows,test_a_stdout_nobody_reads_...,test_an_oserror_before_the_first_write_is_raised_and_says_nothing,test_an_oserror_after_the_commit_puts_nothing_backif undone is None: returnremoved, so nothing-to-put-back still printstest_an_oserror_before_the_first_write_is_raised_and_says_nothing,test_an_oserror_after_the_commit_puts_nothing_backprint(undone)in place ofprint(undone, file=sys.stderr)...is_raisedrows andtest_a_stdout_nobody_reads_...Merges
git merge-tree --write-treeof this head (cc8a87ee) withorigin/main(961640c6, frozen until the v1.2.0 tag) and with each open PR that touchesupdate-shape.pyortests/test_update_shape_unreadable.py, at the heads GitHub lists now. The open PRs were re-listed live after the fix commit (48 open); no PR other than these touches either file.All are clean. #1166 edits the same test file, in other rows, and merges without conflict. #1176 is #1188: it touches only
tests/test_bom_tool_io.py, no file this change touches.Fix round
RV1187's review (ruling 8, READY, no MUST) of
7a9d397basked for two one-line test changes and a body fix. One new commit,cc8a87ee, touchestests/test_update_shape_unreadable.pyalone;update-shape.pyis byte for byte as it was.PYTHONUNBUFFERED. With it exported as 1, stdout is unbuffered, so thecopiedprint breaks the pipe first andRollback.writinganswersREFUSED update-write-failed: ... Broken pipeinstead of the rollback list. The child's env is now{**os.environ, "LANES_LANE": "", "PYTHONUNBUFFERED": ""}, astests/test_bootstrap_refusal_exit.pyblanks it. At7a9d397bthe row FAILED withPYTHONUNBUFFERED=1exported (stderr beganREFUSED update-write-failed); at this head it passes with it exported as 1 and with it unset (2 passed each, with the refused-read row).test_any_read_refused_after_the_writes_rolls_them_backnow also asserts(out + err).count("ROLLED BACK") == 1. Mutant M8 (the_roll_back_before_raising(ledger)call moved aboveif refusal is None) SURVIVED all 54 rows at7a9d397b(54 passed) and is KILLED now, by that row alone, withupdate-shape.pyrestored and checked bycmp(exit 0).utf-8-sigread that drops the mark and a cut that also takes spaces both survive #1176 lines are corrected above and in Merges: update-shape.py and bump-leg.py: the byte-order-mark rows (#1050) hold what each rewriter returns but not the tools' own read and write, nor that only U+FEFF is cut, so autf-8-sigread that drops the mark and a cut that also takes spaces both survive #1176 is tests/test_bom_tool_io.py: the byte-order-mark rows hold each tool's own read and write and that only U+FEFF is cut, so autf-8-sigread that drops the mark and a cut that also takes spaces no longer survive (#1176) #1188 (aa0275be), which touches onlytests/test_bom_tool_io.py, and merge-tree with it is clean.update-shape.py:5276, left as it is. RV1187 marked it optional and "fine as is" (when stderr itself is the dead pipe, the print of the rollback list raises its ownBrokenPipeErrorover the original; the tree is already back and the exit is 120, as at main)._rolled_back's flush above,landing_failed's own stdout flush, which after the commit loses the refusal and the NEXT line the same way, andRollback.writing, whosecopiedprint underPYTHONUNBUFFERED=1reports a broken stdout pipe asREFUSED update-write-failed.Re-run at
cc8a87ee, one pytest at a time, each under the stub-ghhost rules:tests/test_update_shape_unreadable.pywithtests/test_update_shape.pyandtests/test_update_shape_add_ignored.py, in one pytesttests/test_path_guard_hygiene.pytests/test_repo_hygiene.pytests/test_no_auto_maintenance.pyMutants M1 to M6 and M8, each run against the whole of
tests/test_update_shape_unreadable.py(54 rows), the file restored from a saved copy and checked withcmp(exit 0 each):if refusal is None: raise, no rollback...is_raisedrows andtest_a_stdout_nobody_reads_...print(undone, file=sys.stderr)removedsys.stdout.flush()test_a_stdout_nobody_reads_......is_raisedrows,test_a_stdout_nobody_reads_...,test_an_oserror_before_the_first_write_...,test_an_oserror_after_the_commit_...if undone is None: returnremovedtest_an_oserror_before_the_first_write_...,test_an_oserror_after_the_commit_...print(undone)on stdout...is_raisedrows andtest_a_stdout_nobody_reads_..._roll_back_before_raising(ledger)call aboveif refusal is None(SURVIVED at7a9d397b)test_any_read_refused_after_the_writes_rolls_them_backCloses #1173
Lane: openreposhape-3 (openRepoShape-3)
🤖 Generated with Claude Code