Repository navigation
family.py: add asks every refusal of the holder and of a throwaway clone of the member before it mounts anything, and unwinds and names whatever fails after the mount, so a refused add (a leg, a short or unknown --at, an empty member) no longer leaves a staged gitlink and .gitmodules section that the next add commits under its own title (#203) - #236
Merged
Merged
Conversation
…nwinds any failure after the mount, so a refused add (a leg, a short or unknown `--at`) no longer leaves a staged gitlink and `.gitmodules` section for the next add to commit CHECK BEFORE MOUNT. Every refusal `add` makes is now asked before the holder is touched: the holder's own (`_checked_add_target`: already present, path taken, cached object store, a `members:` block `write_members` would refuse, `--at`'s shape) and the member's (`_probe_member`: a bare clone in a temporary directory outside the holder, `--at` or the tip peeled to a commit, and `project.yaml` read from the member's objects by the same `_read_member_project` the mount is read with). New refusals for what used to fail inside the mount: `member-at-unknown`, `member-no-tip`. UNWIND AFTER IT. `MountRollback` records, before `git submodule add`, the gitlink's and both files' index entries, `.gitmodules` and `family.yaml` bytes, the `submodule.<path>` keys in `.git/config`, and which of the two directories (the mount, and its object store as `git rev-parse --git-path` names it, which is right for a linked-worktree holder too) do not exist yet. Any refusal, failed git command or other exception after the mount puts all of it back, never over a commit that landed, and the refusal names what was restored. A failed pin write is a refusal (`family-manifest-unwritable`), not a traceback. The happy path is unchanged: the mount is still made by `git submodule add` and left on its branch. REMAINING: the new tests/test_family_add_refusals.py (leg refused with the holder byte-identical; bad `--at`; a second add after a refused one commits only its own entry; injected pin-write and commit failures unwound; happy path unchanged), then the targeted run with test_shape_doctor.py and test_cli_reference.py. No `--help` text changed, so docs/cli.md is untouched. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…it was and every failure between the mount and the commit is unwound and named, and `member-not-a-project` names the legs of the root a `-spec` or `-code` suffix came off instead of `IRRS-spec-spec`, finishing the WIP (#203) WHAT THE TESTS PIN. tests/test_family_add_refusals.py, 21 tests, members faked with local bare repositories and a `gh` on PATH that logs and exits 97. After every refused `add` the holder is asked the same question: is it exactly as it was? HEAD, the index, `git status --porcelain --ignored`, the local git config, `.gitmodules` and `family.yaml` byte for byte, `members/` and `.git/modules/`, with the throwaway clone deleted and no `gh` call. Refused BEFORE the mount: a leg (both suffixes), a repository with no `project.yaml`, a `project.yaml` that is not UTF-8 (named `<repository> @ <commit>:project.yaml`, never the deleted clone's path), a short `--at`, an `--at` no ref of the member reaches (forty zeros, and the spec leg's tip), an empty member (`member-no-tip`), a member that cannot be read, and a `members:` block the pin write would refuse. For the NEXT command: a good `add` after a refused leg commits one gitlink and one new `.gitmodules` section, its own; the corrected `--at` runs first time. Unwound AFTER the mount: a read-only `family.yaml` and a full disk (`family-manifest-unwritable`), a refusing pre-commit hook, a member that loses `project.yaml` between the probe and the mount, the first member of an empty family (`.gitmodules` removed, as there was none), a holder that is a LINKED WORKTREE (the store under `.git/worktrees/<name>/modules/` removed, and the same command then runs), an unwind step that fails (named with git's reason, never claimed complete), and a failure after the commit landed (nothing undone, the commit stands and validates). The happy path: one commit of three paths, the mount on its branch at the tip, the same lines plus the new `checking` line. `add` has no `--dry-run`. Every `REFUSED <code>` refusal's two lines are asserted exactly, and the two loud command-failure blocks line by line (the unreadable member's command by its prefix, as the rest names a temporary clone). WHY THE SENTENCE CHANGED. `member-not-a-project` told `--member InkRouter/IRRS-spec` that adding `IRRS-spec-spec` or `IRRS-spec-code` is never right: names no naming rule makes and no repository is. `LEG_SUFFIX_RE` strips the `-spec`/`-code` the naming policy's leg pattern ends in, so the sentence names `IRRS-spec` and `IRRS-code` and the root `IRRS` they belong to; a name with no such suffix keeps the sentence it always had. MUTATION CHECK, each mutant applied alone to `scripts/family.py` in a scratch worktree and only this file run (unmutated control: 21 passed). The member checks moved back after `git submodule add`: 9 tests fail (both legs, the no-manifest, unreadable-manifest, both unknown-`--at`, no-tip, unreadable-member and moved-on tests). The unwind skipped: 7 (every test whose failure comes after the mount). An unwind that reports what it would restore and restores nothing: 6, on the holder's state after the identical sentence matched. `member-at-unknown` dropped: both unknown-`--at` cases. The probe's file name ignored: the unreadable-manifest test. The module store assumed at `.git/modules/<path>`: the linked-worktree test. The sentence change reverted: both leg cases. The early `members:` check dropped: the hand-edited-manifest test. The landed-commit guard dropped: the after-commit test. A partial unwind reported as complete: the failing-step test. The file-name, module-store, landed-guard and partial-unwind mutants survived the 17 tests the WIP's finisher wrote; the four tests that catch them are added here. Targeted run over these bytes: tests/test_family.py, tests/test_family_add_refusals.py, tests/test_shape_doctor.py and tests/test_cli_reference.py, 291 passed. No `--help` text changed, so docs/cli.md is untouched. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 3 days by commenting |
Contributor
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's Guide
Sequence diagram for preflighted family add and rollbacksequenceDiagram
actor User
participant Family as family.py add
participant Holder as Family holder
participant Probe as Throwaway bare clone
participant Git as Git submodule
User->>Family: cmd_add(args)
Family->>Holder: _checked_add_target(family, project, path, at)
Holder-->>Family: checked target and --at
Family->>Probe: _probe_member(url, protocol, repository, project, at)
Probe->>Probe: _probe_commit(clone, repository, at)
Probe->>Probe: _probe_manifest(clone, commit, scratch, project, repository)
Probe-->>Family: commit
Family->>Git: _mount_and_pin(...)
Git->>Holder: git submodule add
Family->>Holder: write_members(rows)
Family->>Holder: _commit_one(...)
Holder-->>Family: new HEAD
Family-->>User: successful pin
alt refusal before mount
Probe-->>Family: Refusal
Family-->>User: named refusal, holder unchanged
else failure after mount
Family->>Holder: MountRollback.undo()
Holder-->>Family: restore gitlink, files, config, module store
Family-->>User: failure and cleanup result
end
State diagram for family add lifecyclestateDiagram-v2
[*] --> HolderCheck
HolderCheck --> MemberProbe: holder accepted
HolderCheck --> Refused: target, pin, manifest, or --at refusal
MemberProbe --> Mount: commit and project.yaml accepted
MemberProbe --> Refused: member-at-unknown, member-no-tip, or member-not-a-project
Mount --> PinAndCommit: submodule mounted
PinAndCommit --> Completed: pin and commit succeed
PinAndCommit --> Rollback: post-mount failure
Rollback --> Refused: holder restored or cleanup reported
Completed --> [*]
Refused --> [*]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
…wn to be unwound and re-raised, a registration the holder already had is put back rather than dropped, a holder with no commit yet gets its root commit again, an `--at` naming a tree or a blob is refused as one, and an unwind half-stopped by a held `index.lock` says which half of `.gitmodules` is back (#203) The review of d25dd4d (MERGEABLE WITH NITS) found the code right and five of its branches untested, plus three small defects. This answers its S1, N1, N2, N6 and N3. S1. `cmd_add` unwinds any exception after the mount, not only a refusal or a failed git command, and re-raises it. No test reached that branch: the review's mutant R2, which deletes it, left all 21 tests green. A new in-process test patches `tree_digest` (it runs after the mount, before the pin is written) to raise `KeyboardInterrupt`, and again `ValueError`. Each asserts that the SAME exception object leaves `main`, that stderr is the `Nothing was added: ...` sentence, that `assert_unchanged` holds and that the throwaway clone's temporary directory is empty. R2 now fails both. N1. One test for each of the four other surviving mutants: - R1 (the holder's `submodule.<path>` keys matched by prefix): the holder already registers `members/IRQS.v2` and `members/IRQSX`. Under R1 the unwind writes a second copy of the `.v2` url back. - R6 (the registration the holder had before the mount not re-added): a plain `git rm` of a member keeps its `submodule.<path>` url and active keys in `.git/config`. Once its store is deleted, `add` runs and `git submodule add` rewrites that url in place, so the unwind must put the old keys back. Writing this test showed the sentence said "dropped" for a registration it had in fact restored, so it now says "restored in the holder's git config" when one existed. - R5 (a symlinked `project.yaml` judged by the probe instead of left to the mount): a member whose manifest is a link, built in its index, is added. Under R5 the probe reads the link's text as YAML and refuses it. Skipped on Windows, where a tracked symlink checks out as a plain file. - R7 (`member-git-dir-cached` looking only in `.git/modules/<path>`): in a linked-worktree holder, an add and a remove leave the store under `.git/worktrees/<n>/modules/`. The next add must be refused before anything is read or mounted, naming that path. Under R7 it runs into git's own refusal after the mount instead. `assert_unchanged` now compares the member's registration with the one `before` held, by section. A substring check would fail both new neighbours, and an absolute "none" would fail a holder that had one. N2. `MountRollback` read HEAD with a bare `git rev-parse HEAD`, so a holder with no commit yet (`git init` plus a hand-written `family.yaml`) was refused `git-failed` where main gave it its root commit. HEAD is now read with `-q --verify`, and None means no commit yet. `undo` already treats an unreadable HEAD as "nothing landed", and any HEAD at all as the commit having landed. Two tests: the add makes the root commit, and a failed pin write leaves the holder unborn, `.gitmodules`, `members/` and `.git/modules/` all absent again. N6. An `--at` that is a tree's or a blob's sha in the member was refused as "not a commit any branch or tag ... reaches", which sends the human looking for a missing push. `_probe_commit` now asks `git cat-file -t <at>^{}` and says `--at <sha> names a tree|blob in <repo>, not a commit`, still `member-at-unknown` and still before the mount. Tested for both. N3. Under a held `index.lock` the unwind restores the bytes of `.gitmodules` but not its index entry, and it reported the whole file as NOT restored. It also quoted git's last line, `remove the file manually to continue.`, which ended in a doubled full stop. Now `_undo_file` raises `_EntryNotRestored` when only the index half fails. The sentence then lists the bytes as restored and "the index entry of .gitmodules put back as it was" as NOT. `_reason` quotes git's first `fatal:` or `error:` line and drops its own full stop. The test holds a real `index.lock` and checks that the holder's state matches what the sentence says. Not done: the review's other suggestion, leaving the trees in place when the gitlink cannot be unstaged. The sentence now says exactly what was and was not put back, and that choice was not asked for. The review's mutants, applied to this commit's family.py, against tests/test_family_add_refusals.py alone: - R1: 1 failed (the dotted-neighbours test), 31 passed - R2: 2 failed (both S1 cases), 30 passed - R5: 1 failed (the linked-manifest test), 31 passed - R6: 1 failed (the registration-it-already-had test), 31 passed - R7: 1 failed (the linked-worktree cached-store test), 31 passed - d25dd4d's own family.py: 6 failed (N6 x2, N2 x2, N3 and the R6 sentence), 26 passed Targeted run over these bytes: tests/test_family.py tests/test_family_add_refusals.py tests/test_shape_doctor.py tests/test_cli_reference.py: 302 passed, none skipped, exit 0 (12m48s), over family.py b6a697ba and test_family_add_refusals.py 6d0de3bd (sha256). No `--help` text changed, so docs/cli.md is not regenerated. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
(lane openRepoShape-4, 2026-10-11T19:59:42Z) Review round 1 is in: c34addc, one commit on d25dd4d, answering the adversarial review's SHOULD and the nits worth a test or a fix.
|
Contributor
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Author
|
(lane openRepoShape-4, 2026-10-11T20:03:46Z) Filed, re-verified on |
…taken, so a holder whose HEAD is an annotated tag or names a missing object is unwound instead of being called landed, and a test pins the `^{}` that names an annotated tag of a tree or a blob as one (#203)
Fix round 2 of PR #236, answering the re-check of c34addc (its NIT 1 and
NIT 2; NIT 3 is cosmetic and NIT 4 is a landing note).
NIT 1. `MountRollback.__init__` recorded HEAD with `rev-parse -q --verify
HEAD^{commit}`, which PEELS and VERIFIES, and `undo` compared it with a bare
`rev-parse HEAD`, which does neither. In a holder whose HEAD names no readable
commit the two disagree: a symbolic ref to an annotated tag answers with the
tag's own sha where the record holds the commit's, and a branch naming a
missing object answers with that object's sha where the record holds None. So
`undo` took a difference nothing had committed for the one commit having
landed, said "The commit had already landed, so nothing was undone" and left
the half-added member staged, in holders d25dd4d unwound. Both reads now go
through one `MountRollback._head`, so they cannot drift apart again. `undo`
still says "landed" only when HEAD names a commit now that it did not name
then; a holder with no commit yet reads None before and after and unwinds to
no commit, and a failed read of HEAD at undo time is still no commit landed,
as before. The new test builds both states with git's own plumbing (`git tag
-a` and `git symbolic-ref HEAD refs/tags/v1`; a loose ref file naming
deadbeef x 5, skipped with its reason if that git does not read one), proves
in the test that the two readings disagree there, makes the pin write fail
after the mount, and asserts the unwind sentence and the holder equal to its
pre-add snapshot. `snapshot` and `assert_unchanged` gain `status=False` for
the missing-object holder, over which `git status` is a fatal error.
NIT 2. `_probe_commit` peels with `cat-file -t <at>^{}`, and nothing pinned it:
without the peel the mutant survived all 32 tests. The sha of an annotated tag
is the tag object's own and `cat-file -t` of it is `tag`, so a tag of a tree
or a blob would be refused as a commit no branch or tag reaches, the wording
the tree and blob refusal exists to replace. Two tests: an `--at` that is an
annotated tag of a tree, or of a blob, is refused as `names a tree|blob in
<repository>, not a commit`; one that is an annotated tag of the commit before
the tip is accepted and pins that commit, as checking the tag out always did.
The remediation both refusals share is now one constant.
Mutants, each applied to this commit's `scripts/family.py` and run over
tests/test_family_add_refusals.py (37 tests):
- `undo` reads a bare `rev-parse HEAD` again (c34addc's own reading): 2 failed,
35 passed, both parameters of
test_a_head_that_names_no_readable_commit_is_unwound_like_any_other, with
"The commit had already landed" where the unwind sentence belongs.
- `cat-file -t <at>` without the `^{}`: 2 failed, 35 passed, both parameters of
test_an_at_that_is_a_tag_of_a_tree_or_a_blob_is_named_for_what_it_tags, with
"is not a commit any branch or tag of InkRouter/IRQS reaches".
Run, over these exact bytes (sha256 family.py bef0c399…, test file 4fbf2fcb…):
tests/test_family.py tests/test_family_add_refusals.py tests/test_shape_doctor.py
tests/test_cli_reference.py with git-filter-repo 2.47.0 on PATH, a gh that
exits 97 and a git config of identity only: 307 passed, 0 skipped, in 4:23
(302 before, plus the five new cases). Not run: tests-macos and tests-windows,
which only CI can see; the loose-ref case is untried on either.
Lane: openreposhape-4 (openRepoShape-4)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Contributor
Author
|
(lane openRepoShape-4, 2026-10-11T20:38:31Z) A re-check of c34addc found it MERGEABLE WITH NITS. On the merge with
|
… (60588d5) and this branch both edit family.py's add path, resolved so _checked_add_target asks family.holding(repository, project) before anything is mounted (#203) Lane 3's #221 (#206) made `family.holding(repository, project)` the one question `add` asks about a member the family already pins, by name in any case and then by repository, and rewrote the opening of `cmd_add` around it. This branch (#203) had already moved that opening into `_checked_add_target` so every refusal about the holder is asked before `git submodule add` runs. The two edits conflict in two hunks of scripts/family.py, and the conflict is the only one: every other path in main merged without a marker. Resolved by the four steps lane 4 left on #221 as its landing note: 1. `_checked_add_target(family, repository, project, path, at)`: the signature gains `repository`, which `holding` needs for its by-repository lookup. 2. Inside it, `held = family.holding(repository, project)` replaces this branch's `family.member(project)`, keeping #221's refusal message (it names the row that pins the repository) and its remediation (`bump --member` names the row's own spelling, not the one typed). 3. #221's `path = f"{MEMBERS_DIR}/{project}"` line is dropped: `cmd_add` already derives `path` and passes it in, and a second assignment inside the function would shadow the argument with the same value. 4. `cmd_add` passes `repository` to `_checked_add_target`. `gh pr update-branch` would not have done this. Git auto-merges #221's `holding()` wording into `_checked_add_target` without a marker, where `held` is never bound and the first `add` of any member raises a NameError; the tests of #221 catch it (4 failed on that tree), the merge itself does not. Resolving by hand, in a commit, is the only way the two behaviours meet. The docstring of `_checked_add_target` gains one paragraph saying that the already-pinned question is asked of `family.holding`, so the helper's own account of what it refuses no longer stops at the by-name check it had. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
This was referenced Oct 12, 2026
This was referenced Oct 12, 2026
brettheap
added a commit
that referenced
this pull request
Oct 12, 2026
… via #236) into fix/adopt-record-submodule-placement Brings in b30595c (#249, #250), be2790e (#204) and 6d8d88b (#203) on top of c3a9645. No rebase. b30595c changes scripts/repo_shape.py and scripts/shape_materialize.py, as this branch does. be2790e changes update-shape.py and 6d8d88b changes scripts/family.py, neither of which this branch touches. One conflict, in scripts/shape_materialize.py's `from repo_shape import` block. b30595c imports `double_quoted` for `render`, and this branch imports `PATH_CLIMBS`, `PATH_ITSELF`, `PATH_NOT_CANONICAL` and `path_spelling_fault` for `_spelling_reason` and `LEG_SEGMENT_REASONS`. Kept all of them, in the block's order. Merged without a conflict, and checked: - scripts/repo_shape.py: b30595c reads a quote as opening a quoted scalar only where a scalar begins, and requires the whole text to be that one scalar (`_unquoted`, `_quote_end`, `_quoted_scalar`; `_scalar` hands them any text that begins with a quote). The record writes every path and reason as one whole scalar through `double_quoted`. `double_quoted`, `_escaped`, `_ESCAPE_OF`, `_unescape`, `parse_yaml` and `path_spelling_fault` are unchanged by the merge. Through the merged reader, `submodule_record` then `parse_yaml`, path and resolution, there are 0 mismatches in each set: - 4,525 names, plus 35 more; - 240 aimed at #252 and #248; - 195 aimed at #249 and #250: `it's`, `a"b`, `"abc`, `a" b "c`, `a' b 'c`, `"a\"`, `a\`, `[it's, a]`, `note "[1]"`, `"Brett Heap`, `O'Brien & Co: #1 [draft] {x}`, and more. - scripts/shape_materialize.py: b30595c spells a YAML template's `"{{KEY}}"` value with `double_quoted` in `render`. The record is appended after `render` has run (`text += append.get(rel, "")` in `_materialize`), so no part of it passes through `render`. This branch's `_spelling_reason`, `LEG_SEGMENT_REASONS` and `leg_path_problem` are unchanged. - No top-level name is bound twice in adopt-project.py, scaffold-project.py, scripts/repo_shape.py, scripts/shape_materialize.py, shape-doctor.py, validate-manifest.py, update-shape.py or scripts/family.py. Each merged module's names are the base's plus what either side added. The only thing both sides changed is the import block above. - The branch's diff against main is still #189 alone: 12 files, +1670 -157, file by file the same as against c3a9645. Lane: openreposhape-1 (openRepoShape-1) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap
added a commit
that referenced
this pull request
Oct 12, 2026
Main brings two commits since b30595c: - be2790e (#204 via #227): update-shape.py names a push or `gh pr create` that fails after `apply --branch` committed, with a NEXT line, with docs/cli.md and its tests; - 6d8d88b (#203 via #236): family.py `add` asks every refusal before it mounts anything. Neither changes a file this branch changes, nor scripts/repo_shape.py or scripts/shape_materialize.py. Every file the merge changes against this branch is main's, and every file it changes against main is this branch's. `render-cli-reference.py --check` still finds docs/cli.md current. Lane: openreposhape-1 (openRepoShape-1) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap
added a commit
that referenced
this pull request
Oct 12, 2026
…s `_entry_findings` composed with #255's, and this PR's pair renamed `unwritable_findings`/`unwritable_remediation` #231 (6f45bf9) puts each entry's path first in `_entry_findings` and returns `unsafe_remediation` ahead of the rest; this branch appends each value `execute` writes into a file that no file can hold. The two hunks are its docstring's order sentence and its return. The sentence keeps main's order and appends this PR's clause; the return is `(findings + submodule_findings + unwritable_findings, unsafe_remediation + remediation + unwritable_remediation)`, so the paths still come first, as #196 needs. This PR's local pair `value_findings, value_remediation` is renamed `unwritable_findings, unwritable_remediation`. #261's branch (ca21b57) binds `value_findings, value_remediation` in the same function, from `_manifest_findings`: a resolution keeping both lines under one name would silently drop one PR's findings. With the rename it cannot. The rest of main (#254/#281, #238/#287, #203/#236, #204/#227, #249/#250, #241, #215 and the earlier merges) merges with no conflict. The resolution is the lane's re-verify one (`resolve-main.py`, 385 passed on 6f45bf9), rename aside. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap
added a commit
that referenced
this pull request
Oct 12, 2026
…`git --literal-pathspecs add -f`, so an operator whose own excludes name `members`, `members/`, `*` or the member gets one commit instead of exit 2, `bump` moves a pin under them too, and each refusal `--force` turns off is made before the member is read (#242) (#308) `add` ran `git submodule add` with no `--force` and `_commit_one` a plain `git add`, and both read a global `core.excludesFile`. `members/` is a fixed path, so an operator whose excludes named `members`, `members/`, `*` or the member's name had every add of every family refused: git's dry run refused the first four before the clone, and a directory-only rule on the member's name (`Atlas/`) refused it only at git's last `git add`, after the clone. `bump` died the same way under `members` or `*`: it stages the gitlink twice, at the `git add` #219 put before its nothing-to-record check and in `_commit_one`, and both read the excludes. This is #174 (adopt, fixed by #200) and #229 (scaffold) on the family side. Every reader is forced now. The mount passes `--force`, and every stage `add`, `bump` and `remove` make is `shape_materialize.force_stage` (#233), `git --literal-pathspecs add -f -- <paths>`: `-f` reaches only the named paths, each taken literally, so no ignored neighbour a glob character would match is staged with them (#202). One definition: `_commit_one` and `bump`'s re-stage both call it. `--force` turns off more than the ignore check, so each other thing it changes is refused before the member is read, the holder left exactly as it was, and a comment at the mount names each guard: - a `.git/modules/<path>` left by a removed member, which `--force` reactivates: `member-git-dir-cached` (#236), at the store `git rev-parse --git-path` names, a linked worktree's included; - an index entry at the path or under it with nothing on disk, which `--force` accepts and replaces with the member's gitlink: `member-path-taken` now asks the index too, and names the `git rm -r --cached` that clears it; on disk it asks `lexists`, so a dangling link is refused as well; - git's own dry run, which `--force` skips (a symlinked `members` had the member cloned through the link, outside the holder; a `members` that is a submodule had it cloned into that submodule's tree): `member-path-unaddable` makes the same dry run first, with `-f`, so an ignore rule passes it and nothing else does; the refusal quotes git's line, and its remediation presumes neither a link nor a submodule (a held `index.lock` fails it too); - a name another submodule already has, which git 2.51 and later rename the mount past and git 2.43 answers by rewriting the other section: `member-name-taken`, reading `.gitmodules` where git reads it (the working tree, else the index's copy, else HEAD's). The PRIVATE/access sentence is printed only for the probe's failed clone of the member, which no ignore rule reaches. tests/test_family_add_excludes.py: under `members`, `members/`, `*`, `IRQS`, `IRQS/` and none, `add` is ONE commit of `.gitmodules`, `family.yaml` and a 160000 gitlink at the member's tip, read with `git ls-tree`, that validates under the same excludes and clones recursively for an operator with none; `bump` under `members` moves the gitlink in one commit and `remove` is the control; each guard above is refused before the member is read with the holder unchanged. Mutations: without `--force` at the mount every rule's row fails; without `-f` in `_commit_one` the `members`, `members/` and `*` rows and the bump test fail; with `bump`'s re-stage a plain `git add` the bump test fails; dropping the index check, the dry run, the name check, its index/HEAD fallback or `lexists` fails exactly that guard's tests. Closes #242 Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap
added a commit
that referenced
this pull request
Oct 12, 2026
…#241, #249/#250, #204, #203 and #238 merge with no conflict, and a `.gitmodules` value git refuses is answered before #190's relative-url check, which reads no url from a file git cannot read Main since d94d155, the main GitHub's update-branch merged at 8ce5614: c2ff2b0 (#215 via #240), c3a9645 (#241 via #256), b30595c (#249/#250 via #284), be2790e (#204 via #227), 6d8d88b (#203 via #236) and ede15ce (#238 via #287). No conflict hunk. Three files were changed on both sides; `git merge` auto-merged each, and every hunk of each side lies apart from the other's. Each side's +/- lines are identical before and after the merge in all three (`git diff -U0` d94d155..8ce5614 against ede15ce..merge, and d94d155..ede15ce against 8ce5614..merge). Line numbers are the merged file's. adopt-project.py - main, c2ff2b0 (#215): 384-506, `SUBMODULE_TYPED_KEY_RE` to `_refused_value` and `_who_refuses_a_boolean`; 510-515 and 522-524, `_registrations`' docstring and its return of `_refused_value`'s words; 791-797, `Source.registered_submodules`' docstring; 2181-2185, `_where_git_fails`' docstring. - #190: 36-38 and 51-53, the module docstring; 102-103, the `repo_shape` import list; 334-339, `Registrations.url`; 2252-2501, `RELATIVE_URL` to `_relative_url_findings`; 2503-2548, `_submodule_findings` and `_entry_findings` with `local_remote_dir` and the NOTEs; 2608 and 2630, `cmd_check`; 3070-3092, `_repository_urls` and `_remote_urls`; 3111-3216, `_refuse_what_check_finds` and `_as_resolved_here`; 3443, `cmd_execute`. scripts/repo_shape.py - main, b30595c (#249/#250): 173-535, the YAML reader's quote boundaries, from the comment after `YamlError` to `_split_flow`. - #190: 1975-1985, `REMOTE_PORT_RE` and `REMOTE_HOST_ALIASES`; 2099-2116, `remote_key`'s docstring and body. tests/test_shape_doctor.py - #190: 2662-2693, `test_a_port_and_githubs_ssh_host_are_not_part_of_a_remotes_key`. - main, ede15ce (#238): 5209-5808, the validator-PYTHONPATH tests after `test_a_drifted_roots_next_command_runs_when_it_is_pasted`. Every other file is one side's blob: main's for the 17 main changed, and 8ce5614's for tests/test_adopt_relative_submodule_url.py and tests/test_adopt_submodules.py. The merge's diff against main is #190's five files, with the stat it had against d94d155 (1097+, 41-). No function or class is bound twice at the top level of adopt-project.py, scaffold-project.py, scripts/repo_shape.py, scripts/shape_materialize.py, shape-doctor.py, scripts/family.py or the three test files. The helpers c2ff2b0 moved into tests/conftest.py (`planned`, `findings`, ...) are plain functions, and tests/test_adopt_relative_submodule_url.py imports seven named helpers from conftest and defines its own `planned`. HOW #215 AND #190 MEET. `_submodule_findings` lists `submodule_plan_problems` first, which holds #215's `plan-gitmodules-unreadable`, then `_relative_url_findings`. Where git's submodule reader refuses a value of the source's `.gitmodules`, `registered_submodules` answers with git's words, a string, and `_kept_relative_urls` reads no url from a string. So a source with both problems gets #215's finding alone, and no NOTE either. #190's finding is not lost: once the value is fixed, `check` reports it, and `execute` refuses it through the same list. #190 adds lines only after #215's, so it can hide nothing of #215's. A relative url kept in a leg is always beside a gitlink kept in a leg, where `_where_git_fails` names a failure, so the unreadable finding is always printed whenever #190's would be skipped. Measured on this tree with a source holding `upstream/dependency`, `url = ../dependency` and `update = bogus`, with `.gitmodules` and `upstream/` kept in code for testorg: - `origin` in otherorg: `check` 1 with `plan-gitmodules-unreadable` alone; `execute --local-remote-dir` 2, `REFUSED plan-gitmodules-unreadable`, with no remotes directory made. With `update = checkout`: `check` 1, `plan-submodule-relative-url`. - `origin` in testorg: the same `check` 1 and `execute` 2, with no NOTE. With `update = checkout`: `check` 0 and the NOTE. - At 8ce5614, before #215, the first source was `plan-submodule-relative-url`. The second was `check` 0 with the NOTE, over a file `git fast-export` dies on. Lane: openreposhape-1 (openRepoShape-1) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 12, 2026
brettheap
added a commit
that referenced
this pull request
Oct 12, 2026
…, #238, #196, #254, #242 and #262/#263 merge with three adopt-project.py hunks kept on both sides, and #262's `_CASE_ASIDE` import kept as the name's one binding origin/main moved b30595c -> 71b58a1: - be2790e (#204 via #227), update-shape.py; - 6d8d88b (#203 via #236), family.py; - ede15ce (#238 via #287), shape-doctor.py; - 6f45bf9 (#196 via #231): adopt-project.py `entry_path_problem`, scripts/shape_materialize.py; - b421e6b (#254 via #281): scripts/shape_materialize.py, bump-leg.py, update-shape.py; - c0ed615 (#242 via #308), family.py; - 71b58a1 (#262 and #263 via #313): adopt-project.py `_root_shape_findings` and scripts/shape_materialize.py `copy_in_the_way`. It fixes what this branch's pull request listed as follow-ups (i) and (ii): a source's own `shape/` where the shape writes its copy, and a plain FILE where the shape needs a directory. Conflicts, all in adopt-project.py, each resolved by keeping both sides: - The module docstring's `check` paragraph. 71b58a1 adds "no path kept in the root where the shape writes ..." before the submodule clause, and this branch adds the symlink clause after it. Both are kept, in that order. - `_entry_findings`' docstring. 6f45bf9 and 71b58a1 give the order "each entry's path, then coverage, then each entry's `leg:`, the two mount paths and what the root keeps where the shape writes, then the source's own submodules". This branch adds "then each symlink the plan keeps in the root that a shape file cannot be written through". - `_entry_findings`' return line. 6f45bf9 returns `unsafe_remediation + remediation`, and this branch appends `_root_symlink_findings(plan, source, tree)` to the findings. The merge returns both. Found after the textual merge, and resolved here so that no commit binds a name twice: 71b58a1 (through 65a9262) imports #198's phrase as `_CASE_ASIDE` in the "#172's shape half" import block, and b37159a had imported it on a line of its own. Main's import is kept; this branch's line and its five-line comment go. scripts/shape_materialize.py merges with no conflict. `place` gets 71b58a1's `copy_in_the_way` inside its collision block and this branch's `_refuse_unwritable` after it, as 71b58a1's message foresaw. Checked on the merged tree: - `_entry_findings` still hands `_leg_findings` the tree, and the symlink findings still come last in its list. - AST: no top-level name is bound twice in adopt-project.py, scaffold-project.py, scripts/repo_shape.py or scripts/shape_materialize.py, and none is loaded but bound nowhere. Main changed the signature of no function this branch calls. - Both sides are intact by +/- line sets, apart from the three hunks and the import above. The next two commits fix what this merge breaks, each with a revert line. #262's rule and this branch's both name a symlink kept AT the copy under `shape/`. And where the shape's copy lands is now `copy_in_the_way`'s question, asked before this branch's own check of it. Lane: openreposhape-1 (openRepoShape-1) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap
added a commit
that referenced
this pull request
Oct 12, 2026
…ing when the member's `project.yaml` records an `id` a row already pins, so adding a repository renamed at the forge no longer commits a second row the holder's own `validate-family.py` refuses (#330) (#381) `add` asked whether the family already pins the member by NAME only: `family.holding` compares the row's project and its repository, case-blind since #206. A repository renamed at the forge keeps its `project.yaml`, so on a family pinning `testorg/Atlas` (id `atlas`), `add --member testorg/Atlas2` (a bare copy under the new name) passed every check, was mounted, and committed a second row recording id `atlas`. The holder's own `validate-family.py` (`_check_member_ids_unique`) then refused that commit as `member-duplicate-id` with exit 1, under `--no-members` too, so the holder's CI went red on a commit `add` itself had written. - `_probe_manifest` and `_probe_member` (#236's throwaway-clone probe) now return the `id` they read from the member's `project.yaml`. It is None only for a linked `project.yaml`, which the probe leaves to the mount, as before. - New `_checked_member_id`, asked in `cmd_add` after the probe and before `MountRollback` is taken or anything is mounted. It refuses as `member-duplicate-id`, the validator's own code (as #299's `member-path` reuses the validator's), names the row that already records the id, and gives the remedy: `family.py remove --member <row>` takes the old row out so this `add` can pin the project under its new name, or `family.py bump --member <row> --to <commit>` moves the row that is there. Two projects recording one id are a fault in one `project.yaml`, mended in that project. - The same check runs again in `_mount_and_pin` on the mount's own reading, the id the row records. A linked `project.yaml`, or a member whose id changed between the probe and the mount, is refused there and unwound by `MountRollback` like any other refusal after the mount. - It checks the rows the validator checks, the way the validator checks them: `str(id)`, compared exactly, skipping a row with no `project:` or no `id:` and a row whose `project:` an earlier row already spells exactly. So `add` refuses what the gate would refuse and nothing the gate would pass; a new test runs the holder's validator over each manifest `add` would write to show it. - Where `member-path-registered` (#300) applies too, a section under another name already giving the mount path, that refusal is said first: it is one of the holder's, asked before the member is read, so the add prints not one line on stdout and clones nothing. `member-duplicate-id` is said on the re-run, once that section is out. A test takes the section out with the commands the first refusal prints and re-runs the same add to show the order. Unchanged: `templates/family-root/scripts/validate-family.py`, which is not in the diff (it ships into every holder, so a change would be an `update-shape` for all of them); every other refusal `add` makes, in the same order; `add`'s output and commit for a member whose id no row records; `init`, `bump`, `remove` and `siblings`; every `--help` text, so `docs/cli.md` is not regenerated. Left out: `bump` does not ask whether the id at `--to` is one another row records, so a member whose `project.yaml` takes another member's id would be bumped into the same `member-duplicate-id`. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 12, 2026
brettheap
added a commit
that referenced
this pull request
Oct 13, 2026
…e or a blob as `member-to-not-a-commit` before the checkout, naming the commit a tag names, and its dry run refuses alike, so a release tag's own id no longer exits 0 with a commit pinning the tag over a gitlink at its commit, which the holder's `validate-family.py` refused as `member-gitlink-mismatch` (#405) (#466) `COMMIT_RE` asks only that `--to` be 40 hex, and an annotated tag's own id is 40 hex: it is what `git rev-parse v2` prints for a release tag. The checkout peeled it to the commit the tag names, the gitlink recorded that commit, and `_bumped_rows` wrote the tag's id into `pin.commit` as typed. `bump --dry-run` printed the split on its `commit` and `gitlink` lines. `_checked_to_names_a_commit` is asked in `cmd_bump` after the fetch, before the dry run leaves and before the real run checks the member out: - `git cat-file -t <to>` in the member. A commit goes on exactly as before; an id the member does not hold goes on to git's own failure, exit 2, as before. - An annotated tag whose `^{}` is a commit, through a tag of a tag too, is refused `member-to-not-a-commit`, exit 2, naming that commit and the re-run `--to <commit>`. - A tree, a blob, or an annotated tag of either is refused `member-to-not-a-commit` naming what it is. Both runs used to exit 2 on git's own error, in two different wordings. - The real run and its dry run print the same two lines, and the holder's HEAD, index, `family.yaml`, work tree and the member's checkout are left as they were. - It is asked of what the fetch brought. A release tag pushed to the member's remote reaches the mount only through `bump`'s own fetch, and a check asked before the fetch would let it through. One test holds that order (`test_a_release_tag_only_the_fetch_brings_is_refused_after_the_fetch`). Refused, not peeled, because `bump-leg.py` refuses the same `--to` for a leg: its `validate-pins.py` goes red on the tag's id in the pin and the bump is rolled back, exit 2. `member-to-not-a-commit` already says a pin is a commit or it is nothing, and the re-run with the named commit is the person's. `add --at` still peels a tag (#236), unchanged. Unchanged: a `--to` that is a commit's id, the `nothing to do` rule, the dry run's row and a `--to` the member does not hold. No refusal code is added or renamed, and no `--help` text, README or AGENTS.md line changes. Left out: `bump-leg.py` refuses a tag's id only through its validators, not by name before anything moves, and its `--dry-run` previews the tag's id and exits 0. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lane: openreposhape-4 (openRepoShape-4)
Claim: #203 (comment)
Fixes #203.
What
family.py addnow asks every question it can refuse on beforegit submodule addruns:family.yamlis a manifest this tool will rewrite.--atcommit exists and is a commit;project.yamlis there and loads.New refusals name the cases that used to fail late:
member-at-unknown,member-no-tipandfamily-manifest-unwritable.Anything that fails after the mount (the pin write, the manifest write, the commit, or a member that moved between the probe and the mount) is unwound: the mount, its
.gitmodulessection, the holder'ssubmodule.<path>config and the module store, which is under.git/worktrees/<name>/modules/for a linked-worktree holder. The refusal names what was put back and what, if anything, could not be.member-not-a-projectnow names the legs of the ROOT a-spec/-codesuffix came off, and that root (IRRS-specgivesIRRS-spec/IRRS-code, rootIRRS), instead ofIRRS-spec-spec.The branch is 11c1168 (the fix) plus d25dd4d (that sentence and the tests). 11c1168's subject starts
WIP (#203):, and the no-force-push ruleset keeps it that way; this PR's title is the squash subject.--helpis unchanged, sodocs/cli.mdis too.Why (repro on
main)addmounted the member first and checked--atand the member'sproject.yamlafter. A refused add (a leg, a short--at) therefore left a staged gitlink, a.gitmodulesregistration and a cached module store. The nextaddof a different member committed the refused leg's registration under its own title, and bothvalidate-family.pyand the doctor passed the result. The review re-ran the issue's repro againstmain(leftover committed, both gates green) and against this head (holder identical after every refusal path: status, both files' bytes,.git/modules,members/, config, branches and HEAD).Tests
New file.
tests/test_family_add_refusals.pyhas 21 tests, with members faked as local bare repositories and aghthat fails if called:--at, an--atthat is not a commit of the member, and a member with no tip;Every refusal's code and NEXT line is asserted exactly.
Targeted runs.
test_family.py test_family_add_refusals.py test_shape_doctor.py test_cli_reference.py: 291 passed on this head, and 291 passed on this head merged withmaina5b09c3.Mutants. Ten of the fix were caught. The adversarial review (lane openRepoShape-4) checked the injected failures and found them honest: each patch replaces only the trigger, and the unwind always runs real git.
Review verdict. MERGEABLE WITH NITS, with no blocking finding. Its SHOULD is one more test: a non-refusal exception after the mount, such as Ctrl-C, is unwound and re-raised. The code handles it today, and no test pins it. That test, tests for four branches no mutant reached, and the review's wording and edge nits follow as one more commit on this branch.
macOS and Windows have not run these tests yet; this PR's CI is the first time.
Landing notes
addrefuses a case-variant of a member the family already pins asmember-already-presentnaming the row that pins it,bumpandremovefind a member in any case, and validate-family.py refuses two rows with oneid, so--member inkrouter/irrsbesideInkRouter/IRRSno longer mounts one project twice past both gates andbump --member irssno longer recommends that add (#206) #221 (lane 3, family.pyaddcompares--memberwith the existing rows case-sensitively, so--member inkrouter/irrson a family that already pins InkRouter/IRRS mounts the same repository again asmembers/irrs, both gates pass a family that pins one project twice, andbump --member irss's refusal recommends exactly that add #206) edits the same opening ofadd, and the merge is NOT mechanical. Git auto-merges family.pyaddcompares--memberwith the existing rows case-sensitively, so--member inkrouter/irrson a family that already pins InkRouter/IRRS mounts the same repository again asmembers/irrs, both gates pass a family that pins one project twice, andbump --member irss's refusal recommends exactly that add #206'sholding()wording into_checked_add_target, where itsheldvariable does not exist, so taking this side alone raises a NameError. family.pyaddcompares--memberwith the existing rows case-sensitively, so--member inkrouter/irrson a family that already pins InkRouter/IRRS mounts the same repository again asmembers/irrs, both gates pass a family that pins one project twice, andbump --member irss's refusal recommends exactly that add #206's own test catches it._checked_add_target(family, repository, project, path, at); inside it,held = family.holding(repository, project), keeping family.pyaddcompares--memberwith the existing rows case-sensitively, so--member inkrouter/irrson a family that already pins InkRouter/IRRS mounts the same repository again asmembers/irrs, both gates pass a family that pins one project twice, andbump --member irss's refusal recommends exactly that add #206's message and remediation; drop family.pyaddcompares--memberwith the existing rows case-sensitively, so--member inkrouter/irrson a family that already pins InkRouter/IRRS mounts the same repository again asmembers/irrs, both gates pass a family that pins one project twice, andbump --member irss's refusal recommends exactly that add #206'spath = …line; havecmd_addpassrepository. On that tree,test_family.pywithtest_family_add_refusals.pygave 109 passed.addrefuses a case-variant of a member the family already pins asmember-already-presentnaming the row that pins it,bumpandremovefind a member in any case, and validate-family.py refuses two rows with oneid, so--member inkrouter/irrsbesideInkRouter/IRRSno longer mounts one project twice past both gates andbump --member irssno longer recommends that add (#206) #221 first. This branch then mergesmainwith that resolution and pushes before it lands. If this lands first, lane 3 applies the same four steps.bumpto the commit a member is already pinned at saysnothing to doand exits 1, so a re-run of a bump that landed is no longerREFUSED a git or gh command failed, exit 2, around git'snothing to commit(#210) #219 (family.pybumpto the commit already pinned exits 2 as "REFUSED a git or gh command failed" around git's "nothing to commit", where family.py documents exit 1 for nothing and bump-leg.py answers the same re-run with "nothing to do", exit 1 #210) and lane 1's shape_materialize.run() and 26 other git and gh calls read what the child wrote as UTF-8 whatever the locale, so anadopt-project.py executewhose leg push is rejected with a message naming a non-ASCII path endsREFUSED pushing the spec legand the ruleset hint underLC_ALL=C PYTHONUTF8=0, not a UnicodeDecodeError traceback (#217) #230 both merge cleanly with this branch. On shape_materialize.run() and 26 other git and gh calls read what the child wrote as UTF-8 whatever the locale, so anadopt-project.py executewhose leg push is rejected with a message naming a non-ASCII path endsREFUSED pushing the spec legand the ruleset hint underLC_ALL=C PYTHONUTF8=0, not a UnicodeDecodeError traceback (#217) #230's head with this branch merged, its decoding tests plus the refusal tests pass.🤖 Generated with Claude Code
Summary by Sourcery
Make
family.py addvalidate all foreseeable refusal conditions before mounting and safely unwind any changes when a later operation fails.Bug Fixes:
family.py addfrom leaving staged gitlinks,.gitmodulesentries, cached module stores, or partial manifest changes when an add is refused or fails after mounting.Enhancements:
Tests:
tests/test_family_add_refusals.py.