Skip to content

validate.yml (assembly-root and family-root templates): the three url-reading steps read .gitmodules by the rules the doctor and siblings.py read it by, so a section git ignores, a section with no path, a path or url beginning with -, an unmerged file or a file git refuses whole no longer puts a repository in the App token or the lists, and a refused file fails the step with its cause (#786) - #790

Merged
brettheap merged 11 commits into
mainfrom
l3-786-ci-gitmodules-rules
Oct 18, 2026

Conversation

@brettheap

@brettheap brettheap commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Lane: openreposhape-3 (openRepoShape-3)
Claim: #786 (comment)

Closes #786.

Was stacked on #782, which has landed as squash a9d15797 and is merged in (see the merge round). The siblings rows skip until #775 is on main. The v1.1.0 tag is out (190133d5).

This branch holds #782's two commits (1bb59b6a, 67324e91), three of its own (85754e2f, 24df81cb, 31c3792c), the merge round's three merges, and one more of its own (e91b1a5d). Its diff against main is #786 alone: the changed lines of git diff 67324e91 31c3792c with the handbook markers recomputed, and the CR-row commit.

What changed and why

The two validate.yml templates read every .url key of .gitmodules, at three sites each: the names step (the repositories the App token is minted for), the fetch step's list of urls, and the failing step's list. So CI named repositories git never fetches. RV-c measured these on #782 (SHOULD 3): [submodule ""] and [submodule "../a"], a section with a url and no path, a [remote "origin"] url or [-x] url in .gitmodules, a bare url that makes git refuse the whole file, and url = -oProxyCommand=x. Where the App installation cannot reach such a repository, the mint step failed the job, though git would never fetch it.

Now all three sites, in both templates, read the file by the rules the doctor and siblings.py read it by (#733, #775's submodule_urls, git_dies_on and the constants):

  • Only a submodule.?*.url key counts (case $key in submodule.?*.url) ;; *) continue ;; esac).
  • A section whose name holds a .. component, between / or \, is skipped (case /$name/ in *[/\\]..[/\\]*)). An empty name is the ?*.
  • A section with no path is skipped.
  • A value beginning with - is ignored, that value alone, so an earlier one stands: both reads pass git a value pattern, --get -- "$key" '^([^-]|$)' for the url and --get -- "submodule.$name.path" '^([^-]|$)' for the path. url = A then url = -x is A, as git submodule init, the doctor and siblings.py have it.
  • Nothing is read while .gitmodules is unmerged: git ls-files -u -- .gitmodules must say nothing.
  • A file git refuses whole is refused. The step prints ::error::.gitmodules holds <cause>, so git refuses the whole file: it reads none of it, and no repository is named from it. git submodule status names the key and line git stops at. and exits 1, naming nothing. Each cause names the key git stops at, escaped as workflow-command data (% and CR): a path, url, ignore, update or branch key with no value; an update other than none, checkout, rebase or merge; a shallow that is no boolean; a fetchRecurseSubmodules that is neither a boolean nor on-demand (compared as git does, case-sensitively), each in a section git reads. A file git cannot parse prints git's own fatal: text instead: .gitmodules holds a line git cannot parse (bad config line 5 in file .gitmodules), so git refuses ….

#782's reading stays: --name-only, --get -- "$key", the line-break skip and the duplicate check.

How the refusal is read, and why. The file is read once as git config -z --file .gitmodules --list, mapped with tr '\000\n' '\n\001' so that each record is one line, key, then a \001 and the value when it has one. A key has no value at all only where the record has no \001, which is how a bare url is told from url =, and a value with a newline cannot start a record of its own. That is POSIX shell: no read -d '', no $'\n'. The booleans (shallow, fetchRecurseSubmodules) are asked of the runner's own git, git -c x.y=<value> config --type=bool --get x.y, so CI and git submodule agree on a value like 0x10 or 2 by construction, with no table of ours to drift.

Changed, with line numbers in the new files (the refusal, then the key rules, at each site):

  • templates/assembly-root/.github/workflows/validate.yml: the step comment :71-84; the names step :93-138 (refusal :94-127, its parse check :97, key rules :129-138); the fetch step :245-296 (refusal first, :247-282, then the advice :283, then the list :284-296); the failing step :344-357 (key rules only); the fetch and failing step comments :209-211 and :330-339.
  • templates/family-root/.github/workflows/validate.yml: the step comment :67-80; the names step :89-134 (refusal :90-123, its parse check :93, key rules :125-134); the fetch step :246-297 (refusal first, :247-283, then the advice :284, then the list :285-297); the failing step :339-352 (key rules only); the fetch and failing step comments :210-212 and :327-333.
  • tests/test_validate_workflow_gitmodules_urls.py: RULES and the tests below; the earlier fixtures gain the path each section needs and a git repository.

One behaviour to review

A .gitmodules that git refuses now fails the fetch step too, with or without a leg credential. Before, a failed fetch with no credential configured was a DEGRADE (green, the pin check skipped with a warning). That degrade is for private legs a default token cannot read. A file git refuses fails the fetch whatever the credential, so a credential cannot cure it, and a green run over a broken .gitmodules hid it. The refusal comes first in the fetch step, BEFORE the App advice (submodule checkout failed … configure a GitHub App …), which is the wrong advice for this file and is not printed. The names step and the fetch step each refuse on their own. The failing step carries no refusal: the fetch step exits 1 on a refused file, so the failing step, which runs only after a fetch that did not, is never reached with one (its comment says so). README.md, docs/handbook.html (markers recomputed), both template READMEs and the two failing-step comments said a run "fails outright only if a credential is configured"; each now says a .gitmodules git refuses fails the run whatever the credential, naming the key or line. README.md grows one line, tests/readme-cap.d/786.txt.

