From e1692c147bc3b61951414a04a24f4e2d55fd484d Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sun, 8 Nov 2026 16:36:56 +0000 Subject: [PATCH 1/5] update-shape.py: `prepare` asks the shape pin's `source_repository:` once, before the upstream opens and with `--upstream` passed too, refusing a path or a value that is no `owner/repo` in the pin's name; shape-doctor.py answers as it did (#1203) `prepare` read the pin's `source_repository:` and used it two ways. With no `--upstream` it was the upstream `open_upstream` opened, and a value naming something in the working directory that is no repository (`contracts`, `scripts/repo_shape.py`) was `update-upstream-not-a-repo: --upstream contracts exists but is not a git repository`, a flag nobody passed. With `--upstream` passed it was never asked: a value ending in a line feed went into `apply`'s commit subject and the `gh pr create` line it prints, both broken by it, exit 0 (#1166's review, findings 3 and 9). `pin_source` now asks it once, in `prepare`, before the upstream is opened, whichever way the run reaches the upstream, under the codes these values had: something there that holds no repository is `update-upstream-not-a-repo`, a value `REPOSITORY_RE` does not take whole is `update-upstream-unresolvable`, and `checked_value`'s is `unsafe-value`, each naming the pin and its field. A path is refused with `--upstream` passed too, and a clone kept at a path that is not spelled `owner/repo` is refused with no `--upstream` (main opened it in place): the pin records the name `apply` commits and opens its pull request with. The remedy no longer offers `--upstream` a path, which asks the field too. An `owner/repo` spelling that is a repository where the command runs is taken, as it was. `open_upstream` asks `--upstream` alone again. `shape-doctor.py` passes `prepare(..., ask_source=False)` at its two calls: it reads a refusal out of `prepare` as its own checkout unable to answer, so asking there turned a hand-edited field into CANNOT ANSWER, exit 3, with a `git fetch --all` NEXT (measured). Its rows are main's. Closes #1203 Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Opus 5.5 --- shape-doctor.py | 11 +- tests/test_update_shape_pin_source.py | 276 +++++++++++++++++++++++++ tests/test_update_shape_unreadable.py | 4 +- tests/test_value_pattern_line_break.py | 29 ++- update-shape.py | 130 +++++++++--- 5 files changed, 409 insertions(+), 41 deletions(-) create mode 100644 tests/test_update_shape_pin_source.py diff --git a/shape-doctor.py b/shape-doctor.py index 4a8c353c..a193456e 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -3353,8 +3353,9 @@ def refusals_ahead(ctx: Context, us) -> dict | None: args = argparse.Namespace(root=str(ctx.root), upstream=str(ctx.shape), at=None) try: + # `ask_source=False`, as `check_shape_currency` asks it (#1203). root, _pin, rows, _added, upstream, _pinned, _target, kind = \ - us.prepare(args) + us.prepare(args, ask_source=False) except Refusal: return None try: @@ -6312,8 +6313,14 @@ def check_shape_currency(ctx: Context) -> Row: if partial is not None: return partial try: + # THE PIN'S `source_repository:` IS NOT ASKED HERE (#1203). A + # `Refusal` out of `prepare` is this checkout unable to answer + # (below), and that field is the root's: asked here, a hand edit of + # it was CANNOT ANSWER with a `fetch --all` that mends nothing. + # `update-shape.py check`, this row's NEXT where the root is behind, + # asks it, in the pin's name. _root, _pin, rows, additions, upstream, pinned, target, _kind = \ - us.prepare(args) + us.prepare(args, ask_source=False) except Refusal as exc: if exc.code == us.UPSTREAM_UNREADABLE: # A READ THAT FAILED, NOT A COMMIT THAT IS NOT HERE (#683), so diff --git a/tests/test_update_shape_pin_source.py b/tests/test_update_shape_pin_source.py new file mode 100644 index 00000000..2f097e3a --- /dev/null +++ b/tests/test_update_shape_pin_source.py @@ -0,0 +1,276 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`update-shape.py` asks the shape pin's `source_repository:` once, +whichever way a run reaches the upstream, and refuses a value that is no +`owner/repo` in the pin's name (#1203, from the review of #1166, findings 3 +and 9). + +THE DEFECT, measured on main be21751c (Python 3.12.3, git 2.43.0). `prepare` +read the pin's value and used it two ways: + +- with no `--upstream` it was the upstream `open_upstream` opened, and a + value that named something in the directory the command ran in that is + no repository (`contracts`, `scripts/repo_shape.py`, the pin itself) was + `REFUSED update-upstream-not-a-repo: --upstream contracts exists but is + not a git repository`, naming a flag nobody passed; +- with `--upstream` passed it was never asked. Every value below was + `check` exit 1 over the upstream one commit on, as the scaffolded pin is, + and `apply --yes --branch shape/fu-lf` of `"opensoft/openRepoShape\\n"` + exited 0 with a commit whose subject broke after the name. + +THE RULE NOW. `pin_source` asks the value in `prepare`, before the upstream +is opened, `--upstream` or not: something there that is no repository is +`update-upstream-not-a-repo`, a value `REPOSITORY_RE` does not take whole is +`update-upstream-unresolvable`, and `checked_value`'s is `unsafe-value`, +each naming the pin and its `source_repository:`. One the pin holds is +named before a bad `--upstream`. + +A PATH IS REFUSED WITH `--upstream` PASSED TOO, the decision #1203 asked +for: the value names the upstream in the commit and the pull request +`apply` makes, and no tool writes a path there. A path that holds a +repository and is not spelled `owner/repo` (`~/shape`) is refused in both +routes, where with no `--upstream` main opened it in place. An `owner/repo` +spelling that is a repository where the command runs is taken, as it was, +and with no `--upstream` it is opened in place. + +THE DOCTOR ANSWERS AS IT DID. `shape-doctor.py` calls `prepare` with its +own checkout as the upstream and reads a refusal out of it as that checkout +unable to answer; asked there, a hand-edited field was CANNOT ANSWER with a +`git fetch --all` for its remedy. It passes `ask_source=False`, and the +`update-shape.py check` its row names where the root is behind asks it. + +No network and no GitHub: every `--upstream` is a local clone, the A -> B +upstream of `test_update_shape.py`, and the one run with no `--upstream` +that would clone runs in this process, with a stand-in that records the +`owner/repo` it is handed and opens that upstream instead. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from conftest import REPO, git, run_script +from test_shape_doctor import doctor, rows_of +from test_update_shape import ( # noqa: F401 + CHANGED, UPDATE, assert_refused_and_unpinned, root, update_shape, + upstream_and_project, verdicts) +from test_value_pattern_line_break import LF, WRITE_THE_PIN, pin_of, rewritten + +#: The pin's line as both templates scaffold it. +SCAFFOLDED = "source_repository: opensoft/openRepoShape\n" +#: The words of the two refusals that ask what the value is. +NOT_A_REPO = "which exists but is not a git repository" +NOT_A_NAME = "which is not an `owner/repo` name" + + +def written_as(root: Path, value: str) -> None: + """The pin's `source_repository:` written double-quoted as `value`, as + a hand edit makes it, and committed, so `apply` starts from a clean + tree as the issue's measurement did.""" + rewritten(root / "contracts" / "shape-pin.yaml", SCAFFOLDED, + f'source_repository: "{value}"\n') + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", + "the pin's source, as a hand edit wrote it", "--", + "contracts/shape-pin.yaml", cwd=root) + + +def run(root: Path, command: str, *extra: str, cwd: Path | None = None, + env: dict | None = None): + """`update-shape.py ` of `root`, run from the root unless + `cwd` says otherwise: where a value such as `contracts` names a + folder.""" + return run_script(UPDATE, command, "--root", str(root), *extra, + cwd=cwd or root, env=env) + + +def upstream_of(upstream_and_project: dict) -> tuple[str, str]: + """`--upstream`, the local clone the A -> B fixture made.""" + return ("--upstream", str(upstream_and_project["upstream"])) + + +def refused(root: Path, code: str, read: str, said: str) -> list[str]: + """The two lines a refused pin's value is told, whole.""" + return [f"REFUSED {code}: {pin_of(root)}: `source_repository:` is " + f"{read}, {said}", WRITE_THE_PIN] + + +ROUTES = [ + pytest.param(False, id="no-upstream"), + pytest.param(True, id="upstream-a-clone"), +] + +#: `(the value as the pin writes it, double-quoted; as the refusal reads +#: it; the code; the words)`: the issue's table. +VALUES = [ + pytest.param("contracts", "'contracts'", "update-upstream-not-a-repo", + NOT_A_REPO, id="a-folder"), + pytest.param("scripts/repo_shape.py", "'scripts/repo_shape.py'", + "update-upstream-not-a-repo", NOT_A_REPO, + id="a-file-spelled-owner-repo"), + pytest.param("contracts/shape-pin.yaml", "'contracts/shape-pin.yaml'", + "update-upstream-not-a-repo", NOT_A_REPO, + id="the-pin-itself"), + pytest.param("opensoft/openRepoShape\\n", "'opensoft/openRepoShape\\n'", + "update-upstream-unresolvable", NOT_A_NAME, id="line-feed"), + pytest.param("opensoft/openRepoShape ", "'opensoft/openRepoShape '", + "update-upstream-unresolvable", NOT_A_NAME, + id="trailing-space"), + pytest.param("has space/x", "'has space/x'", + "update-upstream-unresolvable", NOT_A_NAME, id="a-space"), +] + + +@pytest.mark.parametrize("passed", ROUTES) +@pytest.mark.parametrize("value, read, code, said", VALUES) +def test_the_pins_source_is_asked_in_both_routes_in_the_pins_name( + root, upstream_and_project, passed, value, read, code, said): + """THE ISSUE'S TABLE, in both routes, run from the root. On main the + three paths were `update-upstream-not-a-repo` of `--upstream` with none + passed, the other three named the pin with a remedy that offered + `--upstream` a path, and with `--upstream` passed every one was exit 1 + and a report. + Each is refused now, exit 2, in the pin's name, before anything is + compared.""" + written_as(root, value) + result = run(root, "check", + *(upstream_of(upstream_and_project) if passed else ())) + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.splitlines() == refused(root, code, read, said), \ + result.stderr + assert result.stdout == "", result.stdout + + +@pytest.mark.parametrize("passed", ROUTES) +@pytest.mark.parametrize("kept, read, code, said", [ + pytest.param("shape", "'~/shape'", "update-upstream-unresolvable", + NOT_A_NAME, id="a-clone"), + pytest.param("notes", "'~/notes'", "update-upstream-not-a-repo", + NOT_A_REPO, id="a-folder"), +]) +def test_a_source_under_home_is_looked_up_as_open_upstream_looks( + root, upstream_and_project, tmp_path, passed, kept, read, code, + said): + """`~/`, with `~` expanded as `open_upstream` expands it. A clone + of the upstream there is THE DECISION's row: with no `--upstream` main + opened it in place (exit 1, the copies compared), and with one passed + it was never asked; it is a path and no `owner/repo` name, so it is + refused in both routes. A folder there that holds no repository is + `update-upstream-not-a-repo` in the pin's name, as `contracts` is.""" + home = tmp_path / "home" + (home / "notes").mkdir(parents=True) + git("clone", "-q", str(upstream_and_project["upstream"]), + str(home / "shape"), cwd=tmp_path) + written_as(root, f"~/{kept}") + result = run(root, "check", + *(upstream_of(upstream_and_project) if passed else ()), + env={"HOME": str(home), "USERPROFILE": str(home)}) + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.splitlines() == refused(root, code, read, said), \ + result.stderr + + +def test_a_source_checked_value_refuses_is_refused_with_upstream_passed( + root, upstream_and_project): + """`checked_value`'s refusal, of a value `REPOSITORY_RE` takes: with no + `--upstream` it names the pin since #1139; with one passed, main never + asked it and `check` exited 1.""" + written_as(root, "-opensoft/openRepoShape") + result = run(root, "check", *upstream_of(upstream_and_project)) + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.splitlines()[0] == ( + f"REFUSED unsafe-value: {pin_of(root)}: `source_repository:` is " + "'-opensoft/openRepoShape', which is not a value this tool will put " + "on a `git` or `gh` command line"), result.stderr + + +def test_the_pins_source_is_named_before_a_bad_upstream(root): + """Asked before the upstream is opened: where the pin's value and + `--upstream` are both refused, the pin is named first, and no clone is + started for a flag whose run would end in the pin's refusal. On main + the flag was named.""" + written_as(root, "opensoft/openRepoShape\\n") + result = run(root, "check", "--upstream", "opensoft/openRepoShape" + LF) + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.splitlines() == refused( + root, "update-upstream-unresolvable", "'opensoft/openRepoShape\\n'", + NOT_A_NAME), result.stderr + + +def test_apply_over_a_refused_source_writes_nothing(root, + upstream_and_project): + """THE ISSUE'S `apply`: on main, over the upstream one commit on, `apply + --yes --branch shape/fu-lf` exited 0 and committed `Re-sync the shape + copies to opensoft/openRepoShape`, a line feed, then ` @ `. It is + refused now before the upstream is opened: no branch, no commit, the + pin's `commit:` as it was and the tree clean.""" + written_as(root, "opensoft/openRepoShape\\n") + head = git("rev-parse", "HEAD", cwd=root).stdout + result = run(root, "apply", "--yes", *upstream_of(upstream_and_project), + "--at", upstream_and_project["b"], "--branch", "shape/fu-lf") + assert_refused_and_unpinned( + result, root, upstream_and_project, "update-upstream-unresolvable", + refused(root, "update-upstream-unresolvable", + "'opensoft/openRepoShape\\n'", NOT_A_NAME)[0]) + assert git("rev-parse", "HEAD", cwd=root).stdout == head + assert git("branch", "--list", "shape/fu-lf", cwd=root).stdout == "" + assert git("status", "--porcelain", cwd=root).stdout == "" + + +def test_an_owner_repo_that_is_a_clone_where_it_runs_is_opened_in_place( + root, upstream_and_project, tmp_path): + """UNCHANGED, AND ON PURPOSE. The scaffolded `opensoft/openRepoShape`, + run with no `--upstream` from a folder that keeps a clone at + `opensoft/openRepoShape`, as a checkout laid out by owner does: main + opened that clone in place, and it still does, so `check` answers as it + does with `--upstream` naming the clone.""" + here = tmp_path / "here" + clone = here / "opensoft" / "openRepoShape" + git("clone", "-q", str(upstream_and_project["upstream"]), str(clone), + cwd=tmp_path) + in_place = run(root, "check", cwd=here) + passed = run(root, "check", "--upstream", str(clone), cwd=here) + assert in_place.returncode == 1, in_place.stdout + in_place.stderr + assert verdicts(in_place.stdout)[CHANGED] == "upstream-changed" + assert (in_place.returncode, in_place.stdout) == (passed.returncode, + passed.stdout) + + +def test_a_pin_as_scaffolded_hands_its_owner_repo_to_open_upstream( + update_shape, root, upstream_and_project, monkeypatch, capsys): + """UNCHANGED: the scaffolded pin with no `--upstream` reaches + `open_upstream` with `opensoft/openRepoShape`, which bare-clones it. + In this process, with a stand-in that records what it is handed and + opens the local upstream instead, `check` answers as it does with + `--upstream` naming that upstream. The stand-in takes, and drops, the + pin's path that main's `prepare` handed `open_upstream` too, so this + row is asked of main's code as it is of this.""" + real = update_shape.open_upstream + asked: list[str] = [] + + def opening(spec, *_pin_path): + asked.append(spec) + return real(str(upstream_and_project["upstream"])) + + monkeypatch.setattr(update_shape, "open_upstream", opening) + code = update_shape.main(["check", "--root", str(root)]) + out = capsys.readouterr().out + passed = run(root, "check", *upstream_of(upstream_and_project)) + assert asked == ["opensoft/openRepoShape"] + assert (code, out) == (passed.returncode, passed.stdout) + + +def test_the_doctor_does_not_call_a_hand_edited_source_the_standards(root): + """THE DOCTOR'S ANSWER IS MAIN'S. `shape-doctor.py` hands `prepare` its + own checkout as the upstream and reads a refusal out of it as that + checkout unable to answer, so, asked there, a hand edit of the field + was CANNOT ANSWER, exit 3, with a `git fetch --all` NEXT that mends + nothing (measured with `ask_source` left True). The doctor passes + `ask_source=False`: its row is no `environment` row, and it does not + exit 3. The doctor here is this checkout's, and so is the + `update-shape.py` it loads.""" + written_as(root, "opensoft/openRepoShape\\n") + result = doctor(REPO, root, "--json") + row = rows_of(result)["shape-currency"] + assert result.returncode != 3, result.stdout + result.stderr + assert "environment" not in row["detail"], row diff --git a/tests/test_update_shape_unreadable.py b/tests/test_update_shape_unreadable.py index c8282562..31947a84 100644 --- a/tests/test_update_shape_unreadable.py +++ b/tests/test_update_shape_unreadable.py @@ -225,8 +225,8 @@ def temporaries(update_shape, monkeypatch, tmp_path): real = update_shape.open_upstream made: list[Path] = [] - def opening(spec, pin_path=None): - upstream = real(spec, pin_path) + def opening(spec): + upstream = real(spec) temporary = tmp_path / f"openreposhape-upstream-{len(made)}" temporary.mkdir() (temporary / "HEAD").write_bytes(b"ref: refs/heads/main\n") diff --git a/tests/test_value_pattern_line_break.py b/tests/test_value_pattern_line_break.py index 2cded3c8..4f458c2b 100644 --- a/tests/test_value_pattern_line_break.py +++ b/tests/test_value_pattern_line_break.py @@ -35,7 +35,8 @@ read an id out of a file ask it with `fullmatch`: the two validators, and `family.py`'s reading of a member's `project.yaml`, which is `member-bad-id` now. `update-shape.py` names the pin's `source_repository:`, under the codes -it had, where no `--upstream` was passed. +it had, and since #1203 asks it whether or not `--upstream` is passed +(`tests/test_update_shape_pin_source.py`). LEFT AS THEY ARE, AND WHY: @@ -386,11 +387,15 @@ def pin_of(project: Path) -> Path: return project.resolve() / "contracts" / "shape-pin.yaml" -#: What a pin's `source_repository:` that is no `owner/repo` is told. +#: What a pin's `source_repository:` that is no `owner/repo` is told. Since +#: #1203 it is asked with `--upstream` passed too, so passing a path there is +#: no remedy for it, and the words no longer offer one. WRITE_THE_PIN = ( "Remediation: write `source_repository:` in the pin as the `owner/repo` " "its shape was copied from, `opensoft/openRepoShape` for this " - "standard's, or pass `--upstream` a path to a clone of openRepoShape.") + "standard's. It is asked with `--upstream` passed too, because `apply` " + "names it in its commit and its pull request; a path to a clone goes " + "to `--upstream`.") @pytest.mark.parametrize("value, read", [ @@ -403,13 +408,15 @@ def test_a_pins_source_that_is_no_owner_repo_is_named_as_the_pins( """THE ISSUE'S ROW, and one the pattern refused on main too. On main the line feed passed `REPOSITORY_RE` and was `unsafe-value` of `--upstream`, and the space was `update-upstream-unresolvable` of `--upstream`, with - no `--upstream` passed. Both are that code now, naming the pin.""" + no `--upstream` passed. Both are that code now, naming the pin. Since + #1203 a path is never what the pin holds, so the words no longer say + "neither a path on this machine".""" result = check_with_source(project, value) assert result.returncode == 2, result.stdout + result.stderr assert result.stderr.splitlines() == [ f"REFUSED update-upstream-unresolvable: {pin_of(project)}: " - f"`source_repository:` is {read}, which is neither a path on this " - "machine nor an `owner/repo` name", WRITE_THE_PIN], result.stderr + f"`source_repository:` is {read}, which is not an `owner/repo` name", + WRITE_THE_PIN], result.stderr def test_a_pins_source_checked_value_refuses_is_named_as_the_pins(project): @@ -426,9 +433,13 @@ def test_a_pins_source_checked_value_refuses_is_named_as_the_pins(project): def test_an_upstream_passed_with_a_line_feed_is_named_as_passed(project): """`--upstream` itself takes no line feed either: on main the pattern took it and `checked_value` refused it as `unsafe-value`; it is not an - `owner/repo` name now, and is named as passed, whatever the pin says.""" - result = check_with_source(project, "-opensoft/openRepoShape", - "--upstream", "opensoft/openRepoShape" + LF) + `owner/repo` name now, and is named as passed. The pin is as scaffolded: + since #1203 its own value is asked first, `--upstream` or not, and one + it refuses is named before the flag + (`tests/test_update_shape_pin_source.py`).""" + result = run_script(UPDATE, "check", "--root", str(project), + "--upstream", "opensoft/openRepoShape" + LF, + cwd=project) assert result.returncode == 2, result.stdout + result.stderr assert result.stderr.splitlines()[0] == ( "REFUSED update-upstream-unresolvable: --upstream " diff --git a/update-shape.py b/update-shape.py index 200cb843..1486a008 100755 --- a/update-shape.py +++ b/update-shape.py @@ -642,9 +642,20 @@ def unreadable_upstream(said: str, proc: subprocess.CompletedProcess "openRepoShape.") -def open_upstream(spec: str, pin_path: Path | None = None) -> Upstream: +def _holds_a_repository(path: Path) -> bool: + """Whether `path` is a git repository by the two questions + `open_upstream` asks of a path: a `.git` there, or, only where there is + none, a bare repository's `HEAD` file. Asked of the shape pin's + `source_repository:` too (`pin_source`), so the pin's value and + `--upstream` are one question (#1203).""" + return (_is_a(path / ".git", _anything) + or _is_a(path / "HEAD", stat.S_ISREG)) + + +def open_upstream(spec: str) -> Upstream: """`--upstream` as a repository this tool can read, or the shape pin's - `source_repository:` where none was passed, read from `pin_path`. + `source_repository:` where none was passed, once `pin_source` has asked + it. A PATH IS USED IN PLACE AND THIS TOOL FETCHES NOTHING INTO IT. That is deliberate: it is what lets the tests run with no network, and it is @@ -669,19 +680,17 @@ def open_upstream(spec: str, pin_path: Path | None = None) -> Upstream: then cloned from github.com; a checkout it cannot search was `update-upstream-not-a-repo`. - A VALUE THE PIN HOLDS IS REFUSED IN THE PIN'S NAME (#1139). The pin's - `source_repository:` read as `opensoft/openRepoShape` and a line feed - passed `REPOSITORY_RE`, whose `$` took the line feed, and was refused - by `checked_value` as `unsafe-value` of `--upstream`, a flag nobody had - passed. The pattern takes no line feed now, so the value is - `update-upstream-unresolvable`, and that refusal and `checked_value`'s - name the pin and its field. + THE PIN'S VALUE IS ASKED BEFORE IT REACHES HERE (#1139, #1203). + `pin_source` asks it in `prepare`, whether or not `--upstream` was + passed, and refuses in the pin's name a value that is no `owner/repo`, + or one that names something here that is no git repository. So where + `spec` is the pin's, none of the refusals below is reached for it, and + each of them names the flag. """ candidate = Path(spec).expanduser() if _is_a(candidate, _anything): path = candidate.resolve() - if (not _is_a(path / ".git", _anything) - and not _is_a(path / "HEAD", stat.S_ISREG)): + if not _holds_a_repository(path): raise Refusal( "update-upstream-not-a-repo", f"--upstream {spec} exists but is not a git repository", @@ -689,23 +698,13 @@ def open_upstream(spec: str, pin_path: Path | None = None) -> Upstream: "`--upstream opensoft/openRepoShape` to clone one.") return Upstream(path, str(path)) if not REPOSITORY_RE.match(spec): - if pin_path is not None: - raise Refusal( - "update-upstream-unresolvable", - f"{pin_path}: `source_repository:` is {spec!r}, which is " - "neither a path on this machine nor an `owner/repo` name", - "Remediation: write `source_repository:` in the pin as the " - "`owner/repo` its shape was copied from, " - f"`{SHAPE_REPOSITORY}` for this standard's, or pass " - "`--upstream` a path to a clone of openRepoShape.") raise Refusal( "update-upstream-unresolvable", f"--upstream {spec!r} is neither a path on this machine nor an " "`owner/repo` name", "Remediation: pass a path to a clone of openRepoShape, or " "`--upstream opensoft/openRepoShape`.") - checked_value("--upstream" if pin_path is None - else f"{pin_path}: `source_repository:`", spec) + checked_value("--upstream", spec) temporary = Path(tempfile.mkdtemp(prefix="openreposhape-upstream-")) url = f"https://github.com/{spec}.git" args = ["git", "clone", "--quiet", "--bare", url, str(temporary / "shape.git")] @@ -724,6 +723,66 @@ def open_upstream(spec: str, pin_path: Path | None = None) -> Upstream: return Upstream(temporary / "shape.git", spec, temporary=temporary) +#: What a shape pin's `source_repository:` that `pin_source` refuses is told. +PIN_SOURCE_REMEDY = ( + "Remediation: write `source_repository:` in the pin as the `owner/repo` " + f"its shape was copied from, `{SHAPE_REPOSITORY}` for this standard's. " + "It is asked with `--upstream` passed too, because `apply` names it in " + "its commit and its pull request; a path to a clone goes to " + "`--upstream`.") + + +def pin_source(pin: dict, pin_path: Path) -> str: + """The shape pin's `source_repository:`, the `owner/repo` its shape was + copied from (`SHAPE_REPOSITORY` where the pin names none), or a refusal + in the pin's name, `pin_path` naming the pin. + + ASKED ONCE, IN `prepare`, WHICHEVER WAY THE RUN REACHES THE UPSTREAM + (#1203). With no `--upstream` the value is the upstream `open_upstream` + opens. With one passed, the value was never asked, and it is still the + name `prepare` gives the upstream: the README `Shape:` line is compared + with it, and `apply` writes it into its commit subject, its pull + request's title and the lines it prints. So `"opensoft/openRepoShape\\n"` + made a commit whose subject broke after the name, and a `gh pr create` + line split inside its quotes, exit 0. With no `--upstream`, a path that + is no repository was `update-upstream-not-a-repo` of `--upstream`, a + flag nobody passed. + + THREE QUESTIONS, under the codes these values had (#1139): + - something HERE at the value, looked up as `open_upstream` looks it up + (`expanduser`, relative to the directory the command runs in), that + is no git repository: `update-upstream-not-a-repo`. The pin gets the + same answer whether or not `--upstream` is passed. An `owner/repo` + spelling that IS a repository here is taken, as it was, and with no + `--upstream` it is opened in place. + - a value `REPOSITORY_RE` does not take WHOLE: + `update-upstream-unresolvable`. A path that is not spelled + `owner/repo` (`/srv/shape`, `~/shape`, `shape`) is refused here, + whether it holds a repository or not: the pin records a repository's + name, never where a clone is kept. + - `checked_value`'s: `unsafe-value`. + + NOTHING WRITES A VALUE THESE REFUSE. Both pin templates hold + `{{SHAPE_REPOSITORY}}`, so each such value is a hand edit, and refusing + it costs no root that a tool made. + """ + declared = str(pin.get("source_repository") or SHAPE_REPOSITORY) + named = f"{pin_path}: `source_repository:`" + candidate = Path(declared).expanduser() + if (_is_a(candidate, _anything) + and not _holds_a_repository(candidate.resolve())): + raise Refusal( + "update-upstream-not-a-repo", + f"{named} is {declared!r}, which exists but is not a git " + "repository", PIN_SOURCE_REMEDY) + if not REPOSITORY_RE.fullmatch(declared): + raise Refusal( + "update-upstream-unresolvable", + f"{named} is {declared!r}, which is not an `owner/repo` name", + PIN_SOURCE_REMEDY) + return checked_value(named, declared) + + # --------------------------------------------------------------------------- # The root, its pin, and what each pinned row is # --------------------------------------------------------------------------- @@ -2391,8 +2450,9 @@ def counted(rows: list[Row]) -> dict[str, int]: # --------------------------------------------------------------------------- -def prepare(args) -> tuple[Path, dict, list[Row], list[Addition], Upstream, - str, str, Kind]: +def prepare(args, *, ask_source: bool = True + ) -> tuple[Path, dict, list[Row], list[Addition], Upstream, + str, str, Kind]: """The root, its pin, its rows judged and its additions found, the upstream opened, the two commits and the kind of root: everything `check` reports and `apply` acts on, read before anything is written. @@ -2404,7 +2464,17 @@ def prepare(args) -> tuple[Path, dict, list[Row], list[Addition], Upstream, answer. The root is asked first: its own `stat` (a folder above it this user cannot search), then whether it can be searched at all (`_search`). A read refused once the upstream is open closes it - first, as a `Refusal` does.""" + first, as a `Refusal` does. + + THE PIN'S `source_repository:` IS ASKED BEFORE THE UPSTREAM IS OPENED + (#1203), by `pin_source`, with or without `--upstream`. `ask_source` + False is `shape-doctor.py`'s alone: it passes its own checkout as the + upstream, writes no commit and opens no pull request, and it reads a + `Refusal` from here as that checkout unable to answer, so a refusal of + the root's own field would be called the standard's, with a `git fetch` + for its remedy; `check`, the NEXT its row names where the root is + behind, asks the field. With no `--upstream` the field IS the upstream, + and is asked whatever `ask_source` says.""" root = root_of(args) if not _is_a(root, stat.S_ISDIR): raise Refusal("update-root-missing", f"{root} is not a directory") @@ -2418,9 +2488,13 @@ def prepare(args) -> tuple[Path, dict, list[Row], list[Addition], Upstream, if linked is not None: raise rewrite_link_refusal([linked]) pinned = str(pin["commit"]).lower() - declared = str(pin.get("source_repository") or SHAPE_REPOSITORY) - upstream = open_upstream(str(args.upstream or declared), - None if args.upstream else root / SHAPE_PIN) + # THE PIN'S `source_repository:` IS ASKED HERE, ONCE, before the + # upstream is opened and whether or not `--upstream` was passed (#1203): + # it names the upstream in what `check` and `apply` print and commit. + declared = (pin_source(pin, root / SHAPE_PIN) + if ask_source or not args.upstream + else str(pin.get("source_repository") or SHAPE_REPOSITORY)) + upstream = open_upstream(str(args.upstream or declared)) upstream.repository = declared try: target = upstream.resolve(checked_value("--at", args.at) From 86c1e6ea28633888b4873740e3dae844fae4d79c Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sun, 8 Nov 2026 16:54:24 +0000 Subject: [PATCH 2/5] tests/test_doctor_unreadable_pin.py: the stand-in for `prepare` takes the `ask_source` keyword the doctor now passes, so the rows that raise an `OSError` out of the comparison raise it again (#1203) `shape-doctor.py` calls `prepare(args, ask_source=False)` since the commit before this one, and the stand-in took one argument, so its three rows ended in a `TypeError` instead of the error they raise. Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Opus 5.5 --- tests/test_doctor_unreadable_pin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_doctor_unreadable_pin.py b/tests/test_doctor_unreadable_pin.py index 6afe17a8..18beb66b 100644 --- a/tests/test_doctor_unreadable_pin.py +++ b/tests/test_doctor_unreadable_pin.py @@ -501,7 +501,7 @@ def test_an_oserror_the_root_did_not_refuse_is_raised_as_it_was( module, ctx = in_process(standard, project) error = raised(standard, ctx.root) - def prepare(_args): + def prepare(_args, **_ask_source): raise error monkeypatch.setattr(ctx.update_shape(), "prepare", prepare) From 175d9b7185895f7b88df8f63ee7ff192a2ba8ab4 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sun, 8 Nov 2026 18:50:58 +0000 Subject: [PATCH 3/5] update-shape.py and shape-doctor.py: a pin's `source_repository:` of `~/...` or over 255 bytes is nothing there and is refused in the pin's name, the doctor's `pins` row asks the field, a folder-named value is told where to run (#1203) Review of PR #1269 (rv1269), two SHOULDs and the NITs it asked for: - S1: `Path(declared).expanduser()` raised `RuntimeError` for `~/...` with no such user, a traceback with exit 1, the code `check` uses for "behind". `_named_here` keeps the bare path, so the name question refuses it `update-upstream-unresolvable` in the pin's name, with or without `--upstream`. - N1: a first part over 255 bytes made `stat` raise `ENAMETOOLONG`, which was `update-unreadable` naming neither the pin nor the field. That errno is nothing there; every other refused `stat` is raised as it was. - S2: `refusals_ahead` no longer passes `ask_source=False`. Where `check` refuses the pin's field, the `pins` row said `check` reports the copies `merged-away` and the `apply` it prints drops their rows; it exits 2 and does neither. `check_shape_currency` keeps the opt-out. - N2: the `update-upstream-not-a-repo` remedy of a pin's value says to run from a directory where nothing is at that path. - N3: `REPOSITORY_RE`'s comment no longer says it is for the pin "where no `--upstream` is passed". - N4: the doctor guard row holds the `shape currency` row to main's `ok`. Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Sonnet 5.5 --- shape-doctor.py | 15 ++- tests/test_update_shape_merged_away.py | 44 +++++++ tests/test_update_shape_pin_source.py | 153 +++++++++++++++++++++++-- update-shape.py | 65 ++++++++--- 4 files changed, 248 insertions(+), 29 deletions(-) diff --git a/shape-doctor.py b/shape-doctor.py index a193456e..5a9aadcb 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -3341,8 +3341,9 @@ def refusals_ahead(ctx: Context, us) -> dict | None: NONE WHERE THE QUESTION CANNOT BE ASKED: the pin does not read, the checkout of the standard is a partial clone short of what the comparison reads, where `prepare` would fetch (`partial_clone_row`, - #683), or `prepare` refuses. The `shape currency` row says which, and - the `pins` row keeps its gate. + #683), or `prepare` refuses, the root's own `source_repository:` among + its reasons (#1203). The `shape currency` row says which of the others, + and the `pins` row keeps its gate. """ try: pin, _rows = us.read_pin(ctx.root) @@ -3353,9 +3354,15 @@ def refusals_ahead(ctx: Context, us) -> dict | None: args = argparse.Namespace(root=str(ctx.root), upstream=str(ctx.shape), at=None) try: - # `ask_source=False`, as `check_shape_currency` asks it (#1203). + # THE PIN'S `source_repository:` IS ASKED HERE, unlike in + # `check_shape_currency` (#1203, the review of PR #1269, S2): the + # `pins` row says `update-shape.py check` reports the copies + # `merged-away` and the `apply` it prints drops their rows, and for + # a root whose field that `check` refuses it exits 2 and does + # neither. A refusal here is "cannot ask", below, and the row keeps + # the gate's finding. root, _pin, rows, _added, upstream, _pinned, _target, kind = \ - us.prepare(args, ask_source=False) + us.prepare(args) except Refusal: return None try: diff --git a/tests/test_update_shape_merged_away.py b/tests/test_update_shape_merged_away.py index 682893c9..a44f7fe1 100644 --- a/tests/test_update_shape_merged_away.py +++ b/tests/test_update_shape_merged_away.py @@ -522,6 +522,50 @@ def test_the_pins_row_keeps_the_gate_where_check_refuses_the_standard( rows_of(doctor(stranger, root, "--json"))["pins"]) +def test_the_pins_row_keeps_the_gate_where_check_refuses_the_pins_source( + standard, root): + """Run over two collision copies merged away AND a pin whose + `source_repository:` is a hand edit (#1203, the review of PR #1269, S2). + `update-shape.py check` asks that field before it classifies a row and + refuses `update-upstream-unresolvable`, exit 2, in the pin's name: it + reports no copy `merged-away`, and the `apply` that would drop their + rows never starts. So `refusals_ahead` cannot ask its question here, as + where `check` refuses the standard, and the `pins` row keeps its gate. + With the field left unasked there (`ask_source=False`) the row said that + `check` reports them `merged-away` and that the `apply` it prints drops + their pin rows, and its NEXT was that `check`. + + The `shape currency` row does not ask the field, and answers as it did: + it counts the two rows `merged-away`, exit 1 for the verdict, with no + CANNOT ANSWER. The scaffolded value, with the same two copies, is + `test_a_merged_away_copys_row_goes_and_the_project_is_green_again`: the + `pins` row names `check` and `merged_away` there.""" + merge_away(root, *MERGED.values()) + pin = root / SHAPE_PIN + text = pin.read_text(encoding="utf-8") + edited, edits = re.subn(r"(?m)^source_repository: .*$", + 'source_repository: "has space/x"', text) + assert edits == 1, text + pin.write_bytes(edited.encode("utf-8")) + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", + "The pin's source, as a hand edit wrote it", "--", SHAPE_PIN, + cwd=root) + checked = update_shape(standard, root, "check") + assert checked.returncode == 2, checked.stdout + checked.stderr + assert checked.stderr.startswith( + "REFUSED update-upstream-unresolvable: "), checked.stderr + assert "merged-away" not in checked.stdout, checked.stdout + + result = doctor(standard, root, "--json") + rows = rows_of(result) + assert_the_pins_row_is_the_gate(rows["pins"]) + currency = rows["shape-currency"] + assert currency["status"] == "FINDING", currency + assert "environment" not in currency["detail"], currency + assert currency["detail"]["counts"]["merged-away"] == 2, currency + assert result.returncode == 1, result.stdout + result.stderr + + #: The pinned copy the `both` and `upstream-removed` cases move upstream, #: and the file the `unmapped` case pins that no upstream path maps to. MOVED_COPY = "AGENTS-shape.md" diff --git a/tests/test_update_shape_pin_source.py b/tests/test_update_shape_pin_source.py index 2f097e3a..8f4bde38 100644 --- a/tests/test_update_shape_pin_source.py +++ b/tests/test_update_shape_pin_source.py @@ -32,11 +32,19 @@ spelling that is a repository where the command runs is taken, as it was, and with no `--upstream` it is opened in place. -THE DOCTOR ANSWERS AS IT DID. `shape-doctor.py` calls `prepare` with its -own checkout as the upstream and reads a refusal out of it as that checkout -unable to answer; asked there, a hand-edited field was CANNOT ANSWER with a -`git fetch --all` for its remedy. It passes `ask_source=False`, and the -`update-shape.py check` its row names where the root is behind asks it. +THE DOCTOR'S `shape currency` ROW ANSWERS AS IT DID. `shape-doctor.py` +calls `prepare` with its own checkout as the upstream and reads a refusal +out of it as that checkout unable to answer; asked there, a hand-edited +field was CANNOT ANSWER with a `git fetch --all` for its remedy. That row +passes `ask_source=False`, and the `update-shape.py check` its row names +where the root is behind asks it. Its `pins` row (`refusals_ahead`) asks the +field: it names that `check` in a sentence that is false where `check` +refuses (`tests/test_update_shape_merged_away.py`). + +THE REVIEW OF PR #1269 (rv1269) added: `~` for a user nobody has is +nothing there, not a traceback (S1); so is a name over 255 bytes, which +`stat` refuses `ENAMETOOLONG` (N1); the `pins` row asks the field (S2); the +remedy for a value that names a folder here says where to run (N2). No network and no GitHub: every `--upstream` is a local clone, the A -> B upstream of `test_update_shape.py`, and the one run with no `--upstream` @@ -51,6 +59,7 @@ import pytest from conftest import REPO, git, run_script +from test_bump_leg_unopenable import refused_stat from test_shape_doctor import doctor, rows_of from test_update_shape import ( # noqa: F401 CHANGED, UPDATE, assert_refused_and_unpinned, root, update_shape, @@ -62,6 +71,13 @@ #: The words of the two refusals that ask what the value is. NOT_A_REPO = "which exists but is not a git repository" NOT_A_NAME = "which is not an `owner/repo` name" +#: The sentence a value that names something here is told besides: the +#: value is looked up from the directory the command runs in, so a right one +#: needs the command run elsewhere (N2 of the review of PR #1269). +RUN_ELSEWHERE = ( + " The value is also looked up as a path from the directory this runs " + "in: if it is already right, run this from a directory where nothing is " + "at that path.") def written_as(root: Path, value: str) -> None: @@ -90,9 +106,11 @@ def upstream_of(upstream_and_project: dict) -> tuple[str, str]: def refused(root: Path, code: str, read: str, said: str) -> list[str]: - """The two lines a refused pin's value is told, whole.""" + """The two lines a refused pin's value is told, whole: a value that + exists here and holds no repository is told where to run it besides.""" + remedy = WRITE_THE_PIN + (RUN_ELSEWHERE if said == NOT_A_REPO else "") return [f"REFUSED {code}: {pin_of(root)}: `source_repository:` is " - f"{read}, {said}", WRITE_THE_PIN] + f"{read}, {said}", remedy] ROUTES = [ @@ -197,6 +215,118 @@ def test_the_pins_source_is_named_before_a_bad_upstream(root): NOT_A_NAME), result.stderr +#: A home folder that no user has: `Path.expanduser` raises on it (#1203, +#: review of PR #1269, S1). +NO_SUCH_HOME = "~no-such-user-1203/shape" +#: A first part over the 255 bytes a name may have, which `stat` refuses as +#: `ENAMETOOLONG` and no pin names (N1). +LONG = "r" * 300 + + +@pytest.mark.parametrize("passed", ROUTES) +def test_a_tilde_user_who_is_not_there_is_refused_in_the_pins_name( + root, upstream_and_project, passed): + """`~/...` for a user the machine does not have: `expanduser` + raises `RuntimeError`, which was a traceback, exit 1, the code `check` + uses for "behind" (the review of PR #1269, S1). With no `--upstream` + main did the same; with one passed, main never asked the value and + exited 1 with a report. It is nothing there now: the bare path is + looked up, nothing is at it, and the name question refuses it, exit 2, + in the pin's name.""" + written_as(root, NO_SUCH_HOME) + result = run(root, "check", + *(upstream_of(upstream_and_project) if passed else ())) + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.splitlines() == refused( + root, "update-upstream-unresolvable", f"'{NO_SUCH_HOME}'", + NOT_A_NAME), result.stderr + assert result.stdout == "", result.stdout + + +@pytest.mark.parametrize("passed", ROUTES) +def test_a_name_too_long_for_a_path_is_nothing_there_and_is_named_as_the_pins( + root, upstream_and_project, passed): + """A value whose first part is over 255 bytes, and no `owner/repo` + name because of a space: `stat` refuses it `ENAMETOOLONG`, which was + `update-unreadable ... cannot be read (File name too long)` with a + permissions remedy, naming neither the pin nor the field (N1). That + errno says nothing is there to open, as `ENOENT` does, so the name + question is asked, and it refuses in the pin's name.""" + written_as(root, f"{LONG}/has space") + result = run(root, "check", + *(upstream_of(upstream_and_project) if passed else ())) + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.splitlines() == refused( + root, "update-upstream-unresolvable", f"'{LONG}/has space'", + NOT_A_NAME), result.stderr + + +@pytest.mark.parametrize("passed", ROUTES) +def test_a_name_too_long_for_a_path_that_is_an_owner_repo_is_the_pins_unsafe_value( + root, upstream_and_project, passed): + """The same first part with a second that makes it an `owner/repo` + name the pattern takes whole: nothing is there, so `checked_value` + answers, and refuses a value of over 255 characters as `unsafe-value` + in the pin's name. On main, with `--upstream` passed, nobody asked it + and `check` exited 1 with a report.""" + written_as(root, f"{LONG}/x") + result = run(root, "check", + *(upstream_of(upstream_and_project) if passed else ())) + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.splitlines()[0] == ( + f"REFUSED unsafe-value: {pin_of(root)}: `source_repository:` is " + f"'{LONG}/x', which is not a value this tool will put on a `git` " + "or `gh` command line"), result.stderr + + +def test_any_other_refused_stat_of_the_pins_value_is_raised_as_it_was( + update_shape, monkeypatch): + """ONLY `ENAMETOOLONG` IS "NOTHING THERE". A `stat` this user is refused + (`Permission denied`) is raised out of `pin_source`, for `main` to name + `update-unreadable` as it names the other reads the system refused.""" + refused_stat(monkeypatch, "shape") + with pytest.raises(PermissionError): + update_shape.pin_source({"source_repository": "opensoft/shape"}, + Path("/r/contracts/shape-pin.yaml")) + + +@pytest.mark.parametrize("passed", ROUTES) +def test_the_scaffolded_value_beside_a_folder_of_its_name_says_where_to_run( + root, upstream_and_project, tmp_path, passed): + """N2 of the review of PR #1269: the scaffolded pin, run from a folder + that holds a folder (no repository) at `opensoft/openRepoShape`. The + value is looked up as a path from where the command runs, so it is + `update-upstream-not-a-repo` in the pin's name (on main, with + `--upstream` passed, nobody asked it). Its remedy was to write + `opensoft/openRepoShape`, which the pin already says; it ends by saying + to run it where nothing is at that path.""" + here = tmp_path / "here" + (here / "opensoft" / "openRepoShape").mkdir(parents=True) + result = run(root, "check", + *(upstream_of(upstream_and_project) if passed else ()), + cwd=here) + assert result.returncode == 2, result.stdout + result.stderr + lines = refused(root, "update-upstream-not-a-repo", + "'opensoft/openRepoShape'", NOT_A_REPO) + assert result.stderr.splitlines() == lines, result.stderr + assert result.stderr.splitlines()[1].endswith( + "run this from a directory where nothing is at that path."), \ + result.stderr + + +def test_the_scaffolded_value_run_where_nothing_is_at_its_path_is_asked_no_more( + root, upstream_and_project, tmp_path): + """The remedy's last word, done: the same pin with `--upstream` passed, + run from a folder where nothing is at `opensoft/openRepoShape`, answers + as the scaffolded pin does, exit 1 over the upstream one commit on.""" + elsewhere = tmp_path / "elsewhere" + elsewhere.mkdir() + result = run(root, "check", *upstream_of(upstream_and_project), + cwd=elsewhere) + assert result.returncode == 1, result.stdout + result.stderr + assert verdicts(result.stdout)[CHANGED] == "upstream-changed" + + def test_apply_over_a_refused_source_writes_nothing(root, upstream_and_project): """THE ISSUE'S `apply`: on main, over the upstream one commit on, `apply @@ -265,12 +395,13 @@ def test_the_doctor_does_not_call_a_hand_edited_source_the_standards(root): own checkout as the upstream and reads a refusal out of it as that checkout unable to answer, so, asked there, a hand edit of the field was CANNOT ANSWER, exit 3, with a `git fetch --all` NEXT that mends - nothing (measured with `ask_source` left True). The doctor passes - `ask_source=False`: its row is no `environment` row, and it does not - exit 3. The doctor here is this checkout's, and so is the - `update-shape.py` it loads.""" + nothing (measured with `ask_source` left True). The `shape currency` + row passes `ask_source=False`: it is no `environment` row, it does not + exit 3, and its status is main's, `ok` (the root is current). The doctor + here is this checkout's, and so is the `update-shape.py` it loads.""" written_as(root, "opensoft/openRepoShape\\n") result = doctor(REPO, root, "--json") row = rows_of(result)["shape-currency"] assert result.returncode != 3, result.stdout + result.stderr assert "environment" not in row["detail"], row + assert row["status"] == "ok", row diff --git a/update-shape.py b/update-shape.py index 1486a008..8b55c243 100755 --- a/update-shape.py +++ b/update-shape.py @@ -206,8 +206,9 @@ from shape_materialize import readable_branch # noqa: E402 #: `owner/repo`, the only remote spelling `--upstream` accepts, and the -#: shape pin's `source_repository:` where no `--upstream` is passed. `\Z`, -#: NEVER `$` (#1139): `$` also matches just before a trailing line feed. +#: shape pin's `source_repository:`, whether or not `--upstream` is passed +#: (`pin_source`, #1203). `\Z`, NEVER `$` (#1139): `$` also matches just +#: before a trailing line feed. REPOSITORY_RE = re.compile(r"^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+\Z") #: How `--branch` may be spelled: the alphabet `checked_value` already @@ -730,6 +731,40 @@ def open_upstream(spec: str) -> Upstream: "It is asked with `--upstream` passed too, because `apply` names it in " "its commit and its pull request; a path to a clone goes to " "`--upstream`.") +#: The same, for a value that names something HERE that is no repository: +#: the value is looked up as a path from the directory the command runs in, +#: so a value that is already right, `owner/repo` with a folder of that name +#: where the command runs, needs the command run elsewhere (N2 of the review +#: of PR #1269). The sentence is added to this refusal's remedy alone. +PIN_SOURCE_NOT_A_REPO_REMEDY = ( + PIN_SOURCE_REMEDY + " The value is also looked up as a path from the " + "directory this runs in: if it is already right, run this from a " + "directory where nothing is at that path.") + + +def _named_here(declared: str) -> Path | None: + """The path `declared` names on this machine, spelled as `open_upstream` + looks it up (`expanduser`), or None where it names nothing there. + + TWO ANSWERS THAT ARE "NOTHING THERE" for a value no one wrote as a path + (the review of PR #1269, S1 and N1): + - `~/...` for a user the machine has not got: `expanduser` + raises `RuntimeError`, and the bare path is looked up instead, which + names nothing; + - a name over the limit a path component may have (`ENAMETOOLONG`): + `stat` cannot open what no file system can hold, as it cannot open + what is not there. + Every other `OSError` of the `stat` is raised, as `_is_a` raises it.""" + try: + candidate = Path(declared).expanduser() + except RuntimeError: + candidate = Path(declared) + try: + return candidate if _is_a(candidate, _anything) else None + except OSError as exc: + if exc.errno == errno.ENAMETOOLONG: + return None + raise def pin_source(pin: dict, pin_path: Path) -> str: @@ -754,7 +789,8 @@ def pin_source(pin: dict, pin_path: Path) -> str: is no git repository: `update-upstream-not-a-repo`. The pin gets the same answer whether or not `--upstream` is passed. An `owner/repo` spelling that IS a repository here is taken, as it was, and with no - `--upstream` it is opened in place. + `--upstream` it is opened in place. A `~` nobody has, and a + name too long for a path, are nothing there (`_named_here`). - a value `REPOSITORY_RE` does not take WHOLE: `update-upstream-unresolvable`. A path that is not spelled `owner/repo` (`/srv/shape`, `~/shape`, `shape`) is refused here, @@ -768,13 +804,12 @@ def pin_source(pin: dict, pin_path: Path) -> str: """ declared = str(pin.get("source_repository") or SHAPE_REPOSITORY) named = f"{pin_path}: `source_repository:`" - candidate = Path(declared).expanduser() - if (_is_a(candidate, _anything) - and not _holds_a_repository(candidate.resolve())): + candidate = _named_here(declared) + if candidate is not None and not _holds_a_repository(candidate.resolve()): raise Refusal( "update-upstream-not-a-repo", f"{named} is {declared!r}, which exists but is not a git " - "repository", PIN_SOURCE_REMEDY) + "repository", PIN_SOURCE_NOT_A_REPO_REMEDY) if not REPOSITORY_RE.fullmatch(declared): raise Refusal( "update-upstream-unresolvable", @@ -2468,13 +2503,15 @@ def prepare(args, *, ask_source: bool = True THE PIN'S `source_repository:` IS ASKED BEFORE THE UPSTREAM IS OPENED (#1203), by `pin_source`, with or without `--upstream`. `ask_source` - False is `shape-doctor.py`'s alone: it passes its own checkout as the - upstream, writes no commit and opens no pull request, and it reads a - `Refusal` from here as that checkout unable to answer, so a refusal of - the root's own field would be called the standard's, with a `git fetch` - for its remedy; `check`, the NEXT its row names where the root is - behind, asks the field. With no `--upstream` the field IS the upstream, - and is asked whatever `ask_source` says.""" + False is the doctor's `shape currency` row's alone: it passes its own + checkout as the upstream, writes no commit and opens no pull request, + and it reads a `Refusal` from here as that checkout unable to answer, so + a refusal of the root's own field would be called the standard's, with a + `git fetch` for its remedy; `check`, the NEXT that row names where the + root is behind, asks the field. The doctor's `pins` row asks it: it + names `check` in a sentence that is false where `check` refuses (S2 of + the review of PR #1269). With no `--upstream` the field IS the + upstream, and is asked whatever `ask_source` says.""" root = root_of(args) if not _is_a(root, stat.S_ISDIR): raise Refusal("update-root-missing", f"{root} is not a directory") From c792615f4ddb022225960a4f60ddbf81bb6424ab Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sun, 8 Nov 2026 18:51:48 +0000 Subject: [PATCH 4/5] tests/test_update_shape_pin_source.py: a pin's `~` that `expanduser` cannot expand is looked up as the bare path, so a folder of that name where the command runs is `update-upstream-not-a-repo` in both routes (#1203) S1 of the review of PR #1269 asks the guard to "keep the bare path". The row holds that half: a guard that returned "nothing there" instead gave the same answer for every value but this one. Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Sonnet 5.5 --- tests/test_update_shape_pin_source.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/test_update_shape_pin_source.py b/tests/test_update_shape_pin_source.py index 8f4bde38..fb725007 100644 --- a/tests/test_update_shape_pin_source.py +++ b/tests/test_update_shape_pin_source.py @@ -243,6 +243,24 @@ def test_a_tilde_user_who_is_not_there_is_refused_in_the_pins_name( assert result.stdout == "", result.stdout +@pytest.mark.parametrize("passed", ROUTES) +def test_a_tilde_user_who_is_not_there_is_looked_up_as_the_bare_path( + root, upstream_and_project, tmp_path, passed): + """The bare path is kept, as `expanduser` could not expand it: a folder + of that very name where the command runs, holding no repository, is + `update-upstream-not-a-repo` as any folder the value names is.""" + here = tmp_path / "here" + (here / NO_SUCH_HOME).mkdir(parents=True) + written_as(root, NO_SUCH_HOME) + result = run(root, "check", + *(upstream_of(upstream_and_project) if passed else ()), + cwd=here) + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.splitlines() == refused( + root, "update-upstream-not-a-repo", f"'{NO_SUCH_HOME}'", + NOT_A_REPO), result.stderr + + @pytest.mark.parametrize("passed", ROUTES) def test_a_name_too_long_for_a_path_is_nothing_there_and_is_named_as_the_pins( root, upstream_and_project, passed): From 52e5c18830c3a617367466b5104117b4a4865d0f Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sun, 8 Nov 2026 20:47:17 +0000 Subject: [PATCH 5/5] update-shape.py and shape-doctor.py: a pin's `source_repository:` with a NUL byte after the `~` is nothing there and is refused in the pin's name, and the `pins` row keeps the gate's finding where `prepare` raises an `OSError`, a row for each (#1203) Second review of PR #1269 (rv1269b), NITs A and B: - A: `Path(declared).expanduser()` raises `ValueError: embedded null byte` for a raw NUL byte after the `~`, which `_named_here` did not catch (it caught `RuntimeError` only): a traceback, exit 1, in both routes. It now catches `(RuntimeError, ValueError)` and keeps the bare path, which no file has the name of, so the name question refuses the value `update-upstream-unresolvable` in the pin's name. A row in each route. - B: `refusals_ahead` caught `Refusal` only from `prepare`, since the doctor began to ask the pin's field. A value `locked/x` with an unsearchable `locked` in the doctor's directory raises the `OSError` of `pin_source`'s `stat` out of `prepare`, and the `pins` row read `could not be run: [Errno 13] ...` where it had kept the gate's finding. It now catches `(Refusal, OSError)` and returns None. A row, with real modes. - The pin-edit lines of the existing S2 doctor row move into `source_written_as`, which the new row uses too. Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Sonnet 5.5 --- shape-doctor.py | 16 +++-- tests/test_update_shape_merged_away.py | 82 ++++++++++++++++++++++---- tests/test_update_shape_pin_source.py | 32 +++++++++- update-shape.py | 11 ++-- 4 files changed, 122 insertions(+), 19 deletions(-) diff --git a/shape-doctor.py b/shape-doctor.py index 5a9aadcb..ead78b00 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -3341,9 +3341,9 @@ def refusals_ahead(ctx: Context, us) -> dict | None: NONE WHERE THE QUESTION CANNOT BE ASKED: the pin does not read, the checkout of the standard is a partial clone short of what the comparison reads, where `prepare` would fetch (`partial_clone_row`, - #683), or `prepare` refuses, the root's own `source_repository:` among - its reasons (#1203). The `shape currency` row says which of the others, - and the `pins` row keeps its gate. + #683), or `prepare` refuses or cannot read what it asks, the root's own + `source_repository:` among its reasons (#1203). The `shape currency` row + says which of the others, and the `pins` row keeps its gate. """ try: pin, _rows = us.read_pin(ctx.root) @@ -3361,9 +3361,17 @@ def refusals_ahead(ctx: Context, us) -> dict | None: # a root whose field that `check` refuses it exits 2 and does # neither. A refusal here is "cannot ask", below, and the row keeps # the gate's finding. + # + # AN `OSError` OF THAT ASK IS "CANNOT ASK" TOO (NIT B of the second + # review of PR #1269): `pin_source` stats the value as a path from + # the directory the doctor runs in, and a stat the system refuses + # (`locked/x` with `locked` unsearchable) is raised out of `prepare` + # as the `update-unreadable` that `check` names. Caught as + # `Refusal` alone it left here, and the row read `could not be run: + # [Errno 13] ...` where it had kept the gate's finding. root, _pin, rows, _added, upstream, _pinned, _target, kind = \ us.prepare(args) - except Refusal: + except (Refusal, OSError): return None try: ahead = {row.path: row.state for row in rows if row.is_conflict} diff --git a/tests/test_update_shape_merged_away.py b/tests/test_update_shape_merged_away.py index a44f7fe1..2f442b89 100644 --- a/tests/test_update_shape_merged_away.py +++ b/tests/test_update_shape_merged_away.py @@ -42,7 +42,7 @@ from conftest import REPO, WINDOWS_SKIP, clone_just_pushed, git, \ make_source_repo, run_script from test_shape_doctor import ( # noqa: F401 - commit_all, doctor, keep_as_committed_link, rows_of, standard, + DOCTOR, commit_all, doctor, keep_as_committed_link, rows_of, standard, standard_ahead, standard_of_another_history, verdict_line, ) #: A partial clone of the standard one commit on, from the file that owns @@ -62,6 +62,13 @@ PROJECT = "Medx" SHAPE_PIN = "contracts/shape-pin.yaml" +#: A mode that refuses the owner is a POSIX mode, and root reads through every +#: one (the mark of `tests/test_bump_leg_unopenable.py`). +needs_a_mode = pytest.mark.skipif( + sys.platform == "win32" or (hasattr(os, "geteuid") and os.geteuid() == 0), + reason="a mode that refuses the owner is a POSIX mode, and root reads " + "through every one (tests/test_bump_leg_unopenable.py)") + #: The issue's own source, with a `Makefile` beside its `.gitignore`: two #: names the shape copies too, so the adoption writes both of its copies #: under `shape/` and pins them there. `README.md` collides as well, and is @@ -506,6 +513,20 @@ def assert_the_pins_row_is_the_gate(pins_row: dict) -> None: assert "update-shape.py" not in pins_row["next"], pins_row["next"] +def source_written_as(root: Path, value: str) -> None: + """The pin's `source_repository:` written double-quoted as `value`, as + a hand edit makes it, and committed (#1203).""" + pin = root / SHAPE_PIN + text = pin.read_text(encoding="utf-8") + edited, edits = re.subn(r"(?m)^source_repository: .*$", + f'source_repository: "{value}"', text) + assert edits == 1, text + pin.write_bytes(edited.encode("utf-8")) + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", + "The pin's source, as a hand edit wrote it", "--", SHAPE_PIN, + cwd=root) + + def test_the_pins_row_keeps_the_gate_where_check_refuses_the_standard( standard, root, tmp_path): """Run from a standard of another history, which does not carry the @@ -541,15 +562,7 @@ def test_the_pins_row_keeps_the_gate_where_check_refuses_the_pins_source( `test_a_merged_away_copys_row_goes_and_the_project_is_green_again`: the `pins` row names `check` and `merged_away` there.""" merge_away(root, *MERGED.values()) - pin = root / SHAPE_PIN - text = pin.read_text(encoding="utf-8") - edited, edits = re.subn(r"(?m)^source_repository: .*$", - 'source_repository: "has space/x"', text) - assert edits == 1, text - pin.write_bytes(edited.encode("utf-8")) - git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", - "The pin's source, as a hand edit wrote it", "--", SHAPE_PIN, - cwd=root) + source_written_as(root, "has space/x") checked = update_shape(standard, root, "check") assert checked.returncode == 2, checked.stdout + checked.stderr assert checked.stderr.startswith( @@ -566,6 +579,55 @@ def test_the_pins_row_keeps_the_gate_where_check_refuses_the_pins_source( assert result.returncode == 1, result.stdout + result.stderr +@needs_a_mode +def test_the_pins_row_keeps_the_gate_where_the_pins_source_cannot_be_read( + standard, root, tmp_path): + """Run over two collision copies merged away AND a pin whose + `source_repository:` is `locked/x`, a folder of the directory the doctor + runs in that this user may not search (NIT B of the second review of PR + #1269). `pin_source` stats the value as a path, the stat is refused + `Permission denied`, and the `OSError` is raised out of `prepare`: + `update-shape.py check` names it `update-unreadable`, exit 2, and + reports no copy `merged-away`. `refusals_ahead` caught `Refusal` only, + so the error left it and the `pins` row read `could not be run: [Errno + 13] ...`, where it kept the gate's finding before the field was asked. + It cannot ask its question here, as where `check` refuses the field in + the pin's name, and the `pins` row keeps its gate. + + The `shape currency` row does not ask the field, and answers as it did: + it counts the two rows `merged-away`, with no CANNOT ANSWER. The mode is + given back in a `finally`.""" + merge_away(root, *MERGED.values()) + source_written_as(root, "locked/x") + here = tmp_path / "here" + locked = here / "locked" + locked.mkdir(parents=True) + locked.chmod(0) + try: + checked = run_script(standard / "update-shape.py", "check", "--root", + str(root), "--upstream", str(standard), + cwd=here) + result = run_script(standard / DOCTOR, "--root", str(root), "--json", + cwd=here) + finally: + locked.chmod(0o755) + assert checked.returncode == 2, checked.stdout + checked.stderr + assert checked.stderr.startswith( + "REFUSED update-unreadable: locked/x: cannot be read "), \ + checked.stderr + assert "merged-away" not in checked.stdout, checked.stdout + + rows = rows_of(result) + assert "could not be run" not in rows["pins"]["reason"], rows["pins"] + assert "shape-copy-missing" in rows["pins"]["reason"], rows["pins"] + assert_the_pins_row_is_the_gate(rows["pins"]) + currency = rows["shape-currency"] + assert currency["status"] == "FINDING", currency + assert "environment" not in currency["detail"], currency + assert currency["detail"]["counts"]["merged-away"] == 2, currency + assert result.returncode == 1, result.stdout + result.stderr + + #: The pinned copy the `both` and `upstream-removed` cases move upstream, #: and the file the `unmapped` case pins that no upstream path maps to. MOVED_COPY = "AGENTS-shape.md" diff --git a/tests/test_update_shape_pin_source.py b/tests/test_update_shape_pin_source.py index fb725007..3b29edb9 100644 --- a/tests/test_update_shape_pin_source.py +++ b/tests/test_update_shape_pin_source.py @@ -44,7 +44,9 @@ THE REVIEW OF PR #1269 (rv1269) added: `~` for a user nobody has is nothing there, not a traceback (S1); so is a name over 255 bytes, which `stat` refuses `ENAMETOOLONG` (N1); the `pins` row asks the field (S2); the -remedy for a value that names a folder here says where to run (N2). +remedy for a value that names a folder here says where to run (N2). The +second review (rv1269b) added: a raw NUL byte after the `~` is nothing there +too, as `ValueError` is caught beside `RuntimeError` (NIT A). No network and no GitHub: every `--upstream` is a local clone, the A -> B upstream of `test_update_shape.py`, and the one run with no `--upstream` @@ -261,6 +263,34 @@ def test_a_tilde_user_who_is_not_there_is_looked_up_as_the_bare_path( NOT_A_REPO), result.stderr +#: A raw NUL byte after a `~`, as a pin hand-edited in a binary editor holds +#: one: `Path.expanduser` raises `ValueError` (`embedded null byte`) on it, +#: not the `RuntimeError` it raises for a user nobody has (NIT A of the +#: second review of PR #1269). Built with `chr`, so no escape is in the file. +NUL_AFTER_TILDE = "~a" + chr(0) + "b/x" + + +@pytest.mark.parametrize("passed", ROUTES) +def test_a_nul_byte_after_a_tilde_is_refused_in_the_pins_name( + root, upstream_and_project, passed): + """A pin whose value has a raw NUL byte after the `~`: `expanduser` + raised `ValueError: embedded null byte`, which `_named_here` did not + catch, so `check` ended in a traceback, exit 1, the code it uses for + "behind" (NIT A of the second review of PR #1269). With no `--upstream` + main did the same; with one passed, main never asked the value and + exited 1 with a report. It is nothing there as `~` nobody has is: + the bare path is looked up, nothing is at it, and the name question + refuses it, exit 2, in the pin's name.""" + written_as(root, NUL_AFTER_TILDE) + result = run(root, "check", + *(upstream_of(upstream_and_project) if passed else ())) + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.splitlines() == refused( + root, "update-upstream-unresolvable", repr(NUL_AFTER_TILDE), + NOT_A_NAME), result.stderr + assert result.stdout == "", result.stdout + + @pytest.mark.parametrize("passed", ROUTES) def test_a_name_too_long_for_a_path_is_nothing_there_and_is_named_as_the_pins( root, upstream_and_project, passed): diff --git a/update-shape.py b/update-shape.py index 8b55c243..328e856f 100755 --- a/update-shape.py +++ b/update-shape.py @@ -750,14 +750,16 @@ def _named_here(declared: str) -> Path | None: (the review of PR #1269, S1 and N1): - `~/...` for a user the machine has not got: `expanduser` raises `RuntimeError`, and the bare path is looked up instead, which - names nothing; + names nothing. So does a NUL byte after the `~`: `expanduser` raises + `ValueError` (`embedded null byte`), and the bare path, which no file + has the name of, names nothing either (NIT A of the second review); - a name over the limit a path component may have (`ENAMETOOLONG`): `stat` cannot open what no file system can hold, as it cannot open what is not there. Every other `OSError` of the `stat` is raised, as `_is_a` raises it.""" try: candidate = Path(declared).expanduser() - except RuntimeError: + except (RuntimeError, ValueError): candidate = Path(declared) try: return candidate if _is_a(candidate, _anything) else None @@ -789,8 +791,9 @@ def pin_source(pin: dict, pin_path: Path) -> str: is no git repository: `update-upstream-not-a-repo`. The pin gets the same answer whether or not `--upstream` is passed. An `owner/repo` spelling that IS a repository here is taken, as it was, and with no - `--upstream` it is opened in place. A `~` nobody has, and a - name too long for a path, are nothing there (`_named_here`). + `--upstream` it is opened in place. A `~` nobody has (a NUL + byte after the `~` too), and a name too long for a path, are nothing + there (`_named_here`). - a value `REPOSITORY_RE` does not take WHOLE: `update-upstream-unresolvable`. A path that is not spelled `owner/repo` (`/srv/shape`, `~/shape`, `shape`) is refused here,