Skip to content

update-shape.py: update-accept-local-missing-name asks a copy an earlier apply kept, read unchanged, as a module a written copy imports from and as an importer of a written one, so a plain apply that stops either on its import is refused (#1082) - #1133

Merged
brettheap merged 5 commits into
mainfrom
fix/1082-guard-earlier-kept
Nov 8, 2026

Conversation

@brettheap

@brettheap brettheap commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Goal

#1082: update-shape.py apply refuses, before any write, every run that would leave a copy importing a name from a copy that does not define it, wherever the kept copy came from: this run's --accept-local, or an earlier run's.

Fix round 1 (53791e45, on a merge of main 038dafba) answers rv1133's review: its SHOULD 1 (the known limit was inside this Goal) is fixed here rather than filed, and its SHOULD 2 and NIT are taken.

What was measured on main, before the change

All probes are offline. gh is a stub that exits 97, GIT_SSH_COMMAND=false, GIT_CONFIG_GLOBAL=/dev/null, and git is 2.43.0. The tool is main's update-shape.py (sha256 ca47f90e, blob a7ae7505, the same at 2a119f28, 6da757cb, 4ed7c7bf and 038dafba). Scripts and logs are in lane 5's scratch v118/w1082/ (tools/probe1023.sh, tools/probe1075.sh, probe-*) and, for fix round 1, v120/f1133/ (tools/knownlimit.sh, tools/probe1075.sh, limit-*, probe-*1075).

#1075's untraced, on main's own history. #1075 has now landed, so main itself holds the case.

#1023's member_make_environment. A holder is cut at f1946d04, the parent of #1023's 9e154d87. Its bootstrap.py has no member_make_environment, and its siblings.py does not name it. Its own edit to bootstrap.py is committed.

The reverse direction: a copy an earlier run kept, as the importer (rv1133's SHOULD 1). rv1133 measured it with main's tool and with aa7ddf59's; fix round 1 measured it again with main's (limit-m/).

  • A family holder is cut at 4ed7c7bf. Its own edit to siblings.py adds from bootstrap import told_past, which the upstream's siblings.py does not import, and is committed. siblings.py --help still exits 0.
  • apply --at U1 --accept-local scripts/siblings.py --branch shape/first exits 0. U1 is main plus an unrelated change to validate-family.py.
  • U2 then renames told_past in bootstrap.py alone. check --at U2 reads upstream-changed scripts/bootstrap.py and unchanged scripts/siblings.py.
  • apply --at U2 --branch shape/second, with no --accept-local, exits 0, prints copied scripts/bootstrap.py, and commits. siblings.py --help then exits 1 with ImportError: cannot import name 'told_past' from 'bootstrap'.
  • This run's own write breaks a copy that worked before it, so it is the Goal's case. The one-step run, --accept-local scripts/siblings.py, was already refused by update-shape.py apply --accept-local: a kept .py copy that imports a name the run's new repo_shape.py dropped passes, and make bootstrap then dies on an ImportError #604 with scripts/bootstrap.py (written) no longer defines told_past, which scripts/siblings.py (kept) imports from it.

What changed

update-shape.py, _refuse_kept_copies_missing_names (#536, #604):

  • Each written .py copy is asked against its module as this run leaves it, whoever kept it. The reading is _copies_as_left, _written_copies_lacking and _names_as_left. The module may be:
    • kept by this run's --accept-local: as before, the line says (kept);
    • an unchanged row whose pin records the project's own bytes, which is a copy an earlier run kept that the upstream has not changed since. The line says (kept by an earlier run);
    • the upstream's own bytes at the target: written by this run (an upstream-changed row or an --add file), already-at-target, or unchanged with the target's bytes. The line says (the upstream's). No shipped copy meets this today: the reader rows of tests/test_update_shape_kept_helper.py find every name each shipped importer takes defined beside it. So it refuses only a target whose own copies disagree.
  • A copy an earlier run kept is asked as an importer too (fix round 1). update-shape.py apply --accept-local: a kept .py copy that imports a name the run's new repo_shape.py dropped passes, and make bootstrap then dies on an ImportError #604 asked only a copy this run's --accept-local keeps against a module this run writes (_kept_copies_importing_dropped). The rows _copies_as_left reads as kept by an earlier run are now passed beside the kept ones, under a new side, WRITTEN_UNDER_EARLIER.
    • Its line says who kept the importer: scripts/bootstrap.py (written) no longer defines told_past, which scripts/siblings.py (kept by an earlier run) imports from it. KEPT_IMPORTERS maps each (written) side to its importer's word, so a (kept) importer's line is unchanged.
    • _names_dropped still limits the refusal to names this run takes away: a name the module's bytes on disk define and its written bytes do not. A copy an earlier run kept whose names all stay, or that takes a name the module lacked before too, still applies.
    • An unchanged copy holding the upstream's own bytes is still not asked as an importer: that pair is two of the target's own copies.
  • The early if not kept: return is gone. A run with no --accept-local is asked too. The guard still stands after update-conflict, update-local-drift and update-rewrite-symlink, and before confirm and any write.
  • The refusal's code is kept, and so are its words where every line is one this run's --accept-local makes.
  • The remediation gives each kind its exit.
    • For a copy an earlier run kept, it says why check reads it unchanged, then gives two exits. As a module that lacks a name: merge the upstream's version by hand so it defines every name its line names. As an importer of a name the written module drops (fix round 1): change it so it no longer takes any name its (written) no longer defines line names. Then commit, and run again with --accept-local on each copy an earlier run kept that you changed, whose row then reads locally-modified. Or write the upstream's bytes over it (the remedy's own git show line names that copy), commit, and run again. All four exits are tested end to end, and each checks the state check then reads: locally-modified after the first, already-at-target after the second.
    • For two of the upstream's own copies, it says run this again with an --at whose copies agree.
  • The known limit is gone. The docstring paragraph that named it, and the row that pinned it, are replaced by the rows below that hold the refusal. KEPT_LEAD, the code and every (kept) line are as main has them.

tests/test_update_shape_kept_helper.py:

Fix round 1, after rv1133's review

  • Main merged. git merge origin/main (038dafba) was clean: merge commit 7aee5efa, tree 27a2785e, the same as git merge-tree --write-tree aa7ddf59 038dafba. The fix is one new commit on it, 53791e45. Nothing was amended, rebased or forced.
  • SHOULD 1 (the known limit). Fixed as above, rather than filed. New rows:
    • test_an_earlier_kept_copy_is_asked_when_the_next_apply_drops_its_name: end to end, check reads the kept bootstrap.py unchanged and repo_shape.py upstream-changed; the plain apply to dropped is refused with nothing written, one detail line, the general opening, and that copy's exits, and none of the other kinds' exits. Its premise is measured: the kept bootstrap.py beside dropped's repo_shape.py stops on the import.
    • test_an_earlier_kept_copy_importing_what_this_run_keeps_applies (2 controls): a name repo_shape.py lacked before too, and one the written repo_shape.py still defines. Both apply with no --accept-local, and the kept copy starts after the run exactly where it started before.
    • test_either_exit_for_an_earlier_kept_importer_applies (2): each exit the remedy gives that copy, with the state check reads after it; the apply then writes repo_shape.py, and bootstrap.py starts.
    • test_an_importer_an_earlier_run_kept_is_asked_against_a_written_module (2, in process): the line, the opening, the exits, and the again wording alone and beside a copy this run keeps.
    • test_an_importer_holding_the_targets_bytes_is_not_asked replaces test_an_importer_this_run_leaves_alone_is_not_asked, whose second param pinned the limit.
  • The scenario itself, with the head's tool (limit-h/): the second apply exits 2 with scripts/bootstrap.py (written) no longer defines told_past, which scripts/siblings.py (kept by an earlier run) imports from it, the holder is unchanged, and siblings.py --help exits 0. The one-step, first-apply and check logs match main's tool once paths and shas are masked. the family root's siblings.py: with a token in use, the clone and the fetch that carry it run in bootstrap.py's untraced() environment, so make siblings and family.py siblings write the token to no trace the caller asks git for (#1055) #1075's probe (probe-h1075/) is refused at its second apply too, with (kept by an earlier run) lacks untraced.
  • SHOULD 2. test_keeping_a_copy_that_imports_a_name_the_run_drops_is_refused, a update-shape.py apply --accept-local: a kept .py copy that imports a name the run's new repo_shape.py dropped passes, and make bootstrap then dies on an ImportError #604 end-to-end (written) row, now asserts startswith(KEPT_LEAD). rv1133's R2 is killed (below).
  • NIT. The exits rows (test_either_exit_the_remedy_names_applies and the new one) check that check reads locally-modified after the change exit and already-at-target after the drop exit. The first exit's id now says the edit was changed by hand, as the remedy's changed does.

Rows that fail on main

The new test file (185 rows) was run against main's update-shape.py (038dafba, sha256 ca47f90e, in a scratch clone): 17 failed, 168 passed (base-main.log). Against aa7ddf59's: 13 failed, 172 passed (base-aa7.log). With the change: 185 passed.

row on main on aa7ddf59
test_an_earlier_kept_copy_is_asked_when_the_next_apply_writes_its_importer assert 0 == 2: the second apply copies siblings.py fails: the old opening
test_with_branch_an_earlier_kept_copy_is_refused_before_any_branch assert 0 == 2 passes
test_either_exit_the_remedy_names_applies (2 params) assert 0 == 2: the apply the remedy follows is not refused passes
test_a_written_copy_is_asked_against_its_module_as_this_run_leaves_it (4 params) no refusal fails: the old opening
test_each_kind_of_module_is_given_its_own_exit (2 params) no refusal 1 fails: its remedy says merged, not changed
test_every_kind_of_line_in_one_run_is_one_refusal (2 params) only the (kept) line fails: the old opening
new: test_an_earlier_kept_copy_is_asked_when_the_next_apply_drops_its_name assert 0 == 2: the plain apply copies repo_shape.py the same
new: test_either_exit_for_an_earlier_kept_importer_applies (2 params) assert 0 == 2: the apply the remedy follows is not refused the same
new: test_an_importer_an_earlier_run_kept_is_asked_against_a_written_module (2 params) no refusal; beside a kept copy, only its (kept) line the same

These pass on main and after: the one-step row (test_the_one_step_run_is_refused_as_it_was, which pins the old opening), the #604 row that now pins KEPT_LEAD, the controls (test_an_earlier_kept_copy_that_defines_every_name_applies, the two new earlier-kept importer controls, the four module-defines-every-name controls), and test_an_importer_holding_the_targets_bytes_is_not_asked.

Mutants

Each was applied alone in a scratch clone, run on the test file, and restored by checkout with its sha256 checked.

Fix round 1, at 53791e45 (185 rows):

mutant result
N1, the earlier-kept rows not passed to _kept_copies_importing_dropped ({}), as aa7ddf59 had it killed, 5 failed: the 5 new rows that fail on main
N2, an earlier-kept importer called (kept) in its line killed, 3 failed: the end-to-end refusal row and both in-process rows
N3, an earlier-kept importer given the (written)-under-(kept) side killed, 3 failed: the same three
N4, the KEPT_LEAD opening kept for a (written) line under a copy an earlier run kept killed, 3 failed: the same three
N5, the change exit for an earlier-kept importer dropped killed, 3 failed: the same three
N6, the unchanged why and the --accept-local on each copy an earlier run kept exit given to an earlier-kept module only killed, 3 failed: the same three
N7, the module, not the earlier-kept importer, named for git show killed, 3 failed: the same three
N8, the "what the upstream's own copy of that file imports instead" clause for a (written)-under-(kept) line only killed, 1 failed: the end-to-end refusal row
N9, every unchanged row asked as an importer, the target's own bytes too killed, 1 failed: test_an_importer_holding_the_targets_bytes_is_not_asked
N10, the old general opening (a copy this run writes or keeps imports ...) killed, 10 failed
rv1133's R2, a (written)-only refusal given the general opening (sides <= {KEPT_MODULE}) killed, 1 failed: test_keeping_a_copy_that_imports_a_name_the_run_drops_is_refused (SHOULD 2); it survived at aa7ddf59
the first round's M1, the old kept set restored killed, 17 failed: the 17 rows that fail on main
the first round's M3, the guard called after _write_copies killed, 15 failed: every end-to-end refusal row, the 3 new ones among them
the first round's M7, earlier-kept and upstream's own swapped killed, 12 failed

No survivor. Every mutant was restored by checkout with its sha256 checked (5b386f8b), and each clone's status was clean at the end (mutants1.out, mutants2.out, mut-*.log).

The first round, at 7b447602 (179 rows):

mutant result
M1, the old kept set restored (left = dict.fromkeys(kept, KEPT_MODULE)) killed, 12 failed: the 12 rows that fail on main
M2, unchanged rows asked as importers (against the modules this run writes) but the written importers asked against the old kept set only killed, 14 failed
M3, the guard called after _write_copies (refused after a write) killed, 12 failed: every end-to-end refusal row sees copied
M4, a copy an earlier run kept called (kept) killed, 5 failed
M5, the upstream's own modules not asked killed, 5 failed
M6, the new opening always used killed, 1 failed: the one-step row's old opening
M7, earlier-kept and upstream's own swapped killed, 6 failed
M8, the --accept-local on each copy an earlier run kept exit dropped killed, 3 failed
M9, a written module read from its bytes on disk killed, 4 failed
M10, the unchanged sentence dropped killed, 2 failed

Runs

All runs went through lane 5's run9.sh, with the stub gh, GIT_SSH_COMMAND=false, GIT_CONFIG_GLOBAL=/dev/null and EMAIL exported. No more than two ran at once. The full suite was not run.

Fix round 1, at 53791e45 (each START line records the head and dirty=0; logs in v120/f1133/):

run git result log
the new file, tests/test_update_shape_kept_helper.py 2.43.0 185 passed in 100.27s own243.log
the same 2.47.1 185 passed in 95.67s own247.log
tests/test_update_shape.py, tests/test_update_shape_messages.py, tests/test_update_shape_paste_lines.py 2.43.0 144 passed in 181.71s runC.log
tests/test_update_shape_apply_lines_and_headers.py, tests/test_update_shape_merged_away.py, tests/test_update_shape_rewrite_symlink.py, tests/test_update_shape_landing.py 2.43.0 163 passed in 223.09s runD.log
tests/test_update_shape_add_not_a_directory.py 2.43.0 13 passed in 33.11s runF.log
tests/test_path_guard_hygiene.py and the trio (tests/test_repo_hygiene.py, tests/test_agents_md_codes.py, tests/test_cli_reference.py) 2.43.0 219 passed in 54.58s runE.log

No failure and no skip in any run at this head. The two runs of the new file went through run9.sh on 2026-10-22 at 13:31Z, the next three on 2026-10-22 from 15:16Z to 15:21Z, and runF.log at 15:23Z; each START line reads head=53791e45... and dirty=0. Of the 19 tests/test_update_shape*.py files, these 9 ran at this head. The other 10 did not run at this head: all 10 ran at the first round's head 7b447602 in shard A or B (below), over the first round's update-shape.py, which this round's commit changed. The full suite was not run.

The first round, at 7b447602 (tree e0e2d19e, the same tree as aa7ddf59): the new file 179 passed on git 2.43.0 and on git 2.47.1; shard A (29 files) 2031 passed, 5 skipped; shard B (29 files) 1814 passed, 5 skipped; the trio 170 passed. No failure in any run.

Merges

Read at 53791e45, after a re-fetch of main (origin/main is still 038dafba, the second parent of the branch's merge commit 7aee5efa):

  • git merge-tree --write-tree origin/main HEAD exits 0 and writes tree bfa8c1d4, which is HEAD's own tree: the branch holds 038dafba as an ancestor, so the merge adds nothing.
  • The merge commit 7aee5efa (parents aa7ddf59 and 038dafba) has tree 27a2785e, the same tree as git merge-tree --write-tree aa7ddf59 038dafba, which exits 0. It was clean.
  • Open pull requests: 37 were listed with gh pr list --state open --limit 100 --json number,headRefName,headRefOid,files. None but this one touches update-shape.py or tests/test_update_shape_kept_helper.py, so there is no other head to merge against. The list is open-prs-d1.json in v120/f1133/.
  • git diff --name-only origin/main HEAD names those two files and no other.

NIT round, after rc1133's re-check

rc1133 re-checked 53791e45 (#1133 issuecomment-6347273216): READY, with three NITs. This round takes all three, in one new commit, a7bfee1b, on 53791e45. It was pushed as a fast-forward of the branch; nothing was amended, rebased or forced. The commit changes tests/test_update_shape_kept_helper.py alone (+7, -4). update-shape.py is 53791e45's, sha256 5b386f8b, so the fix and every measurement in the sections above are unchanged.

  • NIT 1 (taken): a refusal in both directions at once now has its opening pinned. _missing_name_refusal opens with KEPT_LEAD where every line is one this run's --accept-local makes. A run with a (kept) lacks line and a (written) no longer defines line is such a run, but test_both_directions_in_one_run_are_one_refusal pinned its lines, its clause and its remedy, not its opening. rc1133's mutant O1 (lead = (KEPT_LEAD if len(sides) == 1 and sides <= {KEPT_MODULE, WRITTEN_MODULE} else LEFT_LEAD)) therefore survived the 185 rows. The row now asserts result.stderr.startswith(KEPT_LEAD), result.stderr, right after assert_refused_writing_nothing, and its docstring says why the opening is that one.
  • NIT 2 (taken): the Merges section lacked validate-manifest.py, validate-family.py, family.py and update-shape.py: an id:, a date or a repository: ending in a line feed is refused by name, and a pin's source_repository: that is no path or owner/repo name is named as the pin's (#1139) #1166. Its row is in "Merges at a7bfee1b" below, and it replaces the Merges section's "Open pull requests" line.
  • NIT 3 (taken): the title named only the first round's direction, a written copy asked against its module. It now names the copy an earlier apply kept in both roles: as the module a written copy imports from, and as an importer of a written module. It is 249 characters and ASCII. rc1133's 242-character rewrite was close, but its "a plain apply that would break it" is true only where the kept copy is the importer: where it is the module, the apply breaks the written importer. The new title says "either".

Mutant O1, before and after

Applied alone in this worktree and restored by checkout, sha256 5b386f8b checked, status clean afterwards. The mutant is rc1133's (mutate_rc.py, copied unchanged).

test file run result
53791e45's, without the new assert the row alone, with O1 applied survives: 1 passed, 184 deselected (o1-before-row.log). rc1133 measured the whole file: 185 passed
a7bfee1b's, with the new assert the whole file, with O1 applied killed: 1 failed, 184 passed (o1-after-file.log). The failure is test_both_directions_in_one_run_are_one_refusal at the new assert: the opening read a copy imports a name from a copy beside it whose bytes, as this run leaves them, do not define it, ...

No other mutant was needed: the round adds no behaviour and no new row, and the earlier rounds' 14 + 10 mutants stand at an unchanged update-shape.py.

Rows that fail on main

None, and that is measured, not assumed. The new assert pins an opening the head keeps exactly as main prints it, so it cannot fail on main. rc1133 had that only by reading, so it is measured here: the row, new assert included, run with main's update-shape.py (origin/main 961640c6, sha256 ca47f90e) swapped into the worktree, gives 1 passed (main-row.log); the file was restored to 5b386f8b afterwards. The assert guards that opening against a later regression like O1, as SHOULD 2's assert guards the one-step opening against R2.

Runs at a7bfee1b

All through lane 5's run9.sh, each START line at head=a7bfee1b... and dirty=0, with the stub gh and EMAIL exported; two ran at once, and the full suite was not run. No failure and no skip. Logs are in lane 5's scratch v124/n1133/.

run git result log
tests/test_update_shape_kept_helper.py and tests/test_path_guard_hygiene.py 2.43.0 234 passed (185 + 49) in 61.57s own243.log
tests/test_update_shape_kept_helper.py 2.47.1 (first on PATH, printed in the log) 185 passed in 48.73s own247.log

The commit changes no function, no path guard and no refusal text, so no other test file names anything it changed. The test file still has no assert that joins conditions with and or or (206 asserts, checked on the syntax tree), no non-ASCII byte, and its widest line is 79 characters.

Merges at a7bfee1b

Read after a re-fetch of main: origin/main is 961640c6 (#1155, which adds docs/release-notes/v1.2.0.md and nothing else). It touches neither of this PR's files.

Closes #1082

Claim: #1082 (comment)
Lane: openreposhape-5 (openRepoShape-5)

🤖 Generated with Claude Code

Summary by Sourcery

Prevent update-shape applies from creating unresolved Python imports when local copies preserved by earlier runs conflict with files written by the current run.

New Features:

  • Extend missing-name safety checks to copies preserved by earlier applies, both as imported modules and as importers of modules written by the current apply.
  • Provide targeted remediation guidance for earlier-kept copies and distinguish earlier-kept, current-kept, and upstream-owned bytes in refusal messages.

Bug Fixes:

  • Prevent plain applies from writing copies that would leave imports unresolved after an earlier apply preserved incompatible local bytes.
  • Ensure missing-name refusals occur before branch creation or any file write, including applies without --accept-local.

Enhancements:

  • Model the files as they will exist after the apply and refine refusal messaging and recovery paths for each copy ownership state.

Tests:

  • Add comprehensive end-to-end and in-process coverage for earlier-kept copies, both import directions, remediation exits, controls, and refusal-message behavior.

… apply writes against its module as the run leaves it, so a copy an earlier apply kept, read `unchanged`, is asked too, and no `siblings.py` stopping on its import is copied (#1082)

#536 built the guard's set of modules from the rows `--accept-local`
names in this run, so a copy an earlier apply kept, which reads
`unchanged` once the upstream stops changing it, was never asked. The
next apply wrote an importer beside it unasked. Measured offline: a
holder cut at 6476c17 that applied 2a119f2 keeping its own
bootstrap.py was then written main's siblings.py, with its `untraced`
import (#1075), by `apply --at 6da757c`. That apply exited 0 and
committed, and `siblings.py --help` stopped on an ImportError. The same
held for #1023's `member_make_environment` with #1023 landed in two
halves.

Each written .py copy is now asked against its module as the run leaves
it: a copy this run keeps (`(kept)`), a copy an earlier run kept, whose
pin records the project's own bytes (`(kept by an earlier run)`), or the
upstream's own bytes at the target (`(the upstream's)`). The code stays
`update-accept-local-missing-name`, and a refusal whose every line is
one this run's --accept-local makes prints what it printed. The guard
still runs before `confirm` and before any write. An importer this run
does not write is still not asked, but for a kept one against a module
this run writes (#604). So a copy an earlier run kept that imports a
name the written module drops is a known limit, named in the docstring.

Closes #1082

Lane: openreposhape-5 (openRepoShape-5)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sourcery-ai

sourcery-ai Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Reviewer's Guide

Implementation

Expand apply-time missing-import validation to evaluate module bytes as the run leaves them and detect disagreements involving copies kept by earlier runs, with source-specific refusal and remediation behavior.

  • update-shape.py L4250-4302, L4341-4379, L4596-4647 (+9 small edits)

Tests

Add end-to-end and unit coverage for earlier-kept copies as importers and imported modules, including refusal timing, diagnostics, controls, and remediation paths.

  • tests/test_update_shape_kept_helper.py L45-62, L1373-1387, L1769-2313 (+11 small edits)

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

Independent review of #1133 (#1082) at aa7ddf59, for lane openRepoShape-5: READY

The change does what #1082's Verify section asks. Both two-step cases are now refused before anything is written, the one-step refusal prints exactly what main prints, and no valid run I tried is refused. There are two SHOULDs: a real gap the PR documents but that is still inside the issue's Goal, and one claim of unchanged words that no row pins. Neither blocks a draft.

Findings

SHOULD 1: the known limit is a real gap inside #1082's Goal, so it needs a follow-up issue before this lands.

  • Where: the docstring paragraph at update-shape.py:4261, and the call at update-shape.py:4315, which passes kept alone to _kept_copies_importing_dropped. The row that pins the limit is tests/test_update_shape_kept_helper.py:2109.
  • Measured scenario ($R/tools/knownlimit.sh, where $R is /tmp/claude-1000/-workspace-projects-openRepoShape/50cede29-ab82-4ca3-9f67-d98e1a58c8d2/scratchpad/v118/rv1133; logs in $R/limit-h/ and $R/limit-m/):
    1. A family holder is cut at 4ed7c7bf. Its own edit to siblings.py adds from bootstrap import told_past, which the upstream's siblings.py does not import. --help still exits 0.
    2. apply --at U1 --accept-local scripts/siblings.py exits 0. U1 is main plus an unrelated change.
    3. U2 then renames told_past in bootstrap.py only. check reads upstream-changed scripts/bootstrap.py and unchanged scripts/siblings.py.
    4. A plain apply --at U2 exits 0, copies bootstrap.py and commits. siblings.py --help then exits 1 with ImportError: cannot import name 'told_past'. Main's tool and the head's behave the same.
    5. The one-step run, --accept-local scripts/siblings.py, is refused by update-shape.py apply --accept-local: a kept .py copy that imports a name the run's new repo_shape.py dropped passes, and make bootstrap then dies on an ImportError #604 with (written) no longer defines told_past.
  • Why it is the Goal's case: this run's own write breaks a copy an earlier run kept. The docstring's reason ("an edit that already broke an untouched importer") does not cover it: the importer worked before the run.
  • Cheap to fix: I applied the sketch below in a scratch tree:
    earlier = {row.path: row for row in rows
               if left.get(row.path) == EARLIER_MODULE}
    ... + _kept_copies_importing_dropped(root, written, {**kept, **earlier})
    • The new file then gave 1 failed and 178 passed. The one failure is the known-limit row itself.
    • The scenario above was then refused before any write, and the holder was unchanged.
    • _names_dropped already limits the refusal to names this run takes away, so this refuses no valid run.
    • The words still need work: the sketch prints (kept), and the opening for a copy this run's --accept-local keeps.
  • Ask: file that scenario as a follow-up issue, or widen the guard here.

SHOULD 2: nothing pins the opening of a (written)-only refusal.

NIT: the exits row does not check the row state the remedy promises.

  • Where: tests/test_update_shape_kept_helper.py:1955.
  • The remedy says the merged copy's row "then reads locally-modified". The row never checks that state, nor the drop exit's already-at-target.
  • By hand both are true. The merge exit gives locally-modified scripts/bootstrap.py. The drop exit gives already-at-target scripts/bootstrap.py, with kept ... (already the target's bytes; the row was stale).

NIT, the coordinator's #1111 check: there are none.

Checked and true

Could not verify

  • Shards A (2031 passed, 5 skipped) and B (1814 passed, 5 skipped). I only read the writer's logs; runs that size are not for a subagent.
  • The writer's M2, M4-M6 and M8-M10.
  • The writer's T1 shas, which depend on when they were made.
  • CI's git 2.55 and a Windows host.
  • S3776 counted by hand only; I sent no code to Sonar.

Checklist candidates

  • When a refusal gains a second opening, pin the old opening for every old kind of line, not just one.
  • When a guard is widened from "what this run's flag names" to "what earlier runs left", ask both directions, or file the direction left out at PR time.

Scratch left

  • Notes, probes and logs: /tmp/claude-1000/-workspace-projects-openRepoShape/50cede29-ab82-4ca3-9f67-d98e1a58c8d2/scratchpad/v118/rv1133/.
    • The local upstream clone is up0/.
    • Probe trees are probe-*, chain-* and limit-*.
    • Scripts are tools/probe1075.sh, tools/probe1023.sh, tools/chain.sh, tools/exits.sh, tools/knownlimit.sh, tools/scan_pairs.py, tools/mutate_r.py, tools/mutate_w.py and tools/mutants.sh.
  • Basetemps: /tmp/claude-1000/-workspace-projects-openRepoShape/50cede29-ab82-4ca3-9f67-d98e1a58c8d2/scratchpad/v110/bt/rv1133-*. The runs meant to fail (base243, mut, sketch, guard1111) kept their trees.
  • Removed: the worktrees rv1133-head, rv1133-merge and rv1133-mut, and the refs refs/l5rv/rv1133-pr, rv1133-merge and rv1133-pr1111. The predicted-merge commit e30a4324 is left as an unreferenced object.
  • Nothing of mine is running. I pushed, posted and filed nothing.

Lane: openreposhape-5 (openRepoShape-5)

…rlier apply kept, read `unchanged`, as an importer too, so a plain apply that drops a name it imports is refused before any write, naming who kept it and giving it its exits (#1082)

Fix round 1 of #1133, after rv1133's review. #604 asked only a copy
this run's --accept-local keeps against a module this run writes, so
the head left a known limit inside #1082's Goal. Measured by the
reviewer offline: a holder whose `apply --accept-local
scripts/siblings.py` kept its own siblings.py, edited to take
`told_past` from bootstrap.py, read `unchanged` against a target whose
bootstrap.py renamed it. A plain `apply` exited 0, copied bootstrap.py,
committed, and `siblings.py --help` stopped on an ImportError. Main's
tool and the head's behaved the same.

The rows read `kept by an earlier run` as a module are now asked as
importers too, beside the kept ones. `_names_dropped` still limits the
refusal to names this run takes away, so a copy an earlier run kept
whose names all stay still applies. Its line reads `(written) no longer
defines X, which Y (kept by an earlier run) imports from it`; the
general opening now fits every kind of line ("a copy imports a name
from a copy beside it whose bytes, as this run leaves them, do not
define it, and this run writes one of the two"); and the remedy gives
that copy its two exits, `--accept-local` on it once changed, or the
upstream's bytes by `git show`. A refusal whose every line is one this
run's --accept-local makes still prints what main prints, and a #604
row now pins its opening. The code stays
`update-accept-local-missing-name`. The known-limit paragraph and the
row that pinned the limit are replaced by rows that hold the refusal,
its controls and its exits; the exits rows also check the state `check`
reads after each exit.

Lane: openreposhape-5 (openRepoShape-5)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@brettheap

Copy link
Copy Markdown
Contributor Author

Re-check of #1133 (#1082) for lane openRepoShape-5

Verdict: READY at 53791e45239d55d696d8f37291196d61c5f809b3. I re-measured fix round 1 against rv1133's review (#1133 issuecomment-6321333729): SHOULD 1, SHOULD 2 and the NIT are all met. The round broke nothing I could find. There are three NITs, and none blocks the draft.

Findings

NIT 1: nothing pins how a refusal in both directions at once opens.

  • Where: update-shape.py:4561 sets the opening: lead = KEPT_LEAD if sides <= {KEPT_MODULE, WRITTEN_MODULE} else LEFT_LEAD. The row test_both_directions_in_one_run_are_one_refusal (tests/test_update_shape_kept_helper.py:1432) checks the lines, the clause and the remedy, but not the opening.
  • Mutant O1: lead = (KEPT_LEAD if len(sides) == 1 and sides <= {KEPT_MODULE, WRITTEN_MODULE} else LEFT_LEAD). Result: 185 passed, it survives (mut-O1-both-directions-lead-general.log).
  • Under O1, a --accept-local run whose lines are (kept) lacks plus (written) no longer defines would open with the general words. Every line in that run is one this run's --accept-local makes. The body and the commit message say such a refusal "still prints what main prints".
  • By reading only, not measured: the head gives that case main's KEPT_LEAD. I did not run a byte comparison with main for the both-directions log, because the LAST STEP came first.
  • Ask: add assert result.stderr.startswith(KEPT_LEAD), result.stderr to that row.
  • Why NIT: this is the gap class of rv1133's SHOULD 2 and of its checklist candidate. But SHOULD 2's own ask, one update-shape.py apply --accept-local: a kept .py copy that imports a name the run's new repo_shape.py dropped passes, and make bootstrap then dies on an ImportError #604 row, is met, and rv1133's R2 is killed.

NIT 2: the Merges section predates #1166.

NIT 3: the title names only the first round's direction.

Checked and true

Each finding is met. I ran the reviewer's own scripts, copied into my folder, with the head's tool and with main's.

Rows fail where the body says. I ran the head's test file with the other tools in scratch clones:

  • With main's update-shape.py (ca47f90e): 17 failed, 168 passed (base-main.log).
  • With aa7ddf59's (6c8467ee): 13 failed, 172 passed (base-aa7.log).
  • The 5 new rows fail on both: ..._drops_its_name, either_exit_for_an_earlier_kept_importer (2) and ..._asked_against_a_written_module (2). On main they fail as assert 0 == 2 or None is not None. The failing rows match the body's table.

Mutants. Each was applied alone in a scratch clone, then restored by checkout with its sha256 checked (5b386f8b). The clone's status was clean at the end (mutants.out).

mutant result
N3 (body) killed, 3 failed
N8 (body) killed, 1 failed
N9 (body) killed, 1 failed
R2 (rv1133's) killed, 1 failed
O3, mine: the mixed again wording for (kept) modules only killed, 1 failed
O1, mine survived: NIT 1

N3, N8, N9 and R2 fail the same counts the body gives.

What the round could have broken. I found nothing.

  • Fresh roots: family and project roots cut at 4ed7c7bf and at v1.0.0 hold no row the head reads as kept by an earlier run. I wrapped the head's _copies_as_left and stopped before any write; every .py row reads the upstream's (fresh-*/left.log). So the new importer pass asks nothing of a root no run has kept a copy in.
  • Real history, with a copy kept by --accept-local and then plain applies:
    • Family: siblings.py kept, cut 9e154d87, then 00f6d7f7, c13c3985, ff0c72eb.
    • Project: bootstrap.py kept, cut af0e60f2, then d3e1e490.
    • Every apply exits 0, and every apply log is identical to main's tool once masked (chain-[hm]-*).
  • A copy an earlier run kept whose import has its own except ImportError fallback: the plain apply that drops the name still applies, and the copy starts (fallback-h2.out).
  • After the conflict refusals, every row left is unchanged, upstream-changed, already-at-target, merged-away, or an accepted both or locally-modified. No --accept-local row can also be an earlier-kept one, and every use of a side constant handles WRITTEN_UNDER_EARLIER.

Gates and runs, all through run9.sh at 53791e45 with dirty=0:

git files result log
2.43.0 the test file and test_path_guard_hygiene.py 234 passed (185 + 49) own243.log
2.47.1 (first on PATH, printed in the log) the test file 185 passed own247.log
2.43.0 _add, _add_ignored, _add_symlink, _branch_any_directory, _hardlink 136 passed, 3 skipped rest1.log
2.43.0 _header_comments, _next_quoting, _partial_rollback, _pr_base, _unreadable 202 passed rest2.log
  • The last two rows are the 10 test_update_shape*.py files the body says did not run at this head.
  • CI at this head (run 39789285738): tests, package, tests-git-bash and SonarCloud all passed. The quality gate passed with 0 new issues.

Merges and body claims.

  • merge-tree origin/main HEAD gives bfa8c1d4, which is HEAD's own tree. origin/main is still 038dafba.
  • The merge commit 7aee5efa has tree 27a2785e, the same as merge-tree aa7ddf59 038dafba. It leaves both PR files as aa7ddf59 had them.
  • The writer's logs say what the body says: own243/own247 185 each, runC 144, runD 163, runE 219, runF 13, each START line at head=53791e45 dirty=0.
  • The test file has 205 asserts, and none joins conditions with and or or (checked on the syntax tree).
  • The fix round adds no path guard and no non-ASCII byte, and its widest added line is 79 characters.
  • closingIssuesReferences is [1082].
  • The commit subject is 250 characters, and the commit carries the Lane and Co-Authored-By lines.
  • The shipped template comments that name the code (siblings.py:216-234, bootstrap.py:2290-2299) are still true.

Could not verify

  • The byte comparison with main for the both-directions refusal (NIT 1). The LAST STEP came first.
  • CI's Windows behaviour; S3776 complexity was not re-counted.

Scratch left

  • Notes, logs and probes: /tmp/claude-1000/-workspace-projects-openRepoShape/50cede29-ab82-4ca3-9f67-d98e1a58c8d2/scratchpad/v123/rc1133/ (STATE.md, tools/).
  • Clones there:
    • base-main (keeps main's update-shape.py on purpose)
    • base-aa7
    • mut (restored)
    • merge1166 (predicted merge c5a98365, made in that clone only)
  • Worktree: /workspace/projects/openRepoShape-worktrees/rc1133-recheck, detached at 53791e45, clean.
  • Nothing of mine is running. I pushed, posted and filed nothing.

Lane: openreposhape-5 (openRepoShape-5)

…ts the refusal opens with `KEPT_LEAD`, as a refusal of this run's own `--accept-local` always has, so an opening that falls back to the general words there fails it (#1082)

rc1133's NIT 1 on #1133: update-shape.py:4561 gives a refusal whose lines are
all ones this run's `--accept-local` makes the opening `KEPT_LEAD`, and
`test_both_directions_in_one_run_are_one_refusal` (a `(kept) lacks` line plus
a `(written) no longer defines` line) pinned its lines, its clause and its
remedy but not its opening. rc1133's mutant O1, `lead = (KEPT_LEAD if
len(sides) == 1 and sides <= {KEPT_MODULE, WRITTEN_MODULE} else LEFT_LEAD)`,
survived the 185 rows. The row now asserts `startswith(KEPT_LEAD)`, and its
docstring says why the opening is that one. Tests only; update-shape.py is
untouched.

Lane: openreposhape-5 (openRepoShape-5)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@brettheap brettheap changed the title update-shape.py: update-accept-local-missing-name asks each copy an apply writes against its module as the run leaves it, so a copy an earlier apply kept, read unchanged, is asked too, and no siblings.py stopping on its import is copied (#1082) update-shape.py: update-accept-local-missing-name asks a copy an earlier apply kept, read unchanged, as a module a written copy imports from and as an importer of a written one, so a plain apply that stops either on its import is refused (#1082) Oct 22, 2026
@brettheap

Copy link
Copy Markdown
Contributor Author

Re-check of #1133 (#1082) NIT round for lane openRepoShape-5

READY at a7bfee1bd (parent 53791e45). The commit changes tests/test_update_shape_kept_helper.py alone (+7, -4), so update-shape.py is 53791e45's (sha256 5b386f8b). All three of rc1133's NITs (issuecomment-6347273216) are met. I found nothing new.

Checked and true

NIT 1: the startswith(KEPT_LEAD) assert

  • rc1133's O1 is lead = (KEPT_LEAD if len(sides) == 1 and sides <= {KEPT_MODULE, WRITTEN_MODULE} else LEFT_LEAD). I applied it alone to the head's update-shape.py. The file was restored and sha-checked each time (5b386f8b9b8bfe13, status 0 lines).
Run, git 2.43.0 Result
O1 and the head's test file (whole file) killed: 1 failed, 184 passed. The failure is assert result.stderr.startswith(KEPT_LEAD) at line 1452. The opening read a copy imports a name from a copy beside it whose bytes, as this run leaves them, do not define it, ..., which is the body's quote.
O1 and 53791e45's test file (whole file) survives: 185 passed. So the new assert is what kills O1.
The head's row alone, with main's update-shape.py (961640c68, sha256 ca47f90e) swapped in 1 passed, as the body says. The assert pins an opening the head keeps as main prints it.
  • The head's own runs pass:
    • git 2.43.0: 234 passed (185 plus the 49 rows of tests/test_path_guard_hygiene.py);
    • git 2.47.1 (printed in the log): 185 passed.
  • Each START line reads head=a7bfee1b dirty=0.

NIT 3: the title is true

  • It is 249 characters and ASCII. The code update-accept-local-missing-name is real (update-shape.py:4197).
  • A copy an earlier apply kept, read unchanged, is asked in both roles:
    • as the module a written copy imports from (_written_copies_lacking, EARLIER_MODULE);
    • as an importer of a written module (_kept_copies_importing_dropped, WRITTEN_UNDER_EARLIER).
  • The rows test_an_earlier_kept_copy_is_asked_when_the_next_apply_writes_its_importer and ..._when_the_next_apply_drops_its_name hold both directions and pass on the head.
  • The body is right about rc1133's 242-character rewrite (which I measured at 242 characters). Its "break it" is true only where the kept copy is the importer. Where it is the module, the apply breaks the written importer.

NIT 2: the Merges row for #1166

Body and commit

  • The live body equals the writer's body-post.md.
  • body-guard exits 0 through the Lane line.
  • closingIssuesReferences is [1082].
  • The commit subject is 241 characters and ASCII. The commit carries the Lane and Co-Authored-By lines.
  • The SonarQube API for PR 1133 shows 0 open issues and the quality gate OK. It lists the PR under the older title, so that analysis may predate the NIT commit.

Not measured

Lane: openreposhape-5 (openRepoShape-5)

@brettheap brettheap added this to the v1.2.1 milestone Oct 23, 2026
brettheap added a commit that referenced this pull request Nov 8, 2026
…py: an `id:`, a date or a `repository:` ending in a line feed is refused by name, and a pin's `source_repository:` that is no path or `owner/repo` name is named as the pin's (#1139) (#1166)

The value patterns the issue names refuse a value that ends in a line feed, by name, as #1126 (#1017) makes `COMMIT_RE`, `SHA256_RE` and `COMMIT_40` do:
- `DATE_RE` and `QUALIFIED_RE` in `validate-manifest.py` and `validate-family.py`, `MEMBER_RE` in `scripts/family.py`, `DATE_RE` in `scripts/shape_advisory.py` and `REPOSITORY_RE` in `update-shape.py`;
- `PROJECT_ID_RE`, read with `fullmatch` where the two validators and `family.py add` read an id from a file.

A shape pin's `source_repository:` that is neither a path on this machine nor an `owner/repo` name is refused in the pin's name, not `--upstream`'s. Values without a line feed keep their outcome, and no refusal code is added or renamed. Two inputs do move from one existing code to another (see "Behaviour changed", below).

**What this PR does not cover, and the title does not claim:**
- the naming-policy patterns that `scripts/repo_shape.py` reads from `contracts/repository-naming.yaml`, which `validate-repository-naming.py` still reads with `.match`;
- `update-upstream-not-a-repo`, which still says `--upstream` when a pin names a folder that exists and is not a repository.

Both are follow-ups, with the other findings of the independent review, under "Follow-ups (not in this PR)".

This does not need #1126 and does not touch its files or its test file.

Measured on main `038dafba` (Python 3.12.3, git 2.43.0) by a probe of each tool, before any change. Each value is written in the file as a YAML double-quoted `"...\n"`, which the reader takes as the value and a line feed.

| Read from | Value | On main |
|---|---|---|
| `project.yaml` (`validate-manifest.py`) | `id: "atlas\n"` | only `FINDING manifest-topic: topic is 'xf-project-atlas', expected 'xf-project-atlas\n' derived from id 'atlas\n'`, exit 1 |
| `project.yaml` | `elected_on: "2026-09-02\n"` | `manifest ok`, exit 0 |
| `project.yaml` | a leg's `repository: "testorg/Atlas-spec\n"` | `manifest ok`, exit 0 |
| `family.yaml` (`validate-family.py`) | `id: "inkrouter\n"` | `family ok: InkRouter (inkrouter` and a line break, exit 0 |
| `family.yaml` | `created_on: "2026-09-04\n"` | `family ok`, exit 0 |
| `family.yaml` | a member's `repository: "InkRouter/IRRS\n"` | `FINDING member-repository-name: ... whose name is 'IRRS\n'`, exit 1 |
| a member's `project.yaml`, read by `family.py add` | `id: "irqs\n"` | mounted and committed, exit 0, with the row `id: "irqs\n"`; the holder's gate then said `family ok` |
| `contracts/shape-pin.yaml`, `update-shape.py check` with no `--upstream` | `source_repository: "opensoft/openRepoShape\n"` | `REFUSED unsafe-value: --upstream is 'opensoft/openRepoShape\n'`, exit 2 |
| the same | `source_repository: "opensoft/open Repo"` | `REFUSED update-upstream-unresolvable: --upstream 'opensoft/open Repo' is neither ...`, exit 2 |
| the same | `source_repository: "-opensoft/openRepoShape"` | `REFUSED unsafe-value: --upstream is '-opensoft/openRepoShape'`, exit 2 |
| an adoption plan (`adopt-project.py check`) | `id: "northwind\n"` | `FINDING plan-manifest-invalid: id is 'northwind\n', which the project's own validate-manifest.py refuses: manifest-topic: ...`, exit 1; `execute` refused it, exit 2, before any leg |

In-process on the same main: `repo_shape.PROJECT_ID_RE.match('atlas\n')`, `shape_advisory.DATE_RE.match('2026-09-02\n')` and `REPOSITORY_RE.match('a/b\n')` each matched, and `date.fromisoformat('2026-09-02\n')` raised `ValueError`.

- **`DATE_RE` and `QUALIFIED_RE`** in `templates/assembly-root/scripts/validate-manifest.py` and `templates/family-root/scripts/validate-family.py` end in `\Z`. So do `MEMBER_RE` in `scripts/family.py` (the third spelling `tests/test_family_member_name.py` holds equal to the two `QUALIFIED_RE`), `DATE_RE` in `scripts/shape_advisory.py` and `REPOSITORY_RE` in `update-shape.py`.
- **`PROJECT_ID_RE` keeps its `$`.** Every id refusal prints its text (`must match ^[a-z0-9][a-z0-9-]*$`, pinned in six test files), the validators run against each project's own copy of `repo_shape.py`, and its definition sits inside #1126's hunk. The three callers that read an id from a file ask it with `fullmatch` instead: `validate-manifest.py` (`manifest-id`), `validate-family.py` (`family-id`) and `family.py`'s reading of a member's `project.yaml` (`member-bad-id`, which `add` now says before anything is mounted).
- **`update-shape.py`.** `open_upstream(spec, pin_path=None)` takes the pin the value was read from, which `prepare` passes when no `--upstream` was given. Then `update-upstream-unresolvable` says ``<root>/contracts/shape-pin.yaml: `source_repository:` is '...', which is neither a path on this machine nor an `owner/repo` name``, with a remedy to write the pin's field or pass `--upstream`, and `checked_value` names the same field. **Code kept:** `update-upstream-unresolvable`, the existing code for a value that is not an `owner/repo` name. A line feed now reaches it, since the pattern no longer takes one. Before, `checked_value` refused it as `unsafe-value`.
- **One existing test line:** `tests/test_family_init_values.py` finds validate-family's `DATE_RE` line by its text, which now ends `\Z`.
- **One existing stand-in:** the `temporaries` fixture in `tests/test_update_shape_unreadable.py` stands in for `open_upstream` with `def opening(spec)`, which the new two-argument call refused with `TypeError: ... takes 1 positional argument but 2 were given`. It now takes `pin_path=None` and hands it on to the real `open_upstream`, so `test_a_read_refused_in_prepare_closes_the_upstream[check|apply]` asks what it asked before.
- **The rows** are in a new file of their own, `tests/test_value_pattern_line_break.py`.
- **A second guard, in a second commit (`5102b4294`, a test only, no production file).** The first guard holds the four pattern names this PR ends in `\Z`, and nothing held `PROJECT_ID_RE`, which keeps its `$`. `test_every_reader_of_the_id_pattern_is_in_the_table` reads every tracked `.py` outside `tests/` with `ast` (it runs none) and holds each call of a method of `PROJECT_ID_RE` (`PROJECT_ID_RE.x(...)` or `repo_shape.PROJECT_ID_RE.x(...)`) to the table `ID_READERS`:
  - the three `fullmatch` readers of an id from a file (`validate-manifest.py`, `validate-family.py`, `scripts/family.py`);
  - the modules that still `match` it, each with its reason: argv `--id` in `scaffold-project.py` and `scripts/family.py`, where `checked_value` refuses a line feed after the pattern; argv `--id` in `adopt-project.py`, which the project's own `validate-manifest.py` refuses (`adopt-manifest-invalid`, below; `ID_READERS` still gives it the `checked_value` reason); and `adopt-project.py`'s two `plan-bad-id` readers, of the plan's `id:` and of the id derived from `legs.assembly`, which a line feed never passes (below).

  A new reader of `PROJECT_ID_RE`, `fullmatch` included, fails it by file and method until it is listed in `ID_READERS` with its reason. The table is exact both ways, so a reader taken away fails it too.

**Left as they are, and why:**
- the argv readers of `PROJECT_ID_RE` (`--id` in `scaffold-project.py`, `family.py init` and `adopt-project.py`). The first two hold the id to `checked_value` after the pattern, which refuses a line feed as `unsafe-value` naming `--id`, and `tests/test_derived_id_refusals.py` pins that answer for them. `adopt-project.py` never calls `checked_value` on `--id`: `plan` with `--id` set to `northwind` and a line feed exits 2 with `adopt-manifest-invalid`, in the words of the project's own `validate-manifest.py` (`manifest-id`), and writes no plan (lane 5's re-check, issuecomment-6371408915, NIT 2; measured on `plan` only);
- `adopt-project.py`'s reading of a plan's `id:`: `check` and `execute` hold it to the project's own `validate-manifest.py`, which refused it on main (above) and now names it in `manifest-id`'s words. It is the same value, refused under the same code, `plan-manifest-invalid`, before any leg exists. The id derived from `legs.assembly` is read with `.match` too, and a line feed in `legs.assembly` is `unsafe-value` before that id is asked (the review measured both, on main and here alike). This keeps the open PRs on `adopt-project.py` (24 at 12:02Z) clear;
- `PIN_ARG_RE`, and `NAME_RE`/`REF_RE`/`PATH_RE` in `setup-project.py`, as the issue says;
- `update-upstream-not-a-repo` still says `--upstream`. A pin's value reaches it only when it names a path that exists, not through the pattern.

**Behaviour changed, stated plainly, and what the issue's Verify means by "No refusal code changes":** the issue's Verify line reads as no code added or renamed, and none is. Two inputs do move from one existing code to another, exit 2 both before and after:
- `update-shape.py --upstream "opensoft/openRepoShape<LF>"` was `unsafe-value` and is now `update-upstream-unresolvable`, still naming `--upstream`;
- a pin's `source_repository:` that ends in a line feed, with no `--upstream`, was `unsafe-value` naming `--upstream` and is now `update-upstream-unresolvable` naming the pin's field.

No test pinned either old answer: every gate below passed. `family.py add --member` is stripped before `MEMBER_RE`, so `add` answers as before.

**Not changed, seen while measuring:** with `--upstream` passed, `update-shape.py` does not read the pin's `source_repository:` through `REPOSITORY_RE` at all, and it keeps the value as the upstream's repository name. `validate-family.py` does not hold a member row's `id:` to `PROJECT_ID_RE`; it compares the row with the member's manifest. Neither is a `$` question. Both are under "Follow-ups (not in this PR)".

All in `tests/test_value_pattern_line_break.py`, run against main's production code: the five production files swapped in place from `origin/main` (`961640c68`), then put back with their sha256 matching (`dirty=5` in the run's START line is those five files). On the head `5102b4294` (30 rows) the result is **20 failed, 10 passed in 23.12s, git 2.43.0**. The 10 that pass are baselines: the seven `test_a_pattern_takes_its_value` rows, the spelling-table guard and the two double-quoted-without-a-line-feed rows.

The first 19 are the first commit's rows (19 failed, 10 passed in 22.12s on its 29 rows at `038dafba`). The independent review reproduced them from main's bytes of the same five files (19 failed, 10 passed in 26.78s, the same 19 ids). The 20th, `test_every_reader_of_the_id_pattern_is_in_the_table`, is the second commit's: it fails on main because the three readers of an id from a file are `.match` there, not `.fullmatch`.

- `test_a_pattern_refuses_its_value_and_a_line_feed[manifest-DATE_RE]`
- `test_a_pattern_refuses_its_value_and_a_line_feed[manifest-QUALIFIED_RE]`
- `test_a_pattern_refuses_its_value_and_a_line_feed[family-DATE_RE]`
- `test_a_pattern_refuses_its_value_and_a_line_feed[family-QUALIFIED_RE]`
- `test_a_pattern_refuses_its_value_and_a_line_feed[family.py-MEMBER_RE]`
- `test_a_pattern_refuses_its_value_and_a_line_feed[shape_advisory-DATE_RE]`
- `test_a_pattern_refuses_its_value_and_a_line_feed[update-shape-REPOSITORY_RE]`
- `test_a_manifest_value_that_ends_in_a_line_break_is_a_finding[id]`
- `test_a_manifest_value_that_ends_in_a_line_break_is_a_finding[elected_on]`
- `test_a_manifest_value_that_ends_in_a_line_break_is_a_finding[leg-repository]`
- `test_adopt_check_names_a_plan_id_that_ends_in_a_line_break_as_an_id`
- `test_a_family_value_that_ends_in_a_line_break_is_a_finding[id]`
- `test_a_family_value_that_ends_in_a_line_break_is_a_finding[created_on]`
- `test_a_family_value_that_ends_in_a_line_break_is_a_finding[member-repository]`
- `test_add_refuses_a_member_whose_id_ends_in_a_line_break`
- `test_a_pins_source_that_is_no_owner_repo_is_named_as_the_pins[line-feed]`
- `test_a_pins_source_that_is_no_owner_repo_is_named_as_the_pins[space]`
- `test_a_pins_source_checked_value_refuses_is_named_as_the_pins`
- `test_an_upstream_passed_with_a_line_feed_is_named_as_passed`
- `test_every_reader_of_the_id_pattern_is_in_the_table`

**13 of 13 killed.** Each mutant was applied alone to one file of the branch's production bytes (the same bytes as the first commit and as the head, which adds a test only), the rows were run, and the file was put back with its sha256 checked (`restored=yes` for all 13). Each run is `tests/test_value_pattern_line_break.py` plus `tests/test_family_member_name.py::test_every_spelling_of_the_member_name_rule_is_the_same_pattern`, git 2.43.0.

| Mutant | File | Killed by |
|---|---|---|
| M1 `DATE_RE` back to `$` | `validate-manifest.py` | 2: `test_a_pattern_refuses_its_value_and_a_line_feed[manifest-DATE_RE]`, `test_a_manifest_value_that_ends_in_a_line_break_is_a_finding[elected_on]` |
| M2 `QUALIFIED_RE` back to `$` | `validate-manifest.py` | 3: `...[manifest-QUALIFIED_RE]`, `...[leg-repository]`, `test_every_spelling_of_the_member_name_rule_is_the_same_pattern` |
| M3 the id read with `.match` | `validate-manifest.py` | 2: `test_a_manifest_value_that_ends_in_a_line_break_is_a_finding[id]`, `test_adopt_check_names_a_plan_id_that_ends_in_a_line_break_as_an_id` |
| M4 `DATE_RE` back to `$` | `validate-family.py` | 2: `...[family-DATE_RE]`, `test_a_family_value_that_ends_in_a_line_break_is_a_finding[created_on]` |
| M5 `QUALIFIED_RE` back to `$` | `validate-family.py` | 3: `...[family-QUALIFIED_RE]`, `...[member-repository]`, `test_every_spelling_of_the_member_name_rule_is_the_same_pattern` |
| M6 the id read with `.match` | `validate-family.py` | 1: `test_a_family_value_that_ends_in_a_line_break_is_a_finding[id]` |
| M7 `MEMBER_RE` back to `$` | `scripts/family.py` | 2: `...[family.py-MEMBER_RE]`, `test_every_spelling_of_the_member_name_rule_is_the_same_pattern` |
| M8 a member's id read with `.match` | `scripts/family.py` | 1: `test_add_refuses_a_member_whose_id_ends_in_a_line_break` |
| M9 `DATE_RE` back to `$` | `scripts/shape_advisory.py` | 1: `...[shape_advisory-DATE_RE]` (its pattern row alone: `date.fromisoformat` refuses the line feed after the pattern, so no tool row sees it) |
| M10 `REPOSITORY_RE` back to `$` | `update-shape.py` | 3: `...[update-shape-REPOSITORY_RE]`, `test_a_pins_source_that_is_no_owner_repo_is_named_as_the_pins[line-feed]`, `test_an_upstream_passed_with_a_line_feed_is_named_as_passed` |
| M11 `prepare` passes no pin | `update-shape.py` | 3: `test_a_pins_source_that_is_no_owner_repo_is_named_as_the_pins[line-feed]` and `[space]`, `test_a_pins_source_checked_value_refuses_is_named_as_the_pins` |
| M12 `checked_value` names `--upstream` again | `update-shape.py` | 1: `test_a_pins_source_checked_value_refuses_is_named_as_the_pins` |
| M13 the pin's refusal sentence removed | `update-shape.py` | 2: `test_a_pins_source_that_is_no_owner_repo_is_named_as_the_pins[line-feed]` and `[space]` |

No mutant is added for the stand-in in `tests/test_update_shape_unreadable.py`: it is a test change.

The independent review re-ran M3, M8, M11 and M12, and one mutant of its own (X1: `prepare` names the pin even when `--upstream` is passed). All five were killed, and each file was put back with its sha256 checked. The other nine of the 13 are this writer's runs alone.

**The second commit's guard: 6 of 6 probes caught.** Each was applied alone to one tracked file at `5102b4294`, `test_every_reader_of_the_id_pattern_is_in_the_table` was run, and the file was put back with its sha256 checked (`restored=yes` for all six). The first commit's guard, `test_every_spelling_of_a_value_pattern_is_in_the_table`, was run with each plant too: it passes every one of them, which is the gap the second guard closes.

| Probe | File | The new guard | The first guard |
|---|---|---|---|
| P1 a new `PROJECT_ID_RE.match(value)` reader appended | `update-shape.py` | failed: `('update-shape.py', 'match'): 1` is more than the table holds | passed |
| P2 a new `repo_shape.PROJECT_ID_RE.search(value)` reader appended | `scripts/shape_advisory.py` | failed | passed |
| P3 `fullmatch` back to `match` | `templates/assembly-root/scripts/validate-manifest.py` | failed | passed |
| P4 `fullmatch` back to `match` | `templates/family-root/scripts/validate-family.py` | failed | passed |
| P5 `fullmatch` back to `match` | `scripts/family.py` | failed | passed |
| P6 main's bytes of those three files together (the 20th row above) | the three | failed | not run |

**The second commit `5102b4294` (dirty=0)**, Python 3.12.3, with the same stub `gh`, bogus token and no global git config:

| Run | Git | Result |
|---|---|---|
| `tests/test_value_pattern_line_break.py` | 2.43.0 | **30 passed** in 23.00s |
| `tests/test_value_pattern_line_break.py` | 2.47.1 | **30 passed** in 23.30s |
| `tests/test_pinned_copy_text.py`, `tests/test_path_guard_hygiene.py`, `tests/test_repo_hygiene.py`, `tests/test_agents_md_codes.py`, `tests/test_cli_reference.py` | 2.43.0 | **306 passed** in 61.54s |

That commit changes only the test file (62 lines added, no assert that joins conditions with `and` or `or`, ASCII), so the first commit's runs below are not repeated: they hold the production files and every other test file, and those are unchanged. CI on `5102b4294`, and an independent look at the second commit, were not done when this was written.

**The independent review of the first commit** (rv1166c, READY, read-only) reports these gates on `2593952c9`, all passing: the stand-in gate's 6 files (298 passed), the new file on 2.47.1 (29 passed), the five hygiene files (306 passed), 15 more test files that name a changed function, pattern or code (1611 passed, 19 skipped) and `tests/test_shape_doctor.py` (445 passed, 3 skipped). It also read CI on that head at about 20:04Z: `tests` (the full suite on 3.14), `tests-git-bash`, `package` and SonarCloud all passed.

**The first commit's gates ran on `2593952c` (dirty=0)**, Python 3.12.3, with the real `gh` unreachable (a stub first on `PATH`, a bogus token) and no global git config:

| Run | Git | Result |
|---|---|---|
| the files that held every failure and setup error of the shards below: `tests/test_family.py`, `tests/test_update_shape_pr_base.py`, `tests/test_update_shape_unreadable.py`, `tests/test_update_shape_header_comments.py`, `tests/test_update_shape_messages.py` and the new `tests/test_value_pattern_line_break.py` | 2.43.0 | **298 passed** in 343.60s |
| `tests/test_value_pattern_line_break.py` | 2.47.1 | **29 passed** in 21.31s |
| `tests/test_pinned_copy_text.py`, `tests/test_path_guard_hygiene.py`, `tests/test_repo_hygiene.py`, `tests/test_agents_md_codes.py`, `tests/test_cli_reference.py` | 2.43.0 | **306 passed** in 69.12s |

**The two shards, on the uncommitted tree** holding the same production bytes as the first commit (the 7 files' sha256 checked before and after), git 2.43.0: the validators' test files, the `REPOSITORY_RE`/`source_repository` files and the `update-shape.py`, `shape_advisory.py` and `family.py` files, 125 in all.

- shard 1 (62 files): 4 failed, 5290 passed, 13 skipped in 56 min;
- shard 2 (63 files): 3 failed, 4 setup errors, 3761 passed, 29 skipped in 50 min.

Every failure and setup error is in the five existing files of the first row of the table above, which all pass on the first commit:
- 2 were real: `test_a_read_refused_in_prepare_closes_the_upstream[check|apply]`, the `opening(spec)` stand-in under the two-argument call. The stand-in is fixed in the first commit.
- The other 5 failures and the 4 setup errors were `update-conflict` on `scripts/validate-family.py` ("not a verbatim copy ... at the pinned commit"). This is an artifact of an uncommitted tree: `tests/test_family.py`'s `upstream_clone` commits the working tree into its clone, while `family.py init` pins the holder at the real `HEAD`, so an uncommitted `templates/family-root/` file is never verbatim at its pin. On the first commit they pass.

Every other file of the two shards passed on these production bytes, so the shards were not re-run.

`git merge-tree --write-tree <other> 5102b42` (git 2.43.0) at 2026-10-23T12:08Z, after re-fetching main (still `961640c6`; read again at 12:10Z, just before the push) and re-listing the open PRs at 12:02Z (55 open). Main has moved from the `038dafba` that this body measured to `961640c6` by one file, `docs/release-notes/v1.2.0.md`. Each open PR that touches one of this PR's 8 paths, at its current head, plus #1126:

| Against | Head | Shares | Result |
|---|---|---|---|
| origin/main | `961640c6` | | clean, tree `4613729516ff` |
| #1098 | `dad7961a` | `scripts/family.py` | clean, tree `4d1ff9517588` |
| #1117 | `10dc011c` | `scripts/family.py` | `scripts/family.py` merges clean; conflict in `docs/handbook.html` only, which this PR does not touch (see below) |
| #1124 | `8dfa32bd` | `validate-family.py` | clean, tree `cb2ec1073646` |
| #1126 | `4c1f80cf` | none (its `scripts/repo_shape.py` and tests) | clean, tree `e82c8945ac37` |
| #1133 | `a7bfee1b` | `update-shape.py` | clean, tree `a710870d1ffc` |
| #1144 | `89486f69` | `scripts/shape_advisory.py` | clean, tree `2d3e3e1ef43d` |
| #1147 | `b75babee` | `validate-manifest.py` | clean, tree `a82ac3b47a3f` |
| #1159 | `66e68f94` | `scripts/shape_advisory.py` | clean, tree `4abeb7b38665` |
| #1164 | `74096eb4` | `scripts/family.py` | clean, tree `ee285e1cb8cf` |
| #1172 | `5aa0f116` | `validate-family.py` | clean, tree `aa32d41e6e2c` |
| #1187 | `cc8a87ee` | `update-shape.py`, `tests/test_update_shape_unreadable.py` | clean, tree `25711eb1cac4` |
| #1201 | `eac177af` | `scripts/family.py` | `scripts/family.py` merges clean; conflict in `docs/handbook.html` only (see below) |

- #1117 and #1201 conflict in `docs/handbook.html` alone. #1201 is stacked on #1117 (their merge-base is #1117's head `10dc011c`). Each of them against origin/main alone gives the same conflict in the same file, so it is theirs: #1117's, from its older base `6da757cb`, which #1201 inherits.
- #1187 and #1201 are new since the first merge-trees. #1187 shares `tests/test_update_shape_unreadable.py` with this PR; its diff adds or removes no line that names `open_upstream`, `def opening` or `REPOSITORY_RE`, so it brings no one-argument stand-in for the two-argument call. That was read for all 55 open heads at 12:12Z; only this PR's own lines name them.
- No other open PR adds or removes a call of a method of `PROJECT_ID_RE`, which the second guard counts: the lines each of the 55 open heads adds or removes outside `tests/`, `docs/` and `*.md` were read at 12:04Z. Only this PR's own name it, and #1201's one new `MEMBER_RE.fullmatch` is another pattern. So a sibling landing first does not move the guard's table.

Each is a candidate for its own issue. They come from the independent review (rv1166c, READY at `2593952c9`, issuecomment-6352173084), which measured each of them on main and at the head unless the item says it did not. The review's finding numbers are in brackets.

- **The naming-policy patterns [1].** `scripts/repo_shape.py` reads the families, the roles and the topic pattern of `contracts/repository-naming.yaml` with `.match` (`family["_re"].match(name)`, `family["_split_re"].match(name)`, `role["_re"].match(name)`, and `topic_pattern`, which defaults to `^xf-project-[a-z0-9-]+$`). `validate-repository-naming.py --project project.yaml` runs in the root template's `Makefile` and `validate.yml`. At the head, and on main, it exits 0 on a leg `repository: "testorg/Atlas\n"` and prints the line feed inside its table; `-spec` and `-code` the same. The project's gate as a whole still fails, because `validate-manifest.py` now refuses that manifest.
- **`update-upstream-not-a-repo` with a pin [3].** With no `--upstream`, a pin whose `source_repository:` names a folder that exists and is no repository (the review measured `contracts` and `scripts/repo_shape.py`) is `REFUSED update-upstream-not-a-repo: --upstream contracts exists but is not a git repository`, exit 2, on main and here. `pin_path` is in scope at `update-shape.py:686-687`, so that refusal can name the pin as `update-upstream-unresolvable` now does.
- **A pin's line feed when `--upstream` is passed [9].** `update-shape.py check --upstream <clone>` over a pin whose `source_repository:` ends in a line feed exits 0, `nothing to do`. The value is kept as `upstream.repository`, which reaches the README shape line's check, `apply`'s commit subject and the pull request words (`update-shape.py:2525`, `3017` and `5121`). The review measured `check`, not `apply`. It belongs with the previous item.
- **A member row's `id:` [4].** On main, `add` of a member whose `project.yaml` reads `id: "irqs\n"` commits the row `id: "irqs\n"`, and the holder's `validate-family.py` then says `family ok`. Under mutant M8 (this branch with main's `.match` in `add`) the same row is written, and the gate still says `family ok`. By code reading, not measured end to end: `validate-family.py` compares a row's id with the member manifest's by `str` equality (`member-id-mismatch`, `validate-family.py:1608`), and `_check_member_ids_unique` does the same, so neither asks `PROJECT_ID_RE` of a row. This PR makes `add` refuse such a member, so no new holder gets such a row.
- **Values that no pattern reads [8].** The review's sweep found these `manifest ok` or `family ok`, exit 0, with a trailing line feed, at the head and on main alike:
  - in `project.yaml`: `name`, a spec or code leg's `path`, `shape.repository` and `shape.digests.digest_algorithm`;
  - in `family.yaml`: `org`, the family's own `repository`, `tracking_branch`, and the shape's and each member pin's `digest_algorithm`.

  No `^...$` pattern reads them, so they are not this PR's. A leg's `path` is checked only by `_leg_path_findings`, which asks no pattern, and the family's `tracking_branch` is refused in `project.yaml` (`manifest-tracking-branch`, U+000A) but not in `family.yaml`. `shape.commit` and `tree_sha256` with a line feed are also `manifest ok`; that is #1126's.

The first commit's message says "the three readers of an id from a file ask it with `fullmatch` (`manifest-id`, `family-id`, `member-bad-id`)". That is true of those three, but it reads as if there were no others [2]. What the review measured instead:
- `adopt-project.py` still reads the id derived from `legs.assembly` (`:6780`) and the plan's `id:` (`:6791`), both `plan-bad-id`, with `.match`;
- neither lets a line feed through. A plan `id: "northwind\n"` is `plan-manifest-invalid`, now in `manifest-id`'s words. A plan with no `id:` and `legs.assembly: "NorthWind\n"` is `REFUSED unsafe-value: legs.assembly is 'NorthWind\n'`, exit 2, before the derived id is asked, on main and here alike.

So it is the wording alone that overstates. The second commit's `ID_READERS` lists both readers with that reason.

The title's pin clause is the narrower one (the pin's name only where the value is no path and no `owner/repo` name), and two inputs change from `unsafe-value` to `update-upstream-unresolvable` (see "Behaviour changed", above).

Review: rv1166 and rv1166c, lane 5's independent opus reviewers, at `2593952c` (issuecomment-6352173084); then rc1166b, lane 5's independent sonnet re-checker, which wrote none of #1166: READY at `5102b429`, H0 of this landing (issuecomment-6371408915), with no MUST or SHOULD; its NITs 1 and 2 are corrected in the body. CI (Brett Heap's ruling 27 and his fast-refresh ruling of 2026-10-15T22:48:35Z): H0 5102b42 passed tests, tests-git-bash and SonarCloud; the head 5c8afef is a clean update-branch merge of main 93d2931, with tests-git-bash and SonarCloud green; and the local gate passed on its predicted squash tree 3c89065 over main 11560eb: the family_init_values, update_shape_unreadable, value_pattern_line_break, repo_hygiene, agents_md_codes, cli_reference sets, 365 passed, 19 skipped. Landed on Brett Heap's word of 2026-10-23T13:10:26Z, relayed to every lane by lane 3 (#473 issuecomment-6365697123): "merge v1.2.1 PRs once reviewed and green".

Closes #1139

Lane: openreposhape-5 (openRepoShape-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sonarqubecloud

sonarqubecloud Bot commented Nov 8, 2026

Copy link
Copy Markdown

@brettheap
brettheap marked this pull request as ready for review November 8, 2026 14:55
@sourcery-ai

sourcery-ai Bot commented Nov 8, 2026

Copy link
Copy Markdown

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 6 days and 22 hours by commenting @sourcery-ai review. Upgrade to get a review now.

@brettheap
brettheap merged commit b66d8f0 into main Nov 8, 2026
5 checks passed
@brettheap
brettheap deleted the fix/1082-guard-earlier-kept branch November 8, 2026 14:56
brettheap pushed a commit that referenced this pull request Nov 8, 2026
Main 0d5704b is five commits past #1144's squash b1f45fa (#1169, #1223,
#1166, #1133, #1104). This is the third of the three merges that restack
#1159 after #1144 landed.

Lane: openreposhape-1 (openRepoShape-1)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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