[brettheap]

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 validate-family.py (family root): gitmodules-path-unnamed reads only the work tree's .gitmodules, so where that file is gone, a section the index still holds passes the gate and is committed #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.
• validate-family.py (family root): gitmodules-path-unnamed reads only the work tree's .gitmodules, so where that file is gone, a section the index still holds passes the gate and is committed #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): 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'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

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

Summary by Sourcery

Align .gitmodules validation and remediation with Git’s effective file reads, while making all section fixes safe for ordinary commits.

Bug Fixes:

• Make .gitmodules validation inspect the same worktree, index, or HEAD content that Git reads, detecting duplicate paths and invalid values in staged-only files.
• Preserve Git-read .gitmodules content during remediation and ensure generated fixes retain valid staged changes.
• Ensure section-removal guidance stages .gitmodules before committing so plain commits cannot leave invalid sections in HEAD.

Enhancements:

• Add comprehensive regression coverage for index-only .gitmodules content, missing worktree files, remediation ordering, case variants, and plain-commit behavior.

Tests:

• Expand validation tests to cover Git's index/HEAD fallback behavior, staged remediation commands, clone gates, and mutation scenarios.

[sourcery-ai]

Reviewer's Guide

Updates validate-family’s .gitmodules checks to follow Git’s work-tree/index/HEAD read precedence, preserves the file Git actually reads during remediation, and ensures every printed section fix stages the resulting file so a plain commit produces a valid repository, with comprehensive regression coverage for all four follow-ups.

Sequence diagram for Git-aware .gitmodules validation and remediation

sequenceDiagram
    participant Validator
    participant GitmodulesChecks
    participant Git
    participant Report

    Validator->>GitmodulesChecks: _check_members(root, members, no_members, report)
    GitmodulesChecks->>GitmodulesChecks: _check_gitmodules_values(root, members, report)
    GitmodulesChecks->>GitmodulesChecks: _listed_as_git_reads(root)
    alt .gitmodules exists in work tree
        GitmodulesChecks->>Git: _gitmodules_listed(root)
        Git-->>GitmodulesChecks: work tree file
    else work tree file is absent
        GitmodulesChecks->>Git: _gitmodules_put_back(root)
        Git-->>GitmodulesChecks: index or HEAD spec
        GitmodulesChecks->>Git: _blob_listed(root, spec)
        Git-->>GitmodulesChecks: index or HEAD file
    end
    GitmodulesChecks->>Report: finding gitmodules-value-refused
    GitmodulesChecks->>Report: _file_put_back(root, report)
    GitmodulesChecks->>GitmodulesChecks: _check_gitmodules_paths(root, sections, report)
    GitmodulesChecks->>Report: finding gitmodules-path-twice
    GitmodulesChecks->>Report: _file_put_back(root, report)
    GitmodulesChecks->>Report: print stage and commit .gitmodules

Loading

Flow diagram for .gitmodules read precedence and staging

