Skip to content

update-shape.py: apply --add force-stages its additions past the operator's own excludes, and a branch, stage or commit that fails before the commit lands now puts back the copies, the pin, the README line, the index and the branch and names the step, so check no longer exits 0 over an uncommitted re-pin (#202) - #233

Merged
brettheap merged 4 commits into
mainfrom
fix/update-shape-add-rollback
Oct 11, 2026

Conversation

@brettheap

@brettheap brettheap commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Lane: openreposhape-4 (openRepoShape-4)
Claim: #202 (comment)

Fixes #202.

What

The branch is 346bccf (the fix, and one updated assertion in tests/test_update_shape_add.py) plus c022b64 (tests only). 346bccf's subject starts WIP (#202):, and the no-force-push ruleset keeps it that way; this PR's title is the squash subject.

Why (repro on main)

A plain git add refused an --add path that the operator's own excludes ignore. By then the copies, the re-pin, the validators and checkout -b had already run, and nothing was undone. apply exited 2 and left the checkout on the new branch, with three modified files and the ignored addition. The re-run without --add said "nothing to do", exit 0, and so did check. Every later run read an update that existed only as uncommitted bytes as done, against AGENTS.md's promise that apply rolls every byte back.

On this head the same sequence exits 0, and the commit holds AGENTS-shape.md, contracts/shape-pin.yaml, project.yaml and scripts/validate-manifest.py.

Tests

  • New file. tests/test_update_shape_add_ignored.py has 15 tests:
    • an addition ignored through .git/info/exclude, through a global core.excludesFile, and through a nested .gitignore, each committed with the root still validating green;
    • an addition nothing ignores, landing exactly as before;
    • eight failures injected before the commit (staging, a pre-commit hook, two branch-creation failures, a branch that exists, the current branch, a detached HEAD, an already-staged path). Each puts back every byte, mode, directory, HEAD, branch, index record and git status --ignored, under PYTHONDONTWRITEBYTECODE=1, names the step, and leaves check printing what it printed before;
    • a failure after the commit lands, rolling nothing back;
    • force_stage as a unit.
  • Targeted runs. The targeted set (test_update_shape.py test_update_shape_add.py test_update_shape_add_ignored.py test_scaffold_ignored_shape_files.py test_shape_doctor.py test_cli_reference.py) was 302 passed on this head. Ten test files on this head merged with main a5b09c3 were 470 passed.
  • Mutants. Sixteen mutants of the rollback and the forced add were all caught by the new tests.
  • Review. An adversarial review (lane openRepoShape-4) found the head MERGEABLE WITH NITS, with no blocking finding. Its SHOULD is a test gap and the code is right there: no failure test stages an unrelated change of the operator's first, so a rollback that reset the whole index would still pass. That test, the review's literal-pathspec hardening of force_stage (git --literal-pathspecs add -f, so a copy-list name with a glob character cannot force in an ignored file it matches; none has one today), and its other nits follow as one more commit on this branch, together with anything CI or review turns up.

macOS and Windows have not run these tests yet; this PR's CI is the first time.

Landing notes

🤖 Generated with Claude Code

Summary by Sourcery

Make shape updates force-stage only named additions and fully restore repository state when an update fails before its commit is created.

New Features:

  • Force-stage explicitly named additions so files excluded by repository, local, or global ignore rules are included without staging unrelated changes.

Bug Fixes:

  • Restore written files, index state, HEAD, and newly created branches when staging, branch creation, or committing fails before the commit lands.
  • Preserve post-commit results without rollback when a later push fails.
  • Prevent pathspec glob expansion in generated NEXT commands and handle Git output using UTF-8 consistently.

Enhancements:

  • Report the failed operation and provide a detailed account of restored or unrecovered state, including actionable recovery commands for partial rollbacks.
  • Track and restore pre-existing index entries exactly, including staged and unmerged records, while protecting the branch currently checked out.

Documentation:

  • Update apply behavior and exit-code documentation to describe rollback of failures before commit completion.

Tests:

  • Add comprehensive coverage for ignored additions, literal path handling, pre- and post-commit failures, index and branch rollback, partial rollback reporting, non-ASCII branches, and generated recovery commands.

brettheap and others added 2 commits October 11, 2026 16:38
…past the operator's own excludes, and a failed branch, stage or commit now rolls back the copies, the pin, the README line, the index and the branch and names the step, so `check` no longer exits 0 over an uncommitted re-pin

