From 5ac03de309e851db92165dadc128aa7295dd0559 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 11:13:25 +0000 Subject: [PATCH 1/3] shape_materialize.py: `RULESET_HINT` names the run's tracking branch where it named `main`, and the scaffold, the family holder and both adopt legs pass it, so a `--tracking-branch trunk` run's refused push no longer advises `push -u origin main:seed/scaffold` (a branch its work clone does not have) and `gh pr create --base main` (a base its repository does not have) (#324) `RULESET_HINT` is what a person reads when an organisation's ruleset has just refused a direct push: seed a branch, open a pull request. It hard-coded `main` as the local branch of `git -C push -u origin main:seed/scaffold` and as `gh pr create --base main`. A run made with `--tracking-branch trunk` makes its work clones with `git init -b trunk` and pushes `trunk`, so the first command died on `error: src refspec main does not match any` and the second named a base the repository does not have. The hint now carries a `{tracking}` field in exactly those two places, and each place that prints it passes the run's tracking branch: - `scaffold-project.py`: `_seed_legs` (a leg's push) and `_build_assembly_root` (the root's) pass `args.tracking_branch`. - `scripts/family.py`: `_create_holder_worktree` passes `args.tracking_branch`. - `adopt-project.py`: `_extract_leg` and `_seed_leg` pass the plan's `tracking`, the branch `_open_the_pull_request` already uses as its own `--base`. - No caller lacked a tracking branch to pass. Each value has been through `checked_value` by then (it admits only `[A-Za-z0-9._/@+~-]` and no leading `-`), so it is printed as it stands, like `{repo}`. - `tests/test_ruleset_hint_tracking_branch.py` runs each of the five callers against local bare remotes whose `pre-receive` hook refuses everything but `seed/*` (a ruleset in miniature, installed through `GIT_TEMPLATE_DIR`), once with `--tracking-branch trunk` and once with the default. It pins the printed block against a literal copy of the hint as it was, runs the printed push where it can run (the scaffold's legs and root, the family holder) and sees `seed/scaffold` arrive, and scans every shipped `RULESET_HINT.format(` for a call that leaves `tracking=` out, which would raise `KeyError` inside the failure path that exists to explain a refusal. Unchanged, and diffed on the five callers' printed output before and after: for the default `main` the hint is byte-identical. What the hint advises is the same, and so are its two exits (seed by pull request, or ask an operator with the bypass right; never `--admin`, per AGENTS.md); only the branch it names moves. Left out: an adoption's printed `push -u origin` still cannot run, because `git filter-repo` removed `origin` from the extracted clone and the seeded clone never had one (#177, open). The branch it now names is the one the clone holds whenever the plan's tracking branch is the source's default branch, which is where the adoption's own pull request already lands; #177's own hint is the place to push `HEAD`. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Sonnet 5.5 --- adopt-project.py | 8 +- scaffold-project.py | 8 +- scripts/family.py | 4 +- scripts/shape_materialize.py | 10 +- tests/test_ruleset_hint_tracking_branch.py | 318 +++++++++++++++++++++ 5 files changed, 339 insertions(+), 9 deletions(-) create mode 100644 tests/test_ruleset_hint_tracking_branch.py diff --git a/adopt-project.py b/adopt-project.py index aaf2c5bc..6b9ebdd2 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -3081,8 +3081,8 @@ def _extract_leg(role: str, source: Source, work: Path, paths: list[str], run(["git", "push", "-q", url, f"HEAD:refs/heads/{tracking}"], cwd=work) except CommandFailed as exc: print(exc.loudly(f"pushing the {role} leg"), file=sys.stderr) - print(RULESET_HINT.format(work=work, repo=repository, role=role), - file=sys.stderr) + print(RULESET_HINT.format(work=work, repo=repository, role=role, + tracking=tracking), file=sys.stderr) raise count = git_out(["rev-list", "--count", "HEAD"], cwd=work) print(f" {role:<5} {len(paths):>3} path(s) -> {head[:12]} " @@ -3129,8 +3129,8 @@ def _seed_leg(role: str, work: Path, values: dict, branch: str, url: str, run(["git", "push", "-q", url, f"HEAD:refs/heads/{tracking}"], cwd=work) except CommandFailed as exc: print(exc.loudly(f"pushing the seeded {role} leg"), file=sys.stderr) - print(RULESET_HINT.format(work=work, repo=repository, role=role), - file=sys.stderr) + print(RULESET_HINT.format(work=work, repo=repository, role=role, + tracking=tracking), file=sys.stderr) raise print(f" {role:<5} 0 path(s) -> {commit[:12]} (SEEDED from " f"{SEED_TEMPLATE[role]}/) -> {url}") diff --git a/scaffold-project.py b/scaffold-project.py index b6d5e3ac..8707e855 100755 --- a/scaffold-project.py +++ b/scaffold-project.py @@ -1044,7 +1044,9 @@ def _seed_legs(args, values: dict, names: dict, repositories: dict, except CommandFailed as exc: print(exc.loudly(f"pushing the {role} leg"), file=sys.stderr) print(RULESET_HINT.format(work=work, repo=repositories[role], - role=role), file=sys.stderr) + role=role, + tracking=args.tracking_branch), + file=sys.stderr) return None print(f" {role:<9} {commit[:12]} tree {leg_digests[role][:12]}… " f"-> {urls[role]}") @@ -1160,7 +1162,9 @@ def _build_assembly_root(args, values: dict, names: dict, repositories: dict, print(exc.loudly("pushing the assembly root"), file=sys.stderr) print(RULESET_HINT.format(work=assembly, repo=repositories["assembly"], - role="assembly"), file=sys.stderr) + role="assembly", + tracking=args.tracking_branch), + file=sys.stderr) return False print(f" assembly {run(['git', 'rev-parse', 'HEAD'], cwd=assembly)[:12]} " f"-> {urls['assembly']}") diff --git a/scripts/family.py b/scripts/family.py index 03c41335..2256c7ca 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -921,7 +921,9 @@ def _create_holder_worktree(args, work: Path, values: dict, shape_commit: str, except CommandFailed as exc: print(exc.loudly("pushing the family holder"), file=sys.stderr) print(RULESET_HINT.format(work=work, repo=repository, - role="family"), file=sys.stderr) + role="family", + tracking=args.tracking_branch), + file=sys.stderr) return 2 return None diff --git a/scripts/shape_materialize.py b/scripts/shape_materialize.py index e6f41ead..b54c6dba 100644 --- a/scripts/shape_materialize.py +++ b/scripts/shape_materialize.py @@ -711,6 +711,12 @@ def loudly(self, what: str) -> str: ) +#: `{tracking}` is the run's tracking branch (`--tracking-branch`, the plan's +#: `tracking_branch`): the branch the refused push targeted, which is also the +#: branch a work clone is made on and the base a seeding pull request lands on +#: (#324). Every caller passes it, after `checked_value` has admitted it, so +#: it is a safe argument and is not quoted here. A call that leaves it out +#: fails `tests/test_ruleset_hint_tracking_branch.py`. RULESET_HINT = """ If the organisation applies a ruleset requiring changes to arrive by pull request, a direct push to the default branch is refused BY DESIGN and must not @@ -719,8 +725,8 @@ def loudly(self, what: str) -> str: (1) have an operator holding the bypass right seed the default branch once, then re-run this scaffold; or (2) push a seed BRANCH and open a pull request: - git -C {work} push -u origin main:seed/scaffold - gh pr create --repo {repo} --base main --head seed/scaffold \\ + git -C {work} push -u origin {tracking}:seed/scaffold + gh pr create --repo {repo} --base {tracking} --head seed/scaffold \\ --title 'Seed the {role} leg' --body 'Scaffolded shape.' NOTHING has been rolled back. What already exists is listed above; delete it diff --git a/tests/test_ruleset_hint_tracking_branch.py b/tests/test_ruleset_hint_tracking_branch.py new file mode 100644 index 00000000..bea3cdd7 --- /dev/null +++ b/tests/test_ruleset_hint_tracking_branch.py @@ -0,0 +1,318 @@ +# SPDX-License-Identifier: Apache-2.0 +"""The hint a refused push prints names the run's own tracking branch (#324). + +`RULESET_HINT` is what a person reads when an organisation's ruleset has just +refused a direct push, and it says to push a seed BRANCH and open a pull +request. It named `main` twice, as a constant: the local branch in `git -C + push -u origin main:seed/scaffold`, and `gh pr create --base main`. A +run made with `--tracking-branch trunk` creates its work clones with `git init +-b trunk` and pushes `trunk`, so the first command died on `error: src refspec +main does not match any` and the second named a base the repository does not +have. + +WHAT IS PINNED HERE + * the text itself: byte for byte what it was for `main` (the literal below is + the hint as it stood before this change, with the branch left as a marker), + and `trunk` in exactly the two places `main` stood, for `trunk`; + * every one of the five places that prints it passes the branch, run through + the real tools against local bare remotes that refuse like a ruleset; + * where the printed push is runnable (the scaffold's legs and root, a family + holder), it is RUN, and the seed branch arrives. + +THE RULESET IS A `pre-receive` HOOK in each bare remote the tool makes, put +there by `GIT_TEMPLATE_DIR`, which every `git init` the tool runs honours. The +hook refuses a push to anything but `seed/*` for the repositories named in +`RULESET_REFUSES`: the hook runs in the remote's own directory, so the name +picks which of the three a run sees refuse. It is a shebang script, which Git +for Windows runs through its own `sh` (`test_update_shape_add_ignored.py` does +the same). No `gh`, no network and no real repository is involved. + +The extracted and the seeded leg of an ADOPTION print the hint too, and its +`push -u origin` cannot run there: `git filter-repo` removed `origin` from the +extracted clone, and the seeded clone never had one (#177, open). That is not +this change's to fix, so those two assert the text and that the branch the +text names is one the work clone holds. +""" + +from __future__ import annotations + +import ast +import os +import re +import shutil +import string +import subprocess +import sys +from pathlib import Path + +import pytest + +from conftest import (ADOPT, ORG, PROJECT, REPO, SCAFFOLD, git, make_source_repo, + resolve, run_script, write_plan) +from test_adopt_spec_only import ANSWERS, EDITS, SPEC_ONLY_TREE +from test_git_output_decoding import shipped_python + +sys.path.insert(0, str(REPO / "scripts")) +from shape_materialize import RULESET_HINT # noqa: E402 + +FAMILY = REPO / "scripts" / "family.py" + +#: The hint as it stood on `main` before #324, from its first line to its last, +#: with the four values it carries as markers. Raw, because the second command +#: ends in a backslash. It is a literal rather than a copy of the module's own +#: constant so that the test cannot agree with whatever the constant becomes. +HINT = r"""If the organisation applies a ruleset requiring changes to arrive by pull +request, a direct push to the default branch is refused BY DESIGN and must not +be worked around. Two legitimate exits: + + (1) have an operator holding the bypass right seed the default branch once, + then re-run this scaffold; or + (2) push a seed BRANCH and open a pull request: + git -C @WORK@ push -u origin @BRANCH@:seed/scaffold + gh pr create --repo @REPO@ --base @BRANCH@ --head seed/scaffold \ + --title 'Seed the @ROLE@ leg' --body 'Scaffolded shape.' + +NOTHING has been rolled back. What already exists is listed above; delete it +by hand if you want a clean re-run.""" + + +def hint_for(work: str, repo: str, role: str, tracking: str) -> str: + return (HINT.replace("@WORK@", work).replace("@REPO@", repo) + .replace("@ROLE@", role).replace("@BRANCH@", tracking)) + + +# --- the text --------------------------------------------------------------- + +def test_the_hint_for_main_is_the_text_it_always_was(): + printed = RULESET_HINT.format(work="W", repo="o/r", role="spec", + tracking="main") + assert printed == "\n" + hint_for("W", "o/r", "spec", "main") + "\n" + + +def test_the_hint_for_another_branch_changes_that_branch_and_nothing_else(): + printed = RULESET_HINT.format(work="W", repo="o/r", role="spec", + tracking="trunk") + assert printed == "\n" + hint_for("W", "o/r", "spec", "trunk") + "\n" + assert "main" not in printed, "no `main` is left in the hint" + assert printed.count("trunk") == 2, "the local branch and `--base`" + # The exits are the ones AGENTS.md allows, and `--admin` is not one. + assert "--admin" not in printed + assert "bypass right" in printed and "pull request" in printed + + +def test_the_hint_takes_four_fields_and_the_branch_is_one_of_them(): + fields = {name for _, name, _, _ in string.Formatter().parse(RULESET_HINT) + if name} + assert fields == {"work", "repo", "role", "tracking"} + + +# --- every caller passes it -------------------------------------------------- + +def hint_calls() -> list[tuple[str, int, set]]: + """Every `RULESET_HINT.format(...)` in the shipped tools, as `(file, line, + the keywords it names)`.""" + found = [] + for path in shipped_python(): + tree = ast.parse(path.read_text(encoding="utf-8")) + for node in ast.walk(tree): + if (isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == "format" + and isinstance(node.func.value, ast.Name) + and node.func.value.id == "RULESET_HINT"): + found.append((path.relative_to(REPO).as_posix(), node.lineno, + {keyword.arg for keyword in node.keywords})) + return found + + +def test_no_call_that_prints_the_hint_leaves_the_branch_out(): + calls = hint_calls() + assert {"scaffold-project.py", "adopt-project.py", + "scripts/family.py"} <= {path for path, _, _ in calls}, ( + "the scan no longer finds the tools that print the hint, so it is not " + "reading what it claims to") + offenders = [f"{path}:{line} names only {sorted(named)}" + for path, line, named in calls if "tracking" not in named] + assert not offenders, ( + "`RULESET_HINT.format` without `tracking=` raises KeyError inside the " + "failure path that exists to explain a refusal (#324):\n " + + "\n ".join(offenders)) + + +# --- a ruleset in miniature -------------------------------------------------- + +HOOK = (b"#!/bin/sh\n" + b"# A ruleset: while RULESET_REFUSES names the repository this runs in,\n" + b"# a push to anything but a seed/* branch is refused.\n" + b'here=$(basename "$(pwd)")\n' + b'case " $RULESET_REFUSES " in\n' + b' *" $here "*) ;;\n' + b" *) exit 0 ;;\n" + b"esac\n" + b"while read old new ref; do\n" + b' case "$ref" in\n' + b" refs/heads/seed/*) ;;\n" + b' *) echo "remote: error: GH013: Repository rule violations found ' + b'for $ref: changes must be made through a pull request." >&2\n' + b" exit 1 ;;\n" + b" esac\n" + b"done\n" + b"exit 0\n") + + +def refusing(template, *repositories: str) -> dict: + """The environment under which every bare repository a tool makes refuses + a direct push, for the repositories named (`Atlas-spec.git`).""" + return {"GIT_TEMPLATE_DIR": str(template), + "RULESET_REFUSES": " ".join(repositories)} + + +@pytest.fixture(scope="module") +def template(tmp_path_factory): + """A git template directory whose only file is the refusing hook, after + proving the hook refuses on this machine. + + A machine whose own `core.hooksPath` is set never reads a repository's + hooks, which would make every test below fail for a reason that is about + the machine; that, and only that, is a skip. Anything else that lets the + push through is a failure. + """ + root = tmp_path_factory.mktemp("ruleset") + hooks = root / "template" / "hooks" + hooks.mkdir(parents=True) + (hooks / "pre-receive").write_bytes(HOOK) + (hooks / "pre-receive").chmod(0o755) + template = root / "template" + env = {**os.environ, **refusing(template, "probe.git")} + bare, work = root / "probe.git", root / "probe" + work.mkdir() + + def probe(*args, cwd=root): + return subprocess.run(["git", *args], cwd=str(cwd), env=env, + capture_output=True, text=True, check=False) + + assert probe("init", "-q", "--bare", "-b", "trunk", str(bare)).returncode == 0 + assert probe("init", "-q", "-b", "trunk", ".", cwd=work).returncode == 0 + made = probe("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", + "--allow-empty", "-m", "probe", cwd=work) + assert made.returncode == 0, made.stderr + direct = probe("push", "-q", str(bare), "HEAD:refs/heads/trunk", cwd=work) + if direct.returncode == 0: + hooks_path = probe("config", "--get", "core.hooksPath") + if hooks_path.stdout.strip(): + pytest.skip("this machine's git sets core.hooksPath, so a " + "repository's own hooks are never run and a bare " + "remote cannot be made to refuse like a ruleset") + pytest.fail("the simulated ruleset let a direct push through: " + + direct.stderr) + seed = probe("push", "-q", str(bare), "HEAD:refs/heads/seed/scaffold", + cwd=work) + assert seed.returncode == 0, "the ruleset also refused the seed branch" + return template + + +# --- the five places that print it ------------------------------------------- + +#: What each caller refuses, with the role and repository its hint names. +#: `scaffold-spec` is the first leg's push, `scaffold-root` the assembly +#: root's after both legs went through, `family` the holder's. +CASES = { + "scaffold-spec": ("Atlas-spec.git", "spec", f"{ORG}/Atlas-spec"), + "scaffold-root": ("Atlas.git", "assembly", f"{ORG}/Atlas"), + "family": ("Contoso.git", "family", f"{ORG}/Contoso"), + "adopt-extracted": ("Redwood-spec.git", "spec", f"{ORG}/Redwood-spec"), + "adopt-seeded": ("Redwood-code.git", "code", f"{ORG}/Redwood-code"), +} +NEEDS_FILTER_REPO = ("adopt-extracted", "adopt-seeded") + + +def refused_run(case: str, tracking: str, tmp_path, template): + """Run the tool that prints `case`'s hint against remotes that refuse its + push, with `--tracking-branch `; returns the process, the + directory the remotes are in and the work directory.""" + refused = CASES[case][0] + env = refusing(template, refused) + remotes, work = tmp_path / "remotes", tmp_path / "work" + where = ("--local-remote-dir", str(remotes), "--work-dir", str(work)) + if case.startswith("scaffold"): + result = run_script( + SCAFFOLD, "--org", ORG, "--project", PROJECT, + "--elected-by", "Test Human", "--elected-on", "2026-09-02", + "--tracking-branch", tracking, *where, env=env) + elif case == "family": + result = run_script( + FAMILY, "init", "--org", ORG, "--family", "Contoso", + "--created-by", "Test Human", "--created-on", "2026-09-04", + "--tracking-branch", tracking, *where, env=env) + else: + # The source's own default branch IS the tracking branch, as it is + # wherever the plan's pull request has a base to land on. + source = make_source_repo(tmp_path / "IRRS", SPEC_ONLY_TREE, + branch=tracking, edits=EDITS) + plan = tmp_path / "adoption-plan.yaml" + written = write_plan(source, plan, project="Redwood", + extra=("--tracking-branch", tracking, + "--allow-empty-leg", "code")) + assert written.returncode == 0, written.stderr + written.stdout + for path, leg in ANSWERS: + resolve(plan, path, leg) + result = run_script(ADOPT, "execute", "--plan", str(plan), "--yes", + "--allow-empty-leg", "code", *where, env=env) + return result, remotes, work + + +def printed_hint(result, case: str, tracking: str) -> tuple[str, str]: + """`(the hint block, the work clone it names)`, after checking the block + is the whole hint with this case's role and repository.""" + assert result.returncode == 2, result.stderr + result.stdout + start = result.stderr.index("If the organisation applies a ruleset") + block = result.stderr[start:] + named = re.search(r"^ +git -C (.+) push -u origin \S+:seed/scaffold$", + block, re.M) + assert named, block + _, role, repository = CASES[case] + assert block.startswith(hint_for(named[1], repository, role, tracking) + + "\n"), block + return block, named[1] + + +@pytest.mark.parametrize("tracking", ["trunk", "main"]) +@pytest.mark.parametrize("case", CASES) +def test_the_hint_names_the_tracking_branch_the_run_was_made_with( + case, tracking, tmp_path, template): + if case in NEEDS_FILTER_REPO and shutil.which("git-filter-repo") is None: + pytest.skip("git filter-repo is not installed: " + "`pip install git-filter-repo`") + result, remotes, _ = refused_run(case, tracking, tmp_path, template) + block, work = printed_hint(result, case, tracking) + assert f"origin {tracking}:seed/scaffold" in block + assert f"--base {tracking} --head seed/scaffold" in block + if tracking != "main": + # Not the work path: a directory may be called anything. + assert "main" not in block.replace(work, ""), ( + "the default branch is not this run's") + + # The branch the hint names as the LOCAL one is one the clone holds. + held = git("rev-parse", "--verify", "-q", f"refs/heads/{tracking}", + cwd=Path(work), check=False) + assert held.returncode == 0, ( + f"the work clone has no branch {tracking}: " + + git("branch", "-a", cwd=Path(work)).stdout) + + # The refusal was real: nothing reached the tracking branch. + bare = remotes / (CASES[case][0]) + assert git("rev-parse", "--verify", "-q", f"refs/heads/{tracking}", + cwd=bare, check=False).returncode != 0 + + if case in NEEDS_FILTER_REPO: + return + # Where the printed command can run, run it: the seed branch arrives, at + # the commit the work clone holds. + pushed = subprocess.run(["git", "-C", work, "push", "-u", "origin", + f"{tracking}:seed/scaffold"], + capture_output=True, text=True, check=False, + env={**os.environ, **refusing(template, + CASES[case][0])}) + assert pushed.returncode == 0, pushed.stderr + assert git("rev-parse", "refs/heads/seed/scaffold", cwd=bare).stdout == \ + git("rev-parse", f"refs/heads/{tracking}", cwd=Path(work)).stdout From cdf9f98e1cfa528b2498af4a996068f1d28e9381 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 15:32:04 +0000 Subject: [PATCH 2/3] shape_materialize.py: `RULESET_HINT` names the ref its refused push carried, `HEAD` for both adopt legs, so an extracted leg's hint no longer advises pushing the source's own branch of the tracking branch's name in place of the commit the tool pushed (#324) Review of the first commit of this branch found that `{tracking}` was one value doing two jobs. As the base of `gh pr create` it is right everywhere. As the local side of `git -C push -u origin :seed/scaffold` it is the ref the refused push CARRIED only where the tool pushed the branch by name, which the scaffold and the family holder do from a clone made on it. An adoption pushes `HEAD:refs/heads/`, and the extracted leg's clone holds the source's own branch of that name, filtered by `git filter-repo`: another commit than the one pushed whenever the adopted branch is not the tracking branch. Once `origin` exists (#177) the printed push would have seeded the leg with the wrong line's history, where on `main` it failed loudly; and for a source with no branch of that name the hint still named a ref the clone does not have. The hint now has two fields where it had one: - `{local}` is the ref in `{work}` the refused push carried, and stands in the push command only. The scaffold's two pushes and the family holder's pass `args.tracking_branch`, which is what they push; `_extract_leg` and `_seed_leg` pass `HEAD`, which is what they push. - `{tracking}` is the remote's branch and stands in `--base` only, passed by every caller as before. Tests (`tests/test_ruleset_hint_tracking_branch.py`): - the pre-change literal gains a marker for the local ref, so the text for `main` is still pinned byte for byte, and a case pins `HEAD` with `trunk`; - the scan of `RULESET_HINT.format(` calls requires every field the template has, not only `tracking`; - the test hook records the commit it refuses in `refused`, and each of the five callers asserts that the ref its hint names is AT that commit, not only that a branch of the name exists; - an adoption's source is on `master` with a second line named like the tracking branch, so the extracted clone holds a `` that is another commit than the push, and the test asserts that too, so it cannot pass by a scenario in which `HEAD` and the tracking branch happen to agree; - a source with no branch of the tracking branch's name is adopted too: the clone has none, and the hint names `HEAD`. Unchanged: the text for the scaffold's two pushes and the family holder is byte-identical for any branch to what the first commit printed, and for the default branch to what `main` printed before this branch. An adoption's push line reads `HEAD:seed/scaffold` where it read `main:seed/scaffold` for the default branch; the hint's other lines and its two exits are untouched. Left out: `gh pr create --base ` cannot run in any of the five places, for `main` as for any other branch, because the repository the push was refused at is empty (`gh repo create` is called without a README), so the pull request has no base to land on; this is older than this branch and is named in the pull request. The printed adopt push still cannot run for want of `origin` (#177). Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Sonnet 5.5 --- adopt-project.py | 6 +- scaffold-project.py | 2 + scripts/family.py | 1 + scripts/shape_materialize.py | 22 ++- tests/test_ruleset_hint_tracking_branch.py | 198 ++++++++++++++++----- 5 files changed, 173 insertions(+), 56 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index 6b9ebdd2..eb9b1086 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -3082,7 +3082,8 @@ def _extract_leg(role: str, source: Source, work: Path, paths: list[str], except CommandFailed as exc: print(exc.loudly(f"pushing the {role} leg"), file=sys.stderr) print(RULESET_HINT.format(work=work, repo=repository, role=role, - tracking=tracking), file=sys.stderr) + local="HEAD", tracking=tracking), + file=sys.stderr) raise count = git_out(["rev-list", "--count", "HEAD"], cwd=work) print(f" {role:<5} {len(paths):>3} path(s) -> {head[:12]} " @@ -3130,7 +3131,8 @@ def _seed_leg(role: str, work: Path, values: dict, branch: str, url: str, except CommandFailed as exc: print(exc.loudly(f"pushing the seeded {role} leg"), file=sys.stderr) print(RULESET_HINT.format(work=work, repo=repository, role=role, - tracking=tracking), file=sys.stderr) + local="HEAD", tracking=tracking), + file=sys.stderr) raise print(f" {role:<5} 0 path(s) -> {commit[:12]} (SEEDED from " f"{SEED_TEMPLATE[role]}/) -> {url}") diff --git a/scaffold-project.py b/scaffold-project.py index 8707e855..ffbc1f9a 100755 --- a/scaffold-project.py +++ b/scaffold-project.py @@ -1045,6 +1045,7 @@ def _seed_legs(args, values: dict, names: dict, repositories: dict, print(exc.loudly(f"pushing the {role} leg"), file=sys.stderr) print(RULESET_HINT.format(work=work, repo=repositories[role], role=role, + local=args.tracking_branch, tracking=args.tracking_branch), file=sys.stderr) return None @@ -1163,6 +1164,7 @@ def _build_assembly_root(args, values: dict, names: dict, repositories: dict, print(RULESET_HINT.format(work=assembly, repo=repositories["assembly"], role="assembly", + local=args.tracking_branch, tracking=args.tracking_branch), file=sys.stderr) return False diff --git a/scripts/family.py b/scripts/family.py index 2256c7ca..99f80211 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -922,6 +922,7 @@ def _create_holder_worktree(args, work: Path, values: dict, shape_commit: str, print(exc.loudly("pushing the family holder"), file=sys.stderr) print(RULESET_HINT.format(work=work, repo=repository, role="family", + local=args.tracking_branch, tracking=args.tracking_branch), file=sys.stderr) return 2 diff --git a/scripts/shape_materialize.py b/scripts/shape_materialize.py index b54c6dba..7cd9d1ae 100644 --- a/scripts/shape_materialize.py +++ b/scripts/shape_materialize.py @@ -711,12 +711,20 @@ def loudly(self, what: str) -> str: ) -#: `{tracking}` is the run's tracking branch (`--tracking-branch`, the plan's -#: `tracking_branch`): the branch the refused push targeted, which is also the -#: branch a work clone is made on and the base a seeding pull request lands on -#: (#324). Every caller passes it, after `checked_value` has admitted it, so -#: it is a safe argument and is not quoted here. A call that leaves it out -#: fails `tests/test_ruleset_hint_tracking_branch.py`. +#: Two fields name a branch, and they are NOT the same thing (#324): +#: `{tracking}` is the run's tracking branch (`--tracking-branch`, the +#: plan's `tracking_branch`): the branch of the REMOTE the refused push +#: targeted, and so the base a seeding pull request lands on. +#: `{local}` is the ref in `{work}` that the refused push carried: the +#: tracking branch where the tool pushed it by name (the scaffold, the family +#: holder: their clones are made on it), `HEAD` where it pushed `HEAD:` +#: (both adopt legs). An extracted leg's clone holds the source's OWN branch +#: of the tracking branch's name, filtered, which is another commit than the +#: one pushed, so there `{tracking}` as the local ref would seed a leg with +#: the wrong history. +#: Every caller passes both, after `checked_value` has admitted the tracking +#: branch, so each is a safe argument and neither is quoted here. A call that +#: leaves one out fails `tests/test_ruleset_hint_tracking_branch.py`. RULESET_HINT = """ If the organisation applies a ruleset requiring changes to arrive by pull request, a direct push to the default branch is refused BY DESIGN and must not @@ -725,7 +733,7 @@ def loudly(self, what: str) -> str: (1) have an operator holding the bypass right seed the default branch once, then re-run this scaffold; or (2) push a seed BRANCH and open a pull request: - git -C {work} push -u origin {tracking}:seed/scaffold + git -C {work} push -u origin {local}:seed/scaffold gh pr create --repo {repo} --base {tracking} --head seed/scaffold \\ --title 'Seed the {role} leg' --body 'Scaffolded shape.' diff --git a/tests/test_ruleset_hint_tracking_branch.py b/tests/test_ruleset_hint_tracking_branch.py index bea3cdd7..d0ae3afa 100644 --- a/tests/test_ruleset_hint_tracking_branch.py +++ b/tests/test_ruleset_hint_tracking_branch.py @@ -10,12 +10,22 @@ main does not match any` and the second named a base the repository does not have. +TWO FIELDS NAME A BRANCH. `{tracking}` is the remote's branch, the base the pull +request lands on. `{local}` is the ref in the work clone that the refused push +CARRIED: the tracking branch where the tool pushed it by name (the scaffold, the +family holder), `HEAD` where it pushed `HEAD:` (both adopt legs). An extracted +leg's clone holds the source's own branch of the tracking branch's name, which +is another commit than the one pushed, so the tracking branch there would seed +a leg with the wrong history once `origin` exists. + WHAT IS PINNED HERE * the text itself: byte for byte what it was for `main` (the literal below is - the hint as it stood before this change, with the branch left as a marker), - and `trunk` in exactly the two places `main` stood, for `trunk`; - * every one of the five places that prints it passes the branch, run through - the real tools against local bare remotes that refuse like a ruleset; + the hint as it stood before this change, with the two branches left as + markers), and `trunk` in exactly the two places `main` stood, for `trunk`; + * every one of the five places that prints it passes both fields, run through + the real tools against local bare remotes that refuse like a ruleset, and + the ref the hint names as local is AT the commit the refused push carried: + the remote's own hook records it; * where the printed push is runnable (the scaffold's legs and root, a family holder), it is RUN, and the seed branch arrives. @@ -30,8 +40,8 @@ The extracted and the seeded leg of an ADOPTION print the hint too, and its `push -u origin` cannot run there: `git filter-repo` removed `origin` from the extracted clone, and the seeded clone never had one (#177, open). That is not -this change's to fix, so those two assert the text and that the branch the -text names is one the work clone holds. +this change's to fix, so those two assert the text and that the ref the text +names is the commit the refused push carried. """ from __future__ import annotations @@ -58,8 +68,8 @@ FAMILY = REPO / "scripts" / "family.py" #: The hint as it stood on `main` before #324, from its first line to its last, -#: with the four values it carries as markers. Raw, because the second command -#: ends in a backslash. It is a literal rather than a copy of the module's own +#: with the five values it carries as markers (the two branches were one `main` +#: then). Raw, because the second command ends in a backslash. It is a literal rather than a copy of the module's own #: constant so that the test cannot agree with whatever the constant becomes. HINT = r"""If the organisation applies a ruleset requiring changes to arrive by pull request, a direct push to the default branch is refused BY DESIGN and must not @@ -68,7 +78,7 @@ (1) have an operator holding the bypass right seed the default branch once, then re-run this scaffold; or (2) push a seed BRANCH and open a pull request: - git -C @WORK@ push -u origin @BRANCH@:seed/scaffold + git -C @WORK@ push -u origin @LOCAL@:seed/scaffold gh pr create --repo @REPO@ --base @BRANCH@ --head seed/scaffold \ --title 'Seed the @ROLE@ leg' --body 'Scaffolded shape.' @@ -76,23 +86,25 @@ by hand if you want a clean re-run.""" -def hint_for(work: str, repo: str, role: str, tracking: str) -> str: +def hint_for(work: str, repo: str, role: str, local: str, + tracking: str) -> str: return (HINT.replace("@WORK@", work).replace("@REPO@", repo) - .replace("@ROLE@", role).replace("@BRANCH@", tracking)) + .replace("@ROLE@", role).replace("@LOCAL@", local) + .replace("@BRANCH@", tracking)) # --- the text --------------------------------------------------------------- def test_the_hint_for_main_is_the_text_it_always_was(): printed = RULESET_HINT.format(work="W", repo="o/r", role="spec", - tracking="main") - assert printed == "\n" + hint_for("W", "o/r", "spec", "main") + "\n" + local="main", tracking="main") + assert printed == "\n" + hint_for("W", "o/r", "spec", "main", "main") + "\n" def test_the_hint_for_another_branch_changes_that_branch_and_nothing_else(): printed = RULESET_HINT.format(work="W", repo="o/r", role="spec", - tracking="trunk") - assert printed == "\n" + hint_for("W", "o/r", "spec", "trunk") + "\n" + local="trunk", tracking="trunk") + assert printed == "\n" + hint_for("W", "o/r", "spec", "trunk", "trunk") + "\n" assert "main" not in printed, "no `main` is left in the hint" assert printed.count("trunk") == 2, "the local branch and `--base`" # The exits are the ones AGENTS.md allows, and `--admin` is not one. @@ -100,10 +112,21 @@ def test_the_hint_for_another_branch_changes_that_branch_and_nothing_else(): assert "bypass right" in printed and "pull request" in printed -def test_the_hint_takes_four_fields_and_the_branch_is_one_of_them(): +def test_the_local_ref_and_the_base_are_two_fields(): + """An adoption names `HEAD` where it pushed `HEAD:`, and still lands its + pull request on the tracking branch.""" + printed = RULESET_HINT.format(work="W", repo="o/r", role="spec", + local="HEAD", tracking="trunk") + assert printed == "\n" + hint_for("W", "o/r", "spec", "HEAD", "trunk") + "\n" + assert "origin HEAD:seed/scaffold" in printed + assert "--base trunk --head seed/scaffold" in printed + assert "main" not in printed and printed.count("trunk") == 1 + + +def test_the_hint_takes_five_fields_two_of_them_a_branch(): fields = {name for _, name, _, _ in string.Formatter().parse(RULESET_HINT) if name} - assert fields == {"work", "repo", "role", "tracking"} + assert fields == {"work", "repo", "role", "local", "tracking"} # --- every caller passes it -------------------------------------------------- @@ -125,17 +148,19 @@ def hint_calls() -> list[tuple[str, int, set]]: return found -def test_no_call_that_prints_the_hint_leaves_the_branch_out(): +def test_no_call_that_prints_the_hint_leaves_a_branch_out(): + fields = {name for _, name, _, _ in string.Formatter().parse(RULESET_HINT) + if name} calls = hint_calls() assert {"scaffold-project.py", "adopt-project.py", "scripts/family.py"} <= {path for path, _, _ in calls}, ( "the scan no longer finds the tools that print the hint, so it is not " "reading what it claims to") offenders = [f"{path}:{line} names only {sorted(named)}" - for path, line, named in calls if "tracking" not in named] + for path, line, named in calls if not fields <= named] assert not offenders, ( - "`RULESET_HINT.format` without `tracking=` raises KeyError inside the " - "failure path that exists to explain a refusal (#324):\n " + "`RULESET_HINT.format` without `local=` or `tracking=` raises KeyError " + "inside the failure path that exists to explain a refusal (#324):\n " + "\n ".join(offenders)) @@ -143,7 +168,8 @@ def test_no_call_that_prints_the_hint_leaves_the_branch_out(): HOOK = (b"#!/bin/sh\n" b"# A ruleset: while RULESET_REFUSES names the repository this runs in,\n" - b"# a push to anything but a seed/* branch is refused.\n" + b"# a push to anything but a seed/* branch is refused, and what it\n" + b"# refused is written to `refused` in the repository, as ` `.\n" b'here=$(basename "$(pwd)")\n' b'case " $RULESET_REFUSES " in\n' b' *" $here "*) ;;\n' @@ -154,6 +180,7 @@ def test_no_call_that_prints_the_hint_leaves_the_branch_out(): b" refs/heads/seed/*) ;;\n" b' *) echo "remote: error: GH013: Repository rule violations found ' b'for $ref: changes must be made through a pull request." >&2\n' + b' echo "$new $ref" >> refused\n' b" exit 1 ;;\n" b" esac\n" b"done\n" @@ -226,10 +253,33 @@ def probe(*args, cwd=root): NEEDS_FILTER_REPO = ("adopt-extracted", "adopt-seeded") -def refused_run(case: str, tracking: str, tmp_path, template): +def source_with_a_second_line(path: Path, tracking: str) -> Path: + """A source whose default branch is `master`, whatever the tracking branch + is, and which ALSO has a branch of the tracking branch's name: another + line, one commit behind `master` and with a commit and a spec file of its + own. + That is what a source adopted into a tracking branch of another name + looks like, and it is where the extracted leg's clone holds a branch of + the tracking branch's name that is not the commit the push carried.""" + source = make_source_repo(path, SPEC_ONLY_TREE, branch="master", + edits=EDITS) + git("checkout", "-q", "-b", tracking, "HEAD~1", cwd=source) + # A spec path, so the commit survives the extraction of the spec leg. + (source / "specs" / "001-render" / "second-line.md").write_text( + "only on this line\n", encoding="utf-8") + git("add", "specs/001-render/second-line.md", cwd=source) + git("commit", "-q", "-m", "A second line", cwd=source) + git("checkout", "-q", "master", cwd=source) + return source + + +def refused_run(case: str, tracking: str, tmp_path, template, + second_line: bool = True): """Run the tool that prints `case`'s hint against remotes that refuse its push, with `--tracking-branch `; returns the process, the - directory the remotes are in and the work directory.""" + directory the remotes are in and the work directory. An adoption's source + is on `master`, with or without a second line of the tracking branch's + name.""" refused = CASES[case][0] env = refusing(template, refused) remotes, work = tmp_path / "remotes", tmp_path / "work" @@ -245,10 +295,11 @@ def refused_run(case: str, tracking: str, tmp_path, template): "--created-by", "Test Human", "--created-on", "2026-09-04", "--tracking-branch", tracking, *where, env=env) else: - # The source's own default branch IS the tracking branch, as it is - # wherever the plan's pull request has a base to land on. - source = make_source_repo(tmp_path / "IRRS", SPEC_ONLY_TREE, - branch=tracking, edits=EDITS) + if second_line: + source = source_with_a_second_line(tmp_path / "IRRS", tracking) + else: + source = make_source_repo(tmp_path / "IRRS", SPEC_ONLY_TREE, + branch="master", edits=EDITS) plan = tmp_path / "adoption-plan.yaml" written = write_plan(source, plan, project="Redwood", extra=("--tracking-branch", tracking, @@ -261,9 +312,15 @@ def refused_run(case: str, tracking: str, tmp_path, template): return result, remotes, work +def local_ref(case: str, tracking: str) -> str: + """The ref each case's hint names: where the tool pushed the tracking + branch by name, that branch; where it pushed `HEAD:`, `HEAD`.""" + return "HEAD" if case in NEEDS_FILTER_REPO else tracking + + def printed_hint(result, case: str, tracking: str) -> tuple[str, str]: """`(the hint block, the work clone it names)`, after checking the block - is the whole hint with this case's role and repository.""" + is the whole hint, with this case's role, repository and local ref.""" assert result.returncode == 2, result.stderr + result.stdout start = result.stderr.index("If the organisation applies a ruleset") block = result.stderr[start:] @@ -271,11 +328,27 @@ def printed_hint(result, case: str, tracking: str) -> tuple[str, str]: block, re.M) assert named, block _, role, repository = CASES[case] - assert block.startswith(hint_for(named[1], repository, role, tracking) + assert block.startswith(hint_for(named[1], repository, role, + local_ref(case, tracking), tracking) + "\n"), block return block, named[1] +def refused_commit(bare: Path, tracking: str) -> str: + """The commit the refused push carried, as the remote's own hook saw it: + exactly one push was refused, and it was to the tracking branch.""" + lines = (bare / "refused").read_text(encoding="utf-8").splitlines() + assert len(lines) == 1, lines + commit, ref = lines[0].split() + assert ref == f"refs/heads/{tracking}", lines + return commit + + +def commit_of(ref: str, work: str) -> str: + return git("rev-parse", "--verify", "-q", f"{ref}^{{commit}}", + cwd=Path(work), check=False).stdout.strip() + + @pytest.mark.parametrize("tracking", ["trunk", "main"]) @pytest.mark.parametrize("case", CASES) def test_the_hint_names_the_tracking_branch_the_run_was_made_with( @@ -285,34 +358,65 @@ def test_the_hint_names_the_tracking_branch_the_run_was_made_with( "`pip install git-filter-repo`") result, remotes, _ = refused_run(case, tracking, tmp_path, template) block, work = printed_hint(result, case, tracking) - assert f"origin {tracking}:seed/scaffold" in block + local = local_ref(case, tracking) + assert f"origin {local}:seed/scaffold" in block assert f"--base {tracking} --head seed/scaffold" in block if tracking != "main": # Not the work path: a directory may be called anything. assert "main" not in block.replace(work, ""), ( "the default branch is not this run's") - # The branch the hint names as the LOCAL one is one the clone holds. - held = git("rev-parse", "--verify", "-q", f"refs/heads/{tracking}", - cwd=Path(work), check=False) - assert held.returncode == 0, ( - f"the work clone has no branch {tracking}: " - + git("branch", "-a", cwd=Path(work)).stdout) - - # The refusal was real: nothing reached the tracking branch. + # The refusal was real: nothing reached the tracking branch, and the one + # push the remote refused is the one the hint is about. bare = remotes / (CASES[case][0]) assert git("rev-parse", "--verify", "-q", f"refs/heads/{tracking}", cwd=bare, check=False).returncode != 0 + pushed = refused_commit(bare, tracking) + + # THE LOCAL REF THE HINT NAMES IS AT THE COMMIT THE REFUSED PUSH CARRIED, + # which is what advising it to be pushed to `seed/scaffold` means. + assert commit_of(local, work) == pushed, ( + f"`{local}` in the work clone is not what was pushed: " + + git("branch", "-a", "-v", cwd=Path(work)).stdout) + if case == "adopt-extracted": + # The scenario is the one that bites: the clone also holds the + # source's own ``, another commit than the one pushed. + assert commit_of(tracking, work) not in ("", pushed), ( + "the source's second line is not in the extracted clone, so this " + "run cannot tell `HEAD` from the tracking branch") if case in NEEDS_FILTER_REPO: return # Where the printed command can run, run it: the seed branch arrives, at # the commit the work clone holds. - pushed = subprocess.run(["git", "-C", work, "push", "-u", "origin", - f"{tracking}:seed/scaffold"], - capture_output=True, text=True, check=False, - env={**os.environ, **refusing(template, - CASES[case][0])}) - assert pushed.returncode == 0, pushed.stderr - assert git("rev-parse", "refs/heads/seed/scaffold", cwd=bare).stdout == \ - git("rev-parse", f"refs/heads/{tracking}", cwd=Path(work)).stdout + ran = subprocess.run(["git", "-C", work, "push", "-u", "origin", + f"{local}:seed/scaffold"], + capture_output=True, text=True, check=False, + env={**os.environ, **refusing(template, + CASES[case][0])}) + assert ran.returncode == 0, ran.stderr + assert git("rev-parse", "refs/heads/seed/scaffold", + cwd=bare).stdout.strip() == pushed + + +@pytest.mark.parametrize("tracking", ["trunk", "main"]) +def test_an_extracted_leg_names_what_it_pushed_when_the_source_has_no_such_branch( + tracking, tmp_path, template): + """A source on `master` adopted into `--tracking-branch ` has no + branch of that name for its clone to hold: the clone has `master` and the + adopt branch only. The hint must still name a ref the clone has, and it is + `HEAD`, the commit the refused push carried.""" + if shutil.which("git-filter-repo") is None: + pytest.skip("git filter-repo is not installed: " + "`pip install git-filter-repo`") + result, remotes, _ = refused_run("adopt-extracted", tracking, tmp_path, + template, second_line=False) + block, work = printed_hint(result, "adopt-extracted", tracking) + assert "origin HEAD:seed/scaffold" in block + assert f"--base {tracking} --head seed/scaffold" in block + pushed = refused_commit(remotes / CASES["adopt-extracted"][0], tracking) + assert commit_of("HEAD", work) == pushed + assert commit_of(tracking, work) == "", ( + "the clone has a branch of the tracking branch's name after all, so " + "this run does not show what it is meant to: " + + git("branch", "-a", cwd=Path(work)).stdout) From 741cbf2a34f68c5dd560c9c3404075f6c957ce7f Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 15:50:59 +0000 Subject: [PATCH 3/3] tests/test_ruleset_hint_tracking_branch.py: two `assert a and b` lines are two asserts each, so a failure names the half that failed (SonarCloud python:S9073) (#324) The text test for another branch asserted `"bypass right" in printed and "pull request" in printed` and the test for the two fields asserted `"main" not in printed and printed.count("trunk") == 1`. Each is split into one assert per condition, in the same order; neither had a message, so no failure message changes, and nothing else in the file does. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Sonnet 5.5 --- tests/test_ruleset_hint_tracking_branch.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_ruleset_hint_tracking_branch.py b/tests/test_ruleset_hint_tracking_branch.py index d0ae3afa..7d4c84a1 100644 --- a/tests/test_ruleset_hint_tracking_branch.py +++ b/tests/test_ruleset_hint_tracking_branch.py @@ -109,7 +109,8 @@ def test_the_hint_for_another_branch_changes_that_branch_and_nothing_else(): assert printed.count("trunk") == 2, "the local branch and `--base`" # The exits are the ones AGENTS.md allows, and `--admin` is not one. assert "--admin" not in printed - assert "bypass right" in printed and "pull request" in printed + assert "bypass right" in printed + assert "pull request" in printed def test_the_local_ref_and_the_base_are_two_fields(): @@ -120,7 +121,8 @@ def test_the_local_ref_and_the_base_are_two_fields(): assert printed == "\n" + hint_for("W", "o/r", "spec", "HEAD", "trunk") + "\n" assert "origin HEAD:seed/scaffold" in printed assert "--base trunk --head seed/scaffold" in printed - assert "main" not in printed and printed.count("trunk") == 1 + assert "main" not in printed + assert printed.count("trunk") == 1 def test_the_hint_takes_five_fields_two_of_them_a_branch():