From 76e680b1ed13adeada35151d647ad2b43170555b Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Thu, 15 Oct 2026 00:03:56 +0000 Subject: [PATCH 1/2] adopt-project.py: what `plan`, `execute` and AGENTS.md say around a run is what the run does: the plan's last follow-up is a review of the pull request `execute` opens itself, a failed verification names the pull request and the topic it left in place, AGENTS.md says `check` passes an answer with no `resolution:`, NEXT under `--local-remote-dir` names the split's branch, and the ruleset hint no longer calls a seed branch and a pull request the scaffold's other exit (#523) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - N1: the plan's last follow-up said "open the split as a pull request …; the legs -spec and -code are seeded before it", and the split commit and the pull request's body list it under the FOLLOW-UPS the commit does NOT do, though against GitHub `execute` opens that pull request itself, and the legs are extracted. It is now a review of the root's diff with the two new repositories `execute` creates and pushes before the split, and says `execute` opens the pull request itself, still naming its base by the plan's `tracking_branch` key (#493's review, F3). - auA-12: against GitHub `execute` sets the topic and opens the pull request before it verifies, so a failed verification leaves both in place, and neither the exit-1 text nor AGENTS.md said so (AGENTS.md said "the source is untouched"). `_finish_by_hand` now names the open pull request as one for a failed split, which deleting its branch (the way out's command) closes, and the topic as staying set; AGENTS.md step 7 says the split branch is pushed and, on GitHub, its pull request may be open and the topic set. - N3: AGENTS.md step 2 said an answer with no reason "will be read as a finding"; `check` never reads `resolution:` and says `plan ok`. It now says `check` passes it and the human reads the reasons at step 4. - Ruleset hint: `ADOPT_RULESET_HINT` called a seed branch and a pull request "the scaffold's other exit", which #506 removed from the scaffold. It names that route only as what cannot finish an adoption. - N2 (rides along): under `--local-remote-dir` no pull request is opened, yet NEXT said "review the pull request" and cloned the default branch. It names the split on its branch and clones that branch. AGENTS.md stays at its 490-line cap: the two-line "`execute` opens a pull request …" paragraph is folded, nearly word for word, into step 4. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 20 +-- adopt-project.py | 138 +++++++++++++++------ tests/test_adopt_e2e.py | 37 ++++-- tests/test_adopt_github_exits.py | 70 +++++++++++ tests/test_adopt_late_failure.py | 13 +- tests/test_adopt_plan.py | 32 +++++ tests/test_adopt_tracking_branch.py | 28 +++-- tests/test_ruleset_hint_tracking_branch.py | 12 +- 8 files changed, 277 insertions(+), 73 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e579245c..b3b4ae2b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -132,21 +132,24 @@ install command). Seven steps, in order, and step 4 is the one you must not skip entry carries a `question:`. Set `leg:` to `spec`, `code`, `root` or `drop` AND add a `resolution:` line saying why — read the repository to answer it (does the specification cite `examples/golden-run/`, or do the tests?). An - answer with no reason beside it is a guess that will be read as a finding. - Never delete an entry to make the question go away: `check` refuses a path - that no entry covers, and it is right to. + answer with no reason is a guess, and `check` passes it: the human reads the + reasons at step 4. Never delete an entry to make the question go away: + `check` refuses a path that no entry covers, and it is right to. 4. **Show the human the plan and the follow-ups and let them say yes.** The follow-ups are code changes the split MAKES NECESSARY — the harness that reads `contracts/` from beside it will read it across the root mount — and - `execute` does not make them. Do not pass `--yes` on your own initiative; - it is the human's word, not your convenience. + `execute` does not make them; it opens the split's pull request itself, + because these organisations are pull-request only. Never suggest a direct + push to the default branch, and never `--admin`. Do not pass `--yes` on your + own initiative; it is the human's word, not your convenience. 7. **Read the verification table back to them.** It accounts for every source path at the source commit, by blob sha (a kept root `.gitattributes` as the source's bytes plus the line-ending block the split appends), in exactly one of {spec leg, code leg, root tree} or as `drop`. If it exits non-zero, STOP and relay it, and take the way out THAT exit prints. Exit 1, a failed - verification: the source is untouched — nothing is deleted, `main` is where - it was, the split is on a branch — so fix the plan, delete the split branch, + verification: nothing is deleted and `main` is where it was, but the split + branch is pushed and, on GitHub, its pull request may be open and the topic + set — so fix the plan, delete the split branch (closing that pull request), and re-run into fresh legs and a fresh `--work-dir`, or none. Exit 2 saying `IS pushed` (GitHub only: `gh pr create` or the topic refused) comes after both legs and the split are pushed and verified: finish with the command it @@ -157,9 +160,6 @@ the split's `project.yaml`, under `submodules:` — per submodule a leg keeps, its `path`, `leg`, `resolution:` and `source_commit`. It confers nothing, nothing rewrites it, and the doctor holds each submodule to it. -`execute` opens a pull request, because these organisations are pull-request -only. Never suggest a direct push to the default branch, and never `--admin`. - ### Adopting a repository that has NO code yet (or no spec) A leg no plan entry assigns a path to is SEEDED from `templates/-root/`, diff --git a/adopt-project.py b/adopt-project.py index e4fa0451..889f28c8 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -141,7 +141,9 @@ 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. +command to open the pull request or to set the topic; it says what the run +already put in place, the pull request open for the failed split, which +deleting the split branch closes, and the topic, which stays set (#523). """ from __future__ import annotations @@ -1718,6 +1720,16 @@ def follow_ups_for(source: "Source", entries: list["Entry"], names: dict, 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). + + It is a review, and never "open the split as a pull request" (#523). + `plan` prints it and the split commit lists it under the FOLLOW-UPS this + commit does NOT do, which is also the pull request's body, while against + GitHub `execute` opens that pull request itself, and under + `--local-remote-dir` opens none. So it says what `execute` does in either + run, and calls the legs what they are: new repositories `execute` creates + and pushes before the split, extracted or, with no path, seeded from a + template. "Seeded", said of both, named an extracted leg by the word kept + for a template's. """ out = cross_leg_references(source, entries, spec_path, code_path) if any(e.leg == "spec" and (e.path == "contracts/" @@ -1734,10 +1746,11 @@ 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 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") + f"review the root's diff together with {names['spec']} and " + f"{names['code']}, the two new repositories `execute` creates and " + "pushes before the split; against GitHub it opens the split's pull " + "request itself, whose base is the plan's `tracking_branch`, the " + "branch the assembly root and both legs track") return out @@ -6684,29 +6697,31 @@ def _refuse_an_unrunnable_plan(plan: Plan, source: Source) -> None: #: The adoption's own ruleset hint (#177). `RULESET_HINT` is the scaffold's, -#: and it stays the scaffold's word for word. ITS SECOND EXIT IS NOT OFFERED -#: HERE, because it cannot finish an adoption (#188 N2). It pushed the leg to -#: a seed branch and opened a pull request against the tracking branch, but -#: the leg is a repository this run created and its first push was the one -#: refused, so there is no tracking branch for that pull request to target; -#: and even a merged one would resume nothing, since the rest of the adoption -#: (the other leg, the split) is made only by a re-run, into new legs. So an -#: adoption names its one exit, a re-run by somebody holding the bypass right, -#: and the notice printed after this hint (`_nothing_rolled_back`) names what -#: that re-run must clear first. The notice also says to fix what failed -#: before the re-run, and under such a ruleset the bypass IS that fix: the -#: hint says so last, or "fixed", read alone, would invite a change to the -#: ruleset this hint forbids (#188's re-verification, R4). +#: and it stays the scaffold's word for word. A SEED BRANCH AND A PULL +#: REQUEST ARE NAMED ONLY AS WHAT CANNOT FINISH AN ADOPTION (#188 N2). That +#: route pushed the leg to a seed branch and opened a pull request against the +#: tracking branch, but the leg is a repository this run created and its first +#: push was the one refused, so there is no tracking branch for that pull +#: request to target; and even a merged one would resume nothing, since the +#: rest of the adoption (the other leg, the split) is made only by a re-run, +#: into new legs. Nor is it "the scaffold's other exit", as this hint called +#: it until #523: #506 took it out of the scaffold's hint too, which names it +#: only as what cannot finish a scaffold. So an adoption names its one exit, a +#: re-run by somebody holding the bypass right, and the notice printed after +#: this hint (`_nothing_rolled_back`) names what that re-run must clear +#: first. The notice also says to fix what failed before the re-run, and +#: under such a ruleset the bypass IS that fix: the hint says so last, or +#: "fixed", read alone, would invite a change to the ruleset this hint +#: forbids (#188's re-verification, R4). ADOPT_RULESET_HINT = """ If the organisation applies a ruleset requiring changes to arrive by pull request, a direct push to the default branch is refused BY DESIGN and must not be worked around. An adoption has ONE legitimate exit: have an operator holding the bypass right re-run it, once what the notice below names is -cleared. The scaffold's other exit, a seed branch and a pull request, cannot -finish an adoption: {repo} has no {tracking} for that pull request to target, -and nothing would run the rest of this adoption after it was merged. Under -such a ruleset, that bypass is the fix the notice below asks for, never a -change to the ruleset. +cleared. A seed branch and a pull request cannot finish an adoption: +{repo} has no {tracking} for that pull request to target, and nothing would +run the rest of this adoption after it was merged. Under such a ruleset, that +bypass is the fix the notice below asks for, never a change to the ruleset. """ #: What a step that runs once a leg repository exists reports as a REFUSAL, @@ -7812,14 +7827,19 @@ def _open_the_pull_request(repositories: dict, names: dict, tracking: str, def _finish_by_hand(passed: bool, refused: list[str] | None, topic: str, - unset: list[str]) -> None: + unset: list[str], *, opened: str | None = None, + carried: list[str] | None = None) -> 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`. + `_pull_request_command` built it, and None when the pull request is open + or none was tried (`--local-remote-dir`); `unset` names each repository + `_set_the_topic` could not give `topic`. `opened` is the repository the + split's pull request IS open on, None where none is, and `carried` each + repository that has the topic: what a failed verification leaves in + place, and says (#523). 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 @@ -7836,6 +7856,17 @@ def _finish_by_hand(passed: bool, refused: list[str] | None, topic: str, 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. + + WHAT IT LEAVES IN PLACE IS SAID TOO (#523). `execute` sets the topic and + opens the pull request BEFORE it verifies, so against GitHub a split that + failed has its pull request open on the source and the topic on every + repository that took it, and the way out said neither: it said NOTHING + was deleted from the source, which is true, and AGENTS.md said the source + was untouched, which is not. So the open pull request is named as one + for a split that failed, with what closes it, the deletion of its branch + the way out already names; and the topic as staying set, which the + re-run sets again. + 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, @@ -7843,9 +7874,19 @@ def _finish_by_hand(passed: bool, refused: list[str] | None, topic: str, """ _flush_stdout() if not passed: + said = [] 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) + said.append("No pull request was opened, and none should be: the " + "split above did not pass verification.") + elif opened is not None: + said.append(f"A pull request for that branch IS open on {opened}, " + "for a split that did not pass verification: " + "deleting the branch, as above, closes it.") + if carried: + said.append(f"The topic {topic} stays set on " + f"{_and_listed(carried)}; the re-run sets it again.") + if said: + print(" ".join(said), file=sys.stderr) return topics = "".join(f"\n gh repo edit {repository} --add-topic {topic}" for repository in unset) @@ -8275,13 +8316,17 @@ def cmd_execute(args) -> int: # 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] = [] + carried: list[str] = [] refused: list[str] | None = None + opened: str | None = None if not local: 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): + if _open_the_pull_request(repositories, names, tracking, branch, + message): + opened = repositories["assembly"] + else: refused = _pull_request_command(repositories, names, tracking, branch, message) @@ -8291,9 +8336,12 @@ def cmd_execute(args) -> int: urls=urls, branch=branch) if not verified and refused is None: # 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) + # materializer reports it, and not a second look at the tree (#480); + # under `--local-remote-dir` no pull request is opened, so NEXT names + # the split's branch instead (#523). + _next_steps(names, collisions, branch if local else None) + _finish_by_hand(not verified, refused, topic, unset, opened=opened, + carried=carried) if verified: # A verification mismatch outranks a refused pull request and a # missing topic. @@ -8808,10 +8856,18 @@ def _left_by_the_split(source: Source, urls: dict, names: dict, SHAPE_MAKEFILE = "Makefile" -def _next_steps(names: dict, collisions: list) -> None: +def _next_steps(names: dict, collisions: list, + branch: str | None = None) -> 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). + open, or none was tried. Not said after a refused `gh pr create`: there + the command to open it is the next step (#304). + + `branch` is the split's branch where no pull request was tried, under + `--local-remote-dir`. NEXT said "review the pull request" there as well, + over none (#523, N2), and named a clone of the default branch, where the + split is not. There it names the split on its branch, and the clone of + that branch, quoted for `sh` as `_branch_deletion` quotes it. Without + one, the block is what it was, byte for byte. `collisions` is each `(intended, actual)` the materializer wrote beside a file the source kept: where the shape's files WENT, and not a second @@ -8834,8 +8890,14 @@ def _next_steps(names: dict, collisions: list) -> None: 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>" + if branch is None: + review, clone = f"the pull request on {names['assembly']}", "" + else: + review = (f"the split on its branch in {names['assembly']} (no pull " + "request is opened under --local-remote-dir)") + clone = f"-b {shlex.quote(branch)} " + print(f"\nNEXT: review {review}, then\n" + f" git clone --recurse-submodules {clone}<{names['assembly']} url>" f"{bootstrap}") if script: kept = [path for path in (SHAPE_MAKEFILE, BOOTSTRAP) diff --git a/tests/test_adopt_e2e.py b/tests/test_adopt_e2e.py index 77f2147d..db88cab3 100644 --- a/tests/test_adopt_e2e.py +++ b/tests/test_adopt_e2e.py @@ -221,6 +221,16 @@ def next_block(stdout: str) -> list[str]: return stdout.split("\nNEXT: ", 1)[1].splitlines() +#: NEXT's first two lines after `execute --local-remote-dir`, which opens no +#: pull request: the split on its branch, and the clone of that branch, where +#: the split is. They said "review the pull request", over none, and cloned +#: the default branch (#523, N2). +LOCAL_NEXT = [f"review the split on its branch in {PROJECT} (no pull request " + "is opened under --local-remote-dir), then", + " git clone --recurse-submodules -b adopt/three-repo-shape " + f"<{PROJECT} url>"] + + 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` @@ -230,9 +240,7 @@ def test_next_names_the_bootstrap_a_root_with_its_own_makefile_can_run( 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] == [*LOCAL_NEXT, " 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() @@ -291,6 +299,23 @@ def test_next_keeps_make_bootstrap_where_the_shapes_makefile_is_the_roots( " make bootstrap\n") +def test_next_names_the_split_branch_where_no_pull_request_was_opened( + adopter, capsys): + """Given the split's branch, as `execute --local-remote-dir` gives it, + NEXT names no pull request and clones that branch, quoted for `sh`; the + rest of the block is unchanged (#523, N2).""" + adopter._next_steps({"assembly": PROJECT}, [], "adopt/three repo") + + out = capsys.readouterr().out + assert out == ( + f"\nNEXT: review the split on its branch in {PROJECT} (no pull " + "request is opened under --local-remote-dir), then\n" + " git clone --recurse-submodules -b 'adopt/three repo' " + f"<{PROJECT} url>\n" + " make bootstrap\n") + assert "review the pull request" not in out + + #: 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. @@ -341,10 +366,8 @@ def test_next_names_no_bootstrap_where_the_roots_script_is_the_sources_own( 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 + assert next_block(result.stdout) == [*LOCAL_NEXT, BOTH_ASIDE], \ + result.stdout def test_next_names_no_bootstrap_where_only_the_script_went_aside( diff --git a/tests/test_adopt_github_exits.py b/tests/test_adopt_github_exits.py index 5c757f12..6bf16930 100644 --- a/tests/test_adopt_github_exits.py +++ b/tests/test_adopt_github_exits.py @@ -60,6 +60,7 @@ from conftest import (ADOPT, REPO, WINDOWS_SKIP, git, make_source_repo, run_script, write_plan) +from test_adopt_tracking_branch import OPEN_THE_SPLIT, PULL_REQUEST_FOLLOW_UP #: `execute` REQUIRES `git filter-repo`; without it the runs below are #: skipped, as every `execute` test in this suite is. @@ -106,6 +107,16 @@ #: verification: the refusal above names the exact command that was tried. NOT_BY_HAND = ("No pull request was opened, and none should be: the split " "above did not pass verification.") +#: What a failed verification says of what the run put in place on GitHub +#: BEFORE it verified (#523, auA-12): the pull request it opened, which the +#: branch deletion the way out names closes, and the topic, on each +#: repository that took it. +STILL_OPEN = (f"A pull request for that branch IS open on {ORG}/{NORTHWIND}, " + "for a split that did not pass verification: deleting the " + "branch, as above, closes it.") +TOPIC_STAYS = (f"The topic {TOPIC} stays set on {ORG}/{NORTHWIND}, " + f"{ORG}/{NORTHWIND}-spec and {ORG}/{NORTHWIND}-code; the " + "re-run sets it again.") VERIFIED = "adoption verified: every source path is in exactly one place" FINDING = "FINDING adopt-shape-file-missing:" #: What `_verify` used to say after every pass, whether or not a pull request @@ -369,6 +380,23 @@ def test_step_7_keys_its_advice_to_the_exit_and_not_to_non_zero(): assert said.index("Exit 1") < said.index("Exit 2 saying"), said +def test_step_7_says_what_a_failed_verification_leaves_on_github(): + """Exit 1 said "the source is untouched". Against GitHub `execute` sets + the topic and opens the pull request BEFORE it verifies, so a failed + verification leaves the pull request open on the source and the topic + set, unless `gh` refused them, and deleting the split branch, as the way + out says, closes that pull request (#523, auA-12).""" + said = step_7() + assert "untouched" not in said, said + assert ("Exit 1, a failed verification: nothing is deleted and `main` is " + "where it was, but the split branch is pushed and, on GitHub, its " + "pull request may be open and the topic set") in said, said + assert "delete the split branch (closing that pull request)" in said, said + # What the tool prints for it is what the step names. + assert "IS open" in STILL_OPEN and "closes it" in STILL_OPEN + assert "stays set" in TOPIC_STAYS + + def test_the_modules_exit_codes_paragraph_names_the_exits_after_the_push( adopter): """The EXIT CODES paragraph gave exit 2 one meaning, "the question could @@ -493,6 +521,14 @@ def test_the_command_printed_for_a_refused_pull_request_is_the_one_refused( f"{NORTHWIND}-spec and {NORTHWIND}-code"), title assert body.split("\n", 1)[0] == title, body assert "MOVED TO THE CODE LEG" in body and "\n" in body, body + # The body lists the FOLLOW-UPS the split does not do, and the last is a + # review of the pull request this run opens itself, never one telling + # the reviewer to open it, or calling the extracted legs seeded (#523). + follow_ups = body.split("FOLLOW-UPS, which this commit does NOT do:\n", + 1)[1] + assert follow_ups.endswith(f" - {PULL_REQUEST_FOLLOW_UP}\n"), follow_ups + assert OPEN_THE_SPLIT not in body, body + assert "seeded" not in follow_ups, follow_ups @WINDOWS_SKIP # the `gh` is a `#!` script, which Windows does not run @@ -546,6 +582,40 @@ def test_a_failed_verification_outranks_a_refused_pull_request(tmp_path): assert_pushed(github) +@WINDOWS_SKIP # the `gh` is a `#!` script, which Windows does not run +@needs_filter_repo +@pytest.mark.parametrize("refuses, said", [ + pytest.param("never", (STILL_OPEN, TOPIC_STAYS), id="both-in-place"), + pytest.param("pr create", (NOT_BY_HAND, TOPIC_STAYS), id="topic-only"), + pytest.param("repo edit", (STILL_OPEN,), id="pull-request-only"), +]) +def test_a_failed_verification_says_what_it_left_in_place(tmp_path, refuses, + said): + """The split fails the verification after `execute` has set the topic + and opened the pull request, whichever of the two `gh` took. The way out + said NOTHING was deleted from the source, which is true, and nothing of + the pull request open for the failed split, or of the topic (#523, + auA-12). The last thing the run says now names each one it left, after + the command that deletes the split branch, which closes the pull + request; and still no command to open one or to set the topic, and no + NEXT. Both refused is + `test_a_failed_verification_outranks_a_refused_pull_request`.""" + result, github = github_run(tmp_path, refuses, tree=FAILING_TREE, + keep_contracts=True) + + assert result.returncode == 1, result.stdout + result.stderr + assert FINDING in result.stderr, result.stderr + assert result.stderr.rstrip("\n").rsplit("\n", 1)[1] == " ".join(said), \ + result.stderr + assert result.stderr.index(f"--delete {SPLIT_BRANCH}") \ + < result.stderr.index(said[0]), result.stderr + assert (github / "opened").exists() is (STILL_OPEN in said) + for unsaid in ("Open the pull request by hand", "Set the topic by hand", + "IS pushed", NEXT, VERIFIED): + assert unsaid not in result.stderr + result.stdout, unsaid + assert_pushed(github) + + @WINDOWS_SKIP # the `gh` is a `#!` script, which Windows does not run @needs_filter_repo def test_the_same_source_verifies_when_gh_opens_the_pull_request(tmp_path): diff --git a/tests/test_adopt_late_failure.py b/tests/test_adopt_late_failure.py index b6f64a27..7ad636f7 100644 --- a/tests/test_adopt_late_failure.py +++ b/tests/test_adopt_late_failure.py @@ -1132,12 +1132,13 @@ def test_a_closed_stdout_does_not_swallow_the_report( def test_a_refused_leg_push_offers_an_adoption_its_one_exit( adopter, monkeypatch, capsys, tmp_path): - """#188 N2. The scaffold's second exit, a seed branch and a pull request, - cannot finish an adoption: the leg is a repository this run created, its - first push is the one refused, so it has no `main` for that pull request - to target, and nothing resumes the adoption from a merged one. The hint - names the one exit, and no command for the other. A seeded leg, pushed - to a bare remote whose hook refuses `main` as a ruleset does.""" + """#188 N2. A seed branch and a pull request, the scaffold's second exit + until #506 and never one here (#523), cannot finish an adoption: the leg + is a repository this run created, its first push is the one refused, so + it has no `main` for that pull request to target, and nothing resumes the + adoption from a merged one. The hint names the one exit, and no command + for that route. A seeded leg, pushed to a bare remote whose hook refuses + `main` as a ruleset does.""" monkeypatch.setattr(adopter, COPY_TREE, seed_one_file) remote = tmp_path / REMOTES / f"{ASSEMBLY}-{SPEC}.git" git("init", "-q", "--bare", "-b", "main", str(remote), cwd=tmp_path) diff --git a/tests/test_adopt_plan.py b/tests/test_adopt_plan.py index 15402876..843a3526 100644 --- a/tests/test_adopt_plan.py +++ b/tests/test_adopt_plan.py @@ -219,6 +219,38 @@ def test_check_passes_once_every_question_is_answered(source_repo, tmp_path): in result.stdout +def test_check_passes_an_answer_with_no_resolution_as_agents_md_says( + source_repo, tmp_path): + """AGENTS.md step 2 said an answer with no reason beside it "will be read + as a finding", and `check` says `plan ok` for one: it never reads + `resolution:`, which only `execute` copies, into a kept submodule's + record. Step 2 says so now, and who reads the reason: the human the plan + is shown to at step 4 (#523, N3). A `check` that starts asking for one + fails here, and so does the old sentence.""" + out = tmp_path / "plan.yaml" + assert write_plan(source_repo, out).returncode == 0 + text = out.read_text(encoding="utf-8") + for path, leg in (("examples/", "spec"), (".claude/", "root"), + ("release.yaml", "root")): + unanswered = f" - path: {path}\n leg: null\n" + assert unanswered in text, text + text = text.replace(unanswered, f" - path: {path}\n leg: {leg}\n") + with out.open("w", encoding="utf-8", newline="\n") as handle: + handle.write(text) + + result = run_script(ADOPT, "check", "--plan", str(out)) + + assert result.returncode == 0, result.stdout + result.stderr + assert "plan ok" in result.stdout, result.stdout + agents = (REPO / "AGENTS.md").read_text(encoding="utf-8") + step = " ".join(agents.split( + "2. **You resolve the questions, in writing, in the plan.**", 1)[1] + .split("\n4. ", 1)[0].split()) + assert "read as a finding" not in step, step + assert ("An answer with no reason is a guess, and `check` passes it: the " + "human reads the reasons at step 4.") in step, step + + def test_check_plans_the_topic_on_all_three(source_repo, tmp_path): """`check` is where the human reads what will happen, and setting `xf-project-` on the adopted root and both legs is part of it. It runs diff --git a/tests/test_adopt_tracking_branch.py b/tests/test_adopt_tracking_branch.py index 798c6665..1e8d9105 100644 --- a/tests/test_adopt_tracking_branch.py +++ b/tests/test_adopt_tracking_branch.py @@ -43,11 +43,21 @@ "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). +#: The plan's last follow-up, whole, for the `Northwind` that `planned` adopts. +#: It names the pull request's base by the plan's key, never its value +#: (#493's review, F3). It is a review: `execute` opens that pull request +#: itself against GitHub, so it is never "open the split as a pull request", +#: and the legs it creates and pushes first are extracted, never "seeded" +#: (#523, N1). `plan` writes it, `check` prints it, and the split commit and +#: the pull request's body list it. 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;") + "review the root's diff together with Northwind-spec and Northwind-code, " + "the two new repositories `execute` creates and pushes before the split; " + "against GitHub it opens the split's pull request itself, whose base is " + "the plan's `tracking_branch`, the branch the assembly root and both legs " + "track") +#: What it said until #523, which no follow-up says now. +OPEN_THE_SPLIT = "open the split as a pull request" def planned(tmp_path, source, *flags: str): @@ -89,7 +99,10 @@ 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(PULL_REQUEST_FOLLOW_UP), \ + assert written["follow_ups"][-1] == PULL_REQUEST_FOLLOW_UP, \ + written["follow_ups"] + assert not [item for item in written["follow_ups"] + if OPEN_THE_SPLIT in item or "seeded" in item], \ written["follow_ups"] assert MISSING not in result.stdout + result.stderr @@ -118,9 +131,10 @@ def test_the_follow_up_names_no_branch_a_hand_edit_can_leave_behind(tmp_path): 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 + if "the plan's `tracking_branch`" in line] + assert said == f" follow-up: {PULL_REQUEST_FOLLOW_UP}", said assert "develop" not in said and "master" not in said, said + assert OPEN_THE_SPLIT not in result.stdout, result.stdout def test_check_finds_a_tracking_branch_the_source_does_not_have(tmp_path): diff --git a/tests/test_ruleset_hint_tracking_branch.py b/tests/test_ruleset_hint_tracking_branch.py index 16ea46d8..bfe9fc05 100644 --- a/tests/test_ruleset_hint_tracking_branch.py +++ b/tests/test_ruleset_hint_tracking_branch.py @@ -112,11 +112,10 @@ def _adopt_module(): request, a direct push to the default branch is refused BY DESIGN and must not be worked around. An adoption has ONE legitimate exit: have an operator holding the bypass right re-run it, once what the notice below names is -cleared. The scaffold's other exit, a seed branch and a pull request, cannot -finish an adoption: @REPO@ has no @BRANCH@ for that pull request to target, -and nothing would run the rest of this adoption after it was merged. Under -such a ruleset, that bypass is the fix the notice below asks for, never a -change to the ruleset.""" +cleared. A seed branch and a pull request cannot finish an adoption: +@REPO@ has no @BRANCH@ for that pull request to target, and nothing would +run the rest of this adoption after it was merged. Under such a ruleset, that +bypass is the fix the notice below asks for, never a change to the ruleset.""" #: Each constant, the name a tool prints it by, and its literal. HINTS = { @@ -162,6 +161,9 @@ def test_each_hint_for_another_branch_changes_that_branch_and_nothing_else( assert "gh pr create" not in printed assert "push -u" not in printed assert "Two legitimate exits" not in printed + # Nor as "the scaffold's other exit": #506 took that exit out of the + # scaffold's hint, and the adoption's went on calling it one (#523). + assert "other exit" not in printed def test_the_scaffold_and_the_family_re_run_with_the_flag_and_adoption_not(): From fe8d5550213f2a0cc58f822881e66e7aee9b0c5e Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Thu, 15 Oct 2026 11:12:11 +0000 Subject: [PATCH 2/2] adopt-project.py: under `--local-remote-dir`, NEXT's clone line carries `-c protocol.file.allow=always` and names the repository the split is pushed to, so it clones both legs and bootstraps when pasted as printed, and a failed verification says a leg left behind keeps the topic (#523) #582's review (RV582), fix round: - MUST: NEXT under `--local-remote-dir` printed `git clone --recurse-submodules -b adopt/three-repo-shape `. Pasted, it died `transport 'file' not allowed` for both legs, because the split's `.gitmodules` names each leg by its plain path. The line is now `git -c protocol.file.allow=always clone --recurse-submodules -b `, from the module's `FILE_PROTOCOL`. Branch and url are quoted for `sh`. The url is the source the split is pushed back into, not a placeholder (the review's nit 4). The GitHub-mode NEXT is unchanged. - The e2e test parses NEXT's clone line and runs it as printed. The only change is the transport every test gives a just-pushed remote (#440): `--no-local`, and leg paths read as `file://`. It then runs the printed bootstrap in that clone. The fixture's own clone, which passed while the pasted line failed, is no longer used. The unit test asserts the arguments, `-c protocol.file.allow=always` among them. - nit: the failed-verification topic sentence said "the re-run sets it again", which a leg left behind by a re-run into fresh legs does not get undone. It now says "the re-run sets it again on the root and the legs it makes, and a leg left behind keeps it". - nit: `test_step_7_says_what_a_failed_verification_leaves_on_github` drops two asserts that checked only the test's own constants. Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 51 +++++++++++------ tests/test_adopt_e2e.py | 98 +++++++++++++++++++++++++------- tests/test_adopt_github_exits.py | 8 +-- 3 files changed, 113 insertions(+), 44 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index 889f28c8..e283d12a 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -7864,8 +7864,10 @@ def _finish_by_hand(passed: bool, refused: list[str] | None, topic: str, was deleted from the source, which is true, and AGENTS.md said the source was untouched, which is not. So the open pull request is named as one for a split that failed, with what closes it, the deletion of its branch - the way out already names; and the topic as staying set, which the - re-run sets again. + the way out already names; and the topic as staying set: the re-run sets + it again on the root and on the legs it makes, and a leg the person + keeps, re-running into fresh leg repositories rather than deleting it, + keeps the topic too (#582's review). 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 @@ -7884,7 +7886,9 @@ def _finish_by_hand(passed: bool, refused: list[str] | None, topic: str, "deleting the branch, as above, closes it.") if carried: said.append(f"The topic {topic} stays set on " - f"{_and_listed(carried)}; the re-run sets it again.") + f"{_and_listed(carried)}: the re-run sets it again " + "on the root and the legs it makes, and a leg left " + "behind keeps it.") if said: print(" ".join(said), file=sys.stderr) return @@ -8338,8 +8342,9 @@ def cmd_execute(args) -> int: # Where the shape's Makefile and bootstrap script WENT, as the # materializer reports it, and not a second look at the tree (#480); # under `--local-remote-dir` no pull request is opened, so NEXT names - # the split's branch instead (#523). - _next_steps(names, collisions, branch if local else None) + # the split's branch, and the repository it is pushed to (#523). + _next_steps(names, collisions, + (branch, urls["assembly"]) if local else None) _finish_by_hand(not verified, refused, topic, unset, opened=opened, carried=carried) if verified: @@ -8857,17 +8862,24 @@ def _left_by_the_split(source: Source, urls: dict, names: dict, def _next_steps(names: dict, collisions: list, - branch: str | None = None) -> None: + split: tuple[str, str] | None = None) -> None: """What a person does once the split is verified AND its pull request is open, or none was tried. Not said after a refused `gh pr create`: there the command to open it is the next step (#304). - `branch` is the split's branch where no pull request was tried, under - `--local-remote-dir`. NEXT said "review the pull request" there as well, - over none (#523, N2), and named a clone of the default branch, where the - split is not. There it names the split on its branch, and the clone of - that branch, quoted for `sh` as `_branch_deletion` quotes it. Without - one, the block is what it was, byte for byte. + `split` is `(branch, url)`, the split's branch and the repository it is + pushed to, where no pull request was tried: under `--local-remote-dir`, + where that repository is the source itself. NEXT said "review the pull + request" there as well, over none (#523, N2), and named a clone of the + default branch, where the split is not. There it names the split on its + branch, and a clone of that branch from that repository, each value + quoted for `sh` as `_branch_deletion` quotes it, so the line pastes as + printed. The clone carries `FILE_PROTOCOL`: the split's `.gitmodules` + names each leg by its plain path under `--local-remote-dir`, and git + clones a submodule over the `file` transport only where it is told to, + so without it both leg clones die `transport 'file' not allowed`, as + pasting the line did in #582's review. Without `split`, the block is + what it was, byte for byte. `collisions` is each `(intended, actual)` the materializer wrote beside a file the source kept: where the shape's files WENT, and not a second @@ -8890,15 +8902,18 @@ def _next_steps(names: dict, collisions: list, bootstrap = ("" if script else "\n python3 scripts/bootstrap.py" if makefile else "\n make bootstrap") - if branch is None: - review, clone = f"the pull request on {names['assembly']}", "" + if split is None: + review = f"the pull request on {names['assembly']}" + clone = ("git clone --recurse-submodules " + f"<{names['assembly']} url>") else: + branch, url = split review = (f"the split on its branch in {names['assembly']} (no pull " "request is opened under --local-remote-dir)") - clone = f"-b {shlex.quote(branch)} " - print(f"\nNEXT: review {review}, then\n" - f" git clone --recurse-submodules {clone}<{names['assembly']} url>" - f"{bootstrap}") + clone = " ".join(["git", *FILE_PROTOCOL, "clone", + "--recurse-submodules", "-b", shlex.quote(branch), + shlex.quote(url)]) + print(f"\nNEXT: review {review}, then\n {clone}{bootstrap}") if script: kept = [path for path in (SHAPE_MAKEFILE, BOOTSTRAP) if path in aside] diff --git a/tests/test_adopt_e2e.py b/tests/test_adopt_e2e.py index db88cab3..40945225 100644 --- a/tests/test_adopt_e2e.py +++ b/tests/test_adopt_e2e.py @@ -11,10 +11,13 @@ from __future__ import annotations import importlib.util +import os import re +import shlex import shutil import subprocess import sys +from pathlib import Path import pytest @@ -221,35 +224,82 @@ def next_block(stdout: str) -> list[str]: return stdout.split("\nNEXT: ", 1)[1].splitlines() -#: NEXT's first two lines after `execute --local-remote-dir`, which opens no -#: pull request: the split on its branch, and the clone of that branch, where -#: the split is. They said "review the pull request", over none, and cloned -#: the default branch (#523, N2). -LOCAL_NEXT = [f"review the split on its branch in {PROJECT} (no pull request " - "is opened under --local-remote-dir), then", - " git clone --recurse-submodules -b adopt/three-repo-shape " - f"<{PROJECT} url>"] +#: What the clone NEXT prints after `execute --local-remote-dir` must carry: +#: the split's `.gitmodules` names each leg by a plain path, and git clones a +#: submodule over the `file` transport only where it is told to (#582's +#: review). A literal, so the test holds the tool to the words it prints. +ALLOW_FILE = ["-c", "protocol.file.allow=always"] + + +def local_next(source: Path) -> list[str]: + """NEXT's first two lines after `execute --local-remote-dir`, which opens + no pull request: the split on its branch, and the command that clones + that branch from `source`, where the split is pushed, as `execute` names + it (resolved, quoted for `sh`). They said "review the pull request", over + none, and cloned the default branch (#523, N2). The clone carries `-c + protocol.file.allow=always`: pasted without it, git refuses both leg + clones, `transport 'file' not allowed` (#582's review).""" + return [f"review the split on its branch in {PROJECT} (no pull request " + "is opened under --local-remote-dir), then", + " git -c protocol.file.allow=always clone --recurse-submodules " + f"-b adopt/three-repo-shape {shlex.quote(str(source.resolve()))}"] + + +def pasted(line: str, cwd: Path, remotes: Path) -> subprocess.CompletedProcess: + """A printed `git ... clone ...` line, run in `cwd` as a person pastes it, + with one change: the transport every test gives a clone of a remote just + pushed into (#440). A plain path is git's local clone, which copies the + object files one by one, and maintenance a push left running can pack + them away mid-copy (`failed to copy file`, on the macOS runner). So + `--no-local` reads the root through git, and each leg's plain path under + `remotes`, as `.gitmodules` records it (the directory resolved, in both + of a Windows path's spellings), is read as its `file://` URI. Every + printed argument is kept as printed, `-c protocol.file.allow=always` + among them; a URI is the `file` transport too, so a line without it is + refused here as it is refused pasted.""" + argv = shlex.split(line) + resolved = remotes.resolve() + uri = resolved.as_uri().rstrip("/") + "/" + prefixes = dict.fromkeys([os.path.join(str(resolved), ""), + resolved.as_posix().rstrip("/") + "/"]) + at = argv.index("clone") + return subprocess.run( + [*argv[:at], *(arg for prefix in prefixes + for arg in ("-c", f"url.{uri}.insteadOf={prefix}")), + "clone", "--no-local", *argv[at + 1:]], + cwd=str(cwd), capture_output=True, encoding="utf-8", errors="replace", + check=False) def test_next_names_the_bootstrap_a_root_with_its_own_makefile_can_run( - adopted, adopted_clone): + adopted, tmp_path): """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.)""" + run in the clone NEXT's own clone line makes, exits 0. (It is run with + the interpreter running this test, which is what `python3` names here.) + + THE CLONE IS THE PRINTED ONE (#582's review): it is parsed and run as + printed, but for `pasted`'s transport, where it was a clone the test made + its own way and passed while the printed line, pasted, failed both leg + clones for want of `-c protocol.file.allow=always`.""" said = next_block(adopted["stdout"]) - assert said[:3] == [*LOCAL_NEXT, " python3 scripts/bootstrap.py"], said + assert said[:3] == [*local_next(adopted["source"]), + " 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`") + assert shlex.split(said[1])[:3] == ["git", *ALLOW_FILE], said[1] + cloned = pasted(said[1], tmp_path, adopted["remotes"]) + assert cloned.returncode == 0, cloned.stdout + cloned.stderr + clone = tmp_path / adopted["source"].name command = said[2].split() - ran = subprocess.run([sys.executable, *command[1:]], cwd=adopted_clone, + ran = subprocess.run([sys.executable, *command[1:]], cwd=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, + made = subprocess.run(["make", "bootstrap"], cwd=clone, capture_output=True, text=True, encoding="utf-8", errors="replace", check=False) assert made.returncode != 0, made.stdout + made.stderr @@ -301,19 +351,25 @@ def test_next_keeps_make_bootstrap_where_the_shapes_makefile_is_the_roots( def test_next_names_the_split_branch_where_no_pull_request_was_opened( adopter, capsys): - """Given the split's branch, as `execute --local-remote-dir` gives it, - NEXT names no pull request and clones that branch, quoted for `sh`; the - rest of the block is unchanged (#523, N2).""" - adopter._next_steps({"assembly": PROJECT}, [], "adopt/three repo") + """Given the split's branch and the repository it is pushed to, as + `execute --local-remote-dir` gives them, NEXT names no pull request and + clones that branch from that repository, each quoted for `sh`, with + `-c protocol.file.allow=always`, which the leg clones need; the rest of + the block is unchanged (#523, N2; #582's review).""" + adopter._next_steps({"assembly": PROJECT}, [], + ("adopt/three repo", "/x/my source")) out = capsys.readouterr().out assert out == ( f"\nNEXT: review the split on its branch in {PROJECT} (no pull " "request is opened under --local-remote-dir), then\n" - " git clone --recurse-submodules -b 'adopt/three repo' " - f"<{PROJECT} url>\n" + " git -c protocol.file.allow=always clone --recurse-submodules " + "-b 'adopt/three repo' '/x/my source'\n" " make bootstrap\n") assert "review the pull request" not in out + assert shlex.split(out.splitlines()[2]) == [ + "git", *ALLOW_FILE, "clone", "--recurse-submodules", "-b", + "adopt/three repo", "/x/my source"] #: A source that keeps its own `scripts/bootstrap.py` in the root beside its @@ -366,7 +422,7 @@ def test_next_names_no_bootstrap_where_the_roots_script_is_the_sources_own( assert result.returncode == 0, result.stderr + result.stdout assert " beside shape/scripts/bootstrap.py " in result.stdout - assert next_block(result.stdout) == [*LOCAL_NEXT, BOTH_ASIDE], \ + assert next_block(result.stdout) == [*local_next(source), BOTH_ASIDE], \ result.stdout diff --git a/tests/test_adopt_github_exits.py b/tests/test_adopt_github_exits.py index 6bf16930..88e515ba 100644 --- a/tests/test_adopt_github_exits.py +++ b/tests/test_adopt_github_exits.py @@ -115,8 +115,9 @@ "for a split that did not pass verification: deleting the " "branch, as above, closes it.") TOPIC_STAYS = (f"The topic {TOPIC} stays set on {ORG}/{NORTHWIND}, " - f"{ORG}/{NORTHWIND}-spec and {ORG}/{NORTHWIND}-code; the " - "re-run sets it again.") + f"{ORG}/{NORTHWIND}-spec and {ORG}/{NORTHWIND}-code: the " + "re-run sets it again on the root and the legs it makes, and " + "a leg left behind keeps it.") VERIFIED = "adoption verified: every source path is in exactly one place" FINDING = "FINDING adopt-shape-file-missing:" #: What `_verify` used to say after every pass, whether or not a pull request @@ -392,9 +393,6 @@ def test_step_7_says_what_a_failed_verification_leaves_on_github(): "where it was, but the split branch is pushed and, on GitHub, its " "pull request may be open and the topic set") in said, said assert "delete the split branch (closing that pull request)" in said, said - # What the tool prints for it is what the step names. - assert "IS open" in STILL_OPEN and "closes it" in STILL_OPEN - assert "stays set" in TOPIC_STAYS def test_the_modules_exit_codes_paragraph_names_the_exits_after_the_push(