Skip to content

fix(adopt): preserve existing gitmodules through leg extraction - #162

Merged
brettheap merged 4 commits into
mainfrom
fix/adopt-existing-gitmodules
Oct 7, 2026
Merged

brettheap merged 4 commits into
mainfrom
fix/adopt-existing-gitmodules

Conversation

@brettheap

@brettheap brettheap commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Change

Fix the mount failure when an adoption plan moves the source's .gitmodules into a leg with an existing dependency. After removing moved paths, create and stage a fresh assembly registration file only if it is absent. Retained assembly registrations are preserved.

The original .gitmodules, dependency gitlink and history remain in the extracted leg. Content verification is unchanged.

Verification

  • Before the fix, the new regression reproduced Git's fatal: please make sure that the .gitmodules file is in the working tree refusal at the first new leg mount.
  • python3 -m pytest tests/test_adopt_*.py -q: 90 passed, 1 skipped inside py-bench with git-filter-repo 2.47.0.
  • Final regression run after adding explicit gitlink-mode and bootstrap assertions: 4 passed.
  • New coverage exercises a populated dependency moved into code or spec, exact registration/blob/gitlink/history preservation, recursive clone and bootstrap, retained assembly registrations, and recovery from a failed mount into a fresh clone. Existing adopter tests cover sources without submodules.
  • Disposable openRepoTools rehearsal at f5444c5e1f3c6c9ed43aeafcc000b0e78b10497a completed with this commit. Independent path/mode/type/object accounting: 94 original entries, 45 spec, 43 code, 6 root, zero missing or duplicated.
  • Recursive clone, generated assembly bootstrap and validators passed. The nested openRepoShape dependency stayed at 39d5c986fcfac1a160474bfe91c5f1c37fccc72c, digest a44c0165beb9d3b6ddfce46925bf04b8d19934d5327b5a304b5f3f11af699b02.
  • The rehearsal used disposable local bare remotes and file-only Git transport. The isolated source stayed clean on its original main commit. Live openRepoTools main, gitlink and dependency contract remain unchanged.

Delivery boundary

This is the upstream prerequisite for openRepoTools migration task T005. Consumer repinning waits for a reviewed commit on upstream main and a freshly computed digest. The rehearsal does not authorize real leg repository creation, migration cutover, seat movement or merging.

Summary by Sourcery

Preserve existing submodule registrations and dependency history when extracting adoption legs.

Bug Fixes:

  • Preserve an existing dependency’s .gitmodules, gitlink, and history when adoption moves it into a leg.
  • Allow leg mounting to recreate and stage .gitmodules safely, including repositories whose ignore rules hide dotfiles.

Enhancements:

  • Retain assembly-level submodule registrations that are not moved by the adoption plan.

Tests:

  • Add regression coverage for moved and retained submodule registrations, gitlink preservation, ignored .gitmodules files, mount failure recovery, recursive cloning, and bootstrap validation.

@sourcery-ai

sourcery-ai Bot commented Oct 6, 2026

Copy link
Copy Markdown

Reviewer's Guide

The adoption mount now stages a fresh empty .gitmodules only when extraction removed the source registration, while preserving registrations retained in the assembly; comprehensive local Git regression tests cover dependency/history fidelity, registration merging, validation/bootstrap, and recovery after failed mounts.

Sequence diagram for preserving gitmodules during leg extraction

sequenceDiagram
    participant Mount as AdoptionMount
    participant Assembly
    participant Git
    participant Leg as ExtractedLeg

    Mount->>Git: git rm -r -q path
    Git-->>Assembly: Source paths removed and staged
    Mount->>Assembly: .gitmodules exists?
    alt .gitmodules absent
        Mount->>Assembly: write_lf .gitmodules
        Mount->>Git: git add .gitmodules
        Git-->>Assembly: Fresh registration file staged
    else .gitmodules retained
        Git-->>Assembly: Existing registrations preserved
    end
    Mount->>Git: git submodule add work_root/spec spec_path
    Mount->>Git: git submodule add work_root/code code_path
    Git-->>Leg: Original .gitmodules, gitlink, and history remain
