diff --git a/AGENTS.md b/AGENTS.md index 31e9f9a9..44a31c7a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -134,21 +134,24 @@ install command). Seven steps, in order, and step 4 is the one you must not skip record). 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 @@ -159,9 +162,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 29c4a828..6a335cef 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -149,7 +149,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 @@ -2019,6 +2021,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/" @@ -2035,10 +2047,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 @@ -7055,29 +7068,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, @@ -8231,14 +8246,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 @@ -8255,6 +8275,19 @@ 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: 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 depends on in a merged log; a stdout that is closed, `| head` and gone, @@ -8262,9 +8295,21 @@ 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 " + "on the root and the legs it makes, and a leg left " + "behind keeps it.") + if said: + print(" ".join(said), file=sys.stderr) return topics = "".join(f"\n gh repo edit {repository} --add-topic {topic}" for repository in unset) @@ -8696,13 +8741,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) @@ -8712,9 +8761,13 @@ 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, 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: # A verification mismatch outranks a refused pull request and a # missing topic. @@ -9229,10 +9282,25 @@ 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, + split: tuple[str, 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). + + `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 @@ -9255,9 +9323,18 @@ 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>" - f"{bootstrap}") + 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 = " ".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 1403b7de..8295c4ae 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 @@ -212,27 +215,82 @@ def next_block(stdout: str) -> list[str]: return stdout.split("\nNEXT: ", 1)[1].splitlines() +#: 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] == [f"review the pull request on {PROJECT}, then", - f" git clone --recurse-submodules <{PROJECT} url>", + 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 @@ -282,6 +340,29 @@ 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 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 -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 #: own Makefile. `scripts/` goes to the code leg unless the plan is answered #: otherwise, as `kept_scripts` answers it. @@ -332,10 +413,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(source), 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 12878062..2d34927d 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, a_hook_that_unstages, 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. @@ -100,6 +101,17 @@ #: 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 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 @@ -361,6 +373,20 @@ 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 + + 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 @@ -485,6 +511,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 @@ -538,6 +572,39 @@ 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, failing=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 91376cf2..d8cce966 100644 --- a/tests/test_adopt_late_failure.py +++ b/tests/test_adopt_late_failure.py @@ -1123,12 +1123,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():