From bb7929519040295528b22b81d3d4dcac888cb49d Mon Sep 17 00:00:00 2001 From: openRepoShape CI Date: Sun, 11 Oct 2026 16:38:02 +0000 Subject: [PATCH] update-shape.py: `apply` keeps each of the manifest's `shape:` pin lines at the indentation it already has and reads the rewrite back, so a `project.yaml` indented four spaces takes the update instead of being rolled back under the validators' lockstep remedy (#208) `rewrite_manifest` FINDS `commit:` and `tree_sha256:` inside `shape:` by their stripped text, so it recognises the block at any indentation the reader accepts, and it then WROTE them back at a fixed two and four spaces. A `project.yaml` whose `shape:` block is indented four spaces (`tree_sha256:` at six) came back with `commit:` at two among siblings at four, which `load_yaml` refuses. Both validators went red, `apply` rolled back, and the refusal blamed the project: each `yaml-unparsable` line carried the lockstep remedy ("advance the gitlink ...") and `update-validators-red` said a tool that left a project red would have moved the cost to somebody else's pull request. `check` kept offering the same `apply`. - Each of the two lines is written as its own leading whitespace + key + the new quoted value. Only the value moves; the diff to the manifest is those two values, whatever indentation the block was written in. - What `rewrite_manifest` returns has been READ BACK with `parse_yaml`: it must give `shape.commit` and `shape.digests.tree_sha256` equal to the target, or it is refused as `update-manifest-unrecognised` before a byte is written, with a remedy that says the fault is this tool's line rewrite. The one layout a line rewrite cannot carry, a value on the line below its key, reads and validates, and is now refused here by name instead of by the validators with a remedy about the pin. - The read-back blames this tool only when the fault is its own: a manifest that did not read before the rewrite was red before `apply` ran, and is rewritten as before and left to the validators, which name that line. - `family.yaml` goes through the same function; its block is covered too. Unchanged: a manifest indented the way the scaffold writes it. Measured on main a5b09c3 and on this branch against one upstream commit, two byte-for- byte copies of one scaffolded project: the two project trees after `apply`, `check`'s output and `apply`'s output are identical (`diff -r`, `diff`). Left out: `rewrite_shape_pin` matches ` tree_sha256:` with exactly two spaces, so a `contracts/shape-pin.yaml` whose `digests:` child is indented four is refused as `update-pin-unrecognised`, loudly and rolled back, on main and here alike (measured); `scripts/bump-leg.py`'s `rewrite_pin` matches the same way (read, not run). That file is written by the scaffold and says not to hand-edit it, and its refusal names the file, so it is not changed here. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Sonnet 5.5 --- tests/test_update_shape.py | 198 +++++++++++++++++++++++++++++++++++++ update-shape.py | 84 ++++++++++++++-- 2 files changed, 276 insertions(+), 6 deletions(-) diff --git a/tests/test_update_shape.py b/tests/test_update_shape.py index 514685b4..361be799 100644 --- a/tests/test_update_shape.py +++ b/tests/test_update_shape.py @@ -421,6 +421,204 @@ def test_apply_with_no_shape_block_names_this_kinds_own_manifest_validator( == upstream_and_project["a"], "a refused apply writes nothing at all" +# --- the manifest's own indentation (#208) ---------------------------------- + +#: Indentations of the `shape:` block that `load_yaml` and both validators +#: accept, as (direct children, `digests:` children). `(2, 4)` is what the +#: scaffold writes; the rest are what a hand re-indent or an editor's +#: formatter leaves. The reader asks only that a level nest deeper than its +#: parent, so every one of these is a manifest this project may hold. +SHAPE_INDENTS = [(2, 4), (4, 6), (3, 6), (1, 2), (8, 12), (4, 5), (2, 6), + (3, 4)] + + +def indent_shape_block(text: str, child: int, grandchild: int) -> str: + """`text` with ONLY its `shape:` block re-indented: each direct child at + `child` spaces, each `digests:` child at `grandchild`. Every other line, + the comments above the block included, is left as it was.""" + out, inside = [], False + for line in text.split("\n"): + if line.rstrip() == "shape:": + inside = True + elif inside and line.strip() and not line.startswith(" "): + inside = False + elif inside and line.strip(): + depth = grandchild if line.startswith(" ") else child + line = " " * depth + line.strip() + out.append(line) + return "\n".join(out) + + +def commit_manifest(root, text: str, message: str) -> None: + """Write `project.yaml` as exactly these bytes and commit it. Bytes, not + `write_text`, so the line endings under test are the ones written on + every platform.""" + (root / "project.yaml").write_bytes(text.encode("utf-8")) + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", + message, "--", "project.yaml", cwd=root) + + +@pytest.mark.parametrize("child, grandchild", SHAPE_INDENTS) +def test_the_manifest_rewrite_moves_two_values_and_keeps_every_indent( + update_shape, upstream_and_project, tmp_path, child, grandchild): + """ONLY THE TWO VALUES MOVE (#208). The lines were found at any + indentation and written back at a fixed two and four spaces, so a block + indented four spaces came back with `commit:` at two among siblings at + four — YAML the same reader refuses. The expected text here is the input + with the two quoted values substituted and nothing else, which for + `(2, 4)` is byte for byte what the fixed-indent rewrite wrote before.""" + a = upstream_and_project["a"] + scaffolded = (upstream_and_project["clone"] / "project.yaml").read_text( + encoding="utf-8") + text = indent_shape_block(scaffolded, child, grandchild) + (tmp_path / "before.yaml").write_bytes(text.encode("utf-8")) + old_tree = load_yaml(tmp_path / "before.yaml")["shape"]["digests"][ + "tree_sha256"] + commit, tree = "c" * 40, "d" * 64 + + new = update_shape.rewrite_manifest(text, commit, tree, + update_shape.PROJECT_KIND) + + assert new == (text.replace(f'commit: "{a}"', f'commit: "{commit}"') + .replace(f'tree_sha256: "{old_tree}"', + f'tree_sha256: "{tree}"')) + (tmp_path / "after.yaml").write_bytes(new.encode("utf-8")) + shape = load_yaml(tmp_path / "after.yaml")["shape"] + assert (shape["commit"], shape["digests"]["tree_sha256"]) == (commit, tree) + + +#: A family holder's manifest, cut down to the lines that matter here: a +#: member row carrying a `commit:` of its own BEFORE the block, and a key +#: after it, so the block's two bounds are both exercised. `family.yaml`'s +#: `shape:` block has the shape `project.yaml`'s has, and goes through the +#: same `rewrite_manifest`. +FAMILY_MANIFEST = """schema_version: 1 +kind: family-manifest + +members: + - name: Atlas + commit: "{member}" + +shape: + repository: opensoft/openRepoShape + revision_kind: commit + commit: "{commit}" + digest_algorithm: sha256 + digests: + tree_sha256: "{tree}" + +visibility: private +""" + + +@pytest.mark.parametrize("child, grandchild", [(2, 4), (4, 6), (3, 6)]) +def test_the_family_manifest_rewrite_keeps_its_indent_and_its_members_pin( + update_shape, tmp_path, child, grandchild): + """THE FAMILY HOLDER IS THE SAME REWRITE (#208). `family.yaml` carries + the same `shape:` block, so the same two lines move and nothing else does: + not the member row's own `commit:`, which sits above the block and spells + the key the same way.""" + member, old_commit, old_tree = "a" * 40, "b" * 40, "e" * 64 + text = indent_shape_block( + FAMILY_MANIFEST.format(member=member, commit=old_commit, + tree=old_tree), child, grandchild) + commit, tree = "c" * 40, "d" * 64 + + new = update_shape.rewrite_manifest(text, commit, tree, + update_shape.FAMILY_KIND) + + assert new == (text.replace(f'commit: "{old_commit}"', + f'commit: "{commit}"') + .replace(f'tree_sha256: "{old_tree}"', + f'tree_sha256: "{tree}"')) + assert f'commit: "{member}"' in new + (tmp_path / "family.yaml").write_bytes(new.encode("utf-8")) + loaded = load_yaml(tmp_path / "family.yaml") + assert (loaded["shape"]["commit"], + loaded["shape"]["digests"]["tree_sha256"]) == (commit, tree) + + +@pytest.mark.parametrize("child, grandchild", [(4, 6), (3, 4), (2, 6)]) +def test_apply_leaves_a_manifest_indented_its_own_way_green_and_indented( + root, upstream_and_project, child, grandchild): + """The issue's reproduction, end to end: a project whose `shape:` block + is indented four spaces (`tree_sha256:` at six) takes the update, both + validators stay green, and the diff to `project.yaml` is the two values. + `(3, 4)` nests by one space; `(2, 6)` is the scaffold's own two with + `digests:` nested by four, which the fixed-indent rewrite did not break + but silently re-indented.""" + a, target = upstream_and_project["a"], upstream_and_project["b"] + upstream = upstream_and_project["upstream"] + manifest = root / "project.yaml" + commit_manifest(root, indent_shape_block( + manifest.read_text(encoding="utf-8"), child, grandchild), + "Re-indent the shape block") + validators_are_green(root) + before = manifest.read_text(encoding="utf-8") + old_tree = load_yaml(manifest)["shape"]["digests"]["tree_sha256"] + + result = apply(root, upstream_and_project) + + assert result.returncode == 0, result.stdout + result.stderr + tree = tree_digest(upstream, target) + assert manifest.read_text(encoding="utf-8") == ( + before.replace(f'commit: "{a}"', f'commit: "{target}"') + .replace(f'tree_sha256: "{old_tree}"', f'tree_sha256: "{tree}"')) + shape = load_yaml(manifest)["shape"] + assert shape["commit"].lower() == target + assert shape["digests"]["tree_sha256"].lower() == tree + validators_are_green(root) + + +def test_a_layout_the_rewrite_cannot_carry_is_refused_as_its_own_fault( + root, upstream_and_project): + """A VALUE ON THE LINE BELOW ITS KEY reads, and validates, and is the + one layout a line rewrite cannot move: it replaces the key's line and + leaves the value's line behind, which the reader then refuses. Before + #208 that reached the validators, and `apply` relayed their lockstep + remedy — advance the gitlink — over a manifest the project had not + broken. The read-back refuses it first, by this tool's name, and the + tree is rolled back to the byte.""" + a = upstream_and_project["a"] + manifest = root / "project.yaml" + text = manifest.read_text(encoding="utf-8") + folded = text.replace(f' commit: "{a}"\n', f" commit: >-\n {a}\n", 1) + assert folded != text, "fixture: the scaffold's commit line was found" + commit_manifest(root, folded, "Fold the shape commit onto its own line") + validators_are_green(root) + + result = apply(root, upstream_and_project) + + assert_refused_and_unpinned(result, root, upstream_and_project, + "update-manifest-unrecognised", + "the fault is this tool's line rewrite", + "unexpected indentation") + assert "update-validators-red" not in result.stderr + assert "advance the gitlink" not in result.stderr + assert manifest.read_bytes() == folded.encode("utf-8") + assert git("status", "--porcelain", cwd=root).stdout == "" + + +def test_a_manifest_that_did_not_read_before_is_left_to_its_validators( + update_shape, upstream_and_project): + """THE READ-BACK BLAMES THIS TOOL ONLY WHEN THE FAULT IS ITS OWN. A + manifest the reader refused before a byte was rewritten was red before + `apply` ran; refusing it as `update-manifest-unrecognised` would blame + the rewrite for the project's own line. It is rewritten as before and + left to the validators, which name that line.""" + text = (upstream_and_project["clone"] / "project.yaml").read_text( + encoding="utf-8") + broken = text.replace("\nkind: project-manifest\n", + "\nkind: project-manifest\n\tstray: tab\n", 1) + assert broken != text, "fixture: the line to break was found" + + new = update_shape.rewrite_manifest(broken, "c" * 40, "d" * 64, + update_shape.PROJECT_KIND) + + assert ' commit: "' + "c" * 40 + '"' in new + assert "\tstray: tab" in new + + # --- drift the project introduced ------------------------------------------ def edit_locally(root, rel: str) -> None: diff --git a/update-shape.py b/update-shape.py index 553c1508..43b9a88a 100755 --- a/update-shape.py +++ b/update-shape.py @@ -101,8 +101,9 @@ SHAPE_ROOT = Path(__file__).resolve().parent sys.path.insert(0, str(SHAPE_ROOT / "scripts")) from repo_shape import ( # noqa: E402 - COMMIT_RE, PYTHON, TREE_DIGEST_DEFINITION, Refusal, checked_value, die, - file_sha256, git_out, load_yaml, tree_digest, + COMMIT_RE, PYTHON, TREE_DIGEST_DEFINITION, Refusal, YamlError, + checked_value, die, file_sha256, git_out, load_yaml, parse_yaml, + tree_digest, ) from shape_materialize import ( # noqa: E402 COPIED_FROM_SHAPE, COPIED_VERBATIM, FAMILY_COPIED_FROM_SHAPE, @@ -861,18 +862,33 @@ def rewrite_manifest(text: str, commit: str, tree: str, kind: Kind) -> str: Scoped to the block on purpose: `commit:` and `tree_sha256:` are spelled the same way in every pin, and a whole-file replacement would move a leg's pin while claiming to move the shape's. + + EACH LINE KEEPS ITS OWN INDENTATION (#208). The two lines are FOUND by + their stripped text, so a block at any indentation `load_yaml` accepts is + recognised; they were then WRITTEN at a fixed two and four spaces, which + turned a block indented four spaces — accepted by the reader and by both + validators — into one the reader refuses, and the validators' refusal + that followed blamed the project for this tool's write. Only the value + after the key changes now; the whitespace in front of it is the line's. + + WHAT IS RETURNED HAS BEEN READ BACK. A layout this line rewrite cannot + carry — a value on the line below its key — is refused HERE, as this + tool's failure, rather than by a validator after the fact + (`_refuse_unless_it_reads_back`). """ lines = text.splitlines() start, end = _shape_block_bounds(lines, kind) seen = {"commit": 0, "tree": 0} for index in range(start + 1, end): - stripped = lines[index].strip() + line = lines[index] + stripped = line.strip() + indent = line[:len(line) - len(line.lstrip())] if stripped.startswith("commit:"): seen["commit"] += 1 - lines[index] = f' commit: "{commit}"' + lines[index] = f'{indent}commit: "{commit}"' elif stripped.startswith("tree_sha256:"): seen["tree"] += 1 - lines[index] = f' tree_sha256: "{tree}"' + lines[index] = f'{indent}tree_sha256: "{tree}"' for key, count in seen.items(): if count != 1: raise Refusal( @@ -881,7 +897,63 @@ def rewrite_manifest(text: str, commit: str, tree: str, kind: Kind) -> str: "where exactly one was expected", "Remediation: fix the block by hand this once; this tool will " "not guess at a manifest it does not recognise.") - return "\n".join(lines) + ("\n" if text.endswith("\n") else "") + rewritten = "\n".join(lines) + ("\n" if text.endswith("\n") else "") + _refuse_unless_it_reads_back(text, rewritten, commit, tree, kind) + return rewritten + + +def _shape_pin_of(text: str) -> tuple[str, str]: + """`shape.commit` and `shape.digests.tree_sha256`, read out of a + manifest's text the way its validator reads them, or a `ValueError` + saying why they cannot be (#208).""" + try: + manifest = parse_yaml(text) + except YamlError as exc: + raise ValueError(str(exc)) from exc + shape = manifest.get("shape") if isinstance(manifest, dict) else None + digests = shape.get("digests") if isinstance(shape, dict) else None + commit = shape.get("commit") if isinstance(shape, dict) else None + tree = digests.get("tree_sha256") if isinstance(digests, dict) else None + if commit is None or tree is None: + raise ValueError("no `shape.commit` and `shape.digests.tree_sha256` " + "where the validator reads them") + return str(commit), str(tree) + + +def _refuse_unless_it_reads_back(before: str, after: str, commit: str, + tree: str, kind: Kind) -> None: + """Refuse a rewrite that does not read back to the target (#208). + + THE REFUSAL IS THIS TOOL'S, SO IT IS RAISED ONLY WHEN THE FAULT IS. A + manifest that read correctly before the rewrite and does not after it + was broken by the rewrite, and the remedy says so. One that did not read + before — unparsable, or no pin where the validator looks — was red + before this command ran: that is left to the validators, which name the + project's own line, and blaming this tool for it would be the same wrong + answer turned around. + """ + try: + found = _shape_pin_of(after) + if found == (commit, tree): + return + why = (f"it would read `commit: {found[0]}` and " + f"`tree_sha256: {found[1]}`") + except ValueError as exc: + why = f"the reader would refuse it: {exc}" + try: + _shape_pin_of(before) + except ValueError: + return + raise Refusal( + "update-manifest-unrecognised", + f"{kind.manifest}'s `shape:` block reads as it stands, but this " + f"tool's rewrite of it does not read back to the target ({why}), so " + "this tool will not write it", + "Remediation: the fault is this tool's line rewrite, not the " + "project's manifest. Put each of `commit:` and `tree_sha256:` on one " + "line with its quoted value, as the scaffold writes them, and run " + "the same command again; this tool will not guess at a manifest it " + "does not recognise.") def _shape_block_bounds(lines: list[str], kind: Kind) -> tuple[int, int]: