Skip to content

update-shape.py: a line with U+00A0 or U+3000 in front of shape: or files: is no block header, as it is none to the reader (#1220) - #1231

Draft
brettheap wants to merge 2 commits into
mainfrom
l3-1220-block-header-blanks
Draft

brettheap wants to merge 2 commits into
mainfrom
l3-1220-block-header-blanks

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 (#1220). It lands under Brett Heap's v1.2.1 merge word (2026-10-23T13:10:26Z) once reviewed and green.

update-shape.py's _is_block_header, which finds the pin's files: and the manifest's shape: header, cut each line with str.strip() and str.lstrip() and no argument. With no argument they also cut U+00A0 and U+3000. The reader's blanks are a space and a tab (_YAML_WHITE, used by _significant_text and _split_key), so to it a line that opens with U+00A0 or U+3000 and then reads shape: is the key <U+00A0>shape, a key of its own. This tool took it for the header.

  • update-shape.py: _YAML_WHITE is imported from repo_shape, one more private name beside _split_key and _strip_comment. _is_block_header strips with line.strip(_YAML_WHITE) and measures the blanks in front with line.lstrip(_YAML_WHITE). The docstring sentence that said the text "is .strip()ped first, as _tokenise strips it" was not true of _tokenise; it now says the text is stripped of _YAML_WHITE as _significant_text does, and why. The import comment says the same in one sentence. No command, flag, code or message changed.
  • tests/test_update_shape_apply_lines_and_headers.py and tests/test_update_shape_header_comments.py: the rows the issue names, and the end-to-end runs it asks for (below).

Fix round (RV1231)

  • The four composite assert a and b fixture checks in tests/test_update_shape_apply_lines_and_headers.py (SonarCloud python:S9073) are two asserts each, each keeping its "fixture: ..." message.
  • The docstring of test_a_files_key_that_opens_with_a_blank_the_reader_keeps_is_carried_over no longer says main's result "did not read back". It did: rewrite_fault reads only the commit and the tree, so it returned None; the result lost its files key, and validate-pins refused it after the write.
  • A tab-in-front row (a tab, then files:) joins the header rows, so the tab half of _is_block_header's column guard is pinned (mutant M5 below). It is written as an escape in the file, which adds no character above U+007F.
  • The trailing-blank manifest end to end is a row of the measured table below.

Measured first

At origin/main 50ce8ab15, with check and apply run through the real tool on the Atlas project the partial-rollback tests scaffold, each run with a local --upstream under a stub gh (exit 97), no network.

Input check apply at main apply here
pin: the only files: line has U+00A0 or U+3000 in front update-pin-no-files, exit 2 the same, tree untouched the same (unchanged)
manifest: the only shape: line has U+00A0 or U+3000 in front exit 1, NEXT apply (as for a manifest with no shape key at all) update-validators-red ("after the rewrite, scripts/validate-manifest.py refused"), rolled back update-manifest-no-shape ("project.yaml has no shape: block"), rolled back: what a manifest with no shape: line gets
pin: <blank>files: above the real files: both validators pass; exit 1, NEXT apply update-validators-red (shape-pin-no-files): the rows went under the stray line and break dropped the real header; rolled back exit 0, the stray line kept as it was, pin at B
manifest: <blank>shape: above the real shape: both validators pass; exit 1, NEXT apply update-manifest-unrecognised, "holds the shape: key twice, at lines 80 and 81", which is false of a file the reader reads as two keys; rolled back exit 0, the stray line kept, manifest and pin at B
manifest: the only shape: line has U+00A0 after the colon (shape: <U+00A0>) exit 1, NEXT apply, though both validators refuse the file (yaml-unparsable, "line 81: unexpected indentation": the reader gives shape the one blank as its value, and the block under it is indented under a scalar) update-validators-red, the validators naming "line 81: unexpected indentation"; rolled back update-manifest-no-shape ("project.yaml has no shape: block"), rolled back

So the answer to "do check and apply reach the rewrite on such a file at all" is: for a pin with only the blanked line, no, the reader's own update-pin-no-files stops both commands first, and the header finder is reached only by a direct call (RV1188's _repin case). For the other four inputs they do. The unrecognised-header refusal the issue wants is the manifest's update-manifest-no-shape, now given; a pin that holds the blanked line beside the real files: is now rewritten as the reader reads it, where it was written wrong and rolled back.

Design

_is_block_header asks the reader's question: _split_key(_strip_comment(line.strip(_YAML_WHITE))) == (key, ""). A trailing U+00A0 or U+3000 is covered by the same call (shape: <blank> is a key with a value to the reader, and with the blank right after the colon the line has no key at all), so the unit rows pin both ends. The lstrip change is the Direction's and keeps the function in one vocabulary; no row can tell it from the bare lstrip() (see the mutants).

The two other bare .strip() calls: both left

Read against the reader, and measured, with the same probe.

  • rewrite_manifest's line.strip() before startswith("commit:") and startswith("tree_sha256:") (inside the shape: block). The same family: a stray <blank>commit: inside the block beside the real commit: is counted, so apply refuses update-manifest-unrecognised, "has 2 commit line(s)", rolled back, for a manifest the reader and both validators accept (two keys). It is a false refusal by name with the tree untouched, not a wrong write. Left because the issue lists it as not asked here. A follow-up would give it strip(_YAML_WHITE) and rows of its own; a manifest holding only the stray line, with no real commit:, was not measured.
  • _shape_block_bounds' end test, line.strip() and not line.startswith((" ", "\t")). A column-0 line of only U+00A0 or U+3000 reads blank to the bare strip(), so the block runs past it. The reader refuses the whole file for such a line (expected key: value, naming the line's text), so the run ends update-validators-red, rolled back, with or without the change, and the only thing the end test decides is the scope of a rewrite no validator will accept. Left: changing it changes no outcome a person can see.

_header_as_it_was's tail.strip() is the same call a third time, and unaffected: it only ever sees a real header, so its tail is a comment or a run of spaces and tabs.

Out of scope

Nothing else in update-shape.py. with_scalar in scripts/shape_materialize.py also measures a line's indentation with a bare lstrip(); the one call that can hand it a blanked line is rewrite_manifest's, left above, so a follow-up starts there.

Tests

Each run is one pytest at a time under a stub gh (exit 97), GH_TOKEN=bogus, an empty GH_CONFIG_DIR, GIT_SSH_COMMAND=false, GIT_CONFIG_GLOBAL=/dev/null and EMAIL=tests@openreposhape.invalid: /usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch> -o tmp_path_retention_policy=failed tests/<file>.

New rows: 27 (the 5 header rows, the 8 manifest damage rows, the 2 pin and 4 manifest carry-over rows, and 8 end-to-end runs). The selector I ran at main takes 26 of them and the two old rows of test_a_files_header_with_trailing_blanks_only_is_written_bare, so it selects 28: on origin/main's update-shape.py 24 of the 28 fail and 4 pass, the two old rows and the two update-pin-no-files end-to-end runs, which hold what check and apply do before the rewrite is reached and so cannot fail before. With this change all 28 pass. The 27th row, tab-in-front, passes at main as well as here: main already holds the tab half of the guard, so this row is no fix row but the pin of that half, which mutant M5 below drops.

File Result
tests/test_update_shape_apply_lines_and_headers.py with tests/test_update_shape_partial_rollback.py (its fixtures) 83 passed in 69.49s
tests/test_update_shape_header_comments.py 62 passed in 28.24s
tests/test_path_guard_hygiene.py 121 passed in 22.58s
tests/test_repo_hygiene.py 101 passed in 2.92s
tests/test_no_auto_maintenance.py 58 passed, 2 skipped in 15.03s

Run again after the fix round, at this head. Run at the first head and not again, because the fix round leaves update-shape.py byte for byte as it was and changes no test they run: tests/test_update_shape.py (90 passed in 193.71s), tests/test_pin_rewrite_layout.py (115 passed), tests/test_yaml_reader_spaces.py (513 passed), tests/test_bom_rewrites.py (12 passed), tests/test_bom_tool_io.py (126 passed), tests/test_agents_md_codes.py (47 passed) and tests/test_cli_reference.py (23 passed).

tests/test_shape_doctor.py was not run: this change does not touch shape-doctor.py or scripts/repo_shape.py. No new bare is_file, is_dir, exists, is_symlink or os.path guard, and no new refusal code or message.

Mutants

Each is a change to update-shape.py, the new rows run (M5 over both test files), then the file is put back and checked with cmp against the committed bytes (cmp ok each time).