flowchart TD
    A[validate-family checks .gitmodules] --> B{Work tree .gitmodules exists?}
    B -->|yes| C[Read work tree file]
    B -->|no| D[_listed_as_git_reads]
    D --> E{Index file available?}
    E -->|yes| F[Read index file]
    E -->|no| G[Read HEAD file]
    C --> H[_check_gitmodules_values]
    F --> H
    G --> H
    H --> I{Value git dies on?}
    I -->|yes| J[_file_put_back]
    J --> K[Print git config remediation]
    K --> L{HEAD also gives value?}
    L -->|yes| M[Print then stage and commit .gitmodules]
    L -->|no| N[Print commit the index's .gitmodules]
    I -->|no| O[_check_gitmodules_paths]
    O --> P{Duplicate path?}
    P -->|yes| Q[_file_put_back if needed]
    Q --> M
    P -->|no| R[Continue validation]

Loading

File-Level Changes

Change	Details	Files
Align .gitmodules validation with Git’s file-selection behavior when the work-tree file is absent.	Added shared logic to inspect the work-tree file, then the index, then HEAD, including Git-readable blob handling and refusal cases. Updated duplicate-path and invalid-value checks to report the actual file Git reads. Preserved staged index edits during remediation by recording a single put-back and avoiding later overwrites. Adjusted member-gitmodules-absent coordination so it restores the file only when needed.	templates/family-root/scripts/validate-family.py
Make generated remediation commands safe for a subsequent plain commit.	Added git add -- .gitmodules to section-removal remedies in all branches. Changed affected messages to distinguish staging alone from staging and committing. Updated duplicate-path remedies to restore the Git-read file before editing and then stage and commit it.	templates/family-root/scripts/validate-family.py tests/test_family_add_refusals.py tests/test_family_hand_mount_git_rm_lines.py tests/test_family_unnamed_gitlink.py tests/test_validate_family_remedies_typed.py tests/test_validate_family_unnamed_mounts.py
Expand regression coverage for index-only .gitmodules states and commit/remediation ordering.	Added N1–N4 scenarios covering index-only sections, invalid values, HEAD-only values, missing files, stale sections, gitlinks, and case variants. Each scenario types only printed commands, performs a plain commit, and verifies the gate, clone, HEAD contents, and clean status. Updated existing expectations and composed-remedy tests for index put-backs and mandatory staging.	tests/test_validate_family_index_gitmodules.py tests/test_family_gitmodules_values.py tests/test_family_hand_mount_git_rm_lines.py tests/test_family_holder_bootstrap_texts.py tests/test_family_holder_remedies_compose.py tests/test_family_unnamed_gitlink.py tests/test_validate_family_remedies_typed.py tests/test_validate_family_unnamed_mounts.py

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#1077	Make gitmodules-path-twice inspect the .gitmodules contents Git reads: the work-tree file when present, otherwise the index's file, then HEAD's.	✅
#1077	Make the check for values Git refuses to read inspect the same Git-selected .gitmodules file as well as HEAD's, and ensure its remedies preserve and stage the file Git reads.	✅

---

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

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[brettheap]

RV1124: independent review of #1124 at 672845cc: NOT READY

One SHOULD (S1): #1111's hygiene test fails this PR's new guard once #1111 is on main. The rest are NITs, with the suspected follow-up measured and graded below. The change itself reads the file git reads in every state I measured, and its lines type cleanly.

What I measured

Which .gitmodules git 2.43.0 reads, against what the script now reads. Each state is a scratch repository with gitlinks at members/W, members/I and members/H. The work tree's file maps W, the index's maps I and HEAD's maps H. "git" is the path git submodule status -- <path> maps. The script's columns are _listed_as_git_reads (the value check) and _sections_as_git_reads (path-twice and path-unnamed), loaded from the PR's script.

State	git reads	value check	sections	put-back line
only in the work tree	W	W	W	none
only in the index	I	I (:.gitmodules)	I	git checkout -- .gitmodules
only in HEAD	H	H (HEAD:.gitmodules)	H	git checkout HEAD -- .gitmodules
work tree's differs from the index's	W	W	W	none
deleted in the work tree	I	I	I	git checkout -- .gitmodules
a symlink to a file	W (through it)	W	W	none
a dangling symlink, or a directory	nothing	gitmodules-unreadable refusal	same	none
unmerged (stages 1-3), file on disk	nothing	W	W	none
unmerged, file deleted	nothing	H	H	git checkout HEAD -- .gitmodules

• The script agrees with git in every state but the unmerged ones (N6), which v1.1.1's release notes already list as a known limit.
• Git's file_exists is an lstat, so os.path.lexists is the right test. A symlinked .gitmodules is read and written through by git, but git add -- .gitmodules refuses it (invalid path '.gitmodules', exit 128). The stage line cannot be typed there, but git will not track such a file at all, and the old "then commit" could not either. No finding.

Remedies typed as printed, then a plain git commit. These are probe rows in my scratch copy, not in the PR.

• P2: path-twice where HEAD's file is the source (deletion staged). Gate, git submodule update and a plain clone all pass, exit 0.
• P3: a plain git rm members/IRRS, then a value staged in the index's file in IRSS's section, the work tree's deleted.
  • On main the value is missed.
  • On the PR the value finding puts back the index's file, and the removal writes MEMBER's section back "in the file a line above put back".
  • Typed in order and committed, the gate passes in every mode and a plain clone checks both out.
• P5: a value in HEAD's alone, the index's mended but with a stale members/Old section, the work tree's deleted.
  • The PR says "Commit the index's", and path-unnamed puts the index's file back itself.
  • Typed and committed: HEAD gives two members, no value and no Old, and the gate exits 0.
• P7: path-twice (second) and a stale section in HEAD's file, deletion staged. Typed in order: HEAD loses both, and the gate exits 0 in both modes.

Copies. templates/family-root/scripts/validate-family.py is the only copy in the tree. No 64-hex digest appears in templates/ or tests/: contracts/shape-pin.yaml is filled from placeholders at scaffold time. test_pinned_copy_text, test_agents_md_codes, test_cli_reference and test_doctor_validator_rows pass (below).

Rows on main's script. The new file gives 10 failed and 2 passed, as claimed. The 2 are the N4 rows. The failures are for the right reasons:

• N1 and N2 are family ok, exit 0.
• N2b and N2c fail on text: main puts back HEAD's.
• The six N3 rows fail commands(found[0])[-1] == "git add -- .gitmodules".

Findings

S1 (SHOULD): #1111's test_path_guard_hygiene.py fails the new guard in _listed_as_git_reads.

• On a scratch merge of origin/main 4ed7c7bf + 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 b4484647 + this PR, it fails test_no_bare_guard_outside_a_stat_helper_or_the_allowlist[templates/family-root/scripts/validate-family.py]. The message is: validate-family.py:3876, in _listed_as_git_reads: os.path.lexists(root / GITMODULES) is a bare guard.
• Main + 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 alone does not fail for this file. It fails only for adopt-project.py, which is 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's own update after adopt-project.py: check names what the clone's .git holds as git keeps refs, and asks a pin's file through the links the root keeps, so a link into .git or onto a pin's file is refused before any leg or adopted with the copy beside it (#904) #1091, not this PR's.
• The template is in the test's scope: HELPERS names it, and BENIGN lists its other seven .gitmodules lexists sites by function.
• Fix: drop the guard. The only caller, _check_gitmodules_values, calls _listed_as_git_reads only where present is False, and _gitmodules_put_back already asks lexists (it is allowlisted). So the function can be spec = _gitmodules_put_back(root)[0]; return (_blob_listed(root, spec) if spec else b""), spec, with its docstring saying the caller asks it only where the work tree's file is gone.
• The other route: whichever of 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 and validate-family.py: gitmodules-path-twice and the check for values 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 lands second adds (f"{FR}/validate-family.py", "_listed_as_git_reads", GITMODULES_LSTAT): ("ROOT", VF_GITMODULES) to BENIGN.

N1 (NIT, follow-up): the suspected follow-up is real, pre-existing and self-correcting.

• P1 setup: update = !true in IRRS's section, in the work tree's file on disk and staged; HEAD's does not give it.
• Round 1 prints only git config -f .gitmodules --unset-all submodule.members/IRRS.update, with no stage word. After it and a plain git commit:
  • HEAD gives !true;
  • a plain clone's git submodule update --init --recursive exits 128 (fatal: invalid value for 'submodule.members/IRRS.update');
  • the next gate run says "in HEAD ... Stage and commit the work tree's .gitmodules".
• Main's script prints the same text and gives the same result, so this is not a regression. CI's gate would refuse such a commit.
• P1b, a sibling: the index alone gives the value and the work tree's file is clean on disk. The gate is family ok in --no-members (exit 0), and a plain commit records the value in HEAD.
• Fix, in a follow-up issue: where the work tree's file is on disk, read the index's too (_blob_listed(root, ":.gitmodules")). Where it gives the value, end the lines with git add -- .gitmodules and "then stage" (or "then stage and commit" where HEAD gives it). Printing the stage line as a line, as _section_out now does, would also give N2's "then stage" a line to type.

N2 (NIT, pre-existing): an index blob with a line git cannot parse, after a value, is family ok.

• P9 setup: the index's file is HEAD's plus update = !true in IRRS's section and then this is no config line, with the work tree's deleted.
• Git's submodule status exits 128 (invalid value for 'submodule.members/IRRS.update'), but the gate exits 0 in --no-members and in members mode.
• The cause: _blob_listed returns b"" on the exit 128 of git config --blob ... --list, which drops the entries git printed before the bad line. Bootstrap's gitmodules_listed keeps them (bootstrap.py and validate-family.py (family root): a .gitmodules in the index or HEAD with one bad line is read as no sections where git reads those before it, so a no-credential run fails wrongly, and the value rules are written twice #856). _blob_sections (validate-family.py (family root): gitmodules-path-unnamed reads only the work tree's .gitmodules, so where that file is gone, a section the index still holds passes the gate and is committed #1036) shares this.
• Fix: read a blob up to its bad line as bootstrap does, or refuse gitmodules-unreadable for the index's file as for HEAD's.

N3 (NIT): two of the new behaviours are pinned by no row (mutants A and B below survive all 439 rows of the PR's 9 files).

• (a) The value's write-back reads the file git reads.
  • P10: the index's IRRS section gives url = https://github.com/InkRouter/IRRS.git, then a url with no value, the work tree's deleted.
  • The PR writes back that url. Mutant A writes back HEAD's url, which silently undoes the staged change.
• (b) path-twice records its put-back.
  • Under mutant B, P7's path-unnamed prints git checkout HEAD -- .gitmodules again, and the commit puts second back in HEAD.
• Fix: add P7 and P10 as rows. Each kills its mutant here.

N4 (NIT): path-twice's wording where HEAD's file is the source.

• The text reads "back from HEAD's, as the index holds none first, git checkout HEAD -- .gitmodules". The word "first" lands on "the index holds none".
• Fix: "put .gitmodules, gone from the work tree, back first from {whose}, ..." (validate-family.py:2318).

N5 (NIT): a docstring this PR made stale. _gitlink_out_first still says "It ends as the section's finding does, then commit .gitmodules, but where HEAD's .gitmodules holds no section of its name ... staged" (validate-family.py:3368-3372). Every branch stages now.

N6 (NIT, pre-existing): an unmerged .gitmodules.

• P6 setup: stages 1-3, the work tree's file deleted, and HEAD's gives a value.
• Git reads no file (submodule status exits 128). The value finding says "here and in HEAD" and "puts back HEAD's, as the index holds none", but the index holds the file at its conflict stages.
• The model is validate-family.py (family root): gitmodules-path-unnamed reads only the work tree's .gitmodules, so where that file is gone, a section the index still holds passes the gate and is committed #1036's (GITMODULES_SOURCES), and the release notes list the unmerged case as a known limit.
• Fix (follow-up): treat an index that holds the file only at stages 1-3 as bootstrap's gitmodules_source does.

Mutants (mine)

Each mutant ran in its own worktree of the scratch merge, with -x, over the PR's 9 test files (439 rows). Each file was restored afterwards, and its sha256 checked: d0b4c63d... is the head's.

Mutant	Result
A: the value's write-back from HEAD's where the work tree's file is gone (as before)	SURVIVED, 439 passed; P10 kills it
B: path-twice names its put-back without recording it (_file_put_back(root, None))	SURVIVED, 439 passed; P7 kills it
C: path-twice and path-unnamed read nothing where HEAD's alone gives a value (old refused gating)	killed: test_a_value_head_s_file_alone_gives_leaves_the_work_tree_s_read
D: READ_HERE names the index's file "the work tree"	killed: N2
E: the value check reads HEAD's, never the index's	killed: N2
F: the value's lines always say "then stage"	killed: test_the_gate_says_which_file_gives_it[work-tree-only]
I: the value's put-back is not recorded	killed: test_two_keys_with_no_gitmodules_in_the_work_tree_put_it_back_once
P: the value's lines never say "commit"	killed: test_a_value_git_dies_on_fails_bootstrap_and_every_mode_of_the_gate[update-None]

Merge-trees

• Against origin/main 4ed7c7bf: git merge-tree --write-tree is clean (tree 36ca31aa). The merged tree differs from main only in the PR's 10 files. Main's scaffold-project.py: the --reuse-empty-repo help, its docs and each remedy that sends a person into that re-run say ssh, a credential helper or askpass can still make it wait, and how to keep it from waiting (#915) #1101, templates/assembly-root/project.yaml and templates/family-root/family.yaml: the comment above visibility: says it is the visibility the run asked for, and that a repository the run reuses keeps its own, where it said "this is all three repositories' visibility" (#913) #1102 and adopt-project.py: check names what the clone's .git holds as git keeps refs, and asks a pin's file through the links the root keeps, so a link into .git or onto a pin's file is refused before any leg or adopted with the copy beside it (#904) #1091 touch none of them.
• Against open PRs: clean against 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, the family root's bootstrap.py: credential_advice and ended_on say a member's failed clone ends git's update with 1 before any checkout but a leg's lets it go on, and killed_said says git's update was itself killed, not that it went on (#1085) #1088, family.py: the remove that both member-duplicate-id remedies name and the add that bump's member-unknown names carry --family-root <holder>, quoted for the host, so each runs as printed (#1005) #1117 and README.md, adopt-project.py and the handbook: a kept directory entry makefile/ is tested, README says make at the root reads a kept GNUmakefile or makefile instead, and a docstring says the shape's Makefile alone holds CONTRACTS_DIR (#1061) #1123. No open PR touches these 10 files; I checked all 21 by their file lists.
• Against 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: main + 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 + this PR merges clean (tree d0cdfe9c), but fails S1.

Gate

The gate ran on scratch merge e8d7f845 (main 4ed7c7bf + 672845cc), with /usr/bin/python3 -m pytest -p no:cacheprovider and the landing gate's environment.

• The environment: an empty GIT_CONFIG_GLOBAL, GIT_CONFIG_NOSYSTEM=1, test identities, a stub gh, GH_TOKEN=bogus and an empty GH_CONFIG_DIR.
• The files (61): every tests/test_family*.py and tests/test_validate_family*.py (the PR's 9 among them), test_repo_hygiene.py and test_shape_doctor.py.
• Result: 3178 passed, 27 skipped and 0 failed, in six shards:
  • 688;
  • 690;
  • 667 + 23 skipped;
  • 688 + 1 skipped;
  • test_shape_doctor.py in two halves: 224, and 221 + 3 skipped.
• The doctor: it runs the holder's validate-family.py (FAMILY_GATE), so it was in the gate.
• Extras: test_agents_md_codes, test_cli_reference, test_pinned_copy_text and test_doctor_validator_rows gave 228 passed.
• The new rows file: its 12 rows passed inside the gate at the PR's script. At main's script: 10 failed, 2 passed.
• 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's test_path_guard_hygiene.py on main + 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 + validate-family.py: gitmodules-path-twice and the check for values 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: 2 failed, 47 passed. One failure is S1; the other is adopt-project.py, which also fails on main + 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 alone.

Lane: openreposhape-4 (openRepoShape-4)

[brettheap]

RV1124b: independent review of #1124 at 8dfa32bd: READY

No MUST and no SHOULD. RV1124's S1 is fixed, and N3, N4 and N5 are done as the fix round says. Two mutants survive all 442 rows of the PR's nine test files, and one docstring line overstates; all three are NITs and can go to a follow-up.

What I measured

Everything ran on scratch merge 338635f4 of origin/main 038dafba9 and 8dfa32bd (tree 23c7194f0b6a), with /usr/bin/python3 -m pytest -p no:cacheprovider and the landing gate's environment: EMAIL set, GIT_ALLOW_PROTOCOL, GIT_SSH_COMMAND and LANES_* unset, an empty GIT_CONFIG_GLOBAL, GIT_CONFIG_NOSYSTEM=1, test identities, a stub gh first on PATH, GH_TOKEN=bogus and an empty GH_CONFIG_DIR. Git is 2.43.0.

S1: _listed_as_git_reads asks no guard of its own.

• Its only caller is still _check_gitmodules_values (validate-family.py:2052, (on_disk, "") if present else _listed_as_git_reads(root)), so it is asked only where os.path.lexists(root / ".gitmodules") is False. That is the predicate the dropped guard tested, and _gitmodules_put_back asks it again. No test calls the function.
• Behaviour. A probe loaded both scripts, 672845cc and 8dfa32bd, and compared _check_gitmodules_values in 12 states: its return value, its findings, Report.put_back, and _listed_as_git_reads itself where the file is gone. The states are RV1124's (only in the work tree, only in the index, only in HEAD, the work tree's differing from the index's, deleted from the work tree, a symlink to a file, a dangling symlink, a directory, unmerged on disk, unmerged and deleted), plus no file at all, plus deleted from disk and not staged. All 12 agree. The file each reads matches RV1124's table.
• test_path_guard_hygiene.py on the merge: 49 passed. That includes test_every_allowlist_entry_names_a_guard_that_is_there, so no BENIGN entry is stale, and no entry names _listed_as_git_reads. With 672845cc's script in its place, test_no_bare_guard_outside_a_stat_helper_or_the_allowlist[templates/family-root/scripts/validate-family.py] fails (1 failed, 48 passed), so the test pins S1.

N3: the new rows file (15 rows).

• At this head: 15 passed.
• At 672845cc's script: 14 passed, 1 failed. The one is test_1077_a_second_section_the_index_alone_gives_is_path_twice (N4's old wording); the P7 and P10 rows pass there.
• At main's script: 13 failed, 2 passed (the 2 are N4's rows).
• All three match the body.

N4 and N5.

• Path-twice now says "put .gitmodules, gone from the work tree, back first from {whose}, {put_back}, so that every other section stays, and take each other out by hand". The only pin is TWICE_PUT_BACK in the new rows file, which covers the index's case. PUT_BACK_THEN (test_family_holder_remedies_compose.py:135) pins another finding's wording (script line 1074), which this round leaves alone.
• _gitlink_out_first's docstring now agrees with _section_then, which it calls. No "then commit .gitmodules" is left in the script.

Nothing else moved. 672845cc..8dfa32bd touches the script (+12/-12: N4's string, N5's docstring, S1's docstring and the two dropped lines) and the new rows file (TWICE_PUT_BACK, STALE, and the P7 and P10 rows). I found no behaviour change the body does not name.

Mutants

Each ran in its own worktree of the scratch merge. Each script was restored afterwards, and its sha256 checked against the head's (63f4b9d5...).

Mutant	Result
A: the value's write-back reads HEAD's where the work tree's file is gone (here if present else there)	killed by the P10 row (1 of 15)
B: path-twice names its put-back without recording it (_file_put_back(root, None))	killed by both P7 rows (2 of 15)
S1a: the bare lexists guard back (672845cc's script)	killed by test_path_guard_hygiene.py
S1b: the caller always asks _listed_as_git_reads, dropping present	survived the new rows file; killed by test_family_gitmodules_values.py::test_a_value_git_dies_on_fails_bootstrap_and_every_mode_of_the_gate[update-None]
S1c: _listed_as_git_reads always reads :.gitmodules, with no fallback to HEAD's	SURVIVED all 442 rows (15, then 427 with -x): NIT 2
S1d: _listed_as_git_reads returns b""	killed (2 of 15)
S1e: _listed_as_git_reads returns spec ""	killed (3 of 15)
PT4: path-twice records a put-back for every path, not only one given twice	killed (9 of 15)
PT7: path-twice always says "from the index's"	SURVIVED all 442 rows (15, then 427 with -x): NIT 1

Merge-trees

• Against origin/main 038dafba9: git merge-tree --write-tree is clean, tree 23c7194f0b6a08f56b93bdc80a05924e90e703a4, as the body says. The merged tree differs from main in exactly the PR's 10 files, and each is the PR head's.
• Against open PRs: of the 37 open, none but validate-family.py: gitmodules-path-twice and the check for values 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 touches validate-family.py or any of the PR's test files. No PR's file list was cut short (changedFiles equals the files listed for each).

Gate

• tests/test_family*.py and tests/test_validate_family*.py (59 files, the PR's 9 among them), plus test_repo_hygiene.py and test_path_guard_hygiene.py: 61 files in 6 shards, 2785 passed, 24 skipped, 0 failed (365; 474; 366 and 4 skipped; 499 and 19 skipped; 553; 528 and 1 skipped).
• test_shape_doctor.py in 8 shards of 56 ids from a bash script (mapfile -t, every list checked non-empty, no timeout): 445 passed, 3 skipped, 0 failed (56 in each shard but the last, 53 and 3 skipped). The 448 ids came from --collect-only -q; the 8 lists, concatenated, equal that list (cmp), with no id twice. 25 ids hold spaces.

Findings

NIT 1: path-twice's wording where HEAD's file is the source is pinned by no row.

• Mutant PT7, which always says "back first from the index's", survives all 442 rows. The P7 row on deletion-staged checks only that the put-back line is in the text.
• Fix: in test_1077_path_twice_records_its_put_back_for_the_next_finding, assert f"back first from {GONE[how][0]}, `{put_back}`" in twice, which pins "HEAD's, as the index holds none" on deletion-staged.

NIT 2: no row pins the value check's fallback to HEAD's where the index holds no .gitmodules.

• Mutant S1c reads :.gitmodules alone and survives all 442 rows. With the deletion staged and HEAD's file giving a value, it reads nothing as the file git reads. It would then say "Commit the index's .gitmodules, which git reads here and does not give it", and that commit records the deletion. The head reads HEAD's there, as git does: the state probe's "only in HEAD" puts back git checkout HEAD -- .gitmodules and names the value.
• Fix: add a row in which HEAD's file gives update = !true in a member's section and the deletion is staged (gone(holder, env, "deletion-staged")). Assert that the first line is git checkout HEAD -- .gitmodules and that the text ends "then stage and commit .gitmodules"; type the lines in order, make a plain git commit, and check that HEAD gives no value and the gate passes.

NIT 3: _listed_as_git_reads's docstring opens by saying it returns .gitmodules "as git reads it".

• Where the work tree's file is on disk, it now returns nothing. The later sentence, "The caller asks it only where the work tree's file is gone", covers today's one caller, but a new caller could take the first line at its word.
• Fix: open with "where the work tree's .gitmodules is gone from disk," or say it returns nothing where the work tree's file is on disk.

Not run: RV1124's P1 to P10 probes, which the brief left optional.

Lane: openreposhape-4 (openRepoShape-4)

[sourcery-ai]

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 17 hours and 31 minutes by commenting @sourcery-ai review. Upgrade to get a review now.

[sonarqubecloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud