From d3f71d6b03c9230614ca4d772ea46ba93633d4f9 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 11:30:58 +0000 Subject: [PATCH 1/5] AGENTS.md: step 7 keys the way out of `adopt-project.py execute` to the exit it prints, so an exit 2 saying `IS pushed` (`gh pr create` refused, or the topic will not set) is finished with the command it prints and no longer abandoned for a second pair of legs, and the module's EXIT CODES paragraph names both (#304) Step 7 told an assistant that any non-zero exit of `execute` is to fix the plan and re-run into fresh legs. That is the way out of a failed verification, exit 1. In GitHub mode `execute` has two more non-zero exits, both exit 2 and both AFTER both legs and the split branch are pushed: `gh pr create` refused (`The branch IS pushed. Open the pull request by hand:`, returning before `_verify`, so no table), and the topic that will not set (`The split IS pushed and the pull request IS open. Set the topic by hand:`, after `adoption verified`). On either, the way out is the printed command, and a re-run into fresh legs abandons a finished adoption. Step 7 now says which exit is which and what each is for, in five more lines, and the AGENTS.md cap moves 464 -> 469 with a dated entry. The module's EXIT CODES paragraph said exit 2 is "the question could not be asked"; it now names the two that come after the push. The printed text and the behaviour of `execute` are unchanged, so `--help` and docs/cli.md are too. tests/test_adopt_github_exits.py is the issue's reproduction as a test, with a `gh` shim that refuses one command and every github.com URL rewritten to a bare repository: both exits are exit 2 with their printed command, the legs and the split branch are pushed, and the first has no table. Step 7's anchors (`IS pushed`, `gh pr create`, the topic) are asserted against the words the tool prints, so rewording either side alone fails a test. Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Sonnet 5.5 --- AGENTS.md | 9 +- adopt-project.py | 7 + tests/test_adopt_github_exits.py | 235 +++++++++++++++++++++++++++++++ tests/test_repo_hygiene.py | 18 ++- 4 files changed, 265 insertions(+), 4 deletions(-) create mode 100644 tests/test_adopt_github_exits.py diff --git a/AGENTS.md b/AGENTS.md index 1199b938..3a3b98f1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -134,9 +134,14 @@ order, and step 4 is the one you must not skip. 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, in exactly one of {spec leg, code - leg, root tree} or as `drop`. If it exits non-zero, STOP and relay it: the + 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 the exit is to fix the plan and re-run into fresh legs. + is on a branch — so fix the plan and re-run into fresh legs. Exit 2 saying + `IS pushed` (GitHub only: `gh pr create` or the topic refused) comes after + both legs and the split are pushed: finish with the command it prints and + never re-run into fresh legs, which abandons that pair. A refused + `gh pr create` prints no table, so say the split is unverified. `execute` opens a pull request, because these organisations are pull-request only. Never suggest a direct push to the default branch, and never `--admin`. diff --git a/adopt-project.py b/adopt-project.py index 3ed2498e..8fe7da4c 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -78,6 +78,13 @@ EXIT CODES: 0 done · 1 findings (a plan that does not check out; a verification mismatch) · 2 a refusal — the question could not be asked. +`execute` has two more exit 2s, GitHub mode's alone, and both come AFTER both +legs and the split branch are pushed: `gh pr create` refused (`The branch IS +pushed`; it returns before the verification, so no table is printed), and the +topic would not set (`The split IS pushed and the pull request IS open`; after +`adoption verified`). 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. """ from __future__ import annotations diff --git a/tests/test_adopt_github_exits.py b/tests/test_adopt_github_exits.py new file mode 100644 index 00000000..e9029012 --- /dev/null +++ b/tests/test_adopt_github_exits.py @@ -0,0 +1,235 @@ +# SPDX-License-Identifier: Apache-2.0 +"""AGENTS.md step 7 names the two exits `execute` has in GitHub mode alone (#304). + +Step 7 said that a non-zero exit of `adopt-project.py execute` is to fix the +plan and re-run into fresh legs. That is the way out of a failed verification, +exit 1. A GitHub run has two exits more, and both come AFTER both legs and the +split branch are pushed: `gh pr create` refused, and the topic that will not +set. Each is exit 2, each prints the command that finishes the job by hand, +and following the old sentence abandoned a pushed adoption for a second pair +of legs beside it. + +No test reached either exit: `--local-remote-dir`, the path every test takes, +skips both (`if not local:` in `cmd_execute`). The two runs here are the +issue's own reproduction, with no network and nothing created anywhere: a `gh` +that refuses one command and makes the other repositories as bare ones under +the test's directory, and every `https://github.com/` URL rewritten to that +directory by a throwaway global git config. + +THE SHIM IS A `#!` SCRIPT, so the runs are `WINDOWS_SKIP`ped, and they need +`git filter-repo` as every `execute` test does. Reading step 7, and the +module's own EXIT CODES paragraph, runs everywhere. + +`adopt-project.py`'s own two messages are asserted here WORD FOR WORD, and the +anchors step 7 gives (`IS pushed`, `gh pr create`, the topic) are asserted +against them, so rewording either side alone fails a test instead of leaving +an assistant a sentence about a message that no longer exists. +""" + +from __future__ import annotations + +import importlib.util +import os +import shutil +import stat +import textwrap +from pathlib import Path + +import pytest + +from conftest import (ADOPT, REPO, WINDOWS_SKIP, git, make_source_repo, + run_script, write_plan) + +#: `execute` REQUIRES `git filter-repo`; without it the two runs below are +#: skipped, as every `execute` test in this suite is. +needs_filter_repo = pytest.mark.skipif( + shutil.which("git-filter-repo") is None, + reason="git filter-repo is not installed: `pip install git-filter-repo`") + +ORG = "testorg" +NORTHWIND = "Northwind" +TOPIC = "xf-project-northwind" +SPLIT_BRANCH = "adopt/three-repo-shape" +#: A source small enough that `plan` asks no question, so `execute` runs on +#: the plan as written. +SOURCE_TREE = {"README.md": "# Northwind\n", "src/app.py": "print(1)\n", + "docs/spec.md": "# Spec\n"} + +#: The heading of step 7, which the point runs from to the next blank line. +STEP_7 = "7. **Read the verification table back to them.**" + +#: What a refused `gh pr create` prints after the REFUSED block, and what a +#: topic that will not set prints. Both are `adopt-project.py`'s own words. +BRANCH_IS_PUSHED = ( + "The branch IS pushed. Open the pull request by hand:\n" + f" gh pr create --repo {ORG}/{NORTHWIND} --base main " + f"--head {SPLIT_BRANCH}") +SPLIT_IS_PUSHED = ( + "The split IS pushed and the pull request IS open. Set the topic by " + "hand:\n" + "\n".join( + f" gh repo edit {ORG}/{name} --add-topic {TOPIC}" + for name in (NORTHWIND, f"{NORTHWIND}-spec", f"{NORTHWIND}-code"))) + +#: `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 +#: token without the permission is. +GH_SHIM = textwrap.dedent("""\ + #!/bin/sh + case "$1 $2" in + "$FAKE_GH_REFUSES") + 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";; + "repo edit") ;; + *) exit 1;; + esac + """) + + +@pytest.fixture +def adopter(): + """`adopt-project.py` as a module, to read its EXIT CODES paragraph.""" + module_spec = importlib.util.spec_from_file_location("adopt_github_exits", + ADOPT) + module = importlib.util.module_from_spec(module_spec) + module_spec.loader.exec_module(module) + return module + + +def step_7() -> str: + """Step 7 of AGENTS.md, from its heading to the blank line after it, as + one line of single-spaced text.""" + agents = (REPO / "AGENTS.md").read_text(encoding="utf-8") + return " ".join(agents.split(STEP_7, 1)[1].split("\n\n", 1)[0].split()) + + +def github_run(tmp_path: Path, refuses: str): + """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.""" + github = tmp_path / "github" + (github / ORG).mkdir(parents=True) + source = make_source_repo(tmp_path / NORTHWIND, tree=SOURCE_TREE, edits=()) + # 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"), + cwd=source) + + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + shim = bin_dir / "gh" + shim.write_text(GH_SHIM, encoding="utf-8") + shim.chmod(shim.stat().st_mode | stat.S_IXUSR) + config = tmp_path / "gitconfig" + config.write_text(f'[url "{github.as_posix()}/"]\n' + "\tinsteadOf = https://github.com/\n", encoding="utf-8") + + plan = tmp_path / "adoption-plan.yaml" + written = write_plan(source, plan) + assert written.returncode == 0, written.stderr + written.stdout + result = run_script( + ADOPT, "execute", "--plan", str(plan), "--yes", + "--work-dir", str(tmp_path / "work"), + env={"PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}", + "GIT_CONFIG_GLOBAL": str(config), "GIT_ALLOW_PROTOCOL": "file", + "FAKE_GH_ROOT": str(github), "FAKE_GH_REFUSES": refuses}) + return result, github + + +def heads(repository: Path) -> list[str]: + """The branches `repository` (a bare one) holds.""" + listed = git("for-each-ref", "--format=%(refname:short)", "refs/heads", + cwd=repository).stdout + return sorted(listed.split()) + + +# --- AGENTS.md step 7 -------------------------------------------------------- + +def test_step_7_keys_its_advice_to_the_exit_and_not_to_non_zero(): + """Exit 1 is the failed verification and keeps its way out; exit 2 saying + `IS pushed` is the GitHub run's two exits after the push, and its way out + is the command the tool prints, never a re-run into a second pair of legs. + + Each phrase is one the sentence cannot be true without. The exit 1 half + is read as `fix the plan` and `fresh legs` only, so a rewording of how the + failed verification is left (#188 asks for the split branch deleted too) + does not fail a test about the two exits after the push. + """ + said = step_7() + assert "If it exits non-zero, STOP and relay it" in said, said + assert "Exit 1, a failed verification:" in said, said + assert "fix the plan" in said and "fresh legs" in said, said + assert "Exit 2 saying `IS pushed`" in said, said + assert "GitHub only" in said, said + assert "`gh pr create`" in said and "the topic" in said, said + assert "finish with the command it prints" in said, said + assert "never re-run into fresh legs" in said, said + assert "prints no table" in said, said + # The order is the reading order: the failed verification first, and the + # exit-2 case read AS the exception to it, not under it. + assert said.index("Exit 1") < said.index("Exit 2 saying"), 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 + not be asked". It says what comes after the push, in the tool's own + words, so the file and the messages it describes cannot part.""" + codes = " ".join(adopter.__doc__.split("EXIT CODES:", 1)[1].split()) + assert "two more exit 2s" in codes, codes + assert "AFTER both legs and the split branch are pushed" in codes, codes + assert "`gh pr create` refused (`The branch IS pushed`" in codes, codes + assert ("the topic would not set (`The split IS pushed and the pull " + "request IS open`") in codes, codes + assert "no table is printed" in codes, codes + assert "a re-run into fresh legs abandons the pushed pair" in codes, codes + + +# --- the two exits, run ----------------------------------------------------- + +@WINDOWS_SKIP # the `gh` is a `#!` script, which Windows does not run +@needs_filter_repo +@pytest.mark.parametrize("refuses, refused, by_hand, verified", [ + pytest.param("pr create", "REFUSED opening the pull request.", + BRANCH_IS_PUSHED, False, id="gh-pr-create-refused"), + pytest.param("repo edit", "REFUSED setting the project topic.", + SPLIT_IS_PUSHED, True, id="the-topic-will-not-set"), +]) +def test_each_exit_after_the_push_is_exit_2_with_a_command_to_finish_by_hand( + tmp_path, refuses, refused, by_hand, verified): + result, github = github_run(tmp_path, refuses) + + # The exit, and what it prints: the REFUSED block, then the command. + assert result.returncode == 2, result.stdout + result.stderr + assert refused in result.stderr, result.stderr + assert by_hand in result.stderr, result.stderr + + # 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. + for leg in ("spec", "code"): + assert heads(github / ORG / f"{NORTHWIND}-{leg}.git") == ["main"] + assert SPLIT_BRANCH in heads(github / ORG / f"{NORTHWIND}.git") + + # Whether there is a table. A refused `gh pr create` returns before + # `_verify` runs; a refused topic is reported after it. If `execute` ever + # verifies before it exits on the first, step 7's "prints no table" is + # the sentence to change with it. + if verified: + assert f" pull request https://github.com/{ORG}/{NORTHWIND}/pull/1" \ + in result.stdout, result.stdout + assert "VERIFICATION" in result.stdout, result.stdout + assert ("adoption verified: every source path is in exactly one " + "place") in result.stdout, result.stdout + else: + assert "VERIFICATION" not in result.stdout, result.stdout + assert "adoption verified" not in result.stdout, result.stdout + assert " split " in result.stdout, result.stdout + + # What step 7 says of it is what the tool printed: the marker words and + # the command it names. + said = step_7() + assert "IS pushed" in said and "IS pushed" in by_hand + assert "gh pr create" in said and "gh pr create" in BRANCH_IS_PUSHED + assert "the topic" in said and "--add-topic" in SPLIT_IS_PUSHED diff --git a/tests/test_repo_hygiene.py b/tests/test_repo_hygiene.py index 3370c49c..4c9641da 100644 --- a/tests/test_repo_hygiene.py +++ b/tests/test_repo_hygiene.py @@ -559,9 +559,23 @@ def test_agents_md_is_short_enough_to_be_read(): class, that a subagent says nothing, and that it never converts, writes the record, blocks, or puts the shape in a review or a gate -- the warning-in-CI the ratified text calls an amendment of the doctrine - rather than a realization of it.""" + rather than a realization of it. + + 2026-10-12: 464 -> 469 -- step 7 of "Adopting an existing repository" + (#304). Five lines, inside that one numbered point and nowhere else. It + said that ANY non-zero exit of `adopt-project.py execute` is to fix the + plan and re-run into fresh legs, which holds for a failed verification + (exit 1) and for neither of the two exits a GitHub run adds: a refused + `gh pr create` and a topic that will not set. Both are exit 2, both come + after the legs and the split branch are pushed, and what each prints is + a command to finish by hand, so an assistant that followed the sentence + abandoned a pushed adoption for a second pair of legs. The lines buy what + the point could not say: that the exit decides, which message marks the + two (`IS pushed`), that the second pair is the harm, and that a refused + pull request has printed no table, so the split it opens is unverified. + `tests/test_adopt_github_exits.py` holds the point to both printed exits.""" lines = (REPO / "AGENTS.md").read_text().splitlines() - assert len(lines) <= 464, f"AGENTS.md is {len(lines)} lines; the cap is 464" + assert len(lines) <= 469, f"AGENTS.md is {len(lines)} lines; the cap is 469" def test_claude_md_points_at_agents_md(): From 16aa9ff2bd9cc01029f1d9bbcb696579d45174f2 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 12:02:15 +0000 Subject: [PATCH 2/5] adopt-project.py: `execute` verifies the split before it exits on a refused `gh pr create`, and says the pull request is to be opened by hand only when the table passed, so the command it prints is for a split somebody checked, and a split that fails is exit 1 with no such command (#304) A refused `gh pr create` ended `execute` with `return 2` before `_verify`, so the pull request the person was told to open by hand was for a split nothing had verified, and the run printed no table. A source whose split FAILS the verification got the same words, "The branch IS pushed. Open the pull request by hand", and exit 2: reproduced on d3f71d6 with a source holding its own `contracts/shape-pin.yaml` kept in the root. Now `cmd_execute` runs `_verify` on that path as it always did when the topic would not set, and keeps the topic untried after a refused pull request (its message says the pull request IS open). `_open_the_pull_request` prints the REFUSED block and returns False; the by-hand lines moved to `_open_it_by_hand`, printed AFTER the verification: with the unchanged "The branch IS pushed. Open the pull request by hand:" and its command when the table passed, exit 2; with "No pull request was opened, and none should be", exit 1 and the verification's own footer when it did not. It flushes stdout first: under a pipe the table is in a block buffer, and the command printed ahead of it in a merged log. AGENTS.md step 7 loses the sentence that a refused `gh pr create` prints no table, and says the exit 2 comes after the split is pushed and verified: four lines over main, not five, so the cap is 468 and its dated entry says so. The EXIT CODES paragraph says the same, and that a failed verification outranks both exits. tests/test_adopt_github_exits.py: both exits now have a table; the command comes after the table in a merged stream; a verification that really fails (a source's own pin kept in the root) gives exit 1 with a refused pull request and prints no command to open one; its control verifies when the pull request is not refused. Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Sonnet 5.5 --- AGENTS.md | 5 +- adopt-project.py | 70 ++++++++--- tests/test_adopt_github_exits.py | 193 +++++++++++++++++++++++++------ tests/test_repo_hygiene.py | 12 +- 4 files changed, 215 insertions(+), 65 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 3a3b98f1..6589222d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -139,9 +139,8 @@ order, and step 4 is the one you must not skip. source is untouched — nothing is deleted, `main` is where it was, the split is on a branch — so fix the plan and re-run into fresh legs. Exit 2 saying `IS pushed` (GitHub only: `gh pr create` or the topic refused) comes after - both legs and the split are pushed: finish with the command it prints and - never re-run into fresh legs, which abandons that pair. A refused - `gh pr create` prints no table, so say the split is unverified. + both legs and the split are pushed and verified: finish with the command it + prints and never re-run into fresh legs, which abandons that pair. `execute` opens a pull request, because these organisations are pull-request only. Never suggest a direct push to the default branch, and never `--admin`. diff --git a/adopt-project.py b/adopt-project.py index 8fe7da4c..2352a683 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -79,12 +79,13 @@ EXIT CODES: 0 done · 1 findings (a plan that does not check out; a verification mismatch) · 2 a refusal — the question could not be asked. `execute` has two more exit 2s, GitHub mode's alone, and both come AFTER both -legs and the split branch are pushed: `gh pr create` refused (`The branch IS -pushed`; it returns before the verification, so no table is printed), and the -topic would not set (`The split IS pushed and the pull request IS open`; after -`adoption verified`). 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. +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. """ from __future__ import annotations @@ -3483,9 +3484,12 @@ def _open_the_pull_request(repositories: dict, names: dict, tracking: str, """(c′) The pull request, because these organisations are pull-request only. - Returns False when `gh` refused, having printed why and the command that - finishes the job by hand — the branch IS pushed by the time this runs, so - the exit is never "start again". Split out of `cmd_execute` for #138. + Returns False when `gh` refused, 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 + 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. """ try: url = run(["gh", "pr", "create", "--repo", repositories["assembly"], @@ -3496,13 +3500,32 @@ def _open_the_pull_request(repositories: dict, names: dict, tracking: str, print(f" pull request {url}") except CommandFailed as exc: print(exc.loudly("opening the pull request"), file=sys.stderr) - print("The branch IS pushed. Open the pull request by hand:\n" - f" gh pr create --repo {repositories['assembly']} " - f"--base {tracking} --head {branch}", file=sys.stderr) return False return True +def _open_it_by_hand(repositories: dict, tracking: str, branch: 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. 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. + """ + sys.stdout.flush() + if not passed: + print("No pull request was opened, and none should be: the split " + "above did not pass verification.", file=sys.stderr) + return + print("The branch IS pushed. Open the pull request by hand:\n" + f" gh pr create --repo {repositories['assembly']} " + f"--base {tracking} --head {branch}", file=sys.stderr) + + def _set_the_topic(repositories: dict, topic: str) -> bool: """(c″) The topic, on all three. @@ -3692,19 +3715,28 @@ def cmd_execute(args) -> int: return 2 split_commit, message = split - topics_failed = False + # 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 if not local: - if not _open_the_pull_request(repositories, names, tracking, branch, - message): - return 2 - topics_failed = not _set_the_topic(repositories, topic) + 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) # ---- (d) verification, by blob sha ------------------------------------ verified = _verify(source, assembly, work_root, names, paths_for, split_commit, spec_path, code_path, seeded) + if pull_request_refused: + _open_it_by_hand(repositories, tracking, branch, not verified) if verified: - return verified # a verification mismatch outranks a missing topic - return 2 if topics_failed else 0 + # 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 def _template_values(plan: Plan, names, repositories, urls, spec_path, diff --git a/tests/test_adopt_github_exits.py b/tests/test_adopt_github_exits.py index e9029012..410c4b8c 100644 --- a/tests/test_adopt_github_exits.py +++ b/tests/test_adopt_github_exits.py @@ -9,11 +9,18 @@ and following the old sentence abandoned a pushed adoption for a second pair of legs beside it. +A refused `gh pr create` used to end the run before the verification, so the +pull request the person was told to open by hand was for a split nothing had +checked. `execute` now verifies on that path too, as it always did when the +topic would not set, and says what the verification found: a split that passes +gets the command to open its pull request AFTER the table, and one that fails +is exit 1 with no such command. + No test reached either exit: `--local-remote-dir`, the path every test takes, -skips both (`if not local:` in `cmd_execute`). The two runs here are the -issue's own reproduction, with no network and nothing created anywhere: a `gh` -that refuses one command and makes the other repositories as bare ones under -the test's directory, and every `https://github.com/` URL rewritten to that +skips both (`if not local:` in `cmd_execute`). The runs here are the issue's +own reproduction, with no network and nothing created anywhere: a `gh` that +refuses one command and makes the other repositories as bare ones under the +test's directory, and every `https://github.com/` URL rewritten to that directory by a throwaway global git config. THE SHIM IS A `#!` SCRIPT, so the runs are `WINDOWS_SKIP`ped, and they need @@ -32,15 +39,17 @@ import os import shutil import stat +import subprocess +import sys import textwrap from pathlib import Path import pytest from conftest import (ADOPT, REPO, WINDOWS_SKIP, git, make_source_repo, - run_script, write_plan) + write_plan) -#: `execute` REQUIRES `git filter-repo`; without it the two runs below are +#: `execute` REQUIRES `git filter-repo`; without it the runs below are #: skipped, as every `execute` test in this suite is. needs_filter_repo = pytest.mark.skipif( shutil.which("git-filter-repo") is None, @@ -54,6 +63,18 @@ #: the plan as written. SOURCE_TREE = {"README.md": "# Northwind\n", "src/app.py": "print(1)\n", "docs/spec.md": "# Spec\n"} +#: The same source with a `contracts/shape-pin.yaml` of its OWN, naming a file +#: the plan sends to the code leg. `plan` sends `contracts/` to the spec leg, +#: where the adoption verifies; kept in the root, as `KEEP_CONTRACTS` answers, +#: it is the pin the first bootstrap would read, naming a file that is not +#: there, and the verification fails (`adopt-shape-file-missing`). A REAL +#: failure of `_verify`, with nothing in the tool replaced. +FAILING_TREE = {**SOURCE_TREE, "contracts/shape-pin.yaml": ( + "schema_version: 1\nkind: pinned_contract_manifest\nfiles:\n" + f" - path: src/app.py\n sha256: \"{'0' * 64}\"\n")} +CONTRACTS_TO_SPEC = " - path: contracts/\n leg: spec\n" +KEEP_CONTRACTS = (" - path: contracts/\n leg: root\n" + " resolution: \"kept beside the shape's own\"\n") #: The heading of step 7, which the point runs from to the next blank line. STEP_7 = "7. **Read the verification table back to them.**" @@ -69,6 +90,12 @@ "hand:\n" + "\n".join( f" gh repo edit {ORG}/{name} --add-topic {TOPIC}" for name in (NORTHWIND, f"{NORTHWIND}-spec", f"{NORTHWIND}-code"))) +#: What a refused `gh pr create` says INSTEAD when the split failed the +#: 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.") +VERIFIED = "adoption verified: every source path is in exactly one place" +FINDING = "FINDING adopt-shape-file-missing:" #: `gh` with no network. `repo create` makes a bare repository under #: $FAKE_GH_ROOT, which is what the rewritten `https://github.com/` URL then @@ -105,13 +132,20 @@ def step_7() -> str: return " ".join(agents.split(STEP_7, 1)[1].split("\n\n", 1)[0].split()) -def github_run(tmp_path: Path, refuses: str): +def github_run(tmp_path: Path, refuses: str, tree: dict = SOURCE_TREE, + keep_contracts: bool = False, merge_stderr: bool = False): """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.""" + that stands for github.com, with `testorg/.git` in it. + + `keep_contracts` answers the plan so that the source's own pin stays in + the root, which fails the verification. `merge_stderr` runs with both + streams in one pipe, as a person redirecting a log has them: stdout is + then block-buffered, and the order the lines arrive in is the order the + tool flushed them.""" github = tmp_path / "github" (github / ORG).mkdir(parents=True) - source = make_source_repo(tmp_path / NORTHWIND, tree=SOURCE_TREE, edits=()) + source = make_source_repo(tmp_path / NORTHWIND, tree=tree, edits=()) # 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"), @@ -129,10 +163,24 @@ def github_run(tmp_path: Path, refuses: str): plan = tmp_path / "adoption-plan.yaml" written = write_plan(source, plan) assert written.returncode == 0, written.stderr + written.stdout - result = run_script( - ADOPT, "execute", "--plan", str(plan), "--yes", - "--work-dir", str(tmp_path / "work"), - env={"PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}", + if keep_contracts: + answered = plan.read_text(encoding="utf-8") + assert CONTRACTS_TO_SPEC in answered, answered + with plan.open("w", encoding="utf-8", newline="\n") as handle: + handle.write(answered.replace(CONTRACTS_TO_SPEC, KEEP_CONTRACTS, + 1)) + result = subprocess.run( + [sys.executable, str(ADOPT), "execute", "--plan", str(plan), "--yes", + "--work-dir", str(tmp_path / "work")], + stdin=subprocess.DEVNULL, stdout=subprocess.PIPE, + stderr=subprocess.STDOUT if merge_stderr else subprocess.PIPE, + text=True, check=False, + env={**os.environ, "LANES_LANE": "", + "GIT_AUTHOR_NAME": "openRepoShape tests", + "GIT_AUTHOR_EMAIL": "tests@openreposhape.invalid", + "GIT_COMMITTER_NAME": "openRepoShape tests", + "GIT_COMMITTER_EMAIL": "tests@openreposhape.invalid", + "PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}", "GIT_CONFIG_GLOBAL": str(config), "GIT_ALLOW_PROTOCOL": "file", "FAKE_GH_ROOT": str(github), "FAKE_GH_REFUSES": refuses}) return result, github @@ -145,6 +193,14 @@ def heads(repository: Path) -> list[str]: return sorted(listed.split()) +def assert_pushed(github: Path) -> 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.""" + for leg in ("spec", "code"): + assert heads(github / ORG / f"{NORTHWIND}-{leg}.git") == ["main"] + assert SPLIT_BRANCH in heads(github / ORG / f"{NORTHWIND}.git") + + # --- AGENTS.md step 7 -------------------------------------------------------- def test_step_7_keys_its_advice_to_the_exit_and_not_to_non_zero(): @@ -164,9 +220,12 @@ def test_step_7_keys_its_advice_to_the_exit_and_not_to_non_zero(): assert "Exit 2 saying `IS pushed`" in said, said assert "GitHub only" in said, said assert "`gh pr create`" in said and "the topic" in said, said + assert "pushed and verified" in said, said assert "finish with the command it prints" in said, said assert "never re-run into fresh legs" in said, said - assert "prints no table" in said, said + # A refused `gh pr create` used to return before the verification, and + # the point said it printed no table. It prints one now. + assert "no table" not in said, said # The order is the reading order: the failed verification first, and the # exit-2 case read AS the exception to it, not under it. assert said.index("Exit 1") < said.index("Exit 2 saying"), said @@ -180,52 +239,49 @@ def test_the_modules_exit_codes_paragraph_names_the_exits_after_the_push( codes = " ".join(adopter.__doc__.split("EXIT CODES:", 1)[1].split()) assert "two more exit 2s" in codes, codes assert "AFTER both legs and the split branch are pushed" in codes, codes + assert "the verification has passed" in codes, codes assert "`gh pr create` refused (`The branch IS pushed`" in codes, codes assert ("the topic would not set (`The split IS pushed and the pull " "request IS open`") in codes, codes - assert "no table is printed" in codes, codes assert "a re-run into fresh legs abandons the pushed pair" in codes, codes + assert ("A verification that fails outranks both, with exit 1 and no " + "command to open the pull request") in codes, codes # --- the two exits, run ----------------------------------------------------- @WINDOWS_SKIP # the `gh` is a `#!` script, which Windows does not run @needs_filter_repo -@pytest.mark.parametrize("refuses, refused, by_hand, verified", [ +@pytest.mark.parametrize("refuses, refused, by_hand, opened", [ pytest.param("pr create", "REFUSED opening the pull request.", BRANCH_IS_PUSHED, False, id="gh-pr-create-refused"), pytest.param("repo edit", "REFUSED setting the project topic.", SPLIT_IS_PUSHED, True, id="the-topic-will-not-set"), ]) def test_each_exit_after_the_push_is_exit_2_with_a_command_to_finish_by_hand( - tmp_path, refuses, refused, by_hand, verified): + tmp_path, refuses, refused, by_hand, opened): result, github = github_run(tmp_path, refuses) # The exit, and what it prints: the REFUSED block, then the command. assert result.returncode == 2, result.stdout + result.stderr assert refused in result.stderr, result.stderr assert by_hand in result.stderr, result.stderr - - # 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. - for leg in ("spec", "code"): - assert heads(github / ORG / f"{NORTHWIND}-{leg}.git") == ["main"] - assert SPLIT_BRANCH in heads(github / ORG / f"{NORTHWIND}.git") - - # Whether there is a table. A refused `gh pr create` returns before - # `_verify` runs; a refused topic is reported after it. If `execute` ever - # verifies before it exits on the first, step 7's "prints no table" is - # the sentence to change with it. - if verified: - assert f" pull request https://github.com/{ORG}/{NORTHWIND}/pull/1" \ - in result.stdout, result.stdout - assert "VERIFICATION" in result.stdout, result.stdout - assert ("adoption verified: every source path is in exactly one " - "place") in result.stdout, result.stdout - else: - assert "VERIFICATION" not in result.stdout, result.stdout - assert "adoption verified" not in result.stdout, result.stdout - assert " split " in result.stdout, result.stdout + assert_pushed(github) + + # BOTH have a table: the split is verified before the person is told to + # finish by hand, and the pull request line is there only when there is + # one. (A refused `gh pr create` returned before `_verify` until #304.) + assert "VERIFICATION" in result.stdout, result.stdout + assert VERIFIED in result.stdout, result.stdout + assert (f" pull request https://github.com/{ORG}/{NORTHWIND}/pull/1" + 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 # What step 7 says of it is what the tool printed: the marker words and # the command it names. @@ -233,3 +289,64 @@ def test_each_exit_after_the_push_is_exit_2_with_a_command_to_finish_by_hand( assert "IS pushed" in said and "IS pushed" in by_hand assert "gh pr create" in said and "gh pr create" in BRANCH_IS_PUSHED assert "the topic" in said and "--add-topic" in SPLIT_IS_PUSHED + + +@WINDOWS_SKIP # the `gh` is a `#!` script, which Windows does not run +@needs_filter_repo +def test_the_command_to_open_a_refused_pull_request_comes_after_the_table( + tmp_path): + """The by-hand command depends on the table, so it is the LAST thing the + run says. With both streams in one pipe, stdout holds the table in a + block buffer until the tool flushes it; a command printed to stderr + without that flush came first, ahead of the table it depends on.""" + result, github = github_run(tmp_path, "pr create", merge_stderr=True) + + assert result.returncode == 2, result.stdout + table = result.stdout.index("VERIFICATION") + verified = result.stdout.index(VERIFIED) + by_hand = result.stdout.index(BRANCH_IS_PUSHED) + assert table < verified < by_hand, result.stdout + assert result.stdout.count("Open the pull request by hand") == 1 + assert result.stdout.rstrip().endswith(BRANCH_IS_PUSHED), result.stdout + assert_pushed(github) + + +@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) + + assert result.returncode == 1, result.stdout + result.stderr + assert FINDING in result.stderr, result.stderr + assert "1 verification finding(s)." in result.stderr, result.stderr + # 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. + assert "Open the pull request by hand" not in result.stderr + result.stdout + assert "IS pushed" not in result.stderr + result.stdout + assert VERIFIED not in result.stdout, result.stdout + assert "VERIFICATION" in result.stdout, result.stdout + # The legs and the branch exist, as the footer says, and no re-run into + # fresh legs is pretended to be the same as a command by hand. + 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): + """The control for the test above: it is the plan's answer that fails the + verification, not the refused pull request. Sent to the spec leg, as + `plan` sends it, the pin verifies and the run is exit 0.""" + result, github = github_run(tmp_path, "never", tree=FAILING_TREE) + + assert result.returncode == 0, result.stdout + result.stderr + assert VERIFIED in result.stdout, result.stdout + assert "Open the pull request by hand" not in result.stderr + assert_pushed(github) diff --git a/tests/test_repo_hygiene.py b/tests/test_repo_hygiene.py index 4c9641da..b152764c 100644 --- a/tests/test_repo_hygiene.py +++ b/tests/test_repo_hygiene.py @@ -561,8 +561,8 @@ def test_agents_md_is_short_enough_to_be_read(): warning-in-CI the ratified text calls an amendment of the doctrine rather than a realization of it. - 2026-10-12: 464 -> 469 -- step 7 of "Adopting an existing repository" - (#304). Five lines, inside that one numbered point and nowhere else. It + 2026-10-12: 464 -> 468 -- step 7 of "Adopting an existing repository" + (#304). Four lines, inside that one numbered point and nowhere else. It said that ANY non-zero exit of `adopt-project.py execute` is to fix the plan and re-run into fresh legs, which holds for a failed verification (exit 1) and for neither of the two exits a GitHub run adds: a refused @@ -571,11 +571,13 @@ def test_agents_md_is_short_enough_to_be_read(): a command to finish by hand, so an assistant that followed the sentence abandoned a pushed adoption for a second pair of legs. The lines buy what the point could not say: that the exit decides, which message marks the - two (`IS pushed`), that the second pair is the harm, and that a refused - pull request has printed no table, so the split it opens is unverified. + two (`IS pushed`), and that the second pair is the harm. They are four + and not five because `execute` now verifies before it exits on a refused + `gh pr create` too, so both exits are of a split that passed the table + and the point need not say that one of them printed none (fix round 1). `tests/test_adopt_github_exits.py` holds the point to both printed exits.""" lines = (REPO / "AGENTS.md").read_text().splitlines() - assert len(lines) <= 469, f"AGENTS.md is {len(lines)} lines; the cap is 469" + assert len(lines) <= 468, f"AGENTS.md is {len(lines)} lines; the cap is 468" def test_claude_md_points_at_agents_md(): From 57abe9314f24b47551a7d20935bde5ac7180031f Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 12:43:00 +0000 Subject: [PATCH 3/5] adopt-project.py: a closed stdout no longer takes the by-hand `gh pr create` down with it, the command it prints carries its title and body, and `NEXT: review the pull request` is not said where none was opened, so `execute ... | head` after a refused pull request ends in the command and not a traceback, and a person with no terminal can finish with what it prints (#304) - `_open_it_by_hand` flushes stdout through `_flush_stdout()`, which is lane 1's #188 helper under its own name and shape (`contextlib.suppress(OSError)`), at the same place in the file, so the two merge to one definition. The unguarded `sys.stdout.flush()` raised BrokenPipeError before the command was printed; the run now prints it and exits as main did (Python's 120 for a stdout it cannot flush as it exits). A test drives the real CLI with a stdout whose reading end is closed. - The printed command is `_pull_request_command`, the one `_open_the_pull_request` runs, shell-quoted: `gh pr create` without `--title` and `--body` exits 1 where there is no terminal to prompt on. A test compares the printed command with the arguments the stub `gh` was refused with, and never runs it. - `cmd_execute` says `NEXT: review the pull request` (moved out of `_verify`) only when a pull request exists; after a refused one the by-hand command is the next step and the last line. - The EXIT CODES paragraph says "other exit 2s, once the leg repositories exist" and "Two of them are GitHub mode's alone", where it said "two more exit 2s" and read as the whole list. - The e2e environment sets `GH_TOKEN` to a bogus token and `GH_CONFIG_DIR` to an empty directory and takes every directory that holds a real `gh` off PATH when `git`, `git-filter-repo` and the shell helpers are found at the same places without them. Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Sonnet 5.5 --- adopt-project.py | 78 ++++++++--- tests/test_adopt_github_exits.py | 218 ++++++++++++++++++++++++++++--- 2 files changed, 257 insertions(+), 39 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index 68b379e1..cb91753a 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -78,7 +78,8 @@ EXIT CODES: 0 done · 1 findings (a plan that does not check out; a verification mismatch) · 2 a refusal — the question could not be asked. -`execute` has two more exit 2s, GitHub mode's alone, and both come AFTER both +`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 @@ -91,9 +92,11 @@ from __future__ import annotations import argparse +import contextlib import datetime as _dt import os import re +import shlex import shutil import subprocess import sys @@ -2977,6 +2980,20 @@ def _refuse_an_unrunnable_plan(plan: Plan, source: Source) -> None: "goes missing.") +def _flush_stdout() -> None: + """Flush stdout before a failure is written to stderr (#171, #177, #188). + + `execute` announces each leg repository, each pushed leg and the split on + stdout. Piped, stdout is block-buffered and stderr is not, so without + this a log that merges the two (`> log 2>&1`) shows the failure BEFORE the + lines it is about. A stdout nobody reads any more, such as a closed pipe, + must not turn the report into a BrokenPipeError, so that error is + swallowed: the report on stderr is what the human needs. + """ + with contextlib.suppress(OSError): + sys.stdout.flush() + + def _create_leg_remotes(plan: Plan, names: dict, repositories: dict, urls: dict, tracking: str, local: bool) -> None: """(a) The two NEW repositories. The assembly root is never created here.""" @@ -3526,6 +3543,18 @@ def _build_the_legs(source: Source, names: dict, repositories: dict, return leg_commits, leg_digests +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).""" + return ["gh", "pr", "create", "--repo", repositories["assembly"], + "--base", tracking, "--head", branch, + "--title", f"Adopt the three-repository shape: " + f"{names['spec']} and {names['code']}", + "--body", message] + + 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 @@ -3539,11 +3568,8 @@ def _open_the_pull_request(repositories: dict, names: dict, tracking: str, out of `cmd_execute` for #138. """ try: - url = run(["gh", "pr", "create", "--repo", repositories["assembly"], - "--base", tracking, "--head", branch, - "--title", f"Adopt the three-repository shape: " - f"{names['spec']} and {names['code']}", - "--body", message]) + url = run(_pull_request_command(repositories, names, tracking, branch, + message)) print(f" pull request {url}") except CommandFailed as exc: print(exc.loudly("opening the pull request"), file=sys.stderr) @@ -3551,26 +3577,31 @@ def _open_the_pull_request(repositories: dict, names: dict, tracking: str, return True -def _open_it_by_hand(repositories: dict, tracking: str, branch: str, - passed: bool) -> None: +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. 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. + 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`). """ - sys.stdout.flush() + _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) return - print("The branch IS pushed. Open the pull request by hand:\n" - f" gh pr create --repo {repositories['assembly']} " - f"--base {tracking} --head {branch}", file=sys.stderr) + 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) def _set_the_topic(repositories: dict, topic: str) -> bool: @@ -3809,7 +3840,10 @@ def cmd_execute(args) -> int: verified = _verify(source, assembly, work_root, names, paths_for, split_commit, spec_path, code_path, seeded) if pull_request_refused: - _open_it_by_hand(repositories, tracking, branch, not verified) + _open_it_by_hand(repositories, names, tracking, branch, message, + not verified) + elif not verified: + _next_steps(names) if verified: # A verification mismatch outranks a refused pull request and a # missing topic. @@ -3984,10 +4018,16 @@ def _verify(source: Source, assembly: Path, work_root: Path, names, "another exit.", file=sys.stderr) return 1 print("\nadoption verified: every source path is in exactly one place") + return 0 + + +def _next_steps(names: dict) -> 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).""" print(f"\nNEXT: review the pull request on {names['assembly']}, then\n" f" git clone --recurse-submodules <{names['assembly']} url>\n" " make bootstrap") - return 0 def _account_for(before: dict, after: dict, drops: list[str]) -> tuple[dict, list]: diff --git a/tests/test_adopt_github_exits.py b/tests/test_adopt_github_exits.py index 410c4b8c..6ec6f811 100644 --- a/tests/test_adopt_github_exits.py +++ b/tests/test_adopt_github_exits.py @@ -27,6 +27,17 @@ `git filter-repo` as every `execute` test does. Reading step 7, and the module's own EXIT CODES paragraph, runs everywhere. +THE REAL `gh` MUST NOT BE REACHABLE FROM A RUN. The shim is first on PATH, but +`subprocess` goes on to the next directory when the program it found cannot be +executed, and a `gh` there would be the operator's. So the run's environment +also carries an unauthenticated token and an empty config directory, and takes +every directory that holds a `gh` out of PATH where it can (`without_a_real_gh`). + +The command a refused `gh pr create` leaves to do by hand is the command that +was refused, title and body included, because `gh` will not take a pull +request without them where there is no terminal to ask on. The tests parse it +and compare it with what the shim was run with; none of them runs it. + `adopt-project.py`'s own two messages are asserted here WORD FOR WORD, and the anchors step 7 gives (`IS pushed`, `gh pr create`, the topic) are asserted against them, so rewording either side alone fails a test instead of leaving @@ -37,6 +48,7 @@ import importlib.util import os +import shlex import shutil import stat import subprocess @@ -96,15 +108,24 @@ "above did not pass verification.") 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 +#: existed to review. +NEXT = f"NEXT: review the pull request on {NORTHWIND}" +BY_HAND = "Open the pull request by hand:" #: `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 -#: token without the permission is. +#: token without the permission is, after writing the arguments it was given, +#: NUL-separated, to $FAKE_GH_ROOT/refused-arguments. GH_SHIM = textwrap.dedent("""\ #!/bin/sh 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";; @@ -125,6 +146,33 @@ def adopter(): return module +#: What a run needs to find on PATH besides the shim: `git`, the `git +#: filter-repo` it dispatches to, the interpreter that script names, and what +#: git's own shell helpers call. +NEEDED_ON_PATH = ("git", "git-filter-repo", "python3", "python", "sh", "env", + "sed", "tr", "uname", "cat") + + +def without_a_real_gh(path: str, needed=NEEDED_ON_PATH) -> str: + """`path` with every directory that holds a `gh` taken out, so that a run + whose shim cannot be executed has no operator's `gh` to fall through to. + + Taken out only when nothing is lost: each program in `needed` must be found + at the same place without those directories as with them. `/usr/bin` on + Linux holds `gh` beside `git` and `sed`, and Homebrew's `bin` holds a `gh` + beside the `git` a run is meant to use, so on those `path` comes back + whole, and what keeps the real `gh` from acting is the unauthenticated + token and the empty config directory the environment also carries.""" + kept = os.pathsep.join( + directory for directory in path.split(os.pathsep) + if not any(os.path.isfile(os.path.join(directory, name)) + for name in ("gh", "gh.exe"))) + if all(shutil.which(program, path=kept) == shutil.which(program, path=path) + for program in needed): + return kept + return path + + def step_7() -> str: """Step 7 of AGENTS.md, from its heading to the blank line after it, as one line of single-spaced text.""" @@ -133,7 +181,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): + keep_contracts: bool = False, merge_stderr: bool = False, + stdout_closed: bool = False): """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. @@ -142,7 +191,9 @@ def github_run(tmp_path: Path, refuses: str, tree: dict = SOURCE_TREE, the root, which fails the verification. `merge_stderr` runs with both streams in one pipe, as a person redirecting a log has them: stdout is then block-buffered, and the order the lines arrive in is the order the - tool flushed them.""" + 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.""" github = tmp_path / "github" (github / ORG).mkdir(parents=True) source = make_source_repo(tmp_path / NORTHWIND, tree=tree, edits=()) @@ -159,6 +210,11 @@ def github_run(tmp_path: Path, refuses: str, tree: dict = SOURCE_TREE, config = tmp_path / "gitconfig" config.write_text(f'[url "{github.as_posix()}/"]\n' "\tinsteadOf = https://github.com/\n", encoding="utf-8") + # The operator's `gh` must not be reachable from here: no token it would + # accept, no config to read a login from, and (where it can be done + # without losing `git`) no directory that holds one on PATH. + gh_config = tmp_path / "gh-config" + gh_config.mkdir() plan = tmp_path / "adoption-plan.yaml" written = write_plan(source, plan) @@ -169,23 +225,49 @@ def github_run(tmp_path: Path, refuses: str, tree: dict = SOURCE_TREE, with plan.open("w", encoding="utf-8", newline="\n") as handle: handle.write(answered.replace(CONTRACTS_TO_SPEC, KEEP_CONTRACTS, 1)) - result = subprocess.run( - [sys.executable, str(ADOPT), "execute", "--plan", str(plan), "--yes", - "--work-dir", str(tmp_path / "work")], - stdin=subprocess.DEVNULL, stdout=subprocess.PIPE, - stderr=subprocess.STDOUT if merge_stderr else subprocess.PIPE, - text=True, check=False, - env={**os.environ, "LANES_LANE": "", - "GIT_AUTHOR_NAME": "openRepoShape tests", - "GIT_AUTHOR_EMAIL": "tests@openreposhape.invalid", - "GIT_COMMITTER_NAME": "openRepoShape tests", - "GIT_COMMITTER_EMAIL": "tests@openreposhape.invalid", - "PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}", - "GIT_CONFIG_GLOBAL": str(config), "GIT_ALLOW_PROTOCOL": "file", - "FAKE_GH_ROOT": str(github), "FAKE_GH_REFUSES": refuses}) + stdout = subprocess.PIPE + if stdout_closed: + read_end, stdout = os.pipe() + os.close(read_end) + try: + result = subprocess.run( + [sys.executable, str(ADOPT), "execute", "--plan", str(plan), + "--yes", "--work-dir", str(tmp_path / "work")], + stdin=subprocess.DEVNULL, stdout=stdout, + stderr=subprocess.STDOUT if merge_stderr else subprocess.PIPE, + text=True, check=False, + env={**os.environ, "LANES_LANE": "", + "GIT_AUTHOR_NAME": "openRepoShape tests", + "GIT_AUTHOR_EMAIL": "tests@openreposhape.invalid", + "GIT_COMMITTER_NAME": "openRepoShape tests", + "GIT_COMMITTER_EMAIL": "tests@openreposhape.invalid", + "PATH": os.pathsep.join( + [str(bin_dir), without_a_real_gh(os.environ["PATH"])]), + "GH_TOKEN": "bogus-token-for-tests", + "GH_CONFIG_DIR": str(gh_config), + "GIT_CONFIG_GLOBAL": str(config), + "GIT_ALLOW_PROTOCOL": "file", + "FAKE_GH_ROOT": str(github), "FAKE_GH_REFUSES": refuses}) + finally: + if stdout_closed: + os.close(stdout) return result, github +def refused_arguments(github: Path) -> list[str]: + """The arguments the shim was run with when it refused: the command + `adopt-project.py` tried, after `gh`.""" + recorded = (github / "refused-arguments").read_bytes().decode("utf-8") + return recorded.split("\0")[:-1] + + +def printed_command(text: str) -> list[str]: + """The command printed after `Open the pull request by hand:`, as the + arguments a shell would give it. Everything after that line is the command, + for it is the last thing the run says. It is parsed and never run.""" + return shlex.split(text.split(BY_HAND, 1)[1]) + + def heads(repository: Path) -> list[str]: """The branches `repository` (a bare one) holds.""" listed = git("for-each-ref", "--format=%(refname:short)", "refs/heads", @@ -237,7 +319,11 @@ def test_the_modules_exit_codes_paragraph_names_the_exits_after_the_push( not be asked". It says what comes after the push, in the tool's own words, so the file and the messages it describes cannot part.""" codes = " ".join(adopter.__doc__.split("EXIT CODES:", 1)[1].split()) - assert "two more exit 2s" in codes, codes + # Not every exit 2 is the question that could not be asked, and not every + # other one is GitHub's: two of them are. + assert "other exit 2s, once the leg repositories exist" in codes, codes + assert "Two of them are GitHub mode's alone" in codes, codes + assert "two more exit 2s" not in codes, codes assert "AFTER both legs and the split branch are pushed" in codes, codes assert "the verification has passed" in codes, codes assert "`gh pr create` refused (`The branch IS pushed`" in codes, codes @@ -282,6 +368,10 @@ def test_each_exit_after_the_push_is_exit_2_with_a_command_to_finish_by_hand( # 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 + # `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. + assert (NEXT in result.stdout) is opened, result.stdout # What step 7 says of it is what the tool printed: the marker words and # the command it names. @@ -306,8 +396,57 @@ def test_the_command_to_open_a_refused_pull_request_comes_after_the_table( verified = result.stdout.index(VERIFIED) by_hand = result.stdout.index(BRANCH_IS_PUSHED) assert table < verified < by_hand, result.stdout - assert result.stdout.count("Open the pull request by hand") == 1 - assert result.stdout.rstrip().endswith(BRANCH_IS_PUSHED), result.stdout + assert result.stdout.count(BY_HAND) == 1 + # Nothing follows the command: the whole of what comes after it parses as + # the command, whose last argument is the body. + printed = printed_command(result.stdout) + assert printed[:3] == ["gh", "pr", "create"], printed + assert printed[-2] == "--body", printed + assert NEXT not in result.stdout, result.stdout + assert_pushed(github) + + +@WINDOWS_SKIP # the `gh` is a `#!` script, which Windows does not run +@needs_filter_repo +def test_the_command_printed_for_a_refused_pull_request_is_the_one_refused( + tmp_path): + """The printed command has its title and body. `gh pr create` without + them asks for them, and where there is no terminal it refuses instead + (`must provide --title and --body ... when not running interactively`), so + an assistant told to finish with the command it prints could not. It is the + command `adopt-project.py` ran, argument for argument: what the shim was + run with is read back, and the printed command is parsed and compared with + it. Nothing here runs the printed command.""" + result, github = github_run(tmp_path, "pr create") + + assert result.returncode == 2, result.stdout + result.stderr + refused = refused_arguments(github) + printed = printed_command(result.stderr) + assert refused[:2] == ["pr", "create"], refused + assert "--title" in refused and "--body" in refused, refused + assert printed == ["gh", *refused], (printed, refused) + # The body is the split's commit message, which says what moved where. + body = refused[refused.index("--body") + 1] + assert body.startswith("Adopt the three-repository shape: "), body + assert "MOVED TO THE CODE LEG" in body and "\n" in body, body + + +@WINDOWS_SKIP # the `gh` is a `#!` script, which Windows does not run +@needs_filter_repo +def test_a_closed_stdout_does_not_lose_the_command_to_open_the_pull_request( + tmp_path): + """`execute ... | head`, once `head` has quit, leaves a stdout nobody reads. + The command is on stderr, so it is still printed: flushing stdout first, + which the order test needs, raised BrokenPipeError there and ended the run + in a traceback with no command, where it was printed before #304 and + the exit was Python's own 120 for a stdout it cannot flush at exit.""" + result, github = github_run(tmp_path, "pr create", stdout_closed=True) + + assert "Traceback" not in result.stderr, result.stderr + assert BRANCH_IS_PUSHED in result.stderr, result.stderr + # 2 is the tool's own exit; 120 is what Python makes of it when it cannot + # flush the closed stdout as it exits, and what this run gave before. + assert result.returncode in (2, 120), result.stderr assert_pushed(github) @@ -331,6 +470,7 @@ def test_a_failed_verification_outranks_a_refused_pull_request(tmp_path): # Nothing tells the person to open one, or that the adoption is done. assert "Open the pull request 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 assert "VERIFICATION" in result.stdout, result.stdout # The legs and the branch exist, as the footer says, and no re-run into @@ -348,5 +488,43 @@ def test_the_same_source_verifies_when_gh_opens_the_pull_request(tmp_path): assert result.returncode == 0, result.stdout + result.stderr assert VERIFIED in result.stdout, result.stdout + assert NEXT in result.stdout, result.stdout assert "Open the pull request by hand" not in result.stderr assert_pushed(github) + + +# --- the real `gh` stays out of reach --------------------------------------- + +def program(directory: Path, *names: str) -> None: + """Executable files called `names` in `directory`.""" + directory.mkdir(parents=True, exist_ok=True) + for name in names: + (directory / name).write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") + (directory / name).chmod(0o755) + + +@WINDOWS_SKIP # `shutil.which` reads PATHEXT there, and nothing here runs +def test_the_directory_that_holds_a_real_gh_is_taken_off_path_only_when_free( + tmp_path): + """A directory with a `gh` and nothing the run needs goes; one that holds + `gh` beside `git`, or whose going changes which `git` is found, stays, + because losing `git` fails the run and swapping it changes what is + tested. Then the token and the config directory are what is left.""" + system, extra, only_gh = (tmp_path / "system", tmp_path / "extra", + tmp_path / "only-gh") + program(system, "git", "sed") + program(only_gh, "gh") + path = os.pathsep.join(map(str, (only_gh, system))) + needed = ("git", "sed") + assert without_a_real_gh(path, needed) == str(system) + + # `gh` beside `git`: removing the directory would lose `git`. + program(extra, "gh", "git", "sed") + shared = os.pathsep.join(map(str, (extra, system))) + assert without_a_real_gh(shared, needed) == shared + # `gh` beside the `git` that comes first: the other `git` would be used. + program(extra, "gh", "git") + program(system, "git") + assert without_a_real_gh(shared, ("git",)) == shared + # No `gh` anywhere, and nothing to take out. + assert without_a_real_gh(str(system), needed) == str(system) From d45854f49cd744bff48781f13c3242ec2a6d3185 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 12:46:23 +0000 Subject: [PATCH 4/5] tests/test_adopt_github_exits.py: the stub `gh` records the token and config directory it was run with and a test holds them to the bogus token and the empty directory, so the guard that keeps the operator's real `gh` unauthenticated if the stub ever cannot be executed is itself guarded (#304) Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Sonnet 5.5 --- tests/test_adopt_github_exits.py | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/tests/test_adopt_github_exits.py b/tests/test_adopt_github_exits.py index 6ec6f811..0a840a01 100644 --- a/tests/test_adopt_github_exits.py +++ b/tests/test_adopt_github_exits.py @@ -117,9 +117,12 @@ #: $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 #: token without the permission is, after writing the arguments it was given, -#: NUL-separated, to $FAKE_GH_ROOT/refused-arguments. +#: 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. GH_SHIM = textwrap.dedent("""\ #!/bin/sh + printf '%s\\n%s\\n' "$GH_TOKEN" "$GH_CONFIG_DIR" > "$FAKE_GH_ROOT/environment" case "$1 $2" in "$FAKE_GH_REFUSES") : > "$FAKE_GH_ROOT/refused-arguments" @@ -495,6 +498,23 @@ def test_the_same_source_verifies_when_gh_opens_the_pull_request(tmp_path): # --- the real `gh` stays out of reach --------------------------------------- +@WINDOWS_SKIP # the `gh` is a `#!` script, which Windows does not run +@needs_filter_repo +def test_a_run_gives_gh_no_token_it_would_accept_and_no_config(tmp_path): + """Whatever `gh` the run reaches is run with what the shim was run with: a + token no organisation knows, and a config directory with nothing in it, so + that the operator's login is not there to be used if the shim ever cannot + be executed and the next `gh` on PATH answers instead.""" + result, github = github_run(tmp_path, "pr create") + + assert result.returncode == 2, result.stdout + result.stderr + token, config, _ = (github / "environment").read_text( + encoding="utf-8").split("\n") + assert token == "bogus-token-for-tests", token + assert Path(config) == tmp_path / "gh-config", config + assert list(Path(config).iterdir()) == [], config + + def program(directory: Path, *names: str) -> None: """Executable files called `names` in `directory`.""" directory.mkdir(parents=True, exist_ok=True) From c5a8a557da743021000fe24cd1c4a0bf1743b7d3 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 12:51:55 +0000 Subject: [PATCH 5/5] tests/test_adopt_github_exits.py: the printed-command test holds the pull request's title to the two leg names and to the first line of the body, so a title that part from the split's message fails a test and no longer only changes what `gh` is run with (#304) Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Sonnet 5.5 --- tests/test_adopt_github_exits.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/test_adopt_github_exits.py b/tests/test_adopt_github_exits.py index 0a840a01..d89a46a1 100644 --- a/tests/test_adopt_github_exits.py +++ b/tests/test_adopt_github_exits.py @@ -428,9 +428,13 @@ def test_the_command_printed_for_a_refused_pull_request_is_the_one_refused( assert refused[:2] == ["pr", "create"], refused assert "--title" in refused and "--body" in refused, refused assert printed == ["gh", *refused], (printed, refused) - # The body is the split's commit message, which says what moved where. + # The title names the two legs, and is the first line of the body: the + # split's commit message, which says what moved where. + title = refused[refused.index("--title") + 1] body = refused[refused.index("--body") + 1] - assert body.startswith("Adopt the three-repository shape: "), body + assert title == ("Adopt the three-repository shape: " + 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