Skip to content

update-shape.py: apply puts its REFUSED and ROLLED BACK report, and a failed push's account of the commit that stands, on stderr when nobody reads stdout any more, and a broken pipe on a copied line is rolled back and raised as itself, not called update-write-failed (#1218) - #1241

Draft
brettheap wants to merge 1 commit into
mainfrom
l3-1218-stdout-report

Conversation

@brettheap

@brettheap brettheap commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

What changed

A v1.2.1 follow-up from lane 3's independent reviews (#1218). It lands under Brett Heap's v1.2.1 merge word (2026-10-23T13:10:26Z) once reviewed and green.

update-shape.py apply now puts its failure report on stderr whatever stdout is. Its three parts are all in scope: #1187 has landed (8899c69b), so cmd_apply's OSError arm rolls back through _roll_back_before_raising on main.

Two flushes that raised over the report (_rolled_back, landing_failed)

  • Both flushed stdout bare before writing to stderr. Into a pipe whose reader had gone, the flush raised BrokenPipeError over the whole report.
    • In _rolled_back, the tree was put back, and stderr held a traceback instead of the REFUSED line and the ROLLED BACK list.
    • In landing_failed, stderr said neither which step failed nor that the commit stands.
  • Both now call a new _flush_stdout(): a contextlib.suppress(OSError) around the flush, as adopt-project.py's _flush_stdout does.
  • The flush is still made first, so a log that merges the two streams keeps its order.
  • The bytes a closed pipe will not take stay buffered. The interpreter tries them once more at exit and reports that itself, with exit 120, as the issue's Direction says.

A broken pipe on a copied line was called a failed write (Rollback.writing)

What I decided where the Direction left it open, and why