Loading

File-Level Changes

Change Details Files
Ensure leg mounting can recreate the assembly’s Git submodule registration file after the source registration is moved away.
  • Remove paths selected for extraction before mounting generated legs.
  • Create and stage an empty .gitmodules only when no registration file remains, allowing subsequent git submodule add operations.
  • Leave retained assembly registrations untouched instead of replacing the file.
adopt-project.py
Add regression coverage for preserving dependencies and mount recovery across adoption workflows.
  • Exercise moving .gitmodules and its dependency into both code and spec legs, including exact blob, gitlink mode, commit, history, recursive clone, validation, and bootstrap checks.
  • Verify existing assembly registrations survive alongside generated leg registrations.
  • Verify a failed mount does not mutate the source and a fresh clone can be mounted successfully afterward.
tests/test_adopt_submodules.py

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

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

brettheap and others added 2 commits October 7, 2026 00:28
…ay scripts/family.py already spells it (Sonar S1192 on #162)

SonarCloud flagged python:S1192 on PR #162: the literal ".gitmodules" was
repeated four times in `_mount_the_legs` (the existence guard, the
`write_lf`, the `git add` and the `git config -f`). It is now one
module-level constant, GITMODULES, mirroring scripts/family.py. Behaviour is
unchanged; the adopt, plan, spec-only, hygiene and CLI-reference tests pass
as before.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… moved the source's, staged past a .* .gitignore with -f, and removed before the submodules it registers; the mount tests run without git-filter-repo (PR #162 review)

The review of PR #162 found that the fresh `.gitmodules` was written on the
`.exists()` test, so a source with NO submodules, which never had one, was
given an empty file and a plain `git add` of it. `git add` respects
`.gitignore`, and the common `.*` + `!.gitignore` pattern hides `.gitmodules`:
such a source adopted cleanly before the PR and now failed at `git add`, after
both leg repositories had been created and pushed. `_mount_the_legs` now
writes the file only when `.gitmodules` is in the set of removed paths, so a
source without one is mounted exactly as before (`git submodule add` creates
and stages the file itself and ignores `.gitignore`), and stages it with
`git add -f` for the case where the plan did move the source's file and the
source's `.gitignore` hides it.

The removal order was the second defect. A submodule whose path sorts before
`.gitmodules` (anything under `.deps/`, `.claude/`, `.github/`) has its
`git rm -r` edit the file that registers it, and the later `git rm
.gitmodules` was refused for "changes staged in the index". The removed paths
are now computed once and `.gitmodules` is removed FIRST. Whether the
registrations it held went to the same leg as the gitlinks they name stays the
plan's question (#166); a `.gitmodules` the plan kept in the root is verified
byte for byte today and fails (#165). The comment above the guard says both.

The tests: the module-wide skip for a missing git-filter-repo is now a
decorator on the end-to-end test alone, so the mount tests, which call
`_mount_the_legs` on hand-made repositories, run everywhere including the
Windows job that installs no filter-repo; the URL comparison compares `Path`s
rather than strings for the same reason. The loader no longer registers the
module in `sys.modules` (nothing reads it and nothing removed it).
`test_failed_mount_leaves_source_reusable` accepted ANY `CommandFailed` and
passed on the wrong failure with the fix reverted; it now matches the missing
code leg, drops the three assertions about a `source` that `_mount_the_legs`
never touches, and is renamed `test_failed_mount_does_not_poison_a_fresh_clone`.
New mount tests cover a source with and without submodules under a `.*`
`.gitignore`, and a submodule at `.deps/moved`; each fails against the code it
guards (the PR as it stood, the ordering alone, and `-f` alone were each
reverted in turn).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@sonarqubecloud

sonarqubecloud Bot commented Oct 7, 2026

Copy link
Copy Markdown

@brettheap

Copy link
Copy Markdown
Contributor Author

Review, and what changed since 91d5685

An adversarial review of head 91d5685 ran the new tests with the hunk reverted (3 of 4 fail with the quoted .gitmodules refusal; all 4 pass with it), ran tests/test_adopt_*.py on a local merge with #164 (90 passed, 1 skipped), and then built the cases the hunk did not test. Three findings are fixed on this branch, two are filed as issues, and one Sonar finding is taken.

Fixed here

  • A regression on the common case. git add -- .gitmodules respects .gitignore. A source with NO submodules whose .gitignore is the usual .* + !.gitignore adopted cleanly before this PR and failed at the new git add after both legs had already been created and pushed, so a re-run met REFUSED leg-remote-exists. The fresh file is now written ONLY when the plan moved or dropped the source's .gitmodules, and staged with -f. A source that never had one is mounted exactly as before.
  • Order of removal. A submodule whose path sorts before .gitmodules (.agents/, .claude/, .deps/, .github/ ...) made git rm -r stage an edit to .gitmodules, and the later git rm .gitmodules was refused. Pre-existing, but inside this hunk's remit: .gitmodules is now removed first.
  • The tests. The module-wide skip kept the two mount-level tests off every machine without git-filter-repo, including the Windows job; it now sits on the end-to-end test alone. The failed-mount test accepted any CommandFailed (with the fix reverted it passed on the wrong failure) and asserted three things about a source the function never touches; it now matches the missing leg. The sys.modules registration the fixture never removed is gone. Mount-level tests cover the .* gitignore with and without a submodule, and the dot-directory ordering.
  • Sonar S1192: .gitmodules is spelled once, as GITMODULES, the way scripts/family.py already spells it.

Filed, not fixed here

Branch brought up to main (1a9fc53) by merge, no rebase. Fix commits: 4b5591e (S1192), 6fa4e07 (the rest).

Re-verified at 6fa4e07: every fix put back in turn fails its own test; CI green on ubuntu, macOS and Windows (where the mount tests now run); SonarCloud reports zero open issues on the PR.

@brettheap

Copy link
Copy Markdown
Contributor Author

@codex review

@brettheap
brettheap marked this pull request as ready for review October 7, 2026 01:08
@sourcery-ai

sourcery-ai Bot commented Oct 7, 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 16 hours and 11 minutes by commenting @sourcery-ai review. Upgrade to get a review now.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@brettheap
brettheap merged commit 7f84ca4 into main Oct 7, 2026
5 checks passed
@brettheap
brettheap deleted the fix/adopt-existing-gitmodules branch October 7, 2026 01:09
brettheap added a commit to opensoft/openRepoTools that referenced this pull request Oct 8, 2026
…adopter fix for a source with an existing submodule and opensoft/openRepoShape#164's prefer-triad advisory; none of the eight mounted files moved (#188)

The triad migration (#186, first act; the plan's
T005) rehearses adopt-project.py against this repository, which carries
a submodule of its own. At 39d5c98 the adopter refused that source at
the first leg mount (adopter-blocker.md on 004-migrate-to-triad);
opensoft/openRepoShape#162 fixed it and landed on that repository's main as
7f84ca4. The rehearsal must run the tool at a commit this repository
pins, so the pin moves first.

On openRepoShape's main, as AGENTS.md's rule 2 requires:
  gh api repos/opensoft/openRepoShape/compare/main...7f84ca42ca86a8902928345109d2bf6bad87bd91 --jq .status
  -> identical (7f84ca4 is main's head)

39d5c98...7f84ca4: ahead 2, behind 0, 15 files (1a9fc53 is
opensoft/openRepoShape#164, 7f84ca4 is opensoft/openRepoShape#162):
AGENTS.md, README.md, adopt-project.py,
contracts/single-repository-record.yaml, docs/handbook.html,
scaffold-project.py, scripts/shape_advisory.py, shape-doctor.py,
templates/assembly-root/AGENTS-shape.md,
templates/assembly-root/project.yaml,
templates/single-repository/single-repository.yaml,
tests/test_adopt_submodules.py, tests/test_repo_hygiene.py,
tests/test_shape_advisory.py, tests/test_shape_doctor.py. None of the
eight files this suite mounts (the two root Makefiles, their two
.gitignores, the two bootstrap.py, siblings.py, scripts/repo_shape.py)
is among them; `git diff --stat 39d5c98 7f84ca4 -- <the eight>` in the
submodule is empty.

Bump run exactly as contracts/openreposhape-pin.yaml states:
  git -C upstream/openRepoShape fetch origin
  git -C upstream/openRepoShape checkout 7f84ca42ca86a8902928345109d2bf6bad87bd91
  python3 -c "import sys; sys.path.insert(0, 'upstream/openRepoShape/scripts'); from repo_shape import tree_digest; print(tree_digest('upstream/openRepoShape', 'HEAD'))"

Old commit:      39d5c986fcfac1a160474bfe91c5f1c37fccc72c
Old tree_sha256: a44c0165beb9d3b6ddfce46925bf04b8d19934d5327b5a304b5f3f11af699b02
New commit:      7f84ca42ca86a8902928345109d2bf6bad87bd91
New tree_sha256: 3be52767e727d924dd05f9e78b18eb70f1682fe48614fab519f85331eeff5fa8

The digest is recomputed, not adjusted. The same 7f84ca4 helper, asked
for 39d5c98's tree, prints the old row a44c0165... back, so
sorted-ls-tree-r-v1 means the same thing on both sides of the bump. The
pin file and the gitlink move together in this one commit.

Part of #186

Lane: openRepoTools-1

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
brettheap added a commit that referenced this pull request Oct 9, 2026
…ry that holds one of the source's submodules, whatever rule matched it, so it no longer proposes the `root` that `check` refuses, and `check` now prints the submodule remediation after its findings (#166)

The second review of aaa9099, by lane openRepoShape-2, found that `plan` proposes what `check` refuses. `walk` sees paths, not modes. So a submodule vendored at `.specify/vendored`, `.cursor` or `.agents` came out `leg: root` with no question (rules `root-spec-kit` and `root-assistant-instructions`). One at `docs/theme` came out `spec` (`spec-governance`), also unasked. `check` then answered `plan-submodule-root`, or a split once `.gitmodules` was answered. Reproduced at 09d7889: `.specify/` written `root`, then `plan-submodule-root: .specify/vendored (entry .specify/)`.

- `ask_about_submodules` runs in `cmd_plan` right after `walk`. Every entry that `_covering` says covers a gitlink takes the verdict `.gitmodules` itself gets: `leg: null`, `review_required: true`, rule `ambiguous-gitmodules` and its reason. It also takes that question, plus "This entry holds the submodule(s) <paths>: it is asked this question, not classified by its rule `<rule>`." When the entry holds other paths too, the question adds that it can be split. `walk`'s folding is unchanged; only the covering entry is replaced.
- `check` on the untouched plan now reports `plan-unresolved` there, never `plan-submodule-root`.
- The 2026-09-02 ruling is unchanged for files. The `ambiguous-gitmodules` header comment says that a vendored SUBMODULE under a root-ruled path is asked rather than rooted, because the root keeps no submodule of the source's.
- A source with no gitlink gets its entries back untouched. On the suite's SYNTHETIC_TREE source, these are byte-identical to main's: the plan file, `plan`'s output, and `check`'s output both unresolved and resolved.

Not fixed here, and now said so: `execute` refuses every SUBMODULE problem `check` finds, but not yet every finding. An entry that is uncovered, covered twice or has a misspelled leg still reaches the split; that is #168, which merges right after this. The docstrings that said "the plan `check` passes is the plan `execute` runs" now say exactly this, in `submodule_plan_problems`, `_answered_legs` and `_refuse_an_unrunnable_plan`.

The same review's smaller findings:
- The question and the remediation offered no way to be rid of one submodule except `git rm` after adopting. The rule has always allowed dropping one on its own, and both texts now say so: its registration stays behind in the leg, inert.
- `check` prints SUBMODULE_REMEDIATION once after its findings when any of them is a submodule problem, as `execute`'s refusal does. `_submodule_findings` returns it, so `cmd_check` gains no branch.
- The module docstring's list of what `check` validates now includes the submodule rule. docs/cli.md is generated from the first line only and is unchanged.
- #162's kept-root mount test and the `_mount_the_legs` comment are re-labelled as mount defence in depth for a plan that `check` and `execute` refuse (#166). In `_mount_the_legs` only the comment changes.

Tests in tests/test_adopt_submodules.py, 52 -> 77 cases:
- `plan` asks on the entry holding a gitlink at `.specify/vendored`, `.cursor`, `.agents`, `docs/theme` and `upstream/dependency`, and on one entry holding two gitlinks. Each row is the `.gitmodules` row apart from its path and size. Its question names the gitlinks and the rule, and `check` on the untouched plan reports only `plan-unresolved`.
- The `tools/dep` test now answers `tools/` as its rule would have, since `plan` asks there.
- Branches no test reached before: a `.gitmodules` with no gitlink beside it; two submodules split across legs; a gitlink that is its own entry. Also uncovered, doubly covered and bad-leg entries: `check` reports each by its own finding, and the helper skips it.
- The `check` matrix asserts the remediation is printed once, after the findings, exactly when there is a submodule finding.
- One `needs_filter_repo` end-to-end test runs `execute` on accepted plans: both code, both spec, and the submodule dropped on its own. `git clone --recurse-submodules` of the assembly and `bootstrap.py` in the clone then both exit 0.

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 11, 2026
…ounts no leg, the source's own submodule at its own commit as `adopt-root-submodule`, so a split that keeps one beside a `.gitmodules` that does not register it no longer says `adoption verified` when a plan check fails to refuse it (#165)

`_land_the_root_tree` counted the source's own submodule, kept in the root
at its own commit, as a `root` landing and nothing more. Since #166 and #168
no plan `execute` runs can put one there, so the only ways left are a defect
or a regression in those refusals, and in both the run said `adoption
verified` over an assembly whose fresh `.gitmodules` registers only the two
mounts: its recursive clone leaves that directory empty and `git submodule
status` exits 128 (lane openRepoShape-2's P2-1 on 05fa0e0). Every gitlink
other than the two mounts is now a finding: `adopt-root-submodule` for the
source's own at its own commit, `adopt-unexpected-gitlink` for anything
else, as before. It is still counted where it is, so a submodule left in a
leg and the root is both `adopt-duplicated` and `adopt-root-submodule`.

The findings' exit text no longer sends the person to re-spell
`legs.spec_path` first: #169, merged in 0448944, refuses a mount path git
would record at another spelling before any leg exists, so every one of
these findings means the split itself is wrong. #166's docstring and #162's
mount comment say the byte comparison of a root-kept `.gitmodules` is
deliberate, and name the finding the mount's defence in depth meets, instead
of reading as work #165 had still to do.

Tests: the root-kept case at test_adopt_verify.py:154 is flipped (it now
fails verification), the duplicated case gets both findings, a gitlink over
a source blob is `adopt-unexpected-gitlink`, the mount-level defence in
depth in test_adopt_submodules.py is shown to fail verification, and two
`needs_filter_repo` runs of `execute`: the nested-mount adoption is cloned
with `--recurse-submodules` after its work directory is moved away and
bootstrapped, and a root-kept plan with `_refuse_what_check_finds` taken away
exits 1 on `adopt-root-submodule`.

Lane: openreposhape-1 (openRepoShape-1)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant