From 1f8d57331ab65d7d78dbd21ad3c2988101bd9a32 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Wed, 9 Sep 2026 23:22:53 -0400 Subject: [PATCH 1/2] update-shape.py: --accept-local re-pins a `both` row from the merged file, as AGENTS.md promised, and --branch may name the branch you are on (#84) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `apply` refused every `both` row before it ever consulted `--accept-local`, so the exit AGENTS.md already documented — merge by hand, commit, re-run with `--accept-local ` on the file you merged — could never actually be taken. Found running S4 of #77 on InkRouter/IRRS and IRSS (their #13s): both adopted an in-place Makefile that appends a CONTRACTS_DIR block and keeps its pin row, so `classify()`'s "not a verbatim copy at the pinned commit" branch reads it as `both` the moment upstream touches the template, forever, regardless of any further local edit. `cmd_apply`'s conflict refusal now excludes a `both` row whose path `--accept-local` names: upstream bytes are never copied over it, and `classify()` precomputes its digest from the working file the same way it already does for `locally-modified`, so the row is recomputed from whatever is on disk — the person's word, not a check that a merge actually happened. An unaccepted `both` row still refuses with the existing wording; `check`'s per-row detail for `both` gains one sentence naming `--accept-local ` as the exit (kept out of `row.detail` itself, so the refusal's own wording does not change). `commit_on_branch` no longer assumes `--branch ` names a branch that does not exist yet: `` naming the root's tracking branch is refused outright, current or not; naming the CURRENT branch (so, by that rule, not the tracking branch) commits onto it directly — exactly the checkout the documented `both` exit leaves behind once the hand merge is committed; an existing branch that is not current is refused by name (`update-branch-exists`); anything else is created fresh, as before. Tests in test_update_shape.py: the existing CHANGED-file `both` case now also asserts `--accept-local` succeeds and re-pins from the local, unmerged bytes (`test_accept_local_on_a_both_row_repins_from_the_local_bytes`). A new `adopted_root` fixture mirrors an in-place adoption's Makefile append (`ADOPT_MAKEFILE_BLOCK`, hand-patching the pin row's digest the way `_materialize` itself computes it) in its own upstream clone, so its second commit moving the Makefile TEMPLATE (and, alongside it, scripts/validate-manifest.py) cannot move any other test's default target. Four tests cover `check` naming the exit, the unaccepted refusal, the hand-merge-then-`--branch`-onto-that-branch exit end to end (re-pin, the other changed file copied, green validate-pins.py/validate-manifest.py), and an unmerged `--accept-local` still succeeding. Three more cover `--branch`'s postures: current non-tracking branch, existing non-current branch, and the tracking-branch refusal (checked on `main`, which is both this root's tracking AND current branch, so it proves the tracking check runs first). AGENTS.md point 4 now says the refusal has this exception and that the hand-merge commit goes on the branch `apply` is then pointed at with `--branch`; its cap moves 342 -> 344 with a dated entry in test_repo_hygiene.py's own style. 82 passed (test_update_shape.py, test_update_shape_add.py, test_repo_hygiene.py); 738 passed, 1 skipped for the full suite. Re-ran the fix against a scratch clone of InkRouter/IRRS on branch shape/update-3abeb8a4a27e, where the hand-merged Makefile from #13 is already committed: `apply --accept-local Makefile --branch shape/update-3abeb8a4a27e` re-pinned from that merged file, copied AGENTS-shape.md, committed onto the same branch, and `make validate` is green. Lane: xfactory-2 (xFactory-2) Co-Authored-By: Claude Fable 5.1 --- AGENTS.md | 6 +- tests/test_repo_hygiene.py | 15 +- tests/test_update_shape.py | 311 ++++++++++++++++++++++++++++++++++++- update-shape.py | 124 ++++++++++++++- 4 files changed, 438 insertions(+), 18 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e52a3076..14742155 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -196,8 +196,10 @@ git -C push -u origin shape/update- # then open a pull request `validate-pins.py` reports today invisible. Whether the project keeps its edit is the human's decision; the other two exits are to revert it or to carry it upstream. -4. **`both` is a merge, and the tool refuses it.** Merge by hand, commit, then - re-run with `--accept-local ` on the file you merged. +4. **`both` is a merge, and the tool refuses it unless you say `--accept-local + ` too.** Merge by hand, commit, then re-run with `--accept-local + ` on the file you merged — that commit goes on the branch `apply` is + then pointed at with `--branch`. 5. **`upstream-added` is a file the STANDARD gained since the pin, and `--add` is per file.** Report the paths `check` names; pass `--add ` only for a file the human has said to take, and never to quiet the report. Without diff --git a/tests/test_repo_hygiene.py b/tests/test_repo_hygiene.py index 574d44f0..7ca6453c 100644 --- a/tests/test_repo_hygiene.py +++ b/tests/test_repo_hygiene.py @@ -303,9 +303,20 @@ def test_agents_md_is_short_enough_to_be_read(): before anyone is told the estate is parked — a member SKIPPED for want of a working clone beside the holder was never asked at all, which is the one outcome an assistant reads as success. The mechanics themselves are - the Speckit git extension's and are deliberately NOT described here.""" + the Speckit git extension's and are deliberately NOT described here. + + 342 -> 344 on 2026-09-10, for point 4's exception (#84). `apply` had + refused every `both` row before it ever consulted `--accept-local`, so + the exit this file already documented — merge by hand, commit, re-run + with `--accept-local ` — could never actually be taken; the tool + was fixed to match the file rather than the other way around. The one + sentence added says where that commit goes: on the branch `apply` is + then pointed at with `--branch`, because that is the checkout the + documented exit leaves behind, and an assistant that recreated the + branch instead of committing onto the one already there would be + re-running the tool at the wrong git state.""" lines = (REPO / "AGENTS.md").read_text().splitlines() - assert len(lines) <= 342, f"AGENTS.md is {len(lines)} lines; the cap is 342" + assert len(lines) <= 344, f"AGENTS.md is {len(lines)} lines; the cap is 344" def test_claude_md_points_at_agents_md(): diff --git a/tests/test_update_shape.py b/tests/test_update_shape.py index a22cb6a5..42e28c96 100644 --- a/tests/test_update_shape.py +++ b/tests/test_update_shape.py @@ -27,6 +27,10 @@ sys.path.insert(0, str(REPO / "scripts")) from repo_shape import Refusal, file_sha256, load_yaml, tree_digest # noqa: E402 +#: The exact block `adopt-project.py` appends to an adopted Makefile, +#: imported rather than retyped — see `adopted_root` below, whose whole point +#: is that it cannot drift from what a real adoption writes. +from shape_materialize import ADOPT_MAKEFILE_BLOCK # noqa: E402 UPDATE = REPO / "update-shape.py" PROJECT = "Atlas" @@ -57,6 +61,13 @@ def update_shape(): #: A second copied file, edited in the PROJECT rather than upstream. LOCAL = "scripts/bootstrap.py" +#: The Makefile: a COPIED_VERBATIM file every scaffolded project already +#: carries a pin row for, and the one an in-place adoption appends +#: `ADOPT_MAKEFILE_BLOCK` to. `adopted_root`, further down, mirrors that +#: append onto an otherwise-ordinary scaffolded project. +MAKEFILE = "Makefile" +MAKEFILE_SOURCE = f"templates/assembly-root/{MAKEFILE}" + @pytest.fixture(scope="module") def upstream_and_project(tmp_path_factory) -> dict: @@ -227,6 +238,51 @@ def test_apply_on_a_branch_commits_only_what_it_wrote(root, cwd=root).stdout +# --- --branch: the three postures (#84) ------------------------------------- + +def test_branch_onto_the_current_non_tracking_branch_commits_there( + root, upstream_and_project): + """The shape a `both` exit leaves a checkout in: a human already checked + out a branch and committed a hand merge there, so `apply --branch ` must land ON it rather than trying `git checkout -b` a second + time and finding the branch already exists.""" + git("checkout", "-q", "-b", "shape/already-here", cwd=root) + result = apply(root, upstream_and_project, "--branch", "shape/already-here") + assert result.returncode == 0, result.stdout + result.stderr + assert git("rev-parse", "--abbrev-ref", "HEAD", + cwd=root).stdout.strip() == "shape/already-here" + assert "committed on shape/already-here" in result.stdout + + +def test_branch_naming_an_existing_non_current_branch_refuses_by_name( + root, upstream_and_project): + """Committing onto a branch a human already put something on is a + decision for them, not an assumption this tool makes on their behalf.""" + git("branch", "shape/someone-elses", cwd=root) + result = apply(root, upstream_and_project, + "--branch", "shape/someone-elses") + assert result.returncode == 2, result.stdout + assert "update-branch-exists" in result.stderr + assert "shape/someone-elses" in result.stderr + assert git("rev-parse", "--abbrev-ref", "HEAD", + cwd=root).stdout.strip() == "main", ( + "a refused apply must not leave the checkout on a different branch") + assert load_yaml(root / "contracts" / "shape-pin.yaml")["commit"].lower() \ + == upstream_and_project["a"], "a refused apply writes nothing at all" + + +def test_branch_naming_the_tracking_branch_refuses(root, upstream_and_project): + """`main` is both this root's tracking branch AND its current branch — + proof the tracking-branch refusal is checked BEFORE the current-branch + fast path, not after it.""" + result = apply(root, upstream_and_project, "--branch", "main") + assert result.returncode == 2, result.stdout + assert "update-branch-tracking" in result.stderr + assert "main" in result.stderr + assert load_yaml(root / "contracts" / "shape-pin.yaml")["commit"].lower() \ + == upstream_and_project["a"] + + def test_apply_without_yes_refuses_where_nobody_can_be_asked( root, upstream_and_project): result = run_script(UPDATE, "apply", "--root", str(root), "--upstream", @@ -349,21 +405,44 @@ def test_accept_local_on_a_path_with_no_row_refuses(root, def test_changed_on_both_sides_refuses_and_names_the_file( root, upstream_and_project): - """Two edits to one file is a merge, and a merge is a human's judgement.""" + """Two edits to one file is a merge, and a merge is a human's judgement — + UNLESS the human says `--accept-local`, which is + `test_accept_local_on_a_both_row_repins_from_the_local_bytes`'s exit, not + this test's: this one is the refusal that fires without it, and `check`'s + detail for `both` now names that exit by path.""" edit_locally(root, CHANGED) checked = check(root, upstream_and_project) assert checked.returncode == 1 assert verdicts(checked.stdout)[CHANGED] == "both" + assert f"--accept-local {CHANGED}" in checked.stdout - for extra in ((), ("--accept-local", CHANGED)): - result = apply(root, upstream_and_project, *extra) - assert result.returncode == 2, result.stdout - assert "update-conflict" in result.stderr - assert CHANGED in result.stderr + result = apply(root, upstream_and_project) + assert result.returncode == 2, result.stdout + assert "update-conflict" in result.stderr + assert CHANGED in result.stderr assert load_yaml(root / "contracts" / "shape-pin.yaml")["commit"].lower() \ == upstream_and_project["a"] +def test_accept_local_on_a_both_row_repins_from_the_local_bytes( + root, upstream_and_project): + """`--accept-local` on a `both` row is the person's word, exactly as it + already is for `locally-modified` (`test_accept_local_on_a_path_with_no_ + row_refuses`'s sibling, not its opposite): the row is recomputed from + whatever is on disk, whether or not those bytes actually incorporate the + upstream's side of the conflict. So this ends up recording the LOCAL + edit alone, not a merge of it with upstream's change to the same file.""" + edit_locally(root, CHANGED) + edited = (root / CHANGED).read_bytes() + result = apply(root, upstream_and_project, "--accept-local", CHANGED) + assert result.returncode == 0, result.stdout + result.stderr + assert (root / CHANGED).read_bytes() == edited, ( + "the local bytes are kept; upstream's side of the conflict is never " + "copied over an accepted row") + assert pin_rows(root)[CHANGED] == file_sha256(root / CHANGED) + validators_are_green(root) + + # --- the in-place adoption case -------------------------------------------- def test_a_file_with_no_pin_row_is_not_a_shape_file(root, @@ -416,6 +495,226 @@ def test_the_collision_dir_agrees_with_adopt_project(): assert len({tuple(lines) for lines in spellings.values()}) == 1, spellings +# --- both: an in-place adoption's Makefile, forever (#84) ------------------- +# +# THE INKROUTER/IRRS BUG, REPRODUCED. The in-place adoption above merges the +# collision away and drops its row, so the project's Makefile carries none and +# is invisible to this tool. This is the OTHER outcome, and the one +# `adopt-project.py` actually produces when there is no collision: the +# shape's Makefile lands straight at `Makefile`, WITH the adopted project's +# `CONTRACTS_DIR` block appended, and KEEPS its pin row. `classify()`'s +# `verbatim_at_pin` check then reads that row as `both` the moment upstream +# touches the template, forever, regardless of any further local edit — which +# is why `--accept-local` on it could never simply mean "keep my edit": most +# of the time there IS no edit, only the append the adoption itself made. + + +def set_pin_row_sha256(pin_path, path: str, sha256: str) -> None: + """Hand-patch one `files:` row's digest to what a real adoption's own + materializer would have written: over the file AFTER `ADOPT_MAKEFILE_ + BLOCK` was appended (`shape_materialize.py`'s `_materialize`, ~:621-627, + digests `result.shape_files` only once every append has already landed + on disk). That is what makes `locally_modified` false and `verbatim_at_ + pin` false AT ONCE — the second `both` branch, "not a verbatim copy ... + appends to it", rather than the first — which is the actual shape #84 + was filed about. + """ + lines = pin_path.read_text(encoding="utf-8").splitlines() + out = [] + at_path = False + patched = False + for line in lines: + if at_path and line.strip().startswith("sha256:"): + out.append(f' sha256: "{sha256}"') + at_path = False + patched = True + continue + out.append(line) + at_path = line.strip() == f"- path: {path}" + assert patched, f"{path} has no row in {pin_path}" + pin_path.write_text("\n".join(out) + "\n", encoding="utf-8") + + +@pytest.fixture(scope="module") +def adopted_upstream_and_project(tmp_path_factory) -> dict: + """A SEPARATE clone/project pair, so an upstream Makefile-template change + cannot move any OTHER test's target commit out from under it. + + `check()`'s default `--at` is the upstream's HEAD, and `upstream_and_ + project` above is shared by every other test in this module; a further + commit on ITS clone would move all of them onto a target none of them + expect. This fixture's own commit B' changes the Makefile TEMPLATE — the + one thing no other test here may see move — so it gets its own clone, + built the same way `upstream_and_project` is. + """ + base = tmp_path_factory.mktemp("update-shape-adopted") + upstream = base / "openRepoShape" + proc = subprocess.run(["git", "clone", "-q", str(REPO), str(upstream)], + capture_output=True, text=True, check=False) + assert proc.returncode == 0, proc.stderr + commit_a = git("rev-parse", "HEAD", cwd=upstream).stdout.strip() + + result = run_script( + upstream / "scaffold-project.py", "--org", ORG, "--project", PROJECT, + "--elected-by", "Test Human", "--elected-on", "2026-09-02", + "--local-remote-dir", str(base / "remotes"), + "--work-dir", str(base / "work")) + assert result.returncode == 0, result.stderr + result.stdout + + clone = base / "clone" / PROJECT + proc = subprocess.run( + ["git", *FILE_PROTOCOL, "clone", "-q", "--recurse-submodules", + str(base / "remotes" / f"{PROJECT}.git"), str(clone)], + capture_output=True, text=True, check=False) + assert proc.returncode == 0, proc.stderr + + # ---- mirror an in-place adoption's Makefile append --------------------- + makefile_path = clone / MAKEFILE + makefile_path.write_text( + makefile_path.read_text(encoding="utf-8") + ADOPT_MAKEFILE_BLOCK, + encoding="utf-8") + set_pin_row_sha256(clone / "contracts" / "shape-pin.yaml", MAKEFILE, + file_sha256(makefile_path)) + git("add", "--", MAKEFILE, "contracts/shape-pin.yaml", cwd=clone) + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", + "Mirror an in-place adoption's Makefile append", cwd=clone) + + # ---- commit B': the Makefile TEMPLATE changes, like #80's make park ---- + makefile_source = upstream / MAKEFILE_SOURCE + other_source = upstream / CHANGED_SOURCE + makefile_source.write_text( + makefile_source.read_text(encoding="utf-8") + + "\n# make park / make resume, upstream (#80).\n", encoding="utf-8") + other_source.write_text( + other_source.read_text(encoding="utf-8") + + "\n# An upstream fix that must reach every project.\n", + encoding="utf-8") + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", + "Change the Makefile template and the manifest validator", + "--", MAKEFILE_SOURCE, CHANGED_SOURCE, cwd=upstream) + commit_b = git("rev-parse", "HEAD", cwd=upstream).stdout.strip() + assert commit_a != commit_b + + return {"upstream": upstream, "clone": clone, "a": commit_a, "b": commit_b} + + +@pytest.fixture +def adopted_root(adopted_upstream_and_project, tmp_path): + """A private, mutable copy of the adopted project as it stood at A.""" + target = tmp_path / (PROJECT + "Adopted") + shutil.copytree(adopted_upstream_and_project["clone"], target, + symlinks=True) + return target + + +def merged_makefile(adopted_upstream_and_project) -> str: + """The hand merge AGENTS.md's point 4 describes: the upstream's TARGET + template, with the adoption's own block kept on the end — "apply the + upstream delta, keep the block", exactly as `_materialize` would if it + ran again at the new commit.""" + upstream = adopted_upstream_and_project["upstream"] + target = adopted_upstream_and_project["b"] + template = subprocess.run( + ["git", "show", f"{target}:{MAKEFILE_SOURCE}"], cwd=str(upstream), + capture_output=True, check=True).stdout.decode("utf-8") + return template + ADOPT_MAKEFILE_BLOCK + + +def test_an_adopted_makefile_reports_both_and_names_accept_local( + adopted_root, adopted_upstream_and_project): + """The #84 reproduction: an adopted Makefile reads `both` the moment + upstream touches the template, via the "not a verbatim copy" branch + rather than "edited here AND upstream" — there is no local edit yet, only + the adoption's own append — and `check`'s detail now names the exit.""" + result = check(adopted_root, adopted_upstream_and_project) + assert result.returncode == 1, result.stdout + result.stderr + verdict = verdicts(result.stdout) + assert verdict[MAKEFILE] == "both" + assert verdict[CHANGED] == "upstream-changed" + assert f"--accept-local {MAKEFILE}" in result.stdout + assert "appends to it" in result.stdout, ( + "the 'not a verbatim copy ... appends to it' branch must be the one " + "that fired, not 'edited here AND upstream'") + + +def test_apply_without_accept_local_still_refuses_the_adopted_makefile( + adopted_root, adopted_upstream_and_project): + result = apply(adopted_root, adopted_upstream_and_project) + assert result.returncode == 2, result.stdout + assert "update-conflict" in result.stderr + assert MAKEFILE in result.stderr + assert load_yaml(adopted_root / "contracts" / "shape-pin.yaml")[ + "commit"].lower() == adopted_upstream_and_project["a"], ( + "a refused apply writes nothing at all") + + +def test_accept_local_on_an_adopted_makefile_repins_the_hand_merge( + adopted_root, adopted_upstream_and_project): + """AGENTS.md point 4's exit, taken exactly as written: the hand merge is + committed on a branch, then `apply --accept-local --branch ` lands ON it — items 1 and 2 together, because #84's own + reproduction needed both at once.""" + merged = merged_makefile(adopted_upstream_and_project) + (adopted_root / MAKEFILE).write_text(merged, encoding="utf-8") + branch = "shape/update-84-test" + git("checkout", "-q", "-b", branch, cwd=adopted_root) + git("add", "--", MAKEFILE, cwd=adopted_root) + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", + "Hand-merge the Makefile: upstream delta applied, adoption's block " + "kept", cwd=adopted_root) + + result = apply(adopted_root, adopted_upstream_and_project, + "--accept-local", MAKEFILE, "--branch", branch) + assert result.returncode == 0, result.stdout + result.stderr + assert f"committed on {branch}" in result.stdout + + assert git("rev-parse", "--abbrev-ref", "HEAD", + cwd=adopted_root).stdout.strip() == branch, ( + "apply must commit ONTO the branch already checked out, never " + "create a second one") + assert (adopted_root / MAKEFILE).read_text(encoding="utf-8") == merged + + rows = pin_rows(adopted_root) + assert rows[MAKEFILE] == file_sha256(adopted_root / MAKEFILE) + assert rows[CHANGED] == file_sha256(adopted_root / CHANGED) + + target = adopted_upstream_and_project["b"] + upstream = adopted_upstream_and_project["upstream"] + expected_changed = subprocess.run( + ["git", "show", f"{target}:{CHANGED_SOURCE}"], cwd=str(upstream), + capture_output=True, check=True).stdout + assert (adopted_root / CHANGED).read_bytes() == expected_changed + + pin = load_yaml(adopted_root / "contracts" / "shape-pin.yaml") + assert pin["commit"].lower() == target + assert pin["digests"]["tree_sha256"].lower() == tree_digest(upstream, + target) + manifest = load_yaml(adopted_root / "project.yaml") + assert manifest["shape"]["commit"].lower() == target + assert (manifest["shape"]["digests"]["tree_sha256"].lower() + == tree_digest(upstream, target)) + validators_are_green(adopted_root) + + +def test_accept_local_on_an_adopted_makefile_trusts_the_word_even_unmerged( + adopted_root, adopted_upstream_and_project): + """`--accept-local` is the person's word, not a check that a merge + actually happened: an UNMERGED adopted Makefile — still exactly the + bytes the pin already recorded — is accepted just the same, and + `validate-pins.py` stays green because the row is recomputed from + whatever is on disk, merged or not.""" + before = (adopted_root / MAKEFILE).read_bytes() + result = apply(adopted_root, adopted_upstream_and_project, + "--accept-local", MAKEFILE) + assert result.returncode == 0, result.stdout + result.stderr + assert (adopted_root / MAKEFILE).read_bytes() == before, ( + "the unmerged local bytes are kept; upstream's Makefile template is " + "never copied over an accepted row") + assert pin_rows(adopted_root)[MAKEFILE] == file_sha256( + adopted_root / MAKEFILE) + validators_are_green(adopted_root) + + # --- the bootstrap notice -------------------------------------------------- def test_bootstrap_says_so_when_it_can_tell_offline_that_the_shape_moved( diff --git a/update-shape.py b/update-shape.py index 2305da90..44fc7bb3 100755 --- a/update-shape.py +++ b/update-shape.py @@ -36,8 +36,11 @@ reports today into a digest that agrees with the fork — which is exactly the "record the fork as the standard" failure `shape-pin.yaml`'s own header forbids. - * A file changed on BOTH sides is refused outright. Two edits to one file is - a merge, and a merge is a human's judgement, not a byte copy. + * A file changed on BOTH sides is refused outright, UNLESS the human passes + `--accept-local ` on it too — the same exception `locally-modified` + gets, above. Two edits to one file is a merge, and a merge is a human's + judgement, not a byte copy; once they have made it and committed it, the + row is recomputed from what they merged, the same mechanism. * A copy that is not VERBATIM at the pinned commit — `adopt-project.py` appends a `CONTRACTS_DIR` block to an adopted Makefile — is treated as a conflict the moment upstream touches it, because copying the target bytes @@ -481,6 +484,13 @@ def classify(root: Path, rows: list[Row], upstream: Upstream, pinned: str, if locally_modified and upstream_changed: row.state = BOTH row.detail = "edited here AND upstream since the pin" + # Precomputed the same way `locally-modified` precomputes it, + # below: IF `--accept-local` later names this path, `cmd_apply` + # must re-pin from what is on disk NOW, not from whatever was + # true when the pin was written. Dead weight when nobody accepts + # it — an unaccepted `both` row is refused before any digest is + # written anywhere. + row.digest = local_digest continue if upstream_changed and not verbatim_at_pin: row.state = BOTH @@ -489,6 +499,7 @@ def classify(root: Path, rows: list[Row], upstream: Upstream, pinned: str, f"{source} at the pinned commit — an in-place adoption " "appends to it — so copying the target bytes would delete " "what was appended") + row.digest = local_digest continue if upstream_changed: row.state = UPSTREAM_CHANGED @@ -822,6 +833,19 @@ def rewrite_manifest(text: str, commit: str, tree: str, kind: Kind) -> str: # --------------------------------------------------------------------------- +def accept_local_exit(path: str) -> str: + """The one sentence `check` appends to a `both` row's detail: the exit + AGENTS.md documents for it, once a human has merged the file by hand and + committed that merge. + + A DISPLAY-ONLY ADDITION, not folded into `row.detail` itself: the same + field also renders inside `apply`'s `update-conflict` refusal, whose + wording an unaccepted `both` must keep exactly as it was. + """ + return (f". Merge it by hand, commit it, then `apply --accept-local " + f"{path}` so the row is recomputed from what you merged") + + def report(root: Path, upstream: Upstream, pinned: str, target: str, pin_tree: str, target_tree: str, rows: list[Row]) -> None: print(f"root {root}") @@ -836,9 +860,12 @@ def report(root: Path, upstream: Upstream, pinned: str, target: str, for row in rows: if row.state != state: continue + detail = row.detail + if row.state == BOTH and detail: + detail += accept_local_exit(row.path) line = f" {state:<{width}} {row.path}" - if row.detail: - line += f"\n {'':<{width}} ({row.detail})" + if detail: + line += f"\n {'':<{width}} ({detail})" print(line) @@ -975,6 +1002,37 @@ def run_validator(root: Path, script: str) -> int: return proc.returncode +def current_branch(root: Path) -> str | None: + """The branch HEAD is on, or None when it is detached. + + Named the same as `bump-leg.py`'s own helper for the same one-line read: + two different tools reading two different checkouts, so a shared import + would be the wrong coupling, but no reason for the name to drift. + """ + proc = subprocess.run(["git", "symbolic-ref", "--quiet", "--short", "HEAD"], + cwd=str(root), capture_output=True, text=True, + check=False) + return proc.stdout.strip() or None + + +def branch_exists(root: Path, branch: str) -> bool: + """Is `branch` a local branch of `root` — checked out or not?""" + proc = subprocess.run( + ["git", "show-ref", "--quiet", "--verify", f"refs/heads/{branch}"], + cwd=str(root), check=False) + return proc.returncode == 0 + + +def tracking_branch_of(manifest: dict | None) -> str: + """This root's tracking branch, out of an already-loaded manifest — the + same field `open_pull_request` lands its pull request on. `--branch` may + never commit onto it directly: these organisations are pull-request only, + and a commit whose only next step is a push to the default branch is not + one this tool will make. + """ + return str((manifest or {}).get("tracking_branch") or "main") + + def commit_on_branch(root: Path, branch: str, paths: list[str], target: str, upstream: Upstream, count: int, kind: Kind, added: list[str] = ()) -> None: @@ -991,9 +1049,49 @@ def commit_on_branch(root: Path, branch: str, paths: list[str], target: str, to git"). So the additions — and ONLY the additions, by name — are staged first. `git add -A` here would sweep in whatever else is in the tree, which is the very thing the explicit pathspecs exist to prevent. + + THREE POSTURES FOR `--branch `, decided from git rather than + assumed, because AGENTS.md's documented `both` exit needs the second one: + the hand merge is committed on a branch FIRST, and `apply --accept-local + --branch ` must be able to land ON it rather than + finding `git checkout -b` refusing a branch that is already there. + + * `` is the tracking branch — refused, current or not. These + organisations are pull-request only, and a commit whose only next + step is a push to the default branch is not one this tool will make. + * `` is the CURRENT branch (so, by the rule just above, not the + tracking branch) — committed onto it directly, with no checkout at + all. + * `` names a branch that exists and is NOT current — refused by + name: committing there would land this change beside whatever a + human already put on that branch, which is theirs to decide. + * Anything else is created fresh, as before. """ checked_value("--branch", branch) - run(["git", "checkout", "-q", "-b", branch], cwd=root) + manifest = load_yaml(root / kind.manifest) + tracking = tracking_branch_of(manifest) + if branch == tracking: + raise Refusal( + "update-branch-tracking", + f"--branch {branch} names {kind.manifest}'s tracking branch, " + "and this tool never commits a shape update directly onto it", + "Remediation: pass a different --branch. These organisations " + "are pull-request only; a shape update lands as a pull request " + "onto the tracking branch, never a commit made on it directly.") + here = current_branch(root) + if branch != here: + if branch_exists(root, branch): + checked_out = (f"{here!r} is checked out" if here + else "HEAD is detached") + raise Refusal( + "update-branch-exists", + f"--branch {branch} already exists and is not the current " + f"branch ({checked_out})", + "Remediation: check out that branch first — `git -C " + f"{root} checkout {branch}` — if that is where the hand " + "merge AGENTS.md describes was already committed, or pass " + "a different --branch to create a new one.") + run(["git", "checkout", "-q", "-b", branch], cwd=root) if added: run(["git", "add", "--", *added], cwd=root) gained = (f"; {len(added)} new upstream file(s) added because --add named " @@ -1048,7 +1146,17 @@ def cmd_apply(args) -> int: f"{upstream.repository} @ {target[:12]}, which the pin " "already names") return 0 - conflicts = [row for row in rows if row.is_conflict] + # `both` ALONE has an accepted exit: `--accept-local ` on it + # means the human already did the merge `update-conflict` below asks + # for, so that row is excluded here and handled with LOCALLY_MODIFIED + # further down — never copied over, and re-pinned from what is on + # disk. The other three conflict states (upstream-removed, unmapped, + # copy-missing) have no such exit: there is no local edit to trust a + # word about, so `--accept-local` naming one of those is simply not + # among the paths this can ever exclude. + conflicts = [row for row in rows + if row.is_conflict + and not (row.state == BOTH and row.path in accepted)] if conflicts: raise Refusal( "update-conflict", @@ -1104,7 +1212,7 @@ def write_added(path: Path, data: bytes, executable: bool) -> None: write(root / row.path, row.target_bytes or b"") print(f" copied {row.path}") for row in rows: - if row.state == LOCALLY_MODIFIED: + if row.state in (LOCALLY_MODIFIED, BOTH): print(f" kept {row.path} (local bytes; row recomputed " "from them because --accept-local named it)") elif row.state == ALREADY_AT_TARGET: @@ -1176,7 +1284,7 @@ def write_added(path: Path, data: bytes, executable: bool) -> None: def open_pull_request(root: Path, branch: str, target: str, upstream: Upstream, kind: Kind = PROJECT_KIND) -> None: manifest = load_yaml(root / kind.manifest) - base = str((manifest or {}).get("tracking_branch") or "main") + base = tracking_branch_of(manifest) if kind is FAMILY_KIND: # A family declares its own repository: it has no legs to read one # from, being a holder rather than a project. From 795253f6c1d2d176e4102e94a9897fac7a81ac95 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Wed, 9 Sep 2026 23:35:20 -0400 Subject: [PATCH 2/2] test_update_shape.py: the adopted-root fixture builds on the existing one (SonarCloud duplication gate on #85) (#84) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #85's quality gate failed on one metric: 4.4% duplication on new code (required <= 3%), no issues raised. `adopted_upstream_and_project` had retyped `upstream_and_project`'s clone-then-scaffold-then-recursive-clone block verbatim, and three tests each closed with the same "exit 2, refusal id in stderr, pin's commit: untouched" tail. `scaffold_upstream_and_project(tmp_path_factory, subdir)` is the part both fixtures share: a fresh clone of this repository, `PROJECT` scaffolded from it, and a recursive clone of that. Returns the pieces rather than a finished fixture dict on purpose — one shared return shape across two fixtures is how the second one drifts from the first — so each fixture still makes its own commit(s) on top and returns its own dict. `commit_upstream_changes(upstream, message, {source: text})` folds the append-write-then-commit-then-capture-the-hash steps `upstream_and_project`'s commit B and `adopted_upstream_and_project`'s commit B' both took; the appended comment the two share for `CHANGED_SOURCE` is now the one constant `UPSTREAM_FIX_LINE` rather than two copies of the same string. `assert_refused_and_unpinned(result, root, upstream_and_project, refusal_id, *needles)` is the repeated tail, now used by both `--branch` refusal tests, the `both`-row conflict test, and the adopted-Makefile conflict test. Behaviour and assertions are unchanged — same fixtures returned, same refusals, same messages checked — and every docstring survives. Two clones stay two clones: `adopted_upstream_and_project` still gets its OWN clone rather than a further commit on `upstream_and_project`'s, because that one is shared by every bare `check()` call in this module and a further commit on it would move their implicit target commit out from under them. 82 passed (test_update_shape.py, test_update_shape_add.py, test_repo_hygiene.py); 738 passed, 1 skipped for the full suite. Lane: xfactory-2 (xFactory-2) Co-Authored-By: Claude Fable 5.1 --- tests/test_update_shape.py | 143 ++++++++++++++++++++----------------- 1 file changed, 77 insertions(+), 66 deletions(-) diff --git a/tests/test_update_shape.py b/tests/test_update_shape.py index 42e28c96..e48634d7 100644 --- a/tests/test_update_shape.py +++ b/tests/test_update_shape.py @@ -69,10 +69,16 @@ def update_shape(): MAKEFILE_SOURCE = f"templates/assembly-root/{MAKEFILE}" -@pytest.fixture(scope="module") -def upstream_and_project(tmp_path_factory) -> dict: - """A clone of this repository at A, a project scaffolded from it, and B.""" - base = tmp_path_factory.mktemp("update-shape") +def scaffold_upstream_and_project(tmp_path_factory, subdir: str): + """A fresh clone of this repository at A, and `PROJECT` scaffolded from + it into a recursive clone of its own — the part `upstream_and_project` + and `adopted_upstream_and_project` share verbatim. They differ only in + what commits they make on top afterward, which is why this stops here + and returns the pieces rather than a finished fixture dict: a shared + RETURN SHAPE across two fixtures is how the second one quietly drifts + from the first. + """ + base = tmp_path_factory.mktemp(subdir) upstream = base / "openRepoShape" proc = subprocess.run(["git", "clone", "-q", str(REPO), str(upstream)], capture_output=True, text=True, check=False) @@ -92,15 +98,43 @@ def upstream_and_project(tmp_path_factory) -> dict: str(base / "remotes" / f"{PROJECT}.git"), str(clone)], capture_output=True, text=True, check=False) assert proc.returncode == 0, proc.stderr + return base, upstream, commit_a, clone - # ---- commit B: one copied template file changes upstream -------------- - source = upstream / CHANGED_SOURCE - source.write_text(source.read_text(encoding="utf-8") - + "\n# An upstream fix that must reach every project.\n", - encoding="utf-8") + +#: The one line appended to a template source to make it "upstream changed" +#: for `classify()` — shared so `upstream_and_project`'s commit B and +#: `adopted_upstream_and_project`'s commit B' touch `CHANGED_SOURCE` with the +#: exact same bytes rather than two typings of one comment. +UPSTREAM_FIX_LINE = "\n# An upstream fix that must reach every project.\n" + + +def commit_upstream_changes(upstream, message: str, + changes: dict[str, str]) -> str: + """Append `{source path relative to upstream: text}` to each file and + commit them together. The one shape both `upstream_and_project`'s + commit B and `adopted_upstream_and_project`'s commit B' take — append, + commit with explicit pathspecs, return the new hash — so a third upstream + commit anywhere in this module reaches for this rather than retyping it. + """ + for source, text in changes.items(): + path = upstream / source + path.write_text(path.read_text(encoding="utf-8") + text, + encoding="utf-8") git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", - "-m", "Fix the manifest validator", "--", CHANGED_SOURCE, cwd=upstream) - commit_b = git("rev-parse", "HEAD", cwd=upstream).stdout.strip() + "-m", message, "--", *changes, cwd=upstream) + return git("rev-parse", "HEAD", cwd=upstream).stdout.strip() + + +@pytest.fixture(scope="module") +def upstream_and_project(tmp_path_factory) -> dict: + """A clone of this repository at A, a project scaffolded from it, and B.""" + _base, upstream, commit_a, clone = scaffold_upstream_and_project( + tmp_path_factory, "update-shape") + + # ---- commit B: one copied template file changes upstream -------------- + commit_b = commit_upstream_changes( + upstream, "Fix the manifest validator", + {CHANGED_SOURCE: UPSTREAM_FIX_LINE}) assert commit_a != commit_b return {"upstream": upstream, "clone": clone, "a": commit_a, "b": commit_b} @@ -155,6 +189,21 @@ def validators_are_green(root) -> None: f"{script} is red after the update:\n{result.stdout}{result.stderr}") +def assert_refused_and_unpinned(result, root, upstream_and_project, + refusal_id: str, *needles: str) -> None: + """The shape every REFUSED `apply` in this module takes: exit 2, the + refusal id (and anything else `needles` names) in stderr, and the pin's + `commit:` untouched — "a refused apply writes nothing at all", repeated + enough times across the `--branch` postures and the adopted-Makefile + conflict that a helper is the row's own row, not a fourth copy of it.""" + assert result.returncode == 2, result.stdout + assert refusal_id in result.stderr + for needle in needles: + assert needle in result.stderr + assert load_yaml(root / "contracts" / "shape-pin.yaml")["commit"].lower() \ + == upstream_and_project["a"], "a refused apply writes nothing at all" + + # --- check ----------------------------------------------------------------- def test_check_names_the_changed_file_and_calls_the_rest_unchanged( @@ -261,14 +310,11 @@ def test_branch_naming_an_existing_non_current_branch_refuses_by_name( git("branch", "shape/someone-elses", cwd=root) result = apply(root, upstream_and_project, "--branch", "shape/someone-elses") - assert result.returncode == 2, result.stdout - assert "update-branch-exists" in result.stderr - assert "shape/someone-elses" in result.stderr + assert_refused_and_unpinned(result, root, upstream_and_project, + "update-branch-exists", "shape/someone-elses") assert git("rev-parse", "--abbrev-ref", "HEAD", cwd=root).stdout.strip() == "main", ( "a refused apply must not leave the checkout on a different branch") - assert load_yaml(root / "contracts" / "shape-pin.yaml")["commit"].lower() \ - == upstream_and_project["a"], "a refused apply writes nothing at all" def test_branch_naming_the_tracking_branch_refuses(root, upstream_and_project): @@ -276,11 +322,8 @@ def test_branch_naming_the_tracking_branch_refuses(root, upstream_and_project): proof the tracking-branch refusal is checked BEFORE the current-branch fast path, not after it.""" result = apply(root, upstream_and_project, "--branch", "main") - assert result.returncode == 2, result.stdout - assert "update-branch-tracking" in result.stderr - assert "main" in result.stderr - assert load_yaml(root / "contracts" / "shape-pin.yaml")["commit"].lower() \ - == upstream_and_project["a"] + assert_refused_and_unpinned(result, root, upstream_and_project, + "update-branch-tracking", "main") def test_apply_without_yes_refuses_where_nobody_can_be_asked( @@ -417,11 +460,8 @@ def test_changed_on_both_sides_refuses_and_names_the_file( assert f"--accept-local {CHANGED}" in checked.stdout result = apply(root, upstream_and_project) - assert result.returncode == 2, result.stdout - assert "update-conflict" in result.stderr - assert CHANGED in result.stderr - assert load_yaml(root / "contracts" / "shape-pin.yaml")["commit"].lower() \ - == upstream_and_project["a"] + assert_refused_and_unpinned(result, root, upstream_and_project, + "update-conflict", CHANGED) def test_accept_local_on_a_both_row_repins_from_the_local_bytes( @@ -545,28 +585,11 @@ def adopted_upstream_and_project(tmp_path_factory) -> dict: commit on ITS clone would move all of them onto a target none of them expect. This fixture's own commit B' changes the Makefile TEMPLATE — the one thing no other test here may see move — so it gets its own clone, - built the same way `upstream_and_project` is. + built by the same `scaffold_upstream_and_project` helper + `upstream_and_project` calls. """ - base = tmp_path_factory.mktemp("update-shape-adopted") - upstream = base / "openRepoShape" - proc = subprocess.run(["git", "clone", "-q", str(REPO), str(upstream)], - capture_output=True, text=True, check=False) - assert proc.returncode == 0, proc.stderr - commit_a = git("rev-parse", "HEAD", cwd=upstream).stdout.strip() - - result = run_script( - upstream / "scaffold-project.py", "--org", ORG, "--project", PROJECT, - "--elected-by", "Test Human", "--elected-on", "2026-09-02", - "--local-remote-dir", str(base / "remotes"), - "--work-dir", str(base / "work")) - assert result.returncode == 0, result.stderr + result.stdout - - clone = base / "clone" / PROJECT - proc = subprocess.run( - ["git", *FILE_PROTOCOL, "clone", "-q", "--recurse-submodules", - str(base / "remotes" / f"{PROJECT}.git"), str(clone)], - capture_output=True, text=True, check=False) - assert proc.returncode == 0, proc.stderr + _base, upstream, commit_a, clone = scaffold_upstream_and_project( + tmp_path_factory, "update-shape-adopted") # ---- mirror an in-place adoption's Makefile append --------------------- makefile_path = clone / MAKEFILE @@ -580,19 +603,10 @@ def adopted_upstream_and_project(tmp_path_factory) -> dict: "Mirror an in-place adoption's Makefile append", cwd=clone) # ---- commit B': the Makefile TEMPLATE changes, like #80's make park ---- - makefile_source = upstream / MAKEFILE_SOURCE - other_source = upstream / CHANGED_SOURCE - makefile_source.write_text( - makefile_source.read_text(encoding="utf-8") - + "\n# make park / make resume, upstream (#80).\n", encoding="utf-8") - other_source.write_text( - other_source.read_text(encoding="utf-8") - + "\n# An upstream fix that must reach every project.\n", - encoding="utf-8") - git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", - "Change the Makefile template and the manifest validator", - "--", MAKEFILE_SOURCE, CHANGED_SOURCE, cwd=upstream) - commit_b = git("rev-parse", "HEAD", cwd=upstream).stdout.strip() + commit_b = commit_upstream_changes( + upstream, "Change the Makefile template and the manifest validator", + {MAKEFILE_SOURCE: "\n# make park / make resume, upstream (#80).\n", + CHANGED_SOURCE: UPSTREAM_FIX_LINE}) assert commit_a != commit_b return {"upstream": upstream, "clone": clone, "a": commit_a, "b": commit_b} @@ -640,12 +654,9 @@ def test_an_adopted_makefile_reports_both_and_names_accept_local( def test_apply_without_accept_local_still_refuses_the_adopted_makefile( adopted_root, adopted_upstream_and_project): result = apply(adopted_root, adopted_upstream_and_project) - assert result.returncode == 2, result.stdout - assert "update-conflict" in result.stderr - assert MAKEFILE in result.stderr - assert load_yaml(adopted_root / "contracts" / "shape-pin.yaml")[ - "commit"].lower() == adopted_upstream_and_project["a"], ( - "a refused apply writes nothing at all") + assert_refused_and_unpinned(result, adopted_root, + adopted_upstream_and_project, + "update-conflict", MAKEFILE) def test_accept_local_on_an_adopted_makefile_repins_the_hand_merge(