From 441fedc591536d398bc9dc6016b81138e0dfeec8 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 11:13:33 +0000 Subject: [PATCH 1/6] update-shape.py `read_pin` refuses `update-pin-row`, and the pinned validate-pins.py and validate-family.py report `shape-pin-row`, when a shape pin's `files:` is not a list, so `files: 5`, `true` or `1.5` no longer ends `check`, `apply`, shape-doctor.py and the project's own validator in `TypeError: ... is not iterable` (#290) Every reader of `contracts/shape-pin.yaml` took `pin.get("files") or []` and looped over it. `or []` rescues the falsy values, which reach the `no files` refusal or finding; a truthy scalar was iterated. On main 71b58a1 a pin with `files: 5`, `files: true` or `files: 1.5` committed over a clean scaffold ended `update-shape.py check` and `apply`, `shape-doctor.py`, the root's `scripts/validate-pins.py` and a holder's `scripts/validate-family.py` in `TypeError: 'int' (or 'bool', 'float') object is not iterable`, exit 1. The doctor printed nothing on stdout at all, and its one job is to say what is wrong. Each of the four readers now asks for a list: - `update-shape.py` `read_pin` raises `update-pin-row` naming the pin and the value (`files:` is 5, not a list of `path:`/`sha256:` rows). It is reached from `prepare`, so `check`, `apply` and the doctor's shape-currency row all get the refusal; the doctor's row already carries a `read_pin` refusal by its code. The two `update-pin-row` raises now carry a remediation about the pin (put back the pin a tool last wrote, from the root's own history) instead of `repo_shape`'s default trailer, which is about submodules and fixes nothing in this file. - `templates/assembly-root/scripts/validate-pins.py` `_check_shape_pin` and `templates/family-root/scripts/validate-family.py` `_check_shape_pin_files` report one `shape-pin-row` finding for the whole key, exit 1, the id they already use for a row that is not a mapping. A string `files: abc` is now one finding, where it was one per character. - `shape-doctor.py` `pinned_paths` returns the empty set, as it does for a pin that is not a mapping. It is read only when an agent file is missing, so it was masked while `read_pin` crashed first; with `read_pin` fixed, a root with a scalar pin and no `AGENTS.md` reached it. The falsy values are unchanged: `files: 0`, `false`, `null` and `[]` keep `update-pin-no-files` and `shape-pin-no-files`. The two validators are pinned template copies (`COPIED_VERBATIM`, `FAMILY_COPIED_VERBATIM`). Nothing in this repository records their digest: a scaffold, an adoption and `family.py init` digest the bytes they copy, so a project cut from here records the new digest, and every project already cut reads the two files as `upstream-changed` in `update-shape.py check` until its owner runs `apply`. tests/test_shape_pin_files_not_a_list.py: 18 cases over `files: 5`, `true` and `1.5` through each reader (update-shape check and apply, the root's validate-pins.py, the doctor's shape-currency row and its text report, the doctor's agent-files row with no `AGENTS.md`, `pinned_paths` in process, and a holder's validate-family.py, update-shape check and doctor). All 18 fail on 71b58a1 with the TypeError. Two controls for `files: 0` and `false` pass before and after. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Opus 5.5 --- shape-doctor.py | 8 +- .../assembly-root/scripts/validate-pins.py | 8 + .../family-root/scripts/validate-family.py | 8 + tests/test_shape_pin_files_not_a_list.py | 260 ++++++++++++++++++ update-shape.py | 31 ++- 5 files changed, 312 insertions(+), 3 deletions(-) create mode 100644 tests/test_shape_pin_files_not_a_list.py diff --git a/shape-doctor.py b/shape-doctor.py index a7648923..36e8a6b2 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -2934,7 +2934,13 @@ def pinned_paths(root: Path) -> set: pin = Context._read(root / "contracts" / "shape-pin.yaml") if not isinstance(pin, dict): return set() - return {str(row.get("path")) for row in (pin.get("files") or []) + # A `files:` that is not a list pins nothing, as a pin that is not a + # mapping pins nothing (#290): `files: 5` was iterated, a TypeError. The + # shape-currency row is where that pin is reported. + rows = pin.get("files") + if not isinstance(rows, list): + return set() + return {str(row.get("path")) for row in rows if isinstance(row, dict) and row.get("path")} diff --git a/templates/assembly-root/scripts/validate-pins.py b/templates/assembly-root/scripts/validate-pins.py index bcc43bd4..6bebfa4c 100755 --- a/templates/assembly-root/scripts/validate-pins.py +++ b/templates/assembly-root/scripts/validate-pins.py @@ -339,6 +339,14 @@ def _check_shape_pin(root: Path, manifest: dict, report: Report) -> None: pin, rel = _check_shape_pin_header(root, manifest, report) before = len(report.findings) files = pin.get("files") or [] + if not isinstance(files, list): + # Asked, not assumed (#290): `files: 5` was iterated, and the run + # ended in a TypeError with no finding naming the pin. + report.finding("shape-pin-row", + f"{rel}: `files:` is {files!r}, not a list of " + "`path:`/`sha256:` rows, so nothing about the copied " + "shape files is actually asserted") + return if not files: report.finding("shape-pin-no-files", f"{rel}: no `files:` rows, so nothing about the copied " diff --git a/templates/family-root/scripts/validate-family.py b/templates/family-root/scripts/validate-family.py index 0991ba1b..9224486a 100755 --- a/templates/family-root/scripts/validate-family.py +++ b/templates/family-root/scripts/validate-family.py @@ -499,6 +499,14 @@ def _check_shape_pin_files(root: Path, pin: dict, rel: str, """ before = len(report.findings) files = pin.get("files") or [] + if not isinstance(files, list): + # Asked, not assumed (#290): `files: 5` was iterated, and the run + # ended in a TypeError with no finding naming the pin. + report.finding("shape-pin-row", + f"{rel}: `files:` is {files!r}, not a list of " + "`path:`/`sha256:` rows, so nothing about the copied " + "shape files is actually asserted") + return if not files: report.finding("shape-pin-no-files", f"{rel}: no `files:` rows, so nothing about the copied " diff --git a/tests/test_shape_pin_files_not_a_list.py b/tests/test_shape_pin_files_not_a_list.py new file mode 100644 index 00000000..85176152 --- /dev/null +++ b/tests/test_shape_pin_files_not_a_list.py @@ -0,0 +1,260 @@ +# SPDX-License-Identifier: Apache-2.0 +"""A shape pin whose `files:` is not a list is reported, not iterated (#290). + +Four readers took `pin.get("files") or []` and looped over it: `read_pin` in +`update-shape.py` (so `check`, `apply` and the doctor's shape-currency row), +`_check_shape_pin` in the assembly root's `scripts/validate-pins.py`, +`_check_shape_pin_files` in the holder's `scripts/validate-family.py`, and +`pinned_paths` in `shape-doctor.py`. `or []` rescues the falsy values, which +reach the `no files` refusal or finding. A TRUTHY scalar was iterated: +`files: 5`, `files: true` and `files: 1.5` ended every one of them in +`TypeError: 'int' object is not iterable` (or `'bool'`, `'float'`), exit 1, +and the doctor, whose one job is to say what is wrong, printed nothing at all. + +Each case below commits one such pin over a clean scaffold or a clean holder +and runs a reader on it. On the code before #290 each one is a traceback. + +The doctor's VERDICT for a root pin `read_pin` refuses is #237's business +(PR #267 moves it from CANNOT ANSWER, exit 3, to INVALID, exit 1), so the +doctor's cases assert the ROW and that a verdict was printed at all, never +which one. + +NOTHING HERE REACHES A NETWORK: the scaffold and the holder use +`--local-remote-dir`, and `read_pin` refuses before `--upstream` is opened. +""" + +from __future__ import annotations + +import importlib.util +import json +import re +import shutil +import sys + +import pytest + +from conftest import REPO, git, run_script + +sys.path.insert(0, str(REPO / "scripts")) +from repo_shape import load_yaml # noqa: E402 + +UPDATE = REPO / "update-shape.py" +DOCTOR = REPO / "shape-doctor.py" +FAMILY = REPO / "scripts" / "family.py" +SHAPE_PIN = "contracts/shape-pin.yaml" +HOLDER = "Fam" + +#: `(what follows `files:` in the pin, how the reader shows the value back)`. +#: Python's `repr`, because each reader names the value with `!r` the way it +#: already names a `commit:` or a `revision_kind:` it cannot use. +NOT_A_LIST = [ + pytest.param("5", "5", id="files-5"), + pytest.param("true", "True", id="files-true"), + pytest.param("1.5", "1.5", id="files-1.5"), +] + +#: Every verdict the doctor's text report can end on. +VERDICT = re.compile(r"^(COMPLIANT|DRIFTED|MISPLACED|INVALID|NOT A SHAPE ROOT" + r"|CANNOT ANSWER)\b") + + +def scalar_files(root, value: str) -> None: + """Replace the pin's `files:` rows by `files: ` and commit it. + + `files:` is the pin's last key, so the rest of the pin is untouched. + BYTES, never `write_text`, so the file stays LF on every OS. Committed, + as in the issue's reproduction: the pin is wrong in the repository, not + only in a working tree. + """ + pin = root / SHAPE_PIN + text = pin.read_bytes().decode("utf-8") + cut = text.index("\nfiles:\n") + 1 + pin.write_bytes((text[:cut] + f"files: {value}\n").encode("utf-8")) + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", + "-m", f"files: {value}", "--", SHAPE_PIN, cwd=root) + assert not isinstance(load_yaml(pin)["files"], list), \ + "fixture drift: the pin still carries a list of rows" + + +def no_traceback(result) -> None: + assert "Traceback" not in result.stderr, result.stderr + assert "is not iterable" not in result.stderr, result.stderr + + +def rows_of(result) -> dict: + return {row["id"]: row for row in json.loads(result.stdout)["rows"]} + + +def findings(result, code: str) -> list[str]: + return [line for line in result.stderr.splitlines() + if line.startswith(f"FINDING {code}:")] + + +def assert_refused_update_pin_row(result, shown: str) -> None: + """`read_pin`'s refusal: by name, exit 2, the value shown, and a remedy + about the pin -- not `repo_shape`'s default one about submodules.""" + no_traceback(result) + assert result.returncode == 2, result.stdout + result.stderr + assert "REFUSED update-pin-row: " in result.stderr, result.stderr + assert f"`files:` is {shown}, not a list" in result.stderr + assert "git log -- contracts/shape-pin.yaml" in result.stderr + assert "submodule" not in result.stderr + + +def assert_one_shape_pin_row_finding(result, shown: str) -> None: + """A validator's finding, once, for the whole `files:` -- exit 1.""" + no_traceback(result) + assert result.returncode == 1, result.stdout + result.stderr + rows = findings(result, "shape-pin-row") + assert len(rows) == 1, result.stderr + assert f"`files:` is {shown}, not a list" in rows[0], rows[0] + + +def assert_shape_currency_row(result, shown: str) -> dict: + """The doctor printed a report, and its shape-currency row names the + refusal `read_pin` raised for the pin.""" + no_traceback(result) + assert result.returncode in (1, 3), result.stdout + result.stderr + rows = rows_of(result) + row = rows["shape-currency"] + assert row["status"] == "FINDING", row + assert row["detail"]["error"] == "update-pin-row", row["detail"] + assert f"`files:` is {shown}, not a list" in row["reason"], row["reason"] + return rows + + +# --- an assembly root ------------------------------------------------------ + +@pytest.mark.parametrize("value, shown", NOT_A_LIST) +def test_update_shape_check_and_apply_refuse_it_by_name(project, value, + shown): + scalar_files(project, value) + before = (project / SHAPE_PIN).read_bytes() + checked = run_script(UPDATE, "check", "--root", str(project), + "--upstream", str(REPO)) + assert_refused_update_pin_row(checked, shown) + applied = run_script(UPDATE, "apply", "--root", str(project), "--yes", + "--upstream", str(REPO)) + assert_refused_update_pin_row(applied, shown) + assert (project / SHAPE_PIN).read_bytes() == before, \ + "a refused apply writes nothing" + + +@pytest.mark.parametrize("value, shown", NOT_A_LIST) +def test_the_projects_validate_pins_reports_it_once(project, value, shown): + scalar_files(project, value) + result = run_script(project / "scripts" / "validate-pins.py", + cwd=project) + assert_one_shape_pin_row_finding(result, shown) + + +@pytest.mark.parametrize("value, shown", NOT_A_LIST) +def test_the_doctor_reports_it_in_the_shape_currency_row(project, value, + shown): + scalar_files(project, value) + rows = assert_shape_currency_row( + run_script(DOCTOR, "--root", str(project), "--json"), shown) + # The project's own validator, run by the `pins` row, says the same. + assert rows["pins"]["status"] == "FINDING", rows["pins"] + assert "shape-pin-row" in rows["pins"]["reason"], rows["pins"] + plain = run_script(DOCTOR, "--root", str(project)) + no_traceback(plain) + assert f"`files:` is {shown}, not a list" in plain.stdout, plain.stdout + last = [line for line in plain.stdout.splitlines() if line.strip()][-1] + assert VERDICT.match(last), plain.stdout + assert not last.startswith("COMPLIANT"), last + + +@pytest.mark.parametrize("value, shown", NOT_A_LIST) +def test_the_doctor_reaches_the_agent_files_row_past_it(project, value, + shown): + """`pinned_paths` is read only when an agent file is missing, so the root + under test has no `AGENTS.md`: the row is reached, and the pin -- which + names no file -- names none of the absent ones.""" + scalar_files(project, value) + (project / "AGENTS.md").unlink() + rows = assert_shape_currency_row( + run_script(DOCTOR, "--root", str(project), "--json"), shown) + agent = rows["agent-files"] + assert agent["detail"]["pinned"] == [], agent + assert agent["reason"] == "absent: AGENTS.md", agent + + +@pytest.fixture(scope="module") +def doctor_module(): + spec = importlib.util.spec_from_file_location("shape_doctor_pin_files", + DOCTOR) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +@pytest.mark.parametrize("value, shown", NOT_A_LIST) +def test_pinned_paths_of_it_is_the_empty_set(doctor_module, project, value, + shown): + scalar_files(project, value) + assert doctor_module.pinned_paths(project) == set() + + +# --- a family holder ------------------------------------------------------- + +@pytest.fixture(scope="module") +def initialised_holder(tmp_path_factory): + """One `family.py init`, no members: the holder's pin and its validator + are all these cases need, and the validator passes it as it stands.""" + base = tmp_path_factory.mktemp("pin-files-holder") + created = run_script( + FAMILY, "init", "--org", "testorg", "--family", HOLDER, + "--created-by", "Test Human", "--created-on", "2026-09-04", + "--local-remote-dir", str(base / "remotes"), + "--work-dir", str(base / "fam")) + assert created.returncode == 0, created.stderr + created.stdout + root = base / "fam" / HOLDER + clean = run_script(root / "scripts" / "validate-family.py", cwd=root) + assert clean.returncode == 0, clean.stderr + clean.stdout + return root + + +@pytest.fixture +def holder(initialised_holder, tmp_path): + target = tmp_path / HOLDER + shutil.copytree(initialised_holder, target, symlinks=True) + return target + + +@pytest.mark.parametrize("value, shown", NOT_A_LIST) +def test_the_holders_validate_family_and_update_shape_and_doctor(holder, + value, + shown): + scalar_files(holder, value) + assert_one_shape_pin_row_finding( + run_script(holder / "scripts" / "validate-family.py", cwd=holder), + shown) + assert_refused_update_pin_row( + run_script(UPDATE, "check", "--root", str(holder), + "--upstream", str(REPO)), shown) + rows = assert_shape_currency_row( + run_script(DOCTOR, "--root", str(holder), "--json"), shown) + assert rows["family"]["status"] == "FINDING", rows["family"] + assert "shape-pin-row" in rows["family"]["reason"], rows["family"] + + +# --- the control: a falsy `files:` keeps the refusal it had ---------------- + +@pytest.mark.parametrize("value", ["0", "false"]) +def test_a_falsy_files_is_still_no_files(project, value): + """NOT A #290 CASE, and it passes before #290 too. `or []` turns `0` and + `false` into no rows, and they keep `update-pin-no-files` and + `shape-pin-no-files`: the new guard is for what `or []` let through.""" + scalar_files(project, value) + checked = run_script(UPDATE, "check", "--root", str(project), + "--upstream", str(REPO)) + no_traceback(checked) + assert checked.returncode == 2 + assert "REFUSED update-pin-no-files: " in checked.stderr + validated = run_script(project / "scripts" / "validate-pins.py", + cwd=project) + no_traceback(validated) + assert validated.returncode == 1 + assert len(findings(validated, "shape-pin-no-files")) == 1 + assert not findings(validated, "shape-pin-row") diff --git a/update-shape.py b/update-shape.py index f9aeeed7..f8da1e29 100755 --- a/update-shape.py +++ b/update-shape.py @@ -382,6 +382,19 @@ def is_conflict(self) -> bool: return self.state in CONFLICTS +#: What `update-pin-row` tells the person whose pin's `files:` does not read +#: as rows (#290). Without it the refusal carried `repo_shape`'s default +#: trailer, which is about submodules and the lockstep invariant and fixes +#: nothing in this file. +PIN_ROWS_REMEDIATION = ( + "Remediation: `files:` is a list with one `path:`/`sha256:` row per " + "copied file. The tool that cut this root wrote it and only " + "`update-shape.py apply` moves it, so it is never mended by hand: put " + "back the pin a tool last wrote from this repository's own history " + "(`git log -- contracts/shape-pin.yaml` names the commit that changed " + "it), then run this again.") + + def read_pin(root: Path) -> tuple[dict, list[Row]]: pin_path = root / "contracts" / "shape-pin.yaml" if not pin_path.is_file(): @@ -412,11 +425,25 @@ def read_pin(root: Path) -> tuple[dict, list[Row]]: "meaning nobody stated.", "Remediation: a pin written under another definition is updated " "by whatever wrote it.") + # A LIST IS ASKED FOR, NOT ASSUMED (#290). `or []` rescues only the falsy + # values -- `files: 0`, `false`, `null`, `[]` reach `update-pin-no-files` + # below -- and a truthy scalar was iterated: `files: 5`, `true` or `1.5` + # ended `check`, `apply` and `shape-doctor.py` in a `TypeError`, exit 1, + # with nothing naming the pin. A string or a mapping is refused here too, + # once, rather than one character or one key at a time. + files = pin.get("files") or [] + if not isinstance(files, list): + raise Refusal( + "update-pin-row", + f"{pin_path}: `files:` is {files!r}, not a list of " + "`path:`/`sha256:` rows, so no copied file can be read from it", + PIN_ROWS_REMEDIATION) rows: list[Row] = [] - for raw in pin.get("files") or []: + for raw in files: if not isinstance(raw, dict) or not raw.get("path"): raise Refusal("update-pin-row", - f"{pin_path}: a `files:` row is not `path:`/`sha256:`") + f"{pin_path}: a `files:` row is not `path:`/`sha256:`", + PIN_ROWS_REMEDIATION) rows.append(Row(str(raw["path"]), str(raw.get("sha256") or ""))) if not rows: raise Refusal( From 6e2689acb8bafb93bbaa6c93b7b6215d6b4c7705 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 11:47:27 +0000 Subject: [PATCH 2/6] update-shape.py: `read_pin`'s #290 comment says what a string or a mapping `files:` gets, so it no longer claims the old code refused one character at a time (#290) Before #290, `read_pin` refused a string or a mapping `files:` at its first character or key, as a row that is not `path:`/`sha256:`; it was the two validators that reported one finding per character. The comment now says what changed for `read_pin` itself: the value is named for what it is. Comment only. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Opus 5.5 --- update-shape.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/update-shape.py b/update-shape.py index f8da1e29..95986b9b 100755 --- a/update-shape.py +++ b/update-shape.py @@ -429,8 +429,8 @@ def read_pin(root: Path) -> tuple[dict, list[Row]]: # values -- `files: 0`, `false`, `null`, `[]` reach `update-pin-no-files` # below -- and a truthy scalar was iterated: `files: 5`, `true` or `1.5` # ended `check`, `apply` and `shape-doctor.py` in a `TypeError`, exit 1, - # with nothing naming the pin. A string or a mapping is refused here too, - # once, rather than one character or one key at a time. + # with nothing naming the pin. A string or a mapping lands here too, and + # is named for what it is rather than as its first character or key. files = pin.get("files") or [] if not isinstance(files, list): raise Refusal( From 039530ba735b413f8996268c9846b640decc8455 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 12:25:45 +0000 Subject: [PATCH 3/6] update-shape.py: the `update-pin-row` remediation spells its `git log` as `git -C log`, so run from the openRepoShape checkout with `--root ` it shows the project's pin history and not the checkout's (#290) PIN_ROWS_REMEDIATION was a constant that told the person to run a bare `git log -- contracts/shape-pin.yaml`. This tool is run from a checkout of openRepoShape with `--root /path/to/project`, so that command inspects the checkout, which has no such pin, and shows no history (or the wrong one): the person cannot follow the remediation. Codex review comment on PR #343, https://github.com/opensoft/openRepoShape/pull/343#discussion_r4247719793. The remediation is now `pin_rows_remediation(root)`, and both `update-pin-row` raises in `read_pin` pass the root they were given. It spells the command with `_by_hand`, this module's one quoter for a line a person pastes (`shlex.join(["git", "-C", root, ...])`), so a root holding a space is one argument. The rest of the text is unchanged. tests/test_shape_pin_files_not_a_list.py asserts the qualified command, computed with `shlex.join` rather than written as a literal so it holds on Windows, where a backslash path is quoted. A new test runs the tool from the checkout against a root whose path holds a space, takes the printed command out of stderr, runs it from the checkout, and requires the project's own commits to the pin. A second new test covers the other raise in `read_pin` (a `files:` list whose row is not a mapping), which no test reached before. At 6e2689a 8 of the file's 22 tests fail on the missing `git -C `; dropping the `-C ` pair from the command fails them again. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Sonnet 5.5 --- tests/test_shape_pin_files_not_a_list.py | 109 ++++++++++++++++++++--- update-shape.py | 37 +++++--- 2 files changed, 122 insertions(+), 24 deletions(-) diff --git a/tests/test_shape_pin_files_not_a_list.py b/tests/test_shape_pin_files_not_a_list.py index 85176152..4d95d3d4 100644 --- a/tests/test_shape_pin_files_not_a_list.py +++ b/tests/test_shape_pin_files_not_a_list.py @@ -28,6 +28,7 @@ import importlib.util import json import re +import shlex import shutil import sys @@ -58,8 +59,8 @@ r"|CANNOT ANSWER)\b") -def scalar_files(root, value: str) -> None: - """Replace the pin's `files:` rows by `files: ` and commit it. +def commit_files(root, files_text: str, message: str) -> None: + """Replace the pin's `files:` rows by `files_text` and commit it. `files:` is the pin's last key, so the rest of the pin is untouched. BYTES, never `write_text`, so the file stays LF on every OS. Committed, @@ -69,13 +70,31 @@ def scalar_files(root, value: str) -> None: pin = root / SHAPE_PIN text = pin.read_bytes().decode("utf-8") cut = text.index("\nfiles:\n") + 1 - pin.write_bytes((text[:cut] + f"files: {value}\n").encode("utf-8")) + pin.write_bytes((text[:cut] + files_text).encode("utf-8")) git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", - "-m", f"files: {value}", "--", SHAPE_PIN, cwd=root) - assert not isinstance(load_yaml(pin)["files"], list), \ + "-m", message, "--", SHAPE_PIN, cwd=root) + + +def scalar_files(root, value: str) -> None: + """`files: `, committed: a pin whose `files:` is not a list.""" + commit_files(root, f"files: {value}\n", f"files: {value}") + assert not isinstance(load_yaml(root / SHAPE_PIN)["files"], list), \ "fixture drift: the pin still carries a list of rows" +def pin_log_command(root) -> str: + """The remediation's `git log`, as `update-shape.py` spells it for `root`. + + COMPUTED here with `shlex.join`, the quoter `update-shape.py`'s + `_by_hand` uses, and not written as a literal: a root holding a space or, + on Windows, a backslash is quoted, so a literal would pass on one OS and + fail on the next. The root is `resolve()`d because the tool resolves its + `--root` (a Windows temp directory can arrive as an 8.3 short name). + """ + return shlex.join(["git", "-C", str(root.resolve()), "log", "--", + SHAPE_PIN]) + + def no_traceback(result) -> None: assert "Traceback" not in result.stderr, result.stderr assert "is not iterable" not in result.stderr, result.stderr @@ -90,14 +109,18 @@ def findings(result, code: str) -> list[str]: if line.startswith(f"FINDING {code}:")] -def assert_refused_update_pin_row(result, shown: str) -> None: +def assert_refused_update_pin_row(result, shown: str, root) -> None: """`read_pin`'s refusal: by name, exit 2, the value shown, and a remedy - about the pin -- not `repo_shape`'s default one about submodules.""" + about the pin -- not `repo_shape`'s default one about submodules. The + `git log` it points at is `git -C `, never the bare one: this tool + is run from the openRepoShape checkout with `--root `, so a bare + `git log` would read the checkout's history, not the project's.""" no_traceback(result) assert result.returncode == 2, result.stdout + result.stderr assert "REFUSED update-pin-row: " in result.stderr, result.stderr assert f"`files:` is {shown}, not a list" in result.stderr - assert "git log -- contracts/shape-pin.yaml" in result.stderr + assert pin_log_command(root) in result.stderr, result.stderr + assert "git log" not in result.stderr, result.stderr assert "submodule" not in result.stderr @@ -132,14 +155,78 @@ def test_update_shape_check_and_apply_refuse_it_by_name(project, value, before = (project / SHAPE_PIN).read_bytes() checked = run_script(UPDATE, "check", "--root", str(project), "--upstream", str(REPO)) - assert_refused_update_pin_row(checked, shown) + assert_refused_update_pin_row(checked, shown, project) applied = run_script(UPDATE, "apply", "--root", str(project), "--yes", "--upstream", str(REPO)) - assert_refused_update_pin_row(applied, shown) + assert_refused_update_pin_row(applied, shown, project) assert (project / SHAPE_PIN).read_bytes() == before, \ "a refused apply writes nothing" +PIN_LOG_SAID = re.compile(r"`(git -C [^`]+)` names the commit that changed") + + +def printed_pin_log(result) -> str: + """The `git log` the remediation prints, exactly as it is printed.""" + said = PIN_LOG_SAID.search(result.stderr) + assert said, result.stderr + return said.group(1) + + +def test_the_remediations_git_log_reads_the_root_it_was_run_on(project, + tmp_path): + """THE CWD IS NOT THE ROOT, and the root holds a space. + + The documented way to run this tool is from the openRepoShape checkout, + `update-shape.py check --root /path/to/project`. A bare `git log -- + contracts/shape-pin.yaml` printed for that run inspects the CHECKOUT and + shows no pin history, so the person cannot follow the remediation + (codex, PR #343). The printed command is run here, from the checkout, and + must show the project's own commits to the pin. + """ + spaced = tmp_path / "a root with space" / "Atlas" + spaced.parent.mkdir() + shutil.copytree(project, spaced, symlinks=True) + scalar_files(spaced, "5") + checked = run_script(UPDATE, "check", "--root", str(spaced), + "--upstream", str(REPO), cwd=REPO) + assert_refused_update_pin_row(checked, "5", spaced) + command = printed_pin_log(checked) + assert command == pin_log_command(spaced), command + # The space did not split the root into two words. + assert shlex.split(command) == ["git", "-C", str(spaced.resolve()), "log", + "--", SHAPE_PIN], command + + shown = git(*shlex.split(command)[1:], cwd=REPO).stdout + history = git("log", "--format=%H", "--", SHAPE_PIN, cwd=spaced) + commits = history.stdout.split() + assert len(commits) >= 2, history.stdout + for sha in commits: + assert sha in shown, f"{sha} is a commit to the pin, not shown:\n{shown}" + assert git("rev-parse", "HEAD", cwd=spaced).stdout.strip() in shown + + +def test_a_row_that_is_not_a_mapping_carries_the_same_remediation(project, + tmp_path): + """`read_pin` raises `update-pin-row` twice, and both carry the remedy: + `files:` is a list here, but its one row is `5`, not `path:`/`sha256:`.""" + spaced = tmp_path / "a root with space" / "Atlas" + spaced.parent.mkdir() + shutil.copytree(project, spaced, symlinks=True) + commit_files(spaced, "files:\n - 5\n", "files: - 5") + assert load_yaml(spaced / SHAPE_PIN)["files"] == [5], \ + "fixture drift: the pin does not carry one scalar row" + checked = run_script(UPDATE, "check", "--root", str(spaced), + "--upstream", str(REPO), cwd=REPO) + no_traceback(checked) + assert checked.returncode == 2, checked.stdout + checked.stderr + assert "REFUSED update-pin-row: " in checked.stderr, checked.stderr + assert "a `files:` row is not `path:`/`sha256:`" in checked.stderr + assert printed_pin_log(checked) == pin_log_command(spaced) + assert "git log" not in checked.stderr, checked.stderr + assert "submodule" not in checked.stderr + + @pytest.mark.parametrize("value, shown", NOT_A_LIST) def test_the_projects_validate_pins_reports_it_once(project, value, shown): scalar_files(project, value) @@ -232,7 +319,7 @@ def test_the_holders_validate_family_and_update_shape_and_doctor(holder, shown) assert_refused_update_pin_row( run_script(UPDATE, "check", "--root", str(holder), - "--upstream", str(REPO)), shown) + "--upstream", str(REPO)), shown, holder) rows = assert_shape_currency_row( run_script(DOCTOR, "--root", str(holder), "--json"), shown) assert rows["family"]["status"] == "FINDING", rows["family"] diff --git a/update-shape.py b/update-shape.py index 95986b9b..94997f67 100755 --- a/update-shape.py +++ b/update-shape.py @@ -382,17 +382,28 @@ def is_conflict(self) -> bool: return self.state in CONFLICTS -#: What `update-pin-row` tells the person whose pin's `files:` does not read -#: as rows (#290). Without it the refusal carried `repo_shape`'s default -#: trailer, which is about submodules and the lockstep invariant and fixes -#: nothing in this file. -PIN_ROWS_REMEDIATION = ( - "Remediation: `files:` is a list with one `path:`/`sha256:` row per " - "copied file. The tool that cut this root wrote it and only " - "`update-shape.py apply` moves it, so it is never mended by hand: put " - "back the pin a tool last wrote from this repository's own history " - "(`git log -- contracts/shape-pin.yaml` names the commit that changed " - "it), then run this again.") +def pin_rows_remediation(root: Path) -> str: + """What `update-pin-row` tells the person whose pin's `files:` does not + read as rows (#290). Without it the refusal carried `repo_shape`'s + default trailer, which is about submodules and the lockstep invariant and + fixes nothing in this file. + + A FUNCTION OF THE ROOT, not a constant, because the `git log` it points + at has to read the PROJECT's history. This tool is run from a checkout of + openRepoShape with `--root /path/to/project`, so a bare `git log -- + contracts/shape-pin.yaml` inspects the checkout instead and shows no pin + history, or the wrong one (codex, PR #343). It is spelled `git -C ` + by `_by_hand`, this module's one quoter for a line a person pastes, so a + root holding a space is one argument there. + """ + log = _by_hand(root, "log", "--", "contracts/shape-pin.yaml") + return ( + "Remediation: `files:` is a list with one `path:`/`sha256:` row per " + "copied file. The tool that cut this root wrote it and only " + "`update-shape.py apply` moves it, so it is never mended by hand: " + "put back the pin a tool last wrote from this repository's own " + f"history (`{log}` names the commit that changed it), then run this " + "again.") def read_pin(root: Path) -> tuple[dict, list[Row]]: @@ -437,13 +448,13 @@ def read_pin(root: Path) -> tuple[dict, list[Row]]: "update-pin-row", f"{pin_path}: `files:` is {files!r}, not a list of " "`path:`/`sha256:` rows, so no copied file can be read from it", - PIN_ROWS_REMEDIATION) + pin_rows_remediation(root)) rows: list[Row] = [] for raw in files: if not isinstance(raw, dict) or not raw.get("path"): raise Refusal("update-pin-row", f"{pin_path}: a `files:` row is not `path:`/`sha256:`", - PIN_ROWS_REMEDIATION) + pin_rows_remediation(root)) rows.append(Row(str(raw["path"]), str(raw.get("sha256") or ""))) if not rows: raise Refusal( From 6eb8e4e577f5ae1dc34dd82817e03f1386b16128 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 15:45:53 +0000 Subject: [PATCH 4/6] templates/assembly-root/scripts/validate-pins.py: a workflow file under `.github/workflows/` that is not UTF-8, or that this user cannot open, is ONE `workflow-unreadable` refusal naming every such file, exit 2, so the project's pins gate no longer ends in a `UnicodeDecodeError` traceback, exit 1, with no line naming the file (#359) `_workflow_refs` read every `.yml` and `.yaml` file under `.github/workflows/` with `path.read_text(encoding="utf-8")` and nothing on the path caught the failure. `UnicodeDecodeError` is a `ValueError` and `main()` handles `Refusal` alone, so a committed workflow holding a Latin-1 `# caf\xe9` comment, or saved as UTF-16 with a byte-order mark, ended `python3 scripts/validate-pins.py` in a traceback with exit 1 -- the code this validator uses for a finding -- and a file mode 000 ended it in a `PermissionError` from the same line. A pin that is not UTF-8 is a clean `REFUSED yaml-unreadable`, exit 2, because it goes through `load_yaml`; the workflow read was the one read in the script that did not. The file is opened before it is known to name a leg, so a UTF-16 workflow naming none stopped the run too. This is the project's own gate: `make validate`, and the pins step of the `validate` workflow. `_workflow_refs` now reads every workflow file through `_read_workflow`, collects the ones it cannot read and raises one `Refusal` `workflow-unreadable` (exit 2) after the last one: - each file is named relative to the root, in POSIX spelling and never by its absolute path, in the words `load_yaml` uses: `: not UTF-8 at byte N ()`; a file this user cannot open is `: cannot be read ()`. The first file follows the id on the `REFUSED` line, so `shape-doctor.py`'s `pins` row, which quotes that line, names it; the rest follow, one to a line, so the operator repairs them in one pass instead of meeting them one run at a time. - the remediation says to re-save each file as UTF-8 (a UTF-16 file needs converting, not editing), commit it and run the same command again, or, for a file that cannot be opened, to restore read access. - a file is REFUSED, never skipped and never read lossily: a skipped workflow could hide a `@` reference that is out of lockstep, which is what the gate exists to find, and `errors="replace"` would call green a tree that the project's other YAML readers, and `bump-leg.py` (#289), refuse. THIS IS A PINNED TEMPLATE COPY. `COPIED_VERBATIM` names it, so a scaffold, an adoption and an `update-shape.py apply` record the sha256 of the bytes they copy in the project's own `contracts/shape-pin.yaml`. Nothing in this repository records that digest (`git grep -E '[0-9a-f]{64}'` finds only the actionlint download sum in `.github/workflows/tests.yml`), so there is no pin here to move: a project cut from this commit records the new digest, and every project already cut reads `scripts/validate-pins.py` as `upstream-changed` in `update-shape.py check` until its owner runs `apply`, which rewrites that one row. Stacked on #343, whose commit moved this file's digest for the `files:` change: the two changes now move it once between them. `templates/family-root/scripts/validate-family.py` has no workflow reader (its only reads are `load_yaml`, which already refuses a non-UTF-8 file by name), so the holder's copy and its digest are unchanged. tests/test_validate_pins_unreadable_workflow.py: 10 cases over a scaffold's own `scripts/validate-pins.py` -- a Latin-1 comment, a UTF-16 file naming no leg, two files named in one refusal (`.yml` and `.yaml`), a stale reference in a non-UTF-8 file (refused, not skipped, not read lossily), a mode-000 file (skipped on Windows and as root), and the doctor's `pins` row -- and four controls (`yaml-unreadable` for a Latin-1 pin, a UTF-8 comment, a UTF-8 byte-order mark, a Latin-1 `.txt` that is not a workflow). All six refusal cases fail on 039530b with the traceback; the four controls pass before and after. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Sonnet 5.5 --- .../assembly-root/scripts/validate-pins.py | 90 ++++++- .../test_validate_pins_unreadable_workflow.py | 238 ++++++++++++++++++ 2 files changed, 325 insertions(+), 3 deletions(-) create mode 100644 tests/test_validate_pins_unreadable_workflow.py diff --git a/templates/assembly-root/scripts/validate-pins.py b/templates/assembly-root/scripts/validate-pins.py index 6bebfa4c..651b7800 100755 --- a/templates/assembly-root/scripts/validate-pins.py +++ b/templates/assembly-root/scripts/validate-pins.py @@ -42,7 +42,8 @@ recheckable neutral-product pin disagrees 1 a FINDING: drift a reviewer can act on 2 a REFUSAL: the question could not be asked — an uninitialized submodule, - an unresolvable commit, an unreadable pin. An unanswerable pin question + an unresolvable commit, an unreadable pin or workflow file (one that is + not UTF-8, or that this user cannot open). An unanswerable pin question is never an implicit pass. """ @@ -53,6 +54,7 @@ import re import sys from pathlib import Path +from typing import NamedTuple sys.path.insert(0, str(Path(__file__).resolve().parent)) from repo_shape import ( # noqa: E402 @@ -99,17 +101,99 @@ def skip(self, text: str) -> None: self.notes.append(f" skip {text}") +class _Unreadable(NamedTuple): + """One workflow file that could not be read (#359): the line that names it, + and whether it failed on its ENCODING (`True`) or on being opened.""" + + line: str + encoding: bool + + +def _read_workflow(root: Path, path: Path) -> tuple[str, _Unreadable | None]: + """`(text, None)` for a workflow file read as UTF-8, or `("", why)` for one + that cannot be (#359). + + The line is `: not UTF-8 at byte N ()`, in + the words `repo_shape.load_yaml` gives the same failure (#192), so a file + this script cannot read is named the way its other readers name one. + POSIX, on every platform, and never the absolute path: the reader pastes + it into git and into an editor that opens it from the root. + A file this user cannot open (`OSError`) is the same line with the + operating system's own reason, because it is the same unguarded read. + """ + rel = path.relative_to(root).as_posix() + try: + return path.read_text(encoding="utf-8"), None + except UnicodeDecodeError as exc: + return "", _Unreadable( + f"{rel}: not UTF-8 at byte {exc.start} ({exc.reason})", True) + except OSError as exc: + return "", _Unreadable( + f"{rel}: cannot be read ({exc.strerror or type(exc).__name__})", + False) + + +def _workflow_unreadable(files: list[_Unreadable]) -> Refusal: + """The ONE refusal for every workflow file that could not be read (#359). + + EVERY FILE IS NAMED, in one refusal, so the operator repairs them in one + pass instead of meeting them one run at a time; `_workflow_refs` reads + them all before it raises. The first line follows the id and the rest + follow it, one file to a line, so `shape-doctor.py`, which quotes the + first `REFUSED` line of this script's output, quotes a file's name. + """ + advice = "" + if any(one.encoding for one in files): + advice += ( + "In each `not UTF-8` line the byte is counted from 0 at the start " + "of the file and is the first that is not UTF-8: re-save the file " + "as UTF-8 (a UTF-16 file with a byte-order mark needs converting, " + "not editing), or replace that byte with the character it was " + "meant to be, and commit it. ") + if not all(one.encoding for one in files): + advice += ( + "A `cannot be read` file is not an encoding failure: this user " + "cannot open it, so restore read access to it. ") + return Refusal( + "workflow-unreadable", + "\n".join(one.line for one in files), + f"Remediation: {len(files)} workflow file(s) under " + ".github/workflows/ cannot be read, and every one is named above. " + "This validator reads every `.yml` and `.yaml` file there, whether or " + "not it names a leg, and neither skips one nor reads one lossily: a " + "workflow it skipped could hide a `@` reference that is out of " + "lockstep, which is what it exists to find, and a lossy read would " + "call green a tree that the project's other readers refuse. " + + advice + "Then run the same command again.") + + def _workflow_refs(root: Path) -> list[tuple[Path, str, str]]: - """Every (file, owner/repo, sha) `@` reference in the workflows.""" + """Every (file, owner/repo, sha) `@` reference in the workflows, or + ONE refusal naming each workflow file that cannot be read (#359). + + EVERY FILE IS READ, whether or not it names a leg: the file is opened + before it is known to hold a reference, so a UTF-16 workflow that names no + leg used to stop the run in a `UnicodeDecodeError` (a `ValueError`, which + `main()` does not catch: a traceback, exit 1 -- the code this validator + uses for a finding). It is a REFUSAL, exit 2, `workflow-unreadable`, as a + pin that is not UTF-8 is `yaml-unreadable`: the question cannot be asked. + """ out: list[tuple[Path, str, str]] = [] + unreadable: list[_Unreadable] = [] workflows = root / ".github" / "workflows" if not workflows.is_dir(): return out for path in sorted(workflows.iterdir()): if path.suffix not in (".yml", ".yaml") or not path.is_file(): continue - for match in WORKFLOW_REF_RE.finditer(path.read_text(encoding="utf-8")): + text, problem = _read_workflow(root, path) + if problem is not None: + unreadable.append(problem) + continue + for match in WORKFLOW_REF_RE.finditer(text): out.append((path, match.group("repo"), match.group("sha").lower())) + if unreadable: + raise _workflow_unreadable(unreadable) return out diff --git a/tests/test_validate_pins_unreadable_workflow.py b/tests/test_validate_pins_unreadable_workflow.py new file mode 100644 index 00000000..ffd66c78 --- /dev/null +++ b/tests/test_validate_pins_unreadable_workflow.py @@ -0,0 +1,238 @@ +# SPDX-License-Identifier: Apache-2.0 +"""A workflow file that cannot be read is REFUSED BY NAME, not a traceback (#359). + +`scripts/validate-pins.py` reads every `.yml` and `.yaml` file under +`.github/workflows/` to find the `@` references that name a leg. That read +went around `load_yaml`, so a workflow that was not UTF-8 -- a Latin-1 +`# café` comment, a UTF-16 file with a byte-order mark -- raised +`UnicodeDecodeError`. It is a `ValueError`, `main()` caught `Refusal` alone, +and the run ended in a Python traceback with exit 1: the code this validator +uses for a FINDING, with no line naming the file, where a pin that is not +UTF-8 is a clean `REFUSED yaml-unreadable`, exit 2. A file this user cannot +open (`PermissionError`) was the same unguarded line. + +The script is a template copy that every project carries (it is digest-pinned +in the project's own `contracts/shape-pin.yaml`), so these cases run the copy +a real scaffold wrote, over a committed tree. + +The files are written with `write_bytes`: the point is the bytes, and +`write_text` would re-encode them, and on Windows rewrite their line endings. + +NOTHING HERE REACHES A NETWORK. The scaffold uses `--local-remote-dir`, and no +case asks for a neutral-product pin. +""" + +from __future__ import annotations + +import json +import os +import stat + +import pytest + +from conftest import ORG, PROJECT, REPO, git, run_script + +DOCTOR = REPO / "shape-doctor.py" +WORKFLOWS = ".github/workflows" +OTHER_SHA = "0123456789abcdef0123456789abcdef01234567" + +CANNOT_RUN_AS_ROOT = pytest.mark.skipif( + os.name == "nt" or (hasattr(os, "geteuid") and os.geteuid() == 0), + reason="a mode-000 file is read by root, and Windows modes do not refuse " + "a read") + + +def validate(root): + return run_script(root / "scripts" / "validate-pins.py", cwd=root) + + +def leg_workflow(root, sha: str | None = None) -> bytes: + """A workflow naming the spec leg at its pinned commit (or at `sha`): the + file the lockstep rule exists for, so a skipped one could hide a stale + reference. UTF-8, with LF line endings, whatever the host.""" + pinned = sha or git("rev-parse", "HEAD:spec", cwd=root).stdout.strip() + return (f"on: [pull_request]\njobs:\n s:\n" + f" uses: {ORG}/{PROJECT}-spec/.github/workflows/x.yml@{pinned}\n" + ).encode("utf-8") + + +def commit_bytes(root, rel: str, data: bytes, message: str) -> None: + """Write `rel` as exactly `data` and commit it, as in the issue: the file + is wrong in the repository, not only in a working tree.""" + target = root / rel + target.write_bytes(data) + git("add", "--", rel, cwd=root) + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", + "-m", message, "--", rel, cwd=root) + + +def not_utf8_line(rel: str, data: bytes) -> str: + """`: not UTF-8 at byte N ()`, with N and the reason asked of + Python's own decoder and not typed in here.""" + try: + data.decode("utf-8") + except UnicodeDecodeError as exc: + return f"{rel}: not UTF-8 at byte {exc.start} ({exc.reason})" + raise AssertionError(f"fixture drift: {rel} is UTF-8") + + +def refusal_lines(result) -> list[str]: + """The files the refusal names, one per line, as printed: the first + follows the id on the `REFUSED` line, and the rest follow it.""" + return [line.replace("REFUSED workflow-unreadable: ", "", 1) + for line in result.stderr.splitlines() + if ": not UTF-8 at byte " in line or ": cannot be read (" in line] + + +def assert_refused_by_name(result, root, *names_and_lines: str) -> None: + """One `workflow-unreadable` refusal, exit 2, no traceback, naming each + file relative to the root and never by its absolute path.""" + assert "Traceback" not in result.stderr, result.stderr + assert "UnicodeDecodeError" not in result.stderr, result.stderr + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.count("REFUSED ") == 1, result.stderr + assert result.stderr.startswith("REFUSED workflow-unreadable: "), \ + result.stderr + assert "pins ok" not in result.stdout + assert refusal_lines(result) == list(names_and_lines), result.stderr + assert str(root) not in result.stderr, \ + "the files are named relative to the root, never by absolute path" + assert f"{len(names_and_lines)} workflow file(s)" in result.stderr + + +# --- the refusal ------------------------------------------------------------ + +LATIN_1 = "legs.yml", lambda root: leg_workflow(root) + b"# caf\xe9\n" +UTF_16 = "other.yml", lambda root: "on: [push]\n".encode("utf-16") + + +@pytest.mark.parametrize("name, build", [ + pytest.param(*LATIN_1, id="latin-1-comment-in-the-file-naming-the-leg"), + pytest.param(*UTF_16, id="utf-16-bom-in-a-file-naming-no-leg"), +]) +def test_a_workflow_that_is_not_utf8_is_refused_by_name(project, name, build): + """The first two rows of the issue: a committed Latin-1 comment, and a + UTF-16 file that names no leg and still stopped the run, because the file + is opened before it is known to hold a reference.""" + data = build(project) + commit_bytes(project, f"{WORKFLOWS}/{name}", data, f"workflow {name}") + result = validate(project) + assert_refused_by_name(result, project, + not_utf8_line(f"{WORKFLOWS}/{name}", data)) + # The remedy the operator can act on. + assert "re-save the file as UTF-8" in result.stderr, result.stderr + assert "needs converting, not editing" in result.stderr, result.stderr + assert "run the same command again" in result.stderr, result.stderr + + +def test_every_workflow_that_is_not_utf8_is_named_in_the_one_refusal(project): + """Row (c): two files, one run. `.yaml` is read as `.yml` is, and the files + that ARE readable -- the scaffold's `validate.yml` and a good `ok.yml` -- + are not named.""" + latin = leg_workflow(project) + b"# caf\xe9\n" + utf16 = "on: [push]\n".encode("utf-16") + commit_bytes(project, f"{WORKFLOWS}/ok.yml", leg_workflow(project), "ok") + commit_bytes(project, f"{WORKFLOWS}/legs.yml", latin, "latin-1") + commit_bytes(project, f"{WORKFLOWS}/other.yaml", utf16, "utf-16") + result = validate(project) + assert_refused_by_name( + result, project, + not_utf8_line(f"{WORKFLOWS}/legs.yml", latin), + not_utf8_line(f"{WORKFLOWS}/other.yaml", utf16)) + assert "validate.yml" not in result.stderr, result.stderr + assert "ok.yml" not in result.stderr, result.stderr + + +def test_a_workflow_is_refused_and_neither_skipped_nor_read_lossily(project): + """THE REFUSAL IS THE POINT. The file names the spec leg at a commit that + is NOT the gitlink, which is exactly what this gate exists to find. Skipped, + the run would say `pins ok`; read with `errors="replace"`, it would report + `pin-workflow-ref-mismatch` and exit 1 over a file the project's other + readers refuse. Neither: the question cannot be asked, so it is refused.""" + data = leg_workflow(project, OTHER_SHA) + b"# caf\xe9\n" + commit_bytes(project, f"{WORKFLOWS}/legs.yml", data, "stale and latin-1") + result = validate(project) + assert_refused_by_name(result, project, + not_utf8_line(f"{WORKFLOWS}/legs.yml", data)) + assert "pin-workflow-ref-mismatch" not in result.stderr, result.stderr + + +@CANNOT_RUN_AS_ROOT +def test_a_workflow_this_user_cannot_open_is_refused_by_name(project): + """The last row of the issue: not an encoding failure but the same + unguarded read -- a `PermissionError` traceback. The refusal says what the + operating system said, and its remedy is access, not a re-save.""" + target = project / WORKFLOWS / "legs.yml" + commit_bytes(project, f"{WORKFLOWS}/legs.yml", leg_workflow(project), + "legs") + target.chmod(0) + try: + result = validate(project) + finally: + target.chmod(stat.S_IREAD | stat.S_IWRITE) + assert "Traceback" not in result.stderr, result.stderr + assert "PermissionError" not in result.stderr, result.stderr + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.startswith( + f"REFUSED workflow-unreadable: {WORKFLOWS}/legs.yml: cannot be read " + "("), result.stderr + assert str(project) not in result.stderr, result.stderr + assert "restore read access" in result.stderr, result.stderr + assert "re-save" not in result.stderr, result.stderr + + +def test_the_doctors_pins_row_names_the_file(project): + """`shape-doctor.py` quotes the first `REFUSED` line of the project's own + validator in its `pins` row, so the file's name reaches a reader who never + runs the validator: before, the row quoted the last line of a traceback.""" + data = leg_workflow(project) + b"# caf\xe9\n" + commit_bytes(project, f"{WORKFLOWS}/legs.yml", data, "latin-1") + result = run_script(DOCTOR, "--root", str(project), "--json") + assert "Traceback" not in result.stderr, result.stderr + row = {r["id"]: r for r in json.loads(result.stdout)["rows"]}["pins"] + assert row["status"] == "FINDING", row + assert row["detail"]["exit"] == 2, row + assert "refuses (exit 2)" in row["reason"], row["reason"] + assert ("REFUSED workflow-unreadable: " + + not_utf8_line(f"{WORKFLOWS}/legs.yml", data)) in row["reason"], \ + row["reason"] + assert "UnicodeDecodeError" not in row["reason"], row["reason"] + + +# --- controls: what must not change ----------------------------------------- + +def test_a_pin_that_is_not_utf8_is_still_yaml_unreadable(project): + """Row (d): the refusal the workflow's one is modelled on, unchanged. Its + path is the absolute one `load_yaml` has always printed; the workflow + refusal is relative to the root, as the issue asks.""" + pin = project / "contracts" / "spec-pin.yaml" + commit_bytes(project, "contracts/spec-pin.yaml", + pin.read_bytes() + b"# caf\xe9\n", "latin-1 in the pin") + result = validate(project) + assert "Traceback" not in result.stderr, result.stderr + assert result.returncode == 2, result.stdout + result.stderr + assert "REFUSED yaml-unreadable: " in result.stderr, result.stderr + assert "spec-pin.yaml: not UTF-8 at byte " in result.stderr, result.stderr + assert "workflow-unreadable" not in result.stderr, result.stderr + + +@pytest.mark.parametrize("name, build", [ + pytest.param("legs.yml", + lambda root: leg_workflow(root) + "# café\n".encode("utf-8"), + id="utf-8-comment"), + pytest.param("legs.yml", + lambda root: b"\xef\xbb\xbf" + leg_workflow(root), + id="utf-8-byte-order-mark"), + pytest.param("notes.txt", lambda root: b"# caf\xe9\n", + id="a-latin-1-file-that-is-not-a-workflow"), +]) +def test_what_is_readable_or_not_a_workflow_still_passes(project, name, build): + """Row (e), and its two neighbours: UTF-8 with a non-ASCII comment, UTF-8 + with a byte-order mark (which `utf-8` keeps as a character and does not + refuse), and a file under `.github/workflows/` that is neither `.yml` nor + `.yaml`, which this validator has never read.""" + commit_bytes(project, f"{WORKFLOWS}/{name}", build(project), + f"workflow {name}") + result = validate(project) + assert result.returncode == 0, result.stdout + result.stderr + assert "pins ok" in result.stdout From 7c998ffcc1dad5bc3d23a31a3a9383d51cee8426 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 15:47:29 +0000 Subject: [PATCH 5/6] tests/test_shape_pin_files_not_a_list.py: a `files:` that is a string, `abc`, is a case of its own beside `5`, `true` and `1.5`, so a guard widened to `isinstance(files, (list, str))` in `read_pin`, `validate-pins.py` or `validate-family.py` fails the file (#290) The PR promised that a string `files:` gets one finding and one refusal naming the whole value, and nothing tested it. A string is iterable, so the three scalars the file already had could not tell the right guard from one that lets a string through: the two validators gave one finding per character and `read_pin` refused it as a bad ROW. Three one-line mutants each passed all 22 tests (review of 039530b: mu6 at validate-pins.py, mu7 at update-shape.py's `read_pin`, mu9 at validate-family.py). With `files-abc` in `NOT_A_LIST`, each of the six parametrised tests runs it, so every reader is asked for exactly one finding or one whole-`files:` refusal, and each mutant now fails. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Sonnet 5.5 --- tests/test_shape_pin_files_not_a_list.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/test_shape_pin_files_not_a_list.py b/tests/test_shape_pin_files_not_a_list.py index 4d95d3d4..18459c54 100644 --- a/tests/test_shape_pin_files_not_a_list.py +++ b/tests/test_shape_pin_files_not_a_list.py @@ -10,9 +10,14 @@ `files: 5`, `files: true` and `files: 1.5` ended every one of them in `TypeError: 'int' object is not iterable` (or `'bool'`, `'float'`), exit 1, and the doctor, whose one job is to say what is wrong, printed nothing at all. +A STRING is iterable, so `files: abc` is the case that did not die: the +validators took its characters for rows and gave one finding each, and +`read_pin` refused it as a bad ROW with no word about the value. Each case below commits one such pin over a clean scaffold or a clean holder -and runs a reader on it. On the code before #290 each one is a traceback. +and runs a reader on it. On the code before #290 each one is a traceback, +except `files: abc`, which is a finding per character or a refusal that names +the wrong thing. The doctor's VERDICT for a root pin `read_pin` refuses is #237's business (PR #267 moves it from CANNOT ANSWER, exit 3, to INVALID, exit 1), so the @@ -48,10 +53,17 @@ #: `(what follows `files:` in the pin, how the reader shows the value back)`. #: Python's `repr`, because each reader names the value with `!r` the way it #: already names a `commit:` or a `revision_kind:` it cannot use. +#: +#: `abc` is the one value here that IS iterable: a guard written as +#: `isinstance(files, (list, str))` lets it through, and each reader then +#: takes its characters for rows (the validators, one finding per character). +#: The three scalars above are a TypeError before the guard and cannot tell +#: that guard from the right one, so the string is its own case. NOT_A_LIST = [ pytest.param("5", "5", id="files-5"), pytest.param("true", "True", id="files-true"), pytest.param("1.5", "1.5", id="files-1.5"), + pytest.param("abc", "'abc'", id="files-abc"), ] #: Every verdict the doctor's text report can end on. From 9e6caeb1216588dfde3e40aab8a09da7d83f9eed Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 15:47:37 +0000 Subject: [PATCH 6/6] update-shape.py: `pin_rows_remediation`, `cmd_apply` and `_awaits_its_pull_request` spell the pin's path by the materializer's `SHAPE_PIN`, so `contracts/shape-pin.yaml` has no third copy in this module for Sonar's S1192 (#290) `pin_rows_remediation` spelled `"contracts/shape-pin.yaml"` a third time in update-shape.py, after `cmd_apply`'s set of paths to stage and `_awaits_its_pull_request`; Sonar called the third copy a CRITICAL S1192 and it was the one new issue on the PR. `scripts/shape_materialize.py` already names the path once, as `SHAPE_PIN`, because a second spelling is how the lists beside it would stop matching what is written, and this module already takes its other constants from there. All three now use that name. Two things left as they were, on purpose. The `root / "contracts" / "shape-pin.yaml"` joins in `read_pin` and `_repin` are two short strings, not the flagged literal, and #347 rewrites the top of `read_pin` (a merge of this branch with it conflicts there if the join changes). The name comes in by its own `from shape_materialize import SHAPE_PIN` line after the existing import block, not as one more name in it, because #380 rewraps that block and a merge conflicts there if this branch does too. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Sonnet 5.5 --- update-shape.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/update-shape.py b/update-shape.py index 94997f67..045487e4 100755 --- a/update-shape.py +++ b/update-shape.py @@ -123,6 +123,9 @@ inherited_child_environment, lane_trailer_argument, readme_shape_lines, readme_trailing_shape_line, run, trailer_line, ) +# Where a root keeps its shape pin. The materializer that writes it names it +# once; this module spells the path by that name and never by a literal (#290). +from shape_materialize import SHAPE_PIN # noqa: E402 #: `owner/repo`, the only remote spelling `--upstream` accepts. REPOSITORY_RE = re.compile(r"^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$") @@ -396,7 +399,7 @@ def pin_rows_remediation(root: Path) -> str: by `_by_hand`, this module's one quoter for a line a person pastes, so a root holding a space is one argument there. """ - log = _by_hand(root, "log", "--", "contracts/shape-pin.yaml") + log = _by_hand(root, "log", "--", SHAPE_PIN) return ( "Remediation: `files:` is a list with one `path:`/`sha256:` row per " "copied file. The tool that cut this root wrote it and only " @@ -2508,7 +2511,7 @@ def cmd_apply(args) -> int: _refuse_red_validators(root, kind) added = [add.path for add in taking] paths = sorted({row.path for row in copied} | set(added) - | {"contracts/shape-pin.yaml", kind.manifest} + | {SHAPE_PIN, kind.manifest} | ({README} if readme.rewritten else set())) _land(root, args, paths, added, target, upstream, kind, len(copied), ledger) @@ -2675,7 +2678,7 @@ def _awaits_its_pull_request(root: Path, args, kind: Kind) -> bool: base = tracking_branch_of(load_yaml(root / kind.manifest)) if args.branch == base: return False - pin = "contracts/shape-pin.yaml" + pin = SHAPE_PIN proposed = _git_answer(root, ["rev-parse", "--verify", "--quiet", f"HEAD:{pin}"]) landed = _git_answer(root, ["rev-parse", "--verify", "--quiet",