Tests (in tests/test_update_shape_unreadable.py, beside #1187's pipe row, and in tests/test_update_shape_messages.py, beside test_the_bare_form_a_failed_push_prints_carries_the_words_too)

  • test_a_refusal_after_the_writes_reaches_stderr_when_stdout_is_unread: runs the real tool, with a read-only pin and stdout a closed pipe, block-buffered.
    • Its stderr starts with REFUSED update-write-failed: ... and the ROLLED BACK list, and the tree is as it was.
    • needs_a_mode: a mode root writes through is skipped as root and on Windows.
  • test_a_broken_pipe_at_a_copied_line_is_not_a_failed_write: runs the real tool with PYTHONUNBUFFERED=1 and a closed pipe.
    • Its stderr holds no update-write-failed. It is the ROLLED BACK list, then the traceback, which ends BrokenPipeError: [Errno 32] Broken pipe, and the tree is as it was.
  • test_a_refused_push_is_reported_on_stderr_whatever_stdout_is: calls push_branch with an origin that does not exist and a stdout whose flush raises.
    • It raises LandingIncomplete.
    • Its stderr starts with REFUSED pushing shape/x. and holds that commit stands and nothing was rolled back.
  • 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's test_a_stdout_nobody_reads_is_rolled_back_under_its_broken_pipe keeps its assertions. Its subprocess call moved into a shared helper, apply_b_into_a_closed_pipe, so the three closed-pipe rows are not three copies.
    • Its docstring no longer says that unbuffered "the refusal for it answers instead": that is the new unbuffered row's case now.
    • ROLLED_BACK became rolled_back(*paths), with the old constant kept as its three-path call.

No command, flag or refusal code is added; no documentation names any of this.

Measured before and after

All runs on Python 3.12.3, uid 1000. Each is apply --at B --yes in a scaffolded root with a local --upstream, under a stub gh (exit 97) with no network.

1. A read-only pin, stdout a closed pipe, block-buffered

Before: exit 120, and the tree is as it was. Stderr is only tracebacks: the refusal shows inside the chain, During handling of the above exception, another exception occurred:, ending with

  File ".../update-shape.py", line 5236, in _rolled_back
    sys.stdout.flush()
BrokenPipeError: [Errno 32] Broken pipe
Exception ignored in: <_io.TextIOWrapper name='<stdout>' mode='w' encoding='utf-8'>
BrokenPipeError: [Errno 32] Broken pipe

After: exit 120, and the tree is as it was. Stderr is below; the Remediation line that follows the list is left out of this quote.

REFUSED update-write-failed: re-pinning contracts/shape-pin.yaml and project.yaml failed at contracts/shape-pin.yaml: Permission denied
ROLLED BACK: every byte this command wrote has been put back, and the tree is exactly as it was:
    restored  scripts/validate-manifest.py
Exception ignored in: <_io.TextIOWrapper name='<stdout>' mode='w' encoding='utf-8'>
BrokenPipeError: [Errno 32] Broken pipe

2. PYTHONUNBUFFERED=1, stdout a closed pipe

Before: exit 2, and the tree is as it was. Stderr, without its Remediation line:

REFUSED update-write-failed: writing the copies and the --add files failed: Broken pipe
ROLLED BACK: every byte this command wrote has been put back, and the tree is exactly as it was:
    restored  scripts/validate-manifest.py

After: exit 1, and the tree is as it was. Stderr:

ROLLED BACK: every byte this command wrote has been put back, and the tree is exactly as it was:
    restored  scripts/validate-manifest.py
Traceback (most recent call last):
  ...
  File ".../update-shape.py", line 4912, in _write_copies
    print(f"  copied   {row.path}")
BrokenPipeError: [Errno 32] Broken pipe

3. A refused push after the commit, stdout a stream whose flush raises (in process, push_branch against a bare origin whose pre-receive hook refuses)

Before: BrokenPipeError raised, and stderr empty.

After: LandingIncomplete raised. Stderr is REFUSED pushing shape/x. THE EXACT COMMAND WAS:, the push, git's ! [remote rejected] shape/x -> shape/x (pre-receive hook declined), then

shape/x carries the re-pin at 3e7ebd4097a3, and origin does not have it yet: that commit stands and nothing was rolled back. What is left is the push and the pull request: the NEXT line makes both.

4. A stdout that is read prints byte for byte as before

Each case was run by the tool at main and by this branch's tool, on two copies of one root with fixed git dates. Stdout and stderr were compared apart and merged (2>&1), with the root's path replaced:

case exit main / branch separate streams merged
--branch shape/b, lands 0 / 0 identical identical
read-only pin, update-write-failed 2 / 2 identical identical
--branch shape/b --push, push refused 2 / 2 identical identical

Out of scope

Tests

Each command ran alone, under a stub gh first on PATH (exit 97), GH_TOKEN=bogus, an empty GH_CONFIG_DIR, GIT_SSH_COMMAND=false, a scratch GIT_CONFIG_GLOBAL and EMAIL=tests@openreposhape.invalid, as /usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch> -o tmp_path_retention_policy=failed, on Python 3.12.3:

  • tests/test_update_shape*.py, all 19 files, at base 50ce8ab1, where the three changed files are the same blobs as at e04826db: 826 passed, 3 skipped in 878.91s (0:14:38).
  • The 47 files below, run as one pytest with -rs, at base e04826db: 3221 passed, 6 skipped in 1529.21s (0:25:29). The skips are a case-insensitive disk, Windows' default encoding and a session GIT_CONFIG_GLOBAL.
    • tests/test_adopt_attributes_eol_block.py, tests/test_adopt_eol_block_through_links.py, tests/test_bom_reads.py, tests/test_bom_rewrites.py, tests/test_bom_tool_io.py, tests/test_bootstrap_without_git.py, tests/test_agents_md_codes.py, tests/test_bump_leg_branch_and_push_line.py, tests/test_bump_leg_rollback.py, tests/test_chain_pin_sources.py, tests/test_chain_remedy_callers.py, tests/test_commit_trailers.py, tests/test_bump_leg.py, tests/test_doctor_partial_clone.py, tests/test_doctor_no_lazy_fetch.py, tests/test_doctor_unreadable_paths.py, tests/test_doctor_next_is_a_command.py, tests/test_load_yaml_refusals.py, tests/test_legs_readers_not_a_list.py, tests/test_family_py_unreadable.py, tests/test_family_member_name.py, tests/test_family_makefile_siblings.py, tests/test_pin_pattern_line_break.py, tests/test_pin_source_env_collisions.py, tests/test_doctor_pin_restore_merges.py, tests/test_refusal_default_remediation.py, tests/test_path_guard_hygiene.py, tests/test_doctor_unreadable_pin.py, tests/test_openreposhape_package.py, tests/test_pin_rewrite_layout.py, tests/test_pinned_copy_text.py, tests/test_shape_pin_row_path.py, tests/test_family.py, tests/test_ruleset_hint_tracking_branch.py, tests/test_scaffold_e2e.py, tests/test_shape_pin_files_not_a_list.py, tests/test_update_add_codes_documented.py, tests/test_family_printed_quoting.py, tests/test_repo_hygiene.py, tests/test_validate_pins_lockstep_lines.py, tests/test_stdout_encoding.py, tests/test_validator_child_encoding.py, tests/test_readme_windows_quoting_sentence.py, tests/test_windows_paths.py, tests/test_yaml_reader_spaces.py, tests/test_no_auto_maintenance.py, tests/test_cli_reference.py.
  • The five test_update_shape*.py files that read the family-root templates, rerun at base e04826db once test_validate_family_index_gitmodules.py and validate-family.py: a row pins path-twice's wording of whose file it puts back, the index's or HEAD's as the index holds none, a row pins the value check's reading of HEAD's where the deletion of .gitmodules is staged, and _listed_as_git_reads's docstring says it returns nothing where the work tree's file is on disk (#1181) #1208 changed validate-family.py: tests/test_update_shape.py, tests/test_update_shape_add.py, tests/test_update_shape_header_comments.py, tests/test_update_shape_kept_helper.py, tests/test_update_shape_pr_base.py: 352 passed in 302.81s (0:05:02).

Mutants

Each mutant was applied to update-shape.py and run against the four closed-pipe rows. The file was then copied back and checked with cmp against the saved change; every cmp reported identical.

mutant killed by
M1: the bare sys.stdout.flush() back in _rolled_back test_a_refusal_after_the_writes_reaches_stderr_when_stdout_is_unread
M2: the bare sys.stdout.flush() back in landing_failed test_a_refused_push_is_reported_on_stderr_whatever_stdout_is
M3: the BrokenPipeError re-raise in Rollback.writing removed test_a_broken_pipe_at_a_copied_line_is_not_a_failed_write
M4: _flush_stdout flushing bare (no suppress) the M1 and M2 rows, and #1187's test_a_stdout_nobody_reads_is_rolled_back_under_its_broken_pipe
M5: the bare sys.stdout.flush() back in _roll_back_before_raising test_a_stdout_nobody_reads_is_rolled_back_under_its_broken_pipe

At main's update-shape.py, the three new rows fail and #1187's row passes:

  • the read-only-pin row: stderr starts with Traceback;
  • the unbuffered row: REFUSED update-write-failed: writing the copies and the --add files failed: Broken pipe;
  • the push row: BrokenPipeError out of landing_failed.

Merges

git merge-tree --write-tree of this head (ab137eb5f0976a7c6c2bf40abaa31c6d86d63bca) with each target. The open PRs are the ones that touch this change's files, listed live with gh pr list --state open and fetched as refs/pr/<n>, each head checked against the list. Every merge is clean (exit 0, no conflict).

merged with sha merged tree result
origin/main 0ed3e57f821c474cf1cb97dc580d2d931c54b1a5 dee17ee209fef82cdd9f89e404f2b7ed007222c1 clean
#1231 (update-shape.py) aca00dfd918d78797001ef380602ebd372f55820 4634619a4b03cbe33b1c66c4af0e3603fcf6e863 clean
#1166 (update-shape.py, tests/test_update_shape_unreadable.py) 5102b42945baf3682bc51244a64050e654d25e32 fc07c0905455373e971d6e6e87869d302faa9d16 clean
#1133 (update-shape.py) a7bfee1bdb164eeca7e231aaa819acaf52cfbcbb b265030ecc8cac6488676ef79a8fd8f2677b8ef4 clean

The branch is cut from e04826db; origin/main has since moved to 0ed3e57f8 (#1121), which touches only adopt-project.py and tests/test_adopt_next_make_reads_first.py, none of this change's files, so no gate is stale.

Files

Only update-shape.py, tests/test_update_shape_unreadable.py and tests/test_update_shape_messages.py.

Closes #1218

Lane: openreposhape-3 (openRepoShape-3)

🤖 Generated with Claude Code

Summary by Sourcery

Ensure update-shape.py reports failures reliably when stdout is unreadable and accurately preserves broken-pipe errors.

Bug Fixes:

  • Preserve update-shape.py failure reports on stderr when stdout is unavailable, while retaining output ordering for combined logs.
  • Propagate BrokenPipeError from copied-line output so it is handled as a broken pipe rather than misreported as an update write failure.

Enhancements:

  • Centralize best-effort stdout flushing across rollback and landing failure reporting.

Tests:

  • Add coverage for unreadable stdout during rollback, refused pushes, and unbuffered copied-line output.

… a failed push's account of the commit that stands, on stderr when nobody reads stdout any more, and a broken pipe on a `copied` line is rolled back and raised as itself, not called `update-write-failed` (#1218)

`_rolled_back` and `landing_failed` flushed stdout bare before writing to
stderr. Into a pipe whose reader had gone, the flush raised
`BrokenPipeError` over the whole report: a read-only pin under
`apply --yes | head -1` put the tree back and exited 120 with only a
traceback on stderr. Both now flush through `_flush_stdout`, a
`contextlib.suppress(OSError)` around the flush, as `adopt-project.py`'s
does; `_roll_back_before_raising` (#1187) uses it in place of its own
inline guard.

`Rollback.writing` turned every `OSError` out of its block into
`update-write-failed`, so under `PYTHONUNBUFFERED` a broken pipe on the
`copied` print was refused as a failed write of the copy. A
`BrokenPipeError` is let through as itself, to `cmd_apply`'s arm for an
`OSError` no refusal can name, which puts every byte back and raises it
(#1187, for #1173).

A run whose stdout is read prints byte for byte as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@brettheap brettheap added this to the v1.2.1 milestone Oct 23, 2026
@sourcery-ai

sourcery-ai Bot commented Oct 23, 2026

Copy link
Copy Markdown

Reviewer's Guide

The PR hardens update-shape.py apply against closed stdout pipes by making failure-report flushing best effort, routing complete refusal and rollback/commit-state reports to stderr, and preserving the original BrokenPipeError when an unbuffered copied line fails; focused tests cover all affected paths without changing normal readable-stdout output.

Sequence diagram for apply failure reporting with closed stdout

sequenceDiagram
    participant Apply as cmd_apply
    participant Rollback as Rollback.writing
    participant Stdout
    participant Stderr
    participant Flush as _flush_stdout

    Apply->>Rollback: write copies and add files
    Rollback->>Stdout: print copied line
    Stdout-->>Rollback: BrokenPipeError
    Rollback-->>Apply: re-raise BrokenPipeError
    Apply->>Flush: _flush_stdout()
    Flush->>Stdout: flush()
    Stdout-->>Flush: suppressed OSError
    Apply->>Stderr: print rollback report
    Apply-->>Apply: raise BrokenPipeError
Loading

Sequence diagram for refused push reporting after commit

sequenceDiagram
    participant Apply as cmd_apply
    participant Git
    participant Stdout
    participant Flush as _flush_stdout
    participant Stderr

    Apply->>Git: push_branch()
    Git-->>Apply: CommandFailed
    Apply->>Flush: _flush_stdout()
    Flush->>Stdout: flush()
    Stdout-->>Flush: suppressed OSError
    Apply->>Stderr: print refusal and commit-stands report
    Apply-->>Apply: raise LandingIncomplete
Loading

File-Level Changes

Change Details Files
Preserve failure reports when stdout is connected to a closed pipe by centralizing a best-effort stdout flush before stderr reporting.
  • Added _flush_stdout() that suppresses flush-related OSErrors while preserving merged-stream ordering.
  • Reused the helper for rollback and failed-push reporting paths.
  • Kept the existing rollback, commit-state, and NEXT output semantics intact.
update-shape.py
Prevent stdout's broken pipe from being misclassified as a filesystem write failure during the copy phase.
  • Re-raised BrokenPipeError from Rollback.writing instead of converting it to update-write-failed.
  • Allowed the existing top-level OSError handling to restore changes, report rollback, and expose the original broken pipe.
update-shape.py
Added regression coverage for closed-pipe behavior in buffered, unbuffered, and failed-push scenarios.
  • Verified refusal and rollback reports reach stderr when stdout cannot be flushed.
  • Verified a broken copied line is not reported as update-write-failed and the tree is restored.
  • Verified failed pushes report the failing step and that the commit remains, while retaining the NEXT line behavior.
  • Factored subprocess setup and rollback-message construction for the expanded scenarios.
tests/test_update_shape_unreadable.py
tests/test_update_shape_messages.py

Assessment against linked issues

Issue Objective Addressed Explanation
#1218 Ensure apply’s rollback and post-commit failure reports reach stderr even when flushing stdout raises because its reader has gone away. ✅
#1218 Ensure a BrokenPipeError from a progress-line print inside Rollback.writing is propagated as the original error, not converted into update-write-failed, so cmd_apply can roll back and report it appropriately. ✅
#1218 Preserve the existing output behavior when stdout is readable, including stream ordering for merged logs. ✅

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@sonarqubecloud

Copy link
Copy Markdown

@brettheap

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-3, 2026-10-24T14:44:38Z) RV1241, independent review of #1241 at ab137eb: READY.

Measured with my own probe at main 50869f3 and at the head (Python 3.12.3, local --upstream, stub gh), stdout a pipe whose reader has gone:

  • Read-only pin, block-buffered: main exit 120, stderr only tracebacks; head exit 120, with the REFUSED line, the ROLLED BACK list and the Remediation line whole, then the interpreter's "Exception ignored" line. Tree as it was at both.
  • Same, PYTHONUNBUFFERED=1: main exit 2, "update-write-failed ... Broken pipe"; head exit 1, the ROLLED BACK list, then the BrokenPipeError traceback from the copied print. Tree as it was at both.
  • Failed push, the reader leaving after the validators: main loses the whole push report under a traceback (exit 120); head prints REFUSED pushing and "that commit stands and nothing was rolled back" whole (exit 120). The commit stands at both.
  • A reader gone from the start never reaches the push at either: run_validator's bare flush breaks first and 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 rolls back.
  • Stdout read to the end: 16 runs (4 cases, buffered or unbuffered, apart or merged) and two >log 2>&1 runs are byte-identical, main against head. The merged logs keep their order.
  • Exit 120 is the interpreter's flush at exit, as the Direction says. No caller reads it.
  • A BrokenPipeError out of writing can only be stdout's: the blocks start no child and write no pipe, and the error carries no filename, so it never becomes update-unreadable. cmd_apply's OSError arm rolls it back and raises it.

Tests at the head, in one pytest: the two PR files with test_update_shape.py, test_update_shape_header_comments.py and the three hygiene files: 507 passed, 2 skipped.
Mutants, re-made with my own script, each restored and checked with cmp: the guard in _flush_stdout, the re-raise in writing, the bare flush in _rolled_back and in landing_failed, and a helper that does not flush: all KILLED. Removing only _rolled_back's flush SURVIVES. No row pins that merged-log order. That gap predates this PR, and the behaviour measures the same as main, so it needs no change here.
Merges clean: main 50869f3 (tree 1eb50d9a), #1231, #1166, #1133. SonarCloud gate OK, 0.0% duplication, 0 new issues. CI: tests, tests-git-bash, package and SonarCloud all pass.

SHOULD (body only): delete the "Summary by Sourcery" block after the robot line.
NIT (body only): the Merges table and the sentence under it name origin/main 0ed3e57. Main is 50869f3 now. It touched only adopt-project.py, shape-doctor.py and their tests, and the merge is clean.
Out of scope, nothing asked: unbuffered, a reader that leaves after the validators turns the "committed on" print into a traceback after the commit, with no stderr account. Main does the same.
Not run: the three new rows against main's tool (the writer reports they fail there), Python 3.14 locally, and Windows.

Lane: openreposhape-3 (openRepoShape-3)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant