Skip to content

shape-doctor.py: a gitlink beside a .gitmodules the leg's index tracks and this disk lacks is named from the index's copy, which git reads, with its own way out that restores the file first (git restore .gitmodules), as #1080 names one beside other sections (#1093) - #1109

Merged
brettheap merged 5 commits into
mainfrom
l3-1093-gitmodules-missing-on-disk
Oct 24, 2026

Conversation

@brettheap

@brettheap brettheap commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

What changed

A follow-up outside v1.2.0 (Brett Heap, 2026-10-21T10:32:04Z). It lands only after the v1.2.0 tag, on a later merge word.

The state #1093 asks for. A leg's index tracks .gitmodules, but this disk lacks it (deleted, the deletion not staged). git's config_from_gitmodules then reads the index's copy, and git submodule status exits 128 for a gitlink that copy does not register. At main the doctor named nothing there, because it read only the file on this disk.

The index's copy is read (shape-doctor.py):

Its own value and clause.

  • The leg's entry says unregistered_beside: index_copy (BESIDE_INDEX_COPY), documented beside the other leg keys and in the module docstring's leg-key prose.
  • index_copy_clause prints the way out: "first restore that .gitmodules in its leg (git restore .gitmodules), then give each a section with its path and url in it, or, once it is staged (git add .gitmodules), remove its gitlink with git rm --cached <its path in the leg> in its leg".
  • Why the restore comes first: where the file is gone, git add .gitmodules stages its deletion (measured: exit 0, and the index then tracks none), so every other submodule's section leaves the index with it. And no section can be written in a file that is not there.
  • Typed as printed in real legs (git 2.43), with the index's copy holding another path's section or none, in a leg with a commit and in one without:
    • git restore .gitmodules, git add .gitmodules, git rm --cached vendor/lib: each exits 0, the gitlink leaves the index, and the index's copy is unchanged;
    • git restore .gitmodules, a section appended, git add .gitmodules: each exits 0, then git submodule status exits 0.
    • The tests type both ways out the same way.
  • A recursive clone of a commit made in this state exits 0 and leaves vendor/lib empty, and git submodule status in the clone exits 128, as the clause says.

Note or finding: a note. Like #914's and #1080's clauses, it is said in the row and never a finding. The row stays the note it was, since the policy still asks about the tracked .gitmodules, or ok where submodules: records the gitlink (case deleted-and-recorded).

An unmerged .gitmodules still names nothing. git reads no copy then. With the file gone from this disk, the index holds no :.gitmodules (rev-parse --verify fails), so registered_in_the_index returns None. The limits sentence (module docstring, and unregistered_gitlinks) now says what the doctor really does, as RV1092e's NIT suggested: it names nothing there only while the copy on this disk is the conflict, registers the submodule, or is not there.

The module docstring's limits sentence no longer lists #1093's state.

Kept byte for byte. I compared every non-docstring string literal of shape-doctor.py with ast (RV1092d's strings.py), at main and at this head.

Decisions the direction left open

Out of scope

Tests

At 8ec615f4, one file at a time, under the stub gh, GH_TOKEN=bogus, an empty GH_CONFIG_DIR, GIT_CONFIG_GLOBAL=/dev/null and EMAIL exported:

file result
tests/test_doctor_plan_overrides.py 69 passed in 92.46s
tests/test_doctor_recorded_answers.py 29 passed in 67.71s
tests/test_doctor_next_is_a_command.py 61 passed in 112.88s
tests/test_shape_doctor.py (once) 445 passed, 3 skipped in 805.52s
tests/test_scaffold_leg_path_collisions.py 104 passed in 6.33s
tests/test_repo_hygiene.py 100 passed in 3.06s
tests/test_no_auto_maintenance.py 58 passed, 2 skipped in 10.19s
tests/test_agents_md_codes.py 47 passed in 2.36s
tests/test_cli_reference.py 23 passed in 39.13s
tests/test_update_add_codes_documented.py 6 passed in 0.58s

New cases in tests/test_doctor_plan_overrides.py:

  • test_a_gitlink_no_section_registers_is_said: deleted-and-registering, deleted-with-no-section, deleted-with-other-sections, deleted-and-recorded, deleted-read-to-its-first-bad-line, deleted-read-to-a-bare-path, deleted-and-refused-whole;
  • test_a_section_written_for_the_gitlink_registers_it[deleted-with-other-sections];
  • test_an_unmerged_gitmodules_this_disk_lacks_names_nothing.

pyflakes:

  • shape-doctor.py: no warning at main or at this head.
  • tests/test_doctor_plan_overrides.py: no new kind of warning. The new test's standard and project parameters add two more of the file's existing fixture-shadowing messages ("redefinition of unused 'standard'/'project'"), as every fixture-taking test there does.

Mutants

Each mutant was run with -k "no_section_registers or section_written or unmerged_gitmodules_this_disk" (28 cases). After each run, the file was restored and cmp said identical. Code with docstrings stripped (ast) is identical between the tree the mutants ran on and 8ec615f4.

mutant break result
M1 the new branch never taken KILLED, 5: deleted-with-no-section, deleted-with-other-sections, deleted-and-recorded, deleted-read-to-its-first-bad-line, deleted-read-to-a-bare-path
M2 no rev-parse --verify :.gitmodules first KILLED, 1: test_an_unmerged_gitmodules_this_disk_lacks_names_nothing
M3 HEAD's copy read in place of the index's KILLED, the same 5 as M1
M4 partial=False in registered_in_blob KILLED, 2: in-head-read-to-its-first-bad-line, deleted-read-to-its-first-bad-line
M5 working_tree=True in registered_in_blob KILLED, 1: deleted-read-to-a-bare-path
M6 a copy git refuses whole read as an empty set KILLED, 1: deleted-and-refused-whole
M7 index_copy_clause left out of held_clauses KILLED, the same 5 as M1
M8 the way out without the restore, in the doctor and the test's copy of the clause alike KILLED, 6: the 5 above (the typed check finds .gitmodules gone) and test_a_section_written_for_the_gitlink_registers_it[deleted-with-other-sections]

Probes

Each probe is a fresh leg, compared against git submodule status (git 2.43). In each row the index tracks .gitmodules, and this disk lacks it unless the case says otherwise.

case status rc main names head names, beside agree at head
idx-registers [0] [] [] None yes
idx-no-byte [128] [] ['vendor/lib'] index_copy yes
idx-only-a-comment [128] [] ['vendor/lib'] index_copy yes
idx-other-section [128] [] ['vendor/lib'] index_copy yes
idx-section-with-no-key [128] [] ['vendor/lib'] index_copy yes
idx-bad-line-before-its-section [128] [] ['vendor/lib'] index_copy yes
idx-bad-line-after-its-section [0] [] [] None yes
idx-bare-path-key [128] [] ['vendor/lib'] index_copy yes
idx-bare-url-then-path [128] [] ['vendor/lib'] index_copy yes
idx-path-then-bare-url [0] [] [] None yes
idx-refused-whole-update-registers [128] [] [] None NO
idx-refused-whole-update-other [128] [] [] None NO
idx-refused-whole-shallow-registers [128] [] [] None NO
idx-twice-lib-then-other [128] [] ['vendor/lib'] index_copy yes
idx-twice-other-then-lib [0] [] [] None yes
idx-split-section-other-last [128] [] ['vendor/lib'] index_copy yes
idx-two-gitlinks-one-registered [0, 128] [] ['vendor/two'] index_copy yes
idx-other-head-registers [128] [] ['vendor/lib'] index_copy yes
idx-registers-head-other [0] [] [] None yes
idx-other-no-commit [128] [] ['vendor/lib'] index_copy yes
idx-registers-no-commit [0] [] [] None yes
idx-registers-disk-other [128] ['vendor/lib'] ['vendor/lib'] other_sections yes
idx-other-disk-registers [0] [] [] None yes
idx-registers-disk-dir [128] [] [] None NO
idx-other-disk-dangling-link [128] [] [] None NO
unmerged-disk-missing [128] [] [] None NO
unmerged-disk-registers [128] [] [] None NO
unmerged-disk-other [128] ['vendor/lib'] ['vendor/lib'] other_sections yes
unmerged-disk-conflict-markers [128] [] [] None NO

Merges

At the first push (current ones are under "Review round 2"). git merge-tree --write-tree, after a fresh fetch:

Review round 1 (RV1109: READY)

RV1109 found no MUST. This round is commit ab140500, on top of 8ec615f4. It changes docstrings and tests only: the ast check of shape-doctor.py shows no non-docstring string changed, and with docstrings stripped the code is identical to 8ec615f4.

  • SHOULD-1, taken. gitmodules_beside_gitlink takes kept="deleted-over-head". It commits the opposite copy with the gitlink first, makes that commit the pin, then stages the case's copy and deletes the file from this disk. Two new rows use it:
    • deleted-over-head-registering: SECTIONED in the index, OTHER_SECTION in HEAD, nothing named;
    • deleted-over-head-with-other-sections: the reverse, named under index_copy.
  • RV1109's M3b, KILLED. M3b reads HEAD's copy where HEAD has one. With RV1109's mut_rv.py, both new rows fail (2 failed, 28 passed). M3 re-run: KILLED, 7 failed. Each time the file was restored and cmp says identical.
  • SHOULD-3, taken. Where .gitmodules is marked skip-worktree and missing from this disk, the gitlink is named (git reads the index's copy, status 128). But git restore .gitmodules exits 1 there, as git checkout -- .gitmodules does (measured). index_copy_clause's docstring and the module docstring now name this as a limit: family.py's gitmodules-sparse state.
  • NIT-2, taken. The unregistered_gitlinks docstring now says a recursive clone and git submodule status both exit 128 with bad config line where the copy holds a line git refuses.
  • NIT-5, taken. index_copy_clause's docstring says why git add .gitmodules stays: once a section is written for one submodule, git rm --cached of another exits 128 until the file is staged.
  • NIT-1, taken. This body replaces the one with the appended bot block.
  • Not taken: NIT-3 (one restore spelling across the tools, for a follow-up ruling) and NIT-4 (the pytest fixture idiom).
  • SHOULD-2, done in round 2 (e446ff58b, below): the BENIGN entry is in tests/test_path_guard_hygiene.py now that lane 1's tests/test_path_guard_hygiene.py and shape-doctor.py: a bare is_file/is_dir/exists guard in a shipped tool fails by file and line unless its _is_a or a reasoned allowlist covers it, and a project.yaml linked into an unsearchable folder is yaml-unreadable (#430) #1111 has landed on main.

Tests at ab140500 (one at a time, under the stub gh, with EMAIL exported):

file result
tests/test_doctor_plan_overrides.py 71 passed in 105.15s
tests/test_doctor_recorded_answers.py 29 passed in 81.69s
tests/test_doctor_next_is_a_command.py 61 passed in 146.85s
tests/test_agents_md_codes.py 47 passed in 3.56s

pyflakes: shape-doctor.py has no warnings. tests/test_doctor_plan_overrides.py has the same 30 messages as at 8ec615f4.

Review round 2 (RV1109 round 2: READY; SHOULD-2 and NIT-6 done)

Lane 1's #1111 (tests/test_path_guard_hygiene.py, #430 slice Z) merged to main at 2026-10-21T18:42:21Z as 038dafba9dbedd9405b316cc749a04dffe02bb36. This round is the merge of that main into this branch (1e733793f, no conflict) and one commit on top of it, e446ff58b. It changes a test table and a docstring: with docstrings stripped, ast shows shape-doctor.py identical to the merge.

Tests at e446ff58b (one at a time, under the stub gh, with EMAIL exported, timeout 2400):

file result
tests/test_path_guard_hygiene.py 49 passed in 8.68s
tests/test_shape_doctor.py 445 passed, 3 skipped in 569.83s
tests/test_scaffold_leg_path_collisions.py 104 passed in 3.73s
tests/test_doctor_plan_overrides.py 71 passed in 75.17s
tests/test_repo_hygiene.py 100 passed in 2.41s

tests/test_doctor_recorded_answers.py, tests/test_doctor_next_is_a_command.py and tests/test_agents_md_codes.py were not run again this round: they were run at ab140500, and since then only the merged #1111 code and the docstring rewrap changed in shape-doctor.py.

Mutant. The new BENIGN entry dropped: tests/test_path_guard_hygiene.py gives 1 failed, 48 passed in 9.34s, the same test as at the merge. The file was restored and cmp says identical.

Merges (git merge-tree --write-tree, after a fresh fetch):

Closes #1093

Lane: openreposhape-3 (openRepoShape-3)
🤖 Generated with Claude Code

Summary by Sourcery

Teach shape-doctor to diagnose unregistered gitlinks using the index's .gitmodules copy when the working-tree file is missing, and explain how to resolve them.

New Features:

  • Detect gitlinks whose missing working-tree .gitmodules is still tracked in the index but does not register them, and report the index copy as the source of truth.
  • Provide remediation guidance that restores the missing .gitmodules before either adding its submodule section or removing the gitlink.

Bug Fixes:

  • Align shape-doctor findings with Git's handling of missing, staged .gitmodules files, including precedence over HEAD and partial parsing behavior.

Enhancements:

  • Share .gitmodules blob registration logic across HEAD and index copies and document the remaining unmerged, refused-file, and sparse-checkout limits.

Documentation:

  • Update shape-doctor guidance and leg metadata documentation for index-backed .gitmodules cases.

Tests:

  • Add coverage for index-copy registration, conflicting HEAD/index contents, malformed files, remediation commands, and unmerged .gitmodules behavior.
  • Extend path-guard hygiene coverage for the new filesystem guard.

…cks and this disk lacks is named from the index's copy, which git reads, with its own way out that restores the file first (`git restore .gitmodules`), as #1080 names one beside other sections (#1093)

#1092 read the `.gitmodules` on this disk, so where the leg's index
tracks one and the disk lacks it (deleted, not staged) the doctor named
nothing, though git reads the index's copy and `git submodule status`
exits 128 beside one with no section for the gitlink.

- `unregistered_gitlinks` reads the index's copy there
  (`registered_in_the_index`: `:.gitmodules`, up to its first bad line,
  as `gitmodules_entry` reads it) and names each gitlink it does not
  register, whether it holds no section or sections for other paths,
  under its own `unregistered_beside: index_copy`.
- `index_copy_clause` says it, a note as #914's and #1080's are: restore
  the file first, then a section in it, or the staged `git rm --cached`.
  Each way out typed as printed exits 0, and the index's copy keeps every
  other section.
- An unmerged `.gitmodules` still names nothing (git reads no copy then);
  the limits sentence now says when.
- HEAD's and the index's copy are read by one helper,
  `registered_in_blob`.

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

sourcery-ai Bot commented Oct 21, 2026

Copy link
Copy Markdown

Reviewer's Guide

Updates shape-doctor.py to identify unregistered gitlinks using the index’s .gitmodules copy when the working-tree file is deleted, emits a restore-first remediation, and adds comprehensive tests for classification, limits, and recovery behavior.

Sequence diagram for detecting unregistered gitlinks from the index copy

sequenceDiagram
    participant Doctor as shape-doctor.py
    participant Index as Git index
    participant Git as Git
    participant Status as git submodule status

    Doctor->>Doctor: unregistered_gitlinks()
    Doctor->>Doctor: Check .gitmodules is tracked and missing on disk
    Doctor->>Git: registered_in_the_index()
    Git->>Index: rev-parse --verify :.gitmodules
    Index-->>Git: Index blob or no entry
    alt Index copy exists
        Doctor->>Git: registered_in_blob(:.gitmodules)
        Git-->>Doctor: Sections up to first bad line
        Doctor->>Doctor: Name gitlinks not registered by index copy
        Git->>Status: Read index .gitmodules
        Status-->>Git: Exit 128 for unregistered gitlink
    else .gitmodules is unmerged
        Git-->>Doctor: None
        Doctor->>Doctor: Name nothing
    end
Loading

Flow diagram for index-copy remediation

flowchart TD
    A[.gitmodules tracked in index but missing on disk] --> B[Read index copy with registered_in_the_index]
    B --> C{Does index copy register gitlink?}
    C -->|Yes| D[Do not name gitlink]
    C -->|No| E[Emit index_copy note]
    E --> F[git restore .gitmodules]
    F --> G{Choose remediation}
    G -->|Keep gitlink| H[Add path and url section]
    G -->|Remove gitlink| I[git add .gitmodules]
    I --> J[git rm --cached <its path in the leg>]
Loading

File-Level Changes

Change Details Files
Teach the doctor to inspect the index’s .gitmodules blob when the tracked file is missing from the working tree, matching Git’s submodule-status behavior.
  • Add an index_copy beside classification for unregistered gitlinks.
  • Read :.gitmodules with partial parsing and preserve the unmerged and whole-file-refusal limits.
  • Refactor HEAD and index blob handling through a shared helper.
shape-doctor.py
Provide a dedicated remediation clause for gitlinks whose registration is missing from the index copy.
  • Add the index_copy_clause to generated plan clauses.
  • Recommend restoring .gitmodules before either adding its section or staging and removing the gitlink.
  • Document the new value and behavior in module and function docstrings.
shape-doctor.py
Expand plan-override coverage for deleted working-tree .gitmodules states and verify the prescribed recovery commands.
  • Cover registering, absent, alternate-section, malformed, refused, recorded, and unmerged cases.
  • Assert row status, beside metadata, exact clause text, and preservation of the index copy.
  • Test both adding a section and removing the gitlink after restoration.
tests/test_doctor_plan_overrides.py

Assessment against linked issues

Issue Objective Addressed Explanation
#1093 When a leg’s index tracks .gitmodules but the working-tree file is absent, read the index copy and name any gitlink it does not register, whether that copy has no section or only sections for other paths. ✅
#1093 Report the missing-index-copy case with its own unregistered_beside value and a recovery clause that restores .gitmodules before registering or removing the gitlink. ✅
#1093 Preserve the intended limits around unmerged or unreadable .gitmodules copies, and test the new detection and recovery behavior. ✅

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@brettheap

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-3, 2026-10-21T13:39:08Z) Independent review RV1109 of #1109 at 8ec615f: READY (no MUST). Three SHOULDs before the landing word.

SHOULD-1 (tests): no test checks that the index's copy is read before HEAD's. Every deleted row has no .gitmodules in HEAD, so a mutant that reads HEAD's copy when HEAD has one passes all 28 #1093 cases. In a probe that mutant names a registered gitlink (index registers, HEAD does not) and misses an unregistered one. Fix: add kept="deleted-over-head" to gitmodules_beside_gitlink (commit the opposite copy with the gitlink, pinned_at_its_head, then stage body and unlink), with rows (SECTIONED, False, "deleted-over-head", None) and (OTHER_SECTION, False, "deleted-over-head", "index_copy"), and widen the two kept == "deleted" checks (:635, :656). Checked in a scratch clone: 24 passed at the head; the mutant fails both new rows.

SHOULD-2 (land order with #1111): the textual merge is clean, but on main + #1111 + #1109, tests/test_path_guard_hygiene.py fails on the new bare guard at shape-doctor.py:8865, os.path.lexists(mount / adopt.GITMODULES) in unregistered_gitlinks. Whichever lands second adds to BENIGN: (SD, "unregistered_gitlinks", "os.path.lexists(mount / adopt.GITMODULES)"): ("ASKED", "a leg git listed the index of first, by lstat").

SHOULD-3 (skip-worktree): with .gitmodules marked skip-worktree and missing from disk, git reads the index's copy (status 128) and the doctor names the gitlink, but the first printed step, git restore .gitmodules, exits 1 (pathspec did not match). Name this as a limit in index_copy_clause's docstring and the module docstring (family.py's gitmodules-sparse state), or file a follow-up.

NITs: (1) Sourcery appended "## Summary by Sourcery" after the robot line (it contains U+2019); restore the body from W1093's pr-body.md before landing. (2) shape-doctor.py:8850 "exits 0 ... no submodule mapping found" is not true where the copy has a bad line before the section: the clone exits 128 with "bad config line". (3) family.py prints git checkout -- .gitmodules for the same restore; both behave the same, and #1093 asked for git restore, so one spelling is a follow-up ruling. (4) pyflakes: two more fixture "redefinition of unused" warnings, the file's usual idiom. (5) Say in the docstring why the git add .gitmodules step stays: without it, git rm --cached exits 128 once a section was written for another submodule.

Checked: every probe disagreement with git submodule status and a recursive clone is a named limit (refused, link, unmerged). Every printed step exits 0 and status exits 0 afterwards. The unmerged sentences are true. The ast check gives 0 strings removed or changed and 8 added; registered_unindexed behaves as before (probe_d identical). Sonar QG OK, duplication on new code 0.0%. Tests: overrides 69, recorded_answers 29, next_is_a_command 61, agents_md_codes 47, all passed. Mutants M1, M3 and M8 killed. Merges with main and #1111 are clean. Body: closes [1093], draft, no milestone.

Lane 3's plan: READY, with a fix round before the landing word. In the next usage window W1093, the writer, takes SHOULD-1 (the deleted-over-head rows), SHOULD-3 (the skip-worktree state named as a limit), NIT-2 and NIT-5 (the docstring words), and NIT-1 (the body restored). RV1109 then checks the new head. SHOULD-2: #1111 (lane 1, for #430, in v1.2.0) lands before this follow-up, so #1109 adds the BENIGN entry when it is brought up to a main that holds #1111. NIT-3 (one restore spelling across the tools) is a possible follow-up ruling, and NIT-4 needs nothing. #1109 stays a draft with no milestone and lands after the v1.2.0 tag, on a later word.

Lane: openreposhape-3 (openRepoShape-3)

…s` gives the other answer hold that the index's copy is read first, and the docstrings name the skip-worktree limit and why the staging step stays (#1093)

Review round 1 (RV1109: READY, no MUST).

- SHOULD-1: `gitmodules_beside_gitlink` takes `deleted-over-head`, which
  commits the opposite copy with the gitlink, makes it the pin, then
  stages the case's copy and deletes it from disk. The rows
  `deleted-over-head-registering` and
  `deleted-over-head-with-other-sections` kill RV1109's M3b (HEAD's copy
  read where HEAD has one).
- SHOULD-3: `.gitmodules` marked skip-worktree is named, but `git restore
  .gitmodules` exits 1 there; `index_copy_clause` and the module
  docstring name it as a limit (family.py's `gitmodules-sparse`).
- NIT-2: a recursive clone exits 128 with `bad config line` where the
  copy holds a line git refuses.
- NIT-5: `git add .gitmodules` stays, since `git rm --cached` exits 128
  once a section was written for another submodule.

Docstrings and tests only: no non-docstring string of shape-doctor.py
changed (ast).

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

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-3, 2026-10-21T14:18:29Z) RV1109 round 2 at ab14050: READY.

NITs: shape-doctor.py:8868 is 88 characters after the rewrap. The SHOULD-2 note covers only #1111 landing first; if #1109 lands first, #1111 carries the BENIGN entry.

Lane 3's plan: #1109 is READY at ab14050. It is a follow-up that lands after the v1.2.0 tag, on a later word, and #1111 (lane 1, for #430, in v1.2.0) is due before it. So #1109 carries the BENIGN entry (SHOULD-2) when it is brought up to a main that holds #1111, and NIT-6's rewrap goes in that same commit, which RV1109 then checks. If the order turns out the other way, lane 3 tells lane 1 on #1111 that #1111 carries the entry (NIT-7).

Lane: openreposhape-3 (openRepoShape-3)

Brett Heap and others added 2 commits October 21, 2026 21:16
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…w `os.path.lexists(mount / adopt.GITMODULES)` in `unregistered_gitlinks` is on `BENIGN` as asked after git listed the leg's index, so the path-guard test passes with #1111 merged, and the docstring paragraph on a recursive clone is wrapped to 79 (#1093)

Review round 2 (RV1109: SHOULD-2 and NIT-6), on top of the merge of
origin/main at 038dafb (#1111).

- SHOULD-2: with #1111 merged, `test_no_bare_guard_outside_a_stat_helper_or_the_allowlist[shape-doctor.py]`
  failed on the bare guard at `unregistered_gitlinks`. The entry sits in
  `BENIGN` with the table's alphabetical order, tagged ASKED: the guard is
  asked only after `.gitmodules` is among the paths git listed in the leg's
  index (`leg_tracked_files`), by `lstat`, in a mount `mount_read` read.
- NIT-6: the `unregistered_gitlinks` docstring paragraph that began at 88
  characters is rewrapped to 75. With docstrings stripped, `ast` shows
  shape-doctor.py identical to the merge.

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

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-3, 2026-10-21T23:04:07Z) RV1109c (round 3, fresh reviewer) at e446ff5: READY. No MUST, no SHOULD.

NITs: (8) the body's round-2 Merges line says no other open PR touches these files; #1144 does now. Add the line above at the next body edit. (9) 1e73379 and e446ff5 carry a Co-Authored-By naming Claude Sonnet 5.5, where writer-common asks for Opus 5.5. The squash replaces both commits, so choose the trailer in the squash message.

Lane 3's plan: #1109 is READY at e446ff5, and its SHOULD-2 owed to #1111 is done. NIT-8's line goes into the body at the next body edit, which is the landing's update-branch after the v1.2.0 tag. Whichever of #1109 and #1144 lands second re-checks their merge. The trailer is set in the explicit squash message. #1109 stays a draft with no milestone until then, and lands on a later word.

Lane: openreposhape-3 (openRepoShape-3)

@brettheap brettheap added this to the v1.2.1 milestone Oct 23, 2026
brettheap added a commit that referenced this pull request Oct 23, 2026
…git dies on read the `.gitmodules` git reads, the index's where the work tree's is gone, and every section's lines end `git add -- .gitmodules`, so a plain `git commit` after them takes the section out of HEAD (#1077) (#1124)

## What this does

RV1069's review of #1069 (the PR for #1036) left four follow-ups, recorded in #1077's body (NIT 1, NIT 2) and in issuecomment-6303768948 (NIT 3, NIT 4). Each was measured on main before this change. The only script changed is `templates/family-root/scripts/validate-family.py`, which has no pinned copy.

- **NIT 1, `gitmodules-path-twice` read the work tree's `.gitmodules` alone.** A second section for `members/IRRS`, staged in the index's file with the work tree's deleted, was `family ok` in every mode, and a clean clone of the commit after it failed its update and its gate.
  - It reads the sections as git reads them now (`_sections_as_git_reads`), as `gitmodules-path-unnamed` does since #1036.
  - Where the work tree's file is gone, it names the line that puts back the file git reads first. That put-back is recorded, so no later finding puts the file back over the hand edit.
  - It ends "then stage and commit .gitmodules".
- **NIT 2, the value check read the work tree's file and HEAD's, never the index's.** An `update = !true` staged in the index's file alone, in a named member's section, was `family ok` while git's own `submodule status` exited 128.
  - It reads the file git reads (`_listed_as_git_reads`), and says "the index's file, which git reads here" where that is the one read.
  - Its lines put that file back, not HEAD's. They end "then stage", or "then stage and commit" where HEAD's gives the value too.
  - Where HEAD's file alone gives the value and the work tree's is gone, it says "Commit the index's .gitmodules, which git reads here and does not give it", with no line, so a fix staged there is kept.
  - `member-gitmodules-absent` then puts the file back itself: `_check_gitlinks_registered` asks `Report.put_back` alone.
  - Git is asked in the order it was before (the work tree's file, then HEAD's, then the one read in its place), so with no git to run, `git-not-runnable` names what it named.
- **NIT 3, a plain `git commit` after a section's lines.** Where HEAD's file held the section, `gitmodules-path-unnamed`'s lines ended "then commit .gitmodules" and staged nothing. A plain `git commit` after them left the section in HEAD, and a clone's gate failed. Every branch (in `members/`, outside it, and in another case) now prints `git add -- .gitmodules` after the keys, and says "then stage and commit" where HEAD holds the section.
- **NIT 4, a test gap.** No row covered the case-variant branch's stage line where HEAD holds no section of the name. RV1069's probe `test_r2_stray_not_in_head` is that row now.

## New rows: `tests/test_validate_family_index_gitmodules.py`

Every row types only the printed lines and does by hand only what the text says in words, then makes a plain `git commit`. The gate and a plain clone then pass.

- N1: a second section in the index's file alone.
- N2: a value in a named member's section, in the index's file alone.
- N2b: HEAD's file gives the value, and the index's is mended.
- N2c: as N2b, with OTHER's section out of both files, so `member-gitmodules-absent` puts the index's file back first.
- N3: a section HEAD holds (a stale one, or `tools/x` with its gitlink), with the file on disk, deleted, or its deletion staged.
- N4: the stray `ghost` (RV1069's probe), on disk and deleted.
- Fix round 1: path-twice's put-back is recorded, so `gitmodules-path-unnamed` after it prints none (RV1124's P7, on `deleted` and `deletion-staged`); and the value's url is written back from the file git reads, not HEAD's (RV1124's P10). See the fix-round section below.

**Before and after.** On main's script the file gives 10 failed and 2 passed. The 2 are N4's rows, which pass there by design (NIT 4 is a test gap) and kill the case-variant mutant M8 below. On this head it gives 12 passed. After fix round 1 the file has 15 rows: on main's script it gives 13 failed and 2 passed (the 2 are N4's), and on this head 15 passed.

## Older rows changed, and why

Each one pinned the behaviour #1077 names as the defect.

- **"then commit" with no stage line (NIT 3).**
  - `SECTION_REMEDY` and a new `STAGE` constant (`test_validate_family_unnamed_mounts.py`), and its rows (b) and (c), plus the two-sections row.
  - `THEN_COMMIT` and `section_first`, which always ends with the stage line now (`test_family_unnamed_gitlink.py`), and its two-sections and control rows.
  - Two `removal(...)` assertions in `test_validate_family_remedies_typed.py`.
  - `ghost_out`, R1's replace anchor and R6 in `test_family_hand_mount_git_rm_lines.py`.
  - Each section in these rows is committed, so HEAD holds it: a plain commit after the old lines left it there.
- **Path-twice's ending (NIT 1 and NIT 3).** `test_family_add_refusals.py`'s path-twice row ends "then stage and commit": the commit records the index's file, not the work tree's hand edit.
- **HEAD's file put back where git reads the index's (NIT 2).**
  - `taken_out(put_back=...)` and the two gone-file value rows in `test_family_gitmodules_values.py`. These now read `HERE_AND_HEAD`; one is renamed `..._put_it_back_once`, since its old name said HEAD's file is put back.
  - The first line of `test_round1_nit_6` in `test_family_holder_remedies_compose.py`.
  - The value row in `test_family_holder_bootstrap_texts.py`, renamed `test_a_value_finding_that_put_the_file_back_is_not_put_back_over`.
  - These are now `git checkout -- .gitmodules`, the index's file.
- **#1036's R5 control (`test_1036_control_a_value_git_dies_on_keeps_what_it_read`).**
  - Round 1 put back HEAD's file, which dropped IRQS's section that only the index's file gave.
  - The value's lines put back the index's file now, so the commit after round 1 records that section, as it does with the file on disk. The next run names it, in every mode.
  - The row now types round 2 as well, then a plain `git commit`; nothing is left, and the gate and a plain clone pass.
  - What the control pins is unchanged: where git reads a file that gives a value git dies on, `gitmodules-path-unnamed` reads nothing from it.

## Gates

Every run used `/usr/bin/python3 -m pytest -p no:cacheprovider` with:
- `EMAIL` set;
- `GIT_ALLOW_PROTOCOL`, `GIT_SSH_COMMAND` and the `LANES_*` variables unset;
- an empty `GIT_CONFIG_GLOBAL` and `GIT_CONFIG_NOSYSTEM=1`;
- test author and committer identities;
- a stub `gh` first on PATH, `GH_TOKEN=bogus` and an empty `GH_CONFIG_DIR`.

- **Targeted set: 78 files.**
  - The files that name `gitmodules-path-twice`, `gitmodules-value-refused`, `gitmodules-path-unnamed` or `validate-family` in the head's tree, plus every test file that imports from a test module this changes.
  - This includes `test_pinned_copy_text`, `test_repo_hygiene`, `test_cli_reference` and `test_agents_md_codes`.
  - Result: 0 failed in all 78 files, 4390 passed and 34 skipped.
  - Shards 0 to 4 and 6 to 9 (70 files) were run at `0cd1c3f5`, the three WIP commits this head squashes (the same tree, and the same sha256 of the script): 3379 passed, 25 skipped.
  - Shard 5 (8 files) was run last, at this head (`672845cc`): 1011 passed, 9 skipped, 0 failed. Two earlier runs of it were cut by checkpoints, at 49% and at 28%, with 0 failures both times. Its files are `test_doctor_legs_members_next.py`, `test_family_add_spelling_leftovers.py`, `test_family_docs_one_spelling.py`, `test_family_makefile_siblings.py`, `test_family_remove_gitmodules.py`, `test_shape_doctor.py`, `test_update_shape_kept_helper.py` and `test_validate_family_unreadable.py`.
  - The PR's CI `tests` job runs the full suite.
- **Older rows.** The 33 rows the change moved, which failed on the new script before their update, now pass with the new rows file (45 passed).

## Mutants

Each mutant was run against the new rows file and the older rows that pin the changed paths. The file was restored after each one, and its sha256 checked against the head's.

| Mutant | Rows that failed |
|---|---|
| M1: the value check reads the work tree's file alone | 8 (N2, N2b, N2c, two gitmodules_values rows, round1 NIT 6, the bootstrap_texts row, R5) |
| M2: path-twice reads the work tree's file alone | 1 (N1) |
| M3: no "commit the index's" branch where HEAD's alone gives the value | 2 (N2b, N2c) |
| M4: the value's put-back is HEAD's, as before | 6 |
| M5: the value's text does not say to stage the file | 1 (N2) |
| M6: no stage line in `members/` | 7 |
| M7: no stage line outside `members/` | 3 |
| M8: no stage line in the case-variant branch (NIT 4) | 2 (N4 on disk, deleted) |
| M9: "then commit" where HEAD holds the section | 10 |
| M10: path-twice names no put-back | 1 (N1) |
| M11: path-twice ends "then commit" | 3 |
| M12: `member-gitmodules-absent` puts nothing back after any value finding | 1 (N2c) |
| M13: the file git reads is asked before HEAD's | 3 (the without_git rows) |

## Merge-trees

`git merge-tree --write-tree` against `origin/main` 823624d is clean, and was clean against 6da757c, the base the gates ran on. Main has since taken #1101 and #1102; neither touches a file of this PR. No open PR touches these files. #1109, the closest in subject, merges clean too.

## Follow-up, not in this PR

RV1124 measured the suspected counterpart of NIT 3 (its N1) and two more older defects (N2 and N6). All three are older than this PR and self-correcting or known limits; the lane files them as the follow-up #1150, and none is fixed here.

- N1: the work tree's `.gitmodules` is on disk, and both it and the index's give a value HEAD's does not. The value's lines say nothing of staging, so a plain `git commit` after them records the index's value; the next run names it in HEAD. Main prints the same text and gives the same result.
- N2: an index blob with a line git cannot parse after a value is `family ok` (`_blob_listed` returns nothing on git's exit 128).
- N6: an unmerged `.gitmodules` (stages 1 to 3), which v1.1.1's release notes list as a known limit.

## Fix round 1: RV1124's review of `672845cc` (S1, N3, N4, N5)

RV1124's review of this PR at `672845cc` found one SHOULD and six NITs. This round fixes S1, N3, N4 and N5, in one commit on top of it (`8dfa32bd`), with no amend and no force. N1, N2 and N6 are left to the follow-up #1150.

- **S1 (SHOULD): #1111's `test_path_guard_hygiene.py` failed the new guard.** `_listed_as_git_reads` opened with a bare `os.path.lexists(root / GITMODULES)`. Its one caller, `_check_gitmodules_values`, asks it only where the work tree's file is gone (`if present else _listed_as_git_reads(root)`), and `_gitmodules_put_back`, which the function calls next, asks the same `lexists` and is on the test's allowlist. The guard is dropped; the function is `spec = _gitmodules_put_back(root)[0]` and the blob read. On the scratch merge of main `038dafba9` and this head, `test_path_guard_hygiene.py` gives 49 passed, with no failure for this file and no stale `BENIGN` entry.
- **N3: two behaviours had no row.** Mutants A and B survived all 439 rows of the PR's nine files. Two rows, written from RV1124's probes P10 and P7:
  - `test_1077_a_url_git_dies_on_is_written_back_as_the_file_git_reads_gave_it` (P10): the index's file gives a url that is not HEAD's, then a `url` with no value; the line that writes the url back takes it from the index's file.
  - `test_1077_path_twice_records_its_put_back_for_the_next_finding` (P7, on `deleted` and `deletion-staged`): HEAD's file holds `second` and a stale `members/Old`; path-twice's put-back is recorded, so `gitmodules-path-unnamed` after it prints none, and typed in order then a plain `git commit`, HEAD loses both.
- **N4: path-twice's wording.** "back from HEAD's, as the index holds none first" put "first" on the wrong phrase. It says "back first from {whose}, `{put_back}`" now. Only `TWICE_PUT_BACK` in the new rows file pinned the old phrase.
- **N5: a stale docstring.** `_gitlink_out_first` said its text ends "then commit .gitmodules"; it now says it ends as `_section_then` does, "then stage and commit" or "then stage".

**Rows.** The new rows file has 15 rows. On main's script (`6da757cb`) it gives 13 failed and 2 passed (the 2 are N4's, by design). On `672845cc` it gives 14 passed and 1 failed: the first row (`test_1077_a_second_section_the_index_alone_gives_is_path_twice`) fails on N4's old wording. The P7 and P10 rows pass on `672845cc`, whose code was right; they kill the mutants below. On this head, 15 passed.

**Mutants.** Each ran in its own worktree over the new rows file, and the script was restored after each and its sha256 checked.

| Mutant | Rows that failed |
|---|---|
| A: the value's write-back from HEAD's where the work tree's file is gone | 1 (the P10 row: the last line wrote the HEAD url, not the staged one) |
| B: path-twice names its put-back without recording it (`_file_put_back(root, None)`) | 2 (the P7 rows: `gitmodules-path-unnamed` printed the put-back again) |

**Gate.** On a scratch merge of main `038dafba9` and `2d4cb64b`, whose tree is this head's: the usual environment (`EMAIL`, an empty `GIT_CONFIG_GLOBAL`, `GIT_CONFIG_NOSYSTEM=1`, test identities, a stub `gh` first on PATH, `GH_TOKEN=bogus`, an empty `GH_CONFIG_DIR`). 0 failed, 3458 passed and 27 skipped in all.
- `test_path_guard_hygiene.py`: 49 passed.
- The 64 files of every `tests/test_family*.py` and `tests/test_validate_family*.py` (the PR's nine among them), plus `test_repo_hygiene.py`, `test_agents_md_codes.py`, `test_cli_reference.py`, `test_pinned_copy_text.py` and `test_doctor_validator_rows.py`: 2964 passed and 24 skipped, in four shards (685; 997 and 20 skipped; 557 and 4 skipped; 725). Those four shards were cut by a checkpoint and re-run on the same merge with the same lists.
- `test_shape_doctor.py`, which runs `validate-family.py`, in 8 shards by test id, each list non-empty: 445 passed and 3 skipped.

**Merge-trees.** `git merge-tree --write-tree origin/main HEAD` at `8dfa32bd` against main `038dafba9` is clean (tree `23c7194f0b6a08f56b93bdc80a05924e90e703a4`).

Closes #1077

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

Copy link
Copy Markdown

@sourcery-ai

sourcery-ai Bot commented Oct 24, 2026

Copy link
Copy Markdown

Sorry @brettheap, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 10 hours and 10 minutes by commenting @sourcery-ai review. Upgrade to get a review now.

@brettheap
brettheap merged commit e9a1650 into main Oct 24, 2026
5 checks passed
@brettheap
brettheap deleted the l3-1093-gitmodules-missing-on-disk branch October 24, 2026 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant