From d3829f5d7138bb660b5cec7c8e2c8e7ba1edefa0 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Mon, 12 Oct 2026 22:26:57 +0000 Subject: [PATCH 1/2] shape-doctor.py and validate-pins.py: a gitlink that is not its pin is offered both repairs, `bump-leg.py --to` the gitlink's commit to keep the move and `--to` the pin's to undo it, so a gitlink committed by hand no longer ends at a gate that prints THE LOCKSTEP RULE and no command (#478) `git -C spec pull`, then `git add spec && git commit` in the root, made the doctor say DRIFTED with a `pins` FINDING `pin-gitlink-mismatch` whose NEXT is the project's `validate-pins.py`, and that gate printed the finding and THE LOCKSTEP RULE and named no command, though AGENTS.md says a finding names its fix and lists `bump-leg.py` among them. - The `pins` row: where the gate reported findings (exit 1, no crash) and a declared leg's recorded gitlink (the index first, as the gate and `bump-leg.py` read it) and its pin are two 40-hex commits, the reason goes on to name two lines and choose neither: `bump-leg.py --root --leg --to ` keeps the gitlink's commit, and `--to ` keeps the pin's. Each is `quote_arg`'s spelling for the reader's shell (`bump_leg_command`, `platform` passed on as `copy_command` passes it), runs from any directory, and carries the lane's `--trailer` where `LANES_LANE` names one, as the `shape currency` row's `apply` does. `detail.gitlink_repairs` lists them per leg. - The NEXT is still the gate, byte for byte, and the row is changed in place, so a field another change adds to `Row` is kept. The choice is in the reason as the `merge` row (#424) keeps its, because a NEXT is run as printed and which commit stays is the person's call. - The reason says what the line needs: a branch other than the tracking branch, and the commit on a branch of the leg's remote. The doctor cannot know the second offline (a remote-tracking ref is what the remote held at the last fetch), and it still fetches nothing. - `validate-pins.py` (the assembly-root template, a pinned copy): the `pin-gitlink-mismatch` finding ends with the same two lines, from a checkout of openRepoShape, the root `shlex.quote`d and `PYTHON` the interpreter, as `validate-family.py` names `family.py`. The undo is `bump-leg.py --to ` and not the issue's `git submodule update --init --checkout -- spec` then a commit: that checks the leg out at the gitlink, the commit being undone, so it changes nothing (measured, and held by a test). Unchanged, diffed byte for byte against main's doctor and template over the same roots: the `pins` row and the gate's output for `pin-digest-mismatch`, `pin-path-mismatch`, `pin-workflow-ref-mismatch`, `shape-copy-drift`, and a refusal (`pin-submodule-uninitialized`) over a gitlink that is off its pin. The `legs` row is untouched; its NEXT, the gate, now names the repair it said the gate names. Moving `templates/assembly-root/scripts/validate-pins.py` moves the digest every project pins for it: `update-shape.py check` reports it `upstream-changed` until `apply` re-copies it. Left out: `bump-leg.py`'s `old -> new` line reads the old commit from the gitlink, so keeping a hand-moved gitlink prints `X -> X` while the pin moves; a gitlink staged and not committed gets the same two lines, which `bump-leg.py` refuses until the root is clean, naming commit or stash. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Opus 5.5 --- shape-doctor.py | 109 ++++- .../assembly-root/scripts/validate-pins.py | 33 +- tests/test_doctor_gitlink_repairs.py | 373 ++++++++++++++++++ 3 files changed, 512 insertions(+), 3 deletions(-) create mode 100644 tests/test_doctor_gitlink_repairs.py diff --git a/shape-doctor.py b/shape-doctor.py index f81db0ef..fc89f9c5 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -1649,8 +1649,113 @@ def check_pins(ctx: Context) -> Row: "a single pin, has nothing to check yet") if waiting is not None: return waiting - return validator_row(ctx, "pins", "pins", *PINS_GATE, - ["--root", str(ctx.root)]) + return with_gitlink_repairs(ctx, validator_row( + ctx, "pins", "pins", *PINS_GATE, ["--root", str(ctx.root)])) + + +def bump_leg_command(shape: Path, root: Path, role: str, commit: str, + platform: str | None = None) -> str: + """The `scripts/bump-leg.py` line that puts a leg's gitlink, its pin and + every workflow reference at `commit` in one commit, spelled for the + READER'S shell (#478). + + From any directory: the standard's own copy of the script by its + absolute path, `--root` the root's, and every value through `quote_arg` + for the platform asked for, as `copy_command` and `validator_command` + spell theirs. It carries the lane's `--trailer` where `LANES_LANE` names + one, as the `shape currency` row's `apply` does, because it writes a + commit too (#111). + """ + target = os.name if platform is None else platform + return " ".join([ + python_command(platform), + quote_arg(shape / "scripts" / "bump-leg.py", target), + "--root", quote_arg(root, target), "--leg", quote_arg(role, target), + "--to", quote_arg(commit, target)]) + lane_trailer_command(platform) + + +def gitlinks_off_their_pins(ctx: Context) -> list[dict]: + """Each declared leg whose gitlink and pin name two different commits, + as `{"role", "path", "gitlink", "pin"}`, in the manifest's order (#478). + + The `legs` row's two reads, `recorded_gitlink` (the index first, as the + gate reads it and as `bump-leg.py` reads the commit it replaces) and + `pin_commit`. A leg where either is not a 40-hex commit is left out: + `bump-leg.py --to` takes nothing else, and `validate-pins.py` refuses + such a pin before it compares it. + """ + off = [] + for leg in ctx.legs(): + role = str(leg.get("role") or "") + path = str(leg.get("path") or "") + if not role or not path: + continue + pinned = pin_commit(ctx.root, f"contracts/{role}-pin.yaml") + gitlink = recorded_gitlink(ctx.root, path) + if pinned and gitlink and COMMIT_RE.match(gitlink) \ + and gitlink != pinned: + off.append({"role": role, "path": path, "gitlink": gitlink, + "pin": pinned}) + return off + + +def with_gitlink_repairs(ctx: Context, row: Row) -> Row: + """The `pins` row, naming both repairs for a gitlink that is not its pin + (#478). + + THE DEFECT. A leg moved by hand and committed (`git -C spec pull`, then + `git add spec && git commit` in the root) made this row a FINDING whose + NEXT is the gate, and the gate printed THE LOCKSTEP RULE and no command, + so a person told that "a finding names its fix" was named none. + + WHICH COMMIT STAYS IS THE PERSON'S CALL, so the reason names two + `bump-leg.py` lines and chooses neither: `--to` the gitlink's commit + keeps the move, and `--to` the pin's undoes it. `bump-leg.py` either + way, because it moves the gitlink, the pin and every workflow reference + in one commit and runs the gate before it commits. `git submodule + update` is no undo: it checks the leg out at the gitlink, which is the + commit being undone. The two lines are in the reason and in + `detail.gitlink_repairs`, never in the NEXT, as the `merge` row offers + its choice (#424): a NEXT is run as printed, and this one is the gate, + which now names the same two. + + THE COMMIT MUST BE ON A BRANCH OF THE LEG'S REMOTE, and offline that + cannot be known: `bump-leg.py` fetches before it asks, and a + remote-tracking ref here says what the remote held at the last fetch, + not what it holds. So the reason says what the command needs, and the + doctor still fetches nothing. + + ONLY A GATE THAT REPORTED FINDINGS, exit 1, and did not crash. A refusal + stops `validate-pins.py` before it reports any finding, and that is the + thing to fix first; any other row is returned untouched. The row is + changed IN PLACE, reason and detail, so whatever else a `Row` carries is + kept. + """ + if row.status != FINDING or row.detail.get("exit") != 1 \ + or row.detail.get("crashed"): + return row + off = gitlinks_off_their_pins(ctx) + if not off: + return row + for leg in off: + leg["keep_gitlink"] = bump_leg_command(ctx.shape, ctx.root, + leg["role"], leg["gitlink"]) + leg["keep_pin"] = bump_leg_command(ctx.shape, ctx.root, leg["role"], + leg["pin"]) + tracking = str((ctx.manifest or {}).get("tracking_branch") or "main") + each = "; ".join( + f"for {leg['path']}, `{leg['keep_gitlink']}` keeps the gitlink's " + f"{leg['gitlink'][:12]} and `{leg['keep_pin']}` keeps the pin's " + f"{leg['pin'][:12]}" for leg in off) + row.reason = ascii_text( + f"{row.reason}. The gitlink and the pin name two commits, and which " + "stays is the person's call; bump-leg.py puts the gitlink, the pin " + f"and every workflow reference at it in one commit: {each}. Either " + f"commits on a branch other than {tracking} and refuses a commit no " + "branch of the leg's remote contains, so a commit made in the leg " + "is pushed first") + row.detail["gitlink_repairs"] = off + return row def check_family(ctx: Context) -> Row: diff --git a/templates/assembly-root/scripts/validate-pins.py b/templates/assembly-root/scripts/validate-pins.py index 18524467..61b9c99b 100755 --- a/templates/assembly-root/scripts/validate-pins.py +++ b/templates/assembly-root/scripts/validate-pins.py @@ -57,6 +57,7 @@ import argparse import os import re +import shlex import stat import sys from collections.abc import Iterator @@ -64,6 +65,8 @@ from pathlib import Path sys.path.insert(0, str(Path(__file__).resolve().parent)) +#: The interpreter the `pin-gitlink-mismatch` repair lines name (#478). +from repo_shape import PYTHON # noqa: E402 from repo_shape import ( # noqa: E402 COMMIT_RE, SHA256_RE, TREE_DIGEST_DEFINITION, Refusal, file_sha256, find_repo_root, git_out, load_yaml, pin_source_env_name, recorded_gitlink, @@ -237,6 +240,33 @@ def _git_runs(unread: str) -> Iterator[None]: "cannot answer is never a pass.") from exc +def _gitlink_repairs(root: Path, role, gitlink: str, commit: str) -> str: + """The two commands a `pin-gitlink-mismatch` ends on (#478). + + WHICH COMMIT STAYS IS THE PERSON'S CALL, so both are named and neither + is chosen. A gitlink moved and committed by hand (`git -C spec pull`, + then `git add spec` in the root) wants the first, `--to` the gitlink's + commit, if the move was meant and the second, `--to` the pin's, if it + was not; a pin edited by hand reads the same with the two the other way + round. Either is `bump-leg.py`, which puts all three facts at one commit + in ONE commit and runs this validator before it commits: a gitlink put + back by hand is the same single-fact move this check refuses. + + FROM A CHECKOUT OF openRepoShape, as `validate-family.py` names + `family.py`: no project carries `bump-leg.py`, and this file cannot know + where the reader's checkout is, so the script is relative to one and the + root, which it does know, is absolute and quoted. + """ + bump = (f"{PYTHON} scripts/bump-leg.py --root {shlex.quote(str(root))} " + f"--leg {shlex.quote(str(role))} --to") + return ("Which commit stays is the person's call. From a checkout of " + "openRepoShape, the first command keeps the gitlink's commit and " + "the second the pin's. Either puts all three at it in one commit, " + "on a branch other than the tracking branch, and refuses a commit " + "no branch of the leg's remote contains, so push one made in the " + f"leg first:\n {bump} {gitlink}\n {bump} {commit}") + + def _check_gitlink_matches_pin(root: Path, role, path: str, commit: str, rel: str, report: Report) -> str: """Fact 1 vs fact 2: the recorded gitlink against the pin file's @@ -256,7 +286,8 @@ def _check_gitlink_matches_pin(root: Path, role, path: str, commit: str, if gitlink != commit: report.finding( "pin-gitlink-mismatch", - f"{path}: gitlink {gitlink} != {rel} commit {commit}\n " + LOCKSTEP, + f"{path}: gitlink {gitlink} != {rel} commit {commit}\n " + LOCKSTEP + + "\n " + _gitlink_repairs(root, role, gitlink, commit), ) else: report.note(f"{path}: gitlink == {rel} commit {commit[:12]}") diff --git a/tests/test_doctor_gitlink_repairs.py b/tests/test_doctor_gitlink_repairs.py new file mode 100644 index 00000000..07c6508e --- /dev/null +++ b/tests/test_doctor_gitlink_repairs.py @@ -0,0 +1,373 @@ +# SPDX-License-Identifier: Apache-2.0 +"""A gitlink committed by hand: the `pins` row and `validate-pins.py` name +both repairs (#478). + +THE DEFECT. `git -C spec pull`, then `git add spec && git commit` in the +root, is the most ordinary way a leg's gitlink leaves its pin. The doctor +said DRIFTED, its `pins` row was `FINDING pin-gitlink-mismatch` with the gate +as its NEXT, and the gate printed THE LOCKSTEP RULE and no command, though +AGENTS.md says a finding names its fix and lists `bump-leg.py` among them. + +WHAT THEY SAY NOW. Two `bump-leg.py` lines, and which one to run is the +person's call: `--to` the gitlink's commit keeps the move, `--to` the pin's +undoes it. The doctor names them in the row's reason and in +`detail.gitlink_repairs`, spelled by `quote_arg` for the reader's shell; the +gate names the same two, from a checkout of openRepoShape. The row's NEXT is +still the gate. + +EVERY REPAIR HERE IS RUN, from a directory that is not the root and through +the argv the reader's own shell makes of the printed line (`pasted`: `shlex` +on POSIX, PowerShell's reading on Windows), so these run on all three +systems. The root sits under a directory whose name holds a blank and an +apostrophe, so a line that is not quoted for its shell fails here. + +NO NETWORK. The leg's new commit is pushed to a branch of its bare remote on +disk, which is what `bump-leg.py` asks of a commit it pins. The fixtures are +`tests/test_shape_doctor.py`'s, imported, so this module has its own scaffold +and its own remotes, and the push moves no other module's. +""" + +from __future__ import annotations + +import json +import os +import re +import shlex +import shutil +import subprocess +import sys +from pathlib import Path, PurePosixPath, PureWindowsPath + +import pytest + +from conftest import FILE_PROTOCOL, REPO, git, run_script +from test_commit_trailers import LANE +from test_shape_doctor import ( # noqa: F401 + DOCTOR, DRIFT_TARGET, GIT_IDENTITY, PROJECT, doctor, doctor_module, + pasted, project, rows_of, scaffolded_here, standard, +) + +sys.path.insert(0, str(REPO / "scripts")) +from repo_shape import PYTHON # noqa: E402 + +#: The branch of the spec leg's remote the new commit is pushed to. +ADVANCE = "doctor-gitlink-advance" + +#: The branch the root is on when the gitlink is committed by hand: +#: `bump-leg.py` refuses the tracking branch, and names this step itself. +WORKING = "take-the-new-spec" + +#: A commit no repository here holds, for a workflow reference that is wrong. +OTHER_SHA = "0123456789abcdef0123456789abcdef01234567" + + +@pytest.fixture(scope="module") +def advanced(scaffolded_here, tmp_path_factory) -> str: + """A new commit on a branch of the spec leg's remote, as a merged pull + request leaves one; its sha.""" + work = tmp_path_factory.mktemp("spec-advance") / f"{PROJECT}-spec" + remote = scaffolded_here["base"] / "remotes" / f"{PROJECT}-spec.git" + proc = subprocess.run(["git", "clone", "-q", str(remote), str(work)], + capture_output=True, text=True, check=False) + assert proc.returncode == 0, proc.stderr + (work / "requirements").mkdir(exist_ok=True) + (work / "requirements" / "r478.md").write_text("# R478\n", + encoding="utf-8") + git("add", "--", "requirements/r478.md", cwd=work) + git("commit", "-qm", "Add R478", cwd=work) + git("push", "-q", "origin", f"HEAD:refs/heads/{ADVANCE}", cwd=work) + return git("rev-parse", "HEAD", cwd=work).stdout.strip() + + +def pin_of(root: Path, role: str = "spec") -> str: + text = (root / "contracts" / f"{role}-pin.yaml").read_text( + encoding="utf-8") + found = re.search(r'(?m)^commit: "([0-9a-f]{40})"$', text) + assert found, text + return found.group(1) + + +def under_an_awkward_name(project: Path, tmp_path: Path) -> Path: + """The root moved under a directory whose name needs quoting in every + shell: a blank, and an apostrophe.""" + root = tmp_path / "it's my work" / PROJECT + root.parent.mkdir() + shutil.move(str(project), str(root)) + return root.resolve() + + +@pytest.fixture +def hand_moved(project, advanced, tmp_path) -> dict: + """The issue's reproduction: the leg fetched and checked out at the new + commit, and the gitlink committed by hand in the root, on a branch.""" + root = under_an_awkward_name(project, tmp_path) + pinned = pin_of(root) + leg = root / "spec" + git(*FILE_PROTOCOL, "fetch", "-q", "origin", ADVANCE, cwd=leg) + git("checkout", "-q", "--detach", advanced, cwd=leg) + git("checkout", "-q", "-b", WORKING, cwd=root) + git("add", "--", "spec", cwd=root) + git("commit", "-qm", "take the new spec", cwd=root) + return {"root": root, "gitlink": advanced, "pin": pinned, + "base": git("rev-parse", "HEAD", cwd=root).stdout.strip()} + + +def pins_row(standard: Path, root: Path, env: dict | None = None) -> dict: + result = run_script(standard / DOCTOR, "--root", str(root), "--json", + env=env) + assert result.returncode == 1, result.stdout + result.stderr + return rows_of(result)["pins"] + + +def gate_row(standard: Path, root: Path) -> dict: + """The row the gate alone makes of `root`, before #478's reason: the + doctor's own `validator_row`, asked in this process.""" + module = doctor_module(standard) + ctx = module.Context(root.resolve()) + return module.validator_row(ctx, "pins", "pins", *module.PINS_GATE, + ["--root", str(ctx.root)]).as_dict() + + +def bump_argv(standard: Path, root: Path, commit: str) -> list[str]: + return [PYTHON, str(standard.resolve() / "scripts" / "bump-leg.py"), + "--root", str(root), "--leg", "spec", "--to", commit] + + +# --- the doctor names both, and the gate is still its NEXT ------------------ + +@pytest.mark.parametrize("own_copy", [True, False]) +def test_a_hand_committed_gitlink_names_both_bump_leg_commands( + standard, hand_moved, own_copy): + """`--to` the gitlink's commit and `--to` the pin's, each a line the + reader's shell hands back whole, in the reason and in `detail`; and the + NEXT is the gate, byte for byte. The template's copy of the gate, which + a root without its own falls back to, names the same two.""" + root = hand_moved["root"] + if not own_copy: + (root / "scripts" / "validate-pins.py").unlink() + gitlink, pinned = hand_moved["gitlink"], hand_moved["pin"] + # The gitlink's commit AS GIT READS IT: the index's stage-0 entry, which + # is what the gate compares and what `bump-leg.py` replaces. + staged = git("ls-files", "--stage", "--", "spec", cwd=root).stdout.split() + assert staged[:3] == ["160000", gitlink, "0"], staged + row = pins_row(standard, root) + gate = gate_row(standard, root) + assert row["status"] == "FINDING", row + assert row["detail"]["exit"] == 1, row + repairs = row["detail"].get("gitlink_repairs") + assert isinstance(repairs, list) and len(repairs) == 1, row["detail"] + keep, back = repairs[0]["keep_gitlink"], repairs[0]["keep_pin"] + assert pasted(keep) == bump_argv(standard, root, gitlink), keep + assert pasted(back) == bump_argv(standard, root, pinned), back + module = doctor_module(standard) + assert repairs == [{ + "role": "spec", "path": "spec", "gitlink": gitlink, "pin": pinned, + "keep_gitlink": module.bump_leg_command(standard.resolve(), root, + "spec", gitlink), + "keep_pin": module.bump_leg_command(standard.resolve(), root, "spec", + pinned)}], repairs + assert f"`{keep}` keeps the gitlink's {gitlink[:12]}" in row["reason"] + assert f"`{back}` keeps the pin's {pinned[:12]}" in row["reason"] + assert "the person's call" in row["reason"], row["reason"] + assert "a branch other than main" in row["reason"], row["reason"] + assert "no branch of the leg's remote contains" in row["reason"] + # What the gate said is kept whole in front of it, and the NEXT and the + # rest of `detail` are the gate's own. + assert row["reason"].startswith(gate["reason"] + ". "), row["reason"] + assert "pin-gitlink-mismatch" in gate["reason"], gate["reason"] + assert row["next"] == gate["next"], row["next"] + assert "bump-leg" not in row["next"], row["next"] + assert {key: value for key, value in row["detail"].items() + if key != "gitlink_repairs"} == gate["detail"] + # And the table says what `--json` says. + table = run_script(standard / DOCTOR, "--root", str(root)) + assert table.returncode == 1, table.stdout + table.stderr + assert row["reason"] in table.stdout, table.stdout + + +def test_the_repairs_carry_the_lane_trailer_inside_a_lane(standard, + hand_moved): + """Each line writes a commit, so it carries the lane's `--trailer`, as + the `shape currency` row's `apply` does (#111); and none without one.""" + root = hand_moved["root"] + plain = pins_row(standard, root, {"LANES_LANE": ""}) + lane = pins_row(standard, root, {"LANES_LANE": LANE}) + for key in ("keep_gitlink", "keep_pin"): + without = plain["detail"]["gitlink_repairs"][0][key] + within = lane["detail"]["gitlink_repairs"][0][key] + assert "--trailer" not in without, without + assert within == without + f" --trailer 'Lane: {LANE}'", within + assert pasted(within)[-2:] == ["--trailer", f"Lane: {LANE}"] + assert f"`{within}`" in lane["reason"], lane["reason"] + + +#: `bump_leg_command` for each shell, from either host: `platform` is passed +#: on to every value, as `copy_command`'s is, so the Windows line is asserted +#: on a POSIX runner too. A blank and an apostrophe in the root, so both +#: quotings are exercised: `sh` closes, escapes and reopens, PowerShell +#: doubles. +BUMP_LINES = { + "posix": (PurePosixPath("/srv/my work/openRepoShape"), + PurePosixPath("/srv/it's/Atlas"), + "python3 '/srv/my work/openRepoShape/scripts/bump-leg.py' " + "--root '/srv/it'\\''s/Atlas' --leg spec --to " + OTHER_SHA), + "nt": (PureWindowsPath("C:/shape/openRepoShape"), + PureWindowsPath("C:/Users/Jane O'Neill/Atlas"), + "python C:\\shape\\openRepoShape\\scripts\\bump-leg.py " + "--root 'C:\\Users\\Jane O''Neill\\Atlas' --leg spec --to " + + OTHER_SHA), +} + + +@pytest.mark.parametrize("platform", sorted(BUMP_LINES)) +def test_the_bump_line_is_spelled_for_each_shell(standard, monkeypatch, + platform): + monkeypatch.delenv("LANES_LANE", raising=False) + shape, root, line = BUMP_LINES[platform] + module = doctor_module(standard) + assert module.bump_leg_command(shape, root, "spec", OTHER_SHA, + platform) == line + + +# --- the gate names the same two -------------------------------------------- + +def test_the_gate_names_the_same_two_commands(hand_moved): + """The project's own `validate-pins.py`: still the finding and THE + LOCKSTEP RULE, then the two lines, from a checkout of openRepoShape, with + this root quoted as `validate-family.py` quotes a holder's.""" + root = hand_moved["root"] + result = run_script(root / "scripts" / "validate-pins.py", cwd=root) + assert result.returncode == 1, result.stdout + result.stderr + lines = result.stderr.splitlines() + first = lines.index( + f"FINDING pin-gitlink-mismatch: spec: gitlink {hand_moved['gitlink']}" + f" != contracts/spec-pin.yaml commit {hand_moved['pin']}") + assert lines[first + 1].startswith(" THE LOCKSTEP RULE: "), lines + assert lines[first + 2].startswith( + " Which commit stays is the person's call. From a checkout of " + "openRepoShape,"), lines + bump = (f" {PYTHON} scripts/bump-leg.py --root " + f"{shlex.quote(str(root))} --leg spec --to") + assert lines[first + 3] == f"{bump} {hand_moved['gitlink']}", lines + assert lines[first + 4] == f"{bump} {hand_moved['pin']}", lines + + +# --- running either one is the cure ----------------------------------------- + +@pytest.mark.parametrize("which", ["keep_gitlink", "keep_pin"]) +def test_each_repair_run_as_printed_makes_the_doctor_compliant( + standard, hand_moved, tmp_path, which): + """Run from elsewhere exactly as the row printed it, each line writes ONE + commit on the branch, puts the gitlink and the pin at its commit, and + the doctor says COMPLIANT, exit 0.""" + root = hand_moved["root"] + row = pins_row(standard, root) + line = row["detail"]["gitlink_repairs"][0][which] + want = hand_moved["gitlink" if which == "keep_gitlink" else "pin"] + elsewhere = tmp_path / "elsewhere" + elsewhere.mkdir() + ran = subprocess.run(pasted(line), cwd=str(elsewhere), capture_output=True, + text=True, encoding="utf-8", errors="replace", + check=False, stdin=subprocess.DEVNULL, + env={**os.environ, **GIT_IDENTITY, + "PYTHONDONTWRITEBYTECODE": "1"}) + assert ran.returncode == 0, line + "\n" + ran.stdout + ran.stderr + count = git("rev-list", "--count", f"{hand_moved['base']}..HEAD", + cwd=root).stdout.strip() + assert count == "1", count + assert git("rev-parse", "HEAD:spec", cwd=root).stdout.strip() == want + assert pin_of(root) == want + assert git("symbolic-ref", "--short", "HEAD", + cwd=root).stdout.strip() == WORKING + after = doctor(standard, root, "--json") + assert after.returncode == 0, after.stdout + after.stderr + rows = rows_of(after) + assert rows["pins"]["status"] == "ok", rows["pins"] + assert rows["legs"]["status"] == "ok", rows["legs"] + assert json.loads(after.stdout)["verdict"] == "COMPLIANT", after.stdout + + +def test_git_submodule_update_is_no_undo(standard, hand_moved): + """WHY THE UNDO IS `bump-leg.py --to ` AND NOT A GIT LINE, + pinned by what git does: `submodule update --checkout` checks the leg out + at the GITLINK, which is the commit being undone, so it leaves the leg + where it is and the row where it was.""" + root = hand_moved["root"] + git(*FILE_PROTOCOL, "submodule", "update", "--init", "--checkout", "--", + "spec", cwd=root) + assert git("rev-parse", "HEAD", cwd=root / "spec").stdout.strip() == \ + hand_moved["gitlink"] + assert "gitlink_repairs" in pins_row(standard, root)["detail"] + + +# --- every other pins row is the gate's, untouched -------------------------- + +def edit(root: Path, relative: str, pattern: str, replacement: str) -> None: + target = root / relative + text = target.read_text(encoding="utf-8") + edited, count = re.subn(pattern, replacement, text) + assert count == 1, (relative, pattern) + target.write_text(edited, encoding="utf-8") + + +def digest_moved(root: Path) -> None: + edit(root, "contracts/spec-pin.yaml", + r'(?m)^( tree_sha256: ")[0-9a-f]{64}(")$', r"\g<1>" + "0" * 64 + + r"\g<2>") + + +def path_moved(root: Path) -> None: + edit(root, "contracts/spec-pin.yaml", r'(?m)^submodule_path: .*$', + 'submodule_path: "elsewhere"') + + +def workflow_off(root: Path) -> None: + workflows = root / ".github" / "workflows" + workflows.mkdir(parents=True, exist_ok=True) + (workflows / "legs.yml").write_text( + "on: [pull_request]\njobs:\n spec:\n uses: " + f"testorg/{PROJECT}-spec/.github/workflows/x.yml@{OTHER_SHA}\n", + encoding="utf-8") + + +def copy_drifted(root: Path) -> None: + target = root / DRIFT_TARGET + target.write_text(target.read_text(encoding="utf-8") + "\n# edit\n", + encoding="utf-8") + + +def refused_with_the_gitlink_moved(root: Path) -> None: + """A gitlink off its pin AND a gate that refuses before it reports it: + the refusal is the thing to fix first, so no repair is named.""" + leg = root / "spec" + git("commit", "-q", "--allow-empty", "-m", "past the pin", cwd=leg) + moved = git("rev-parse", "HEAD", cwd=leg).stdout.strip() + git("update-index", "--cacheinfo", f"160000,{moved},spec", cwd=root) + (root / "code" / ".git").unlink() + + +OTHER_FINDINGS = { + "pin-digest-mismatch": (digest_moved, 1), + "pin-path-mismatch": (path_moved, 1), + "pin-workflow-ref-mismatch": (workflow_off, 1), + "shape-copy-drift": (copy_drifted, 1), + "pin-submodule-uninitialized": (refused_with_the_gitlink_moved, 2), +} + + +@pytest.mark.parametrize("code", sorted(OTHER_FINDINGS)) +def test_every_other_pins_row_is_the_gates_own(standard, project, code): + """Byte for byte the row the gate alone makes, with no repair named: + four findings a gitlink at its pin can have, and a refusal over a + gitlink that is not.""" + root = project.resolve() + make, exit_code = OTHER_FINDINGS[code] + make(root) + row = pins_row(standard, root) + assert row["detail"]["exit"] == exit_code, row + assert code in row["reason"], row["reason"] + assert row == gate_row(standard, root) + assert "gitlink_repairs" not in row["detail"], row["detail"] + assert "bump-leg" not in row["reason"], row["reason"] + gate = run_script(root / "scripts" / "validate-pins.py", cwd=root) + assert "bump-leg.py" not in gate.stdout + gate.stderr From 8c87340b2d32fb65c30453dcfa64656093a110bb Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Tue, 13 Oct 2026 00:09:22 +0000 Subject: [PATCH 2/2] shape-doctor.py and validate-pins.py: a gitlink off its pin is offered the two `bump-leg.py` lines only where one can commit, so two legs pulled by hand, or a gitlink moved beside a `shape-copy-drift`, no longer names lines that `bump-leg.py` rolls back every time (#478) `bump-leg.py` runs the root's own `validate-pins.py` and `validate-manifest.py` after its rewrite and before it commits, and rolls every byte back when either is red. With both legs off their pins, or one leg off beside any finding its bump does not clear, the `pins` row and the gate named lines that exit 2 `bump-leg-validators-red`, and the reason said only the branch and the push stood in their way (review of #495, finding 1; codex thread 1). - The lines are named only where exactly ONE leg's gitlink is off its pin, every other finding the gate reports is one a bump of that leg clears (its pin's digest, recomputed, or a workflow reference to its repository, moved), and, in the doctor, the manifest's gate passes. The doctor reads the gate's findings from its output (`validator_row`'s new `output=`), by the spelling every `validate-pins.py` since v0 prints; the gate tags each finding with the leg that clears it. - Otherwise the row and the gate name no line and say why and what comes first: clear the other finding, or put one leg's gitlink back at its pin (or undo the commit that moved both), after which the lines for the leg that is left are named. `detail.gitlink_repairs_held_back` carries the legs, the finding codes and whether the manifest's gate is red. - Only over a root with a gate of its own: `bump-leg.py` skips an absent one, so the template copy's findings do not say what the bump's run will. Such a root's row is the template gate's, untouched, as before #478. - Where the lines are named, they say to run either with `--dry-run` first (finding 2; codex thread 2), and that the bump commits once both of the root's validators pass on its rewrite. - The NEXT is unchanged: the gate, which names the same thing. - The doctor's docstring no longer cites a `merge` row that is not on main (finding 4). - Tests: two legs off; one leg off beside `shape-copy-drift` and beside the other leg's workflow reference; a red manifest; the leg's own digest and workflow reference holding nothing back; a root without its own gate; the crash guard (finding 5). Unchanged: the `pins` row and the gate's output for every finding where no gitlink is off its pin, and for a refusal (`test_every_other_pins_row_is_the_gates_own`). Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Opus 5.5 --- shape-doctor.py | 168 +++++++-- .../assembly-root/scripts/validate-pins.py | 119 +++++- tests/test_doctor_gitlink_repairs.py | 342 +++++++++++++++--- 3 files changed, 541 insertions(+), 88 deletions(-) diff --git a/shape-doctor.py b/shape-doctor.py index 7f8132bf..1faf2e97 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -1367,7 +1367,8 @@ def validator_command(shape: Path, used: Path, own_copy: bool, args: list, def validator_row(ctx: Context, check_id: str, label: str, own: str, - template: str, args: list[str]) -> Row: + template: str, args: list[str], *, + output: list[str] | None = None) -> Row: """One row for a validator that the ROOT may carry its own copy of. THE PROJECT'S OWN COPY FIRST, ALWAYS, AND THE ROW SAYS SO. That copy is @@ -1419,6 +1420,10 @@ def validator_row(ctx: Context, check_id: str, label: str, own: str, "exit": None, "environment": True}) code, quoted, whole = run_validator(ctx, used, args, validator_scripts(ctx.shape, own_copy)) + if output is not None: + # What the validator printed, whole, for a caller that reads more of + # it than the one line the row quotes: `check_pins` (#478). + output.append(whole) detail = {"validator": used.as_posix(), "own_copy": own_copy, "exit": code} if code == 0: @@ -1681,8 +1686,10 @@ def check_pins(ctx: Context) -> Row: "a single pin, has nothing to check yet") if waiting is not None: return waiting - return with_gitlink_repairs(ctx, validator_row( - ctx, "pins", "pins", *PINS_GATE, ["--root", str(ctx.root)])) + output: list[str] = [] + row = validator_row(ctx, "pins", "pins", *PINS_GATE, + ["--root", str(ctx.root)], output=output) + return with_gitlink_repairs(ctx, row, "".join(output)) def bump_leg_command(shape: Path, root: Path, role: str, commit: str, @@ -1731,9 +1738,82 @@ def gitlinks_off_their_pins(ctx: Context) -> list[dict]: return off -def with_gitlink_repairs(ctx: Context, row: Row) -> Row: +#: A finding as every `validate-pins.py` since v0 prints one, at the start of +#: a line: its code, and the rest of that line (#478). +FINDING_LINE = re.compile(r"FINDING (?P[^\s:]+): (?P.*)") + + +def bump_clears(code: str, rest: str, leg: dict) -> bool: + """Whether the `bump-leg.py` line for `leg` clears the gate's finding + `code`, whose line goes on with `rest` (#478). + + THE THREE FACTS IT REWRITES, read as the gate has spelled each finding + since v0: the leg's gitlink against its pin (`: gitlink ...`), its + pin's digest (`contracts/-pin.yaml: ...`, recomputed), and a + workflow reference to its repository (`` `@` ``, every + one moved). Any other finding still stands after the rewrite, so the + gate the bump runs is red and the bump is rolled back. + """ + if code == "pin-gitlink-mismatch": + return rest.startswith(f"{leg['path']}: gitlink ") + if code == "pin-digest-mismatch": + return rest.startswith(f"contracts/{leg['role']}-pin.yaml: ") + if code == "pin-workflow-ref-mismatch": + return bool(leg["repository"]) and f"`{leg['repository']}@" in rest + return False + + +def findings_no_bump_clears(output: str, off: list[dict]) -> list[str]: + """The codes of the findings in the gate's `output` that no + `bump-leg.py` line for a leg in `off` clears, each once, in the gate's + order (#478).""" + left: list[str] = [] + for line in output.splitlines(): + found = FINDING_LINE.match(line) + if found is None: + continue + code, rest = found.group("code", "rest") + if not any(bump_clears(code, rest, leg) for leg in off) \ + and code not in left: + left.append(code) + return left + + +def gitlink_repairs_held_back(off: list[dict], others: list[str], + manifest: bool) -> str: + """Why the `pins` row names no `bump-leg.py` line for a gitlink off its + pin, and what comes first (#478).""" + paths = [leg["path"] for leg in off] + if manifest: + return ("No single bump-leg.py line can commit while the root's " + "validate-manifest.py is red (the `manifest` row): bump-leg.py " + "runs it after validate-pins.py, before it commits, and rolls " + "the bump back when either is red. First clear that row's " + "finding; this row then names the two bump-leg.py lines for " + f"{paths[0]}") + what, first = [], [] + if len(off) > 1: + what.append(" and ".join(paths) + (" are both" if len(off) == 2 + else " are all") + " off their pins") + if others: + what.append("the gate also reports " + ", ".join(others)) + first.append("clear " + ("that finding" if len(others) == 1 + else "those findings")) + if len(off) > 1: + first.append("put one leg's gitlink back at its pin (check that leg " + "out at the pin's commit and commit its gitlink in the " + "root), or undo the commit that moved them") + return ("No single bump-leg.py line can commit while " + + " and ".join(what) + ": bump-leg.py moves one leg and runs the " + "root's whole validate-pins.py before it commits, rolling the bump " + "back when it is red. First " + ", and ".join(first) + "; this row " + "then names the two bump-leg.py lines for " + + ("the leg that is left" if len(off) > 1 else paths[0])) + + +def with_gitlink_repairs(ctx: Context, row: Row, output: str) -> Row: """The `pins` row, naming both repairs for a gitlink that is not its pin - (#478). + where one can commit, and what comes first where none can (#478). THE DEFECT. A leg moved by hand and committed (`git -C spec pull`, then `git add spec && git commit` in the root) made this row a FINDING whose @@ -1744,12 +1824,22 @@ def with_gitlink_repairs(ctx: Context, row: Row) -> Row: `bump-leg.py` lines and chooses neither: `--to` the gitlink's commit keeps the move, and `--to` the pin's undoes it. `bump-leg.py` either way, because it moves the gitlink, the pin and every workflow reference - in one commit and runs the gate before it commits. `git submodule - update` is no undo: it checks the leg out at the gitlink, which is the - commit being undone. The two lines are in the reason and in - `detail.gitlink_repairs`, never in the NEXT, as the `merge` row offers - its choice (#424): a NEXT is run as printed, and this one is the gate, - which now names the same two. + in one commit, and its `--dry-run` comes first, as AGENTS.md says. + `git submodule update` is no undo: it checks the leg out at the gitlink, + which is the commit being undone. The two lines are in the reason and + in `detail.gitlink_repairs`, never in the NEXT: a NEXT is run as + printed, and which commit stays is not the doctor's to choose. The NEXT + stays the gate, which names the same two. + + ONLY WHERE A LINE CAN COMMIT. `bump-leg.py` runs the root's own + `validate-pins.py` and `validate-manifest.py` after its rewrite and + before it commits, and rolls every byte back when either is red. So the + lines are named only where exactly ONE leg's gitlink is off its pin, + every other finding in the gate's `output` is one a bump of that leg + clears (`bump_clears`), and the manifest's gate passes. Two legs off, or + any other finding, and either line is rolled back: the reason names no + line, says why, and says what comes first, and + `detail.gitlink_repairs_held_back` says the same for `--json`. THE COMMIT MUST BE ON A BRANCH OF THE LEG'S REMOTE, and offline that cannot be known: `bump-leg.py` fetches before it asks, and a @@ -1757,35 +1847,53 @@ def with_gitlink_repairs(ctx: Context, row: Row) -> Row: not what it holds. So the reason says what the command needs, and the doctor still fetches nothing. - ONLY A GATE THAT REPORTED FINDINGS, exit 1, and did not crash. A refusal - stops `validate-pins.py` before it reports any finding, and that is the - thing to fix first; any other row is returned untouched. The row is - changed IN PLACE, reason and detail, so whatever else a `Row` carries is - kept. + ONLY THE ROOT'S OWN GATE, THAT REPORTED FINDINGS, exit 1, and did not + crash. A refusal stops `validate-pins.py` before it reports any finding, + and that is the thing to fix first, and a gate that crashed is one + `bump-leg.py` would run and find red too. A root with no gate of its + own was answered by the standard's template copy, and `bump-leg.py` + runs the root's copy and skips an absent one, so what the template + reports does not say what the bump's run will. Any other row is + returned untouched. The row is changed IN PLACE, reason and detail, so + whatever else a `Row` carries is kept. """ if row.status != FINDING or row.detail.get("exit") != 1 \ - or row.detail.get("crashed"): + or row.detail.get("crashed") or not row.detail.get("own_copy"): return row off = gitlinks_off_their_pins(ctx) if not off: return row - for leg in off: - leg["keep_gitlink"] = bump_leg_command(ctx.shape, ctx.root, - leg["role"], leg["gitlink"]) - leg["keep_pin"] = bump_leg_command(ctx.shape, ctx.root, leg["role"], - leg["pin"]) + repository = {str(leg.get("role") or ""): str(leg.get("repository") or "") + for leg in ctx.legs()} + others = findings_no_bump_clears(output, [ + {**leg, "repository": repository.get(leg["role"], "")} + for leg in off]) + manifest = (len(off) == 1 and not others + and check_manifest(ctx).status == FINDING) + if len(off) > 1 or others or manifest: + row.reason = ascii_text( + f"{row.reason}. " + + gitlink_repairs_held_back(off, others, manifest)) + row.detail["gitlink_repairs_held_back"] = { + "legs": off, "findings": others, "manifest": manifest} + return row + leg = off[0] + leg["keep_gitlink"] = bump_leg_command(ctx.shape, ctx.root, leg["role"], + leg["gitlink"]) + leg["keep_pin"] = bump_leg_command(ctx.shape, ctx.root, leg["role"], + leg["pin"]) tracking = str((ctx.manifest or {}).get("tracking_branch") or "main") - each = "; ".join( - f"for {leg['path']}, `{leg['keep_gitlink']}` keeps the gitlink's " - f"{leg['gitlink'][:12]} and `{leg['keep_pin']}` keeps the pin's " - f"{leg['pin'][:12]}" for leg in off) row.reason = ascii_text( f"{row.reason}. The gitlink and the pin name two commits, and which " "stays is the person's call; bump-leg.py puts the gitlink, the pin " - f"and every workflow reference at it in one commit: {each}. Either " - f"commits on a branch other than {tracking} and refuses a commit no " - "branch of the leg's remote contains, so a commit made in the leg " - "is pushed first") + f"and every workflow reference at it in one commit: for {leg['path']}" + f", `{leg['keep_gitlink']}` keeps the gitlink's {leg['gitlink'][:12]} " + f"and `{leg['keep_pin']}` keeps the pin's {leg['pin'][:12]}; run " + "either with `--dry-run` first. Either commits on a branch other than " + f"{tracking}, once the root's own validate-pins.py and " + "validate-manifest.py pass on its rewrite (it rolls the bump back " + "when either is red), and refuses a commit no branch of the leg's " + "remote contains, so a commit made in the leg is pushed first") row.detail["gitlink_repairs"] = off return row diff --git a/templates/assembly-root/scripts/validate-pins.py b/templates/assembly-root/scripts/validate-pins.py index 61b9c99b..9599bdf9 100755 --- a/templates/assembly-root/scripts/validate-pins.py +++ b/templates/assembly-root/scripts/validate-pins.py @@ -97,9 +97,20 @@ class Report: def __init__(self) -> None: self.findings: list[str] = [] self.notes: list[str] = [] - - def finding(self, code: str, detail: str) -> None: + #: Each finding's code, and the role of the leg whose `bump-leg.py` + #: line puts it right, or None: in step with `findings` (#478). + self.cleared_by: list[tuple[str, object]] = [] + #: Each `pin-gitlink-mismatch`, as `(its index in findings, role, + #: path, gitlink, pin commit)`, for `_end_gitlink_mismatches` (#478). + self.mismatches: list[tuple[int, object, str, str, str]] = [] + + def finding(self, code: str, detail: str, leg=None) -> None: + """`leg` is the role of the leg a `bump-leg.py` line moves when that + line also clears this finding: the leg's gitlink against its pin, + its pin's digest, and a workflow reference to its repository, the + three facts it rewrites (#478).""" self.findings.append(f"FINDING {code}: {detail}") + self.cleared_by.append((code, leg)) def note(self, text: str) -> None: self.notes.append(f" ok {text}") @@ -241,7 +252,8 @@ def _git_runs(unread: str) -> Iterator[None]: def _gitlink_repairs(root: Path, role, gitlink: str, commit: str) -> str: - """The two commands a `pin-gitlink-mismatch` ends on (#478). + """The two commands a `pin-gitlink-mismatch` ends on, when they can + commit (#478; `_end_gitlink_mismatches` says when). WHICH COMMIT STAYS IS THE PERSON'S CALL, so both are named and neither is chosen. A gitlink moved and committed by hand (`git -C spec pull`, @@ -250,7 +262,9 @@ def _gitlink_repairs(root: Path, role, gitlink: str, commit: str) -> str: was not; a pin edited by hand reads the same with the two the other way round. Either is `bump-leg.py`, which puts all three facts at one commit in ONE commit and runs this validator before it commits: a gitlink put - back by hand is the same single-fact move this check refuses. + back by hand is the same single-fact move this check refuses. Its + `--dry-run` comes first, as openRepoShape's AGENTS.md says of advancing + a leg. FROM A CHECKOUT OF openRepoShape, as `validate-family.py` names `family.py`: no project carries `bump-leg.py`, and this file cannot know @@ -261,10 +275,74 @@ def _gitlink_repairs(root: Path, role, gitlink: str, commit: str) -> str: f"--leg {shlex.quote(str(role))} --to") return ("Which commit stays is the person's call. From a checkout of " "openRepoShape, the first command keeps the gitlink's commit and " - "the second the pin's. Either puts all three at it in one commit, " - "on a branch other than the tracking branch, and refuses a commit " - "no branch of the leg's remote contains, so push one made in the " - f"leg first:\n {bump} {gitlink}\n {bump} {commit}") + "the second the pin's; run either with `--dry-run` first. Either " + "puts all three at it in one commit, on a branch other than the " + "tracking branch, once this validator and validate-manifest.py " + "pass on its rewrite (it rolls the bump back when either is red), " + "and refuses a commit no branch of the leg's remote contains, so " + f"push one made in the leg first:\n {bump} {gitlink}\n" + f" {bump} {commit}") + + +def _gitlink_repairs_held_back(paths: list[str], others: list[str]) -> str: + """Why a `pin-gitlink-mismatch` names no command, and what comes first + (#478): `bump-leg.py` runs this validator before it commits and rolls + the bump back when it is red, so no single line commits while another + leg is off its pin or another finding stands.""" + what, first = [], [] + if others: + what.append("this validator also reports " + ", ".join(others)) + first.append("clear " + ("that finding" if len(others) == 1 + else "those findings")) + if len(paths) > 1: + what.insert(0, " and ".join(paths) + (" are both" if len(paths) == 2 + else " are all") + + " off their pins") + first.append("put one leg's gitlink back at its pin (check that leg " + "out at the pin's commit and commit its gitlink in this " + "root), or undo the commit that moved them") + return ("No single bump-leg.py line can commit while " + + " and ".join(what) + ": bump-leg.py moves one leg and runs " + "this validator before it commits, rolling the bump back when it " + "is red. First " + ", and ".join(first) + "; this validator then " + "names the two bump-leg.py lines for " + + ("the leg that is left." if len(paths) > 1 else "that leg.")) + + +def _end_gitlink_mismatches(root: Path, report: Report) -> None: + """What each `pin-gitlink-mismatch` ends on, once every check has run + (#478). + + `bump-leg.py` RUNS THIS WHOLE VALIDATOR after its rewrite and before it + commits, and rolls the bump back when it is red. So its two lines are + named only where they can commit: ONE leg's gitlink is off its pin, and + every other finding is one a bump of that leg clears (`Report.finding`'s + `leg`). Two legs off, or any other finding, and either line is rolled + back, so the finding names none and says what comes first instead. + + ONLY OVER A ROOT THAT HAS THIS GATE OF ITS OWN. `bump-leg.py` runs the + root's `scripts/validate-pins.py` and skips an absent one, so this copy + run against a root that has none cannot say what the bump's run will: + the finding ends at THE LOCKSTEP RULE, as it did before #478, and the + root's own gate is the thing to put back first. + """ + if not report.mismatches \ + or not os.path.isfile(root / "scripts" / "validate-pins.py"): + return + roles = [role for _, role, _, _, _ in report.mismatches] + others: list[str] = [] + for code, leg in report.cleared_by: + if (leg is None or leg not in roles) and code not in others: + others.append(code) + if len(report.mismatches) == 1 and not others: + index, role, _, gitlink, commit = report.mismatches[0] + report.findings[index] += "\n " + _gitlink_repairs(root, role, + gitlink, commit) + return + why = _gitlink_repairs_held_back( + [path for _, _, path, _, _ in report.mismatches], others) + for index, *_ in report.mismatches: + report.findings[index] += "\n " + why def _check_gitlink_matches_pin(root: Path, role, path: str, commit: str, @@ -286,19 +364,22 @@ def _check_gitlink_matches_pin(root: Path, role, path: str, commit: str, if gitlink != commit: report.finding( "pin-gitlink-mismatch", - f"{path}: gitlink {gitlink} != {rel} commit {commit}\n " + LOCKSTEP - + "\n " + _gitlink_repairs(root, role, gitlink, commit), + f"{path}: gitlink {gitlink} != {rel} commit {commit}\n " + LOCKSTEP, + leg=role, ) + report.mismatches.append((len(report.findings) - 1, role, path, + gitlink, commit)) else: report.note(f"{path}: gitlink == {rel} commit {commit[:12]}") return gitlink def _check_pin_digest(root: Path, path: str, commit: str, pin: dict, rel: str, - report: Report) -> None: + report: Report, role=None) -> None: """The digest: split from `_check_leg` for #132. Recomputes the leg's tree sha256 at the pinned commit, because a gitlink and a pin file that - agree on a commit may still disagree on that commit's bytes. + agree on a commit may still disagree on that commit's bytes. A mismatch + is one `bump-leg.py` clears, since it recomputes the digest (#478). """ submodule = root / path if not (submodule / ".git").exists(): @@ -344,15 +425,18 @@ def _check_pin_digest(root: Path, path: str, commit: str, pin: dict, rel: str, f" recomputed at {commit[:12]} {actual}\n" f" The gitlink and the pin may agree on a commit whose bytes " f"are not the bytes the pin was written against.\n " + LOCKSTEP, + leg=role, ) else: report.note(f"{path}: tree digest recomputes ({recorded[:12]}…)") def _check_workflow_refs_for_leg(refs, repository: str, path: str, - gitlink: str, report: Report) -> None: + gitlink: str, report: Report, + role=None) -> None: """Fact 3: split from `_check_leg` for #132 — every workflow `@` - reference naming this leg's repository must agree with the gitlink. + reference naming this leg's repository must agree with the gitlink. A + mismatch is one `bump-leg.py` clears, since it moves every one (#478). """ seen = 0 for wf_path, wf_repo, wf_sha in refs: @@ -364,6 +448,7 @@ def _check_workflow_refs_for_leg(refs, repository: str, path: str, "pin-workflow-ref-mismatch", f"{wf_path.name}: `{wf_repo}@{wf_sha}` != the gitlink " f"{gitlink} recorded at {path!r}\n " + LOCKSTEP, + leg=role, ) if seen: report.note(f"{repository}: {seen} workflow @ reference(s) " @@ -376,8 +461,9 @@ def _check_leg(root: Path, leg: dict, report: Report, refs) -> None: repository = str(leg.get("repository") or "") pin, rel, commit = _load_leg_pin(root, role, path, repository, report) gitlink = _check_gitlink_matches_pin(root, role, path, commit, rel, report) - _check_pin_digest(root, path, commit, pin, rel, report) - _check_workflow_refs_for_leg(refs, repository, path, gitlink, report) + _check_pin_digest(root, path, commit, pin, rel, report, role) + _check_workflow_refs_for_leg(refs, repository, path, gitlink, report, + role) def _check_shape_pin_header(root: Path, manifest: dict, report: Report @@ -868,6 +954,7 @@ def _run_all_checks(root: Path, report: Report, _check_leg(root, leg, report, refs) _check_shape_pin(root, manifest, report) _check_neutral_product_pins(root, manifest, report, pin_source_values) + _end_gitlink_mismatches(root, report) def _emit_report(report: Report, quiet: bool) -> int: diff --git a/tests/test_doctor_gitlink_repairs.py b/tests/test_doctor_gitlink_repairs.py index 07c6508e..59c95c02 100644 --- a/tests/test_doctor_gitlink_repairs.py +++ b/tests/test_doctor_gitlink_repairs.py @@ -15,6 +15,14 @@ gate names the same two, from a checkout of openRepoShape. The row's NEXT is still the gate. +ONLY WHERE ONE CAN COMMIT. `bump-leg.py` runs the root's whole gate, and +`validate-manifest.py`, before it commits, and rolls the bump back when +either is red. With two legs off their pins, or any finding the bump does +not clear, every line would be rolled back, so the doctor and the gate name +none and say what comes first (`detail.gitlink_repairs_held_back`). A +finding the bump DOES clear, the leg's own digest or a workflow reference +to it, holds nothing back. + EVERY REPAIR HERE IS RUN, from a directory that is not the root and through the argv the reader's own shell makes of the printed line (`pasted`: `shlex` on POSIX, PowerShell's reading on Windows), so these run on all three @@ -61,24 +69,37 @@ OTHER_SHA = "0123456789abcdef0123456789abcdef01234567" -@pytest.fixture(scope="module") -def advanced(scaffolded_here, tmp_path_factory) -> str: - """A new commit on a branch of the spec leg's remote, as a merged pull - request leaves one; its sha.""" - work = tmp_path_factory.mktemp("spec-advance") / f"{PROJECT}-spec" - remote = scaffolded_here["base"] / "remotes" / f"{PROJECT}-spec.git" +def advance(scaffolded_here, tmp_path_factory, role: str, + relative: str) -> str: + """A new commit on a branch of a leg's remote, as a merged pull request + leaves one, adding `relative` (a path the `placement` row puts in that + leg); its sha.""" + work = tmp_path_factory.mktemp(f"{role}-advance") / f"{PROJECT}-{role}" + remote = scaffolded_here["base"] / "remotes" / f"{PROJECT}-{role}.git" proc = subprocess.run(["git", "clone", "-q", str(remote), str(work)], capture_output=True, text=True, check=False) assert proc.returncode == 0, proc.stderr - (work / "requirements").mkdir(exist_ok=True) - (work / "requirements" / "r478.md").write_text("# R478\n", - encoding="utf-8") - git("add", "--", "requirements/r478.md", cwd=work) + (work / relative).parent.mkdir(parents=True, exist_ok=True) + (work / relative).write_text("# R478\n", encoding="utf-8") + git("add", "--", relative, cwd=work) git("commit", "-qm", "Add R478", cwd=work) git("push", "-q", "origin", f"HEAD:refs/heads/{ADVANCE}", cwd=work) return git("rev-parse", "HEAD", cwd=work).stdout.strip() +@pytest.fixture(scope="module") +def advanced(scaffolded_here, tmp_path_factory) -> str: + """A new commit on a branch of the SPEC leg's remote.""" + return advance(scaffolded_here, tmp_path_factory, "spec", + "requirements/r478.md") + + +@pytest.fixture(scope="module") +def code_advanced(scaffolded_here, tmp_path_factory) -> str: + """A new commit on a branch of the CODE leg's remote.""" + return advance(scaffolded_here, tmp_path_factory, "code", "src/r478.py") + + def pin_of(root: Path, role: str = "spec") -> str: text = (root / "contracts" / f"{role}-pin.yaml").read_text( encoding="utf-8") @@ -96,22 +117,35 @@ def under_an_awkward_name(project: Path, tmp_path: Path) -> Path: return root.resolve() -@pytest.fixture -def hand_moved(project, advanced, tmp_path) -> dict: - """The issue's reproduction: the leg fetched and checked out at the new - commit, and the gitlink committed by hand in the root, on a branch.""" +def moved_by_hand(project: Path, tmp_path: Path, moves: dict, + also=None) -> dict: + """The issue's reproduction: each leg in `moves` fetched and checked out + at its new commit, and the gitlinks committed by hand in the root, on a + branch, in ONE commit with whatever `also(root)` changed beside them.""" root = under_an_awkward_name(project, tmp_path) - pinned = pin_of(root) - leg = root / "spec" - git(*FILE_PROTOCOL, "fetch", "-q", "origin", ADVANCE, cwd=leg) - git("checkout", "-q", "--detach", advanced, cwd=leg) + pins = {role: pin_of(root, role) for role in moves} git("checkout", "-q", "-b", WORKING, cwd=root) - git("add", "--", "spec", cwd=root) - git("commit", "-qm", "take the new spec", cwd=root) - return {"root": root, "gitlink": advanced, "pin": pinned, + for role, commit in moves.items(): + git(*FILE_PROTOCOL, "fetch", "-q", "origin", ADVANCE, cwd=root / role) + git("checkout", "-q", "--detach", commit, cwd=root / role) + git("add", "--", *moves, cwd=root) + if also is not None: + also(root) + git("add", "-A", "--", ".", cwd=root) + git("commit", "-qm", "take the new legs", cwd=root) + return {"root": root, "pins": pins, "base": git("rev-parse", "HEAD", cwd=root).stdout.strip()} +@pytest.fixture +def hand_moved(project, advanced, tmp_path) -> dict: + """The issue's own case: the spec leg pulled, and its gitlink committed + by hand in the root, on a branch.""" + moved = moved_by_hand(project, tmp_path, {"spec": advanced}) + return {"root": moved["root"], "gitlink": advanced, + "pin": moved["pins"]["spec"], "base": moved["base"]} + + def pins_row(standard: Path, root: Path, env: dict | None = None) -> dict: result = run_script(standard / DOCTOR, "--root", str(root), "--json", env=env) @@ -133,18 +167,38 @@ def bump_argv(standard: Path, root: Path, commit: str) -> list[str]: "--root", str(root), "--leg", "spec", "--to", commit] +def run_as_printed(line: str, tmp_path: Path) -> None: + """Run a line the row named exactly as printed, through the reader's + shell's reading of it, from a directory that is not the root; and + require exit 0.""" + elsewhere = tmp_path / "elsewhere" + elsewhere.mkdir(exist_ok=True) + ran = subprocess.run(pasted(line), cwd=str(elsewhere), capture_output=True, + text=True, encoding="utf-8", errors="replace", + check=False, stdin=subprocess.DEVNULL, + env={**os.environ, **GIT_IDENTITY, + "PYTHONDONTWRITEBYTECODE": "1"}) + assert ran.returncode == 0, line + "\n" + ran.stdout + ran.stderr + + +def assert_compliant(standard: Path, root: Path) -> None: + after = doctor(standard, root, "--json") + assert after.returncode == 0, after.stdout + after.stderr + rows = rows_of(after) + assert rows["pins"]["status"] == "ok", rows["pins"] + assert rows["legs"]["status"] == "ok", rows["legs"] + assert json.loads(after.stdout)["verdict"] == "COMPLIANT", after.stdout + + # --- the doctor names both, and the gate is still its NEXT ------------------ -@pytest.mark.parametrize("own_copy", [True, False]) def test_a_hand_committed_gitlink_names_both_bump_leg_commands( - standard, hand_moved, own_copy): + standard, hand_moved): """`--to` the gitlink's commit and `--to` the pin's, each a line the - reader's shell hands back whole, in the reason and in `detail`; and the - NEXT is the gate, byte for byte. The template's copy of the gate, which - a root without its own falls back to, names the same two.""" + reader's shell hands back whole, in the reason and in `detail`, with + their `--dry-run` first and what each needs to commit; and the NEXT is + the gate, byte for byte.""" root = hand_moved["root"] - if not own_copy: - (root / "scripts" / "validate-pins.py").unlink() gitlink, pinned = hand_moved["gitlink"], hand_moved["pin"] # The gitlink's commit AS GIT READS IT: the index's stage-0 entry, which # is what the gate compares and what `bump-leg.py` replaces. @@ -171,6 +225,10 @@ def test_a_hand_committed_gitlink_names_both_bump_leg_commands( assert "the person's call" in row["reason"], row["reason"] assert "a branch other than main" in row["reason"], row["reason"] assert "no branch of the leg's remote contains" in row["reason"] + assert "run either with `--dry-run` first" in row["reason"] + assert ("once the root's own validate-pins.py and validate-manifest.py " + "pass on its rewrite") in row["reason"], row["reason"] + assert "gitlink_repairs_held_back" not in row["detail"], row["detail"] # What the gate said is kept whole in front of it, and the NEXT and the # rest of `detail` are the gate's own. assert row["reason"].startswith(gate["reason"] + ". "), row["reason"] @@ -246,6 +304,9 @@ def test_the_gate_names_the_same_two_commands(hand_moved): assert lines[first + 2].startswith( " Which commit stays is the person's call. From a checkout of " "openRepoShape,"), lines + assert "run either with `--dry-run` first" in lines[first + 2], lines + assert ("once this validator and validate-manifest.py pass on its " + "rewrite") in lines[first + 2], lines bump = (f" {PYTHON} scripts/bump-leg.py --root " f"{shlex.quote(str(root))} --leg spec --to") assert lines[first + 3] == f"{bump} {hand_moved['gitlink']}", lines @@ -264,14 +325,7 @@ def test_each_repair_run_as_printed_makes_the_doctor_compliant( row = pins_row(standard, root) line = row["detail"]["gitlink_repairs"][0][which] want = hand_moved["gitlink" if which == "keep_gitlink" else "pin"] - elsewhere = tmp_path / "elsewhere" - elsewhere.mkdir() - ran = subprocess.run(pasted(line), cwd=str(elsewhere), capture_output=True, - text=True, encoding="utf-8", errors="replace", - check=False, stdin=subprocess.DEVNULL, - env={**os.environ, **GIT_IDENTITY, - "PYTHONDONTWRITEBYTECODE": "1"}) - assert ran.returncode == 0, line + "\n" + ran.stdout + ran.stderr + run_as_printed(line, tmp_path) count = git("rev-list", "--count", f"{hand_moved['base']}..HEAD", cwd=root).stdout.strip() assert count == "1", count @@ -279,12 +333,7 @@ def test_each_repair_run_as_printed_makes_the_doctor_compliant( assert pin_of(root) == want assert git("symbolic-ref", "--short", "HEAD", cwd=root).stdout.strip() == WORKING - after = doctor(standard, root, "--json") - assert after.returncode == 0, after.stdout + after.stderr - rows = rows_of(after) - assert rows["pins"]["status"] == "ok", rows["pins"] - assert rows["legs"]["status"] == "ok", rows["legs"] - assert json.loads(after.stdout)["verdict"] == "COMPLIANT", after.stdout + assert_compliant(standard, root) def test_git_submodule_update_is_no_undo(standard, hand_moved): @@ -371,3 +420,212 @@ def test_every_other_pins_row_is_the_gates_own(standard, project, code): assert "bump-leg" not in row["reason"], row["reason"] gate = run_script(root / "scripts" / "validate-pins.py", cwd=root) assert "bump-leg.py" not in gate.stdout + gate.stderr + + +# --- named only where a line can commit ------------------------------------- +# +# `bump-leg.py` runs the root's whole `validate-pins.py`, and its +# `validate-manifest.py`, after its rewrite and before it commits, and rolls +# the bump back when either is red. A line that cannot commit is not named. + +def workflow_at(role: str, commit: str): + """A tracked workflow naming the `role` leg's repository at `commit`.""" + def write(root: Path) -> None: + workflows = root / ".github" / "workflows" + workflows.mkdir(parents=True, exist_ok=True) + (workflows / "legs.yml").write_text( + "on: [pull_request]\njobs:\n leg:\n uses: " + f"testorg/{PROJECT}-{role}/.github/workflows/x.yml@{commit}\n", + encoding="utf-8") + return write + + +def gate_says(root: Path) -> list[str]: + """The root's own gate's report, line by line, which exits 1.""" + gate = run_script(root / "scripts" / "validate-pins.py", cwd=root) + assert gate.returncode == 1, gate.stdout + gate.stderr + return gate.stderr.splitlines() + + +def assert_no_line_is_named(standard: Path, root: Path, row: dict) -> None: + """No `bump-leg.py` command in the row or its `detail`, and the NEXT is + the gate's own, byte for byte.""" + assert "gitlink_repairs" not in row["detail"], row["detail"] + assert "--to " not in row["reason"], row["reason"] + assert str(standard.resolve() / "scripts") not in row["reason"] + assert row["next"] == gate_row(standard, root)["next"], row["next"] + + +def test_two_legs_off_their_pins_name_no_line_and_say_what_comes_first( + standard, project, advanced, code_advanced, tmp_path): + """Both legs pulled and committed in one commit: `bump-leg.py` moves ONE + leg and runs the whole gate before it commits, so the other leg's + mismatch rolls either line back. The row and the gate name none and say + what comes first; done, the row names the lines for the leg that is + left, and one of them makes the root COMPLIANT.""" + moved = moved_by_hand(project, tmp_path, + {"spec": advanced, "code": code_advanced}) + root, pins = moved["root"], moved["pins"] + row = pins_row(standard, root) + assert_no_line_is_named(standard, root, row) + assert row["detail"]["gitlink_repairs_held_back"] == { + "legs": [{"role": "spec", "path": "spec", "gitlink": advanced, + "pin": pins["spec"]}, + {"role": "code", "path": "code", "gitlink": code_advanced, + "pin": pins["code"]}], + "findings": [], "manifest": False}, row["detail"] + held = ("No single bump-leg.py line can commit while spec and code are " + "both off their pins: bump-leg.py moves one leg and runs the " + "root's whole validate-pins.py before it commits, rolling the bump " + "back when it is red. First put one leg's gitlink back at its pin " + "(check that leg out at the pin's commit and commit its gitlink " + "in the root), or undo the commit that moved them; this row then " + "names the two bump-leg.py lines for the leg that is left") + assert row["reason"].endswith(". " + held), row["reason"] + lines = gate_says(root) + assert not [line for line in lines if "scripts/bump-leg.py" in line] + said = [line for line in lines if line.startswith( + " No single bump-leg.py line can commit while spec and code are " + "both off their pins: ")] + assert len(said) == 2, lines + assert "First put one leg's gitlink back at its pin" in said[0], said + # What comes first, done by hand: the spec leg back at its pin. + git("checkout", "-q", "--detach", pins["spec"], cwd=root / "spec") + git("add", "--", "spec", cwd=root) + git("commit", "-qm", "put the spec leg back", cwd=root) + repairs = pins_row(standard, root)["detail"]["gitlink_repairs"] + assert [leg["role"] for leg in repairs] == ["code"], repairs + run_as_printed(repairs[0]["keep_gitlink"], tmp_path) + assert_compliant(standard, root) + + +#: A finding the bump does NOT clear, committed with the gitlink: the edit, +#: and the code the gate reports for it. +HOLDS_BACK = { + "shape-copy-drift": copy_drifted, + "the other leg's workflow reference": workflow_at("code", OTHER_SHA), +} + + +@pytest.mark.parametrize("case", sorted(HOLDS_BACK)) +def test_another_finding_holds_the_lines_back(standard, project, advanced, + tmp_path, case): + """One leg off its pin, and a finding a bump of it does not clear: the + gate `bump-leg.py` runs is red after either line's rewrite, so the row + and the gate name none and say to clear that finding first.""" + code = ("pin-workflow-ref-mismatch" if "workflow" in case else case) + moved = moved_by_hand(project, tmp_path, {"spec": advanced}, + HOLDS_BACK[case]) + root = moved["root"] + row = pins_row(standard, root) + assert_no_line_is_named(standard, root, row) + assert row["detail"]["gitlink_repairs_held_back"] == { + "legs": [{"role": "spec", "path": "spec", "gitlink": advanced, + "pin": moved["pins"]["spec"]}], + "findings": [code], "manifest": False}, row["detail"] + assert row["reason"].endswith( + f". No single bump-leg.py line can commit while the gate also reports " + f"{code}: bump-leg.py moves one leg and runs the root's whole " + "validate-pins.py before it commits, rolling the bump back when it is " + "red. First clear that finding; this row then names the two " + "bump-leg.py lines for spec"), row["reason"] + lines = gate_says(root) + assert not [line for line in lines if "scripts/bump-leg.py" in line] + assert (f" No single bump-leg.py line can commit while this validator " + f"also reports {code}: bump-leg.py moves one leg and runs this " + "validator before it commits, rolling the bump back when it is " + "red. First clear that finding; this validator then names the " + "two bump-leg.py lines for that leg.") in lines, lines + + +def test_a_red_manifest_gate_holds_the_lines_back(standard, project, advanced, + tmp_path): + """`bump-leg.py` runs `validate-manifest.py` too, so a manifest it + reports is red rolls either line back: the row names none and sends the + reader to the `manifest` row first.""" + moved = moved_by_hand( + project, tmp_path, {"spec": advanced}, + lambda root: edit(root, "project.yaml", r"(?m)^elected_on: .*$", + 'elected_on: "someday"')) + root = moved["root"] + result = run_script(standard / DOCTOR, "--root", str(root), "--json") + rows = rows_of(result) + assert rows["manifest"]["status"] == "FINDING", rows["manifest"] + row = rows["pins"] + assert_no_line_is_named(standard, root, row) + assert row["detail"]["gitlink_repairs_held_back"]["manifest"] is True + assert row["reason"].endswith( + ". No single bump-leg.py line can commit while the root's " + "validate-manifest.py is red (the `manifest` row): bump-leg.py runs " + "it after validate-pins.py, before it commits, and rolls the bump " + "back when either is red. First clear that row's finding; this row " + "then names the two bump-leg.py lines for spec"), row["reason"] + + +#: A finding the bump DOES clear, committed with the gitlink, and the line +#: run to show it: the leg's own workflow reference, still at the pin, and +#: its pin's digest, written wrong. +CLEARED = { + "its workflow reference": ("keep_gitlink", "pin-workflow-ref-mismatch"), + "its pin's digest": ("keep_pin", "pin-digest-mismatch"), +} + + +@pytest.mark.parametrize("case", sorted(CLEARED)) +def test_a_finding_the_bump_clears_holds_nothing_back(standard, project, + advanced, tmp_path, + case): + """The bump rewrites the leg's pin digest and every workflow reference + to its repository, so neither holds a line back: both are named, and + the one run makes the root COMPLIANT.""" + which, code = CLEARED[case] + pinned = pin_of(project) + also = (workflow_at("spec", pinned) if "workflow" in case + else digest_moved) + moved = moved_by_hand(project, tmp_path, {"spec": advanced}, also) + root = moved["root"] + lines = gate_says(root) + assert f"FINDING {code}: " in "\n".join(lines), lines + assert len([line for line in lines if "scripts/bump-leg.py" in line]) == 2 + row = pins_row(standard, root) + assert "gitlink_repairs_held_back" not in row["detail"], row["detail"] + repairs = row["detail"]["gitlink_repairs"] + run_as_printed(repairs[0][which], tmp_path) + assert_compliant(standard, root) + + +def test_a_root_without_its_own_gate_is_answered_by_the_template_alone( + standard, hand_moved): + """`bump-leg.py` runs the root's own `validate-pins.py` and skips an + absent one, so what the standard's template copy reports does not say + what the bump's run will: the row is the template gate's, untouched, + and the template, run over that root, ends the finding at THE LOCKSTEP + RULE.""" + root = hand_moved["root"] + (root / "scripts" / "validate-pins.py").unlink() + row = pins_row(standard, root) + assert row["detail"]["own_copy"] is False, row["detail"] + assert row == gate_row(standard, root) + assert "gitlink_repairs" not in row["detail"], row["detail"] + assert "gitlink_repairs_held_back" not in row["detail"], row["detail"] + template = run_script( + standard / "templates" / "assembly-root" / "scripts" / + "validate-pins.py", "--root", str(root), cwd=root, + env={"PYTHONPATH": str(standard / "scripts")}) + assert template.returncode == 1, template.stdout + template.stderr + assert "FINDING pin-gitlink-mismatch: " in template.stderr + assert "bump-leg.py" not in template.stdout + template.stderr + + +def test_a_gate_that_crashed_names_no_repair(standard, hand_moved): + """The root's own gate cannot import its `repo_shape.py`, and exits 1 + on a traceback: `bump-leg.py` would run that gate and find it red too, + so the row is the crash, and names no line and holds none back.""" + root = hand_moved["root"] + (root / "scripts" / "repo_shape.py").unlink() + row = pins_row(standard, root) + assert row["detail"].get("crashed") is True, row["detail"] + assert row["detail"]["exit"] == 1, row["detail"] + assert "gitlink_repairs" not in row["detail"], row["detail"] + assert "gitlink_repairs_held_back" not in row["detail"], row["detail"] + assert "bump-leg" not in row["reason"], row["reason"]