Mutant Result
M1: line.strip(_YAML_WHITE) back to the bare line.strip() KILLED: 24 failed, 4 passed. The 24 are every leading-blank and trailing-blank row, the carry-over rows and the end-to-end runs for the manifest and for a stray key
M3: only the front is stripped with _YAML_WHITE, the back is the bare rstrip() KILLED: 6 failed (the four trailing-blank damage rows and the two trailing-blank files: rows)
M5: the tab half of the column guard dropped from _is_block_header (the or <a tab> in line[:blanks] test) KILLED: 1 failed, 122 passed over both test files: test_a_line_that_is_no_files_header_to_the_reader_is_still_counted_none[tab-in-front] fails and nothing else does. M5 survived every test before this row
M4: only the back is stripped with _YAML_WHITE, the front is the bare lstrip() KILLED: 18 failed (every row with the blank in front)
M2: line.lstrip(_YAML_WHITE) back to the bare line.lstrip() SURVIVES, and is an equivalent mutant. That lstrip only measures the blanks in front for the tab test, and a line it would treat differently starts with U+00A0 or U+3000, where the strip(_YAML_WHITE) that follows already makes it no header. No row can tell them apart; it is kept because the issue's Direction names it and the function then speaks one vocabulary

Merges

git merge-tree --write-tree of this head (aca00dfd9) with origin/main (0ed3e57f8) and with each open PR that touches update-shape.py or the two test files, listed live with gh pr list before the push (48 open; #1231 itself is the only one that touches the test files): PR 1166 (5102b4294) and PR 1133 (a7bfee1bd). All three are clean, exit 0, with no conflict lines.

Closes #1220

Lane: openreposhape-3 (openRepoShape-3)

🤖 Generated with Claude Code

… `files:` is no block header, as it is none to the reader, so `apply` no longer writes the pin's rows under it or refuses a manifest for a `shape:` key written "twice" (#1220)

`_is_block_header` cut each line with `str.strip()` and `str.lstrip()` and no
argument, which also cut U+00A0 and U+3000. The reader strips `_YAML_WHITE`,
a space and a tab, so to it `<U+00A0>shape:` is the key `<U+00A0>shape`, a
key of its own, and `parse_yaml` gives the file both. The finder now strips
`_YAML_WHITE`, imported from `repo_shape` beside `_split_key` and
`_strip_comment`, and its docstring says so.

Measured at origin/main with `check` and `apply` over such a file, each run
with a local `--upstream`: a manifest whose only `shape:` line is blanked was
rewritten under that line and refused `update-validators-red` after the
write, where it is now refused `update-manifest-no-shape` by the rewrite's
own count; a blanked key above the real header was taken for the header, so
a pin was written wrong and rolled back and a manifest was refused for a
`shape:` key "twice", and both are now rewritten. A pin whose only `files:`
line is blanked is refused `update-pin-no-files` by the reader before the
rewrite is reached, and still is.

The two other bare `.strip()` calls the issue names are left: see the pull
request.

Co-Authored-By: Claude Sonnet 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 makes update-shape.py determine files: and shape: headers using the same space-and-tab whitespace rules as the YAML reader, preventing U+00A0 and U+3000 lines from being misidentified and rewritten. Expanded tests cover header parsing, duplicate-looking keys, refusal paths, rollback, validators, and end-to-end check/apply behavior.

Sequence diagram for whitespace-aware shape header detection

sequenceDiagram
    participant Tool as update-shape.py
    participant Reader as repo_shape
    participant Validator as Validators

    Tool->>Reader: line.lstrip(_YAML_WHITE)
    Tool->>Reader: _strip_comment(line.strip(_YAML_WHITE))
    Tool->>Reader: _split_key(...)
    alt key matches shape or files
        Tool->>Validator: rewrite using recognized block header
    else U+00A0 or U+3000 precedes header
        Tool->>Validator: refuse as missing or unrecognized header
    end
Loading

Flow diagram for reader-consistent header recognition

flowchart TD
    A[Input line] --> B[Trim only space and tab with _YAML_WHITE]
    B --> C[_strip_comment]
    C --> D[_split_key]
    D --> E{Matches shape: or files:?}
    E -->|Yes| F[Recognize block header]
    E -->|No| G[Do not rewrite as a block header]
Loading

File-Level Changes

Change Details Files
Align block-header detection with the YAML reader’s restricted whitespace vocabulary so non-breaking and ideographic spaces are treated as key text rather than header indentation or trimming.
  • Import _YAML_WHITE and use it for both leading-space measurement and surrounding whitespace removal in _is_block_header.
  • Update _is_block_header documentation and import rationale to describe the reader-compatible behavior.
  • Preserve existing refusal and rewrite behavior for malformed or missing headers while preventing writes under keys the reader does not recognize.
update-shape.py
Add unit, integration, and end-to-end coverage for Unicode whitespace around files: and shape: keys.
  • Cover U+00A0 and U+3000 before and after headers, including stray keys alongside valid headers.
  • Verify valid headers are rewritten correctly while stray Unicode-prefixed keys are carried over unchanged.
  • Verify manifests and pins with only a Unicode-prefixed header are refused without modifying the tree.
  • Run validators and real check/apply flows to confirm consistent behavior and rollback guarantees.
tests/test_update_shape_apply_lines_and_headers.py
tests/test_update_shape_header_comments.py

Assessment against linked issues

Issue Objective Addressed Explanation
#1220 Make _is_block_header use only the YAML reader’s space-and-tab whitespace rules, so U+00A0 or U+3000 before shape: or files: is not recognized as a block header; update the docstring accordingly. ✅
#1220 Verify both manifest and pin rewrites refuse or handle such lines consistently with the reader, including end-to-end behavior and preserving existing header cases. ✅

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

@brettheap

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-3, 2026-10-23T21:34:45Z) RV1231 independent review of #1231 (#1220) at 28deb6e4972b2004c965204b6cb3464f4ef9c1af: NOT READY (one fix round).

The fix is right, with no regression from main: _is_block_header now answers exactly as the reader's _tokenise does. A differential over 522,682 lines found 0 differences between the head and the reader, and 39,482 between main and the reader. The alphabet was a space, a tab, U+00A0, U+3000, U+001F, U+2003, #, x and " around 7 spellings of the key, plus all 19 inline str.isspace characters.

MUST

  • tests/test_update_shape_apply_lines_and_headers.py:298, :377, :414 and :440: SonarCloud python:S9073 (4 OPEN, MAJOR), a composite assert a and b. Split each into two asserts, each keeping its "fixture: ..." message.

NIT (same round)

  • tests/test_update_shape_apply_lines_and_headers.py:266 says main's result "did not read back". It did: rewrite_fault returned None, because it reads only the commit and the tree. The result lost its files key, and validate-pins refused it after the write. Say that.
  • update-shape.py:1706: no row covers the tab half of the guard. Mutant M5, that half dropped, survives 249 tests: the two PR files, test_pin_rewrite_layout.py and test_bom_rewrites.py. Add a tab-in-front param (a tab, then files:) at :220, written as an escape, not a raw tab.
  • Body: add the trailing-blank manifest end to end (shape: <U+00A0>).
    • At main, apply ended update-validators-red, the validators naming "line 81: unexpected indentation".
    • At the head it ends update-manifest-no-shape, rolled back.
  • Body: a Sourcery "Summary" block follows the robot line.

Asked rulings

  • M2 (bare lstrip back): an equivalent mutant, with 0 differences in the differential. So is dropping the strip(_YAML_WHITE) entirely: the guard leaves no leading space or tab, and _strip_comment ends in rstrip(_YAML_WHITE). Nothing to change.
  • rewrite_manifest's line.strip() (update-shape.py:1863): a follow-up issue. Take a stray <U+00A0>commit: beside the real one. The reader and both validators accept it, and check says NEXT apply. apply then refuses update-manifest-unrecognised ("2 commit line(s)") and rolls back, at main and at the head.
  • with_scalar (scripts/shape_materialize.py:3081 indentation, :3086 spacing): fold into that follow-up. A commit: <U+00A0>"<sha>" inside shape: is refused with "the fault is this tool's line rewrite, not the project's manifest", for a manifest both validators had already refused, at main and at the head.
  • _shape_block_bounds' end test (update-shape.py:1971): nothing. The reader refuses every column-0 line made only of such blanks, and apply ends update-validators-red, rolled back.
  • Older than this PR, a separate follow-up candidate under Tracking: update-shape.py and bump-leg.py, keeping a project's shape copies and leg pins current #458: rewrite_shape_pin's break (update-shape.py:1802) drops every line after the files: header. A plain extra: 1 below the rows, which the reader and both validators accept, is deleted by apply with exit 0, at main and at the head.

Measured with RV1231's own probe: check and apply through the real tool, a local --upstream and a stub gh, at main 50ce8ab15 and at the head. Every claim in the body holds. A stray blanked shape: BELOW the block is fixed too: at main "twice, at lines 80 and 180", at the head exit 0 with the stray line kept.

