From ac91a0b41039a1a65a8e84671cece4449d41bfb4 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Mon, 12 Oct 2026 22:26:24 +0000 Subject: [PATCH 1/5] adopt-project.py: `plan` takes the tracking branch from the source's default branch, `execute` sets the topic before the pull request and says what is left in one notice, and NEXT names `python3 scripts/bootstrap.py` where the shape's Makefile went aside, so a `master` source's pull request is no longer refused after both legs are pushed, a refused one no longer leaves the topic unset, and NEXT no longer names a make target the root lacks (#479, #480, #481) `tracking_branch` is the branch both legs are pushed to, the branch the root's `project.yaml` tracks, and the base `execute` opens the split's pull request against. `plan` defaulted it to `main` whatever the source used, so a `master` or `trunk` source was adopted all the way and then `gh pr create --base main` was refused (`Base ref must be a branch`), with the same command printed as the way out (#479). A refused pull request also ended the run before the `xf-project-` topic was tried, and the way out never said so (#481). And a source with its own Makefile keeps it, the shape's going to `shape/Makefile`, while NEXT said `make bootstrap`, which exits 2 there (#480). - `plan --tracking-branch` defaults to the branch `plan` reads the source at (`source.default_branch:`); an explicit flag still wins. A defaulted branch `checked_value` refuses is named as the default, not as a flag. - `check` finds, and `execute` refuses before any leg exists, a tracking branch that is no branch of the source: `plan-tracking-branch-missing`, in the one list `_entry_findings` composes. The branches are the source's remote `origin`'s once it has fetched any (the repository the pull request goes to), and its own otherwise; nothing is asked of the network. `plan` prints the same sentence as a NOTE for the value it records. - The plan's last follow-up names the tracking branch it opens the pull request against, where it said "the source's default branch". - `execute` sets the topic on each of the three repositories before the pull request, asking each whatever the one before it answered, and prints `set on all three` or the ones it set. - What a refused pull request and an unset topic leave is ONE notice after the table, only once the split verified: `The branch IS pushed.`, a `gh repo edit ... --add-topic ...` for each repository left without the topic, then the pull request's command, still last; with the pull request open, `The split IS pushed and the pull request IS open. Set the topic by hand:` and only the repositories left. A failed verification offers no topic command either. - `_commit_the_split` returns the materializer's collisions, and where the shape's Makefile went to `shape/Makefile`, NEXT names `python3 scripts/bootstrap.py` with a NOTE saying why. Elsewhere NEXT is byte for byte what it was. - `collision_follow_up` (scripts/shape_materialize.py) says of the Makefile that the shape's make targets (`bootstrap`, `validate`, `pins`, `park`, `resume`) arrive only with that merge, and what runs meanwhile. - `plan --help` says what `--tracking-branch` is and its default; `docs/cli.md` is regenerated. Unchanged: `execute`'s exit codes, `_verify`, the leg pushes and `ADOPT_RULESET_HINT` (both already took the tracking branch), the pull-request-only posture, every message when nothing is refused (NEXT compared byte for byte by a test), and AGENTS.md (at its cap; the tool's own output carries the change). Left out: a source that keeps a lowercase `makefile` or a `GNUmakefile`, which GNU make reads before the shape's `Makefile` on a case-sensitive disk; and a `--tracking-branch` that names a branch other than the one the split is made on, whose pull request then carries that branch's difference too. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 420 +++++++++++++++++++++------- docs/cli.md | 6 + scripts/shape_materialize.py | 17 +- tests/test_adopt_e2e.py | 72 +++++ tests/test_adopt_github_exits.py | 238 ++++++++++++++-- tests/test_adopt_late_failure.py | 8 +- tests/test_adopt_tracking_branch.py | 193 +++++++++++++ 7 files changed, 817 insertions(+), 137 deletions(-) create mode 100644 tests/test_adopt_tracking_branch.py diff --git a/adopt-project.py b/adopt-project.py index 6b54ebae..ae4df088 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -19,10 +19,12 @@ plan walks the source at its default branch, classifies every top-level path against `contracts/path-classification.yaml`, and writes - `adoption-plan.yaml`. Paths it cannot honestly call, every path - holding one of the source's own submodules, and every path - `execute` will not extract into a leg as it is named, carry - `leg: null`, `review_required: true` and the QUESTION to ask. + `adoption-plan.yaml`, with that branch as the tracking branch + unless `--tracking-branch` names another (#479). Paths it cannot + honestly call, every path holding one of the source's own + submodules, and every path `execute` will not extract into a leg + as it is named, carry `leg: null`, `review_required: true` and + the QUESTION to ask. A leg path over a path it keeps in the root itself is a NOTE, not a refusal: the answer is the person's to change. check validates a plan against the source: each entry path bound for @@ -40,13 +42,16 @@ any case), and the source's own submodules kept on one leg with their `.gitmodules`, registered there and never in the root, each relative url naming the same repository from the leg as from the - source. It prints what will happen and changes nothing. + source, and the tracking branch a branch of the source, which the + pull request is opened against. It prints what will happen and + changes nothing. execute creates the two legs, extracts them with `git filter-repo`, makes the one split commit on a branch of the source, sets the - `xf-project-` topic on all three (skipped for local remotes), - and then VERIFIES by blob sha that every source path landed in - exactly one place, and that the root's only submodules are the - two leg mounts, each at the commit pushed to its leg. + `xf-project-` topic on all three and then opens the split's + pull request against the tracking branch (both skipped for local + remotes), and then VERIFIES by blob sha that every source path + landed in exactly one place, and that the root's only submodules + are the two leg mounts, each at the commit pushed to its leg. THE PLAN IS AN ARTIFACT A HUMAN OR AN AI EDITS. That is why it is YAML with reasons in it rather than a pipe between two processes: the classifier is @@ -96,12 +101,14 @@ `execute` has other exit 2s, once the leg repositories exist (a refused push, a refused mount). Two of them are GitHub mode's alone, and both come AFTER both legs and the split branch are pushed and the verification has passed: `gh pr -create` refused (`The branch IS pushed`, printed after the table) and the -topic would not set (`The split IS pushed and the pull request IS open`). Each -prints the command that finishes the job by hand, and that command is the way -out: a re-run into fresh legs abandons the pushed pair for a second. A -verification that fails outranks both, with exit 1 and no command to open the -pull request. +create` refused (`The branch IS pushed`) and the topic would not set (`The +split IS pushed and the pull request IS open`, or beside a refused pull +request, under `The branch IS pushed`). The topic is set first, so a refused +pull request no longer leaves it unset (#481), and both are said in ONE notice +after the table, each with the commands that finish the job by hand; those +commands are the way out: a re-run into fresh legs abandons the pushed pair +for a second. A verification that fails outranks both, with exit 1 and no +command to open the pull request or to set the topic. """ from __future__ import annotations @@ -1341,12 +1348,17 @@ def _search_by_hand(named: str, why: str) -> str: def follow_ups_for(source: "Source", entries: list["Entry"], names: dict, spec_path: str, code_path: str, - collisions: list[str]) -> list[str]: + collisions: list[str], tracking: str) -> list[str]: """The code changes an in-place split MAKES NECESSARY, named in the plan. A split that moved `contracts/` into the spec leg and said nothing about the harness that reads it would have handed somebody a red build and no explanation. + + The last one names `tracking`, the base `execute` opens the pull request + against (#479). It said "the source's default branch" while `execute` + used `--base main` whatever that branch was; the two now agree by + default, and a `--tracking-branch` that names another is named here. """ out = cross_leg_references(source, entries, spec_path, code_path) if any(e.leg == "spec" and (e.path == "contracts/" @@ -1363,10 +1375,10 @@ def follow_ups_for(source: "Source", entries: list["Entry"], names: dict, "owner rule cannot cross a repository boundary") out.extend(collisions) out.append( - "open the split as a pull request against the source's default " - f"branch; the legs {names['spec']} and {names['code']} are seeded " - "before it, so review the root's diff and the two new repositories " - "together") + f"open the split as a pull request against {tracking}, the branch the " + f"assembly root and both legs track; the legs {names['spec']} and " + f"{names['code']} are seeded before it, so review the root's diff and " + "the two new repositories together") return out @@ -1743,6 +1755,116 @@ def _kept_collision_notes(args, entries: list, tree: list) -> list[str]: return notes +# --------------------------------------------------------------------------- +# The tracking branch is a branch of the source (#479) +# --------------------------------------------------------------------------- +# +# `tracking_branch` is three things at once: the branch both legs are pushed +# to, the branch the assembly root's `project.yaml` says the project tracks +# (its `scripts/bootstrap.py` places each leg on it), and the base `execute` +# opens the split's pull request against. It defaulted to `main` whatever the +# source used, so a `master` or `trunk` source was adopted all the way, both +# legs created and pushed and the split pushed and verified, and then `gh pr +# create --base main` was refused (`Base ref must be a branch`), and the way +# out it printed was the same command. +# +# So `plan` records the branch it reads the source at, its default branch +# (`source.default_branch:`), unless `--tracking-branch` names another; and +# `check` finds, and `execute` refuses before any leg exists, a tracking +# branch that is no branch of the source, which an explicit flag or a hand +# edit can still name. `plan` notes it of the value it records. +# +# WHICH BRANCHES. The pull request goes to the repository the source is a +# clone of, and the source knows that repository's branches as its remote +# `origin`'s: `gh repo clone` and `git clone` fetch every branch. A source +# that has fetched none, a repository with no remote, is asked about its own +# branches; `--local-remote-dir` pushes the split back into it. Nothing is +# asked of the network, and a branch made on GitHub after the clone is +# fetched first, as the remediation says. + +#: `check`'s finding for a tracking branch the source does not have, and +#: what it prints once after it (#479). +TRACKING_BRANCH_MISSING = "plan-tracking-branch-missing" +TRACKING_BRANCH_REMEDIATION = ( + "Remediation: set `tracking_branch:` in the plan to a branch the source " + "has: its default branch, as `source.default_branch:` records it, unless " + "the project is to track another. Or re-run `plan` without " + "`--tracking-branch`, which records that default branch. A branch that " + "is on GitHub and not yet in the source's clone is fetched into it first " + "(`git fetch origin`), and then `check` is run again.") +#: Where `_branches_of` reads the branches of a source that has fetched any. +ORIGIN_BRANCHES = "refs/remotes/origin/" +#: How many branches a finding names; past it, the rest are counted. +BRANCHES_NAMED = 8 + + +def _branches_of(source: Source) -> tuple[str, list[str]]: + """`(whose, branches)`: the branches of the repository the split's pull + request goes to, as the source knows them, and how a finding names + where they were read: its remote `origin`'s when it has fetched any, + and otherwise its own. Each name is read as `Source.tree` reads a path, + so a byte that is not UTF-8 is kept rather than refused.""" + raw = git_out(["for-each-ref", "--format=%(refname)", "refs/heads/", + ORIGIN_BRANCHES], cwd=source.path, binary=True) + refs = [_git_path(line) for line in raw.splitlines()] + fetched = sorted(ref[len(ORIGIN_BRANCHES):] for ref in refs + if ref.startswith(ORIGIN_BRANCHES) + and ref != f"{ORIGIN_BRANCHES}HEAD") + if fetched: + return "the source's remote `origin`", fetched + return "the source", sorted(ref[len("refs/heads/"):] for ref in refs + if ref.startswith("refs/heads/")) + + +def _tracking_branch_problem(source: Source, what: str, + tracking: str) -> str | None: + """Why `tracking`, named as `what`, cannot be this adoption's tracking + branch, as a sentence; None when it is a branch of the source as + `_branches_of` reads them. ONE TEST, asked by `plan` as a NOTE and by + `check` and `execute` as `plan-tracking-branch-missing`.""" + whose, branches = _branches_of(source) + if tracking in branches: + return None + named = _listed(branches[:BRANCHES_NAMED]) or "none" + if len(branches) > BRANCHES_NAMED: + named += f" and {len(branches) - BRANCHES_NAMED} more" + return (f"{what} is {_spelled(tracking)}, which is no branch of {whose} " + f"(its branches: {named}; the source's default branch is " + f"{_spelled(source.branch)}). `execute` pushes both legs to it, " + "writes it into the root's `project.yaml` as the branch the " + "project tracks, and opens the split's pull request against it " + "once both legs and the split are pushed, where GitHub refuses a " + "base that is not a branch (`Base ref must be a branch`)") + + +def _tracking_branch_findings(plan: Plan, source: Source + ) -> tuple[list[str], list[str]]: + """`_tracking_branch_problem` of the plan's tracking branch, as `check` + prints it, and what it prints once after it. The value is the one + `execute` takes (`_checked_plan_values`): `main` when the plan has no + `tracking_branch:`, and as `str` makes it.""" + problem = _tracking_branch_problem( + source, "`tracking_branch`", str(plan.get("tracking_branch", "main"))) + if problem is None: + return [], [] + return ([f"FINDING {TRACKING_BRANCH_MISSING}: {problem}"], + [TRACKING_BRANCH_REMEDIATION]) + + +def _tracking_branch_notes(args, source: Source) -> list[str]: + """What `plan` says when the tracking branch it records is one `check` + will find: a NOTE, in the words `check` will use, and a line saying what + follows. Nothing is refused: the plan is the person's to edit.""" + problem = _tracking_branch_problem(source, "the tracking branch", + args.tracking_branch) + if problem is None: + return [] + return [f"\nNOTE {problem}.", + f" `check` reports this as `{TRACKING_BRANCH_MISSING}` while " + "`tracking_branch:` names it; `plan` refuses nothing for it, " + "because the plan is yours to edit."] + + def _checked_plan_inputs(args, source: Source, naming: NamingPolicy) -> tuple[dict, list]: """Every value the plan will RECORD, defaulted and checked before one of @@ -1754,8 +1876,15 @@ def _checked_plan_inputs(args, source: Source, wrong place for the line that was wrong. """ args.project = checked_value("--project", args.project) - args.tracking_branch = checked_value("--tracking-branch", - args.tracking_branch) + # THE SOURCE'S OWN BRANCH, UNLESS ONE IS NAMED (#479): the branch `plan` + # reads the source at is the one its pull request can be opened against. + # A default this tool filled in is named by where it came from, as + # `--elected-by` is below, and not as a flag the person never passed. + given = args.tracking_branch is not None + args.tracking_branch = checked_value( + "--tracking-branch" if given else + "--tracking-branch (not given, so the source's default branch)", + args.tracking_branch if given else source.branch) # The ONE question first -- spelling, Git's and Windows's names, then the # alphabet `checked_value` allows -- so `spec/` is refused here with the # spelling to retype, before a plan that `check` and `execute` would have @@ -2072,6 +2201,8 @@ def _print_plan_report(args, source: Source, tree: list, entries: list, print(line) for line in _kept_collision_notes(args, entries, tree): print(line) + for line in _tracking_branch_notes(args, source): + print(line) unresolved = [e for e in entries if e.leg is None] print(f"\nplan written to {out}") if unresolved: @@ -2112,7 +2243,8 @@ def cmd_plan(args) -> int: implied = ask_about_refused_paths(ruled, tree) materialized = _predict_collisions(implied) follow_ups = follow_ups_for(source, implied, names, args.spec_path, - args.code_path, materialized) + args.code_path, materialized, + args.tracking_branch) seeded = seeded_legs(assigned_paths(implied)) entries = ask_about_refused_paths( ask_about_submodules(policy, ruled, tree), tree) @@ -3444,8 +3576,9 @@ def _entry_findings(plan: Plan, source: Source, source's own submodules, then each value `execute` writes into a file that no file can hold (#255), then what the project's own `validate-manifest.py` would refuse of the values `execute` writes into - its manifest (#261), and last each relative url of a kept submodule - that names another repository from its leg (#190). + its manifest (#261), then a tracking branch that is no branch of the + source (#479), and last each relative url of a kept submodule that + names another repository from its leg (#190). ONE LIST, read by `check` and by `execute`'s `_refuse_what_check_finds`. #168 and #166 each added findings to `check` and a refusal to `execute` @@ -3488,9 +3621,13 @@ def _entry_findings(plan: Plan, source: Source, _submodule_findings(plan, source, tree, local_remote_dir) unwritable_findings, unwritable_remediation = _value_findings(plan) value_findings, value_remediation = _manifest_findings(plan) - return (findings + split + unwritable_findings + value_findings + moved, + tracking_findings, tracking_remediation = _tracking_branch_findings( + plan, source) + return (findings + split + unwritable_findings + value_findings + + tracking_findings + moved, unsafe_remediation + split_remediation + unwritable_remediation - + value_remediation + moved_remediation, notes) + + value_remediation + tracking_remediation + moved_remediation, + notes) def _topics_line(topic: str, local: bool) -> str: @@ -4740,7 +4877,10 @@ def _pull_request_command(repositories: dict, names: dict, tracking: str, branch: str, message: str) -> list[str]: """The `gh pr create` that opens the split's pull request. It carries its own title and body, so it runs with no terminal to prompt on: this is what - `_open_the_pull_request` runs, and what `_open_it_by_hand` prints (#304).""" + `_open_the_pull_request` runs, and what `_finish_by_hand` prints (#304). + Its base is the tracking branch, which `plan` takes from the source's + default branch unless told another, and which `check` finds when it is + no branch of the source (#479).""" return ["gh", "pr", "create", "--repo", repositories["assembly"], "--base", tracking, "--head", branch, "--title", f"Adopt the three-repository shape: " @@ -4750,13 +4890,13 @@ def _pull_request_command(repositories: dict, names: dict, tracking: str, def _open_the_pull_request(repositories: dict, names: dict, tracking: str, branch: str, message: str) -> bool: - """(c′) The pull request, because these organisations are pull-request - only. + """(c″) The pull request, because these organisations are pull-request + only. It comes after the topic (`_set_the_topic`, #481). Returns False when `gh` refused, or could not be run at all (any of `LATE_FAILURES`, #188), having printed why. The branch IS pushed by the time this runs, so the exit is never "start again". The command - that finishes the job by hand is `_open_it_by_hand`'s, which the caller + that finishes the job by hand is `_finish_by_hand`'s, which the caller prints AFTER the verification and only once it has passed (#304): until then nothing says the split is the one to open a pull request for. Split out of `cmd_execute` for #138. @@ -4773,67 +4913,102 @@ def _open_the_pull_request(repositories: dict, names: dict, tracking: str, return True -def _open_it_by_hand(repositories: dict, names: dict, tracking: str, - branch: str, message: str, passed: bool) -> None: - """What a refused `gh pr create` leaves to do, said once `execute` knows - whether the split it pushed is the one the plan asked for (#304). - - A split that PASSED verification gets the command that opens its pull - request, the same command `_open_the_pull_request` ran, with its title and - body, so that it also runs where there is no terminal to prompt on. One - that did not gets the opposite: the refusal printed above names the exact - command that was tried, and a person who sees only that, with findings - under it, may paste it. Stdout is flushed first. The table is on it and a - pipe holds it in a block buffer, so without the flush this would come - before the table it depends on in a merged log; a stdout that is closed, - `| head` and gone, must not take the command down with it (`_flush_stdout`). +def _finish_by_hand(passed: bool, refused: list[str] | None, topic: str, + unset: list[str]) -> None: + """What a refused `gh pr create` and a topic that would not set leave to + do, said ONCE, after the table, when `execute` knows whether the split it + pushed is the one the plan asked for (#304, #481). + + `refused` is the `gh pr create` that `gh` refused, as + `_pull_request_command` built it, and None when the pull request is open; + `unset` names each repository `_set_the_topic` could not give `topic`. + + A split that PASSED verification gets the commands that finish the job: + a `gh repo edit` for each repository left without the topic, then the + command that opens the pull request, the one `_open_the_pull_request` + ran, with its title and body, so that it also runs where there is no + terminal to prompt on. That command is the LAST thing said, as it was + before the topic came first. With the pull request open, the topic's + commands are said under `The split IS pushed and the pull request IS + open`, as they always were; beside a refused one, under `The branch IS + pushed`, ahead of it. A refused pull request used to stop the run before + the topic was tried, and its way out never said so (#481). + + One that did not pass gets the opposite: the refusal printed above names + the exact command that was tried, and a person who sees only that, with + findings under it, may paste it. No topic is offered there either: the + way out is the verification's, and the re-run it names sets the topic. + Stdout is flushed first. The table is on it and a pipe holds it in a + block buffer, so without the flush this would come before the table it + depends on in a merged log; a stdout that is closed, `| head` and gone, + must not take the commands down with it (`_flush_stdout`). """ _flush_stdout() if not passed: - print("No pull request was opened, and none should be: the split " - "above did not pass verification.", file=sys.stderr) + if refused is not None: + print("No pull request was opened, and none should be: the split " + "above did not pass verification.", file=sys.stderr) + return + topics = "".join(f"\n gh repo edit {repository} --add-topic {topic}" + for repository in unset) + if refused is None: + if unset: + print("The split IS pushed and the pull request IS open. Set the " + f"topic by hand:{topics}", file=sys.stderr) return - command = " ".join(shlex.quote(argument) for argument in - _pull_request_command(repositories, names, tracking, - branch, message)) - print(f"The branch IS pushed. Open the pull request by hand:\n " - f"{command}", file=sys.stderr) + command = " ".join(shlex.quote(argument) for argument in refused) + print("The branch IS pushed. " + + (f"Set the topic by hand:{topics}\n" if unset else "") + + f"Open the pull request by hand:\n {command}", file=sys.stderr) -def _set_the_topic(repositories: dict, topic: str) -> bool: - """(c″) The topic, on all three. +#: The repositories `execute` sets the project topic on, in its order. +TOPIC_ROLES = ("assembly", "spec", "code") + + +def _set_the_topic(repositories: dict, topic: str) -> list[str]: + """(c′) The topic, on all three, BEFORE the pull request (#481): returns + the repositories that carry it now, in `TOPIC_ROLES`' order. The assembly root pre-existed and is still a repository OF THIS PROJECT: `project.yaml` claims `topic: ` either way, and a claim the - organisation cannot see is the defect being fixed here. + organisation cannot see is the defect being fixed here. Neither the + topic nor the pull request needs the other, so the topic comes first: a + refused pull request used to end the run before it was tried, and left + all three repositories without it. + + EACH REPOSITORY IS ASKED, whatever the one before it answered: a token + refused on one may be allowed on the next, and `_finish_by_hand` names + the exact command for each one left without the topic, and for none that + has it. Each refusal is printed as it happens; the commands are said + after the table. A TOPIC THAT WILL NOT SET DOES NOT SUPPRESS THE VERIFICATION TABLE. The - split is pushed and the pull request is open by now, and the blob-sha - accounting for every source path is the report a human is told to read - back (AGENTS.md step 7); losing it to a permission or a rate limit would - be the more expensive failure. So this RETURNS False rather than exiting: - the caller reports it after the table, with the commands to finish by - hand, and still exits non-zero. That holds for a `gh` that could not be - run at all as much as for one that refused (`LATE_FAILURES`, #188). - Split out of `cmd_execute` for #138. + split is pushed by now, and the blob-sha accounting for every source path + is the report a human is told to read back (AGENTS.md step 7); losing it + to a permission or a rate limit would be the more expensive failure. So + this RETURNS what it set rather than exiting: the caller reports the + rest after the table, with the commands to finish by hand, and still + exits non-zero. That holds for a `gh` that could not be run at all as + much as for one that refused (`LATE_FAILURES`, #188). Split out of + `cmd_execute` for #138. """ - try: - for role in ("assembly", "spec", "code"): + carried = [] + for role in TOPIC_ROLES: + try: run(["gh", "repo", "edit", repositories[role], "--add-topic", topic]) + except LATE_FAILURES as exc: + _flush_stdout() # the split is announced on stdout (#188) + print(_failed_loudly("setting the project topic", exc), + file=sys.stderr) + continue + carried.append(repositories[role]) + if len(carried) == len(TOPIC_ROLES): print(f" topic {topic} set on all three") - except LATE_FAILURES as exc: - _flush_stdout() # the pull request is announced on stdout (#188) - print(_failed_loudly("setting the project topic", exc), - file=sys.stderr) - print("The split IS pushed and the pull request IS open. Set the " - "topic by hand:\n" - + "\n".join(f" gh repo edit {repositories[role]} " - f"--add-topic {topic}" - for role in ("assembly", "spec", "code")), - file=sys.stderr) - return False - return True + elif carried: + print(f" topic {topic} set on {_and_listed(carried)} only") + return carried def _stage_the_split(assembly: Path, materialized: Materialized) -> None: @@ -4905,11 +5080,14 @@ def _commit_the_split(plan: Plan, source: Source, assembly: Path, names: dict, urls: dict, values: dict, work_root: Path, paths_for: dict, seeded: list, spec_path: str, code_path: str, branch: str, - leg_commits: dict) -> tuple[str, str] | None: + leg_commits: dict) -> tuple[str, str, list] | None: """(c) ONE split commit on a branch of the source, pushed. - Returns `(split_commit, message)` — the message travels on because it is - also the pull request's body — or None when making it failed, having + Returns `(split_commit, message, collisions)` — the message travels on + because it is also the pull request's body, and the collisions, + `(intended, actual)` for each shape file written beside one the source + kept, because where the shape's Makefile went decides what `_next_steps` + can tell a person to run (#480) — or None when making it failed, having printed which step, why and what it leaves behind. Split out of `cmd_execute` for #138. @@ -4987,7 +5165,7 @@ def _commit_the_split(plan: Plan, source: Source, assembly: Path, interrupted=_interrupted(exc), source=source)) raise print(f"\n split {split_commit[:12]} on {branch} -> {urls['assembly']}") - return split_commit, message + return split_commit, message, materialized.collisions def cmd_execute(args) -> int: @@ -5060,34 +5238,41 @@ def cmd_execute(args) -> int: code_path, branch, leg_commits) if split is None: return 2 - split_commit, message = split - - # A pull request `gh` refused does not end the run here: the legs and the - # split ARE pushed, and what the person needs to know before they open it - # is whether the split is right (#304). The topic is not tried after a - # refused pull request, because what it says is that the pull request IS - # open. - pull_request_refused = topics_failed = False + split_commit, message, collisions = split + + # THE TOPIC FIRST, THEN THE PULL REQUEST (#481). Neither needs the other, + # and with the pull request first a refused one ended the run before the + # topic was tried: three repositories without the topic `project.yaml` + # claims, and a way out that named only `gh pr create`. Neither ends the + # run here: the legs and the split ARE pushed, and what the person needs + # to know before finishing either by hand is whether the split is right + # (#304), so what is left is said once, after the table. + unset: list[str] = [] + refused: list[str] | None = None if not local: - pull_request_refused = not _open_the_pull_request( - repositories, names, tracking, branch, message) - if not pull_request_refused: - topics_failed = not _set_the_topic(repositories, topic) + carried = _set_the_topic(repositories, topic) + unset = [repositories[role] for role in TOPIC_ROLES + if repositories[role] not in carried] + if not _open_the_pull_request(repositories, names, tracking, branch, + message): + refused = _pull_request_command(repositories, names, tracking, + branch, message) # ---- (d) verification, by blob sha ------------------------------------ verified = _verify(source, assembly, work_root, names, paths_for, split_commit, spec_path, code_path, seeded, urls=urls, branch=branch) - if pull_request_refused: - _open_it_by_hand(repositories, names, tracking, branch, message, - not verified) - elif not verified: - _next_steps(names) + if not verified and refused is None: + # Where the shape's Makefile WENT, as the materializer reports it, + # and not a second look at the tree (#480). + _next_steps(names, any(intended == SHAPE_MAKEFILE + for intended, _ in collisions)) + _finish_by_hand(not verified, refused, topic, unset) if verified: # A verification mismatch outranks a refused pull request and a # missing topic. return verified - return 2 if pull_request_refused or topics_failed else 0 + return 2 if refused or unset else 0 # --------------------------------------------------------------------------- @@ -5469,13 +5654,35 @@ def _left_by_the_split(source: Source, urls: dict, names: dict, + _branch_deletion(source, urls, branch)) -def _next_steps(names: dict) -> None: +#: The shape file whose targets `make bootstrap` needs, as the materializer +#: names it in a collision (#480). +SHAPE_MAKEFILE = "Makefile" + + +def _next_steps(names: dict, makefile_aside: bool) -> None: """What a person does once the split is verified AND its pull request is open. Not said where there is none to review: after a refused `gh pr - create`, the command to open it is the next step (#304).""" + create`, the command to open it is the next step (#304). + + `make bootstrap` is a target of the SHAPE's Makefile. A source that kept + its own `Makefile` in the root has the shape's beside it, at + `shape/Makefile`, until a person merges the two as the follow-up says + (`makefile_aside`, read from where `execute` wrote it), and in that root + `make bootstrap` exits 2, `No rule to make target 'bootstrap'` (#480). + So the command there is the one `make bootstrap` runs, `python3 + scripts/bootstrap.py`, and a line says why. Elsewhere the block is what + it was, byte for byte.""" + bootstrap = ("python3 scripts/bootstrap.py" if makefile_aside + else "make bootstrap") print(f"\nNEXT: review the pull request on {names['assembly']}, then\n" f" git clone --recurse-submodules <{names['assembly']} url>\n" - " make bootstrap") + f" {bootstrap}") + if makefile_aside: + print(f"NOTE the root keeps its own {SHAPE_MAKEFILE} and the shape's " + f"is {COLLISION_DIR}/{SHAPE_MAKEFILE}, so `make bootstrap` and " + "the shape's other make targets arrive only once the follow-up " + "merges the two; `python3 scripts/bootstrap.py` is what `make " + "bootstrap` runs.") def _account_for(before: dict, after: dict, drops: list[str]) -> tuple[dict, list]: @@ -6005,7 +6212,14 @@ def main(argv: list[str] | None = None) -> int: "after 2026-09-02, and the staged fragment it was " "ratified from for one dated earlier — so " "--elected-on chooses it.") - plan.add_argument("--tracking-branch", default="main") + plan.add_argument("--tracking-branch", default=None, + help="the branch the assembly root and both legs " + "track, and the base of the split's pull request " + "(default: the source's default branch, which the " + "plan records as `source.default_branch`). " + "`check` finds, and `execute` refuses before any " + "leg exists, one that is no branch of the source " + "(`plan-tracking-branch-missing`).") plan.add_argument("--spec-path", default="spec", help=_leg_path_help("spec", "spec")) plan.add_argument("--code-path", default="code", diff --git a/docs/cli.md b/docs/cli.md index 681c0181..5e1321c7 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -388,6 +388,12 @@ options: fragment it was ratified from for one dated earlier — so --elected-on chooses it. --tracking-branch TRACKING_BRANCH + the branch the assembly root and both legs track, and + the base of the split's pull request (default: the + source's default branch, which the plan records as + `source.default_branch`). `check` finds, and `execute` + refuses before any leg exists, one that is no branch + of the source (`plan-tracking-branch-missing`). --spec-path SPEC_PATH where the spec leg is mounted, as a path in the assembly root (default: spec); the plan records it as diff --git a/scripts/shape_materialize.py b/scripts/shape_materialize.py index 1e3a6cf5..7a0c027e 100644 --- a/scripts/shape_materialize.py +++ b/scripts/shape_materialize.py @@ -641,6 +641,11 @@ def readme_trailing_shape_line(text: str) -> "re.Match | None": #: agents ALREADY read, and telling a human to "merge" two agent instruction #: files is how the project's own instructions get lost inside the shape's. ASSISTANT_INSTRUCTIONS = ("AGENTS.md", "CLAUDE.md") +#: The make targets of the shape's own `Makefile` that a person runs. A root +#: that kept the source's `Makefile` has none of them until the shape's copy +#: is merged into it (#480), and the collision follow-up says so. +#: `tests/test_adopt_e2e.py` holds this to the template's own targets. +SHAPE_MAKE_TARGETS = ("bootstrap", "validate", "pins", "park", "resume") def collision_follow_up(intended: str, actual: str) -> str: @@ -650,6 +655,10 @@ def collision_follow_up(intended: str, actual: str) -> str: the plan's surviving root paths, and `execute` reports the ones that actually happened. Two spellings of the same instruction is how the plan a human approves stops matching the commit message they read afterwards. + + THE MAKEFILE'S MERGE IS WHAT BRINGS THE SHAPE'S MAKE TARGETS (#480). Until + it is made, `make bootstrap` in the root exits 2 with `No rule to make + target 'bootstrap'`, so the follow-up says that, and what runs meanwhile. """ if intended in ASSISTANT_INSTRUCTIONS: return ( @@ -659,10 +668,16 @@ def collision_follow_up(intended: str, actual: str) -> str: "`contracts/path-classification.yaml` keeps it at the root " "(`root-assistant-instructions`) because it addresses the whole " f"project. Then delete {actual}. Nothing was overwritten.") - return ( + merge = ( f"merge {actual} into {intended}: the source repository already has " "that name, so the shape's copy was written beside it and NOTHING was " "overwritten") + if intended != "Makefile": + return merge + targets = ", ".join(f"`{target}`" for target in SHAPE_MAKE_TARGETS) + return (f"{merge}. The shape's make targets ({targets}) arrive only with " + "that merge; until then `python3 scripts/bootstrap.py` is what " + "`make bootstrap` runs") def copy_in_the_way(target: Path, actual: str) -> str | None: diff --git a/tests/test_adopt_e2e.py b/tests/test_adopt_e2e.py index b75efafe..244d7a18 100644 --- a/tests/test_adopt_e2e.py +++ b/tests/test_adopt_e2e.py @@ -10,6 +10,8 @@ from __future__ import annotations +import importlib.util +import re import shutil import subprocess import sys @@ -215,6 +217,76 @@ def test_the_sources_own_agent_files_are_untouched(adopted, adopted_clone): assert "AGENTS-shape.md" in rows +def next_block(stdout: str) -> list[str]: + """The lines `execute` prints from `NEXT:` on, to the end of its run.""" + return stdout.split("\nNEXT: ", 1)[1].splitlines() + + +def test_next_names_the_bootstrap_a_root_with_its_own_makefile_can_run( + adopted, adopted_clone): + """The source kept its own Makefile, so the shape's is `shape/Makefile` + until a person merges the two, and `make bootstrap` in the root exits 2 + (`No rule to make target 'bootstrap'`) (#480). The NEXT block names the + command `make bootstrap` runs instead, a line says why, and that command, + run in a recursive clone of the split, exits 0. (It is run with the + interpreter running this test, which is what `python3` names here.)""" + said = next_block(adopted["stdout"]) + assert said[:3] == [f"review the pull request on {PROJECT}, then", + f" git clone --recurse-submodules <{PROJECT} url>", + " python3 scripts/bootstrap.py"], said + assert said[3].startswith("NOTE the root keeps its own Makefile and the " + "shape's is shape/Makefile, so `make bootstrap`") + command = said[2].split() + ran = subprocess.run([sys.executable, *command[1:]], cwd=adopted_clone, + capture_output=True, text=True, encoding="utf-8", + errors="replace", check=False) + assert ran.returncode == 0, ran.stdout + ran.stderr + if shutil.which("make"): + made = subprocess.run(["make", "bootstrap"], cwd=adopted_clone, + capture_output=True, text=True, + encoding="utf-8", errors="replace", check=False) + assert made.returncode != 0, made.stdout + made.stderr + + +def test_the_makefile_follow_up_says_the_shapes_targets_arrive_with_it( + adopted): + """The follow-up the plan carries, and the split commit lists, names + each of the shape's make targets as arriving only with the merge, and + what runs meanwhile. The targets are read from the template, so a target + the shape gains and the follow-up does not name fails here.""" + template = (REPO / "templates" / "assembly-root" / "Makefile").read_text( + encoding="utf-8") + targets = [name for name in re.findall(r"(?m)^([a-z][a-z-]*):", template) + if name != "help"] + assert "bootstrap" in targets, targets + message = git("log", "-1", "--format=%B", "adopt/three-repo-shape", + cwd=adopted["source"]).stdout + (planned,) = [item for item in load_yaml(adopted["plan"])["follow_ups"] + if item.startswith("merge shape/Makefile into Makefile")] + for text in (planned, " ".join(message.split())): + assert ("The shape's make targets (" + + ", ".join(f"`{target}`" for target in targets) + + ") arrive only with that merge; until then `python3 " + "scripts/bootstrap.py` is what `make bootstrap` runs") in text + + +def test_next_keeps_make_bootstrap_where_the_shapes_makefile_is_the_roots( + capsys): + """Where the shape's Makefile took its own name, NEXT is what it always + was, byte for byte.""" + module_spec = importlib.util.spec_from_file_location("adopt_e2e_next", + ADOPT) + adopter = importlib.util.module_from_spec(module_spec) + module_spec.loader.exec_module(adopter) + + adopter._next_steps({"assembly": PROJECT}, False) + + assert capsys.readouterr().out == ( + f"\nNEXT: review the pull request on {PROJECT}, then\n" + f" git clone --recurse-submodules <{PROJECT} url>\n" + " make bootstrap\n") + + def test_the_agent_file_collision_says_add_a_line_not_merge(adopted): """A merge is the wrong instruction for an instruction file: it is how a project's own agent instructions end up inside the shape's. The follow-up diff --git a/tests/test_adopt_github_exits.py b/tests/test_adopt_github_exits.py index d89a46a1..732b839a 100644 --- a/tests/test_adopt_github_exits.py +++ b/tests/test_adopt_github_exits.py @@ -115,24 +115,48 @@ #: `gh` with no network. `repo create` makes a bare repository under #: $FAKE_GH_ROOT, which is what the rewritten `https://github.com/` URL then -#: names, and the one command named in $FAKE_GH_REFUSES is refused the way a +#: names. Each command $FAKE_GH_REFUSES names (`|` between them; `pr create`, +#: or `repo edit ` for one repository alone) is refused the way a #: token without the permission is, after writing the arguments it was given, -#: NUL-separated, to $FAKE_GH_ROOT/refused-arguments. Every call records the -#: token and the config directory it was run with in $FAKE_GH_ROOT/environment: -#: they are what a real `gh` would have been run with. +#: NUL-separated, to $FAKE_GH_ROOT/refused-arguments. `pr create` is refused +#: as GitHub refuses a base that is no branch of the repository (#479), and +#: records ` ` in $FAKE_GH_ROOT/opened when it opens one. +#: Every call records the token and the config directory it was run with in +#: $FAKE_GH_ROOT/environment, which are what a real `gh` would have been run +#: with, and its first three arguments, a line each, in $FAKE_GH_ROOT/calls. GH_SHIM = textwrap.dedent("""\ #!/bin/sh printf '%s\\n%s\\n' "$GH_TOKEN" "$GH_CONFIG_DIR" > "$FAKE_GH_ROOT/environment" + printf '%s %s %s\\n' "$1" "$2" "$3" >> "$FAKE_GH_ROOT/calls" + refused=no + old_ifs=$IFS; IFS='|' + for one in $FAKE_GH_REFUSES; do + case "$one" in "$1 $2"|"$1 $2 $3") refused=yes;; esac + done + IFS=$old_ifs + if [ "$refused" = yes ]; then + : > "$FAKE_GH_ROOT/refused-arguments" + for argument in "$@"; do + printf '%s\\0' "$argument" >> "$FAKE_GH_ROOT/refused-arguments" + done + echo "HTTP 403: Resource not accessible by integration" >&2; exit 1 + fi case "$1 $2" in - "$FAKE_GH_REFUSES") - : > "$FAKE_GH_ROOT/refused-arguments" - for argument in "$@"; do - printf '%s\\0' "$argument" >> "$FAKE_GH_ROOT/refused-arguments" - done - echo "HTTP 403: Resource not accessible by integration" >&2; exit 1;; "repo create") git init -q --bare -b main "$FAKE_GH_ROOT/$3.git" && echo "https://github.com/$3";; - "pr create") echo "https://github.com/testorg/Northwind/pull/1";; + "pr create") + repo=; base= + while [ $# -gt 0 ]; do + case "$1" in --repo) repo=$2; shift;; --base) base=$2; shift;; esac + shift + done + if ! git --git-dir="$FAKE_GH_ROOT/$repo.git" rev-parse -q --verify \\ + "refs/heads/$base" > /dev/null; then + echo "pull request create failed: GraphQL: Base ref must be a branch" >&2 + exit 1 + fi + printf '%s %s\\n' "$repo" "$base" > "$FAKE_GH_ROOT/opened" + echo "https://github.com/$repo/pull/1";; "repo edit") ;; *) exit 1;; esac @@ -185,7 +209,8 @@ def step_7() -> str: def github_run(tmp_path: Path, refuses: str, tree: dict = SOURCE_TREE, keep_contracts: bool = False, merge_stderr: bool = False, - stdout_closed: bool = False): + stdout_closed: bool = False, branch: str = "main", + plan_flags: tuple = ()): """Plan a small source and `execute` it in GitHub mode, with a `gh` that refuses `refuses`. Returns `(result, github)`: the run, and the directory that stands for github.com, with `testorg/.git` in it. @@ -196,10 +221,12 @@ def github_run(tmp_path: Path, refuses: str, tree: dict = SOURCE_TREE, then block-buffered, and the order the lines arrive in is the order the tool flushed them. `stdout_closed` gives the run a stdout whose reading end is already closed, as `| head` leaves it once `head` has quit, so that - `result.stdout` is None.""" + `result.stdout` is None. `branch` is the source's own (default) branch, + and `plan_flags` go to `plan` after the ones `write_plan` passes.""" github = tmp_path / "github" (github / ORG).mkdir(parents=True) - source = make_source_repo(tmp_path / NORTHWIND, tree=tree, edits=()) + source = make_source_repo(tmp_path / NORTHWIND, tree=tree, edits=(), + branch=branch) # The assembly root is the repository the source was cloned from: the # split branch is pushed back to it. git("clone", "-q", "--bare", ".", str(github / ORG / f"{NORTHWIND}.git"), @@ -220,7 +247,7 @@ def github_run(tmp_path: Path, refuses: str, tree: dict = SOURCE_TREE, gh_config.mkdir() plan = tmp_path / "adoption-plan.yaml" - written = write_plan(source, plan) + written = write_plan(source, plan, extra=plan_flags) assert written.returncode == 0, written.stderr + written.stdout if keep_contracts: answered = plan.read_text(encoding="utf-8") @@ -278,14 +305,36 @@ def heads(repository: Path) -> list[str]: return sorted(listed.split()) -def assert_pushed(github: Path) -> None: +def assert_pushed(github: Path, branch: str = "main") -> None: """Both legs and the split branch ARE pushed, which is why a re-run is - not the way out: it would push a second pair beside these.""" + not the way out: it would push a second pair beside these. Each leg is + pushed to the tracking branch, `branch`.""" for leg in ("spec", "code"): - assert heads(github / ORG / f"{NORTHWIND}-{leg}.git") == ["main"] + assert heads(github / ORG / f"{NORTHWIND}-{leg}.git") == [branch] assert SPLIT_BRANCH in heads(github / ORG / f"{NORTHWIND}.git") +def gh_calls(github: Path) -> list[str]: + """The `gh` commands a run made, in order, each by its first three + arguments (`repo edit testorg/Northwind`, `pr create --repo`).""" + calls = github / "calls" + if not calls.exists(): + return [] + return [line.strip() for line in + calls.read_text(encoding="utf-8").splitlines()] + + +def topic_and_pull_request(github: Path) -> list[str]: + """The `gh repo edit` and `gh pr create` calls a run made, in order.""" + return [call for call in gh_calls(github) + if call.startswith(("repo edit", "pr create"))] + + +#: The three topic calls `execute` makes, one per repository, in its order. +TOPIC_CALLS = [f"repo edit {ORG}/{name}" for name in + (NORTHWIND, f"{NORTHWIND}-spec", f"{NORTHWIND}-code")] + + # --- AGENTS.md step 7 -------------------------------------------------------- def test_step_7_keys_its_advice_to_the_exit_and_not_to_non_zero(): @@ -366,11 +415,15 @@ def test_each_exit_after_the_push_is_exit_2_with_a_command_to_finish_by_hand( in result.stdout) is opened, result.stdout assert " split " in result.stdout, result.stdout assert NOT_BY_HAND not in result.stderr, result.stderr - # The topic is never SET here. After a refused pull request it is not - # tried, because what its by-hand message says is that the pull request - # IS open (the stub would have let it through); after a refused topic it - # was tried and refused. - assert "set on all three" not in result.stdout, result.stdout + # The topic is tried BEFORE the pull request (#481), so a refused pull + # request no longer leaves it unset: the stub lets it through, and it is + # set on all three. It used not to be tried after a refused pull request + # at all, and the way out never said so. The way out names what is left + # and nothing else: the pull request's command, or the topic's. + topic_set = refuses != "repo edit" + assert ("set on all three" in result.stdout) is topic_set, result.stdout + assert ("Set the topic by hand" in result.stderr) is not topic_set, \ + result.stderr # `NEXT: review the pull request` is said where there is one to review, # and not after a refused `gh pr create`, where the command to open it is # the next step. @@ -460,13 +513,15 @@ def test_a_closed_stdout_does_not_lose_the_command_to_open_the_pull_request( @WINDOWS_SKIP # the `gh` is a `#!` script, which Windows does not run @needs_filter_repo def test_a_failed_verification_outranks_a_refused_pull_request(tmp_path): - """The split fails the verification, and `gh pr create` is refused too: - exit 1, the finding and its footer, and NO command to open the pull - request. The refusal above names the exact command that was tried, so the - run says that none is to be opened; it used to print the by-hand command - and exit 2 without ever having verified the split.""" - result, github = github_run(tmp_path, "pr create", tree=FAILING_TREE, - keep_contracts=True) + """The split fails the verification, and `gh pr create` and the topic are + refused too: exit 1, the finding and its footer, and NO command to open + the pull request or to set the topic. The refusal above names the exact + command that was tried, so the run says that none is to be opened; it + used to print the by-hand command and exit 2 without ever having verified + the split. The topic's way out is the verification's too: the re-run it + names sets the topic (#481).""" + result, github = github_run(tmp_path, "pr create|repo edit", + tree=FAILING_TREE, keep_contracts=True) assert result.returncode == 1, result.stdout + result.stderr assert FINDING in result.stderr, result.stderr @@ -474,8 +529,10 @@ def test_a_failed_verification_outranks_a_refused_pull_request(tmp_path): # What gh refused is still said, and so is what that leaves. assert "REFUSED opening the pull request." in result.stderr, result.stderr assert NOT_BY_HAND in result.stderr, result.stderr - # Nothing tells the person to open one, or that the adoption is done. + # Nothing tells the person to open one, to set the topic by hand, or that + # the adoption is done. assert "Open the pull request by hand" not in result.stderr + result.stdout + assert "Set the topic by hand" not in result.stderr + result.stdout assert "IS pushed" not in result.stderr + result.stdout assert NEXT not in result.stdout, result.stdout assert VERIFIED not in result.stdout, result.stdout @@ -500,6 +557,125 @@ def test_the_same_source_verifies_when_gh_opens_the_pull_request(tmp_path): assert_pushed(github) +# --- the topic comes first, and what is left is said once (#481) ------------ + +@WINDOWS_SKIP # the `gh` is a `#!` script, which Windows does not run +@needs_filter_repo +def test_a_refused_pull_request_no_longer_leaves_the_topic_unset(tmp_path): + """The issue's own run: `gh pr create` refused. The stub's log showed two + `repo create` calls, one `pr create` and no `repo edit`, so none of the + three repositories carried the topic `project.yaml` claims, and the way + out named only the pull request. The topic is now set on each of the + three BEFORE the pull request is tried, and the way out names the one + thing left.""" + result, github = github_run(tmp_path, "pr create") + + assert result.returncode == 2, result.stdout + result.stderr + assert topic_and_pull_request(github) == [*TOPIC_CALLS, "pr create --repo"] + assert f" topic {TOPIC} set on all three" in result.stdout + assert BRANCH_IS_PUSHED in result.stderr, result.stderr + assert "Set the topic by hand" not in result.stderr, result.stderr + + +@WINDOWS_SKIP # the `gh` is a `#!` script, which Windows does not run +@needs_filter_repo +def test_a_refused_pull_request_and_topic_are_one_notice_after_the_table( + tmp_path): + """Both refused: each refusal is printed as it happens, and what is left + is ONE notice after the table, `IS pushed` as step 7 reads it, naming + the `gh repo edit` for each repository and then the command that opens + the pull request, which is still the last thing the run says.""" + result, github = github_run(tmp_path, "pr create|repo edit", + merge_stderr=True) + said = result.stdout + + assert result.returncode == 2, said + notice = ("The branch IS pushed. Set the topic by hand:\n" + + "".join(f" gh {call} --add-topic {TOPIC}\n" + for call in TOPIC_CALLS) + + "Open the pull request by hand:\n" + f" gh pr create --repo {ORG}/{NORTHWIND} --base main " + f"--head {SPLIT_BRANCH}") + assert said.index("VERIFICATION") < said.index(VERIFIED) \ + < said.index(notice), said + assert said.count("REFUSED setting the project topic.") == 3, said + assert said.count("REFUSED opening the pull request.") == 1, said + assert said.count(BY_HAND) == 1 and said.count("IS pushed") == 1, said + printed = printed_command(said) + assert printed[:3] == ["gh", "pr", "create"] and printed[-2] == "--body" + assert " topic " not in said and NEXT not in said, said + assert topic_and_pull_request(github) == [*TOPIC_CALLS, "pr create --repo"] + assert_pushed(github) + + +@WINDOWS_SKIP # the `gh` is a `#!` script, which Windows does not run +@needs_filter_repo +def test_a_topic_refused_on_one_repository_names_that_one_alone(tmp_path): + """Each repository is asked, whatever the one before it answered, and the + way out names the command for the one left without the topic and for no + repository that has it. The first refusal used to end the topic step and + name all three.""" + result, github = github_run(tmp_path, f"repo edit {ORG}/{NORTHWIND}-spec") + + assert result.returncode == 2, result.stdout + result.stderr + assert topic_and_pull_request(github) == [*TOPIC_CALLS, "pr create --repo"] + assert (f" topic {TOPIC} set on {ORG}/{NORTHWIND} and " + f"{ORG}/{NORTHWIND}-code only") in result.stdout, result.stdout + assert result.stderr.count("REFUSED setting the project topic.") == 1 + by_hand = result.stderr.split("Set the topic by hand:\n", 1)[1] + assert by_hand == (f" gh repo edit {ORG}/{NORTHWIND}-spec " + f"--add-topic {TOPIC}\n"), result.stderr + assert ("The split IS pushed and the pull request IS open. " + "Set the topic by hand:") in result.stderr + assert NEXT in result.stdout and VERIFIED in result.stdout + + +# --- the pull request's base is a branch the source has (#479) -------------- + +@WINDOWS_SKIP # the `gh` is a `#!` script, which Windows does not run +@needs_filter_repo +@pytest.mark.parametrize("branch", ["master", "trunk"]) +def test_a_source_on_another_branch_opens_its_pull_request_against_it( + tmp_path, branch): + """A `master` or `trunk` source: `plan` records that branch as the + tracking branch, both legs are pushed to it, the root's `project.yaml` + tracks it, and the pull request opens against it. With `main` as the + tracking branch whatever the source used, all of it was pushed and then + GitHub refused the pull request (`Base ref must be a branch`), and the + printed way out was the same command.""" + result, github = github_run(tmp_path, "never", branch=branch) + + assert result.returncode == 0, result.stdout + result.stderr + assert VERIFIED in result.stdout and NEXT in result.stdout + assert (github / "opened").read_text(encoding="utf-8") == \ + f"{ORG}/{NORTHWIND} {branch}\n" + assert_pushed(github, branch) + root = git("show", f"{SPLIT_BRANCH}:project.yaml", + cwd=github / ORG / f"{NORTHWIND}.git").stdout + assert f"\ntracking_branch: {branch}\n" in root, root + assert "Base ref must be a branch" not in result.stderr + + +@WINDOWS_SKIP # the `gh` is a `#!` script, which Windows does not run +@needs_filter_repo +def test_a_base_the_source_lacks_is_refused_before_any_repository(tmp_path): + """`--tracking-branch main` on a `master` source still wins in the plan, + and `execute` refuses it before it makes anything, as `check` finds it: + no `gh` is run at all, where both legs and the split used to be pushed + before GitHub refused the pull request.""" + result, github = github_run(tmp_path, "never", branch="master", + plan_flags=("--tracking-branch", "main")) + + assert result.returncode == 2, result.stdout + result.stderr + assert "REFUSED plan-tracking-branch-missing:" in result.stderr + assert ("`tracking_branch` is `main`, which is no branch of the source " + "(its branches: `master`; the source's default branch is " + "`master`)") in result.stderr, result.stderr + assert "no leg repository was created" in result.stderr + assert gh_calls(github) == [] + assert not (github / ORG / f"{NORTHWIND}-spec.git").exists() + + # --- the real `gh` stays out of reach --------------------------------------- @WINDOWS_SKIP # the `gh` is a `#!` script, which Windows does not run diff --git a/tests/test_adopt_late_failure.py b/tests/test_adopt_late_failure.py index fc94d9ba..0061a706 100644 --- a/tests/test_adopt_late_failure.py +++ b/tests/test_adopt_late_failure.py @@ -1037,8 +1037,8 @@ def refused(args, *_rest, **_kw): # The command to open it by hand is said once the split has passed # its verification, as `cmd_execute` says it (#304), not by the step # that failed. - adopter._open_it_by_hand(repositories, names, "main", BRANCH, "body", - True) + adopter._finish_by_hand(True, adopter._pull_request_command( + repositories, names, "main", BRANCH, "body"), "", []) return "opening the pull request", "Open the pull request by hand" @@ -1049,6 +1049,10 @@ def refused(args, *_rest, **_kw): repositories = {role: f"{ORG}/{ASSEMBLY}-{role}" for role in (*LEGS, ROOT)} assert not adopter._set_the_topic(repositories, "xf-project-northwind") + # The commands to set it by hand are said after the verification too, + # beside the pull request's (#481), not by the step that failed. + adopter._finish_by_hand(True, None, "xf-project-northwind", + list(repositories.values())) return "setting the project topic", "Set the topic by hand" diff --git a/tests/test_adopt_tracking_branch.py b/tests/test_adopt_tracking_branch.py new file mode 100644 index 00000000..6519129d --- /dev/null +++ b/tests/test_adopt_tracking_branch.py @@ -0,0 +1,193 @@ +# SPDX-License-Identifier: Apache-2.0 +"""The tracking branch follows the source's default branch (#479). + +`tracking_branch` is the branch both legs are pushed to, the branch the +assembly root's `project.yaml` says the project tracks, and the base `execute` +opens the split's pull request against. `plan` defaulted it to `main` whatever +the source used, so a `master` or `trunk` source was adopted all the way, both +legs created and pushed and the split pushed and verified, and then its pull +request was refused (`Base ref must be a branch`), with the same refused +command printed as the way out. + +`plan` now records the branch it reads the source at unless `--tracking-branch` +names another, and `check` finds, and `execute` refuses before any leg exists, +a tracking branch that is no branch of the source +(`plan-tracking-branch-missing`). The branches are the source's remote +`origin`'s when it has fetched any, since that is the repository the pull +request goes to, and its own otherwise. + +The GitHub-mode runs, a `master` and a `trunk` source whose pull request opens +against that branch and a base the source lacks refused before any `gh` runs, +are in `tests/test_adopt_github_exits.py`, beside the other exits a GitHub run +has. Nothing here runs `gh`: every run below is `plan` and `check`, which read +a local source, or `execute --local-remote-dir`, which refuses before any leg. +""" + +from __future__ import annotations + +import shutil +import sys + +import pytest + +from conftest import (ADOPT, FILE_PROTOCOL, REPO, git, make_source_repo, + run_script, write_plan) + +sys.path.insert(0, str(REPO / "scripts")) +from repo_shape import load_yaml # noqa: E402 + +#: A source small enough that `plan` asks no question. +TREE = {"README.md": "# Northwind\n", "src/app.py": "print(1)\n", + "docs/spec.md": "# Spec\n"} +MISSING = "plan-tracking-branch-missing" +REMEDIATION = "Remediation: set `tracking_branch:` in the plan" + + +def planned(tmp_path, source, *flags: str): + """`plan` of `source` into `tmp_path`: the run, and the plan it wrote.""" + tmp_path.mkdir(parents=True, exist_ok=True) + plan = tmp_path / "adoption-plan.yaml" + result = write_plan(source, plan, extra=flags) + assert result.returncode == 0, result.stderr + result.stdout + return result, plan + + +def tracking_branch_of(plan) -> str: + return load_yaml(plan)["tracking_branch"] + + +def check(plan): + return run_script(ADOPT, "check", "--plan", str(plan)) + + +def retracked(plan, old: str, new: str) -> None: + """The plan's `tracking_branch:` edited by hand, LF as `plan` wrote it.""" + text = plan.read_text(encoding="utf-8") + line = f"\ntracking_branch: {old}\n" + assert line in text, text + with plan.open("w", encoding="utf-8", newline="\n") as handle: + handle.write(text.replace(line, f"\ntracking_branch: {new}\n", 1)) + + +def test_plan_records_the_branch_it_reads_the_source_at(tmp_path): + """No flag: the source's own branch, `master`, which `plan` also records + as `source.default_branch`, and the last follow-up names it. An explicit + `--tracking-branch` still wins, and the follow-up names that instead.""" + source = make_source_repo(tmp_path / "Northwind", tree=TREE, edits=(), + branch="master") + git("branch", "develop", cwd=source) + + result, plan = planned(tmp_path / "default", source) + written = load_yaml(plan) + assert written["source"]["default_branch"] == "master" + assert written["tracking_branch"] == "master", written + assert written["follow_ups"][-1].startswith( + "open the split as a pull request against master, the branch the " + "assembly root and both legs track;"), written["follow_ups"] + assert MISSING not in result.stdout + result.stderr + + result, plan = planned(tmp_path / "explicit", source, + "--tracking-branch", "develop") + assert tracking_branch_of(plan) == "develop" + assert load_yaml(plan)["follow_ups"][-1].startswith( + "open the split as a pull request against develop,") + assert MISSING not in result.stdout + result.stderr + assert check(plan).returncode == 0 + + +def test_check_finds_a_tracking_branch_the_source_does_not_have(tmp_path): + """A plan whose `tracking_branch:` was edited to a branch the source does + not have is a finding of `check`, naming the value, the branches the + source has and its default branch; the remediation is printed once. It + used to check ok, and the run it led to pushed both legs and the split + before GitHub refused the pull request.""" + source = make_source_repo(tmp_path / "Northwind", tree=TREE, edits=(), + branch="master") + git("branch", "develop", cwd=source) + _, plan = planned(tmp_path, source) + retracked(plan, "master", "main") + + result = check(plan) + + assert result.returncode == 1, result.stdout + result.stderr + assert (f"FINDING {MISSING}: `tracking_branch` is `main`, which is no " + "branch of the source (its branches: `develop`, `master`; the " + "source's default branch is `master`). `execute` pushes both legs " + "to it,") in result.stderr, result.stderr + assert "`Base ref must be a branch`" in result.stderr + assert result.stderr.count(REMEDIATION) == 1, result.stderr + assert "\n1 finding(s) in " in result.stderr + + +@pytest.mark.skipif(shutil.which("git-filter-repo") is None, + reason="git filter-repo is not installed: " + "`pip install git-filter-repo`") +def test_execute_refuses_it_before_any_leg_exists(tmp_path): + """`execute` refuses every finding of `check`'s before it creates a leg + repository (#168), and this one with them: under `--local-remote-dir` + the run used to make both legs at `main` and exit 0, over a project + tracking a branch its root does not have.""" + source = make_source_repo(tmp_path / "Northwind", tree=TREE, edits=(), + branch="master") + _, plan = planned(tmp_path, source) + retracked(plan, "master", "main") + + result = run_script(ADOPT, "execute", "--plan", str(plan), "--yes", + "--local-remote-dir", str(tmp_path / "remotes"), + "--work-dir", str(tmp_path / "work")) + + assert result.returncode == 2, result.stdout + result.stderr + assert f"REFUSED {MISSING}: 1 finding(s) in " in result.stderr + assert "no leg repository was created" in result.stderr + assert REMEDIATION in result.stderr + assert not (tmp_path / "remotes").exists() + + +def test_the_branches_asked_are_origins_once_the_source_has_fetched_any( + tmp_path): + """A clone knows the repository its pull request goes to by `origin`'s + branches. One only `origin` has, `develop`, checks ok; one only the clone + has, `wip`, never pushed, is what GitHub would refuse as a base, so + `plan` notes it and `check` finds it, naming `origin`.""" + seed = make_source_repo(tmp_path / "seed", tree=TREE, edits=()) + git("branch", "develop", cwd=seed) + remote = tmp_path / "Northwind.git" + git(*FILE_PROTOCOL, "clone", "-q", "--bare", str(seed), str(remote), + cwd=tmp_path) + source = tmp_path / "Northwind" + git(*FILE_PROTOCOL, "clone", "-q", str(remote), str(source), cwd=tmp_path) + git("branch", "wip", cwd=source) + + _, plan = planned(tmp_path / "develop", source, + "--tracking-branch", "develop") + checked = check(plan) + assert checked.returncode == 0, checked.stdout + checked.stderr + + result, plan = planned(tmp_path / "wip", source, "--tracking-branch", "wip") + found = ("is `wip`, which is no branch of the source's remote `origin` " + "(its branches: `develop`, `main`; the source's default branch " + "is `main`)") + assert f"NOTE the tracking branch {found}" in result.stdout, result.stdout + assert f"`check` reports this as `{MISSING}`" in result.stdout + checked = check(plan) + assert checked.returncode == 1, checked.stdout + checked.stderr + assert f"FINDING {MISSING}: `tracking_branch` {found}" in checked.stderr + + +def test_a_default_branch_no_command_line_can_carry_is_named_as_the_default( + tmp_path): + """The source's own branch is put on a `git` command line like any + tracking branch, so one outside that alphabet is refused before a plan + is written; and since nobody passed it, the refusal says where it came + from, as `--elected-by` is named when `git config user.name` filled it + in. It used to plan `main`, a branch this source does not have.""" + source = make_source_repo(tmp_path / "Northwind", tree=TREE, edits=(), + branch="feat#1") + plan = tmp_path / "adoption-plan.yaml" + + result = write_plan(source, plan) + + assert result.returncode == 2, result.stdout + result.stderr + assert ("unsafe-value: --tracking-branch (not given, so the source's " + "default branch) is 'feat#1'") in result.stderr, result.stderr + assert not plan.exists() From fc1a35beb1e797a27c5e0e24e9a443f1bc530be4 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Tue, 13 Oct 2026 00:04:33 +0000 Subject: [PATCH 2/5] adopt-project.py: `execute` asks the assembly's remote for the tracking branch before any leg exists, NEXT names no bootstrap command where the source's own `scripts/bootstrap.py` stayed in the root, and the pull-request follow-up names the plan's `tracking_branch`, so a clone whose `origin/master` GitHub has renamed is refused before anything is pushed (#479, #480, #481) Fix round on #493, closing RV493's F1-F3 and codex's two threads. - F1. `check`'s `plan-tracking-branch-missing` reads the branches the source's clone remembers. A clone made before GitHub renamed `master` to `main` keeps `origin/master` and `origin/HEAD -> origin/master` after an ordinary fetch, so `plan` recorded `master`, `check` said ok, and `execute` pushed both legs and the split before `gh pr create` refused the base. `execute` now runs `git ls-remote refs/heads/` after `_refuse_an_occupied_work_dir` and before `_refuse_a_split_branch_left_behind`, so before `_confirm` and before any leg: - branch absent: REFUSED `plan-tracking-branch-missing`, naming the url; the remediation is `git fetch --prune origin`, `git remote set-head origin --auto`, the clone's own branch renamed where the default was renamed, then `plan`, `check` and `execute` again; - remote unreadable: REFUSED `adopt-remote-unreadable`, with git's exit and output (fail closed); - under `--local-remote-dir` the url is the source itself, the split's destination there, and the remediation adds that the branch must be one of the source's own. - F2. `_next_steps` takes the whole collision list. Where `scripts/bootstrap.py` went aside, with or without the Makefile, NEXT names no bootstrap command and says which merges come first, then `make bootstrap`. `collision_follow_up` takes the list too (`beside`), and the Makefile's follow-up names no meanwhile command when the script went aside; `_predict_collisions` passes it. - F3. The last follow-up says "against the plan's `tracking_branch`" and no longer interpolates the branch, so a hand edit of `tracking_branch:` cannot leave `check` and the pull-request body naming the old one. `follow_ups_for` is back to main's signature. Tests: the rename scenario and an unreadable remote in test_adopt_github_exits.py (`github_run` takes a `before_plan` hook); the hand-edit follow-up in test_adopt_tracking_branch.py; NEXT with both files aside (an execute run), NEXT with the script alone and the plan-time Makefile follow-up in test_adopt_e2e.py. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 213 ++++++++++++++++++++++------ scripts/shape_materialize.py | 25 +++- tests/test_adopt_e2e.py | 120 ++++++++++++++-- tests/test_adopt_github_exits.py | 108 ++++++++++++-- tests/test_adopt_tracking_branch.py | 50 +++++-- 5 files changed, 436 insertions(+), 80 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index beb11e1e..e32ffe7b 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -173,6 +173,9 @@ # Whether a value this tool writes into a file is text a file can hold # (#255), asked beside `one_printable_line`, the trailer's question. from shape_materialize import unwritable_reason # noqa: E402 +# The shape's bootstrap script, which its `make bootstrap` runs: where it +# went beside one the source kept decides what NEXT can name (#480). +from shape_materialize import BOOTSTRAP # noqa: E402 #: The naming policy this tool classifies leg names against. One constant, #: because three spellings of the same path is how the second one goes stale. @@ -1351,17 +1354,21 @@ def _search_by_hand(named: str, why: str) -> str: def follow_ups_for(source: "Source", entries: list["Entry"], names: dict, spec_path: str, code_path: str, - collisions: list[str], tracking: str) -> list[str]: + collisions: list[str]) -> list[str]: """The code changes an in-place split MAKES NECESSARY, named in the plan. A split that moved `contracts/` into the spec leg and said nothing about the harness that reads it would have handed somebody a red build and no explanation. - The last one names `tracking`, the base `execute` opens the pull request - against (#479). It said "the source's default branch" while `execute` - used `--base main` whatever that branch was; the two now agree by - default, and a `--tracking-branch` that names another is named here. + The last one names the base `execute` opens the pull request against as + the plan's `tracking_branch` (#479). It said "the source's default + branch" while `execute` used `--base main` whatever that branch was. It + names the KEY and not its value: this list is written once, by `plan`, + and a value named here went on naming the old branch after a hand edit + of `tracking_branch:`, which `plan-tracking-branch-missing`'s + remediation asks for, in `check`'s output and in the pull request's own + body (#493's review, F3). """ out = cross_leg_references(source, entries, spec_path, code_path) if any(e.leg == "spec" and (e.path == "contracts/" @@ -1378,10 +1385,10 @@ def follow_ups_for(source: "Source", entries: list["Entry"], names: dict, "owner rule cannot cross a repository boundary") out.extend(collisions) out.append( - f"open the split as a pull request against {tracking}, the branch the " - f"assembly root and both legs track; the legs {names['spec']} and " - f"{names['code']} are seeded before it, so review the root's diff and " - "the two new repositories together") + "open the split as a pull request against the plan's " + "`tracking_branch`, the branch the assembly root and both legs track; " + f"the legs {names['spec']} and {names['code']} are seeded before it, " + "so review the root's diff and the two new repositories together") return out @@ -1781,12 +1788,22 @@ def _kept_collision_notes(args, entries: list, tree: list) -> list[str]: # clone of, and the source knows that repository's branches as its remote # `origin`'s: `gh repo clone` and `git clone` fetch every branch. A source # that has fetched none, a repository with no remote, is asked about its own -# branches; `--local-remote-dir` pushes the split back into it. Nothing is -# asked of the network, and a branch made on GitHub after the clone is -# fetched first, as the remediation says. +# branches; `--local-remote-dir` pushes the split back into it. `plan` and +# `check` ask nothing of the network, and a branch made on GitHub after the +# clone is fetched first, as the remediation says. +# +# BUT A CLONE REMEMBERS WHAT ITS REMOTE HAS SINCE LOST. An ordinary `git +# fetch` does not prune, so a branch deleted or renamed on GitHub stays as +# `origin/`, and a clone made before GitHub renamed `master` to +# `main` keeps `origin/HEAD -> origin/master`, which `plan` takes as the +# default. Such a plan checked ok, and `execute` pushed both legs and the +# split before GitHub refused the base (#493's review, F1). So `execute` +# also asks the remote itself, before any leg exists +# (`_refuse_a_tracking_branch_the_remote_lacks`). #: `check`'s finding for a tracking branch the source does not have, and -#: what it prints once after it (#479). +#: what it prints once after it (#479). `execute` refuses under the same code +#: a tracking branch the remote the split goes to does not have. TRACKING_BRANCH_MISSING = "plan-tracking-branch-missing" TRACKING_BRANCH_REMEDIATION = ( "Remediation: set `tracking_branch:` in the plan to a branch the source " @@ -1868,6 +1885,85 @@ def _tracking_branch_notes(args, source: Source) -> list[str]: "because the plan is yours to edit."] +#: `execute`'s refusal of a remote `git ls-remote` cannot read, asked before +#: any leg exists whether it has the tracking branch. +REMOTE_UNREADABLE = "adopt-remote-unreadable" +#: How `_refuse_a_tracking_branch_the_remote_lacks` ends, and the sentence it +#: adds under `--local-remote-dir`, where the source is the remote. +REMOTE_LACKS_REMEDIATION = ( + "Remediation: no leg repository was created and nothing was pushed. " + "`check` reads the branches the source's clone remembers, and a clone " + "keeps one its remote has since deleted or renamed: an ordinary `git " + "fetch` does not prune it, and a clone made before GitHub renamed its " + "default branch still names the old one. In the source, run `git fetch " + "--prune origin` and `git remote set-head origin --auto`, and where the " + "default branch was renamed, rename the clone's own to match, as " + "GitHub's notice of the rename says (`git branch -m `). Then " + "re-run `plan` (without `--tracking-branch` it records the default " + "branch the remote names now) and `check`, and run `execute` again.") +LOCAL_REMOTE_LACKS = ( + " Under `--local-remote-dir` the split is pushed back into the source " + "itself, so the tracking branch must be one of the source's own " + "branches, and one only its remote `origin` has is not: `git branch " + " origin/` makes it one.") + + +def _refuse_a_tracking_branch_the_remote_lacks(urls: dict, tracking: str, + local: bool) -> None: + """Refuse a tracking branch the repository the split goes to does not + have, BEFORE a leg repository exists (#479; #493's review, F1). + + `plan-tracking-branch-missing`, as `check` finds it, reads the branches + the source's clone remembers its remote by, and asks no network. A clone + of a repository whose `master` GitHub has since renamed to `main` still + has `origin/master`, and `origin/HEAD` names it, so `plan` recorded + `master` and `check` passed it. `execute` then created and pushed both + legs and the split before `gh pr create` refused the base, `Base ref must + be a branch`, where the same clone had adopted against `main` before + #479. A branch named by `--tracking-branch` and deleted on GitHub since + the clone's last fetch went the same way. + + So the branch is asked of the very remote the split is pushed to, with + `git ls-remote`, as `_refuse_a_split_branch_left_behind` asks it about + the split branch: the answer is the one the pull request will get. Under + `--local-remote-dir` that remote is the source itself, the split's + destination there, so its own branches are the ones asked. A remote that + cannot be read is refused as `adopt-remote-unreadable`, at the same + point: the split push needs it as much, and that came after both legs. + """ + ref = f"refs/heads/{tracking}" + remote = _spelled(urls["assembly"]) + try: + listed = run(["git", "ls-remote", urls["assembly"], ref]) + except (CommandFailed, OSError) as exc: + why = (f"exit {exc.code}: {' '.join(exc.output.split())}" + if isinstance(exc, CommandFailed) else str(exc)) + raise Refusal( + REMOTE_UNREADABLE, + f"`git ls-remote` could not read {remote}, the repository the " + f"split is pushed to, to ask whether it has {_spelled(tracking)}, " + f"the plan's `tracking_branch`: {why}", + "Remediation: no leg repository was created and nothing was " + "pushed. `execute` pushes the split to that repository, which " + "needs it as readable as this question does, so the run stops " + "here and not once both legs exist. Make it readable from here " + "-- its url, the network, the credentials git uses for it -- and " + "run `execute` again.") from exc + if any(line.split("\t")[-1] == ref for line in listed.splitlines()): + return + raise Refusal( + TRACKING_BRANCH_MISSING, + f"`tracking_branch` is {_spelled(tracking)}, which {remote}, the " + "repository the split is pushed to, does not have as `git ls-remote` " + "reads it now: " + + ("it is the branch the split is to be merged into there, and the " + "one the root's `project.yaml` says the project tracks" if local + else "`execute` opens the split's pull request against it once " + "both legs and the split are pushed, where GitHub refuses a base " + "that is not a branch (`Base ref must be a branch`)"), + REMOTE_LACKS_REMEDIATION + (LOCAL_REMOTE_LACKS if local else "")) + + def _checked_plan_inputs(args, source: Source, naming: NamingPolicy) -> tuple[dict, list]: """Every value the plan will RECORD, defaulted and checked before one of @@ -2246,8 +2342,7 @@ def cmd_plan(args) -> int: implied = ask_about_refused_paths(ruled, tree) materialized = _predict_collisions(implied) follow_ups = follow_ups_for(source, implied, names, args.spec_path, - args.code_path, materialized, - args.tracking_branch) + args.code_path, materialized) seeded = seeded_legs(assigned_paths(implied)) entries = ask_about_refused_paths( ask_about_submodules(policy, ruled, tree), tree) @@ -2323,13 +2418,17 @@ def _predict_collisions(entries: list[Entry]) -> list[str]: reads, so the two cannot list different files. The list this function kept itself lacked `contracts/shape-pin.yaml`, which `execute` also writes beside a copy the source keeps, and names in the split commit. + + Each follow-up is told the WHOLE list: the Makefile's names what runs + until it is merged, and that depends on whether `scripts/bootstrap.py` + went aside too (#493's review, F2). """ surviving = {e.path for e in entries if e.leg == "root"} - out = [] - for path in sorted(set(assembly_root_paths())): - if path in surviving or f"{path.split('/')[0]}/" in surviving: - out.append(collision_follow_up(path, f"{COLLISION_DIR}/{path}")) - return out + beside = {path: f"{COLLISION_DIR}/{path}" + for path in sorted(set(assembly_root_paths())) + if path in surviving or f"{path.split('/')[0]}/" in surviving} + return [collision_follow_up(path, actual, beside) + for path, actual in beside.items()] # --------------------------------------------------------------------------- @@ -5183,10 +5282,10 @@ def _commit_the_split(plan: Plan, source: Source, assembly: Path, Returns `(split_commit, message, collisions)` — the message travels on because it is also the pull request's body, and the collisions, `(intended, actual)` for each shape file written beside one the source - kept, because where the shape's Makefile went decides what `_next_steps` - can tell a person to run (#480) — or None when making it failed, having - printed which step, why and what it leaves behind. Split out of - `cmd_execute` for #138. + kept, because where the shape's Makefile and `scripts/bootstrap.py` went + decides what `_next_steps` can tell a person to run (#480) — or None + when making it failed, having printed which step, why and what it leaves + behind. Split out of `cmd_execute` for #138. BOTH LEGS ARE PUSHED BY NOW, so a split that cannot be cloned, mounted, committed or pushed leaves the same two leg repositories a leg that @@ -5299,6 +5398,9 @@ def cmd_execute(args) -> int: # What an earlier run left, the work directory first: a refusal below # whose way out is "run `execute` again" then meets one that is free. _refuse_an_occupied_work_dir(work_root, names, seeded) + # The remote's own answer about the tracking branch, which `check` reads + # only from what the source's clone remembers (#493's review, F1). + _refuse_a_tracking_branch_the_remote_lacks(urls, tracking, local) _refuse_a_split_branch_left_behind(source, urls, branch, local) for line in plan.seeding_record_disagreements(seeded): print(line) @@ -5368,10 +5470,9 @@ def cmd_execute(args) -> int: split_commit, spec_path, code_path, seeded, urls=urls, branch=branch) if not verified and refused is None: - # Where the shape's Makefile WENT, as the materializer reports it, - # and not a second look at the tree (#480). - _next_steps(names, any(intended == SHAPE_MAKEFILE - for intended, _ in collisions)) + # Where the shape's Makefile and bootstrap script WENT, as the + # materializer reports it, and not a second look at the tree (#480). + _next_steps(names, collisions) _finish_by_hand(not verified, refused, topic, unset) if verified: # A verification mismatch outranks a refused pull request and a @@ -5764,30 +5865,50 @@ def _left_by_the_split(source: Source, urls: dict, names: dict, SHAPE_MAKEFILE = "Makefile" -def _next_steps(names: dict, makefile_aside: bool) -> None: +def _next_steps(names: dict, collisions: list) -> None: """What a person does once the split is verified AND its pull request is open. Not said where there is none to review: after a refused `gh pr create`, the command to open it is the next step (#304). - `make bootstrap` is a target of the SHAPE's Makefile. A source that kept - its own `Makefile` in the root has the shape's beside it, at - `shape/Makefile`, until a person merges the two as the follow-up says - (`makefile_aside`, read from where `execute` wrote it), and in that root - `make bootstrap` exits 2, `No rule to make target 'bootstrap'` (#480). - So the command there is the one `make bootstrap` runs, `python3 - scripts/bootstrap.py`, and a line says why. Elsewhere the block is what - it was, byte for byte.""" - bootstrap = ("python3 scripts/bootstrap.py" if makefile_aside - else "make bootstrap") + `collisions` is each `(intended, actual)` the materializer wrote beside a + file the source kept: where the shape's files WENT, and not a second + look at the tree. `make bootstrap` is a target of the SHAPE's Makefile. A + source that kept its own `Makefile` in the root has the shape's beside + it, at `shape/Makefile`, until a person merges the two as the follow-up + says, and in that root `make bootstrap` exits 2, `No rule to make target + 'bootstrap'` (#480). So the command there is the one `make bootstrap` + runs, `python3 scripts/bootstrap.py`, and a line says why. + + UNLESS THAT SCRIPT WENT ASIDE TOO (#493's review, F2). A root that kept + its own `scripts/bootstrap.py` has the shape's at + `shape/scripts/bootstrap.py`, and there both commands run the SOURCE's + script, which can exit 0 having placed no leg and validated nothing. So + no bootstrap command is named: a line says which merges come first, and + that `make bootstrap` is run after them. Elsewhere the block is what it + was, byte for byte.""" + aside = dict(collisions) + script, makefile = aside.get(BOOTSTRAP), aside.get(SHAPE_MAKEFILE) + bootstrap = ("" if script else + "\n python3 scripts/bootstrap.py" if makefile else + "\n make bootstrap") print(f"\nNEXT: review the pull request on {names['assembly']}, then\n" - f" git clone --recurse-submodules <{names['assembly']} url>\n" - f" {bootstrap}") - if makefile_aside: + f" git clone --recurse-submodules <{names['assembly']} url>" + f"{bootstrap}") + if script: + kept = [path for path in (SHAPE_MAKEFILE, BOOTSTRAP) + if path in aside] + print(f"NOTE the root keeps its own {_and_listed(kept)}, and the " + f"shape's {'are' if len(kept) > 1 else 'is'} " + f"{_and_listed([aside[path] for path in kept])}, so neither " + f"`make bootstrap` nor `python3 {BOOTSTRAP}` runs the " + "shape's bootstrap yet: merge " + + _and_listed([f"{aside[path]} into {path}" for path in kept]) + + " first, and then run `make bootstrap`.") + elif makefile: print(f"NOTE the root keeps its own {SHAPE_MAKEFILE} and the shape's " - f"is {COLLISION_DIR}/{SHAPE_MAKEFILE}, so `make bootstrap` and " - "the shape's other make targets arrive only once the follow-up " - "merges the two; `python3 scripts/bootstrap.py` is what `make " - "bootstrap` runs.") + f"is {makefile}, so `make bootstrap` and the shape's other make " + "targets arrive only once the follow-up merges the two; " + "`python3 scripts/bootstrap.py` is what `make bootstrap` runs.") def _account_for(before: dict, after: dict, drops: list[str]) -> tuple[dict, list]: diff --git a/scripts/shape_materialize.py b/scripts/shape_materialize.py index 1abb729f..83de8e38 100644 --- a/scripts/shape_materialize.py +++ b/scripts/shape_materialize.py @@ -649,7 +649,8 @@ def readme_trailing_shape_line(text: str) -> "re.Match | None": SHAPE_MAKE_TARGETS = ("bootstrap", "validate", "pins", "park", "resume") -def collision_follow_up(intended: str, actual: str) -> str: +def collision_follow_up(intended: str, actual: str, + beside: dict | None = None) -> str: """The follow-up for ONE collision, in one place. Both callers say it: `adopt-project.py plan` PREDICTS the collisions from @@ -659,7 +660,13 @@ def collision_follow_up(intended: str, actual: str) -> str: THE MAKEFILE'S MERGE IS WHAT BRINGS THE SHAPE'S MAKE TARGETS (#480). Until it is made, `make bootstrap` in the root exits 2 with `No rule to make - target 'bootstrap'`, so the follow-up says that, and what runs meanwhile. + target 'bootstrap'`, so the follow-up says that, and what runs meanwhile: + `python3 scripts/bootstrap.py`, unless that script went aside as well. + `beside` maps each shape file written beside one the source kept to where + it went, the whole list, and only the Makefile's follow-up reads it. + Where the root's `scripts/bootstrap.py` is the source's own, that command + runs the source's script and exits 0, so the follow-up names no command, + and says both merges come first (#493's review, F2). """ if intended in ASSISTANT_INSTRUCTIONS: return ( @@ -676,9 +683,16 @@ def collision_follow_up(intended: str, actual: str) -> str: if intended != "Makefile": return merge targets = ", ".join(f"`{target}`" for target in SHAPE_MAKE_TARGETS) + script = (beside or {}).get(BOOTSTRAP) + if script is None: + return (f"{merge}. The shape's make targets ({targets}) arrive only " + "with that merge; until then `python3 scripts/bootstrap.py` " + "is what `make bootstrap` runs") return (f"{merge}. The shape's make targets ({targets}) arrive only with " - "that merge; until then `python3 scripts/bootstrap.py` is what " - "`make bootstrap` runs") + f"that merge, and the root's {BOOTSTRAP} is the source's " + f"own until {script} is merged into it as well, so neither `make " + f"bootstrap` nor `python3 {BOOTSTRAP}` runs the shape's " + "bootstrap before both merges are made") def copy_in_the_way(target: Path, actual: str) -> str | None: @@ -1780,7 +1794,8 @@ def __init__(self) -> None: self.shape_files: list[str] = [] def follow_ups(self) -> list[str]: - return [collision_follow_up(intended, actual) + beside = dict(self.collisions) + return [collision_follow_up(intended, actual, beside) for intended, actual in self.collisions] diff --git a/tests/test_adopt_e2e.py b/tests/test_adopt_e2e.py index bdb39339..77f2147d 100644 --- a/tests/test_adopt_e2e.py +++ b/tests/test_adopt_e2e.py @@ -18,8 +18,8 @@ import pytest -from conftest import (ADOPT, FILE_PROTOCOL, REPO, git, resolve, run_script, - write_plan) +from conftest import (ADOPT, FILE_PROTOCOL, REPO, git, make_source_repo, + resolve, run_script, write_plan) sys.path.insert(0, str(REPO / "scripts")) from repo_shape import load_yaml # noqa: E402 @@ -41,7 +41,6 @@ def adopted(tmp_path_factory) -> dict: """One real adoption: plan, answer the questions, check, execute.""" base = tmp_path_factory.mktemp("adopt") source = base / "Thing" - from conftest import make_source_repo make_source_repo(source) plan = base / "adoption-plan.yaml" written = write_plan(source, plan, project=PROJECT) @@ -270,16 +269,21 @@ def test_the_makefile_follow_up_says_the_shapes_targets_arrive_with_it( "scripts/bootstrap.py` is what `make bootstrap` runs") in text -def test_next_keeps_make_bootstrap_where_the_shapes_makefile_is_the_roots( - capsys): - """Where the shape's Makefile took its own name, NEXT is what it always - was, byte for byte.""" +@pytest.fixture(scope="module") +def adopter(): + """`adopt-project.py` as a module, for what it prints and predicts.""" module_spec = importlib.util.spec_from_file_location("adopt_e2e_next", ADOPT) - adopter = importlib.util.module_from_spec(module_spec) - module_spec.loader.exec_module(adopter) + module = importlib.util.module_from_spec(module_spec) + module_spec.loader.exec_module(module) + return module + - adopter._next_steps({"assembly": PROJECT}, False) +def test_next_keeps_make_bootstrap_where_the_shapes_makefile_is_the_roots( + adopter, capsys): + """Where the shape's Makefile took its own name, NEXT is what it always + was, byte for byte.""" + adopter._next_steps({"assembly": PROJECT}, []) assert capsys.readouterr().out == ( f"\nNEXT: review the pull request on {PROJECT}, then\n" @@ -287,6 +291,102 @@ def test_next_keeps_make_bootstrap_where_the_shapes_makefile_is_the_roots( " make bootstrap\n") +#: A source that keeps its own `scripts/bootstrap.py` in the root beside its +#: own Makefile. `scripts/` goes to the code leg unless the plan is answered +#: otherwise, as `kept_scripts` answers it. +OWN_BOOTSTRAP_TREE = { + "README.md": "# Northwind\n", "src/app.py": "print(1)\n", + "docs/spec.md": "# Spec\n", "Makefile": "build:\n\t@echo source build\n", + "scripts/bootstrap.py": "print('THE SOURCE OWN BOOTSTRAP')\n"} +#: What NEXT says in that root, in place of a bootstrap command. +BOTH_ASIDE = ( + "NOTE the root keeps its own Makefile and scripts/bootstrap.py, and the " + "shape's are shape/Makefile and shape/scripts/bootstrap.py, so neither " + "`make bootstrap` nor `python3 scripts/bootstrap.py` runs the shape's " + "bootstrap yet: merge shape/Makefile into Makefile and " + "shape/scripts/bootstrap.py into scripts/bootstrap.py first, and then " + "run `make bootstrap`.") + + +def kept_scripts(plan) -> None: + """The plan's `scripts/` answered `root`, LF as `plan` wrote it.""" + text = plan.read_text(encoding="utf-8") + sent = " - path: scripts/\n leg: code\n" + assert sent in text, text + with plan.open("w", encoding="utf-8", newline="\n") as handle: + handle.write(text.replace(sent, ( + " - path: scripts/\n leg: root\n" + " resolution: \"the source keeps its scripts in the root\"\n"), + 1)) + + +def test_next_names_no_bootstrap_where_the_roots_script_is_the_sources_own( + tmp_path): + """The source kept its own `scripts/bootstrap.py` in the root as well as + its own Makefile, so the shape's script went to + `shape/scripts/bootstrap.py`, and NEXT's `python3 scripts/bootstrap.py` + ran the SOURCE's script in a recursive clone and exited 0, with no leg + placed and nothing validated (#493's review, F2). NEXT names no + bootstrap command there, and says which merges come first.""" + source = make_source_repo(tmp_path / PROJECT, tree=OWN_BOOTSTRAP_TREE, + edits=()) + plan = tmp_path / "adoption-plan.yaml" + written = write_plan(source, plan, project=PROJECT) + assert written.returncode == 0, written.stderr + written.stdout + kept_scripts(plan) + + result = run_script(ADOPT, "execute", "--plan", str(plan), "--yes", + "--local-remote-dir", str(tmp_path / "remotes"), + "--work-dir", str(tmp_path / "work")) + + assert result.returncode == 0, result.stderr + result.stdout + assert " beside shape/scripts/bootstrap.py " in result.stdout + assert next_block(result.stdout) == [ + f"review the pull request on {PROJECT}, then", + f" git clone --recurse-submodules <{PROJECT} url>", + BOTH_ASIDE], result.stdout + + +def test_next_names_no_bootstrap_where_only_the_script_went_aside( + adopter, capsys): + """With the shape's Makefile in the root and the source's own + `scripts/bootstrap.py`, `make bootstrap` runs the source's script, so + NEXT names no command there either.""" + adopter._next_steps({"assembly": PROJECT}, [ + ("scripts/bootstrap.py", "shape/scripts/bootstrap.py")]) + + assert capsys.readouterr().out == ( + f"\nNEXT: review the pull request on {PROJECT}, then\n" + f" git clone --recurse-submodules <{PROJECT} url>\n" + "NOTE the root keeps its own scripts/bootstrap.py, and the shape's is " + "shape/scripts/bootstrap.py, so neither `make bootstrap` nor `python3 " + "scripts/bootstrap.py` runs the shape's bootstrap yet: merge " + "shape/scripts/bootstrap.py into scripts/bootstrap.py first, and then " + "run `make bootstrap`.\n") + + +def test_the_makefile_follow_up_waits_for_a_script_that_went_aside_too( + adopter): + """`plan` predicts the follow-ups from what the plan keeps in the root, + and tells each the whole list: where `scripts/` stays as well, the + root's `scripts/bootstrap.py` is the source's own, so the Makefile's + follow-up does not name `python3 scripts/bootstrap.py` as what runs + meanwhile (#493's review, F2).""" + kept = [adopter.Entry(path, adopter.Verdict( + "root", "test", "kept by the test", "high"), 1, 1) + for path in ("Makefile", "scripts/")] + + (makefile,) = [item for item in adopter._predict_collisions(kept) + if item.startswith("merge shape/Makefile into Makefile")] + + assert makefile.endswith( + "arrive only with that merge, and the root's scripts/bootstrap.py is " + "the source's own until shape/scripts/bootstrap.py is merged into it " + "as well, so neither `make bootstrap` nor `python3 " + "scripts/bootstrap.py` runs the shape's bootstrap before both merges " + "are made"), makefile + + def test_the_agent_file_collision_says_add_a_line_not_merge(adopted): """A merge is the wrong instruction for an instruction file: it is how a project's own agent instructions end up inside the shape's. The follow-up diff --git a/tests/test_adopt_github_exits.py b/tests/test_adopt_github_exits.py index 732b839a..63725dcc 100644 --- a/tests/test_adopt_github_exits.py +++ b/tests/test_adopt_github_exits.py @@ -59,7 +59,7 @@ import pytest from conftest import (ADOPT, REPO, WINDOWS_SKIP, git, make_source_repo, - write_plan) + run_script, write_plan) #: `execute` REQUIRES `git filter-repo`; without it the runs below are #: skipped, as every `execute` test in this suite is. @@ -210,7 +210,7 @@ def step_7() -> str: def github_run(tmp_path: Path, refuses: str, tree: dict = SOURCE_TREE, keep_contracts: bool = False, merge_stderr: bool = False, stdout_closed: bool = False, branch: str = "main", - plan_flags: tuple = ()): + plan_flags: tuple = (), before_plan=None): """Plan a small source and `execute` it in GitHub mode, with a `gh` that refuses `refuses`. Returns `(result, github)`: the run, and the directory that stands for github.com, with `testorg/.git` in it. @@ -222,7 +222,9 @@ def github_run(tmp_path: Path, refuses: str, tree: dict = SOURCE_TREE, tool flushed them. `stdout_closed` gives the run a stdout whose reading end is already closed, as `| head` leaves it once `head` has quit, so that `result.stdout` is None. `branch` is the source's own (default) branch, - and `plan_flags` go to `plan` after the ones `write_plan` passes.""" + and `plan_flags` go to `plan` after the ones `write_plan` passes. + `before_plan`, when given, is called with the source and the directory + that stands for github.com once the assembly is there, before `plan`.""" github = tmp_path / "github" (github / ORG).mkdir(parents=True) source = make_source_repo(tmp_path / NORTHWIND, tree=tree, edits=(), @@ -231,6 +233,8 @@ def github_run(tmp_path: Path, refuses: str, tree: dict = SOURCE_TREE, # split branch is pushed back to it. git("clone", "-q", "--bare", ".", str(github / ORG / f"{NORTHWIND}.git"), cwd=source) + if before_plan is not None: + before_plan(source, github) bin_dir = tmp_path / "bin" bin_dir.mkdir() @@ -632,6 +636,22 @@ def test_a_topic_refused_on_one_repository_names_that_one_alone(tmp_path): # --- the pull request's base is a branch the source has (#479) -------------- +TRACKING_MISSING = "plan-tracking-branch-missing" +#: The assembly's url, as `execute` names it. +ASSEMBLY_URL = f"`https://github.com/{ORG}/{NORTHWIND}.git`" + + +def assert_refused_before_any_repository(result, github: Path, + code: str) -> None: + """`execute` refused as `code`, saying it had made nothing, and it had + not: no `gh` was run and no leg repository is on the fake GitHub.""" + assert result.returncode == 2, result.stdout + result.stderr + assert f"REFUSED {code}: " in result.stderr, result.stderr + assert "no leg repository was created" in result.stderr, result.stderr + assert gh_calls(github) == [] + assert not (github / ORG / f"{NORTHWIND}-spec.git").exists() + + @WINDOWS_SKIP # the `gh` is a `#!` script, which Windows does not run @needs_filter_repo @pytest.mark.parametrize("branch", ["master", "trunk"]) @@ -666,14 +686,86 @@ def test_a_base_the_source_lacks_is_refused_before_any_repository(tmp_path): result, github = github_run(tmp_path, "never", branch="master", plan_flags=("--tracking-branch", "main")) - assert result.returncode == 2, result.stdout + result.stderr - assert "REFUSED plan-tracking-branch-missing:" in result.stderr + assert_refused_before_any_repository(result, github, TRACKING_MISSING) assert ("`tracking_branch` is `main`, which is no branch of the source " "(its branches: `master`; the source's default branch is " "`master`)") in result.stderr, result.stderr - assert "no leg repository was created" in result.stderr - assert gh_calls(github) == [] - assert not (github / ORG / f"{NORTHWIND}-spec.git").exists() + + +# --- the remote is asked, before any repository (#493's review, F1) --------- + +def renamed_on_github(source: Path, github: Path) -> None: + """The source becomes a clone made while its repository's default branch + was `master`, and GitHub then renames `master` to `main`. An ordinary + `git fetch` does not prune, so the clone keeps `origin/master`, and + `origin/HEAD` still names it, beside the `origin/main` the fetch brings.""" + assembly = github / ORG / f"{NORTHWIND}.git" + git("remote", "add", "origin", str(assembly), cwd=source) + git("fetch", "-q", "origin", cwd=source) + git("remote", "set-head", "origin", "master", cwd=source) + git("branch", "-m", "master", "main", cwd=assembly) + git("symbolic-ref", "HEAD", "refs/heads/main", cwd=assembly) + git("fetch", "-q", "origin", cwd=source) + + +@WINDOWS_SKIP # the `gh` is a `#!` script, which Windows does not run +@needs_filter_repo +def test_a_branch_the_clone_remembers_and_github_renamed_is_refused_first( + tmp_path): + """`check` reads the branches the source's clone remembers, and passes + `master`, which `plan` took from `origin/HEAD`. `execute` asks the + remote itself before it makes anything, and refuses: it used to push + both legs and the split, and then GitHub refused the pull request, + `Base ref must be a branch`, where before #479 the same clone planned + `main` and adopted. The way out it names, the clone pruned and told its + remote's default branch, and its own branch renamed to match, makes a + plan that tracks `main` and checks ok.""" + result, github = github_run(tmp_path, "never", branch="master", + before_plan=renamed_on_github) + + assert_refused_before_any_repository(result, github, TRACKING_MISSING) + assert (f"`tracking_branch` is `master`, which {ASSEMBLY_URL}, the " + "repository the split is pushed to, does not have as `git " + "ls-remote` reads it now") in result.stderr, result.stderr + said = " ".join(result.stderr.split()) + assert ("run `git fetch --prune origin` and `git remote set-head origin " + "--auto`, and where the default branch was renamed, rename the " + "clone's own to match, as GitHub's notice of the rename says (`git " + "branch -m `). Then re-run `plan`") in said, said + + source = tmp_path / NORTHWIND + git("fetch", "-q", "--prune", "origin", cwd=source) + git("remote", "set-head", "origin", "--auto", cwd=source) + git("branch", "-m", "master", "main", cwd=source) + replanned = tmp_path / "replanned.yaml" + written = write_plan(source, replanned) + assert written.returncode == 0, written.stdout + written.stderr + assert "\ntracking_branch: main\n" in replanned.read_text(encoding="utf-8") + checked = run_script(ADOPT, "check", "--plan", str(replanned)) + assert checked.returncode == 0, checked.stdout + checked.stderr + + +def moved_off_github(source: Path, github: Path) -> None: + """The assembly is no longer at the url `execute` pushes the split to.""" + assembly = github / ORG / f"{NORTHWIND}.git" + assembly.rename(assembly.with_name("Elsewhere.git")) + + +@WINDOWS_SKIP # the `gh` is a `#!` script, which Windows does not run +@needs_filter_repo +def test_a_remote_that_cannot_be_asked_is_refused_before_any_repository( + tmp_path): + """The question fails closed: a remote `git ls-remote` cannot read is + refused as such, before any leg exists, since the split push needs the + same remote.""" + result, github = github_run(tmp_path, "never", + before_plan=moved_off_github) + + assert_refused_before_any_repository(result, github, + "adopt-remote-unreadable") + assert (f"`git ls-remote` could not read {ASSEMBLY_URL}, the repository " + "the split is pushed to, to ask whether it has `main`, the " + "plan's `tracking_branch`: exit ") in result.stderr, result.stderr # --- the real `gh` stays out of reach --------------------------------------- diff --git a/tests/test_adopt_tracking_branch.py b/tests/test_adopt_tracking_branch.py index 6519129d..68e64846 100644 --- a/tests/test_adopt_tracking_branch.py +++ b/tests/test_adopt_tracking_branch.py @@ -17,10 +17,12 @@ request goes to, and its own otherwise. The GitHub-mode runs, a `master` and a `trunk` source whose pull request opens -against that branch and a base the source lacks refused before any `gh` runs, -are in `tests/test_adopt_github_exits.py`, beside the other exits a GitHub run -has. Nothing here runs `gh`: every run below is `plan` and `check`, which read -a local source, or `execute --local-remote-dir`, which refuses before any leg. +against that branch, a base the source lacks refused before any `gh` runs, and +one the clone remembers after GitHub renamed it, which `execute` asks the +remote about, are in `tests/test_adopt_github_exits.py`, beside the other exits +a GitHub run has. Nothing here runs `gh`: every run below is `plan` and +`check`, which read a local source, or `execute --local-remote-dir`, which +refuses before any leg. """ from __future__ import annotations @@ -41,6 +43,11 @@ "docs/spec.md": "# Spec\n"} MISSING = "plan-tracking-branch-missing" REMEDIATION = "Remediation: set `tracking_branch:` in the plan" +#: How the plan's last follow-up names the pull request's base: by the plan's +#: key, never its value (#493's review, F3). +PULL_REQUEST_FOLLOW_UP = ( + "open the split as a pull request against the plan's `tracking_branch`, " + "the branch the assembly root and both legs track;") def planned(tmp_path, source, *flags: str): @@ -71,8 +78,9 @@ def retracked(plan, old: str, new: str) -> None: def test_plan_records_the_branch_it_reads_the_source_at(tmp_path): """No flag: the source's own branch, `master`, which `plan` also records - as `source.default_branch`, and the last follow-up names it. An explicit - `--tracking-branch` still wins, and the follow-up names that instead.""" + as `source.default_branch`, and the last follow-up names the plan's + `tracking_branch` as the pull request's base. An explicit + `--tracking-branch` still wins.""" source = make_source_repo(tmp_path / "Northwind", tree=TREE, edits=(), branch="master") git("branch", "develop", cwd=source) @@ -81,20 +89,40 @@ def test_plan_records_the_branch_it_reads_the_source_at(tmp_path): written = load_yaml(plan) assert written["source"]["default_branch"] == "master" assert written["tracking_branch"] == "master", written - assert written["follow_ups"][-1].startswith( - "open the split as a pull request against master, the branch the " - "assembly root and both legs track;"), written["follow_ups"] + assert written["follow_ups"][-1].startswith(PULL_REQUEST_FOLLOW_UP), \ + written["follow_ups"] assert MISSING not in result.stdout + result.stderr result, plan = planned(tmp_path / "explicit", source, "--tracking-branch", "develop") assert tracking_branch_of(plan) == "develop" - assert load_yaml(plan)["follow_ups"][-1].startswith( - "open the split as a pull request against develop,") assert MISSING not in result.stdout + result.stderr assert check(plan).returncode == 0 +def test_the_follow_up_names_no_branch_a_hand_edit_can_leave_behind(tmp_path): + """The follow-ups are written once, by `plan`, and `execute` copies them + into the split commit, which is also the pull request's body. The last + one named the branch, so a plan made with `--tracking-branch develop` + and then edited to `master`, as `plan-tracking-branch-missing`'s + remediation asks, checked ok saying `develop`, and its pull request + opened against `master` with a body saying `develop` (#493's review, + F3). It names the plan's key now, which no edit leaves behind.""" + source = make_source_repo(tmp_path / "Northwind", tree=TREE, edits=(), + branch="master") + git("branch", "develop", cwd=source) + _, plan = planned(tmp_path, source, "--tracking-branch", "develop") + retracked(plan, "develop", "master") + + result = check(plan) + + assert result.returncode == 0, result.stdout + result.stderr + (said,) = [line for line in result.stdout.splitlines() + if "open the split as a pull request" in line] + assert said.startswith(f" follow-up: {PULL_REQUEST_FOLLOW_UP}"), said + assert "develop" not in said and "master" not in said, said + + def test_check_finds_a_tracking_branch_the_source_does_not_have(tmp_path): """A plan whose `tracking_branch:` was edited to a branch the source does not have is a finding of `check`, naming the value, the branches the From 56bb04daa4b86938baee9dd2ac7dfaff55631791 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Tue, 13 Oct 2026 00:39:41 +0000 Subject: [PATCH 3/5] tests/test_ruleset_hint_tracking_branch.py: an adoption's source has the tracking branch beside `master`, so `execute` reaches the leg push whose ruleset hint the test reads instead of refusing `plan-tracking-branch-missing` (#479, #480, #481) #373's `refused_run` built the adoption source on `master` alone and planned `--tracking-branch trunk` or `main`. With #479's `plan-tracking-branch-missing`, `execute` refuses that plan before any leg, so the four `adopt-*` cases of `test_an_adoption_hint_names_the_tracking_branch_the_run_was_made_with` found no hint (RV493's verification, N1). The source now gets `git branch ` beside `master`; it stays on `master`, so the test's `"master" not in printed` assertion still means what it says. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Opus 5.5 --- tests/test_ruleset_hint_tracking_branch.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_ruleset_hint_tracking_branch.py b/tests/test_ruleset_hint_tracking_branch.py index d8fca250..3d34a1ff 100644 --- a/tests/test_ruleset_hint_tracking_branch.py +++ b/tests/test_ruleset_hint_tracking_branch.py @@ -307,7 +307,8 @@ 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. An adoption's source - is on `master`, whatever the tracking branch is.""" + is on `master`, with the tracking branch beside it, since `execute` + refuses a tracking branch the source lacks (#479).""" refused = CASES[case][0] env = refusing(template, refused) remotes, work = tmp_path / "remotes", tmp_path / "work" @@ -325,6 +326,7 @@ def refused_run(case: str, tracking: str, tmp_path, template): else: source = make_source_repo(tmp_path / "IRRS", SPEC_ONLY_TREE, branch="master", edits=EDITS) + git("branch", tracking, cwd=source) plan = tmp_path / "adoption-plan.yaml" written = write_plan(source, plan, project="Redwood", extra=("--tracking-branch", tracking, From 7af1a3c0c2b726c68894223071133de77c9601fe Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Tue, 13 Oct 2026 02:18:31 +0000 Subject: [PATCH 4/5] adopt-project.py: the tracking-branch lookup that `execute` makes of the assembly's remote fails a repository git cannot list as a `CommandFailed`, as #382's `_refuse_a_split_branch_left_behind` does, and says itself as `REFUSED adopt-remote-unreadable`, so the two refusals compose and `test_execute_refuses_a_repository_it_cannot_list` and `test_a_remote_that_cannot_be_asked_is_refused_before_any_repository` both hold (#479, #320) `RemoteUnreadable` is a `CommandFailed` whose `loudly`, the one place `main` asks how a failed command is said, returns the `adopt-remote-unreadable` refusal. The `OSError` case keeps the plain `Refusal`: it has no command. Co-Authored-By: Claude Sonnet 5.5 --- adopt-project.py | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index e220fc14..2fbe2eb9 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -1914,6 +1914,28 @@ def _tracking_branch_notes(args, source: Source) -> list[str]: " origin/` makes it one.") +class RemoteUnreadable(CommandFailed): + """The `git ls-remote` that could not read the repository the split is + pushed to, said as `adopt-remote-unreadable` (#479 with #320). + + A repository git cannot list fails `execute`'s listings as `CommandFailed`, + as `_refuse_a_split_branch_left_behind` and `_branches_at` do (#320), and + what catches that, or tests it, takes the command from `args_list`. The + tracking branch is the first listing now, and it is refused as + `REFUSED adopt-remote-unreadable`, with what it left (#493's review, F1). + Both hold of this one exception: it IS the failed command, and `loudly`, + the one place `main` asks how a failed command is said, is the refusal. + """ + + def __init__(self, failed: CommandFailed, refusal: Refusal): + super().__init__(failed.args_list, failed.cwd, failed.code, + failed.output) + self.refusal = refusal + + def loudly(self, what: str) -> str: + return str(self.refusal) + + def _refuse_a_tracking_branch_the_remote_lacks(urls: dict, tracking: str, local: bool) -> None: """Refuse a tracking branch the repository the split goes to does not @@ -1944,7 +1966,7 @@ def _refuse_a_tracking_branch_the_remote_lacks(urls: dict, tracking: str, except (CommandFailed, OSError) as exc: why = (f"exit {exc.code}: {' '.join(exc.output.split())}" if isinstance(exc, CommandFailed) else str(exc)) - raise Refusal( + refusal = Refusal( REMOTE_UNREADABLE, f"`git ls-remote` could not read {remote}, the repository the " f"split is pushed to, to ask whether it has {_spelled(tracking)}, " @@ -1954,7 +1976,13 @@ def _refuse_a_tracking_branch_the_remote_lacks(urls: dict, tracking: str, "needs it as readable as this question does, so the run stops " "here and not once both legs exist. Make it readable from here " "-- its url, the network, the credentials git uses for it -- and " - "run `execute` again.") from exc + "run `execute` again.") + # A failed command stays one (`RemoteUnreadable`), so that this + # listing fails as `_refuse_a_split_branch_left_behind`'s does when + # it is the first to ask; an `OSError` has no command to be. + if isinstance(exc, CommandFailed): + raise RemoteUnreadable(exc, refusal) from exc + raise refusal from exc if any(line.split("\t")[-1] == ref for line in listed.splitlines()): return raise Refusal( From 6b408cad9f56db0b37e59fdab3ff1b12202a88dc Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Tue, 13 Oct 2026 02:26:13 +0000 Subject: [PATCH 5/5] tests/test_adopt_split_branch.py: `test_execute_refuses_a_repository_it_cannot_list` expects #493's `adopt-remote-unreadable` for a remote git cannot list, with the failed `git ls-remote` as its cause, and adopt-project.py drops `RemoteUnreadable`, so #382's test and #493's lookup no longer need a refusal that is also a `CommandFailed` (#479, #320) The first lookup to list the assembly's remote is now `_refuse_a_tracking_branch_the_remote_lacks`, not `_refuse_a_split_branch_left_behind`. The test asserts a `Refusal` with code `adopt-remote-unreadable`, a detail that names the url, the "no leg repository was created" remediation, and a `CommandFailed` cause whose `args_list` starts `git ls-remote` and holds the url. adopt-project.py is back to its 433a1b40 text. Co-Authored-By: Claude Sonnet 5.5 --- adopt-project.py | 32 ++------------------------------ tests/test_adopt_split_branch.py | 21 ++++++++++++++++----- 2 files changed, 18 insertions(+), 35 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index 2fbe2eb9..e220fc14 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -1914,28 +1914,6 @@ def _tracking_branch_notes(args, source: Source) -> list[str]: " origin/` makes it one.") -class RemoteUnreadable(CommandFailed): - """The `git ls-remote` that could not read the repository the split is - pushed to, said as `adopt-remote-unreadable` (#479 with #320). - - A repository git cannot list fails `execute`'s listings as `CommandFailed`, - as `_refuse_a_split_branch_left_behind` and `_branches_at` do (#320), and - what catches that, or tests it, takes the command from `args_list`. The - tracking branch is the first listing now, and it is refused as - `REFUSED adopt-remote-unreadable`, with what it left (#493's review, F1). - Both hold of this one exception: it IS the failed command, and `loudly`, - the one place `main` asks how a failed command is said, is the refusal. - """ - - def __init__(self, failed: CommandFailed, refusal: Refusal): - super().__init__(failed.args_list, failed.cwd, failed.code, - failed.output) - self.refusal = refusal - - def loudly(self, what: str) -> str: - return str(self.refusal) - - def _refuse_a_tracking_branch_the_remote_lacks(urls: dict, tracking: str, local: bool) -> None: """Refuse a tracking branch the repository the split goes to does not @@ -1966,7 +1944,7 @@ def _refuse_a_tracking_branch_the_remote_lacks(urls: dict, tracking: str, except (CommandFailed, OSError) as exc: why = (f"exit {exc.code}: {' '.join(exc.output.split())}" if isinstance(exc, CommandFailed) else str(exc)) - refusal = Refusal( + raise Refusal( REMOTE_UNREADABLE, f"`git ls-remote` could not read {remote}, the repository the " f"split is pushed to, to ask whether it has {_spelled(tracking)}, " @@ -1976,13 +1954,7 @@ def _refuse_a_tracking_branch_the_remote_lacks(urls: dict, tracking: str, "needs it as readable as this question does, so the run stops " "here and not once both legs exist. Make it readable from here " "-- its url, the network, the credentials git uses for it -- and " - "run `execute` again.") - # A failed command stays one (`RemoteUnreadable`), so that this - # listing fails as `_refuse_a_split_branch_left_behind`'s does when - # it is the first to ask; an `OSError` has no command to be. - if isinstance(exc, CommandFailed): - raise RemoteUnreadable(exc, refusal) from exc - raise refusal from exc + "run `execute` again.") from exc if any(line.split("\t")[-1] == ref for line in listed.splitlines()): return raise Refusal( diff --git a/tests/test_adopt_split_branch.py b/tests/test_adopt_split_branch.py index e90a938d..d0c52dba 100644 --- a/tests/test_adopt_split_branch.py +++ b/tests/test_adopt_split_branch.py @@ -45,6 +45,7 @@ TAKEN = "plan-split-branch-taken" BRANCH_EXISTS = "adopt-branch-exists" +REMOTE_UNREADABLE = "adopt-remote-unreadable" UNREADABLE = "plan-gitmodules-unreadable" SPLIT_BRANCH = "adopt/three-repo-shape" PROJECT, ORG = "Northwind", "testorg" @@ -519,16 +520,26 @@ def test_execute_refuses_a_repository_it_cannot_list(adopter, tmp_path, monkeypatch): """No `testorg/Northwind` where git is pointed: the push of the split would fail there after both legs were made, so nothing is made. The - first to list it is `_refuse_a_split_branch_left_behind`, as on main, - and its `git ls-remote` fails the run before `gh repo create`.""" + first to list it is `_refuse_a_tracking_branch_the_remote_lacks` (#479), + which asks it for the tracking branch before + `_refuse_a_split_branch_left_behind` asks for the split branch. It refuses + what `git ls-remote` cannot read as `adopt-remote-unreadable`, before + `gh repo create`, and the failed command is the refusal's `__cause__`.""" source = small_source(tmp_path) plan = plan_for(source, tmp_path) github_at(tmp_path / "nowhere", monkeypatch) asked = no_gh(adopter, monkeypatch) - with pytest.raises(adopter.CommandFailed) as caught: + with pytest.raises(adopter.Refusal) as caught: adopter.cmd_execute(execute_args(plan, tmp_path, local=False)) - assert caught.value.args_list[:2] == ["git", "ls-remote"] - assert f"https://github.com/{ORG}/{PROJECT}.git" in caught.value.args_list + assert caught.value.code == REMOTE_UNREADABLE + url = f"https://github.com/{ORG}/{PROJECT}.git" + assert caught.value.detail.startswith( + f"`git ls-remote` could not read `{url}`, "), caught.value.detail + assert "no leg repository was created" in caught.value.remediation + cause = caught.value.__cause__ + assert isinstance(cause, adopter.CommandFailed), cause + assert cause.args_list[:2] == ["git", "ls-remote"] + assert url in cause.args_list assert asked == []