Round 2 (RV-c's review of 85754e2: NOT READY, three SHOULDs)

Round 3 (NIT, RV-c's review of 24df81c: READY)

Merge round (main 2e476a7)

#782 (#741) landed as squash a9d15797, #630 (#563) landed as 190133d5, and GitHub retargeted this PR to main. The round is three merges and one commit, a fast-forward of 31c3792c. The head is e91b1a5d.

  1. 564898ed merges validate.yml (assembly-root and family-root templates): each submodule url is read by its exact key, --name-only then --get, so a section named members/My IRRS gives the url git gives, where the split at the first space gave IRRS.url <url>, left that repository out of the App token and listed it wrong in both failure steps (#741) #782's final head a59cb537. Its tree is the squash's: git diff --stat a59cb537 a9d15797 is empty.
  2. 47b9d5fc merges the squash a9d15797.
  3. 17cf189a merges main 2e476a77. The merge is clean, and the tree is 1d73091f. It brings in siblings.py: the printed move runs as typed past a taken staging name and names a dangling link to move aside first, a NUL or lone surrogate in project: or url is a finding, .gitmodules is read as git reads it, the warning's prose is spelled, and a tag-only branch is named (#690) #805 (siblings.py (make siblings): the printed mv goes astray over an existing .holder/.folder or a dangling link, a lone surrogate or NUL in project: is a traceback, a url with NEL or U+2028 is cut, two warnings print paths raw #690).
  4. e91b1a5d drops the CR-row exclusion (below).

Two checks at 17cf189a:

  • git diff --name-only origin/main HEAD lists exactly this PR's 8 files.
  • Outside the handbook, the -U0 changed lines are byte-identical to those of git diff 67324e91 31c3792c (676 lines).

Skips

Markers and cap

  • All 23 handbook markers are current. handbook_marker_problems and handbook_footer_problems report nothing.
  • README.md is 1475 lines, at a cap of 1422 + 2 (460.txt) + 5 (550.txt) + 12 (563.txt) + 22 (564.txt) + 10 (606.txt) + 1 (683.txt) + 1 (786.txt) = 1475. tests/readme-cap.d/786.txt stays +1.
  • AGENTS.md is main's: 494 lines, at its cap of 494.

Tests at e91b1a5d, under a stub gh that exits 97, with GH_TOKEN=bogus and an empty GH_CONFIG_DIR:

set result
the 8 docs files: test_agents_md_codes, test_cli_reference, test_readme_sections, test_readme_windows_quoting_sentence, test_release_workflow, test_repo_hygiene, test_setup_by_hand_commands, test_update_add_codes_documented 300 passed in 40.82s
the gate set: test_validate_workflow_gitmodules_urls, test_scaffold_e2e, test_template_values_single_pass, test_template_quoted_values, test_readme_sections, test_setup_by_hand_commands, test_readme_windows_quoting_sentence, test_update_add_codes_documented 787 passed, 27 skipped in 65.12s (the 27 are the #775 skip)
tests/test_siblings_*.py 205 passed in 65.19s
the 15-file set under Tested 1133 passed, 27 skipped in 224.32s (0:03:44)
scripts/render-cli-reference.py --check [ok] docs/cli.md is what the generator produces
git diff --check origin/main...HEAD clean

Main since the round. Main is now 677f9bfa. Its two new commits, #852 and #808, change .github/workflows/ and tests, and none of this PR's 8 files. git merge-tree with the head is clean, giving tree 42f4d9ff. On that tree, test_readme_windows_quoting_sentence, test_release_workflow, test_repo_hygiene, test_readme_sections and test_validate_workflow_gitmodules_urls give 644 passed, 27 skipped in 47.88s.

Tested

RULES has one row per rule (28 rows), each a .gitmodules text (or none), the paths git submodule init is asked about, and the urls git registers for it:

  • a url key outside a submodule section ([remote "origin"], [-x], a bare [submodule]), an empty section name, names holding a .. component (../a, a/../b, ..\c, .., a/.., with a..b read), a url beginning with -, a path beginning with -, each given twice with the later beginning with -, a section with no path, no .gitmodules at all, a .gitmodules left unmerged in the index;
  • refusals: a file git cannot parse; a path, url, ignore, update and branch with no value; an update of bogus and of !echo hi, and one in a section named p%q and one named with a carriage return; a shallow of maybe; a fetchRecurseSubmodules of sometimes and of On-Demand;
  • reads: each update git accepts, each shallow git accepts (true, no, 1, 0x10, empty, bare), each fetchRecurseSubmodules git accepts (on-demand, true, 2, bare), and a bare key in a section git ignores ([submodule ""], [submodule "../x"], [remote "r"], [submodule "..\x"]), which refuses nothing.

Each row is checked three ways:

  1. test_git_registers_what_each_row_says: git submodule init -- <path> itself, so a row's expectation is git's.
  2. test_siblings_reads_what_each_row_says: siblings.submodule_urls from the template's siblings.py. It skips until shape-doctor.py and siblings.py: a .gitmodules section git ignores ([submodule ""], a .. component), a path or url beginning with -, a section with no path, a file git refuses for a bare url, and an unmerged .gitmodules name no mount url, NEXT line or clone, where git says No url found and exits 128 (#733) #775's rules (DOTDOT_COMPONENT) are in main, so on this PR's CI it skips; the unmerged row is not asked, since siblings.py reads the file in the working tree. Since the merge round the CR row is asked too. Run against shape-doctor.py and siblings.py: a .gitmodules section git ignores ([submodule ""], a .. component), a path or url beginning with -, a section with no path, a file git refuses for a bare url, and an unmerged .gitmodules name no mount url, NEXT line or clone, where git says No url found and exits 128 (#733) #775's head 8c5ad0e8 copied over the template (not committed), all 27 pass, so CI, the doctor and git agree on every row it is asked.
  3. test_each_step_reads_what_git_reads, test_each_step_exits_as_expected_and_prints_nothing_on_stderr and test_a_file_git_refuses_whole_is_named_with_its_cause_and_its_key: each step's run: body, unmodified, as real bash, in both templates: it names or lists exactly the row's urls, or (the names and fetch steps) exits 1 with an ::error:: naming the cause once, with its key or line, and nothing on stderr. Two more: the fetch step prints no credential advice for a refused file, and prints it before the list otherwise.

Mutants at 31c3792c, each applied at every site, each failing at least 2 of the 420 tests there: the url read without the pattern (10), the path read without it (6), no [ -f ] guard in the names step (2), on-demand compared case-insensitively (12), the refusal's .. test without \ (8), % not escaped (4), a cause that names no key (44), no CR escape in esc (4), no fatal: text in the parse message (4), keys not deduplicated when printed (4), "a ignore" (8), the refusal after the advice (2), no submodule.?*.url filter (6), no .. check (6), no path requirement (12), no unmerged check (6), no parse check (8), no shallow check (12), no fetchRecurseSubmodules check (24).

env PATH="$S/stub:$PATH" GH_TOKEN=bogus GH_CONFIG_DIR="$S/ghcfg" /usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=$S/bt -o tmp_path_retention_policy=failed tests/test_validate_workflow_gitmodules_urls.py tests/test_scaffold_e2e.py tests/test_family.py tests/test_template_values_single_pass.py tests/test_repo_hygiene.py tests/test_cli_reference.py tests/test_scaffold_pin_and_reuse.py tests/test_windows_paths.py tests/test_validate_pins_unreadable_workflow.py tests/test_validate_pins_unreadable_workflows_dir.py tests/test_readme_sections.py tests/test_agents_md_codes.py tests/test_readme_windows_quoting_sentence.py tests/test_setup_by_hand_commands.py tests/test_update_add_codes_documented.py
1133 passed, 27 skipped in 224.32s (0:03:44)

(At e91b1a5d; at 31c3792c it was 1115 passed, 26 skipped. $S/stub/gh exits 97. The 27 skipped are the siblings.py rows. The set includes the README line cap and the handbook markers. With #775's siblings.py (8c5ad0e8) in place, tests/test_validate_workflow_gitmodules_urls.py alone is 421 passed in 36.62s.) actionlint 1.7.12 with shellcheck is clean on both templates. The new tests are WINDOWS_SKIP like the ones beside them, since they run the steps as real bash and CI is Linux only; the git submodule init rows run anywhere.

Pinned copies

Both files are pinned template copies, carried by digest (scripts/shape_materialize.py:455 and :539). Each project and each holder that carries the shape sees one upstream-changed row for .github/workflows/validate.yml until it takes the new copy with update-shape.py. #782 changes the same two files, so one row covers both. The two template READMEs are rendered, not pinned.

Not changed, and the edges

Overlap

#782, which this was stacked on, and lane 3's #630, which edited README.md and docs/handbook.html, have both landed and are merged in (see the merge round). #775 changes siblings.py and shape-doctor.py, not these files. README.md and docs/handbook.html put this PR in lane 3's README turn.

🤖 Generated with Claude Code

Summary by Sourcery

Make validation workflows derive submodule repositories from Git-readable .gitmodules content and fail clearly when Git rejects the file.

Bug Fixes:

  • Make validation workflows report and fail on .gitmodules files that Git refuses to read, including the specific invalid key or parse line, regardless of configured credentials.
  • Ensure repository naming and URL reporting follow Git's submodule-reading rules so ignored sections, missing paths, unmerged files, and dash-prefixed values are not included.

Enhancements:

  • Align assembly-root and family-root validation workflow handling with the repository doctor and sibling discovery rules.
  • Expand workflow coverage for valid, ignored, malformed, refused, and edge-case .gitmodules configurations across both templates.

Documentation:

  • Document that Git-refused .gitmodules files fail validation independently of credential configuration.

Tests:

  • Add comprehensive tests comparing workflow results with Git's registered submodule URLs and refusal behavior.

Brett Heap and others added 3 commits October 17, 2026 04:01
…e url is read by its exact key, `--name-only` then `--get`, so a section named `members/My IRRS` gives the url git gives, where the split at the first space gave `IRRS.url <url>`, left that repository out of the App token and listed it wrong in both failure steps (#741)

The templates read `.gitmodules` three times each, and each read split
`git config --get-regexp '\.url$'`'s `key value` lines at the first space
(`| cut -d' ' -f2-`, `| while read -r _ url`). A section name may hold a
space, so for `members/My IRRS` the url came out as `IRRS.url <url>`: the
names step matched it to neither `https://github.com/` nor `git@github.com:`
and left the repository out of the App-minted token, and the fetch step and the
failing step printed it wrong. #700 made the doctor read such a name; CI now
agrees with it.

Now `--name-only --get-regexp` lists the keys, one per line (git refuses a
newline in a key), and `git config --get "$key"` asks for each url, as git
reads it: the last value of a section given twice, a url holding a space, a
tab, `#`, `;` or a leading `-` whole. That is POSIX shell, so the two printing
steps stay what they were, and the names step keeps its bash `< <(...)`.

The issue's other way, `-z` with each record split at its first newline, was
not taken: it needs `read -d ''` and `$'\n'`, which are bash, in two steps that
are POSIX shell today, and a key with no value is a record with no newline to
split at.

tests/test_validate_workflow_gitmodules_urls.py runs each of the three steps'
`run:` bodies unmodified, in both templates, over a `.gitmodules` whose section
names hold a space, a tab, and a leading `-` with `#` and `;`, and whose one
url holds the same bytes; all eight tests fail on the old templates.

Both files are pinned template copies: each project and each holder sees one
`upstream-changed` row for `.github/workflows/validate.yml`.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… a line break or a carriage return is skipped, each repository is named once, and each url is read with `--get --`, so a url cannot print a workflow command of its own, `legs` and `members` hold no repository twice, and `[-x] url` is read without git's usage dump (#741)

Round 2 of #782, from RV-c's review.

- A url holding a newline (git's `\n` escape inside quotes) or a carriage
  return is skipped right after it is read, at all six sites. Main prefixed
  every printed line with `::warning::  `; the first round wrote that url's
  second line to `$GITHUB_OUTPUT` and printed it as a line of its own, which a
  runner reads as a command (`::error title=x::injected`). git cannot clone
  such a url.
- The names step names a repository once: `case ",$legs," in *",$name,"*)
  continue ;; esac` before each append (`$members` in the family root). The
  first round gave `legs=A,A` for a url given twice in one section, a section
  reopened further down, and two paths mounting one repository; the pinned
  create-github-app-token sends the list to GitHub's token API as it is.
- `--get -- "$key"`. `--get-regexp '\.url$'` lists any section's url key, not
  only `submodule.*`, and `[-x] url = ...` makes `git config --get -x.url` read
  `-x` as a switch: a usage dump on stderr, exit 129, and the `|| continue`
  swallowing it. The first round's note that every key starts `submodule.` was
  wrong.

tests/test_validate_workflow_gitmodules_urls.py: the shared `.gitmodules` holds
a newline row and a carriage-return row, and each of the three steps in both
templates prints no `::` line but its own and writes only `key=value` lines to
`$GITHUB_OUTPUT`; the names step is run over the three duplicate cases; and each
step is run over a `[remote "origin"]` and a `[-x]` url key with stderr asserted
empty. Against the first round's templates 26 of the 32 tests fail; a site
without `--`, without the line-break skip (or its carriage-return half), or
without the duplicate check fails 6, 14, 14 and 6.

Left for a follow-up issue, as RV-c asked: sections git ignores (`[submodule
""]`, a url with no path, a `[remote]` url named as a repository), which main
reads too.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…-reading steps read `.gitmodules` by the rules the doctor and `siblings.py` read it by, so a section git ignores, a section with no path, a `path` or `url` beginning with `-`, an unmerged file or a file git refuses whole no longer puts a repository in the App token or the lists, and a refused file fails the step with its cause (#786)

Stacked on #782 (#741); lands after it and after #775 (#733).

The names step, the fetch step's list of urls and the failing step's list all
read every `.url` key of the file, so CI named repositories git never fetches:
`[submodule ""]`, a `..` name, a url with no path, a `[remote "origin"] url`
and `[-x] url` in `.gitmodules`, a bare `url` that makes git refuse the file,
`url = -oProxyCommand=x`. Where the App cannot reach such a repository, the
mint step failed the job. Now, at all six sites:

- only a `submodule.?*.url` key counts (a plain `case` match);
- a section whose name holds a `..` component is skipped, between `/` or `\`
  (an empty name is the `?*`);
- a section with no `path` is skipped, and so is a `path` or a `url` that
  begins with `-`;
- nothing is read while `.gitmodules` is unmerged in the index;
- a file git refuses whole is refused: one it cannot parse, a `path`, `url`,
  `ignore`, `update` or `branch` with no value, an `update` other than none,
  checkout, rebase or merge, a `shallow` that is no boolean, a
  `fetchRecurseSubmodules` that is neither a boolean nor on-demand, each in a
  section git reads. The step prints `::error::` naming the cause and exits 1,
  and names nothing: git reads none of the file. The booleans are asked of the
  runner's own git (`git -c x.y=<value> config --type=bool`), so CI and `git
  submodule` agree by construction. The file is read once, as `-z --list`
  records mapped to one line each with `tr`, so the check is POSIX shell and a
  value with a newline cannot start a record.

#782's `--name-only` and `--get -- "$key"` reading, its line-break skip and its
duplicate check stay.

tests/test_validate_workflow_gitmodules_urls.py: `RULES` has one row per rule
(21 rows). Each row is checked three ways: `git submodule init` itself says
what git registers, `siblings.submodule_urls` of #775's head 3fa5bee agrees
(that test skips until the rules are in main; run against that head, all 20 of
its rows agree), and each of the three steps in both templates names or lists
exactly that, or fails with the cause. The earlier fixtures gain the `path`
each section needs.

Both files are pinned template copies: each project and each holder sees one
`upstream-changed` row for `.github/workflows/validate.yml`.

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

sourcery-ai Bot commented Oct 17, 2026

Copy link
Copy Markdown

Reviewer's Guide

Updates both validation workflow templates so all three .gitmodules URL-reading sites consistently mirror Git’s parsing and filtering behavior, fail clearly on files Git refuses, and avoid unsafe or nonexistent repositories; adds extensive cross-template tests covering extraction, refusals, deduplication, and shell behavior.

Sequence diagram for validated gitmodules repository discovery

sequenceDiagram
    participant Workflow as Validate workflow
    participant Gitmodules as .gitmodules
    participant Git as Git config
    participant Output as Token or URL list
    Workflow->>Git: ls-files -u -- .gitmodules
    alt file is unmerged
        Git-->>Workflow: conflict entries
        Workflow-->>Output: name nothing
    else file is resolved
        Workflow->>Git: config --file .gitmodules --list
        alt Git refuses the file
            Git-->>Workflow: refusal cause
            Workflow-->>Output: ::error:: and exit 1
        else file is readable
            Workflow->>Git: config --name-only --get-regexp \.url$
            loop each candidate key
                Workflow->>Git: config --get -- submodule.name.url
                Workflow->>Git: config --get -- submodule.name.path
                Workflow->>Workflow: apply submodule, name, path, URL, and line-break rules
            end
            Workflow->>Output: emit deduplicated valid repositories
        end
    end
Loading

Flow diagram for gitmodules refusal and URL filtering

flowchart TD
    A[Read .gitmodules] --> B{Unmerged index?}
    B -->|Yes| C[Read nothing]
    B -->|No| D{Git accepts whole file?}
    D -->|No| E[Print refusal cause and exit 1]
    D -->|Yes| F[Enumerate exact URL keys]
    F --> G{Valid submodule name?}
    G -->|No| F
    G -->|Yes| H{Has valid path and URL?}
    H -->|No| F
    H -->|Yes| I{URL already emitted?}
    I -->|Yes| F
    I -->|No| J[Add repository to token or diagnostic list]
    J --> F
Loading

File-Level Changes

Change Details Files
Align all .gitmodules URL extraction with Git’s effective parsing rules across the token-name, fetch-warning, and failure-reporting workflow steps.
  • Replace first-space parsing with --name-only plus per-key --get reads.
  • Ignore non-submodule keys, unsafe section names, missing or dash-prefixed paths/URLs, unmerged files, and newline/CR URLs.
  • Deduplicate repositories when constructing App token inputs.
  • Apply the same filtering logic to both assembly-root and family-root templates.
templates/assembly-root/.github/workflows/validate.yml
templates/family-root/.github/workflows/validate.yml
Detect .gitmodules files that Git rejects entirely and fail workflow steps with a diagnostic cause before naming or listing repositories.
  • Validate parseability and scan NUL-delimited Git config records for invalid or valueless submodule settings.
  • Validate supported update, shallow, and fetchRecurseSubmodules values using Git’s own config parser.
  • Make refused files fail the fetch path even when credential-related failures previously degraded to success.
templates/assembly-root/.github/workflows/validate.yml
templates/family-root/.github/workflows/validate.yml
Add comprehensive behavioral tests comparing both templates’ shell steps against Git’s submodule interpretation.
  • Exercise valid, ignored, unsafe, duplicate, unmerged, malformed, and refusal cases.
  • Run unmodified workflow step bodies under Bash and verify outputs, exit status, stderr, and refusal diagnostics.
  • Cross-check expected registrations with git submodule init and, when available, siblings.py.
tests/test_validate_workflow_gitmodules_urls.py

Assessment against linked issues

Issue Objective Addressed Explanation
#786 Make both validate.yml templates include only repositories Git recognizes from .gitmodules, excluding ignored section types, empty or invalid names, sections without a path, and paths or URLs beginning with a dash. ✅
#786 Have all three URL-reading steps in both templates read nothing from an unmerged .gitmodules and fail clearly when Git would refuse the file, including the specified invalid key values. ✅
#786 Add tests for both templates that compare their behavior across the three steps with Git’s submodule behavior for the relevant .gitmodules rules. ✅

Possibly linked issues


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-17T09:36:50Z) Independent review of #790 by RV-c (opus), on head 85754e2f (stacked on #782): NOT READY. No MUST; three SHOULDs, each small. On all 21 RULES rows, CI agrees with git, the doctor and siblings.py (#775 at 3fa5beed), in both templates and all three steps. Rendering is byte-identical, actionlint is clean, and the merges are clean.

SHOULD 1: a later - value drops a section that git, the doctor and siblings all read.

  • Where: the url= and mount= reads, assembly :126-127, :273-274, :362-363 and family :122-123, :274-275, :356-357.
  • Scenario: url = <A> then url = -x. git registers A, ignoring -x alone, and the doctor and siblings give A. CI names only the other sections, so A leaves the App token and both failure lists. A path given twice splits the same way.
  • The PR's reason does not hold: --get takes a value pattern and still returns one whole value:
values result
[A, -x] A
[A, B] B
[-x] rc=1
[-x, A] A
[A, "B\n::error::x"] the whole second value, which the line-break skip then drops
  • Fix: at all 12 reads, use --get -- "$key" '^([^-]|$)' and --get -- "submodule.$name.path" '^([^-]|$)'. Reword the step comment, and add both cases as rows.
  • Tried in scratch: actionlint 0, 0 disagreements, 360 passed.

SHOULD 2: the project cannot see which line or key to fix.

SHOULD 3: red is right, but the docs say otherwise. No credential can cure a file git refuses. These still say a run "fails outright only if a credential is configured":

  • README.md:659-666 and docs/handbook.html:1139-1142;
  • the fail-step comments, assembly :313-321 and family :310-315;
  • templates/family-root/README.md:139-140 ("it still runs the checks that do not need the members").

Add one clause to each, saying a .gitmodules git refuses fails the run whatever the credential. README and handbook change together, which puts this PR in lane 3's README turn.

Optionally, record the refusal as a fetch-step output and fail in a final step, so the checks that need no legs still run. The mint must stay skipped then: an empty repositories: mints a token for the whole installation.

Items 3 and 4 are fine. The runner's own git answers both the range check and the fetch. A CI checkout never has an unmerged index.

NITs:

  • N1: 7 of 13 mutants survive. Three change behaviour on real files:
    • No [ -f .gitmodules ] guard in the names step: an App-configured project with no .gitmodules goes red. Add a row.
    • on-demand compared case-insensitively: git dies on On-Demand. Add a refusal row.
    • The refusal loop's .. test without \: a bare ignore in [submodule "..\x"] refuses the whole file though git reads the rest. Add a row.
  • N2: CI-vs-git edges, each pre-existing or pathological:
    • a section whose path is no gitlink (git rm --cached);
    • two sections for one path;
    • path = with no value;
    • a \x01 name with a bare url;
    • shallow = "\n1";
    • a refused file with no gitlinks.
  • N3: the fail step's refusal block (assembly :326-357, family :320-351) can never fire, because the fetch step has already exited 1.
  • N4 (body): the warning prints before the refusal, and the parse case's message differs from the body's description.

Outside this PR: siblings.py (main and 3fa5beed, :292-304) splits --list output at the first =, so it drops [submodule "a=b"], which git, the doctor and CI all read. Lane 3 files it.

Runs:

Merges: clean with main, with main + #775, and with every other open lane-3 PR except #791, whose conflict is with main itself.

Lane: openreposhape-3 (openRepoShape-3)

…alue is ignored alone as git ignores it, a refused file is named with its key or line before any credential advice, and a `.gitmodules` git refuses fails the run whatever the credential, which README, handbook and the template READMEs now say (#786)

Round 2 of #790, from RV-c's review.

- SHOULD 1: the `url` and `path` reads pass git a value pattern,
  `--get -- "$key" '^([^-]|$)'`, so a value beginning with `-` is ignored and
  the earlier one stands, as git, the doctor and `siblings.py` have it
  (`url = A` then `url = -x` is A). The round's reason, that `--get` returns
  one whole value, held for the line-break skip and not against a pattern.
  The `-` cases that followed the reads are gone.
- SHOULD 2: a file git cannot parse prints git's own `fatal:` text (`bad
  config line 5 in file .gitmodules`) and every other cause prints the key it
  stops at, both escaped as workflow-command data (`%` and CR); both say `git
  submodule status` names the key and line git stops at; "an ignore key" and
  "an update key"; and the fetch step refuses BEFORE it gives its App advice,
  which a refused file never gets. Printed with `printf '%s\n'`, which dash
  does not read backslashes in as `echo` does.
- SHOULD 3: README.md, docs/handbook.html (its markers recomputed), both
  template READMEs and the two failing-step comments say a `.gitmodules` git
  refuses fails the run whatever the credential. README.md grows a line, so
  tests/readme-cap.d/786.txt.
- N1: rows for no `.gitmodules` at all, `On-Demand` (git dies on it), and a
  bare `ignore` in a section named `..\x`; the names step's `[ -f ]` guard, the
  case-sensitive `on-demand` and the refusal's `..` test at `\` are held by
  them.
- N3: the failing step's refusal block is gone, since the fetch step exits 1
  on a file git refuses and this step is never reached with one.
- The two printing steps list a key once, `awk '!seen[$0]++'`: `--get-regexp`
  lists a key once per value, and git registers one url for it.

tests/test_validate_workflow_gitmodules_urls.py: 27 rows (21 and six), each
checked against `git submodule init`, `siblings.submodule_urls` of #775's head
3fa5bee and the steps; the cause test asserts the key or line; the fetch step
gives no advice for a refused file and gives it before the list otherwise.

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

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-3, 2026-10-17T10:12:50Z) Re-verification of #790 round 2 by RV-c (opus), on head 24df81cb (a fast-forward of 85754e2f): READY. No MUST, no SHOULD, three NITs. All three round-1 SHOULDs are fixed, each measured.

Agreement: all 27 rows agree in both templates and every step that can see them (54 of 54). The names and fetch steps agree with git submodule init, the doctor and siblings.py (#775 at 3fa5beed). The fail step agrees on every row that reaches it.

Checked:

  • S1: all 12 reads take '^([^-]|$)'. On both new rows, git, the doctor, siblings and CI all give the earlier value. Dropping the pattern is caught at all 6 sites of each read.
  • S2:
    • The parse case prints (bad config line 5 in file .gitmodules).
    • Each cause names its key, for example (submodule.q.url); % prints as %25 and a CR as %0D.
    • The grammar is fixed, and the refusal comes before the App advice (tested).
    • LF needs no escape: git refuses a literal LF in a section name, a quoted \n reads as n, and the cause texts are fixed single lines. So esc is complete.
  • S3: all seven places are true in both templates. The README cap ledger (tests/readme-cap.d/786.txt, +1) and the recomputed handbook markers pass.
  • N1: the three new rows kill round 1's three meaningful survivors.
  • N3: the skip is honest.
    • A failed submodule update makes the fetch step run the same check and exit 1, which skips the fail step.
    • A successful one sets legs_available=true.
  • N4: the body matches the code.
  • The unrequested awk '!seen[$0]++' dedupe:
    • It never hides a different section, since two sections always have different keys.
    • It matches git's set: a reopened a, plus b, plus [submodule "A"] lists A, B, C, as git does.
    • Without it, the "url given twice" row fails.

NITs:

  1. The CR escape has no test.
    • Removing -e "s/$(printf '\r')/%0D/g" from esc (A :96, :250; F :92, :251) survives, and the names step then prints a raw CR in its ::error:: line (measured).
    • Add a refusing row whose section name holds a CR, with a plain path: section("q\rx", "path = q", …, "update = bogus"), expecting submodule.q%0Dx.update.
  2. Three harmless survivors remain: no break after the first cause, and splitting at the last \001 instead of the first. They matter only for a file with two causes or a value holding a newline.
  3. Out-of-scope edges remain, as measured: empty path =, a path that is not a gitlink, two sections giving one path, \x01 in a name with a bare url, and shallow = "\n1". One more matters for the docs: a refused file with NO gitlinks. git exits 0 there, so with no App the run is green, and the new sentence "fails the run whatever the credential" overstates.

For lane 4's #690: siblings.py also splits a CR in a section name (splitlines() without -z). The -z read fixes this too; it is noted on #690.

Runs:

Merges:

Lane: openreposhape-3 (openRepoShape-3)

…on named with a carriage return is held, so the CR escape in `esc` is tested (#786)

Round 3 (NIT) of #790, from RV-c's review of 24df81c.

`esc` turns `%` and a carriage return into `%25` and `%0D` before a key or git's
`fatal:` text is printed in an `::error::` line, and nothing held the CR half:
removing `-e "s/$(printf '\r')/%0D/g"` at its four sites (assembly `:96`,
`:250`, family `:92`, `:251`) left every test passing, and a raw CR landed in
the message. A row now has a section named `q\rx` (path `q`, `update = bogus`),
and the cause test asserts `submodule.q%0Dx.update`. The mutant fails four
tests, the names and fetch steps in both templates.

`refusing` takes the path a section is mounted at, so a name with a CR keeps a
plain path and the `git submodule init` rows still run on Windows.

The `siblings.submodule_urls` comparison does not ask this row: siblings.py
cuts `git config --list` with `splitlines`, which ends a line at a carriage
return, so it reads `ok` and never the refused `update`, where git reads none.
Beside its split at the first `=`, which lane 3 files.

No template changes.

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

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-3, 2026-10-17T10:18:56Z) #790's NIT round, 24df81cb..31c3792c: NIT 1 is closed, and RV-c's READY stands. Lane 3 checked it directly; it is a test-only round.

Lane: openreposhape-3 (openRepoShape-3)

brettheap and others added 5 commits October 17, 2026 18:17
…parent this branch was stacked on, as it landed (#786)

#782 (#741) landed as squash a9d1579, whose tree is a59cb53's (`git diff
--stat a59cb53 a9d1579` is empty). a59cb53 is #782's last merge of main,
so this brings in main as #782 landed on it, #630's v1.1.0 docs (190133d)
among it.

The one conflict is docs/handbook.html, on the three `§ The three legs`
markers (masthead, `ci`, `legs`): main's 1d9f5bf9, from #630's reading,
against this branch's a1299381, from #790's. README.md merged clean with both
sides standing: #630's text, and #790's sentence in § Reading private legs in
CI. All three markers are set to 4ec313e8424665e41fc92a4147808e0bd4901d78,
the blob of the merged README's § The three legs as `readme_segments` and
`readme_fingerprint` in tests/test_repo_hygiene.py compute it (`git
hash-object --no-filters` of the segment agrees). The page already says what
each side changed in that segment, #630's in its own sections and #790's
sentence in `ci`, so moving the markers is the whole of the change here.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…main holds it, its three files taken from this branch (#786)

a9d1579 is #782 (#741) squashed onto main; its tree is that of a59cb53,
merged in the commit before this one, so this merge changes no file. It
conflicts in the three files #782 changed, both templates'
`.github/workflows/validate.yml` and tests/test_validate_workflow_gitmodules_urls.py,
because the squash carries #782's lines as new ones; each takes this
branch's side, which is #782's final content with #790's own changes on top.
The tree is the parent's, 9f07f48.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…anded, among it #805's `-z` read of `.gitmodules` in siblings.py (#786)

A clean merge. It brings in what main took after #782's squash, among it
#805 (#690, 40e678e), which reads `.gitmodules` in the family template's
siblings.py with `git config -z --list`. README.md stays 1475 lines, at
the cap of 1475 that tests/readme-cap.d/ gives with this branch's 786.txt
(+1); AGENTS.md is main's, 494 lines at its cap of 494; and every
handbook marker is current. The tree is 1d73091, what one merge of main
into 31c3792 gives with the same resolutions.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ask the row whose section name holds a carriage return, since `siblings.py` has read `.gitmodules` with `-z` since #805 (#690) (#786)

`test_siblings_reads_what_each_row_says` left out every row holding a CR,
because `siblings.py` cut `git config --list` at `splitlines` and read the
section `q\rx`'s `update = bogus` as the key `submodule.q` and `x.update`.
#805 (40e678e), which closed #690, reads `git config -z --list`, so the CR
stays inside the key and `update` is read as `update`. The exclusion and the
docstring's sentence about it go; the docstring now says the row is asked.

The rows still skip, all 27 of them, until #775's rules are in the family
template's `siblings.py` on main: the `siblings` fixture skips where
`DOTDOT_COMPONENT` is missing, and that skip stays.

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

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-3, 2026-10-18T00:08:49Z) RV790n's re-check of #790's merge round (31c3792c..e91b1a5d): READY. No MUST and no SHOULD. Its one NIT is the PR body's stale main and README figures, which the squash message does not use. Every merge is exact and the round changed no template byte. All 23 handbook markers are current, CI is green on e91b1a5d, and the predicted squash over e62b5e17 passes the docs, gitmodules and siblings tests.

RV790m, the first re-checker, stalled before it measured anything. RV790n started fresh and used none of its files.


RV790n re-check of #790 (#786) at head e91b1a5d

RV790n verdict: READY for the merge round 31c3792c..e91b1a5d. No MUST, no SHOULD, one NIT. The PR head is still e91b1a5dba55ca090b03dd3568f4a5efa71b2bf8 and origin/main is still e62b5e1734405efb71ac78075d16c6ff9acbb5ad; I re-fetched at 00:03Z and neither moved. I made no GitHub write. I did not read or reuse anything under RV790m.

Checks

1. Each merge is exact. I recomputed each with git merge-tree --write-tree, applied the stated resolution in a scratch detached worktree, and compared trees with git rev-parse <commit>^{tree}.

Commit Parents Result
564898ed 31c3792c + a59cb537 merge-tree conflicts only in docs/handbook.html, 3 hunks, each the § The three legs marker (lines 477, 1133, 1200 of the conflicted blob, a1299381 against 1d9f5bf9). I set all three to 4ec313e8424665e41fc92a4147808e0bd4901d78. The resolved tree is 9f07f48f78d4ae4cb924b24b32da7fb482a9c58b, equal to the commit's tree.
47b9d5fc 564898ed + a9d15797 merge-tree conflicts in both validate.yml templates and tests/test_validate_workflow_gitmodules_urls.py (add/add). With git checkout --ours on those 3 files the tree is 9f07f48f78d4ae4cb924b24b32da7fb482a9c58b, equal to the commit's tree.
17cf189a 47b9d5fc + 2e476a77 merge-tree is clean, tree 1d73091f410f9281c43a39820db19f11a867e2b2, equal to the commit's tree. It touches install.sh, openRepoShape, tests/test_install_sh.py and tests/test_openreposhape_command.py, none of the PR's 8 files.
e91b1a5d 17cf189a one file, tests/test_validate_workflow_gitmodules_urls.py (+5/−6): it drops and "\r" not in (rule.gitmodules or "") and rewords the docstring. git diff --check is clean.

2. Step 2 changed nothing. git diff 564898ed 47b9d5fc on the 3 files is empty, and so is the whole-tree diff. The blobs are identical: assembly 731464c0, family 5da6fc2c, test 138abadd.

3. The PR carries only its own change.

4. Markers. I ran the head's tests/test_repo_hygiene.py helpers on a detached worktree of e91b1a5d:

  • handbook_marker_problems and handbook_footer_problems both return [].
  • _handbook_markers finds 23 markers, none stale.
  • The three The three legs markers (lines 477, 1129, 1192) all read 4ec313e8…. readme_fingerprint and git hash-object --no-filters agree on that blob.
  • README is 1475 lines (splitlines) at a cap of 1422 + 2 + 5 + 12 + 22 + 10 + 1 + 1 = 1475.
  • Mutants: setting the first marker back to 1d9f5bf9 or to a1299381 each fails tests/test_repo_hygiene.py::test_the_handbook_follows_the_readmes_outline (1 failed, 153 passed).

5. The CR row ("an update in a section named with a carriage return").

6. Tests on e91b1a5d, under a stub gh that exits 97, with GH_TOKEN=bogus and an empty GH_CONFIG_DIR, targeted files only:

7. Merge with current origin/main. git merge-tree --write-tree origin/main e91b1a5d is clean.

  • The predicted squash tree is 1f69f54392c1f1f926ecc0dd8db16b254d1e3a93, over main e62b5e1734405efb71ac78075d16c6ff9acbb5ad.
  • The scratch commit is 947805a4095e04df486f5affa5ebd8bb85c8d3f0, never pushed. Its diff against main is exactly the 8 files (+613/−71).
  • The same merge with 677f9bfa gives 42f4d9ff, as the PR body says.

On a detached worktree of 947805a4:

gh pr checks 790 (read-only)

Check Result Link
SonarCloud Code Analysis pass, 19s https://sonarcloud.io/dashboard?id=opensoft_openRepoShape&pullRequest=790
package pass, 27s https://github.com/opensoft/openRepoShape/actions/runs/39075735812/job/117522383531
tests pass, 53m48s https://github.com/opensoft/openRepoShape/actions/runs/39075735812/job/117522383739
tests-git-bash pass, 2m9s https://github.com/opensoft/openRepoShape/actions/runs/39075735812/job/117522383740

Run 39075735812 has headSha e91b1a5dba55ca090b03dd3568f4a5efa71b2bf8. The Sonar comment reports 0 new issues and 0.0% new duplication.

Items

Scratch paths (clone and scratch root; -wt-m1 and -wt-m2 hold uncommitted in-progress merges)

Lane: openreposhape-3 (openRepoShape-3)

@sonarqubecloud

Copy link
Copy Markdown

@brettheap
brettheap marked this pull request as ready for review October 18, 2026 00:18
@sourcery-ai

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

@brettheap
brettheap merged commit 7b21059 into main Oct 18, 2026
5 checks passed
@brettheap
brettheap deleted the l3-786-ci-gitmodules-rules branch October 18, 2026 00:18
brettheap pushed a commit that referenced this pull request Oct 18, 2026
…trap_after_bump.py` imports both `appended_to_command_scope` and `needs_a_byte_name` (#715)

Main moved from `0dee22b3` to `ca47b71d` while round 3 was paused (#846, #779, #772, #738, #842, #814, #790, #820, #774, #830 and #841). `git merge-tree` of `8b4dbcf6` with it stopped in one path:

- `tests/test_bootstrap_after_bump.py`, its imports. Main's `aec3b38f` (tests append what they tell git to the command scope) added `appended_to_command_scope` to the `from conftest import (...)` list. This branch added `from test_bootstrap_spelling import needs_a_byte_name` below it (#715, round 1). Both are kept.

`scaffold-project.py`, `scripts/bump-leg.py` and `scripts/shape_materialize.py` merged without a conflict.

Lane: openreposhape-4 (openRepoShape-4)
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