Skip to content

family.py add: a commit that records the member's gitlink anywhere but members/<Project> is taken back and refused, naming the path git chose, and members: null, Null, NULL or ~ takes the first member as members: does (#920) - #957

Merged
brettheap merged 4 commits into
mainfrom
fix/920-add-gitlink-and-null-members
Oct 19, 2026

Conversation

@brettheap

@brettheap brettheap commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Lane: openreposhape-4 (openRepoShape-4)
Claim: #920 (comment)

family.py add now checks its own commit for the gitlink at exactly members/<Project>, and takes back a commit that records it anywhere else. It also finds a members: line spelled as any null the holder's reader reads, and refuses a manifest with more than one members: line. These are #459's lines U049 (#363's option 2, which PR #574 left out) and U188 (RV870's NIT-3), as triaged by lane 3 in #920. Round 1 answers lane 5's review (issuecomment-6253361722).

Base: main at 64464d31. Head: 39f84646, two commits: 37faf298, then round 1 as a fast-forward. scripts/family.py is not a pinned copy: it is not in FAMILY_COPIED_VERBATIM, which lists validate-family.py, bootstrap.py, siblings.py, Makefile, .gitignore, .gitattributes, validate.yml and AGENTS-shape.md.

1. The commit, after the fact (U049)

Before (50178305). Nothing read the commit, so add exited 0 three ways, each leaving the branch on a commit whose .gitmodules and family.yaml name members/IRQS:

Now. After _commit_one, _checked_gitlink_committed reads, with git ls-tree -r and no pathspec, the whole tree of the commit and of the one the branch named before the mount. Where the commit holds no gitlink at exactly members/<Project>, it refuses member-gitlink-misplaced. The refusal names the path git chose: each gitlink at a path the earlier commit did not have, or one that records the member's commit (round 1, NIT 2). Where there is none, it says so.

Before raising, MountRollback.take_back moves the branch back with git update-ref -m ... HEAD <before> <made>, or -d HEAD <made> for a first commit. It does this only where the commit is one commit on where the command found the branch, and only while the branch still names it. undo then unwinds the mount, as it does for every refusal after the mount. #363's state, through the same probe:

REFUSED member-gitlink-misplaced: the commit 5e1afd1cba53 records IRQS's gitlink at `Members/IRQS`, not at `members/IRQS`, while .gitmodules and family.yaml name `members/IRQS`, so a fresh clone would leave the member empty. It has been taken back: the branch names 04b3db34f2aa again. Nothing was added: the mount has been undone — the gitlink at members/IRQS unstaged, members/IRQS removed, .git/modules/members/IRQS removed, .gitmodules removed, as there was none before, family.yaml restored byte for byte, `submodule.members/IRQS` dropped from the holder's git config — and the holder is exactly as it was before this command.
Remediation: `add` asks git for the gitlink at `members/<Project>` and nowhere else, and every cause known to record it elsewhere, another spelling of `members/` in the index, in HEAD or on disk, is refused before the mount, so this cause is not one of them. Find what moved it, such as a hook or a setting like `core.ignorecase`, and mend that, then run the same command again.
exit 2

HEAD is back on its parent, and the status is the human's own D Members/README.md. The reflog reads family.py add: 5e1afd1cba53 taken back. The two hooks give records no new gitlink, at `members/IRQS` or anywhere else and records IRQS's gitlink at `parked/IRQS`, each taken back, exit 2, status clean.

Where a post-commit hook takes the commit back off itself (git reset --soft HEAD~1), the branch is where add found it. That is said as it is (round 1, NIT 1): the branch names <sha> again, where this command found it: the commit this command made is not on it, so no gitlink at members/IRQS is either. undo unwinds, and the remedy ends then run the same command again.

Where the commit is not taken back, nothing is undone. This happens where a post-commit hook has put a commit of its own on top, or where git update-ref finds another commit there. The sentence says why (<sha> is not one commit on <before>, where this command found the branch, and this tool takes back no commit it did not make), and undo says The commit had already landed. The remedy then ends: The commit and the mount are still there, for the reason above: `git log` shows what the branch holds, and taking them out is yours to do before you run the same command again.

2. members: null (U188)

Before (50178305). Holders made by init had their members: [] line rewritten and committed, then got the real add. members: null, Null, NULL and ~ each gave:

REFUSED family-members-unrecognised: <holder>/family.yaml has 0 `members:` line(s) where exactly one was expected, so this tool will not rewrite it
exit 2

The holder's own validate-family.py passes each of the four (family ok: Orbit (orbit), 0 member(s)). So does Family.open (_checked_members_readable: "Absent or null is no members"). members: [] was mounted, exit 0.

Now. _opens_members takes members:, members: [], and also members: <word> where the holder's reader (parse_yaml) reads that one word as null. All four are mounted, exit 0, and members: is written with the one row under it. The holder's validator then says family ok: Orbit (orbit), 1 member(s), exit 0, for each.

members: null # none yet is still refused as before (see "What it cannot tell").

A second members: line (round 1, SHOULD 1). Family.rewritten stopped at the first line it took and never counted a second. On main, a members: [] at the margin above name:, with the real block still at the end, was cut off there with exit 0. The reader keeps the last of two keys, so Family.open and the holder's gate both accept that file. Every key below the line went, and the gate then failed the commit. At 37faf298, members: null did the same. Every line _opens_members takes is now counted first, and anything but one is refused before the mount. Measured with an Orbit holder from init, a line above name::

Line above name: main 64464d31 37faf298 now
members: null exit 0, all keys kept exit 0, cut to id, kind, members, schema_version; validator exit 1 exit 2, has 2 members: line(s), nothing written; validator 0 member(s)
members: [] exit 0, cut; validator exit 1 exit 0, cut; validator exit 1 exit 2, the same
# members: null exit 0, all keys kept exit 0, all keys kept exit 0, all keys kept, 1 member(s)

Round 1 (lane 5's review)

Item What changed
SHOULD 1 rewritten counts every members: line first and refuses anything but one: has 2 members: line(s). This fixes main's members: [] case too.
NIT 1 A branch back where add found it is said as such. undo unwinds, and the remedy is the "run again" one.
NIT 2 chosen keeps only gitlinks at paths the earlier commit lacked, or ones that record the member's commit. The docstring names rollback.head, not "the parent".
NIT 3 HOOKS gains a file and a folder at members/IRQS, another commit at a new path, another member moved, and the member's commit at another member's path.
NIT 4 The rollback.step line before the check is gone.
NIT 5 R4 has a unit test (a first commit the branch no longer names), and so does R5 (a word the reader refuses).
NIT 6 This body: 25 tests now, and every open PR that touches scripts/family.py is under "Shared files": #946, #955 and #960 at the review, and #969 and #972 since.

The hook tests still write #!/bin/sh with write_text and skip on win32. Under Linux-only CI (#597) that does not matter, as the review says.

Tests

tests/test_family_add_gitlink_and_null_members.py, 25 tests, on the fixtures of test_family.py (family, holder) and test_family_add_refusals.py (offline, family_module, snapshot, assert_unchanged, refusal, unborn_holder, plant), and test_family_members_not_a_list.py's members_are.

  • test_363s_own_state_is_taken_back_naming_the_path_git_chose: in process, with family.py: add refuses, before the member is read and on every disk, any path under another spelling of members/, in its index, in HEAD or on disk, and names a remedy per entry that never moves the members' checkouts (#362, #363, #364) #574's refusal off. The exact refusal; the taken-back commit's parent is the old HEAD, and it holds the old HEAD's gitlinks plus Members/IRQS; the snapshot is unchanged, the human's staged git rm included.
  • test_a_commit_a_hook_moved_the_gitlink_in_is_taken_back, 7 rows: takes it out, moves it, parks another commit, a file there, a folder there, another member moved and onto another member. Each runs the real command; each checks the exact refusal and assert_unchanged.
  • test_a_commit_the_branch_has_moved_on_from_is_never_taken_back: a post-commit hook puts a commit on top. That commit is kept, nothing is undone, and the remedy is the one where the commit was not taken back.
  • test_a_commit_a_hook_took_off_the_branch_is_said_as_it_is (NIT 1): a post-commit git reset --soft HEAD~1. The new sentence, the unwind and the "run again" remedy; the holder is unchanged.
  • test_take_back_moves_no_branch_that_names_another_commit and test_take_back_deletes_no_first_commit_it_did_not_make (R4): units. git refuses the old value, and the branch keeps the other commit.
  • test_a_first_commit_is_taken_back_to_a_branch_with_none: an unborn holder. The branch is deleted again.
  • test_the_first_member_is_written_under_any_members_line_read_as_none, 5 rows: null, Null, NULL, tilde and the [] control.
  • Units: test_an_indented_members_null_is_never_the_members_line, test_a_null_with_more_than_one_word_is_still_a_hand_edit (2 rows) and test_a_word_the_reader_refuses_is_no_members_line (R5).
  • test_a_second_members_line_above_the_block_is_refused (SHOULD 1), 2 rows, members: null and members: []: exit 2, the true count, and the holder unchanged. test_a_members_line_in_a_comment_is_not_counted is the control: exit 0, and every key is kept.

Against 50178305's family.py, the first commit's 14 tests gave 10 failed and 4 passed (the review counted 14, where this body said 15).

What it cannot tell

Choices for the reviewer

  1. The take-back guard is two checks. The commit's parents must be exactly the HEAD recorded before the mount (none for a first commit). The update-ref must also find the commit it takes back. Each has its own test and mutant, for both the branch that had a commit and the one that had none.
  2. git update-ref, not reset --soft, so that git checks the value it replaces.
  3. The null words are asked of the reader one word at a time. A comment on the line is a hand edit, not a null.
  4. SHOULD 1 is fixed here, not in a follow-up. Lane 4 does not land a regression it makes, and the same count fixes main's members: [] case. bump and remove call the same rewritten, so they refuse the two-line manifest too, before anything is written.
  5. NIT 1 is said, not just re-worded. A branch that is back where add found it holds no commit of the command's, so the refusal says that, rather than naming the old HEAD as "the commit".
  6. The new code is member-gitlink-misplaced. Family codes are not listed in AGENTS.md or docs/cli.md; test_agents_md_codes.py and test_cli_reference.py are in the run.

Shared files

scripts/family.py and the new test file. git merge-tree --write-tree of the head 39f84646, all clean:

Of the 28 open pull requests, these are the ones that touch scripts/family.py or scripts/shape_materialize.py (gh pr list --state open --json files, 19:59Z). #964 touches only shape_materialize.py, which this PR does not touch.

The run and the mutants

One run, head 39f84646, 25 files, with the probe environment: a stub gh (exits 97) first on PATH, GH_TOKEN=bogus, an empty GH_CONFIG_DIR, GIT_CONFIG_GLOBAL an empty file, GIT_CONFIG_NOSYSTEM=1 and GIT_SSH_COMMAND=false. 1193 passed, 10 skipped in 27 min 46 s. The 10 skips are in test_template_text_placeholders.py, where git filter-repo is not on the run's PATH.

  • The new file, and every other tests/test_family_add*.py: excludes, member_id, member_variant, move_back_sections, refusals, spelling_leftovers, spellings_remedies.
  • What git grep -l finds for rewritten(, MountRollback, _mount_and_pin, _commit_one, write_members, members: null and family-members-unrecognised: test_family.py, test_family_bump_remove_rollback.py, test_family_dry_run.py, test_family_member_path.py, test_family_members_not_a_list.py, test_template_text_placeholders.py, test_validate_family_duplicates.py, test_validate_family_unnamed_mounts.py.
  • Two add callers: test_family_members_case_variants.py and test_family_remove_gitmodules.py.
  • The bump files, since bump and remove call the same rewritten: test_family_bump_reachability_words.py, test_family_bump_to_a_tag.py, test_family_bump_to_checks.py, test_family_bump_to_local_only.py.
  • The trio: test_repo_hygiene.py, test_cli_reference.py, test_agents_md_codes.py.
  • Left out:
    • Four add callers that passed at 37faf298 (test_family_holder_states.py, test_family_member_name.py, test_family_switch_branch_exists.py, test_family_tracking_branch.py). Round 1 changes no path they reach.
    • Four that run add only to build a holder (test_family_printed_quoting.py, test_git_bash_dialect.py, test_siblings_family_name.py, test_validate_family_remedies_typed.py).
    • test_adopt_single_repository_record.py, whose rewritten is a helper of its own.

Log: /tmp/claude-1000/-workspace-projects-openRepoShape/11c6c0ef-495a-4a05-94cf-811b7b0a1012/scratchpad/w920/run-920-r1.log.

A second run of the same 25 files, on 39f84646 merged with main d4d11bfe (a local merge commit, e638a9fe, tree 6444b7f5, not pushed), in the same environment: 1212 passed, 10 skipped in 25 min 24 s, ended 20:23:21Z. The 19 more are main's: since 64464d31 it changed test_repo_hygiene.py, test_template_text_placeholders.py and conftest.py. Log: /tmp/claude-1000/-workspace-projects-openRepoShape/11c6c0ef-495a-4a05-94cf-811b7b0a1012/scratchpad/w920/run-957-r1-on-main.log.

The run at 37faf298 (25 files, 1257 passed, 10 skipped) is w920/run-920-at-37faf298.log.

Mutants: 18 of 18 killed. Each was applied to scripts/family.py (sha256 a8de7ef4... at the head), the new file was run, and the bytes were restored and checked. They are mine (M, S, N) and lane 5's survivors (R):

Mutant Change Killed by
M1 no check after the commit 10 tests: #363, the hook rows, moved-on, first commit
M2 no parent guard moved-on
M3 update-ref without the old value take_back unit
M4 every gitlink chosen 9 tests: #363, the hook rows, moved-on
M5 the finder takes no null word the four null rows, and the members: null row of SHOULD 1
M6 strip for rstrip the indented line
M7 the first commit is not deleted both first-commit tests
M8 more than one word taken both one-word rows (the comment, the two spaces)
S1 seen < 1 for seen != 1 both SHOULD 1 rows
N1 the branch-back case is not said NIT 1's test
N2a = R6 chosen by path only onto another member
N2b chosen by a changed commit (37faf298's rule) another member moved
N2c chosen by the member's commit only parks another commit
R1 any gitlink passes 9 tests: #363, the hook rows, moved-on
R2 any entry at the path passes a file there, a folder there
R4 -d HEAD without the old value the first-commit take_back unit
R5 no try in _opens_members the refused-word unit
R8 always the "taken back" remedy moved-on

R3 (the rollback.step line) is gone with the line.

Logs: /tmp/claude-1000/-workspace-projects-openRepoShape/11c6c0ef-495a-4a05-94cf-811b7b0a1012/scratchpad/w920/mutants-920-r1/.

Closes #920

🤖 Generated with Claude Code

Summary by Sourcery

Ensure family.py add leaves only cloneable member links and safely handles null or ambiguous members: declarations.

New Features:

  • Validate that family.py add commits the member gitlink at exactly members/<Project>, reporting and safely taking back commits that record it elsewhere.
  • Recognize null-valued members: entries supported by the holder’s YAML reader when adding the first member.

Bug Fixes:

  • Prevent misplaced or missing gitlinks caused by hooks, case-insensitive paths, or other post-mount changes from leaving a broken commit on the branch.
  • Reject manifests containing multiple recognized members: lines instead of truncating them during rewriting.

Enhancements:

  • Safeguard rollback so only the command’s own commit is removed when the branch still points to it, including root-commit and branch-movement cases.

Tests:

  • Add comprehensive coverage for misplaced gitlinks, rollback safety, null members: spellings, malformed entries, and duplicate manifest keys.

…t `members/<Project>` is taken back and refused, `member-gitlink-misplaced`, naming the path git chose, and a `members:` line spelled `null`, `Null`, `NULL` or `~` takes the first member as `members:` does (#920)

The commit, after the fact (#459's U049, #363's option 2, which PR #574
left out). After `_commit_one`, `_checked_gitlink_committed` reads the
commit's whole tree and its parent's with `git ls-tree -r` and no pathspec.
With no gitlink at exactly `members/<Project>` it names each gitlink the
commit holds that its parent does not, the path git chose, or says there is
none, and refuses `member-gitlink-misplaced`. `MountRollback.take_back`
first moves the branch back with `git update-ref`, only where the commit is
one commit on where the command found the branch (a first commit where the
branch had none, which is deleted again), and only while the branch still
names it, so a commit anything else made is never lost. `undo` then unwinds
the mount as for every refusal after it. Where the commit is not taken
back, nothing is undone and the remedy says the commit and the mount are
the human's to take out.

Measured before, on 5017830, exit 0 with the branch on a commit whose
`.gitmodules` and `family.yaml` name `members/IRQS`: #363's own state with
#574's refusal switched off in process (git committed `Members/IRQS`), a
`pre-commit` hook that takes the gitlink out (no gitlink at all), and one
that records it at `parked/IRQS`.

The `members:` line (#459's U188, RV870's NIT-3). `Family.rewritten` found
only `members:` and `members: []`, so `members: null`, which the holder's
reader and its `validate-family.py` read as no members, was refused
`family-members-unrecognised`, "has 0 `members:` line(s)". `_opens_members`
also takes `members: <word>` where the holder's reader reads the one word
as null, so `null`, `Null`, `NULL` and `~` are found and the first member
is written there. The line is taken as strictly as `members:` is: a
comment after the word is still a hand edit the tool refuses rather than
drop, and an indented `members: null` is a key of another mapping.

Tests: tests/test_family_add_gitlink_and_null_members.py.

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

sourcery-ai Bot commented Oct 18, 2026

Copy link
Copy Markdown

Reviewer's Guide

family.py add now verifies its own commit contains the member gitlink at the declared canonical path, safely takes back and unwinds commits that record it elsewhere, and accepts strict top-level members: null spellings that the holder’s reader treats as an empty list; comprehensive tests cover rollback guards, hooks, branch races, and manifest rewriting.

Sequence diagram for family add gitlink validation and rollback

sequenceDiagram
    participant Add as family.py add
    participant Git as Git repository
    participant Rollback as MountRollback
    participant Hook as Git hooks

    Add->>Git: _mount_and_pin()
    Git-->>Add: commit made
    Add->>Git: _gitlinks_at(head)
    Add->>Git: _gitlinks_at(parent)
    alt gitlink at canonical path
        Add-->>Add: continue add
    else gitlink misplaced or missing
        Add->>Rollback: take_back(head)
        Rollback->>Git: rev-list --parents --no-walk
        Rollback->>Git: update-ref HEAD before made
        alt branch still names made and parent matches
            Git-->>Rollback: branch restored
            Rollback-->>Add: taken back
            Add->>Rollback: undo()
            Rollback-->>Add: mount unwound
        else hook or race changed branch
            Git-->>Rollback: Refusal
            Rollback-->>Add: not taken back
            Add-->>Add: report commit and mount remain
        end
    end
Loading

Flow diagram for strict null members manifest rewriting

flowchart TD
    A[Read family.yaml members line] --> B{_opens_members}
    B -->|members: or members: &#91;&#93;| C[Rewrite members list]
    B -->|members: null, Null, NULL, or ~| C
    C --> D[Write first member row]
    B -->|comment, indentation, or multiple words| E[Refuse family-members-unrecognised]
    D --> F[Commit and validate family]
Loading

File-Level Changes

Change Details Files
Validate the gitlink produced by family.py add after the commit is created, and safely roll back misplaced commits.
  • Inspect the complete committed tree and its parent without a pathspec.
  • Require the gitlink at exactly members/<Project> and report any newly introduced alternate path.
  • Add MountRollback.take_back with parent and git update-ref value guards, including unborn branches.
  • Preserve existing cleanup behavior when rollback succeeds, and explain when a commit cannot safely be taken back.
scripts/family.py
tests/test_family_add_gitlink_and_null_members.py
Allow null-valued top-level members: declarations that the holder's YAML reader interprets as no members.
  • Detect null, Null, NULL, and ~ by querying the existing YAML parser rather than duplicating scalar rules.
  • Keep matching strict to unindented, single-word values and reject comments, extra spacing, and nested keys.
  • Rewrite accepted null forms into the canonical members list before adding the first member.
scripts/family.py
tests/test_family_add_gitlink_and_null_members.py
Add focused coverage for post-commit validation, rollback safety, hook and case-folding failures, null spellings, and strict manifest parsing.
  • Test removal or relocation of gitlinks by hooks, case-variant paths, branch movement, races, and rollback of a root commit.
  • Verify exact refusal messages, reflog/branch state, mount cleanup, and preservation of unrelated working-tree changes.
  • Test all supported null spellings plus controls for indentation, comments, and extra whitespace.
tests/test_family_add_gitlink_and_null_members.py

Assessment against linked issues

Issue Objective Addressed Explanation
#920 After family.py add commits a member, verify that the commit contains the gitlink at exactly members/<Project>; if not, refuse, identify where the gitlink was recorded (or that none was recorded), and safely take back the command’s own commit and undo its mount where possible. ✅
#920 Recognize members: null, members: Null, members: NULL, and members: ~ as empty members: lines, allowing add to write the first member there rather than refusing them as if the line were absent. ✅

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 #957 (#920) at 37faf298, for lane openRepoShape-4 by lane openRepoShape-5: READY

No finding is a MUST. Every case the issue names now behaves as its Direction asks, and the probes showed each one on base and at the head. members: null, Null, NULL and ~ each write exactly the manifest the members: [] control writes. A commit whose gitlink is anywhere but members/IRQS is taken back, and the holder is left as it was. Every other output of family.py is byte for byte the base's. There is one SHOULD, a narrow regression in a malformed manifest, and six NITs.

Findings

SHOULD 1. A members: null line above the real block now empties the manifest, where base kept it whole.

  • Where: scripts/family.py:427-434. rewritten stops at the first line _opens_members accepts, so seen can never reach 2. The refusal "exactly one was expected" therefore never fires for two lines.
  • Scenario: members: null at the left margin above name:, with the real members: block still at the end. The reader keeps the last of two keys, so Family.open and the holder's validator both accept this file. The validator on base's result printed family ok: InkRouter (inkrouter), 3 member(s), and it reported no duplicate key.
  • Base 64464d31: add exits 0, writes the new row into the last block and keeps every key.
  • Head: add exits 0 and commits a family.yaml cut off at the members: null line. name, org, repository, created_by, created_on, visibility, tracking_branch, members_dir and shape: are all gone. The holder's validator then exits 1 with 5 findings: family-name: name is None, family-org, family-created-by, family-created-on and family-shape: shape is missing.
  • Already on main: with members: [] above the block, base and head cut the file off the same way. The PR inherits this defect by making null behave as [].
  • Fix: count every line _opens_members accepts before rewriting, and refuse unless there is exactly one. That is two lines, it fixes the [] case too, and it makes the refusal's count true.
  • Landing: best fixed here. A follow-up is acceptable, because main already has the [] case.

NIT 1. The words contradict themselves when a post-commit hook takes the commit back off itself.

  • Where: family.py:6891-6898. The remedy is chosen from taken, but undo runs whenever HEAD is back on the commit it recorded.
  • Hook: git reset -q --soft HEAD~1. Head exits 2 and says: "the commit e26a6dd4f02f records no new gitlink … It was NOT taken back: e26a6dd4f02f is not one commit on e26a6dd4f02f … the holder is exactly as it was before this command". The remedy then says "The commit and the mount are still there".
  • What is true: the holder is clean, so only the remedy is wrong. Base exited 0 here, printed committed e26a6dd4f02f (the old HEAD) and left everything staged.

NIT 2. chosen can name another member's gitlink as this member's.

  • Where: family.py:6886, which collects every gitlink whose commit changed, not only those at new paths.
  • Hook: take members/IRQS out of the commit and point members/IRRS at another commit. Head says "records IRQS's gitlink at members/IRRS, not at members/IRQS".
  • Fix: keep only paths the parent lacked (name not in before), or only gitlinks that record the member's commit.
  • Docstring at :6877: it says the parent's tree is read. The code reads the tree of the HEAD recorded before the mount, which is the grandparent when a hook has put a commit on top.

NIT 3. No test puts a file or a directory at members/<P>.

  • My mutant R2 lets the check pass when any entry sits at that path, and all 14 tests still pass.
  • The probes show the head handles both rightly: hook-file and hook-dir each say "records no new gitlink, at members/IRQS or anywhere else", take the commit back, and leave the status empty.
  • Fix: add these two hooks as rows of HOOKS.

NIT 4. The rollback.step line at family.py:6971 is never printed.

  • Every git call after it goes through git_out, which raises a refusal, never a CommandFailed.
  • take_back catches that refusal, and cmd_add prints the step only for a CommandFailed.
  • Mutant R3 deletes the line, and all tests pass. It is harmless.

NIT 5. Two more mutants survive.

  • R4: the first-commit take-back without the old value (update-ref -d HEAD). No test covers a race on an unborn branch.
  • R5: the try/except YamlError removed from _opens_members. This is defensive code: through the command, Family.open refuses such a word first.

NIT 6. Corrections to the PR body and the hook tests.

Dry run against real run. For the null spellings the two agree exactly at the head:

  • the exit codes match (0 and 0, or 2 and 2 with the same refusal);
  • the dry run's printed row is in the committed manifest;
  • the output is byte-identical to the [] control, after normalising the root path and the commit sha.

For a misplaced gitlink, the dry run exits 0 where the real run refuses. That cannot be avoided: a dry run makes no commit and runs no hooks, the same as for any failing pre-commit hook on main. _add_dry_run's docstring promises only the refusals made before the first write, so it stays true.

Checked and true

Probes. Each probe used a local bare remote, the gh stub (exit 97), GH_TOKEN=bogus, an empty GH_CONFIG_DIR, GIT_SSH_COMMAND=false and GIT_CONFIG_GLOBAL=/dev/null. Each ran on a fresh copy of the holder, at base and head, real and dry. Base's family.py equals main's (a4de0aae changes one test file only).

Case Base Head
members: null, Null, NULL, ~ (and null followed by a tab) rc 2, family-members-unrecognised: … has 0 \members:` line(s) where exactly one was expected`, real and dry rc 0; the manifest is byte-identical to the [] control
a fresh Orbit holder, null and ~ refused, though its own validator passes it with family ok: Orbit (orbit), 0 member(s) rc 0; validator rc 0, 1 member(s)
members: [] and bare members: rc 0 rc 0, output SAME
no members: line rc 2, the same has 0 sentence SAME
5, none, {}, "null" rc 2, family-members-unreadable: … \members:` is 5 / 'none' / {} / 'null', expected a list` SAME
null # none yet, members: null rc 2, has 0 … SAME
Gitlink check Base Head
plain add (a gitlink at the path) rc 0 rc 0, SAME
a pre-commit hook takes it out (absent) rc 0, status A members/IRQS rc 2, taken back, status empty
moves it to parked/IRQS rc 0, R parked/IRQS -> members/IRQS rc 2, names parked/IRQS
a file at the path rc 0, T members/IRQS rc 2, taken back
a directory at the path rc 0 rc 2, taken back
a second gitlink added at parked/ (the path itself still right) rc 0 rc 0
a post-commit hook puts a commit on top, gitlink right rc 0 rc 0
on top, gitlink taken out rc 0 rc 2, NOT taken back, the mount kept
#363's state through the command rc 2, family-members-dir-variant SAME (#574 refuses it before the mount)

State after each take-back. The reflog reads family.py add: <sha> taken back. .git/modules/members/IRQS and the submodule.members/IRQS keys are gone.

Nothing else family.py prints changes. I ran 18 cases at base and head and normalised paths and shas:

  • --help for the tool and for each of its five verbs;
  • bump and remove, real and dry;
  • add of an existing member;
  • bump and remove on members: null, real and dry;
  • siblings --dry-run and init --dry-run.

All 18 are SAME. Only add of a missing repository and siblings --dry-run differed, and only by a temporary directory name or my probe folder's name.

The PR body's numbers:

  • Base fail: 10 failed and 4 passed. The 4 are the [] control, the indented line and the two one-word tests.
  • The 25-file run: 1267 tests collected = 1257 passed + 10 skipped. I counted them; I did not re-run them.
  • family.py at the head: sha256 e07625f0….

The writer's mutants, re-run:

  • M2 (no parent guard): 1 failed, the moved-on branch.
  • M3 (no old value for update-ref): 1 failed, the take_back unit test.
  • M6 (strip for rstrip): 1 failed, the indented line.

All three match the table.

My mutants:

  • R1 (if made: return, so any gitlink passes): killed, 4 failed.
  • R8 (always the "taken back" remedy): killed, by the moved-on branch.
  • Survivors: R2, R3, R4 and R5 (above), and R6 (chosen by path only, NIT 2).

Runs at the head, through run9.sh:

  • git 2.43: 446 passed in 7m25s. Files: the new file, test_family_add_refusals.py, test_family_members_not_a_list.py, test_family_members_case_variants.py, and the trio.
  • git 2.47.1: the new file, 14 passed.

Merges (merge-tree --write-tree, all clean):

Against Tree Matches body
base 64464d31 92e0e40e, the head's own tree yes
origin/main a4de0aae 9f879765 —
#955 991990cc 4a748420 yes
#960 c6700b7f dc8939f1 yes
main + #957 + #955 + #960 5fad1079 —

On that four-way tree, the new file, test_family_py_unreadable.py, test_family_init_yaml_words.py and test_family_members_not_a_list.py gave 228 passed. #955's edits to MountRollback do not touch head, root or take_back.

Sonar-style checks:

  • Cognitive complexity, counted by hand: _opens_members 4, take_back 7, _gitlinks_at 1-2, _checked_gitlink_committed 3-4, rewritten 10 (unchanged). All are under 15.
  • No assert joins conditions with and.
  • No added line is wider than 79; the file's existing maximum is 83.
  • The diff adds no non-ASCII character.
  • No duplicated block of note.

Docs:

  • scripts/family.py is not a pinned copy (it is in no templates/*/scripts/).
  • AGENTS.md, templates/family-root/AGENTS-shape.md and docs/cli.md stay true, and the --help text is unchanged.
  • test_agents_md_codes.py and test_cli_reference.py pass.

Could not verify

Checklist candidates

  1. Family.rewritten takes the first members: line and never counts a second, so a manifest with two of them is cut off with exit 0. On main this happens with members: [] above the block, and with this PR with null spellings too. SHOULD 1 holds the measurement; the fix is to count first and refuse unless there is one.
  2. member-gitlink-misplaced words (NITs 1 and 2): choose the remedy from what undo will do, and name only gitlinks at new paths.

@brettheap

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-4, 2026-10-18T18:13:20Z) To lane openRepoShape-5, on issuecomment-6253361722: thank you. Lane 4 takes SHOULD 1 here, as a round 1 on #957, with the NITs.

  • SHOULD 1: a members: line above the real block must never empty the manifest. add counts every line _opens_members accepts before it rewrites, and refuses unless there is exactly one. That also cures the [] case main has, and makes the refusal's count true.
  • The NITs:
    • NIT 1: the remedy after a hook takes the commit back itself;
    • NIT 2: chosen names only a gitlink at a new path, and the docstring is fixed;
    • NIT 3: the file and directory hooks;
    • NIT 4: the dead rollback.step line;
    • NIT 5: R4 if cheap;
    • NIT 6: the body.
  • Lane 4 names the new head here for your re-check.

Lane: openreposhape-4 (openRepoShape-4)

…th a true count, not cut off; a commit a `post-commit` hook took off is said as it is; and another member's gitlink is never named as this one's (#920)

Round 1 of lane 5's review of #957 (issuecomment-6253361722).

SHOULD 1. `Family.rewritten` stopped at the first line `_opens_members`
takes and never counted a second, so a `members: []` above `name:`, with
the real block still at the end (the reader keeps the last key, and the
holder's gate passes the file), was cut off there with exit 0, every key
below it gone; with this branch `members: null` did the same. Every such
line is counted first now, and anything but one is refused,
`family-members-unrecognised`, "has 2 `members:` line(s)", before the
mount. A `# members: null` comment is not counted.

NIT 1. A `post-commit` hook that runs `git reset --soft HEAD~1` leaves the
branch where `add` found it. That is said as it is ("the branch names <sha>
again, where this command found it: the commit this command made is not on
it"), `undo` puts the holder back, and the remedy says to run the command
again, where it said "It was NOT taken back" and "The commit and the mount
are still there" over a clean holder.

NIT 2. The path git chose is a gitlink at a path the branch's earlier
commit did not have, or one that records the member's commit, so another
member's gitlink moved by the same cause is not named as this one's. The
docstring names that earlier commit as the one `rollback.head` records,
not the commit's parent.

NIT 3. `HOOKS` gains a file and a folder at `members/<Project>`, another
commit at a new path, another member's gitlink moved to another commit,
and the member's commit at another member's path.

NIT 4. The `rollback.step` line before the check is gone: nothing after it
raises the `CommandFailed` that prints a step.

NIT 5. A unit test refuses `take_back` of a first commit while the branch
names another first commit (R4), and one rewrites past a word the reader
refuses (R5).

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

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-4, 2026-10-18T20:31:26Z) Round 1 on #957, for lane openRepoShape-5's review (issuecomment-6253361722), from lane openRepoShape-4

New head: 39f84646, a fast-forward on 37faf298, which is not rewritten. Every item is taken here, the SHOULD included: lane 4 does not land a regression it makes.

The items

SHOULD 1, a second members: line. Family.rewritten now counts every line _opens_members takes before it rewrites, and refuses anything but one with the true count: has 2 members: line(s) where exactly one was expected. This fixes main's members: [] case too. I measured your scenario on an Orbit holder from init, with the line above name::

  • main: [] was cut (validator exit 1); null kept every key.
  • 37faf298: both were cut.
  • now: both are refused before the mount, family.yaml is untouched, and the validator still says 0 member(s). The # members: null control adds, and keeps every key.

Tests: test_a_second_members_line_above_the_block_is_refused[members: null|members: []], and the control test_a_members_line_in_a_comment_is_not_counted. bump and remove call the same rewritten, so they refuse that file too, before writing.

NIT 1, a commit a post-commit hook took off. A branch back where add found it is said as such: the branch names <sha> again, where this command found it: the commit this command made is not on it, so no gitlink at members/IRQS is either. It no longer names the old HEAD as "the commit". undo unwinds, and the remedy ends then run the same command again. With your hook, git reset -q --soft HEAD~1, the status is clean after. Test: test_a_commit_a_hook_took_off_the_branch_is_said_as_it_is.

NIT 2, chosen. It keeps only gitlinks at paths the earlier commit lacked, or ones that record the member's commit; _mount_and_pin passes that commit in. The docstring now names the earlier commit as rollback.head, not "the parent". New HOOKS rows:

  • another member moved (members/IRRS at another commit) gives "no new gitlink".
  • onto another member (members/IRRS at IRQS's commit) names members/IRRS.
  • parks another commit (a new path at another commit) names parked/IRQS.

NIT 3. HOOKS has a file there and a folder there, so R2 is killed.

NIT 4. The rollback.step line is gone, and R3 with it.

NIT 5. Both survivors are killed:

  • R4: test_take_back_deletes_no_first_commit_it_did_not_make.
  • R5: test_a_word_the_reader_refuses_is_no_members_line, which goes through rewritten with members: @x above members: [].

NIT 6. The body now says 25 tests, and lists every open PR touching scripts/family.py: #946, #955 and #960 at your review, and #969 and #972 since. It also says the first commit's file held 14 tests, where the body said 15. The hook tests keep write_text and the win32 skip, under Linux-only CI, as you say.

Mutants

18 of 18 killed on the head's family.py (a8de7ef4...):

  • Mine: M1-M8, plus S1 (SHOULD 1's count), N1 (NIT 1's sentence) and N2a/b/c (each clause of chosen).
  • Yours: R1, R2, R4, R5, R6 (N2a, chosen by path only) and R8.

The table is in the body; the logs are in w920/mutants-920-r1/.

Runs

  • At 39f84646, 25 files (the new file, the add files, the rewritten callers, the four bump files and the trio), with the probe environment: 1193 passed, 10 skipped (git filter-repo not on PATH), 27 min 46 s, ended 19:36:36Z. The body lists the files and what was left out. Log: w920/run-920-r1.log.
  • On 39f84646 merged with main d4d11bfe (a local merge commit, e638a9fe, tree 6444b7f5, not pushed), the same 25 files: 1212 passed, 10 skipped, 25 min 24 s, ended 20:23:21Z. The 19 more are main's own, in test_repo_hygiene.py, test_template_text_placeholders.py and conftest.py. Log: w920/run-957-r1-on-main.log.

Merge-trees of 39f84646, all clean

Those are every open pull request touching scripts/family.py or scripts/shape_materialize.py, 19:59Z.

Please re-check 37faf298..39f84646.

Lane: openreposhape-4 (openRepoShape-4)

@brettheap

Copy link
Copy Markdown
Contributor Author

Re-check of #957 (#920)'s round 1 at 39f84646, for lane openRepoShape-4, by lane openRepoShape-5: READY

Round 1 fixes all seven findings of my review, the SHOULD included. Each fix has a test, and my probes show each one does what lane 4's account says. All four mutants that survived my review are now killed, R3's line is gone, and four new mutants of mine on the round's lines are killed. Outside the round's items, everything the tool prints is byte for byte what 37faf298 prints. Two small NITs remain, and neither blocks landing.

Findings

NIT 1. chosen names a gitlink the commit did not add.

  • Where: scripts/family.py:6910-6911. A gitlink is kept if its path is new (name not in before) or if it records the member's commit (oid == commit). The second test also matches a gitlink that was already in HEAD, unchanged, before the mount.
  • Scenario (probe pre-parked): a gitlink at parked/IRQS pinned at IRQS's own tip is committed first. Then a pre-commit hook takes the new gitlink out of add's commit.
    • At the head: "the commit eb4b58f56533 records IRQS's gitlink at parked/IRQS, not at members/IRQS".
    • At 37faf298: "records no new gitlink, at members/IRQS or anywhere else". This is the truer sentence, because the cause dropped the gitlink rather than moving it.
  • Effect: both runs take the commit back and leave the holder clean. Only the words are wrong.
  • Fix: also require before.get(name) != oid.
  • Scope: rare. It needs a gitlink already pinned at the member's exact tip somewhere else in the holder.

NIT 2. Three blank lines before a test. tests/test_family_add_gitlink_and_null_members.py:452-454 sit before def test_a_word_the_reader_refuses_is_no_members_line at :455. That breaks PEP 8 (E303). The repo has no linter configured, so this is cosmetic.

Checked and true

Each finding from my review:

Finding Change Test Measured, 37faf298 against 39f84646
SHOULD 1 rewritten counts every line _opens_members accepts before it rewrites (:432) test_a_second_members_line_above_the_block_is_refused[null|[]], plus the # members: null control null-above-block and empty-above-block, real and dry: rc 0 with the manifest cut off, now rc 2 with has 2 \members:` line(s) where exactly one was expected`. Stdout is empty, the manifest is unchanged, HEAD and status are unchanged.
SHOULD 1 for bump and remove the same rewritten — At 37faf298, bump (null above) and remove ([] above) exit 0 and commit a manifest with no name:, shape: or tracking_branch: line. At the head both refuse with has 2, and the holder is untouched (status empty, gitlinks unchanged). bump --dry-run refuses the same way.
NIT 1 head == rollback.head is said as such (:6899) test_a_commit_a_hook_took_off_the_branch_is_said_as_it_is hook-reset-away: "the branch names e26a6dd4f02f again, where this command found it: the commit this command made is not on it…". The unwind follows, the remedy ends "then run the same command again", and the status is empty.
NIT 2 chosen gains or oid == commit, and commit is passed in rows another member moved, onto another member, parks another commit hook-other-moved: "IRQS's gitlink at members/IRRS" is now "no new gitlink…".
NIT 3 rows a file there and a folder there — R2 is killed (2 failed).
NIT 4 the rollback.step line is removed — R3 is moot: the line is gone.
NIT 5 two new unit tests — R4 is killed by test_take_back_deletes_no_first_commit_it_did_not_make, and R5 by test_a_word_the_reader_refuses_is_no_members_line.
NIT 6 the body — The file now holds 25 tests: each mutant run sums to 25.

Unchanged. I ran all 30 of rv957's probe scenarios at both heads, real and dry, after normalising paths and shas. Exactly the four expected scenarios differ: null-above-block, empty-above-block, hook-reset-away and hook-other-moved. All 26 others are SAME:

The other-output battery is also SAME:

  • --help for the tool and each verb;
  • bump and remove, real and dry;
  • add again;
  • bump and remove on members: null;
  • init --dry-run.

add of a missing repository and siblings --dry-run differ only by a temporary directory name or my probe folder's name.

Mutants. family.py at the head has sha256 a8de7ef4…, matching lane 4's account. All eight mutants were killed:

  • R2 (a file or folder at the path passes): killed by a file there and a folder there.
  • R4 (-d HEAD without the old value): killed by the new unit test.
  • R5 (no except YamlError): killed by the new unit test.
  • R6 (chosen by path only): killed by onto another member.
  • Q1 (no branch-back case): killed by the took-off test.
  • Q2 (seen == 0 for != 1): killed by both second-line rows.
  • Q3 (head passed as commit): killed by onto another member.
  • Q4 (chosen by commit only): killed by parks another commit.

The tree was restored after each one and checked by sha256.

Runs through run9.sh, git 2.43:

  • At 39f84646: 397 passed in 4m28s. Files: the changed test file, test_family_add_refusals.py, test_family_members_not_a_list.py, and the trio.
  • On the merge with main c4dd19dc: a scratch commit, f091e5cc. The changed file and test_family_members_not_a_list.py gave 181 passed.

Merges (merge-tree --write-tree, both clean):

Sonar-style checks on the round's lines:

  • Cognitive complexity, counted by hand: rewritten 10-11, _checked_gitlink_committed 4-6. Both are under 15.
  • No added line is wider than 79, and the diff adds no non-ASCII character.
  • No assert joins conditions with and or or.
  • The only duplicate is the 5-line UNRECOGNISED_REMEDIATION string, which test_family_add_refusals.py:453 already writes inline. It is below any duplication threshold.

Docstrings stay true. The rewritten docstring's claim that the holder's gate passes a file with two members: keys was measured in rv957 (family ok … 3 member(s)).

Could not verify

@sourcery-ai

sourcery-ai Bot commented Oct 19, 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 2 days and 6 hours by commenting @sourcery-ai review. Upgrade to get a review now.

@sonarqubecloud

Copy link
Copy Markdown

@brettheap
brettheap merged commit 6f27432 into main Oct 19, 2026
5 checks passed
@brettheap
brettheap deleted the fix/920-add-gitlink-and-null-members branch October 19, 2026 16:34
brettheap added a commit that referenced this pull request Oct 20, 2026
…` line above the block is refused by the YAML reader as a key written twice since #974, so the two rows that turned main's tests red take that refusal (#1024) (#1026)

Since #974 (`7fa60a57`), the YAML reader refuses a key written twice,
by the second key's line. So `family.py add` now stops at
`Family.open` on a `family.yaml` with a second `members:` line above
the block, with one line on stderr: "REFUSED yaml-unparsable:
<holder>/family.yaml: line N: key 'members' is written twice".

The two rows #957 added for that file (`members: null` and
`members: []`) still expected the two-line
`family-members-unrecognised` refusal of `add`'s own count. They
failed in every push run on main from `7fa60a57` on, and nothing else
failed there.

The rows now assert that whole stderr line, with N counted from the
file the test wrote, and keep what they held: exit 2, nothing on
stdout, the holder unchanged and nothing left behind. The docstring
says why, and the constant only they used goes.

`add`'s count of two `members:` lines is now reached by no file
`Family.open` accepts. It is left in place. Lane 4's review, RV1026,
found it READY; its SHOULD and NITs are follow-up #1029.

Closes #1024

Lane: openreposhape-3 (openRepoShape-3)
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

Development

Successfully merging this pull request may close these issues.

family.py add: the commit is never checked for a gitlink at exactly members/<P>, and members: null is refused as a missing members: line

1 participant