`commit_on_branch` staged an `--add` file with a plain `git add --`, which
honours `.git/info/exclude` and a global `core.excludesFile`; an ignored
addition failed AFTER the copies, the re-pin, the validators and `git
checkout -b`, and `cmd_apply`'s `except CommandFailed` arm returned 2 with
none of it undone. The additions now go through `shape_materialize.
force_stage` (`git add -f --` on the named paths, never `-A`), which
`stage_written` (#175) now calls for its own forced half, so there is one
spelling.

The `Rollback` ledger now records the branch it creates and the index
entries it forces, as well as the bytes, until `land()` closes it when the
commit exists. `cmd_apply`'s one failure arm (`_rolled_back`) undoes it for
a Refusal or a CommandFailed alike, names the step a failed git command was
in (creating the branch, staging the additions, committing), and appends
the list of what was put back; a failure after the commit lands (push, pull
request, #204) rolls nothing back and prints what it always printed. The
no-branch NEXT hint stages with `add -f --` for the same reason.

Remaining: the new tests (ignored addition under three ignore sources;
staging, commit-hook and branch-creation failures restored byte for byte
with `check` unchanged; non-ignored --add unchanged) and the targeted run.

Lane: openreposhape-4 (openRepoShape-4)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…fo/exclude`, a global `core.excludesFile` or a nested `.gitignore` ignores is committed with the root green, and a failed stage, pre-commit hook or branch creation puts back every byte, the index, HEAD and the branch and names the step, so `check` afterwards reports the same `upstream-added` rather than "nothing to do" (#202)

The tests 346bccf left for later. Fifteen, in three groups:

* AN IGNORED ADDITION LANDS. `test_an_ignored_addition_is_committed_
  and_the_root_stays_green`, once per ignore source (info-exclude,
  global-excludes, nested-gitignore): `apply --add ... --branch` exits
  0, the commit holds every file the run wrote with the upstream's
  bytes and the mode `EXECUTABLE` names, the pin IN THE COMMIT records
  those bytes, the root's own validators are green, and an ignored
  stray nobody named stays out of the index, because `force_stage`
  forces the named paths and never `.`.
* A FAILURE BEFORE THE COMMIT PUTS EVERYTHING BACK. `test_a_failure_
  before_the_commit_puts_the_tree_back_and_names_the_step`, eight ids:
  stage (a required clean filter that fails), pre-commit (a hook that
  says no), branch-df and branch-invalid (a branch git cannot create),
  branch-exists (the Refusal raised after the writes), current-branch
  and detached (the other `--branch` postures), staged-before (an index
  entry `git reset` would not put back). Each exits 2 naming the step
  and listing every file restored or removed, the directory removed and
  the git half put back; the tree is compared byte for byte and mode
  for mode, with its directories, HEAD, every branch, the index's
  `--stage` records and `status --ignored`, under
  `PYTHONDONTWRITEBYTECODE=1` and a git config the test writes; and
  `check` then prints exactly what it printed before, `upstream-added`
  rows included. `test_the_same_command_lands_once_the_failure_is_gone`
  is the issue's second run, and `test_a_failure_after_the_commit_
  lands_rolls_nothing_back` holds the `land()` boundary with a push
  that fails after the commit.
* A NON-IGNORED ADDITION IS UNCHANGED. `test_an_addition_nothing_
  ignores_lands_exactly_as_before`: the same commit, another session's
  staged file still staged and out of it, an untracked file still
  untracked. `test_force_stage_forces_exactly_the_named_paths_and_
  nothing_else` pins the one spelling `stage_written` (#175) shares.

No implementation change: 346bccf stands as pushed.

Targeted run over exactly these bytes, git-filter-repo 2.47.0 on PATH:
tests/test_update_shape.py, tests/test_update_shape_add.py, this file,
tests/test_scaffold_ignored_shape_files.py, tests/test_shape_doctor.py
and tests/test_cli_reference.py, 302 passed in 1408.24s.

Mutation check, each mutant applied alone at 346bccf and this file run
alone (unmutated control: 15 passed); F[...] are the failure test's
ids. index-lies (the index left staged and reported unstaged) ->
F[pre-commit, current-branch, detached, staged-before];
index-info-skipped -> F[staged-before]; head-stays -> F[stage,
pre-commit, detached, staged-before] and the same-command test;
branch-kept (reported deleted) -> the same five; readme-not-restored
-> all eight F ids and the same-command test; dir-kept -> all eight F
ids; exec-kept -> all eight and the same-command test; plain-add
(`Rollback.stage` back to `git add --`) -> the three ignored-addition
ids; force-dot (`force_stage` also adds `.`) -> those three, the
nothing-ignores and force_stage tests, and F[pre-commit,
current-branch, detached, staged-before]; land-not-called -> the
after-the-commit test; land-early -> F[pre-commit, current-branch,
detached, staged-before] and the same-command test; step-frozen (every
failed git command called "committing") -> every F id but
branch-exists; refusal-silent -> F[branch-exists];
commandfailed-no-undo (the pre-#202 arm) -> every F id but
branch-exists, and the same-command test; branch-recorded-late ->
F[branch-df, branch-invalid]; hint-plain-add (the no-branch NEXT line
back to `add --`) -> tests/test_update_shape_add.py's
`test_add_copies_the_file_appends_its_row_and_stays_green`, the
assertion 346bccf updated. No mutant survived.

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

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

@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.

@sourcery-ai

sourcery-ai Bot commented Oct 11, 2026

Copy link
Copy Markdown

Reviewer's Guide

This PR fixes apply --add by force-staging only the named additions and extends the rollback ledger through branch creation, staging, and commit creation, restoring filesystem and Git state on pre-commit failures while deliberately preserving commits when later push or PR steps fail. The new regression suite exercises ignore sources, index/branch/HEAD restoration, error reporting, and post-commit behavior.

Sequence diagram for apply add staging and rollback

sequenceDiagram
    participant Operator
    participant Apply as update-shape.py
    participant Ledger as Rollback
    participant Git
    participant FS as Working tree

    Operator->>Apply: cmd_apply(--add, --branch)
    Apply->>FS: write copies, pin, README
    Apply->>Ledger: new_branch(branch)
    Ledger->>Git: git checkout -b branch
    Apply->>Ledger: stage(added)
    Ledger->>Git: git add -f -- named paths
    Apply->>Ledger: commit_on_branch(...)
    Ledger->>Git: git commit -- paths
    alt branch, staging, or commit fails
        Apply->>Ledger: undo()
        Ledger->>Git: restore index entries
        Ledger->>Git: restore HEAD and delete created branch
        Ledger->>FS: restore written bytes and remove additions
        Apply-->>Operator: refusal names failed step and rollback
    else commit succeeds
        Apply->>Ledger: land()
        opt push or pull request fails
            Apply-->>Operator: failure without rollback
        end
    end
Loading

Flow diagram for pre-commit rollback boundary

flowchart LR
    A[apply writes shape files and metadata] --> B[Create branch]
    B --> C[Force-stage named additions]
    C --> D[Commit]
    D --> E[Commit landed: ledger closes]
    B -. failure .-> R[Rollback files, index, HEAD, and branch]
    C -. failure .-> R
    D -. failure .-> R
    E --> F[Push or create pull request]
    F -. failure .-> G[Keep committed result; no rollback]
Loading

File-Level Changes

Change Details Files
Force-staged named additions so operator-controlled ignore rules cannot prevent shape files from reaching the commit.
  • Added a shared helper that runs git add -f -- only for explicit paths.
  • Reused the helper in existing scaffold staging and in apply --add branch commits.
  • Updated the generated follow-up command to use forced staging.
scripts/shape_materialize.py
update-shape.py
tests/test_update_shape_add.py
Expanded pre-commit rollback to restore both repository content and Git state until the commit is created.
  • Tracked created branches, original HEAD state, and exact index records for forced paths.
  • Restored files, directories, index entries, HEAD, and branches after branch, staging, or commit failures.
  • Marked the ledger landed after commit creation so push failures leave the commit intact.
update-shape.py
Made failures identify the failed operation and report rollback results without masking partial restoration problems.
  • Centralized refusal and command-failure handling through the rollback ledger.
  • Added explicit rollback status for fully restored versus partially restored state.
  • Preserved existing behavior for failures after the commit lands.
update-shape.py
Added comprehensive regression coverage for ignored additions and rollback edge cases.
  • Covered repository, global, and nested ignore rules while ensuring unrelated ignored or staged files remain untouched.
  • Injected staging, hook, branch, detached-HEAD, existing-index, and post-commit failures and compared tree, modes, directories, HEAD, branches, index, and status.
  • Added unit coverage for exact-path forced staging and retained non-ignored addition behavior.
tests/test_update_shape_add_ignored.py
tests/test_update_shape_add.py

Assessment against linked issues

Issue Objective Addressed Explanation
#202 Force-stage files supplied through apply --add so additions are committed even when ignored by .git/info/exclude, a global excludes file, or project .gitignore rules, without staging unrelated operator changes. ✅
#202 Roll back all changes made by apply when branch creation, addition staging, or commit fails before the commit lands, including copied files, the pin, README changes, index entries, HEAD, and any branch created by the command. ✅
#202 Report the specific failed step and rollback results, preserve post-commit state on failures such as push errors, and ensure a failed pre-commit run does not leave an uncommitted re-pin that causes later apply or check commands to report success incorrectly. ✅

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

…iterally, so an added `docs/guide[1].md` no longer forces an ignored `docs/guide1.md` into the commit, a partial rollback's hand fixes are quoted and never `git reset` a path that had its own staged entry, the validators and README refusals claim the rollback once, and every failure test now holds an operator's staged work beside a refused branch deletion and a tag named `main` (#202)

The review of c022b64 (MERGEABLE WITH NITS) left one SHOULD and four
nits; this commit answers all five. Nothing here changes what a run that
succeeds does. No refusal's own text changes except the sentence NIT 4
removes; the one printed line that is rewritten is the index's hand fix
NIT 5 replaces.

SHOULD 1, a test gap. No failure case held an operator's unrelated staged
change, so a rollback that put the index back with `git reset -q` (all of
it, to HEAD) and then rewrote the additions' old records passed every
test while turning `A  NOTES.md` / `MM README.md` into `?? NOTES.md` /
` M README.md` and still printing "exactly as it was". Every `FAILURES`
case now stages a NEW file and an EDIT to a tracked, unpinned file (with
a further unstaged edit on top) before the failing `apply`; `snapshot`
already compares the whole index and `status`, and the test first asserts
both are in the status, so the check cannot quietly stop running.

NIT 2. `force_stage` ran `git add -f -- <paths>`, and a pathspec is a glob
to git: a copy-list name holding `[`/`]` also matched an operator's
ignored neighbour, `-f` forced it in, and the commit took it. It is now
`git --literal-pathspecs add -f -- ...`, which `stage_written` (the
scaffold's legs and root, `family.py init`, adopt's seeded leg) inherits;
for a name with no glob character the two spellings are the same command.
`Rollback._index_entries` reads the same names literally, so the record it
keeps is of exactly the paths it forced. Pinned by a new commit C in the
fixture adding `docs/guide[1].md` beside an ignored `docs/guide1.md`: only
the named file is committed. `[` and `]` are legal in Windows names, so it
runs everywhere. The scaffold test that pins `stage_written`'s argv
now expects the literal spelling. `force_stage`'s docstring no longer calls itself the one
spelling of the forced add: `adopt-project.py` keeps two of its own.

NIT 3. Two defensive branches had no test. (a) A `reference-transaction`
hook refusing the branch deletion: the refusal must say `ROLLED BACK IN
PART`, list the branch under `NOT put back` with the command that deletes
it, leave everything else (the operator's staged work included) as it
was, and that printed command, run once the hook is gone, must make the
tree whole. (b) A `FAILURES` case with a TAG named `main`: the rollback
returns HEAD to `refs/heads/main`, never detached at the tag, which is
what keeping the start branch under `refs/heads/` (not `--short`'s
`heads/main`) is for.

NIT 4. `_symbolic_head` and `_head_is`'s `rev-parse HEAD` re-implemented
this file's `_git_answer`; both ask through it now. `update-validators-red`
and `update-readme-unwritable` said "Every byte this command wrote has been
rolled back; the tree is exactly as it was." in their detail and then got
`_rolled_back`'s `ROLLED BACK: ... exactly as it was:` block under it; the
detail sentence is gone, and the existing tests of both refusals assert
the claim appears once. No test asserted the doubled sentence.

NIT 5. When the index could not be put back, the hand fix was `git -C
<root> reset -q -- <names>`: unquoted, so `docs/shape guide.md` pasted as
two paths, and wrong for a path that HAD a staged entry, because `reset`
writes HEAD's entry, the hazard `Rollback.stage` names. `_index_by_hand`
now prints one fix per path from the records kept before the run:
`update-index --force-remove` for a path with none, `--add --cacheinfo
<mode>,<sha>,<path>` for one staged entry, and its stages through
`--index-info` for an unmerged path. One helper, `_by_hand`, quotes every
argument of all three hand fixes (index, HEAD, branch) with `shlex.join`;
an ordinary root prints what it always printed. No hook reaches this
message (the index is no ref), so the test drives `Rollback.stage` and
`_undo_index` in-process with `_git_quietly` answering as a git that could
not be started, in a root whose path holds a space, and runs each printed
fix through `sh` to show it puts back exactly the records the index held.

MUTANTS (a scratch worktree, the ignored-add test file unless named).
The review's three survivors now fail:
  * `unrelated-reset`: 7 failed, `FAILURES` [stage, pre-commit,
    current-branch, detached, staged-before, tag-main] and the partial
    rollback test, each on "index is not what it was";
  * `partial-said-whole`: 1 failed, the partial rollback test;
  * `short-start`: 1 failed, `[tag-main]` (HEAD left detached, "not back
    on heads/main").
New ones, one per fix, each failing:
  * `glob-literal-off` (c022b64's glob spelling): the glob test and the
    argv test;
  * `by-hand-unquoted`, `draft-reset` (`git reset` offered for a path that
    had a staged entry) and `callsite-reset` (c022b64's line back at the
    undo's call site): the hand-fix test, each;
  * `doubled-claim` (the detail sentence put back):
    `test_a_red_validator_rolls_the_added_file_back_out`.

RUN, over these bytes: `pytest tests/test_update_shape.py
tests/test_update_shape_add.py tests/test_update_shape_add_ignored.py
tests/test_scaffold_ignored_shape_files.py tests/test_scaffold_e2e.py
tests/test_shape_doctor.py tests/test_cli_reference.py -q` with
git-filter-repo 2.47.0 on PATH: 350 passed in 427.15s, 0 failed,
0 skipped, exit 0. Separately, adopt's seeded-leg test, which reaches
`force_stage` through `git_init_commit`, passed.

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

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-4, 2026-10-11T19:41:22Z) Review round 1 is in: 396b946, one commit on c022b64, answering the adversarial review's SHOULD and nits 2-5.

  • SHOULD 1: every injected-failure case now stages an unrelated new file and an edit to a tracked file of the operator's (with an unstaged edit on top) before the failing apply. The review's surviving mutant, which puts the index back by resetting all of it, now fails 7 cases.
  • NIT 2: force_stage runs git --literal-pathspecs add -f --, so a copy-list name holding a glob character cannot force in an ignored file it matches. A test commits docs/guide[1].md beside an ignored docs/guide1.md and checks only the named file lands. Rollback reads its index entries literally too.
  • NIT 3: two new tests. A reference-transaction hook that refuses the branch deletion gives ROLLED BACK IN PART, with one left line naming the branch and a printed fix that completes it. A tag named main no longer leaves HEAD detached after a failure.
  • NIT 4: _symbolic_head and _head_is ask through _git_answer. update-validators-red and update-readme-unwritable claim the rollback once, not twice.
  • NIT 5: the by-hand fix for an index that could not be put back is one pasteable command per path, quoted with shlex.join, and never git reset. It uses update-index --force-remove for a path with no prior entry, --add --cacheinfo for one that had a staged entry, and --index-info for an unmerged one. A test runs each printed fix through sh and checks the index is exactly as it was.

The seven-file targeted set (update-shape, scaffold ignored-files and e2e, doctor, cli) gave 350 passed. Since force_stage is shared with the scaffold and adopt's seeded leg, a focused re-review of this commit is running. Related, found by the first review and filed separately: #239 (a dangling --add symlink written through and then unlinked on rollback; pre-existing on main).

@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.

…rns HEAD to a non-ASCII start branch on Windows too; the branch HEAD is still on is never deleted after a switch back that failed; and the NEXT hint quotes every path and adds them literally, so a name holding `[` stages itself and no ignored neighbour (#202)

Fix round 2 for PR #233, answering the four nits of the re-review at 396b946.

NIT 1 (Windows only, a regression of fix round 1): `_symbolic_head` now
reads HEAD through `_git_answer`, which decoded git's output with the
locale's codec. Under the ANSI code page (Windows without PYTHONUTF8=1) a
start branch `feature/café` came back as `feature/café`, the switch back
named a branch that does not exist, and a name holding a byte that code page
leaves undefined (`Ýmir`) raised out of the very answer that may not raise.
The two-line change that fixes it, `encoding="utf-8", errors="replace"` on
`_git_answer`'s `subprocess.run`, is PR #230's own (#217,
fix/decode-git-output-utf8 @ fbdf8db), copied byte for byte and with none of
its other hunks, so whichever of the two lands second merges it as a no-op:
`git merge-tree --write-tree origin/fix/decode-git-output-utf8` over this
head is clean. Pinned twice, neither depending on the platform's code page:
the rollback's own undo run with every text-mode call that names no codec
forced to cp1252 (both names, on every OS), and `_symbolic_head` in a child
started under `LC_ALL=C PYTHONUTF8=0` (POSIX).

NIT 2 (wording): `_by_hand`'s docstring said an ordinary root prints what it
always printed. That holds on POSIX only: every Windows root holds a
backslash, which `shlex` quotes. It now says so, and why that is left alone.
The two smaller slips: `_index_by_hand` said `git reset` "would replace with
HEAD's" an entry that may BE HEAD's, and `force_stage`'s first line left out
adopt's seeded leg, which reaches it through `git_init_commit`.

NIT 3: the hint `add -f -- {added}` was unquoted, and `-f` is what made that
dangerous: pasted into a shell, an added `docs/guide[1].md` is a pattern the
shell expands to an ignored `docs/guide1.md`, which `-f` then forces in.
Quoting alone is not enough, because git reads a pathspec as a glob too:
`git add -f -- 'docs/guide[1].md'` still stages the neighbour, as the first
round's `force_stage` already found. So each added path is quoted with
`shlex.join`, the helper `_by_hand` uses, and the add is
`--literal-pathspecs`. The commit's own path list in the same line is quoted
the same way, so the whole line pastes whole. An ordinary name prints exactly
as it did. Tested by taking the line apart as a shell would on every OS, and
by pasting it into `sh` from the root beside an ignored neighbour.

NIT 4: when the switch back failed, `_undo_branch` still ran `update-ref -d`
on the branch HEAD was attached to, which takes no index lock. HEAD then
named a ref with no commit and `git status` said "No commits yet" over a
repository with a history. The deletion is skipped while HEAD is still on the
branch, and the partial-rollback report lists the branch as left, with the
switch and then the deletion (`git branch -D` refuses the checked-out
branch). Tested end to end through `apply`: a `post-checkout` hook takes
`.git/index.lock` once the branch exists, so the staging fails and the switch
back with it; once the lock is cleared the printed line makes the tree whole.

Each new test was shown to fail against its regression, by mutants in a
scratch worktree: `_git_answer` without its codec (4 failed), the hint with
its added paths unquoted (2), without `--literal-pathspecs` (3), with the
commit list unquoted (2), and the deletion not guarded (1).

Not touched, and said so: an intent-to-add entry at an `--add` path (NIT 5 of
the review), `git commit -- <paths>` reading each name as a glob in
`commit_on_branch`, and the unquoted root in the NEXT lines, all of which
predate this pull request.

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

Copy link
Copy Markdown

@brettheap

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-4, 2026-10-11T20:26:17Z) A focused re-review of 396b946 found it MERGEABLE WITH NITS, with no MUST or SHOULD and CI green on all three OSes. 5fc0f01 answers its nits 1-4.

The six-file targeted set gave 176 passed. Five mutants of the new lines were all caught. On the merge with #230, its decoding tests plus the new ones gave 10 passed, including #230's check that no shipped text-mode call decodes with the locale codec. Not changed here: an intent-to-add entry at an --add path (narrow; noted in the re-review) and commit_on_branch's glob reading of its paths, which is pre-existing.

@brettheap
brettheap merged commit 150c41d into main Oct 11, 2026
5 checks passed
@brettheap
brettheap deleted the fix/update-shape-add-rollback branch October 11, 2026 21:45
brettheap added a commit that referenced this pull request Oct 11, 2026
…) into fix/plan-report-stdout-encoding (#214)

One conflict, resolved by keeping both sides: the `from shape_materialize
import (...)` list in update-shape.py. This branch added
`escape_what_stdout_cannot_encode` and main added `force_stage` (#202) in the
same place. Both names are now in the list, in its existing order
(commit_trailers, escape_what_stdout_cannot_encode, force_stage,
lane_trailer_argument, ...), wrapped to 79 columns like its neighbours, so the
list's tail lines re-flow with it. No other file conflicted, and nothing else
was changed.

Lane: openreposhape-5 (openRepoShape-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap added a commit that referenced this pull request Oct 11, 2026
…reate-failure

origin/main 39ac59c carries #233 (#202, 150c41d), whose ledger rolls back a
branch, a stage or a commit that fails and closes at the commit. Three hunks
in update-shape.py, both sides kept:
- EXIT CODES, apply 2: one sentence covering the rolled-back branch, stage
  or commit and the push or pull request kept after the commit (#204).
- `_land`'s docstring: #202's ledger sentence, then #204's paragraph.
- #202's `_rolled_back`, then #204's `LandingIncomplete` ...
  `pull_request_target`; only #202's old `open_pull_request` header goes.
  `_rolled_back`'s sentence saying a post-commit push or pull request "is
  #204's, and prints the line it always printed" now says it never reaches
  that arm: `LandingIncomplete` is neither of its two classes.

tests/test_update_shape_add_ignored.py: the `branch-invalid` FAILURES case
(`shape/update..ignored` failing `git checkout -b` after the writes) is now
refused by `checked_branch` in `main` before anything is written, so it
becomes its own test asserting `update-branch-invalid`, empty stdout and an
untouched tree with the operator's staged work; `branch-df` still covers a
branch-creation failure after the writes.

Lane: openreposhape-3 (openRepoShape-3)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap added a commit that referenced this pull request Oct 11, 2026
…s rolled back as #202 rolls it back, and the same command then lands and opens the pull request on one commit

After merging main (#233, #202), the commit is where rolling back stops
from both sides: #202's ledger puts back a branch, a stage or a commit
that fails and closes at the commit, and #204 keeps the commit when a push
or `gh pr create` fails after it. The post-commit half was already pinned
on the merged tree by the landing tests (`the_commit_stands`: clean tree,
HEAD on the branch with the re-pin) and by #202's
`test_a_failure_after_the_commit_lands_rolls_nothing_back`. Nothing drove
the pre-commit half with the landing flags passed. Two tests do now:
- `test_a_failure_before_the_commit_under_push_and_pr_is_rolled_back`
  (every platform; `gh` is never reached): a pre-commit hook refuses the
  commit under `--branch --push --pr`. Exit 2, `REFUSED committing the
  re-synced shape on <branch>`, #202's ROLLED BACK block (pin, manifest
  and copy restored, HEAD switched back, the branch deleted), and nothing
  of the landing: no push, no `gh` call, no NEXT line, no re-run promised,
  origin without the branch, the tree and refs as they were.
- `test_the_same_command_after_a_rolled_back_commit_lands_and_opens`
  (shell `gh` stub, WINDOWS_SKIP): with the hook gone the same command has
  work to write, so it is the first run again, not the `--pr` re-run: one
  commit, pushed, one `gh pr create` and no `gh pr list`.

Lane: openreposhape-3 (openRepoShape-3)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap added a commit that referenced this pull request Oct 11, 2026
…ng-paths

main moved to 39ac59c: #197 (b538015, PR #234) moved `_overlap` and
`_first_and_more` into scripts/shape_materialize.py as `path_overlap` and
`first_and_more`, and adopt-project.py imports them back under the old
names; 150c41d and 39ac59c (PRs #233 and #235) touch update-shape.py and
scaffold-project.py. One conflict, in adopt-project.py: this branch had
edited `_first_and_more` to `_spelled` its path, and main removed the
function.

Resolved by taking main's side of that one hunk: `_overlap` and
`_first_and_more` are deleted from adopt-project.py, and main's import of
the shared definitions stays as it is. No line is written by hand in this
merge. It leaves `_kept_collisions` naming a source path through
`first_and_more`, unspelled, so the spelling case of
tests/test_adopt_finding_spelling.py fails on this commit alone; the next
commit gives `first_and_more` an optional `spell` and passes `_spelled`.

Lane: openreposhape-3 (openRepoShape-3)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
brettheap added a commit that referenced this pull request Oct 11, 2026
…) into fix/adopt-record-submodule-placement

Brings in b538015 (#197), 150c41d (#202) and 39ac59c (#229) on top of
4835fb4. No rebase. They landed while this branch's targeted run was
going, and the files they change overlap this branch's: adopt-project.py
and scripts/shape_materialize.py.

No conflicts. Checked:

- adopt-project.py: main moves `_overlap` and `_first_and_more` into
  shape_materialize.py as `path_overlap` and `first_and_more`; this
  branch's `submodule_record` and its record append are untouched.
- scripts/shape_materialize.py: main adds `shape_path_collisions`,
  `force_stage` and `neutral_pin_path`, and `assembly_root_paths` takes
  `neutral_pins`. This branch's `_spelling_reason` and
  `LEG_SEGMENT_REASONS`, which ask `repo_shape.path_spelling_fault`, are
  untouched, and no line main added names either, nor
  `leg_path_problem` (`git diff 4835fb4 origin/main`).
- The branch's diff against main is still #189 alone: 12 files, +1670
  -156, the same as against 4835fb4.

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 12, 2026
…e-notice

It merged with no conflict. Main brings six commits since 54aa69a:

- e7d1720, update-shape.py (#208 via #224);
- 404760f, scripts/repo_shape.py's double-quoted flow items (#213 via
  #244), with tests/test_yaml_flow_quotes.py;
- 4835fb4, adopt-project.py's plan writer escapes (#199 via #247), with
  tests/test_adopt_plan_escapes.py;
- b538015, scaffold-project.py's leg paths against the shape's own
  paths (#197 via #234);
- 150c41d, update-shape.py's `apply --add` (#202 via #233);
- 39ac59c, scaffold-project.py's leg mount `--force` (#229 via #235).

Two of them touch adopt-project.py, and no hunk of either meets this
branch's:

- 4835fb4 changes the plan writer (`y`, `_PLAIN_RE`), adds `_shown` to
  the plan and check reports, and writes the `--paths-from-file` listing
  in `_extract_leg` through `write_paths_listing`. This branch changes
  other lines of `_extract_leg`: it reads the tree digest before the
  push and pushes through `_push_the_leg`. The signature is the same on
  both sides, so main's new direct `_extract_leg` test calls it as
  written.
- b538015 moves `_overlap`, `_first_and_more`, `MOVE_THE_LEG` and
  `CASE_ASIDE` into scripts/shape_materialize.py, and imports the first
  three back under the same names. This branch adds no use of any of
  them. A `symtable` check finds no name that the merged adopt-project.py,
  scaffold-project.py or scripts/shape_materialize.py reads and nothing
  binds.

`git diff -U0` of this merge against 39ac59c equals the branch's against
54aa69a, apart from the hunk headers. On the merged tree,
tests/test_adopt_plan_escapes.py, tests/test_yaml_flow_quotes.py,
tests/test_scaffold_leg_path_collisions.py and
tests/test_adopt_leg_path_collisions.py give 275 passed.

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 12, 2026
…`git --literal-pathspecs add -f`, so an operator whose own excludes name `members`, `members/`, `*` or the member gets one commit instead of exit 2, `bump` moves a pin under them too, and each refusal `--force` turns off is made before the member is read (#242) (#308)

`add` ran `git submodule add` with no `--force` and `_commit_one` a plain
`git add`, and both read a global `core.excludesFile`. `members/` is a fixed
path, so an operator whose excludes named `members`, `members/`, `*` or the
member's name had every add of every family refused: git's dry run refused
the first four before the clone, and a directory-only rule on the member's
name (`Atlas/`) refused it only at git's last `git add`, after the clone.
`bump` died the same way under `members` or `*`: it stages the gitlink
twice, at the `git add` #219 put before its nothing-to-record check and in
`_commit_one`, and both read the excludes. This is #174 (adopt, fixed
by #200) and #229 (scaffold) on the family side.

Every reader is forced now. The mount passes `--force`, and every stage
`add`, `bump` and `remove` make is `shape_materialize.force_stage` (#233),
`git --literal-pathspecs add -f -- <paths>`: `-f` reaches only the named
paths, each taken literally, so no ignored neighbour a glob character would
match is staged with them (#202). One definition: `_commit_one` and `bump`'s
re-stage both call it.

`--force` turns off more than the ignore check, so each other thing it
changes is refused before the member is read, the holder left exactly as it
was, and a comment at the mount names each guard:
- a `.git/modules/<path>` left by a removed member, which `--force`
  reactivates: `member-git-dir-cached` (#236), at the store `git rev-parse
  --git-path` names, a linked worktree's included;
- an index entry at the path or under it with nothing on disk, which
  `--force` accepts and replaces with the member's gitlink:
  `member-path-taken` now asks the index too, and names the
  `git rm -r --cached` that clears it; on disk it asks `lexists`, so a
  dangling link is refused as well;
- git's own dry run, which `--force` skips (a symlinked `members` had the
  member cloned through the link, outside the holder; a `members` that is a
  submodule had it cloned into that submodule's tree): `member-path-unaddable`
  makes the same dry run first, with `-f`, so an ignore rule passes it and
  nothing else does; the refusal quotes git's line, and its remediation
  presumes neither a link nor a submodule (a held `index.lock` fails it
  too);
- a name another submodule already has, which git 2.51 and later rename the
  mount past and git 2.43 answers by rewriting the other section:
  `member-name-taken`, reading `.gitmodules` where git reads it (the working
  tree, else the index's copy, else HEAD's).
The PRIVATE/access sentence is printed only for the probe's failed clone of
the member, which no ignore rule reaches.

tests/test_family_add_excludes.py: under `members`, `members/`, `*`, `IRQS`,
`IRQS/` and none, `add` is ONE commit of `.gitmodules`, `family.yaml` and a
160000 gitlink at the member's tip, read with `git ls-tree`, that validates
under the same excludes and clones recursively for an operator with none;
`bump` under `members` moves the gitlink in one commit and `remove` is the
control; each guard above is refused before the member is read with the
holder unchanged. Mutations: without `--force` at the mount every rule's row
fails; without `-f` in `_commit_one` the `members`, `members/` and `*` rows
and the bump test fail; with `bump`'s re-stage a plain `git add` the bump
test fails; dropping the index check, the dry run, the name check, its
index/HEAD fallback or `lexists` fails exactly that guard's tests.

Closes #242

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

Labels

None yet

Projects

None yet

1 participant