Tests at the head, one at a time, all green:

  • apply_lines together with partial_rollback: 82 passed;
  • header_comments: 62 passed;
  • pin_rewrite_layout: 115 passed;
  • bom_rewrites: 12 passed;
  • path_guard_hygiene: 121 passed;
  • repo_hygiene: 101 passed;
  • no_auto_maintenance: 58 passed, 2 skipped.

Mutants on the 26 new rows, each restored and checked with cmp:

  • main's whole file: 24 failed, 2 passed;
  • M1: KILLED (24 failed);
  • M3: KILLED (6 failed);
  • M4: KILLED (18 failed);
  • M2: survives, equivalent (above).

Merges: merge-tree with origin/main e04826db, #1166 5102b429 and #1133 a7bfee1b: all clean.

Sonar and CI:

  • Sonar: gate OK, duplication 0.0%, 0 hotspots, 4 new S9073 issues (the MUST above).
  • CI at 21:32:47Z: tests still pending (queued); tests-git-bash, package and SonarCloud pass.

Body and commit:

  • The body is a draft on v1.2.1, with the follow-up sentence first. Closes #1220 is its only closing reference, and nothing above U+007F appears but the robot.
  • The commit is by Brett Heap <service@opensoft.dev>, with a Co-Authored-By line.

Lane: openreposhape-3 (openRepoShape-3)

…k is one assert, the docstring says the old rewrite lost the pin's `files` key, and a tab in front of `files:` is a row that pins the tab half of the column guard (#1220)

The four composite `assert a and b` fixture checks are two asserts each, each
keeping its "fixture: ..." message (SonarCloud python:S9073).

The docstring said main's result "did not read back". It did: `rewrite_fault`
reads only the commit and the tree, so it returned None, and the result lost
its `files` key, which validate-pins refused after the write.

A `tab-in-front` row (a tab, then `files:`) is the tab half of
`_is_block_header`'s column guard. Dropping that half survived every test
before; it is killed by this row.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@brettheap

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-3, 2026-10-23T21:52:12Z) RV1231b independent delta review of #1231's fix round (28deb6e4..aca00dfd): READY. CI tests was still in progress at 21:51Z, so #1231 lands once that is green, at its slot at the end of wave 2.

Every MUST and NIT of RV1231 is answered:

  • The asserts: the four composite asserts are two asserts each, each keeping its "fixture: ..." message. SonarCloud: all four python:S9073 issues are CLOSED, the gate is OK, new duplication is 0.0% and hotspots are 100% reviewed.
  • The docstring at :266-270 is true: rewrite_fault (scripts/shape_materialize.py:3112) checks only the commit and the tree.
  • The tab-in-front row (:222) is written as an escape, and the file has no raw tab. Mutant M5 (the guard reduced to if line.startswith(" "):), re-made with RV1231b's own script, is KILLED: 1 failed, 82 passed, only [tab-in-front] ("DID NOT RAISE Refusal"). The file was restored, cmp ok.
  • The body has the trailing-blank manifest row and ends at the robot line.

update-shape.py: git diff 28deb6e4 aca00dfd -- update-shape.py is empty.

Tests at aca00dfd, one at a time under a stub gh:

  • apply_lines with partial_rollback: 83 passed;
  • header_comments: 62 passed;
  • repo_hygiene: 101 passed;
  • path_guard_hygiene: 121 passed;
  • no_auto_maintenance: 58 passed, 2 skipped.

Probe, RV1231b's own, of shape: <U+00A0>:

  • At main and here, both validators refuse yaml-unparsable "line 81: unexpected indentation", and check exits 1 with NEXT apply.
  • apply ends update-validators-red at main 50ce8ab15 and update-manifest-no-shape here, rolled back both times, with the tree unchanged. That is the body's row.

Merges: the open PRs were listed live (51). Only #1166 (5102b429) and #1133 (a7bfee1b) touch the PR's files. merge-tree with origin/main 0ed3e57f8 and with each of them is clean.

CI on aca00dfd: SonarCloud, package and tests-git-bash pass; tests is in progress.

Body and commit:

  • The body is a draft on v1.2.1, with the follow-up sentence first. Closes #1220 is its only closing reference, nothing above U+007F appears but the robot, and no bot block follows it.
  • The commit is by Brett Heap <service@opensoft.dev>, with a Co-Authored-By line.

NIT (body only, no push): "written as an escape in the file, which stays ASCII". The row is ASCII, but the file holds two U+2014 from main at :792. Say "which adds no character above U+007F".

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