Repository navigation
family.py: bump and remove put the holder back when a hook, a lock or git refuses them after they write, and say what they put back or what is left with the commands that put it back; they ask first what their dry runs ask first; and remove refuses a held index.lock before it unmounts and a .gitmodules that gives the member's path to two submodules, of which git rm took out one (#365, #360) - #416
Merged
Merged
Conversation
… begin with a letter or a digit as `member-malformed` before the member is read, so `testorg/_Zed`, `testorg/.Zed`, `testorg/-Zed`, `_org/Zed` and `.org/Zed` no longer exit 0 with a commit that the holder's own `validate-family.py` fails as `member-repository` (#366) `MEMBER_RE` and the validator's `QUALIFIED_RE` shared one character class; the validator also requires each segment's first character to be a letter or a digit. `add` writes `--member` verbatim as the row's `repository:`, so a name the looser pattern admitted was mounted, pinned and committed with exit 0, and the gate the holder ships then failed the commit `add` had just reported. Two tools of this standard disagreed about one rule, and the one that writes the file was the looser. `MEMBER_RE` now takes `QUALIFIED_RE`'s shape. The rulings: 1. THE `.`/`..` CLAUSE IS KEPT, AND MOVED FIRST. The pattern would refuse `<org>/..` too, for beginning with a dot, but in its general sentence; what is wrong with `.` and `..` is that they are no name at all, and #299's more specific sentence says that. It is read before the pattern, so it is still reachable, and only on a name with a `/` (a bare `.` has no owner to blame, and the general sentence asks for one). Its remedy no longer offers `.github` as the example of a name with a dot, since `.github` is refused now, by the validator too; it says `Ze.d`. 2. ONE RULE, PINNED, NOT MOVED. `repo_shape.py` is a copied file: a constant moved there would reach an existing holder only through `update-shape.py`, and both validators spell `QUALIFIED_RE` themselves and ship into projects that run them from their own copy. So `family.py` spells it a third time, and `tests/test_family_member_name.py` reads every spelling (the source, never running the validators: it finds `MEMBER_RE` and every tracked module's `QUALIFIED_RE` outside `tests/`, so a fourth copy is pinned the day it is written) and fails when they are not the same pattern. A corpus of names says what that pattern means, so a change that keeps the spellings equal and moves the rule has to move the corpus too. 3. THE VALIDATOR'S RULE IS UNCHANGED. Which names GitHub accepts is not claimed; `testorg/.github` was refused by the validator already, so nothing a holder can keep depended on `add` admitting it. The general sentence now says the rule (each name begins with a letter or a digit, then letters, digits, `.`, `_`, `-`) and its remedy says why: a commit that wrote such a name would fail the gate the holder ships. BEHAVIOUR THAT MOVES BESIDE THE FIVE. `--member=-org/Zed`, the one way a leading `-` reaches the owner, passed the old pattern and was refused later by `checked_value` as `unsafe-value`; the pattern is asked first, so it is `member-malformed` now (exit 2, nothing read, either way). `testorg/.github` and `testorg/...` are refused. `tests/test_family_member_path.py`'s `test_add_still_takes_a_name_that_contains_a_dot` asserted `.github` was admitted and its remedy string offered `.github`: both are corrected, `a.b` stays. TESTS, `tests/test_family_member_name.py`, members from local bare remotes (a bare copy of the fixture's spare member under each name): `add --member` over the five spellings of the issue plus `-org/Zed` and `.github`, each `member-malformed`, exit 2, empty stdout, no `gh` call, no throwaway clone, and the holder byte for byte (HEAD, `ls-files -s -z`, `status --porcelain=v1 -z`, `family.yaml`, the rest of the snapshot); controls that a name with an inner `.`, `_` or `-` (and an owner with an inner `.`) still adds in ONE commit and the holder's `validate-family.py` passes; the pinned-equality test and the corpus. MUTANT: the old `MEMBER_RE` restored makes the seven refusal cases and both pin tests fail (9 of 13; the four controls pass), the refusals with exit 0 and a committed row. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…mounts, reads the member from throwaway clones outside the holder for the row's id and digest, and prints the row, gitlink, `.gitmodules` section and trailers without mounting, writing or committing anything, and `add` takes `--trailer` as `bump` does, so every subcommand has the dry run the module's `EXIT CODES` line promises (#326) The second part of #326. Part 1 (#345) gave `bump` and `remove` a `--dry-run` and left `add` out until the changes to `cmd_add` in flight had landed. Until now the only way to see what `add` would pin was to run it: clone the member, mount it, rewrite `family.yaml` and commit. `add --dry-run` leaves `cmd_add` after the probe, which asks the member's refusals of a throwaway clone, and before `MountRollback` is taken or `git submodule add` runs. So every refusal `add` makes before its first write is the real run's own code, asked in the real run's order, with the same code, the same words and exit 2. That covers the holder's refusals (`member-malformed`, `member-already-present`, `member-path-taken`, `member-path-unaddable`, `member-name-taken`, `member-path-registered`, `member-git-dir-cached`, `family-members-unrecognised`, `member-at-not-a-commit`) and the member's (`member-no-tip`, `member-at-unknown`, `member-not-a-project`, the manifest's, and an unreadable member). Lane 3's #381 adds `member-duplicate-id` right after the probe; the dry-run return sits after that point, so on a merge it is asked under `--dry-run` too. The member is read a second time, as the real run reads it a second time. The real run reads it from the probe's clone, then from the mount, whose reading the row records. The dry run has no mount, so it makes a second bare clone, the way the probe makes its own: in a temporary directory outside the holder, deleted whatever the answer. It reads that clone at the commit the probe resolved. The `id:` comes from `_member_id_at`, through the function the mount's checkout is read with. The digest comes from `tree_digest`, which reads objects and never a checkout, so the digest printed is the one the real run computes from the same tree, never a guess. A `project.yaml` that is a symbolic link is left to the real run's mount, which reads through it; the dry run says so and prints no id. It prints, in `bump --dry-run`'s layout: - the commit and digest; - the gitlink; - the `.gitmodules` section `git submodule add` writes, under its name and with its url, and whether the file is new; - the registration in the holder's `.git/config`, or the one whose url it rewrites; - the directory and object store the mount makes; - the row as `family.yaml` would carry it; - the trailers. Without `--at`, a note says the real run pins the tip it finds when it mounts, and gives the `--at` that pins the commit printed. The dry run never runs `git submodule add`. It never writes `.gitmodules`, `family.yaml`, the index, the work tree, `.git/config` or `.git/modules`. Exit 0. `add` had no `--trailer`, so ruling 4 (`--trailer` with `--dry-run` prints the trailers it would write) needed one. It is `bump`'s flag: repeatable, kept in order, validated by `trailer_line`, and carried into the one commit through `_commit_one`, as `bump` carries it. With no `--trailer`, the commit is the one `add` has always written. Git's own refusal at the mount is named, not made. With no `.gitmodules` in the work tree and one in the index or HEAD, `git submodule add` refuses to write it before writing anything (git 2.43: `please make sure that the .gitmodules file is in the working tree`), and the real run exits 2 there with the holder unchanged. Only git can say that sentence. So the dry run prints a note saying the real run will exit 2 and how to put the file back, and exits 0. That is how part 1 treats the checkout's own refusal. The module docstring says so. The module docstring's `EXIT CODES` line now says every subcommand takes `--dry-run`. `add`'s help names the dry run and says it reads the member. `docs/cli.md` is regenerated. AGENTS.md is not changed. Tests, appended to tests/test_family_dry_run.py over the same local-bare-remote family: - a real add previewed, both for a holder with members and for a family's first member (no `.gitmodules`: absent stays absent). The dry run leaves these byte-identical: HEAD, `ls-files -s -z`, `status --porcelain=v1 -z`, `family.yaml`, `.gitmodules`, `.git/config`, the `.git/modules` listing, the work tree and the holder's objects. No `gh` call, no temporary clone left. The real add then writes exactly the printed row, section url, gitlink, registration and directory, and the holder validates; - 13 refusals before the first write, each giving the same stdout, the same stderr and exit 2 from the real run and the dry run, with the holder unchanged; - `--trailer` with `--dry-run` printed, and carried by the real commit; - `--at` previewed and pinned; - a registration the mount rewrites; - the `.gitmodules` note, unstaged and staged deletion: dry run exits 0, real run exits 2 on git's line, holder unchanged; - a linked manifest, POSIX only; - in process: both clones are under the run's TEMP and none is in the holder; the holder's `git status`, asked while the dry run's own clone exists, is the status before; no git command in the holder writes (git's own `add --dry-run` aside); no `git submodule`. Targeted set over this tree (Linux, git 2.43.0, CI's git-filter-repo pin): tests/test_family_dry_run.py, test_family.py, test_family_add_refusals.py, test_family_member_path.py and test_cli_reference.py: 269 passed in 433.53s. Mutants, each run against the test the ruling names, each killed: - the dry-run return moved after `git submodule add`: `index changed`; - the dry run's temporary clone made inside the holder: `the holder's status moved while the member was being read`. This is asked while the clone exists, because an end-state test cannot see a clone deleted before the end; - the temporary directory not removed: the leftover check (`offline.assert_left_nothing`). Closes #326 Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n whose path no row names is a finding in every mode, a hand-edited `path:` is `member-path` with nothing read through it and `recorded_gitlink` reads its path literally, and no git to run refuses `git-not-runnable` instead of a traceback that exits 1 (#361, #367, #368) #361. Every member check started from a row: `_check_members` asked each `members:` row for its gitlink, digest and identity, and nothing walked back from what the holder records to a row. A member mounted by hand with `git submodule add`, a row deleted by hand, the section `family.py remove` leaves of two for one path (#360), and a section for `Members/Atlas` that `add` commits beside the member's own all passed the holder's own gate, and the doctor called the holder COMPLIANT. Two findings now, each with its own code, both made before the `--no-members` return, as #350's `member-duplicate` is, because a private family's CI falls back to that mode: - `member-gitlink-unnamed`: every gitlink in `members/` (the directory in any case, so `Members/Zed` counts and `tools/Zed` does not), listed from the whole index at stage 0, else HEAD, the order `recorded_gitlink` reads one in, with no pathspec and `-z`. A HEAD git cannot list records nothing, as `recorded_gitlink` reads it; an index git cannot list is a new refusal, `gitlinks-unreadable`, since this is the one check of `members/` that `--no-members` runs. - `gitmodules-path-unnamed`: every section whose path no row names, read from `.gitmodules` as git reads it, in one reading shared with #300's `gitmodules-path-twice` (`_gitmodules_sections`, which now keeps every key a section holds, for the remedy). "Unnamed" is decided EXACTLY against the path each row gives, its `path:` as written or `members/<project>`. A path a row names under `str.casefold()` is still a finding, and says it is the case variant of that row's path, since macOS and Windows hold the two as one directory. `family.py add`'s `member-path-registered` is unchanged (#362). The remedies print each command on its own indented line, with no backticks, no `;` and no trailing comment. A section is taken out by key, #300's removal, every key it holds. A gitlink a row names in another case is taken out with `git rm --cached`, which touches no file, because a plain `git rm` would take the member's own checkout with it where case is folded. Any other unnamed gitlink is either a row to put back from git, or a hand mount taken out with a plain `git rm` and mounted with `family.py add`. NOT `git rm --cached`, the remedy the ruling named: it leaves the checkout at the mount path, and `add` then refuses `member-path-taken` (probed). After a plain `git rm`, which refuses while the checkout holds changes and takes its section out too, `add` names the object store `git rm` leaves in `.git/modules` (`member-git-dir-cached`), and once that is gone it mounts and pins the member. A test runs that chain as printed. #367. `_check_member` found a hand-edited `path:` wrong, recorded `member-path`, and then checked the gitlink, tree and identity AT that path. At `members/*` the gitlink listing read a glob and answered with a neighbour's commit (a false `member-gitlink-mismatch`), the tree check refused `member-uninitialized`, whose remedy cannot help, and the refusal dropped both findings unprinted. Now the row's checks stop once its pin is read, the pin being a fact of the row itself, and `member-path` names the one path the row may have. `recorded_gitlink` passes git's global `--literal-pathspecs` to both of its listings (`GITLINK_LISTINGS`), never `:(literal)` magic, which a caller's `GIT_LITERAL_PATHSPECS=1` turns off. git refuses that option beside an exported `GIT_GLOB_PATHSPECS` or `GIT_ICASE_PATHSPECS` (exit 128), and `recorded_gitlink` reads a refused listing as "no gitlink here", so those two variables are kept out of those two calls' environment (`git_out` takes an `env` now), or an operator who exports one would see every gate refuse a gitlink that is there. There is ONE `repo_shape.py`, `scripts/repo_shape.py`: the templates carry none, and `scaffold-project.py` and `family.py init` copy it in (`REPO_SHAPE` in `COPIED_FROM_SHAPE` and `FAMILY_COPIED_FROM_SHAPE`), so the fix reaches `validate-family.py`, the assembly root's `validate-pins.py` and `bootstrap.py`, `bump-leg.py` and `shape-doctor.py`, its callers. The issue's decision 3, a refusal that prints the findings gathered before it, is not taken. #368. `git_out` raises `OSError` when there is no git to start, and only #317's `.gitmodules` reader caught it, so the default and `--pins` modes ended in a `FileNotFoundError` traceback, exit 1, a finding's code. The validator now runs every git-reading check inside `_git_runs`: the `.gitmodules` reader, #361's gitlink listing, each member's gitlink and its tree. It refuses ONE new code, `git-not-runnable`, exit 2, naming the check and that git could not be run. #317's no-git clause moves to that code too, so `gitmodules-unreadable` means only a file git ran and could not read. `recorded_gitlink`'s `except Refusal` lets the `OSError` pass, so the missing git never reads as "no gitlink". `_git_runs` wraps the git calls alone, not the identity check, which reads a file. `--no-members` now runs git for the gitlink listing even with no `.gitmodules`, so without git it refuses where it used to pass. The assembly root's `validate-pins.py` ends in the same traceback, and is not changed here. Also: `_check_members`'s docstring said "the two checks that read a file alone" where #317 had made three; it now names them, without a count. Tests, one new file per issue, every state built in git's index and objects with bytes on stdin but the one that runs the remedy chain on a real mount: - tests/test_validate_family_unnamed_mounts.py (#361): the issue's rows (a), (b) and (c), each a finding in the default mode, `--pins` and `--no-members`; a gitlink at a case variant, and its remedy run as printed; the index-first, HEAD-second order; `members/` alone in any case; a deleted row; each section's removal run as printed; an index git cannot list; the hand-mount remedy chain; and the controls, a holder whose rows name every gitlink and section, and one `add` grew. - tests/test_validate_family_member_path_glob.py (#367): six hand-edited spellings in both member modes print `member-path` and never end in `member-uninitialized`; the other rows are still checked; `recorded_gitlink(root, "members/*")` and three other globs are None in a holder with members and no file named `*`; and the four pathspec variables neither widen nor break it. - tests/test_validate_family_without_git.py (#368): in-process, git failing as a missing (or non-executable) program does, exit 2, the one code and no traceback, in the default mode, `--pins` and `--no-members`, with and without `.gitmodules`, with git failing only in `recorded_gitlink`, and only inside a member. - tests/test_family_add_refusals.py: #317's no-git test expects the new code, and its add-past-a-section control expects the one new finding for a section at another path. - tests/test_validate_family_duplicates.py: five of #331's tests rewrote `members:` without the row of a member the holder still mounts, the state #361 now finds, so each keeps a row for every mounted member (`keeping_mounts`) and still asks only about the names it spells. Mutants, each run against these files: the #361 checks moved after the `--no-members` return; `--literal-pathspecs` removed from both listings; the `OSError` clause in `_git_runs` removed; and also the `member-path` skip removed, and the environment scrub removed. Each fails at least one test, listed in the pull request. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…member's own `project.yaml` records, to deleting its own clone when the member's second reading fails, and to the object store git keeps for a linked-worktree holder, so an invented id, a leaked clone or an assumed `.git/modules` path no longer passes (#326) The fix round on the review of 8bcc57c (PR #400). Each of the three behaviours was right in the code, and no test held it. S1, never an invented id (ruling 3). Every member the family fixture scaffolds records `id:` as its name lowercased (`irqs`, `irln`). The reviewer's mutant M4 printed `project.lower()` for every non-linked manifest, and all 22 `add` tests passed. The new member `IRDI` is built by `one_commit_remote` with `id: routing-core`. The dry run prints that id, read from the member, and the real add then writes exactly the printed row. N3, the dry run's own clone removed on a refusal (ruling 2, "removed afterwards, also on a refusal"). Every refusal the tests provoked was made before that clone exists, so mutant M10 (`finally` turned into `else`, leaking the clone on its failure path) survived. The new test runs in process. It moves the member's remote away between the probe's clone and the dry run's own, so the second clone fails in git, for real. The dry run must then: - exit 2; - print `REFUSED reading`, the exact command and the hint, as every failed reading of the member is printed; - leave its clone's directory gone, TEMP empty and the holder as it was. The test does not stub `_probe_member`, whose return #381 changes, so it holds on that merge too. N3, the store named where git keeps it. Mutant M11 printed `.git/modules/<path>` and survived. The new test adds from a holder that is a linked worktree, where `.git` is a file and git keeps a mount's store under `.git/worktrees/<name>/modules/`. The store the dry run names must not exist before the real add, and must be the one the real add then makes. `add_state` reads `.git/objects` and `.git/modules` as absent where `.git` is a file, which is what a linked worktree has. Mutants, the reviewer's own (applied by its `mutants.py` in a scratch worktree at this tree), each over the review's selection `tests/test_family_dry_run.py -k add`, 25 tests here. Each survived at 8bcc57c, and each is killed now by its one new test, 24 others passing: - M4, the id made from the name: `' id: irdi' != ' id: routing-core'`; - M10, the clone leaked on its failure path: `the dry run's clone was left`; - M11, the store assumed: `the store printed, .../lane/.git/modules/ members/IRQS, is not the one git made`. Targeted set over this tree (Linux, git 2.43.0, CI's git-filter-repo pin): tests/test_family_dry_run.py, test_family.py, test_family_add_refusals.py, test_family_member_path.py and test_cli_reference.py: 272 passed in 192.45s, 0 skipped. Part of #326, which #400 closes. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…k or git refuses them after they write, and say what they put back or what is left with the commands that put it back; they ask first what their dry runs ask first; and `remove` refuses a held `index.lock` before it unmounts and a `.gitmodules` that gives the member's path to two submodules, of which `git rm` took out one (#365, #360) #365. A `bump` or `remove` that failed after its first write exited 2 with `REFUSED a git or gh command failed` and left everything it had done, with nothing said about it. A holder `pre-commit` hook that exits 1 left `bump`'s member checked out at `--to` with `family.yaml` and the gitlink staged. It left `remove`'s member emptied by `git submodule deinit -f`, its registration dropped, the gitlink unstaged and `.gitmodules` and `family.yaml` staged. A held `index.lock` left `bump`'s rewrite unstaged over the old gitlink, which `validate-family.py` reported as `member-gitlink-mismatch`. It let `remove`'s `deinit`, which takes no index lock, empty the member before `git rm` died on the lock. Two refusals the #345 dry runs ask before they write were asked by the real runs only after their first write. `remove` refused a `members:` line this tool will not rewrite after the unmount, and `bump --to` a commit whose tree has no `project.yaml` refused with the member left at `--to`. The coordinator's rulings, as built: - `bump` and `remove` run inside `PinRollback`, `MountRollback`'s rule for `add` (#203), borrowing its generic steps rather than spelling them twice. Before the first write it records the gitlink's and each file's index entries, read from the whole index with no pathspec. It records the files' bytes and the member's commit and branch. For `remove` it also records every `submodule.*` key in the holder's git config, the member's object store, its checked-out legs, and whether `git status --ignored` lists anything in it. On any failure after the first write it puts each back: `update-index --index-info` with bytes, the file bytes, `git checkout -q` to the branch or the commit, and `git config --add`. For `remove` it also runs `git submodule update --init --no-fetch -- members/<P>` from the store `git rm` keeps, only while that store exists, so it never clones; then the member's branch and its legs. It ends with `add`'s kind of sentence. The exit stays 2, and a failed git command is quoted and headed as it was. A step that fails is named with git's reason, and a step that needs it is not tried. The sentence then lists, in order, the commands that put the rest back. What `deinit -f` discarded (uncommitted, untracked, ignored files) is said to be gone, never called put back. A failure after the commit landed undoes nothing. - `remove` refuses a held `index.lock` (`family-index-locked`, where `git rev-parse --git-path` says it is) before it unmounts. - What a dry run asks before its first write, the real run now asks there too, outside the rollback: `_checked_bump_target`, and `family.rewritten` in `cmd_remove`. Both runs refuse in the same words, with the holder byte for byte as it was. - `--dry-run` is otherwise unchanged; it does not ask the lock. #360. git maps a path to ONE submodule name, so when two sections gave `members/<P>`, `git rm` took out one and `remove` committed the other and exited 0. Where the other came after the member's own section, the member's own section and its registration stayed; where it came first, the other stayed. `remove` now refuses `member-path-registered-twice` before any git command that writes, and so does its dry run. The refusal names each section with its url and prints #317's by-key removal for every section not named after the path. A name counts by any of its `path` lines. A section that gives the path and then moves on took the path out of git's lookup, so `git rm` took out nothing; it is refused as well. `member-path-registered`'s remediation no longer offers `family.py remove`. That refusal is asked after `member-already-present`, so no row holds the project when it fires, and `remove` would answer `member-unknown`. Tests. The new `tests/test_family_bump_remove_rollback.py` covers, for `bump` and `remove`, a `pre-commit` hook that exits 1 and a held `index.lock`. It covers `remove` with the member's legs checked out, with an untracked file in the member, and `remove --dry-run` under the lock. It covers the two late refusals, held byte for byte to the dry run's words and to the holder; #360 in both section orders, then removed by key as printed, after which `remove` runs and `validate-family.py` passes; and two sections with neither named after the path. It covers an unwind that meets a lock for each verb, with the printed commands typed back to the recorded holder; a commit that landed; and Ctrl-C. `tests/test_family_dry_run.py`'s ghost-section test now asserts the refusal under both runs and, once the others are taken out as printed, what `git rm` drops. Its single-name cases keep `_gitmodules_names`' overwrite model tested, and its refusal table and two docstrings follow the earlier asking. Mutants, each run against the new file and `tests/test_family_dry_run.py`: - drop `bump`'s restore of the member's HEAD: 4 of 49 fail (hook, lock, an unwind that meets a lock, Ctrl-C); - drop `remove`'s `git submodule update --init` restore: 4 fail (hook, legs, what `deinit` discarded, Ctrl-C); - move #360's check after `deinit`: 8 fail (both orders, neither named after the path, the dry-run refusal row, the four ghost-section cases); - move `remove`'s `members:` check back after the unmount: 1 fails (the coordinator's (a), whose stderr gains the rollback's sentence); - drop `bump`'s pre-checks: 3 fail (the coordinator's (a) for `bump`, (b), and `test_bump_dry_run_reads_the_members_manifest_at_to_...`). 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 4 days and 6 hours by commenting |
Reviewer's GuideThe PR hardens Sequence diagram for bump and remove rollbacksequenceDiagram
participant User
participant family.py
participant PinRollback
participant Git
participant Hook
User->>family.py: bump or remove
family.py->>family.py: _checked_bump_target or family.rewritten
family.py->>PinRollback: record holder state
family.py->>Git: checkout, deinit, rm, rewrite, and commit
Git->>Hook: pre-commit hook
Hook-->>Git: failure
Git-->>family.py: CommandFailed or Refusal
family.py->>PinRollback: undo()
PinRollback->>Git: update-index --index-info
PinRollback->>Git: restore file bytes
PinRollback->>Git: git config --add
PinRollback->>Git: git submodule update --init --no-fetch
PinRollback->>Git: git checkout -q
PinRollback-->>family.py: rollback sentence and commands
family.py-->>User: exit 2 with failure and recovery instructions
Flow diagram for remove preflight refusalsflowchart TD
A[remove] --> B[_checked_one_section]
B -->|duplicate path registration| C[Refusal: member-path-registered-twice]
C --> D[Print git config --unset-all commands]
B -->|single registration| E{--dry-run}
E -->|yes| F[_remove_dry_run]
E -->|no| G[family.rewritten]
G --> H[_checked_index_unlocked]
H -->|index.lock exists| I[Refusal: family-index-locked]
H -->|unlocked| J[PinRollback]
J --> K[_unmount_and_drop]
F --> L[No writes]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
…n so CI tests the two stacked (#326) #399 (`MEMBER_RE`, `_checked_member`, #366) lands before this PR and also edits `scripts/family.py`. A Windows CI run now takes about 55 minutes, so this branch is stacked on #399 here: one CI cycle tests main, #399 and #400 together, and this PR can land right after #399 without a second one. The merge was clean (no conflict); `add --dry-run` calls `_checked_member` as `add` does, so the new member-name rule is asked under `--dry-run` through the same function. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…add-dry-run (#326) Main took #396 (`tests/test_agents_md_codes.py`) after #399's last merge of it, so it is merged here as well: the branch now holds main, #399 and #400 and CI tests all three. The merge was clean (no conflict); the one file it adds is the new test that holds AGENTS.md's code-shaped names to the shipped tools. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…icate-id` and to #399's `member-malformed` names, so a dry run that skipped either check no longer previews a row the real `add` refuses, a copy of a pinned project under a new name or a name beginning with `.`, `_` or `-` included (#326) Lane 3's #381 landed as f0a8bf6 before this PR: `add` refuses a member whose `project.yaml` records an `id` a row already pins, as `member-duplicate-id`, before it mounts anything. The review of 8bcc57c said whichever of the two landed second must give that refusal a row in `ADD_REFUSALS`, and that is this PR. #399 (`MEMBER_RE`, #366) also lands first and is merged here, so CI tests main, #399 and #400 together; both merges were clean. No behaviour changes. `cmd_add` asks `_checked_member_id` of the probe's reading just before the dry-run return, so the dry run reaches the check through the same function as the real run, with the same id, and `member-duplicate-id` comes out of `--dry-run` with the same stdout, the same stderr and exit 2. Only the `_add_dry_run` docstring, which listed the holder's refusals and the member's, now names it. - `ADD_REFUSALS` gains `duplicate-id`, last, in `add`'s own order: the member is `IRRS2`, a bare copy of the pinned `IRRS` made beside the holder from the remote its `.gitmodules` names and never in the module's shared remotes, as f0a8bf6's own tests build the renamed repository. A set-up may now return the directory of remotes the add reads from; the others return None. - It gains five `malformed-*` rows for #399's rule, after `malformed-dot`: a project beginning with `_`, `-` and `.`, and an owner beginning with `_` and `.`. `<org>/..` and a bare name, the table's old two, are refused by clauses that predate the rule, so none of them covered it. An owner beginning with `-` cannot be said as `--member <owner>/<project>` (argparse reads it as an option): it stays with the real run's test in `test_family_member_name.py`. Mutants, each run on the parametrized refusal test over the `malformed` and `duplicate-id` rows: - the dry run skips `_checked_member_id`: `[duplicate-id]` fails (the dry run exits 0 and prints the row for `IRRS2`; the real run exits 2), the other seven pass; - the dry run takes `--member` without `_checked_member`: the seven `malformed*` rows fail, `[duplicate-id]` passes; - `MEMBER_RE` loosened to the pre-#399 class: exactly the five new rows fail. Targeted set (Linux, git 2.43.0, CI's git-filter-repo pin): `test_family_dry_run.py`, `test_family_add_refusals.py`, `test_family_add_member_id.py`, `test_family_member_name.py`, `test_family_member_path.py`, `test_family.py`, `test_cli_reference.py` and `test_agents_md_codes.py`: 352 passed. Not changed: a linked `project.yaml` whose target records an id a row pins still gets exit 0 from the dry run, and the real run refuses it after the mount and unwinds, inside the module docstring's stated limit. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…-bump-remove-rollback, so this PR is tested on top of #400, which lands first and conflicts with it (#365, #360) #400 (`add --dry-run`, #326) carries #399 and main 3ef059c, and lands before this PR. Both rewrote the end of the module docstring's "WHAT ONLY THE WRITE ITSELF CAN ANSWER IS NOT ASKED" paragraph, and both changed `tests/test_family_dry_run.py`'s import list from `test_family_add_refusals`. Resolved as review 365-5d56e75 gives it: - `scripts/family.py`: #400's `.gitmodules` sentence, then this PR's held-`index.lock` sentence, then #400's symbolic-link `project.yaml` sentence, and after a blank line this PR's "A REFUSED `bump` OR `remove` PUTS THE HOLDER BACK" paragraph, unchanged; - `tests/test_family_dry_run.py`: the union of the two import lists. main is not merged separately: #400's head carries it. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…riant of a row's path is told to be taken out (`git rm --cached`, `--unset-all`) only where it is a second path to a member that still stands, so a row hand-edited to `Members/IRSS` is said to be the edit and a member recorded only at `members/irss` is said to be in the wrong case, where the command took the member's own record out and `--no-members` then said `family ok` (#361, #367) Review vf-423fb25 of PR #406: SHOULD-1 and NITs 1, 3 and 4. SHOULD-1. The case-variant branch of `_check_gitlinks_named` and `_check_gitmodules_named` took every path that folds to a row's path for the stray and printed `git rm --cached -- <path>`, or the `--unset-all` lines, for it. That is right in one state: the row's `path:` is its canonical `members/<project>` AND that exact path also holds a gitlink (a section), so the path found is a second one to a member that stands. In two others the command took the member's own record out, and the `--no-members` gate, the run a private family's CI falls back to, then said `family ok` over a holder with a gitlink fewer than its rows. Probed by the reviewer, and built here as tests. `_variant_case` tells the three apart, and `_row_paths` carries each row's canonical path for it: 1. STRAY, the row's path is canonical and that exact path holds a gitlink (a section) too: today's `git rm --cached -- <path>` and `--unset-all` lines, unchanged. 2. ROW_EDITED, the path found IS the row's canonical `members/<project>` and the row spells it in another case: the row is the hand edit, so the finding says to put `members/<project>` back in the row's `path:` and prints no command. Under `--no-members` no `member-path` finding says so, which is why this must. 3. WRONG_CASE, anything else, the row canonical and the only record of the member at a case variant: the member is recorded in the wrong case, the `git log -p -- family.yaml` restore sentence the unnamed gitlink always had, no command. A row that is itself hand-edited and a gitlink at a third spelling, and a row with no `project:`, are decided here too (nothing says either is a second path to a member that stands), as the brief's "otherwise". NIT-1. A hand mount is two findings, and the gitlink's `git rm -- <path>` takes the `.gitmodules` section out too, so the section's `--unset-all` lines exit 5 after it. The section finding now says the `git rm` above takes it out and prints no line. Where two sections give the path (`gitmodules-path-twice`) git removes only the one it reads, the last in the file, so each keeps its own lines (probed on git 2.43; the test asserts one section is left). NIT-3. `_member_gitlinks`'s stage-0 filter had no test. A gitlink held at stages 1-3 only is no gitlink the holder records, and one HEAD records and the index holds only unmerged is HEAD's. Built with `update-index --index-info` bytes. NIT-4. The add remedy quotes its project as the paths beside it are: `--member <org>/'My Proj'`. `<org>` is the operator's to fill in and stays outside the quotes. (`shlex.quote` is the file's own helper.) The PR body's caller list is corrected separately. NIT-2 is not done: it goes to the coordinator as a follow-up. Tests: tests/test_validate_family_unnamed_mounts.py 24 -> 39. Every state is built in git's index and objects (`stage_gitlink`, `git rm --cached`, `update-index --index-info`, a hashed `.gitmodules`) and never as two directories, so a case-insensitive disk holds them: the one real checkout stays the one directory it is. Mutants, each run against that file at this head: SHOULD-1 reverted to the old branch (`_variant_case` always STRAY) killed, 8 failed 31 passed, both state tests among them; the stage-0 filter dropped, 2 failed 37 passed; NIT-1's `--unset-all` lines printed again, 4 failed 35 passed. Also killed: ROW_EDITED never (3), NIT-1 for two sections (1), the member unquoted (2), STRAY without the exact path held (4). Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…the object store git keeps for the name `.gitmodules` gives its path, only where that store holds the gitlink's commit, and under `-c protocol.allow=never` with `--no-fetch`, so a section renamed by hand no longer makes it clone the member over the network; a member whose store is gone is removed again; and the dry run asks the manifest first, as the real run does (#365) Fix round 1 on review 365-5d56e75 of PR #416. MUST-1. The rollback ran `git submodule update --init --no-fetch` while the store the member's own `.git` named was there. git does not look there: it uses `modules/<name>` under the holder's git directory, `<name>` being the submodule `.gitmodules` gives the path to, and it CLONES a store where there is none. With the member's section renamed by hand, the rollback cloned the member from its url (only the reviewer's GIT_ALLOW_PROTOCOL stopped it), and an embedded `.git` that `deinit` had absorbed into the name's own store was called gone. `_unrestorable` now asks which store git will use, through the existing readers (`_gitmodules_names`, `_module_dir`), and whether it holds the gitlink's commit, read with `--work-tree` because `git rm` deleted the store's `core.worktree`. Where it does not, `update` is not run: the directory is made again, empty, so the gitlink does not read as deleted, and the sentence names the store git would use, why it is not used, the store the checkout had where that is still there, and says the checkout comes back only from its remote. No command it prints could clone. The member's legs follow the same rule, one by one. Every git command that puts a checkout back runs through `_offline_git`: `git -c protocol.allow=never` with `GIT_TERMINAL_PROMPT=0`, and `update` keeps `--no-fetch`, so a check that is wrong still reaches no remote. `protocol.allow` yields to a `protocol.<name>.allow` set by name and to `GIT_ALLOW_PROTOCOL`, which is why `--no-fetch` stays beside it and the store is checked first. A failed restore's `submodule.<name>.*` keys (`update --init` registers the name before it checks anything out) are named, with the commands that take them out. SHOULD-1. The rollback's set-up read the member with no guard, so a member whose store is gone, which main removes, was refused `git-failed` with a LEG's remediation while its dry run exited 0. The reads are guarded: the member is removed as its dry run previews, and if a later step fails its checkout is said to be in no store, made again empty and never cloned. NIT-1. `.git/config` is not written back byte for byte: `deinit` rewrites it, and a hook, another git command or `update --init` can write it during the run, so nothing shows the recorded bytes are all it should hold. The sentence now says the git config holds the same keys and values, the section put back written last, and no longer calls the holder exactly as it was. NIT-2. `remove --dry-run` asks `family.rewritten` before its `ls-files` read, in the real run's order, so a gitlink the index lacks beside a `members:` line this tool will not rewrite gets the same refusal from both. NIT-3. `family-manifest-unwritable` is spelled once, in `_rewrite_members`, which `add` now calls too, and `_submodule_paths` no longer claims to be the only reader of `.gitmodules`: it names the two `remove` uses. Tests, `tests/test_family_bump_remove_rollback.py` (9 new, 28 in all): the review's renamed-section probe with no store under the new name and with one that lacks the commit; the backstop, with the store check made wrong, a local url that would succeed and no GIT_ALLOW_PROTOCOL, under `protocol.allow=always` (the clone is refused by `protocol.allow=never`) and under `protocol.file.allow=always` (the fetch is stopped by `--no-fetch`); an absorbed embedded `.git`; a leg whose store is gone; a store-gone member removed as its dry run previews, and put back under a refusing hook; NIT-2's combination. Mutants, each run against this file and tests/test_family_dry_run.py (89 tests): no store guard, 2 fail; a guard that asks only that the store be there, 1; no `--no-fetch`, 2 (the fetch case because the fetch happens); no `protocol.allow=never`, 2 (the clone case because the clone happens); the member read unguarded again, 2; the dry run asking `ls-files` first, 1; every leg restored unguarded, 1. The targeted run gives 383 passed. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ly-bump-remove-rollback, so this PR conflicts with main no more (#365, #360) #400 (`add --dry-run`, #326) landed on main as the squash a92e60b, and this branch carries #400 as a merge of 7062a47, so main's merge base with this branch is 3ef059c and the two docstring and import-list hunks of `scripts/family.py` and `tests/test_family_dry_run.py` conflict again. Resolved with this branch's side of both files (`git checkout --ours`), after proving that it loses nothing of #400's: - for each of #400's four files (`docs/cli.md`, `scripts/family.py`, `tests/test_family_add_refusals.py`, `tests/test_family_dry_run.py`), `git show a92e60b:<file>` and `git show 7062a47:<file>` are the same blob, so the branch's earlier merge of 7062a47 already holds all that the squash holds; - no main commit after a92e60b touches any of the four (`git log a92e60b..origin/main -- <files>` is empty); - both resolved files are byte-identical to 7cca975's, and no other file conflicted. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…o fix/family-bump-remove-rollback, so CI tests the pair that lands together (#365, #360) #406 (`validate-family.py` and unnamed mounts) lands before this PR, and both edit `tests/test_family_add_refusals.py`. Merging it here lets CI test the two together, so this PR can land right after #406 without another cycle. The merge was clean: git merged `tests/test_family_add_refusals.py` with no conflict, and no other file is touched by both branches. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ng, so a caller's `GIT_ALLOW_PROTOCOL` or `protocol.<name>.allow` no longer defeats `-c protocol.allow=never` and lets a wrong store check clone the member, and the sentence for a renamed section says its old store can bring the checkout back with no network, not that it comes back only from its remote (#365) NIT-A of review 365-7cca975. `protocol.allow` yields to a `protocol.<name>.allow` set by name and to the caller's own `GIT_ALLOW_PROTOCOL`, so `-c protocol.allow=never` stopped a clone only for a person whose environment and configuration allowed no transport by name. The review measured it with a local bare repository: a caller's `GIT_ALLOW_PROTOCOL=file`, or `protocol.file.allow=always`, cloned under `-c protocol.allow=never`; an EMPTY `GIT_ALLOW_PROTOCOL` refused the clone with `transport 'file' not allowed` even with the setting by name. `OFFLINE_ENV` now carries it beside `GIT_TERMINAL_PROMPT=0` over the caller's environment, for every git command the restore runs. The restore reads local object stores and needs no transport, so nothing is lost, and `-c protocol.allow=never` and `--no-fetch` stay as the layers under it. `OFFLINE_ENV` is a module constant so that the two backstop tests, which each prove one of those guards by leaving it the only refusal, take the empty variable off to keep doing so; the new test `test_a_callers_protocol_settings_do_not_let_a_wrong_store_check_clone` proves the variable itself, with the caller's `GIT_ALLOW_PROTOCOL=file` and `protocol.file.allow=always`, the store check made wrong, a local bare repository as the url, and a premise check that those settings do defeat `-c protocol.allow=never` for a clone. Without the empty variable that test fails, because the restore clones. NIT-B. For a section renamed by hand the sentence said the old store "is still there" and, in its last words, that the checkout "comes back only from its remote". The old store can bring it back with no network, so where the sentence has named it the last words now say the checkout can come back from the object store named above with no network, or from its remote, and that this command did neither. It prints no restore command, and none that would clone. Every other case keeps "comes back only from". `test_a_store_git_would_not_restore_from_is_never_used` pins the sentence. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Contributor
Author
|
(lane openRepoShape-3, 2026-10-12T19:55:50Z) A v1.0.0 scope notice. Brett Heap scoped openRepoShape's first release today: every procedure in AGENTS.md works end to end for ordinary repositories on Linux, macOS and Windows. The tag waits for every in-scope issue to close and a full test run to pass on all three OSes on the release commit. This PR closes #365 (P2, parent #459): in scope, in the |
brettheap
pushed a commit
that referenced
this pull request
Oct 12, 2026
…(), which both roots' bootstrap.py now import instead of defining twice (#391) Why. SonarCloud failed PR #456 on duplication (14.7% on new code against a 3% gate): spelled() and escape_unencodable_stdout(), with the two sets spelled() reads, were written out once in each root's scripts/bootstrap.py, and a test held the two copies identical. The premise behind the copies, that a root cannot import anything of this repository's, was false: every project and every family holder carries scripts/repo_shape.py, the pinned shared library, and both bootstraps already import Refusal, find_repo_root and load_yaml from it. Brett Heap's decision (2026-10-12): move the two helpers into repo_shape.py and have both bootstraps import them; no Sonar exclusion. Where. A new section at the END of scripts/repo_shape.py, after die(): ESCAPED_CATEGORIES, ESCAPED_BIDI_CONTROLS, escape_unencodable_stdout() and spelled(), copied from the bootstrap source byte for byte. Their code is unchanged (the AST of each, docstrings aside, equals both old copies); only the docstring sentences the move made untrue are reworded ("this file", "this script", "the same in the assembly root's bootstrap.py and the family root's"). `import unicodedata` joins the imports after `import sys`, and the module docstring's "WHAT THIS MODULE OWNS" list gains one bullet after `Refusal`. Those three places are clear of the open PRs that also edit this file (#394, #409, #416, #453): each of them merges with this commit without a conflict in repo_shape.py. Both templates/*/scripts/bootstrap.py lose the two definitions, the two sets and `import unicodedata`, and add the two names to their existing `from repo_shape import` line (the family root's becomes the parenthesised form). escape_unencodable_stdout() is still main's first statement, and the module level is still only `sys.path.insert` and the import. Against origin/main, each bootstrap now differs only in that import line and the spelled() call sites. Tests (tests/test_bootstrap_spelling.py). The test test_the_two_roots_carry_the_same_function is replaced by an ast check that each root's `from repo_shape import` names both and that nothing in the file defines or assigns either function or either set; one more test per root asserts that the name its module exposes IS repo_shape's object. The unit rows of spelled() (keep-raw, escape, the full code-point sweep, the non-string values, the one-line check) and of escape_unencodable_stdout() now run once against repo_shape itself instead of once per root. The end-to-end rows run a real scaffolded project and a real family holder, which carry scripts/repo_shape.py, so they exercise the moved code where it ships; they and the bytecode regression test are unchanged. Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap
pushed a commit
that referenced
this pull request
Oct 12, 2026
…landed: `_commit_the_split` keeps #445's reported steps and reads `follow_ups:` through `_plan_follow_ups` Main moved from dd8d0ea to 580d7df: decff87 (#267), dba5fb0 (#403), 245d775 (#446), 75052cb (#445), ec8e09a (#406) and 580d7df (#416). Only #445 touches adopt-project.py, and it conflicted in one hunk of `_commit_the_split`. #445 wraps that function's body in a `try` that names each step (cloning the source for the split, mounting the legs, committing the split, pushing the split branch), so a failure once both legs are pushed flushes stdout, prints `REFUSED <step>.`, what failed and one notice of what the run left. This branch reads `follow_ups:` there through `_plan_follow_ups`, the one read of it (#293). The resolution keeps #445's `try`, steps and reports whole, and carries this branch's read into its "committing the split" step: git had kept main's `[str(f) for f in plan.get("follow_ups", [])]` inside the `try`. The two contracts meet without overlap. `plan-bad-follow-ups` and `election-date-malformed` are findings of `_entry_findings`, which `execute` refuses through `_refuse_what_check_finds`. That runs before #445's `_refuse_an_occupied_work_dir`, `_refuse_a_split_branch_left_behind` and `_create_leg_remotes`, so these are refusals before any leg exists: exit 2, `REFUSED <code>:` and the remediation that says no leg repository was created and nothing was pushed, as #445's own refusals before the legs say, and no notice. #445's report covers the failures once a leg exists, which these values no longer reach. The guard in `_manifest_values` is unchanged; #445 does not touch it. #445's comment over `_template_values` in `cmd_execute` named "an `elected_on` that is not a date" as a refusal the table can raise. With #294's finding, `_refuse_what_check_finds` refuses such a date first, so the comment now says the date no longer reaches the table. #445's test of a table refusal stubs `_template_values` and is unchanged. tests/test_adopt_execute_refusals.py: the two end-to-end runs through `main` (#293, #294) now also assert that the refusal says no leg was created and that #445's notice ("NOTHING has been rolled back") does not follow it. 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
…ncode and spells each value it prints, so `name: "a\ud83db"` or `日本` under `PYTHONIOENCODING=ascii` no longer ends `make bootstrap` in a traceback (#391) (#456) `templates/assembly-root/scripts/bootstrap.py` `main` opened with `print(f"bootstrap: {name} ({root})")`, before any step ran, and `templates/family-root/scripts/bootstrap.py` with the same line (`:180`, with a literal dash in it). - `name: "a\ud83db"` reads as a string holding a lone surrogate, which no UTF-8 stdout can write. `scripts/bootstrap.py` exited 1 with `UnicodeEncodeError`, `make bootstrap` exited 2, and `validate-manifest.py`, the validator that names the problem, was never reached. - `日本` under `PYTHONIOENCODING=ascii` ended the same way. - The family root's line holds a literal `—`, so under that stdout it raised for EVERY name (`InkRouter` included). The assembly root's own wording has the same dash in two lines and a middle dot in the register readout. - A raw ESC or CR in a name is encodable on every stdout and reached the terminal as the control it is (`bootstrap: a^[b^Mc (...)`). Both files are shape copies, digest-pinned in the project that holds them, and a project carries `scripts/repo_shape.py` and no `shape_materialize.py`, so neither can call #214's helper. Both already import `Refusal`, `find_repo_root` and `load_yaml` from that `repo_shape.py`. `scripts/repo_shape.py`, the pinned shared library every project and every family holder carries beside its `bootstrap.py`, gains two functions and the two sets one of them reads (a new section at its end, `scripts/repo_shape.py:2524`). Both roots' `bootstrap.py` import the two functions on the `from repo_shape import` line they already had, and define neither: - `escape_unencodable_stdout()` is the first statement of each root's `main`. A `strict` or `surrogateescape` stdout becomes `backslashreplace`, the handler stderr already has: what the stdout holds is written as it was, what it cannot hold as `\u65e5`. Any other handler is left alone and the encoding is never changed. It is `validate-pins.py`'s guard from #295 (`validate-pins.py:851`), which is inline there and cannot be imported. - `spelled(value)` writes the escape Python spells (`\x1b`, `\r`, `\n`, `\u2028`, `\ud83d`, `\u202e`) for EXACTLY four kinds of character, chosen by Unicode category and not by `str.isprintable`: - the controls (Cc: C0, DEL and C1); - the surrogates (Cs); - the line and paragraph separators (Zl, Zp: U+2028 and U+2029); - the bidirectional controls: U+061C, U+200E, U+200F, U+202A to U+202E and U+2066 to U+2069. Every other character prints as it did before this change: the zero-width non-joiner (U+200C) a Persian name needs, the zero-width joiner (U+200D) of an emoji sequence, the ideographic space (U+3000) of `山田` U+3000 `太郎`, the no-break space (U+00A0), the soft hyphen (U+00AD), any other format, space or unassigned character, and a backslash. The four categories are fixed ranges, so the answer does not move with the interpreter's Unicode database; `unicodedata` is the standard library's. Helper decision: `repo_shape.double_quoted` / `_escaped` (`scripts/repo_shape.py:347`, `:371`) was read first and still cannot serve: it quotes and escapes `"` and `\`, so a plain printable name would not stay byte-identical. The two new helpers now live beside it, in the same file, the one library both roots import from. `shape_materialize.escape_what_stdout_cannot_encode` is in no project; `shape_advisory.printable` flattens to `?` and is in no copy list. Every print of a value the script read from the manifest, a register row or the disk is spelled: - assembly root: the name and the root (first line); a leg's role, path and tracking branch in step (a); a register row's holder, act, objects, state and expiry, the register's path, and the reader's reason when it cannot be read. - family root: the name and the root (first line), a member's project and directory name, `--members-make` (heading and failure line). The member's own command is still handed the RAW target. A plain name prints exactly what it printed before: the new tests spell out the whole output of a plain project and a plain holder, and that golden also passes on the base. So does every valid name that holds one of the characters kept raw above (see the tests). **Files.** - `scripts/repo_shape.py`: `escape_unencodable_stdout()`, `spelled()`, `ESCAPED_CATEGORIES` and `ESCAPED_BIDI_CONTROLS`, in a new section after `die()`; `import unicodedata`; one bullet in the module docstring's "WHAT THIS MODULE OWNS". - `templates/assembly-root/scripts/bootstrap.py` and `templates/family-root/scripts/bootstrap.py`: both names on the existing `from repo_shape import` line (the family root's becomes the parenthesised form); `escape_unencodable_stdout()` first in `main`; every print of a value read from the manifest, a register row or the disk goes through `spelled`. Against origin/main each differs in nothing else. - `tests/test_bootstrap_spelling.py` (new). `docs/cli.md` is unchanged: no `--help` text moved. **Fix round 1 (review of 118c85a).** The first head's `spelled` wrote as an escape every character `str.isprintable` refuses. Independent review (its S1) showed that is wider than controls, surrogates and line separators: it also refuses every space but ASCII space, every format character and every unassigned code point, so a valid name that `validate-manifest.py` accepts and prints raw on the very next lines was escaped by this script's first line (a Persian name with its ZWNJ, an emoji sequence with its ZWJ, `山田` U+3000 `太郎`, a no-break space, a soft hyphen). The printed `git submodule update --init <path>` command was then wrong to paste for such a leg path, and what was escaped moved with the interpreter's Unicode database (Python 3.9 ships Unicode 13, 3.12 ships 15). The claim "prints byte for byte" was false for those names. **The coordinator decided, and Brett may reverse it:** escape by category, not by `str.isprintable`, the four kinds listed above. Everything else prints raw, as on the base. The bidirectional controls stay escaped on purpose, so a name cannot reorder the text around it on a terminal. The alternative the review offered, and this does not take, is to keep `isprintable`, correct the claims, and pin ZWNJ and U+3000 as escaped by a test row. The other round-1 changes: the lone-surrogate rows now assert `FINDING manifest-name` and no `Traceback` unconditionally, since #280 landed in #390 (cb7063f), and the `make bootstrap` row asserts there is no `Traceback` either; the docstrings, the test module's docstring and the unit rows say what is escaped and what is not. The branch has merged origin/main (53bee5b, which carries #390); the merge touched neither bootstrap file. **Fix round 2 (CI and SonarCloud).** - **CI** (2733c6a). The test module loaded each root's `bootstrap.py` through `importlib`, which wrote `templates/*/scripts/__pycache__/bootstrap.cpython-*.pyc`, and `tests/test_scaffold_leg_path_collisions.py`, which reads every file of the assembly template as UTF-8, then failed on it. `load()` now sets `sys.dont_write_bytecode` around the load, and a regression test asserts that loading either root, from the tree and from a copy in an empty directory, leaves no file behind. - **SonarCloud** (d0aea3e). The quality gate failed on duplication, 14.7% on new code against 3%: `spelled()` and `escape_unencodable_stdout()`, with the two sets, were written out once in each root, and a test held the two copies identical. The premise for the copies, that a root cannot import anything of this repository's, was false: both bootstraps already import from `scripts/repo_shape.py`, which every project and every family holder carries. **Brett Heap decided** to move the two helpers into `repo_shape.py` and have both roots import them, with no Sonar exclusion. The code moved byte for byte (the AST of each function and set, docstrings aside, equals both old copies); only the docstring sentences the move made untrue are reworded. The new section, `import unicodedata` after `import sys`, and the docstring bullet after `Refusal` sit where none of the open pull requests that also edit `repo_shape.py` edits: #409, #453 and #394 each merge with this head with no conflict, in that file or any other (`git merge-tree --write-tree` against each branch's head, 2026-10-12). #416 landed as 580d7df without touching `repo_shape.py`. - The branch merged origin/main ec8e09a (e4b0386) first; that merge touched none of this pull request's files. `git merge-tree --write-tree` of this head with current main is clean (2026-10-12). **How it was tested.** `tests/test_bootstrap_spelling.py` (new, 205 tests at this head, local bare remotes, no `gh`). Answered in bytes, stdout decoded strictly in the encoding the run was given (stderr with `replace`), under `utf-8:strict`, `utf-8:surrogateescape`, `cp1252:surrogateescape`, `cp1252:strict`, `ascii:strict` and `utf-8:replace`: - name rows in a real scaffolded project and a real family holder: - spelled: plain, lone surrogate, ESC and CR, `日本`, newline/tab/DEL, U+2028, the right-to-left override (U+202E) with the right-to-left mark (U+200F), an emoji, a quote and a backslash, two lone surrogates; - kept raw, as on the base: a no-break space (on all six stdouts, the cp1252 ones hold it), a Persian name with its ZWNJ, an emoji sequence with its ZWJ, `山田` U+3000 `太郎`, a soft hyphen, a zero-width space with a byte-order mark. For each, the first line is compared in BYTES with what the plain `print` it replaced wrote (assembly and family), and a full run with the validators shows the validator's own `manifest ok: <name>` line raw beside it; - every other print above: a leg's role, path and tracking branch (and the four lines that name a branch, one with a remote-tracking ref elsewhere), a register row, a register's path and the reader's reason, the root's own directory name in both roots, a member row and `--members-make` (with `MAKE` the interpreter, so a member runs only if the RAW target reached it); - the run reaches the validators: a lone-surrogate name prints `FINDING manifest-name`, `make bootstrap` exits 2 and `bootstrap FAILED: manifest (exit 1)`, with no `Traceback` and no frame in `bootstrap.py`; - `spelled` and `escape_unencodable_stdout` directly, in `repo_shape` itself, once rather than once per root: every code point swept once against a set written from the specification (explicit ranges), which is itself checked against `unicodedata.category`; that each root's `from repo_shape import` names both and that neither root defines or assigns either function or either set (an `ast` check of the file); that the name each root's module exposes IS `repo_shape`'s object; that the reconfigure is `main`'s first statement; and that loading a root writes no bytecode under `templates/`. The end-to-end rows run a real scaffolded project and a real family holder, and each carries `scripts/repo_shape.py`, so they run the moved code where it ships. Results (`Python 3.12.3`, Linux; head d0aea3e = the move, on e4b0386 = the merge of origin/main ec8e09a): | run | result | |---|---| | `test_bootstrap_spelling.py`, `test_scaffold_leg_path_collisions.py`, `test_stdout_encoding.py`, `test_template_quoted_values.py`, `test_template_values_single_pass.py`, `test_repo_hygiene.py`, `test_agents_md_codes.py` | 684 passed, 1 skipped (`tests/test_stdout_encoding.py:286`, Windows-only) | | the five `test_yaml_*.py`, the six `test_update_shape*.py`, `test_scaffold_e2e.py`, `test_family.py`, `test_validate_family_unnamed_mounts.py`, `test_cli_reference.py`, `test_windows_paths.py` | 706 passed, 3 skipped (`tests/test_update_shape_add_symlink.py:831`, NTFS junctions) | | mutation: `repo_shape.spelled` escapes nothing | 61 failed, 144 passed: 29 unit rows (every escape row, the code-point sweep, the non-string and one-line rows) and 32 end-to-end rows in a scaffolded project and a family holder (the name lines, a leg's role and path, a branch, a register row and a register's path, the root's directory, the member rows) | | `test_bootstrap_spelling.py` again, the mutation reverted | 205 passed | Earlier heads, not re-run on this one: on af26b19 the then 260-test file gave 197 failed, 63 passed against the base (cd1fc79) templates, 59 failed with both copies' `spelled` reverted to `str.isprintable`, and caught all 12 mutations of the category sets; reverting each hunk of the first round's fix, one at a time, still failed a new test (34 mutations, listed in the report); on 118c85a the existing bootstrap, scaffold, adopt, update-shape and doctor tests gave 496 passed, 1 skipped, and 305 passed. macOS and Windows have not run here; CI runs them on this head. Every new test reads and writes bytes and compares against `project.resolve()`. What the rest of the run says (the lone-surrogate and `make bootstrap` rows assert it at this head): a lone-surrogate name prints `bootstrap: a\ud83db (<root>)`, reaches step (b), and ends ``` FINDING manifest-name: name is 'a\ud83db', which holds U+D83D, a lone surrogate: no UTF-8 file, commit message or console can hold it. ... bootstrap FAILED: manifest (exit 1) ``` exit 1 (`make bootstrap` 2), with no `Traceback` on a pipe, a TTY or `make`. **What is left out.** - The validators' own stdout. `validate-manifest.py`'s `manifest ok: <name>` still prints through a stdout with no handler: a valid `日本` under `ascii` ends in ITS traceback (the issue's "Not in this issue"), and an ESC or CR name is accepted and the line writes the raw control. - A manifest value that is passed to git rather than printed: `tracking_branch: "a\ud83db"` (or a leg's `branch:`) raises from `subprocess` inside step (a), before the validators; a NUL does too (`ValueError: embedded null byte`). Reproduced on origin/main 53bee5b. `validate-manifest.py` never reads either key, so it accepts the manifest (`manifest ok`, exit 0) and NO tool names the value. Not a `print`; follow-up below. - stderr: `print(str(exc), file=sys.stderr)` (Refusal text built by `repo_shape`, multi-line, manifest text `repr`-escaped; stderr cannot raise) and the family's `FETCH INCOMPLETE` text (git's own words). - `templates/family-root/scripts/siblings.py`, which carries the SAME first line twice (`:676` in `dispatch_all`, `:906` in `_run_placement`) with the same plain `print`, and prints member rows raw. `make siblings`, `make park` and `make resume` on `name: "a\ud83db"` still end in a `UnicodeEncodeError` traceback (reproduced on origin/main), and under `PYTHONIOENCODING=ascii` they end so for every name (the literal dash). It already imports from `repo_shape`, where both helpers now are, so they are two more names on its existing import line. Follow-up below. - Behaviour that differs from the base, all deliberate: - a control, a surrogate, a line or paragraph separator or a bidirectional control in a value now prints as its escape where the base wrote it raw, so a Hebrew or Arabic name that holds an explicit U+200E, U+200F or U+061C prints `\u200f` for it; - a root path holding a byte that is not UTF-8 prints `\udcXX` where the root `bootstrap` shim's #214 handler wrote the raw byte back; - a name holding the four characters `\x1b` prints as one holding ESC does; - the wording's dash and middle dot print `\u2014` and `\xb7` on a stdout that cannot hold them, instead of raising. - The digest: no digest of either bootstrap or of `repo_shape.py` is recorded in this repository. The scaffold, `family.py init` and adopt compute each `files:` row from the bytes they write, and `update-shape.py` compares upstream blobs at two commits. A project scaffolded from origin/main 580d7df, checked against this head (d0aea3e), reads `upstream-changed` for `scripts/bootstrap.py` and `scripts/repo_shape.py` and nothing else (assembly root: 2 changed, 9 unchanged; family holder: 2 changed, 8 unchanged). `apply` copies both and re-pins, the project's own validators say `11 copied shape file(s) match their digests` (the holder's, 10), each copy equals its upstream blob, and `scripts/bootstrap.py` runs. Run for real, local remotes only. - Compatibility: a project that keeps a locally edited `scripts/repo_shape.py` (`check` reads it `both`, and `update-shape.py apply --accept-local scripts/repo_shape.py` keeps the project's bytes) while taking the new `bootstrap.py` meets an `ImportError` the next time it bootstraps: `scripts/bootstrap.py` ends in `cannot import name 'escape_unencodable_stdout' from 'repo_shape'`, exit 1. Following `check`'s own instruction avoids it: `check` says to merge upstream's bytes into the edited file by hand, commit it, and only then `apply --accept-local`, and a merge that takes the new section brings both names. A project that skips the merge gets the error and nothing reports the break after `apply`: its validators still pass, because they import neither name; `check` then reads everything unchanged; and the project's `validate.yml` never runs `bootstrap.py`, so its CI stays green. The first command to fail is the one run after `git clone`. Guarding it is an item on #458's checklist (#458 (comment)): `apply` would refuse a kept `--accept-local` copy that lacks a name another copied shape file imports from it. It is the same coupling as the names the bootstraps already import from that file. Run for real on a project scaffolded from 580d7df. Follow-ups found by lane 5's review of this branch, each reproduced on origin/main and filed separately: - `siblings.py`'s two first-line prints (and its member rows): `make siblings`, `make park` and `make resume` on a lone-surrogate `family.yaml` name end in a `UnicodeEncodeError` traceback (#454). - A lone surrogate or a NUL in `tracking_branch` (or a leg's `branch:`) ends `make bootstrap` in a subprocess traceback in step (a), and `validate-manifest.py` accepts the manifest (#455). - #454 and #455 (open, off the milestone) and the two #459 checklist items moved from #463 and #464 can import `spelled` and `escape_unencodable_stdout` from `repo_shape.py`, wherever they print a value. Reviewed adversarially at 118c85a (CHANGES NEEDED: 0 blockers, 2 should-fixes, 2 nits); fix round 1 (af26b19) was re-verified APPROVE WITH NITS (0/0/2). Fix round 2 stopped the test module writing bytecode into `templates/` (2733c6a) and moved the two helpers into `scripts/repo_shape.py` on Brett Heap's decision (d0aea3e); its independent re-verification read APPROVE WITH NITS (0/0/3), all three about this text and settled here. CI is green on Linux, macOS and Windows, with SonarCloud passing, at d0aea3e (main ec8e09a merged in). Closes #391 Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 12, 2026
brettheap
pushed a commit
that referenced
this pull request
Oct 12, 2026
…_spelled` now spells every symlink this branch names, and #283's tracking-branch question is not asked by adopt-project.py, so it meets none of this branch's refusals Brings in, by sha, main's ten commits since 75052cb: ec8e09a (#406), 580d7df (#416), 1184f67 (#404), d6df36b (#346), 37665ff (#452), 36538fd (#378, #329), 6c2a371 (#355, #283), 8782333 (#401, #340), ec4a7a0 (#456, #391) and 437a360 (#343, #290). No conflict. The tree is `git merge-tree --write-tree 437a360 95f6fc7`'s, ae7be0a. Three files changed on both sides merged by git alone: - adopt-project.py: 36538fd rewrites `_spelled`'s docstring and `_spelled_character` (a byte that is not UTF-8 is `\udcNN`, a character `\xNN`, by `repo_shape.double_quoted`), and two docstrings (`_refused_by_policy`, `_index_entries`); 8782333 adds `_leg_path_help` and the `help=` of `plan`'s `--spec-path` and `--code-path`. This branch's hunks are elsewhere. - scripts/shape_materialize.py: 6c2a371 adds `import tempfile` and the branch-name question (`_ask_git_branch`, `_branch_verdict`, `branch_problem`, `refuse_bad_branch`) after `run`. This branch's hunks are elsewhere, and it adds no import of `tempfile` to either file (adopt-project.py has had one since before 75052cb), so no name is bound twice. - tests/test_adopt_finding_spelling.py: 36538fd's `\udcfe` lines and this branch's `follows too many symlinks` line are more than 100 lines apart. How main's changes meet this branch's code: - 36538fd: every source path and symlink target this branch's findings and the materializer's refusals name is spelled by `_spelled` (`execute` hands it to the materializer as `spell`), so they now spell a byte that is not UTF-8 as `\udcNN`, as the plan's own `path:` does. One test of this branch pinned the old spelling, `nowhere\xff`; the next commit pins `nowhere\udcff`. Nothing in this branch has an escape table of its own. - 6c2a371: `refuse_bad_branch` is called by scaffold-project.py and scripts/family.py only. adopt-project.py's `--tracking-branch` is not asked of git (6c2a371 leaves it as its own change), so the question falls nowhere among `check`'s findings or `execute`'s refusals. - 8782333: `plan --help` text only; `plan` refuses nothing new, and the help names `shape/` as where `execute` writes its copy, which this branch does not move. Checked on the merged tree: no top-level name in adopt-project.py, scaffold-project.py, scripts/repo_shape.py, scripts/shape_materialize.py or scripts/family.py is bound twice that was not already bound twice on main (shape_materialize.py's `YamlError` and `parse_yaml`, as on main), no name is loaded and bound nowhere, and each side's own change reappears whole in the merge (no line of either side lost). 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 13, 2026
… is `PRODUCT=PATH` only when its key names a chain link, and a link source that cannot be entered is a WARNING with the system's reason, not a traceback (#338) (#453) **What was wrong.** Two faults, one in the parser and one in the lookup. - **The parser.** `_link_sources_from_args` split every `--link-source` value at its first `=`. A link tree under a directory named `a=b`, passed bare, became a source keyed to a link `a` that no chain names. Nothing asked for it, so the link stayed `declared-unverified`, exit 0. The WARNING then offered `--link-source openXdox=<path>` to the person who had passed exactly that. A mistyped key failed the same silent way. - **The lookup.** `repo_shape.link_manifest_path` asked `Path.is_dir()` and then `is_file()`. Before Python 3.14 those swallow only ENOENT, ENOTDIR, EBADF and ELOOP. So each of these ended the naming gate in a traceback, exit 1: - a keyed source under a parent this user cannot enter; - `SHAPE_PIN_SOURCE_<LINK>` pointing there; - a checkout `../openXdox` at mode 000, which every `--project` run asks for; - a 300-character name. **What changed.** **How a value is read** (`LinkSources` in `scripts/validate-repository-naming.py`). This mirrors #312's `PinSources`, with a chain link where that file has a declared pin: - The text before the first `=` is a LINK only when it casefolds to a link of a chain this run reads. Those are the entries of `--referent-chain`, and of each leg's `naming.referent_chain:` under `--project`. Both are read before any leg is classified. - The keyed reading wins even over a directory spelled like the whole value. `./` reads that directory. - Any other value is a bare PATH, taken whole. **What a lookup says** (`scripts/repo_shape.py`): - `link_manifest_lookup` asks `stat()` inside a guard, as `_digest_from_source` does in #312. It returns the system's reason when a source cannot answer, or says which of four plain cases it is (`no such file or directory`, `it holds no project.yaml`, `its project.yaml is not a file`, `it is neither a directory nor a file`), and it never raises. It replaces `link_manifest_path`, which is gone: nothing calls it. `link_declarations` gives the reason a manifest could not be read, and `declared_neutral_pins`, which it left with no caller, is removed (fix round 1). - `resolve_link_source` and `link_pins_from_trees` take a keyword-only `tried`. It records each source asked, in order, with its reason. Nothing in `repo_shape` prints. - `link_pins_from_trees` also takes a keyword-only `with_reasons` (fix round 2). Asked, it returns a `LinkPins`, a dict equal to the plain mapping that also remembers why a tree it reached could not be read. Only the scaffold asks. **What the validator prints** (stderr, after each name's WARNINGs): - `WARNING link-source-unreadable: <path> (<how>) could not answer for <link>: <reason>`. This covers a `--link-source` asked for a link that holds a pin, a `SHAPE_PIN_SOURCE_<LINK>` likewise, and a checkout beside the project that exists and still cannot answer. A keyed value that also names a directory gets a second line saying how to ask for that directory with `./`. A bare value holding `=` says it was read as a PATH. - `NOTE --link-source <typed>: not used — <why>.` for a value no such link asked for. The why is one of five: a later value replaced it; no name reads that link's own declaration (a keyed value); no name records a chain at all; no name reads any chain link's own declaration (a bare value, where chains exist and none is read); or every link that is read had a more specific source. - Three kinds of source are never named: a variable or a checkout asked only for a chain's LAST link (no one ever reads that link's own declaration); a variable or a checkout for a chain no classification reads (fix round 1: a spec leg's chain, a name with no referent, a chain whose shape fails; a `--link-source` value for such a chain is the NOTE); and a checkout that is absent or holds no manifest. - `--quiet` drops both, with the WARNINGs. So `make bootstrap` and the doctor's `naming` row print nothing new. `--help`, the module docstring and `docs/cli.md` (regenerated) say all of this. **Fix round 1 (lane 5's review of 40762ca: 0 blockers, 2 should, 4 nits).** Second commit, 4b21263. Each review finding, in order: - **A link manifest whose `neutral_product_pins:` is not a list of names is a tree that cannot answer** (SHOULD-1). `link_declarations` iterated `data.get(key) or []` as it stood, against the `link_pins_from_trees` docstring this change wrote ("nothing a source does raises out of here"). `5` ended the naming gate, the scaffold and the project's `validate-manifest.py` in `TypeError: 'int' object is not iterable`. A scalar `openDox` was read as the characters `D`, `e`, `n`, `o`, `p`, `x`, so the chain broke, and under `--project` that is a FINDING. A mapping was read as its keys, so `openDox: yes` DECLARED `openDox`. Each is now `(None, why)`, as `not a mapping` is: declared-unverified, with the reason named ("`neutral_product_pins` is not a list", "holds an entry that is not text"). An absent or null value, an empty list, and a blank entry (`null`, `''`) in a list, declare nothing, as ever. The fix is in the shared function, so the scaffold and `validate-manifest.py` are reached too. **Three behaviours change on purpose here, and the tests of fix round 2 pin each:** - **A falsy value is no longer an empty list.** `neutral_product_pins: 0`, `false` or `''` was read as no pin at all (`or []`), so a chain through the tree was BROKEN, and under `--project` that is a FINDING. It is now a tree that cannot answer: the link is declared-unverified, a WARNING, exit 0. - **An entry that is not text is no longer read as text.** `[openInk, 0]` and `[openInk, false]` dropped the entry as a blank, and `[openInk, true]` read it as the text `True`, so a chain through the list VERIFIED. Each is now declared-unverified, with a WARNING saying the list "holds an entry that is not text". That is stricter than the link tree's own `validate-manifest.py`, which takes `str(pin)` of an entry (`_declared_pins`, `templates/assembly-root/scripts/validate-manifest.py:85`): a chain is better left unverified than verified through a name nobody wrote. - **The scaffold no longer refuses `chain-not-declared` over a pins value that is a scalar.** With `--pin-source` a tree whose `neutral_product_pins:` is the scalar `openInk`, the scaffold used to refuse (`REFUSED chain-not-declared: ... openXdox declares I, e, k, n, o, p, not openInk`, exit 2, from the characters). It is now the declared-unverified WARNING and the scaffold goes on, exit 0, which is its documented rule for a tree that cannot answer: "a warning, never a refusal, because the check is offline". The warning says the tree was reached and could not be read (fix round 2, below). - **The reach wording is corrected** (SHOULD-2): "Who else this reaches" below, and the docstring of the test that made the claim. What stops crashing is the naming and manifest steps. `make validate`, `validate.yml` and `make bootstrap` still end in a traceback on the sibling row before Python 3.14, at a file this change does not touch (below). On 3.14 the sibling is passed over without a word there instead, which is #430. - **The mutation gaps** (nit). Six of the review's ten mutations survived. Five now die, each by a new test: a `project.yaml` that is a directory; a device or a FIFO as the source; the order A, B, A; a value holding `=` with no chain at all; and every failure of `SHAPE_PIN_SOURCE_<LINK>` for a link that holds a pin (a path that is not there, a directory with no manifest, a file that is no manifest), not only a permission. The sixth, the `typed in self.used` test in `unread_lines`, could not be killed because it was dead: a value is read as one link's keyed source or is the one bare PATH, so every lookup asks it of the same path and it is used or unread, never both. That was asserted over 293 tests and the test is removed. - **A chain no classification reads** (nit). `classify` consults a link's own declaration only for a name that claims a referent, with a recorded chain whose shape holds. For any other chain (one recorded on a spec leg; one on a name with no `open<Product>` to reach; one that does not begin at a held pin or does not end at a referent of the name) a source was still asked, so a path that was not there was reported as `link-source-unreadable` and a good tree was counted used, each for a question nobody put. `NamingPolicy.reads_link_declarations` now says when `resolve_referent` reads a chain, and `LinkSources.link_pins` asks it first: such a chain asks nothing, and a value for it is the NOTE that no name reads that link's declaration. A test runs `resolve_referent` with a mapping that records whether it was read, over every way a chain is and is not, and holds the new method to it. - **Dead code and stale sentences** (nit). `declared_neutral_pins` is removed: `git grep` over the tree, every pushed branch and every sibling worktree finds no caller. `link_manifest_lookup`'s docstring no longer says "replaces" over a name that is gone. This description no longer says `tests/test_naming_policy.py` takes `SEARCH_DENIED` from #312: it defines a mark of its own, with the same condition and its own reason text. **Fix round 2 (lane 5's re-review of 4b21263: 0 blockers, 0 should, 3 nits).** Third commit, 2ac911c. Each finding, in order: - **The three behaviours the body did not state are stated and tested** (nit 1). Round 1's not-a-list fix changed three behaviours on purpose, each listed under SHOULD-1 above, and the re-review found two of them had no test. R1 (`if declared is None` read as `if not declared`, which makes `0`, `false` and `''` declare nothing again) and R2 (`if pin is None or pin == ""` read as `if not pin`, which drops a `0` or `false` entry as a blank) both passed all 210 cases of `tests/test_naming_policy.py` at 4b21263. Six rows join `NOT_A_LIST_OF_NAMES`, which runs at the CLI and in process: `0`, `false` and `''` as the value, and `0`, `false` and `true` among the names. R1 now fails 9 cases and R2 fails 6. - **The scaffold's warning is true for a tree it reached** (nit 1, the third behaviour). Over a `--pin-source` whose `neutral_product_pins:` is not a list, round 1 turned the `chain-not-declared` refusal into a declared-unverified WARNING that says the tree "is not reachable here (a sibling checkout, SHAPE_PIN_SOURCE_<LINK>, or --link-source <LINK>=<path> would let it be read)". `--pin-source` did reach the tree, and only its declaration could not be read, so that blamed a path that was right. Now `link_pins_from_trees` takes a keyword-only `with_reasons`; asked, it returns a `LinkPins`, a dict equal to the plain mapping that also keeps `unreadable`, `{link: why}` for each link whose source answered and whose manifest could not be read. `_chain_link_findings` words such a link "which was reached but could not be read as a declaration (<why>)", in a branch of its own, and only the scaffold asks. The new text is asserted for each of the eleven not-a-list rows, with `scaffold-project.py --dry-run` run as a subprocess. - **Every other path's text is the bytes it was.** The sentence for a link no source reached is the same code in the same lines, and four tests hold it: the generic warning for a plain mapping, for an empty `LinkPins` and for an absent tree; the scaffold's own old warning for a clone that holds no `project.yaml`; the naming validator's two lines (its warning, and the `link-source-unreadable` line that names the reason); and `validate-manifest.py`'s. The naming validator and `validate-manifest.py` do not ask for reasons, so they print what they printed. - **One more scaffold path changes with it, on purpose.** A tree the scaffold reached and could not read as a declaration for any other reason (a manifest that does not parse, or is not a mapping) used to say "not reachable here" too, and was no truer. It now says what it is, with the system's reason, and a test for each of the two (a manifest that is not a mapping, one that does not parse) holds it, at the scaffold's command line and in process: narrowing the wording to the not-a-list reasons fails both. - `_declaration` still returns its four values, so nothing else in `scaffold-project.py` moved. - **A qualified chain link in the spy test** (nit 2). `_chain_read_cases` had no case whose chain link is qualified, so `reads_link_declarations` could take the chain as typed and not through `_chain_links` (R3) and pass every test. With `--referent-chain testorg/openXdox,openDox` that turns a verified chain into a declared-unverified one. `("codexDox", {"openXdox"}, ("testorg/openXdox", "openDox"), True)` is a case now, and R3 fails it. - **The description** (nit 3): the ten corrections of the re-review are applied (the reason a lookup returns, the fifth NOTE reason, the third kind of silent source, the SHOULD-1 bullet, "before Python 3.14", the `validate-pins.py` bullet, the 3.14.7 rows, the merge-tree section, the filed numbers, and the project's own `validate-manifest.py`). The mutation proofs: all three survivors were reproduced first at 4b21263 (210 passed each) and each dies at 2ac911c. Six mutations of the new code each die too: the warning ignoring the reasons (12 cases fail), the lookup recording none (12), the scaffold not asking (11), `with_reasons` on by default (3), an absent tree called reached (3), and the reached sentence used for a tree nothing reached (17). **Where it departs from #312, and why.** - **An explicit source that cannot answer stays a WARNING, exit 0.** #312 made an unreadable `--pin-source` a finding. Three reasons for the difference: - Exit 1 here means "a name is wrong", and this module has always said an unread link tree is a warning. - `SHAPE_PIN_SOURCE_<PRODUCT>` also answers `validate-pins.py`'s digest question, from a checkout that need not hold a manifest. - The lookup falls through to the next source, which may still verify the link. - **The fall-through is kept**, as it always was. With `--pin-source`, a keyed value is final. - **The lines go to stderr, spelled `WARNING`/`NOTE` as this file spells them.** #312's notes are ` note` lines on stdout, where that file's report lives. **Who else this reaches.** The guard is in `repo_shape.py`, so the same `link_pins_from_trees` lookup stops crashing for these callers too: - **The project's own `validate-manifest.py`.** It reaches the lookup through `SHAPE_PIN_SOURCE_<LINK>` or a sibling checkout, and now gives its usual declared-unverified WARNING instead of the traceback. - **`scaffold-project.py --referent-chain`.** A `SHAPE_PIN_SOURCE_<LINK>` under a locked parent no longer kills the plan, and `--pin-source` verifies the link. Neither of these prints the reason a source could not be entered; the scaffold says it only for a tree it reached and could not read (fix round 2, above). A `--pin-source` the scaffold cannot enter is still a traceback: that is #339 (`pin_digest_from_source`), and this PR does not touch it. **What this does not reach, and what the doctor says.** - **The project's gates still end in a traceback on the sibling row.** The NAMING and MANIFEST steps of `make validate`, `validate.yml` and `make bootstrap` stop crashing for a checkout `../openXdox` at mode 000. Those targets also run `validate-pins.py`, whose `resolve_neutral_pin_source` asks `(sibling / ".git").exists()` with no guard (`templates/assembly-root/scripts/validate-pins.py:463` on main), so before 3.14 a sibling at mode 000 still ends them in a `PermissionError`. That file is #312's, and `fix/pin-source-env-collisions` (PR #409, since landed), `fix/adopt-pin-writes-pin-file` and open PRs #343 and #419 touched it when this was written, so this PR does not edit it. The same line is listed for Python 3.14 by #430 (lane 2), where the helper that would cure both halves is asked for; the pre-3.14 traceback reproduction is added there as a comment, not filed apart. A `SHAPE_PIN_SOURCE_<LINK>` under a locked parent is different: `validate-pins.py` makes that a `neutral-pin-source-unreadable` FINDING (#312), and that stays. - **The doctor's `naming` row** runs the STANDARD's copy of the validator, so it stops crashing at once. **Its `manifest` row** runs the project's OWN `validate-manifest.py` on the project's own `repo_shape.py`, so it changes only once the project has applied this change. Over an existing project the doctor says `INVALID (manifest, pins)` until `update-shape.py apply` lands the two `upstream-changed` rows below. - **The verdict.** For `SHAPE_PIN_SOURCE_<LINK>` under a locked parent, on a project that carries these copies, `INVALID (naming, manifest, pins)` becomes `INVALID (pins)`, and `pins` is #312's finding for that locked variable, which is right. For a sibling at mode 000 the `pins` that remains is the traceback above, not a finding. The two copied scripts this change edits (`scripts/repo_shape.py` and `scripts/validate-repository-naming.py`) are copied into every project and pinned there. A project takes the change through `update-shape.py` (two `upstream-changed` rows), and a family holder takes the `repo_shape.py` row. Neither file has a second copy in this repository. **Evidence.** - **Issue rows, main ab623dd** (`repro338.sh`, a `gh` stub first on PATH). Under 3.10.21 and 3.12.3, rows 4-7 (keyed under a mode-000 parent, `SHAPE_PIN_SOURCE_OPENXDOX` there, a sibling at mode 000, a 300-character name) are a traceback, exit 1. Rows 2-3 (`a=b`, the typo) are a silent declared-unverified, exit 0. Under 3.14.7 row 1 verifies, and rows 2-7 are the silent declared-unverified WARNING, exit 0. - **Issue rows, this PR.** Under 3.10.21, 3.11.16, 3.12.3, 3.12.14, 3.13.15, 3.14.7 and 3.15.0rc2 (first commit), all 7 rows exit 0 with no traceback, and the output is byte-identical across versions. `a=b` verifies. The other five rows each name their source with its reason. Re-run on the fix-round head, 4b21263, under 3.10.21, 3.12.3 and 3.14.7: byte-identical to the first commit's run. Re-run on the fix-round-2 head, 2ac911c, under the same three: all 7 rows exit 0 with no traceback, the output is byte-identical across the three versions, and it is identical to the re-review's runs of the previous head over main. - **Reach on main.** Main gives a `PermissionError` traceback, exit 1, for `scaffold --dry-run`, the project's `validate-manifest.py`, `validate-repository-naming.py --project` and its `--quiet` form, each with `SHAPE_PIN_SOURCE_OPENXDOX` under a locked parent. This PR gives exit 0 for all four. `validate-pins.py` on the sibling row is the exception said above. - **New tests.** 17 cases (11 functions) in the first commit; 16 fail on ab623dd. The fix round adds 31 cases in `tests/test_naming_policy.py`, and 17 of them fail on 40762ca: the five shapes of pins that are not a list (each at the CLI, in-process and, for the scalar, under `--project`), and the chains nobody reads. The other 14 guard what a mutation could break (below) or the side the fix must not move (an absent, null or blank pin list still answers). - **Targeted run on 4b21263**, files `test_naming_policy`, `test_manifest_validator`, `test_scaffold_referent_chain_links`, `test_cli_reference`, `test_repo_hygiene`, `test_neutral_pin_validator`, `test_scaffold_e2e`, `test_shape_doctor`, `test_git_output_decoding` and `test_pin_validator`: **765 passed, 3 skipped**. The 3 skips are the PowerShell-only rows. (40762ca: 734 passed, 3 skipped.) `render-cli-reference.py --check` is ok. - **Mutation probes, first commit.** Reverting the stat guard, the parse, keyed-over-directory, the NOTE lines, the chain's-end restriction, the sibling-absence filter, the manifest reason, the `--quiet` drop, the "read as a PATH" sentence, the `./` line, the project-chain read, the WARNING lines or the env reporting each fails at least one new test. - **Targeted runs on 2ac911c (fix round 2).** Under 3.12.3, `test_naming_policy` (238 cases: 210 and 28 new), `test_cli_reference`, `test_manifest_validator`, `test_scaffold_referent_chain_links`, `test_repo_hygiene` and `test_setup_project_py`: **468 passed**; and `test_scaffold_e2e`, `test_scaffold_pin_and_reuse`, `test_scaffold_leg_path_collisions`, `test_scaffold_pin_collisions`, `test_neutral_pin_validator`, `test_git_output_decoding` and `test_pin_validator`: **312 passed**. Under 3.10.21 and under 3.14.7, `test_naming_policy`, `test_manifest_validator` and `test_scaffold_referent_chain_links`: **321 passed** each. `render-cli-reference.py --check` is ok, and `ast.parse(feature_version=(3, 9))` passes on the three changed files. - **Mutation probes, fix round.** The review's ten, re-run on 4b21263: MW1, MW2, MW3, MW5 and MW11 now die (they survived), and MW4, MW7, MW12 and MW13 still die; MW6 is the dead test, removed. (Fix round 2 kills the three the re-review found, R1 to R3, and six more: see above.) Nine more for the new code (the list check, the entry check, the null and blank tolerances, `reads_link_declarations` always true, without the shape check, without the held pins, and the validator ignoring it): eight die. The ninth, `reads_link_declarations` ignoring whether the name has a referent, survives as an equivalent mutant: with no referent `_chain_shape_problem`'s last question always reports a problem, so the explicit early return is redundant, and is kept because `resolve_referent` has the same first line. The spy test holds the two together. - **Windows.** The mode-000 rows skip on Windows and as root, using a `SEARCH_DENIED` mark this file defines, with the same condition as #312's. The new FIFO and `/dev/null` rows skip where those do not exist. `a=b` and the 300-character name run everywhere. On Windows the long-name row asserts the line, not POSIX's reason. None of it had been run on Windows or macOS when this was written; the last paragraph gives the CI result. **Merge-tree.** Recomputed at 19:04Z with `git merge-tree --write-tree` of 2ac911c against `origin/main` as it was then, 53bee5b, and against every one of the other 37 pushed branches that touches a file this PR changes (a scan of each head against its merge-base with main): - **Clean:** origin/main 53bee5b (it carries #314 and #390); #394 `fix/adopt-record-submodule-placement` 563ea16 (`scripts/repo_shape.py` and `validate-manifest.py`) and #406 `fix/validate-family-unnamed-mounts` 369af2e (`scripts/repo_shape.py`); #397 abb27e6, #400 7062a47, #401 409f10b, #416 7cca975, #452 e2df9a8 and #395 1ef3310 (`docs/cli.md`); #355 6fdbe3d, #373 741cbf2 and #395 (`scaffold-project.py`). - **Conflict, four hunks each, all in `scripts/repo_shape.py`:** `fix/pin-source-env-collisions` (PR #409) 6494714, and `fix/adopt-pin-writes-pin-file` 421e4e5 (no PR). Both also touch `scaffold-project.py`, and that merges clean with this PR's one edit there. Fix round 2's new hunks merge with neither: #409's change to the "not reachable here" sentence (the `offered` text, when a variable is shared) and the reached branch sit in different lines of `_chain_link_findings`. The four hunks, each taking both sides: 1. the `resolve_link_source` signature: `default_source=None, env_shared: bool = False, *, tried: list | None = None`; 2. its docstring: both paragraphs; 3. the `link_pins_from_trees` docstring and the lines after it: both paragraphs, then `found`, this PR's `unreadable` and #409's `links` and `shared`; 4. the call: `resolve_link_source(name, root, keyed_sources, default_source, pin_product_key(name) in shared, tried=asked)` (the same spelling on both branches), followed by `if tried is not None: tried[name] = asked`. Resolved that way in a throwaway and committed there, the merge with 6494714 passed **496 tests** (`test_naming_policy`, `test_pin_source_env_collisions`, `test_neutral_pin_validator`, `test_scaffold_referent_chain_links`, `test_manifest_validator`, `test_cli_reference`), and the merge with 421e4e5 passed **559 tests** (the same files and `test_pin_source_unreadable`). `render-cli-reference.py --check` is ok on both, pyflakes is clean, and the grammar parses as 3.9's. Neither branch carried the resolution when this was measured. #409 has since landed, and the merge of main into this branch (the last paragraph) takes those four hunks both ways. The earlier counts (468 and 434 from the first fix round, 430 from the re-review) are those runs' own, at their heads. **Left out.** Each of these was found by lane 5's reviews of this change, and each is filed or folded: - **A bare `--link-source` answers for every link with no more specific source** (#441). In a chain of three or more links, one bare tree is read as two links' declarations, and a chain that holds is reported broken, a FINDING for a name with no direct pin on its referent. Pre-existing, the same on main; `validate-pins.py`'s `--pin-source` has the same documented rule and was not examined. - **An empty path after a key (`--link-source openXdox=`) is `Path("")`, the current directory** (#442). Run from a project root, the project's own `project.yaml` is read as the link's declaration. Pre-existing; `validate-pins.py`'s `--pin-source openXdox=` has the same shape (there a finding naming `.`). - **`validate-pins.py`'s `(sibling / ".git").exists()` is unguarded** (the sibling row above), folded into lane 2's #430, which already covers the Python 3.14 half: the pre-3.14 traceback and its reproduction are added there as a comment. #312's file. - **The project's OWN manifest** with `neutral_product_pins: 5` ends in a `TypeError` in two gates before either can say anything about it (#443): `_pins_from_project` in the naming gate (`scripts/validate-repository-naming.py:302` on main), and `_declared_pins` in the project's own `validate-manifest.py` (`templates/assembly-root/scripts/validate-manifest.py:85`), which ends it before its own `manifest-neutral-product-pins` finding (`_neutral_product_pins_type_finding`, `:524` on main) can print. Reproduced on 3ef059c. That is the project's own declaration, read by the project's own gates, and not a link source, so it is outside this change; "read by the project's own gates" covers two crashing gates. Also not done here: - **Python 3.9 (the floor) was not run** because it is not installed here. The grammar of the three changed files was checked with `ast.parse(feature_version=(3, 9))`. Windows and macOS were not run here; CI ran them (the last paragraph). Reviewed adversarially at 40762ca, the branch's first commit (CHANGES NEEDED: 0 blockers, 2 should-fixes, 4 nits). The head stopped every crash in the issue's table, and the should-fixes were a docstring claim the code did not keep for a `neutral_product_pins:` that is not a list, and reach claims that overstated what the project's gate does on the sibling row. Fix round 1 (4b21263) resolved every finding, and the re-review of 4b21263 read APPROVE WITH NITS (0 blockers, 0 should-fixes, 3 nits: three behaviour changes this description had not stated, two of them untested; a spy-test case for a qualified chain link; and description corrections). Fix round 2 (2ac911c) resolved those three, and its re-verification of 2ac911c read APPROVE WITH NITS (0 blockers, 0 should-fixes, 2 nits: no test of the scaffold's wider wording for a manifest that is not a mapping or does not parse, now pinned by two tests that each fail when the wording is narrowed, and a cross-reference in this description that pointed the wrong way, now corrected). Main (9736424, #409's squash) was merged into the branch as ced8fac. `scripts/repo_shape.py` conflicted in four hunks; each takes both sides, as the review of 2ac911c resolved it: `resolve_link_source(..., default_source=None, env_shared: bool = False, *, tried=None)`, both docstrings kept, `link_pins_from_trees` opening with `found`, `unreadable`, `links` and `shared`, and the call passing `pin_product_key(name) in shared` with `tried=asked`. Nothing else conflicted. After the merge, `tests/test_pin_source_env_collisions.py` and the targeted set passed (667 and 312), and `render-cli-reference.py --check` printed `[ok]`. CI is green on Linux, macOS and Windows, with SonarCloud passing, at 938a131 (main 48585e6 merged in). Its merge with main 994be39 passed the targeted set (the naming, manifest, scaffold, pin and setup test files): 983 passed. Closes #338 Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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: #365 (comment)
Claim: #360 (comment)
What and why
#365.
bumpandremovehad no rollback. When one of them failed after its first write, it exited 2 withREFUSED a git or gh command failed. Everything it had done stayed, and nothing it printed said so. Reproduced on 3ae0109 with a holderpre-commithook that exits 1:bumpleftM family.yaml,M members/Atlas, and the member checked out at--to;removeleftM .gitmodules,M family.yaml,D members/Atlas, and an empty member directory.With a held
index.lock,bumpleft its rewrite unstaged over the old gitlink, whichvalidate-family.pythen reported asmember-gitlink-mismatch.removewas worse:git submodule deinit, which takes no index lock, emptied the member and dropped its registration beforegit rmdied on the lock.addputs the holder back on the same failures (#203); these two verbs predate that.The coordinator's rulings, as built:
A rollback for both verbs.
bumpandremoverun insidePinRollback,MountRollback's rule foradd. Its generic steps are borrowed, and add's class is not touched.removealso: everysubmodule.*key in the holder's git config, the member's object store, its checked-out legs, and whethergit status --ignoredlists anything in it.update-index --index-infowith bytes;git checkout -qto the branch or the commit;git config --add;remove,git submodule update --init --no-fetch -- members/<P>, run only where the object store git keeps for the submodule's NAME is there and holds the gitlink's commit, and asgit -c protocol.allow=neverwith no terminal prompt, then the member's branch and its legs. Where git has no such store, the checkout is not put back, nothing is cloned, and the sentence says the checkout comes back only from its remote (fix round 1, below).add's kind of sentence. The exit stays 2, and a failed git command is quoted and headed exactly as before.deinit -fdiscarded (uncommitted, untracked and ignored files) is said to be gone, never called put back.removerefuses a heldindex.lock(family-index-locked) before it unmounts, at the pathgit rev-parse --git-path index.lockgives.Refusals asked first (the coordinator's addition). Whatever a dry run asks before its first write, the real run now asks there too, outside the rollback (
_checked_bump_target, andfamily.rewrittenincmd_remove). This fixes two cases:removeover amembers:line this tool will not rewrite, which used to be refused after the unmount;bump --toa commit whose tree has noproject.yaml, which used to be refused with the member left at--to.Both runs now refuse in the same words, with the holder byte for byte as it was.
--dry-runis otherwise unchanged. In particular it does not ask the lock.#360. Two sections for one path. git maps a path to ONE submodule name. When
.gitmodulesgavemembers/<P>to two sections,git rmtook out one, andremovecommitted the other and exited 0. Which one stayed depended on the order:.git/configregistration stayed;Now:
removerefusesmember-path-registered-twicebefore any git command that writes. Its dry run makes the same refusal, as the module's exit-code contract requires.addrefusesmember-path-registeredwhen a.gitmodulessection under another name already gives the mount path, andvalidate-family.pyfindsgitmodules-path-twice, soaddno longer commits two sections for one path that a recursive clone resolves to the later one, and a holder that already has such a ghost section fails its own gate (#300) #317's by-key removal for every section not named after the path. Whose registration it is stays the human's call, as inadd's model.pathlines. A section that gives the path and then moves on had taken the path out of git's lookup, sogit rmtook out nothing; that case is refused too.remove, when.gitmodulesgives the member's path to two sections, takes out only the one git finds by that path (the later in the file) and exits 0, so its commit still registersmembers/<P>with no gitlink and no row,validate-family.pypasses the result (after #317 it reported the doubled file and stops), and the nextaddof that member is refusedmember-path-registeredor silently reuses the leftover section and the.git/configregistrationdeinitnever touched #360 decision 3:member-path-registered's remediation no longer offersfamily.py remove. That refusal is asked aftermember-already-present, so no row holds the project when it fires, andremoveanswersmember-unknown. Probed on 3ae0109:addwas refusedmember-path-registered, thenremovegavemember-unknownfor the project and for the ghost.Tests
New
tests/test_family_bump_remove_rollback.py(19 tests). Every doubled.gitmodulesis built from bytes in git's objects, and every hook is a#!/bin/shscript set through a localcore.hooksPath.Rollback.
bumpandremoveunder apre-commithook that exits 1, and under a heldindex.lock. The holder's snapshot must be the same before and after:ls-files -s -z,status --porcelain=v1 -z;family.yamland.gitmodulesbytes;submodule.*keys;The same command then runs once the hook is gone.
removeedge cases:remove --dry-rununder the lock, which still exits 0.The coordinator's (a) and (b) (
removeandbumpover themembers:comment;bump --toa commit with noproject.yaml): the real run's stderr equals the dry run's, and the whole holder is identical, its objects and.git/configbytes included.family.py
remove, when.gitmodulesgives the member's path to two sections, takes out only the one git finds by that path (the later in the file) and exits 0, so its commit still registersmembers/<P>with no gitlink and no row,validate-family.pypasses the result (after #317 it reported the doubled file and stops), and the nextaddof that member is refusedmember-path-registeredor silently reuses the leftover section and the.git/configregistrationdeinitnever touched #360:removeandremove --dry-run, with the holder byte-identical;removeexits 0,HEAD:.gitmodulesgives the path to no section, neither name is registered, andvalidate-family.pypasses;Partial unwinds, for each verb: a lock taken during the unwind; the printed commands are then typed back and give the recorded holder.
Other failures: a commit that already landed undoes nothing, and Ctrl-C is put back and re-raised.
tests/test_family_dry_run.py:git rmdrops._gitmodules_names' overwrite model tested.remove, when.gitmodulesgives the member's path to two sections, takes out only the one git finds by that path (the later in the file) and exits 0, so its commit still registersmembers/<P>with no gitlink and no row,validate-family.pypasses the result (after #317 it reported the doubled file and stops), and the nextaddof that member is refusedmember-path-registeredor silently reuses the leftover section and the.git/configregistrationdeinitnever touched #360 row...._real_run_asks_first_too.tests/test_family_add_refusals.py:ghost_remediationdrops thefamily.py removeclause.Targeted run (Linux, git 2.43):
/usr/bin/python3 -m pytest tests/test_family_bump_remove_rollback.py tests/test_family_add_refusals.py tests/test_family_dry_run.py tests/test_family.py tests/test_family_member_path.py tests/test_cli_reference.py -qat 5d56e75: 269 passed, 0 failed, 0 skipped. macOS and Windows are CI's.Mutants, each run against the new file and
tests/test_family_dry_run.py(49 tests):bump's restore of the member's HEADremove'sgit submodule update --initrestoredeinitremove'smembers:check back after the unmount (a)bump's pre-checks (b)Not in this change
remove, when a single name gives the path in one section and then moves on in a later one, still takes out nothing. That is one submodule, not two. The dry run says "none ... left in place".removein a holder whose work tree has no.gitmodules(the index has one) exits 0 with a commit that deletes.gitmoduleswhole, every other member's section included. Probed on 3ae0109 and on this branch alike. It is not touched here.Fix round 1 (review 365-5d56e75)
Stacked on #400. #400 (
fix/family-add-dry-run,add --dry-run) lands before this PR and conflicts with it, so its head 7062a47, which carries #399 and main 3ef059c, is merged into this branch (merge commit 209f8b3). Main is not merged separately. The two conflicting hunks are resolved as the review gives them:add --dry-runmakes every refusaladdmakes before it mounts, reads the member from throwaway clones outside the holder for the row's id and digest, and prints the row, gitlink,.gitmodulessection and trailers without mounting, writing or committing anything, andaddtakes--trailerasbumpdoes, so every subcommand has the dry run the module'sEXIT CODESline promises (#326) #400's.gitmodulessentence, then this PR's held-index.locksentence, then family.py:add --dry-runmakes every refusaladdmakes before it mounts, reads the member from throwaway clones outside the holder for the row's id and digest, and prints the row, gitlink,.gitmodulessection and trailers without mounting, writing or committing anything, andaddtakes--trailerasbumpdoes, so every subcommand has the dry run the module'sEXIT CODESline promises (#326) #400's symbolic-linkproject.yamlsentence, followed by this PR's "A REFUSEDbumpORremovePUTS THE HOLDER BACK" paragraph;tests/test_family_dry_run.pyimports the union of the two lists.MUST-1: the rollback could clone. The guard checked the store the member's own
.gitnamed. git does not use that store: it usesmodules/<name>,<name>being the submodule.gitmodulesgives the path to, and it clones a store where there is none. With the section renamed by hand, the rollback cloned the member from its url. Now:_unrestorableasks which store git will use, through the existing_gitmodules_namesand_module_dir, and whether that store holds the gitlink's commit. Where it does not,git submodule updateis not run. The directory is made again, empty, so the gitlink does not read as deleted. The sentence names the store git would use, why it is not used, and the store the checkout had where that is still there, and says the checkout comes back only from its remote. No command it prints could clone..gitthatdeinitabsorbed into the name's store is restored from there, and the sentence says the member's object store has moved.git -c protocol.allow=neverwithGIT_TERMINAL_PROMPT=0(_offline_git), andupdatekeeps--no-fetch.protocol.allowyields to aprotocol.<name>.allowset by name and toGIT_ALLOW_PROTOCOL;--no-fetchand the store check cover that.submodule.<name>.*keys a failed restore leaves in.git/config(update --initregisters the name first) are named, with the commands that take them out.SHOULD-1: a member whose store is gone. The rollback's set-up read the member with no guard, so
removewas refusedgit-failedwith a leg's remediation while its dry run exited 0. The reads are guarded, and main's behaviour is back: the member is removed as its dry run previews. If a later step fails, the sentence says its checkout is in no store; the directory is made again, empty, and nothing is cloned.NIT-1:
.git/config. It is not written back byte for byte.deinitrewrites it, and a hook, another git command orupdate --initcan write it during the run, so nothing shows that the recorded bytes are all it should hold. The sentence now says the git config holds the same keys and values, with the section put back written last, and no longer calls the holder exactly as it was.NIT-2.
remove --dry-runasksfamily.rewrittenbefore itsls-filesread, in the real run's order.NIT-3.
family-manifest-unwritableis spelled once, in_rewrite_members, whichaddnow calls too._submodule_pathsno longer claims to be the only reader of.gitmodules; it names_gitmodules_namesand_sections_giving.The false sentence corrected. "run only while that store exists so it can never clone" was not true; the "What and why" list above now says what is.
Tests: 9 new in
tests/test_family_bump_remove_rollback.py(28 in all):GIT_ALLOW_PROTOCOL:protocol.allow=always, the clone is refused byprotocol.allow=never;protocol.file.allow=always, the fetch is stopped by--no-fetch;.git;Targeted run (Linux, git 2.43, no
GIT_ALLOW_PROTOCOL), on the bytes of 7cca975:/usr/bin/python3 -m pytest tests/test_family_bump_remove_rollback.py tests/test_family_add_refusals.py tests/test_family_dry_run.py tests/test_family.py tests/test_family_member_path.py tests/test_family_member_name.py tests/test_family_add_member_id.py tests/test_cli_reference.py tests/test_agents_md_codes.py -q: 383 passed, 0 failed, 0 skipped. macOS and Windows are CI's.Mutants, each run against the new file and
tests/test_family_dry_run.py(89 tests). The review's R6 and R7 survived its round; each is killed now:--no-fetchprotocol.allow=neverls-filesfirstLanding round (review 365-7cca975, MERGEABLE WITH NITS)
Merge of main (0737aa0). #400 landed on main as the squash a92e60b, while this branch carries #400 as a merge of 7062a47, so merging
origin/main(dd8d0ea) conflicts inscripts/family.pyandtests/test_family_dry_run.py, and in no other file. Both are resolved with this branch's side (git checkout --ours), and the proof was run before the merge was committed:add --dry-runmakes every refusaladdmakes before it mounts, reads the member from throwaway clones outside the holder for the row's id and digest, and prints the row, gitlink,.gitmodulessection and trailers without mounting, writing or committing anything, andaddtakes--trailerasbumpdoes, so every subcommand has the dry run the module'sEXIT CODESline promises (#326) #400's four files,git show a92e60b:<file>andgit show 7062a47:<file>are the same blob:docs/cli.md94b4003,scripts/family.py3bc79e6,tests/test_family_add_refusals.pybec2518,tests/test_family_dry_run.pyf418ccf. The branch's earlier merge of 7062a47 already holds all that the squash holds.git log a92e60b..origin/main -- <those four files>is empty: no later main commit touches any of them.Merge of #406 (14431df).
origin/fix/validate-family-unnamed-mountsat e83cc21 lands before this PR, and both edittests/test_family_add_refusals.py. The merge is clean, no file conflicted, and CI now tests the pair, so this PR can land right after #406 without another cycle. Until #406 lands, this PR's diff against main shows #406's files too.NIT-A: the backstop's two gaps (39e27ff).
protocol.allowyields to aprotocol.<name>.allowset by name and to the caller's ownGIT_ALLOW_PROTOCOL, so-c protocol.allow=neverdid not stop a clone for a caller whose environment or configuration allowed the file protocol (the review measured both with a local bare repository)._offline_gitnow setsGIT_ALLOW_PROTOCOLto the empty string, inOFFLINE_ENVbesideGIT_TERMINAL_PROMPT=0: git then allows no transport at all, whatever the caller's variable or a setting by name says. The restore reads local stores and needs no transport.-c protocol.allow=neverand--no-fetchstay as the layers under it, and the docstring says why.test_a_callers_protocol_settings_do_not_let_a_wrong_store_check_clone: the caller's environment hasGIT_ALLOW_PROTOCOL=fileandprotocol.file.allow=always, the store check is made wrong as in the backstop tests, the url is a local bare repository, and a premise check shows those settings do let a clone through under-c protocol.allow=never. The restore must not clone: git saystransport 'file' not allowedand no store appears.OFFLINE_ENVis a module constant so they take the variable off and keep proving that guard alone.GIT_ALLOW_PROTOCOLfromOFFLINE_ENV, and 1 of the 90 tests of the new file andtests/test_family_dry_run.pyfails: the new test, because the restore clones. Setting it tofileinstead fails the same test.NIT-B: the renamed-section sentence. It said the old store "is still there" and, at its end, that the checkout "comes back only from its remote". Where the sentence has named the old store, it now ends "The checkout of members/
can come back from the object store named above with no network, or from its remote; this command did neither." It prints no restore command, and none that could clone. Every other case keeps "comes back only from".
test_a_store_git_would_not_restore_from_is_never_usedpins the sentence, and fails 2 of 90 if the old closing words come back.What this supersedes in fix round 1 above: the "Backstop" bullet's last sentence, which now has the empty
GIT_ALLOW_PROTOCOLbeside--no-fetchand the store check, and "says the checkout comes back only from its remote", which is now true where no old store is there and says what is above where one is. "Main is not merged separately", under "Stacked on #400", no longer holds: main is merged, by the first merge above. NIT-C (GIT_TERMINAL_PROMPT=0has no offline test) is not taken.Targeted run (Linux, git 2.43, no
GIT_ALLOW_PROTOCOL), on the bytes of 39e27ff:/usr/bin/python3 -m pytest tests/test_family_bump_remove_rollback.py tests/test_family_add_refusals.py tests/test_family_dry_run.py tests/test_family.py tests/test_family_member_path.py tests/test_family_member_name.py tests/test_family_add_member_id.py tests/test_validate_family_unnamed_mounts.py tests/test_validate_family_member_path_glob.py tests/test_validate_family_without_git.py tests/test_validate_family_duplicates.py tests/test_cli_reference.py tests/test_agents_md_codes.py -q: 478 passed, 0 failed, 0 skipped.tests/test_family_bump_remove_rollback.pyholds 29 tests. macOS and Windows are CI's.Closes #365
Closes #360
🤖 Generated with Claude Code