From 60698ed19c24c61f49ce42108c3640c7a891bc6b Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:11:22 -0400 Subject: [PATCH 1/5] update-shape.py: `classify` hands each pinned row to `_classify_row` and the verdict ladder to `_row_verdict`, so the function Sonar scored at 24 is a loop over rows and each decision it makes is readable alone (#133) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SonarCloud's `python:S3776` scored `classify` at 24 against a limit of 15 (complexipy 8.0.1 agrees: 24). One loop body answered three "these bytes cannot be compared" questions and then walked a five-branch verdict ladder over three derived booleans, so the four byte-strings the docstring names — the recorded digest, the root's bytes, the upstream's bytes at the pinned commit and at the target — had to be reconstructed from the ladder by anyone reading it. It is now three functions and the same code. `classify` is the loop. `_classify_row` holds the three exits where a byte-string does not exist at all (no file in the root, no upstream path this pin maps to, nothing at the target), each of which is a human's decision rather than a difference this tool acts on. `_row_verdict` is the ladder, reached only when all four read. Nothing moved across the reads: the root's bytes, then the pinned blob, then the target blob, in that order, because `Upstream.blob` shells out to git and a reordering would change what a failing run prints. `file_sha256` is still called exactly where it was, passed into `_row_verdict` as the argument that was the local variable. Every state, detail string and digest assignment is byte-identical, `continue` becoming `return` in a body that now ends the function instead of the iteration. classify 24 -> 1; _classify_row 3 (new); _row_verdict 11 (new). Proven by tests/test_update_shape.py, tests/test_update_shape_add.py and tests/test_commit_trailers.py: 178 passed. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Fable 5.1 --- update-shape.py | 162 +++++++++++++++++++++++++++++------------------- 1 file changed, 97 insertions(+), 65 deletions(-) diff --git a/update-shape.py b/update-shape.py index 5959e9e6..1cae0bee 100755 --- a/update-shape.py +++ b/update-shape.py @@ -446,71 +446,103 @@ def classify(root: Path, rows: list[Row], upstream: Upstream, pinned: str, same answer it gives. """ for row in rows: - target_path = root / row.path - if not target_path.is_file(): - row.state = COPY_MISSING - row.detail = ("pinned but absent from the root; a human decides " - "whether it was deleted on purpose") - continue - source = source_for(row.path, upstream, target, kind) - row.source = source - if source is None: - row.state = UNMAPPED - row.detail = ("no file in the upstream tree corresponds to this " - "pinned path") - continue - here = target_path.read_bytes() - pinned_bytes = upstream.blob(pinned, source) - target_bytes = upstream.blob(target, source) - if target_bytes is None: - row.state = UPSTREAM_REMOVED - row.detail = (f"{source} is gone from the upstream at the target " - "commit; deleting a project's copy is a decision, " - "not a copy") - continue - row.target_bytes = target_bytes - local_digest = file_sha256(target_path) - locally_modified = local_digest != row.recorded - upstream_changed = pinned_bytes is None or pinned_bytes != target_bytes - verbatim_at_pin = (pinned_bytes is not None - and file_bytes_sha256(pinned_bytes) == row.recorded) - - if here == target_bytes: - # Already holding the target's bytes. Only the row is stale, and - # recomputing it hides nothing: these ARE the upstream's bytes. - row.state = (ALREADY_AT_TARGET if locally_modified else UNCHANGED) - row.digest = local_digest - continue - if locally_modified and upstream_changed: - row.state = BOTH - row.detail = "edited here AND upstream since the pin" - # Precomputed the same way `locally-modified` precomputes it, - # below: IF `--accept-local` later names this path, `cmd_apply` - # must re-pin from what is on disk NOW, not from whatever was - # true when the pin was written. Dead weight when nobody accepts - # it — an unaccepted `both` row is refused before any digest is - # written anywhere. - row.digest = local_digest - continue - if upstream_changed and not verbatim_at_pin: - row.state = BOTH - row.detail = ( - "upstream changed, but this copy is not a verbatim copy of " - f"{source} at the pinned commit — an in-place adoption " - "appends to it — so copying the target bytes would delete " - "what was appended") - row.digest = local_digest - continue - if upstream_changed: - row.state = UPSTREAM_CHANGED - row.digest = file_bytes_sha256(target_bytes) - continue - if locally_modified: - row.state = LOCALLY_MODIFIED - row.detail = "edited in this project since the pin" - row.digest = local_digest - continue - row.state = UNCHANGED + _classify_row(root, row, upstream, pinned, target, kind) + + +def _classify_row(root: Path, row: Row, upstream: Upstream, pinned: str, + target: str, kind: Kind) -> None: + """One pinned row's verdict, split out of `classify` for #133. + + THE THREE EXITS HERE ARE THE ONES WHERE THE COMPARISON CANNOT BE MADE AT + ALL, because one of the four byte-strings `classify` names does not + exist: the root has no file at this path, no upstream path corresponds to + it, or the upstream has none at the target commit. Each is a decision for + a human rather than a difference this tool can act on, so `_row_verdict` + below is never reached holding a hole. The reads are left in the order + they were written in — the root's bytes, then the pinned blob, then the + target blob — because `Upstream.blob` shells out to git and a reordering + would change what a failing run prints. + """ + target_path = root / row.path + if not target_path.is_file(): + row.state = COPY_MISSING + row.detail = ("pinned but absent from the root; a human decides " + "whether it was deleted on purpose") + return + source = source_for(row.path, upstream, target, kind) + row.source = source + if source is None: + row.state = UNMAPPED + row.detail = ("no file in the upstream tree corresponds to this " + "pinned path") + return + here = target_path.read_bytes() + pinned_bytes = upstream.blob(pinned, source) + target_bytes = upstream.blob(target, source) + if target_bytes is None: + row.state = UPSTREAM_REMOVED + row.detail = (f"{source} is gone from the upstream at the target " + "commit; deleting a project's copy is a decision, " + "not a copy") + return + row.target_bytes = target_bytes + _row_verdict(row, source, here, pinned_bytes, target_bytes, + file_sha256(target_path)) + + +def _row_verdict(row: Row, source: str, here: bytes, + pinned_bytes: bytes | None, target_bytes: bytes, + local_digest: str) -> None: + """The verdict ladder itself, split out of `classify` for #133. + + Reached only for a row whose four byte-strings all read, so this is the + ladder and nothing else: the three derived booleans the parent's + docstring argues for, and the first branch that matches. The order of the + branches is the order of the argument — already at the target, then the + two ways to be a `both`, then the two single-sided answers — and moving + one would change a verdict. + """ + locally_modified = local_digest != row.recorded + upstream_changed = pinned_bytes is None or pinned_bytes != target_bytes + verbatim_at_pin = (pinned_bytes is not None + and file_bytes_sha256(pinned_bytes) == row.recorded) + + if here == target_bytes: + # Already holding the target's bytes. Only the row is stale, and + # recomputing it hides nothing: these ARE the upstream's bytes. + row.state = (ALREADY_AT_TARGET if locally_modified else UNCHANGED) + row.digest = local_digest + return + if locally_modified and upstream_changed: + row.state = BOTH + row.detail = "edited here AND upstream since the pin" + # Precomputed the same way `locally-modified` precomputes it, + # below: IF `--accept-local` later names this path, `cmd_apply` + # must re-pin from what is on disk NOW, not from whatever was + # true when the pin was written. Dead weight when nobody accepts + # it — an unaccepted `both` row is refused before any digest is + # written anywhere. + row.digest = local_digest + return + if upstream_changed and not verbatim_at_pin: + row.state = BOTH + row.detail = ( + "upstream changed, but this copy is not a verbatim copy of " + f"{source} at the pinned commit — an in-place adoption " + "appends to it — so copying the target bytes would delete " + "what was appended") + row.digest = local_digest + return + if upstream_changed: + row.state = UPSTREAM_CHANGED + row.digest = file_bytes_sha256(target_bytes) + return + if locally_modified: + row.state = LOCALLY_MODIFIED + row.detail = "edited in this project since the pin" + row.digest = local_digest + return + row.state = UNCHANGED def file_bytes_sha256(data: bytes) -> str: From 42cd51127b508916e2e16dfebeb92a758e707762 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:16:11 -0400 Subject: [PATCH 2/5] update-shape.py: `upstream_copies` asks `_tuple_constants` what the materializer's copy lists are and `_listed_copies` what they mean, so the `ast` scan and the mapping onto `Copy` are two functions rather than one body Sonar scored at 19 (#133) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SonarCloud's `python:S3776` scored `upstream_copies` at 19 against a limit of 15 (complexipy 8.0.1 scored it 21). It did two unrelated jobs in one body: an `ast` walk that harvests the upstream module's top-level string and tuple constants, and then the mapping of the three copy lists that walk found onto `Copy` objects. Reading either required stepping over the other. The scan is now `_tuple_constants`, which returns only the tuples — the strings exist so `as_literal` can resolve a list entry that names a constant rather than repeating a path, and they are not the answer, so returning them would have left the caller holding a value it never reads. The mapping is now `_listed_copies`, which keeps both loops in their original order, so a `..._FROM_SHAPE` entry still overwrites a verbatim entry of the same path exactly as it did before. `upstream_copies` keeps the two exits that make it the entry point: no materializer at that commit, and a module that will not parse. No shape check moved and none was relaxed: the entry-shape tests each loop runs are the ones that were there, because these tuples arrive from a file read out of somebody's git history and a copy list this tool cannot vouch for is dropped rather than guessed at. upstream_copies 21 -> 2; _tuple_constants 9 (new); _listed_copies 10 (new). (complexipy's numbers; Sonar scored the original 19.) Proven by tests/test_update_shape.py, tests/test_update_shape_add.py and tests/test_commit_trailers.py: 178 passed. `tests/test_update_shape_add.py` is the one that drives this path — it renames copy-list entries out of a throwaway upstream clone and asserts the `upstream-added` verdicts that come back. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Fable 5.1 --- update-shape.py | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/update-shape.py b/update-shape.py index 1cae0bee..1b48a154 100755 --- a/update-shape.py +++ b/update-shape.py @@ -636,6 +636,21 @@ def upstream_copies(upstream: Upstream, rev: str, kind: Kind) -> dict[str, Copy] # `UnicodeError`), so `.decode("utf-8")`'s own failure is already # caught here, under the name that reaches every ValueError alike. return {} + return _listed_copies(_tuple_constants(module), kind) + + +def _tuple_constants(module: ast.Module) -> dict[str, tuple]: + """The materializer's top-level TUPLE constants, by the name each is + assigned to. Split out of `upstream_copies` for #133. + + The string constants are harvested in the same pass and then thrown away, + because they are not the answer: they exist so `as_literal` can resolve + `COPIED_FROM_SHAPE`'s `VALUE_NAMING`-style references BY NAME as it walks + the module, and a copy list is a tuple. Anything that is neither — a + call, a comprehension, an entry this will not guess at — is skipped, for + the reason the section header above gives: a guess about where a file + comes from is what `unmapped` exists to refuse. + """ constants: dict[str, str] = {} tuples: dict[str, tuple] = {} for node in module.body: @@ -649,6 +664,21 @@ def upstream_copies(upstream: Upstream, rev: str, kind: Kind) -> dict[str, Copy] constants[target.id] = value elif isinstance(value, tuple): tuples[target.id] = value + return tuples + + +def _listed_copies(tuples: dict[str, tuple], kind: Kind) -> dict[str, Copy]: + """`{path in the root: Copy}` for the three lists `kind` names, out of + the constants `_tuple_constants` harvested. Split out of + `upstream_copies` for #133, which documents the mapping itself. + + The shape of each entry is CHECKED rather than assumed: these tuples came + out of a file read from somebody's git history, so a verbatim entry that + is not a string, a `..._FROM_SHAPE` entry that is not a pair of them, and + any path `inside_a_root` refuses are all dropped instead of raising. The + `..._FROM_SHAPE` pass runs second and may overwrite a verbatim entry of + the same path, exactly as it did when both loops sat in one body. + """ verbatim, from_shape, executable = kind.lists executables = {entry for entry in tuples.get(executable, ()) if isinstance(entry, str)} From 161fd70841754bed0f7eb1d5e92ed45471cdb1ce Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:18:21 -0400 Subject: [PATCH 3/5] update-shape.py: `rewrite_manifest` asks `_shape_block_bounds` where the `shape:` block is and then only edits inside it, so finding the block and rewriting it are no longer interleaved in one body Sonar scored at 16 (#133) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SonarCloud's `python:S3776` scored `rewrite_manifest` at 16 against a limit of 15. Two scans located the block — one for the `shape:` key, one for the next top-level key after it — with the `update-manifest-no-shape` refusal between them, and only then did the rewrite begin. Finding the block is a different question from editing it, and the answer is what SCOPES the edit: `commit:` and `tree_sha256:` are spelled the same way in every pin a manifest carries, so a rewrite that ran past the end of this block would move a leg's pin while claiming to move the shape's. `_shape_block_bounds` now answers it and raises the one refusal that belongs to it, because a bounds function with no bounds to return has nothing else to do. The end of the block is unchanged in every case: the next non-blank unindented line, or the end of the file when `shape:` is the last block a scaffold wrote. `rewrite_manifest` keeps the rewrite, the `seen` counts and the `update-manifest-unrecognised` refusal, all byte-identical. rewrite_manifest 15 -> 7; _shape_block_bounds 8 (new). (complexipy's numbers; Sonar scored the original 16.) Proven by tests/test_update_shape.py, tests/test_update_shape_add.py, tests/test_commit_trailers.py and tests/test_family.py: 252 passed. The family suite is in that list because a holder mirrors the same pin into `family.yaml` through this function. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Fable 5.1 --- update-shape.py | 51 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/update-shape.py b/update-shape.py index 1b48a154..ba6cd788 100755 --- a/update-shape.py +++ b/update-shape.py @@ -855,24 +855,7 @@ def rewrite_manifest(text: str, commit: str, tree: str, kind: Kind) -> str: pin while claiming to move the shape's. """ lines = text.splitlines() - start = None - for index, line in enumerate(lines): - if line.rstrip() == "shape:": - start = index - break - if start is None: - raise Refusal( - "update-manifest-no-shape", - f"{kind.manifest} has no `shape:` block, so there is nothing to " - "mirror the pin into", - f"Remediation: `{Path(kind.manifest_validator).name}` requires " - "one; add it, or re-scaffold.") - end = len(lines) - for index in range(start + 1, len(lines)): - line = lines[index] - if line.strip() and not line.startswith((" ", "\t")): - end = index - break + start, end = _shape_block_bounds(lines, kind) seen = {"commit": 0, "tree": 0} for index in range(start + 1, end): stripped = lines[index].strip() @@ -893,6 +876,38 @@ def rewrite_manifest(text: str, commit: str, tree: str, kind: Kind) -> str: return "\n".join(lines) + ("\n" if text.endswith("\n") else "") +def _shape_block_bounds(lines: list[str], kind: Kind) -> tuple[int, int]: + """Where the manifest's `shape:` block starts and ends, as line indices. + Split out of `rewrite_manifest` for #133. + + FINDING THE BLOCK IS A SEPARATE QUESTION FROM EDITING IT, and the answer + is what scopes the edit: `commit:` and `tree_sha256:` are spelled the same + way in every pin the manifest carries, so a rewrite that ran past the end + of this block would move a leg's pin while claiming to move the shape's. + The block ends at the next line that is non-blank and not indented — the + next top-level key — or at the end of the file, which is the `shape:` + block a scaffold writes last. No `shape:` block at all is the one + refusal, raised here because there is nothing to return. + """ + start = None + for index, line in enumerate(lines): + if line.rstrip() == "shape:": + start = index + break + if start is None: + raise Refusal( + "update-manifest-no-shape", + f"{kind.manifest} has no `shape:` block, so there is nothing to " + "mirror the pin into", + f"Remediation: `{Path(kind.manifest_validator).name}` requires " + "one; add it, or re-scaffold.") + for index in range(start + 1, len(lines)): + line = lines[index] + if line.strip() and not line.startswith((" ", "\t")): + return start, index + return start, len(lines) + + # --------------------------------------------------------------------------- # Reporting # --------------------------------------------------------------------------- From 184becb3988e4053af0c5cedc4f6e3496b61982e Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:30:21 -0400 Subject: [PATCH 4/5] update-shape.py: `cmd_apply` hands each phase to a named helper and the rollback ledger to a `Rollback` object, so the body Sonar scored at 47 is the sequence of what `apply` does rather than every decision inside all of it (#133) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The function carried, in one body: the `--accept-local` unknown-path refusal, the `--add` notes, the nothing-to-do exit, the `update-conflict` and `update-local-drift` refusals, two nested closures that remembered every byte written, the copy/kept/added print loops, both pin rewrites, the validator run and its refusal, the branch/push/pull-request landing, and the two loops in the `except Refusal` arm that put the tree back. A reader who wanted to check one refusal read all of it, which is what a cognitive complexity of 47 is a statement about. What is here is the same code in the same order, moved: * `Rollback` is the two closures and the two undo loops as one object. It keeps the distinction they had — a file the root already owned is restored from its first bytes, a file `--add` created is deleted along with any directory made to hold it — and `undo` unwinds the creations in reverse so a directory is only considered after the file it was made for is gone. * `_refuse_unknown_accept_local`, `_refuse_conflicts`, `_refuse_local_drift` and `_refuse_red_validators` are one refusal each, asked in the order they were asked in, with every id, message and remediation byte-identical. The reasons those orderings are deliberate — a mistyped path reported as a mistyped path, `both` alone having an accepted exit — moved with them. * `_note_untaken_additions`, `_nothing_to_do`, `_write_copies`, `_repin` and `_land` are the report, the exit test, the write pass, the re-pin and the three optional acts after a clean rewrite. `cmd_apply` keeps the sequence, the `try`/`except`/`finally` and the two `return` values, so what it reads as now is the order in which `apply` does things. Nothing moved across the boundary `scripts/shape_materialize.py` shares: `commit_on_branch` and the `--trailer` path (#112) are untouched, and `_land` only calls them. complexipy 8.0.1 scores the function at 8, and every helper at 11 or less. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Fable 5.1 --- update-shape.py | 396 +++++++++++++++++++++++++++++++----------------- 1 file changed, 257 insertions(+), 139 deletions(-) diff --git a/update-shape.py b/update-shape.py index ba6cd788..f3442448 100755 --- a/update-shape.py +++ b/update-shape.py @@ -1219,164 +1219,282 @@ def commit_on_branch(root: Path, branch: str, paths: list[str], target: str, proc.stderr + proc.stdout) +class Rollback: + """Every byte `apply` writes, and how to put the tree back. + + ONE LEDGER, TWO KINDS OF WRITE, because the undo differs. A file the root + already had is restored from the original bytes kept here; a file `--add` + created has to be DELETED, along with any directory that had to be made to + hold it. `cmd_apply` carried both as closures over two of its own locals + and the undo as two loops in an `except` arm, which is a third of what + Sonar scored that function at; split out for #133, with the same writes in + the same order and the same undo. + """ + + def __init__(self) -> None: + self.written: dict[Path, bytes] = {} + self.created: list[Path] = [] + + def write(self, path: Path, data: bytes) -> None: + """Overwrite a file the root already has, keeping its first bytes. + + `setdefault` so a path written twice is still restored to what it held + before the FIRST write; nothing does that today, and a ledger that + remembered the second write would silently stop being a rollback. + """ + self.written.setdefault(path, path.read_bytes()) + path.write_bytes(data) + + def write_added(self, path: Path, data: bytes, executable: bool) -> None: + """A file the root does not have yet, remembered for the rollback. + + `write` cannot serve it: there are no original bytes to keep, so the + rollback has to DELETE what this made — including any directory it had + to create to put the file in. The mode is set the way `_materialize` + sets it, which is to chmod the executables and leave everything else to + the umask. + """ + for parent in reversed([p for p in path.parents if not p.exists()]): + parent.mkdir() + self.created.append(parent) + self.created.append(path) + path.write_bytes(data) + if executable: + path.chmod(0o755) + + def undo(self) -> None: + """Put the tree back exactly as it was, and leave nothing behind. + + Restores first, then unwinds the creations in reverse so a directory + is only considered after the file it was made for is gone. A directory + that is not empty is LEFT: something nobody here wrote is in it, and + removing that is not a rollback. + """ + for path, original in self.written.items(): + path.write_bytes(original) + for path in reversed(self.created): + if path.is_dir(): + if not any(path.iterdir()): + path.rmdir() + elif path.exists(): + path.unlink() + + +def _refuse_unknown_accept_local(rows: list[Row], accepted: set) -> None: + """`--accept-local` on a path the pin carries no row for. Split out of + `cmd_apply` for #133, and asked FIRST for the reason the remediation + gives: a mistyped path reported as a drift refusal sends a human looking + for drift that is not there. + """ + stray = accepted - {row.path for row in rows} + if stray: + raise Refusal( + "update-accept-local-unknown", + f"--accept-local names {sorted(stray)}, which the shape pin " + "does not carry a row for", + "Remediation: only a pinned copy can be accepted; a file with " + "no row is not a shape copy. Checked FIRST, before the drift " + "refusal, so a mistyped path is reported as a mistyped path.") + + +def _refuse_conflicts(rows: list[Row], accepted: set) -> None: + """The `update-conflict` refusal, split out of `cmd_apply` for #133. + + `both` ALONE has an accepted exit: `--accept-local ` on it means the + human already did the merge this refusal asks for, so that row is excluded + here and handled with LOCALLY_MODIFIED further down — never copied over, + and re-pinned from what is on disk. The other three conflict states + (upstream-removed, unmapped, copy-missing) have no such exit: there is no + local edit to trust a word about, so `--accept-local` naming one of those + is simply not among the paths this can ever exclude. + """ + conflicts = [row for row in rows + if row.is_conflict + and not (row.state == BOTH and row.path in accepted)] + if conflicts: + raise Refusal( + "update-conflict", + "these files changed on BOTH sides, or cannot be compared, and " + "a merge is a human's judgement rather than a byte copy:\n" + + "\n".join(f" {row.path}: {row.detail or row.state}" + for row in conflicts), + "Remediation: merge each one by hand (the upstream bytes are " + "`git show :` in a shape clone), commit " + "it, then re-run this with --accept-local on that path so the " + "row is recomputed from what you merged.") + + +def _refuse_local_drift(rows: list[Row], accepted: set) -> None: + """The `update-local-drift` refusal, split out of `cmd_apply` for #133: + a file the project edited since the pin, which nobody has said to accept. + """ + unaccepted = [row for row in rows + if row.state == LOCALLY_MODIFIED and row.path not in accepted] + if unaccepted: + raise Refusal( + "update-local-drift", + "these files were edited in this project since the pin:\n" + + "\n".join(f" {row.path}" for row in unaccepted) + + "\n Re-pinning them from the root's own bytes would make " + "the drift `validate-pins.py` reports today invisible, " + "which is the fork recorded as the standard.", + "Remediation: revert each edit, or carry it upstream, or — " + "having decided the project keeps it — re-run with " + + " ".join(f"--accept-local {row.path}" for row in unaccepted)) + + +def _note_untaken_additions(additions: list[Addition], + taking: list[Addition]) -> None: + """One `note` line per `upstream-added` path `--add` did not name. Split + out of `cmd_apply` for #133. + + Printed before the nothing-to-do exit and before any refusal, because it + is the report rather than the act: a run that then refuses has still told + the human which files the standard has gained and what to pass to take + one. An addition the root already HAS a file at is silent here for the + reason `Addition` gives. + """ + for add in additions: + if not add.present and add not in taking: + print(f" note {add.path} is new upstream; not added; " + f"pass --add {add.path}") + + +def _nothing_to_do(rows: list[Row], pinned: str, target: str, + taking: list[Addition]) -> bool: + """Is there no write to make at all? Split out of `cmd_apply` for #133. + + All three have to hold: no copied file differs, the pin already names the + target, and `--add` named nothing. A pin that names a different commit is + a write even when every file matches, which is the re-pin `check` reports + as "`apply` would move the pin alone". + """ + return (all(row.state == UNCHANGED for row in rows) + and pinned == target and not taking) + + +def _write_copies(root: Path, rows: list[Row], taking: list[Addition], + ledger: Rollback) -> list[Row]: + """Write every byte this run copies, and say what happened to each row. + Split out of `cmd_apply` for #133. Returns the rows actually re-copied. + + THE ORDER IS THE REPORT'S ORDER: the copies, then what was kept and why, + then the additions. `rows` GROWS here — an `--add` row is appended rather + than inserted, so the pin keeps the order the scaffold wrote it in and the + new row reads as what it is: a file this project took later, by name. + """ + copied = [row for row in rows if row.state == UPSTREAM_CHANGED] + for row in copied: + ledger.write(root / row.path, row.target_bytes or b"") + print(f" copied {row.path}") + for row in rows: + if row.state in (LOCALLY_MODIFIED, BOTH): + print(f" kept {row.path} (local bytes; row recomputed " + "from them because --accept-local named it)") + elif row.state == ALREADY_AT_TARGET: + print(f" kept {row.path} (already the target's bytes; " + "the row was stale)") + for add in taking: + ledger.write_added(root / add.path, add.target_bytes or b"", + add.executable) + rows.append(add) + print(f" added {add.path} (new upstream file, copied from " + f"{add.source}; its row is appended to the pin)") + return copied + + +def _repin(root: Path, kind: Kind, ledger: Rollback, rows: list[Row], + pinned: str, target: str, target_tree: str) -> None: + """Rewrite `contracts/shape-pin.yaml` and the kind's manifest onto the + target, through the ledger so both roll back. Split out of `cmd_apply` + for #133; the two rewriters are where the refusals about an unrecognised + pin or manifest live. + """ + pin_path = root / "contracts" / "shape-pin.yaml" + manifest_path = root / kind.manifest + ledger.write(pin_path, rewrite_shape_pin( + pin_path.read_text(encoding="utf-8"), target, target_tree, rows + ).encode("utf-8")) + ledger.write(manifest_path, rewrite_manifest( + manifest_path.read_text(encoding="utf-8"), target, target_tree, kind + ).encode("utf-8")) + print(f" re-pinned contracts/shape-pin.yaml and {kind.manifest} " + f"{pinned[:12]} -> {target[:12]}") + + +def _refuse_red_validators(root: Path, kind: Kind) -> None: + """Run the root's OWN validators over what was just written, and refuse + if either goes red. Split out of `cmd_apply` for #133; the rollback is + the caller's `except Refusal` arm, which is why this only has to raise. + """ + print(f"\nthe {kind.name}'s own validators") + failed = [name for name in kind.validators + if run_validator(root, name) != 0] + if failed: + raise Refusal( + "update-validators-red", + "after the rewrite, " + " and ".join(failed) + " refused. " + "Every byte this command wrote has been rolled back; the tree " + "is exactly as it was.", + "Remediation: read the finding above. A tool that left a " + "project red would have moved the cost from this command to " + "somebody else's pull request.") + + +def _land(root: Path, args, paths: list[str], added: list[str], target: str, + upstream: Upstream, kind: Kind, count: int) -> None: + """The three optional acts after a clean rewrite — commit, push, open a + pull request — each done only because a flag asked for it. Split out of + `cmd_apply` for #133. + + Nested the way the flags are: `--push` and `--pr` have nothing to act on + without `--branch`, which `main` refuses before this is ever reached. + """ + if not args.branch: + return + commit_on_branch(root, args.branch, paths, target, upstream, + count, kind, added, args.trailer) + print(f"\n committed on {args.branch}: " + ", ".join(paths)) + if args.push: + run(["git", "push", "-q", "-u", "origin", args.branch], cwd=root) + print(f" pushed {args.branch} to origin") + if args.pr: + open_pull_request(root, args.branch, target, upstream, kind) + + def cmd_apply(args) -> int: # `pin` (the raw mapping `read_pin` returned) is not read again below: # `pinned` is the commit string this command compares and writes back. root, _, rows, additions, upstream, pinned, target, kind = prepare(args) - written: dict[Path, bytes] = {} - created: list[Path] = [] + ledger = Rollback() try: accepted = {str(path) for path in (args.accept_local or [])} - stray = accepted - {row.path for row in rows} - if stray: - raise Refusal( - "update-accept-local-unknown", - f"--accept-local names {sorted(stray)}, which the shape pin " - "does not carry a row for", - "Remediation: only a pinned copy can be accepted; a file with " - "no row is not a shape copy. Checked FIRST, before the drift " - "refusal, so a mistyped path is reported as a mistyped path.") + _refuse_unknown_accept_local(rows, accepted) taking = resolve_additions(rows, additions, [str(path) for path in (args.add or [])], target) - for add in additions: - if not add.present and add not in taking: - print(f" note {add.path} is new upstream; not added; " - f"pass --add {add.path}") - if all(row.state == UNCHANGED for row in rows) and pinned == target \ - and not taking: + _note_untaken_additions(additions, taking) + if _nothing_to_do(rows, pinned, target, taking): print(f"nothing to do: every copied file matches " f"{upstream.repository} @ {target[:12]}, which the pin " "already names") return 0 - # `both` ALONE has an accepted exit: `--accept-local ` on it - # means the human already did the merge `update-conflict` below asks - # for, so that row is excluded here and handled with LOCALLY_MODIFIED - # further down — never copied over, and re-pinned from what is on - # disk. The other three conflict states (upstream-removed, unmapped, - # copy-missing) have no such exit: there is no local edit to trust a - # word about, so `--accept-local` naming one of those is simply not - # among the paths this can ever exclude. - conflicts = [row for row in rows - if row.is_conflict - and not (row.state == BOTH and row.path in accepted)] - if conflicts: - raise Refusal( - "update-conflict", - "these files changed on BOTH sides, or cannot be compared, and " - "a merge is a human's judgement rather than a byte copy:\n" - + "\n".join(f" {row.path}: {row.detail or row.state}" - for row in conflicts), - "Remediation: merge each one by hand (the upstream bytes are " - "`git show :` in a shape clone), commit " - "it, then re-run this with --accept-local on that path so the " - "row is recomputed from what you merged.") - unaccepted = [row for row in rows - if row.state == LOCALLY_MODIFIED and row.path not in accepted] - if unaccepted: - raise Refusal( - "update-local-drift", - "these files were edited in this project since the pin:\n" - + "\n".join(f" {row.path}" for row in unaccepted) - + "\n Re-pinning them from the root's own bytes would make " - "the drift `validate-pins.py` reports today invisible, " - "which is the fork recorded as the standard.", - "Remediation: revert each edit, or carry it upstream, or — " - "having decided the project keeps it — re-run with " - + " ".join(f"--accept-local {row.path}" for row in unaccepted)) + _refuse_conflicts(rows, accepted) + _refuse_local_drift(rows, accepted) confirm(args, rows, taking, target) target_tree = upstream.tree_sha256(target) - pin_path = root / "contracts" / "shape-pin.yaml" - manifest_path = root / kind.manifest - - def write(path: Path, data: bytes) -> None: - written.setdefault(path, path.read_bytes()) - path.write_bytes(data) - - def write_added(path: Path, data: bytes, executable: bool) -> None: - """A file the root does not have yet, remembered for the rollback. - - `write` cannot serve it: there are no original bytes to keep, so - the rollback has to DELETE what this made — including any directory - it had to create to put the file in. The mode is set the way - `_materialize` sets it, which is to chmod the executables and leave - everything else to the umask. - """ - for parent in reversed([p for p in path.parents if not p.exists()]): - parent.mkdir() - created.append(parent) - created.append(path) - path.write_bytes(data) - if executable: - path.chmod(0o755) - - copied = [row for row in rows if row.state == UPSTREAM_CHANGED] - for row in copied: - write(root / row.path, row.target_bytes or b"") - print(f" copied {row.path}") - for row in rows: - if row.state in (LOCALLY_MODIFIED, BOTH): - print(f" kept {row.path} (local bytes; row recomputed " - "from them because --accept-local named it)") - elif row.state == ALREADY_AT_TARGET: - print(f" kept {row.path} (already the target's bytes; " - "the row was stale)") - for add in taking: - write_added(root / add.path, add.target_bytes or b"", - add.executable) - # Appended, so the pin's existing rows keep the order the scaffold - # wrote them in and the new one reads as what it is: a file this - # project took later, by name. - rows.append(add) - print(f" added {add.path} (new upstream file, copied from " - f"{add.source}; its row is appended to the pin)") - write(pin_path, rewrite_shape_pin( - pin_path.read_text(encoding="utf-8"), target, target_tree, rows - ).encode("utf-8")) - write(manifest_path, rewrite_manifest( - manifest_path.read_text(encoding="utf-8"), target, target_tree, kind - ).encode("utf-8")) - print(f" re-pinned contracts/shape-pin.yaml and {kind.manifest} " - f"{pinned[:12]} -> {target[:12]}") - - print(f"\nthe {kind.name}'s own validators") - failed = [name for name in kind.validators - if run_validator(root, name) != 0] - if failed: - raise Refusal( - "update-validators-red", - "after the rewrite, " + " and ".join(failed) + " refused. " - "Every byte this command wrote has been rolled back; the tree " - "is exactly as it was.", - "Remediation: read the finding above. A tool that left a " - "project red would have moved the cost from this command to " - "somebody else's pull request.") - - paths = sorted({row.path for row in copied} - | {add.path for add in taking} + copied = _write_copies(root, rows, taking, ledger) + _repin(root, kind, ledger, rows, pinned, target, target_tree) + _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}) - if args.branch: - commit_on_branch(root, args.branch, paths, target, upstream, - len(copied), kind, [add.path for add in taking], - args.trailer) - print(f"\n committed on {args.branch}: " + ", ".join(paths)) - if args.push: - run(["git", "push", "-q", "-u", "origin", args.branch], cwd=root) - print(f" pushed {args.branch} to origin") - if args.pr: - open_pull_request(root, args.branch, target, upstream, kind) + _land(root, args, paths, added, target, upstream, kind, len(copied)) print() - next_line(root, args, paths, target, [add.path for add in taking]) + next_line(root, args, paths, target, added) return 0 except Refusal: - for path, original in written.items(): - path.write_bytes(original) - for path in reversed(created): - if path.is_dir(): - if not any(path.iterdir()): - path.rmdir() - elif path.exists(): - path.unlink() + ledger.undo() raise except CommandFailed as exc: print(exc.loudly("committing the re-synced shape"), file=sys.stderr) From 4f302ec63cc618372a82471f7caf816bb91e9aca Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Sat, 12 Sep 2026 10:34:40 -0400 Subject: [PATCH 5/5] update-shape.py: the docstrings on `_classify_row` and `_row_verdict` say that the PINNED blob is the one byte-string allowed to be absent, so neither claims a contract the code does not have (#133) Copilot read the three new docstrings against the code on this branch and found that two of them overstate what the split guarantees. It is right, and the fix is the prose: nothing here changes a statement, and the file's AST is identical to the previous commit's once docstrings are stripped from both. * `_classify_row` said its three early exits cover "one of the four byte-strings" being missing, and that `_row_verdict` is therefore "never reached holding a hole". Three of the four are covered; the PINNED blob is not. `upstream.blob(pinned, source)` returns `None` for a source the standard did not carry at the pinned commit, and that value is passed straight on. The docstring now says which three the exits are, and that the pinned blob is the deliberate exception. * `_row_verdict` said it is "reached only for a row whose four byte-strings all read", which is the same overstatement from the other side, and its very next line is `pinned_bytes is None or ...`. It now says what a `None` pinned blob MEANS to the ladder: `upstream_changed` true and `verbatim_at_pin` false, because a copy cannot be a verbatim copy of a blob that was never there. * `Rollback`'s docstring compared the moved code to "a third of what Sonar scored that function at" without naming the score, which read to a reviewer as a sentence with a number missing from it. It names the 47. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Fable 5.1 --- update-shape.py | 43 +++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/update-shape.py b/update-shape.py index f3442448..139195e0 100755 --- a/update-shape.py +++ b/update-shape.py @@ -453,15 +453,18 @@ def _classify_row(root: Path, row: Row, upstream: Upstream, pinned: str, target: str, kind: Kind) -> None: """One pinned row's verdict, split out of `classify` for #133. - THE THREE EXITS HERE ARE THE ONES WHERE THE COMPARISON CANNOT BE MADE AT - ALL, because one of the four byte-strings `classify` names does not - exist: the root has no file at this path, no upstream path corresponds to - it, or the upstream has none at the target commit. Each is a decision for - a human rather than a difference this tool can act on, so `_row_verdict` - below is never reached holding a hole. The reads are left in the order - they were written in — the root's bytes, then the pinned blob, then the - target blob — because `Upstream.blob` shells out to git and a reordering - would change what a failing run prints. + THE THREE EXITS HERE ARE THE ONES WHERE THERE IS NOTHING TO COMPARE: the + root has no file at this path, no upstream path corresponds to it, or the + upstream has none at the target commit. Each is a decision for a human + rather than a difference this tool can act on, so `_row_verdict` below is + only ever reached holding the root's bytes and the target's bytes for + certain. THE PINNED BLOB IS THE EXCEPTION and is passed on as `None` when + the standard carried no such source at the pinned commit: that is not a + hole in the comparison but an upstream change, and the ladder reads it as + one. The reads are left in the order they were written in — the root's + bytes, then the pinned blob, then the target blob — because + `Upstream.blob` shells out to git and a reordering would change what a + failing run prints. """ target_path = root / row.path if not target_path.is_file(): @@ -495,12 +498,16 @@ def _row_verdict(row: Row, source: str, here: bytes, local_digest: str) -> None: """The verdict ladder itself, split out of `classify` for #133. - Reached only for a row whose four byte-strings all read, so this is the - ladder and nothing else: the three derived booleans the parent's - docstring argues for, and the first branch that matches. The order of the - branches is the order of the argument — already at the target, then the - two ways to be a `both`, then the two single-sided answers — and moving - one would change a verdict. + Reached with the root's bytes and the target's bytes both in hand, so + this is the ladder and nothing else: the three derived booleans the + parent's docstring argues for, and the first branch that matches. + `pinned_bytes` is the one that may be `None` — the standard carried no + such source at the pinned commit — and that is deliberately not treated + as a missing comparison: it makes `upstream_changed` true, and it makes + `verbatim_at_pin` false, because a copy cannot be a verbatim copy of a + blob that was never there. The order of the branches is the order of the + argument — already at the target, then the two ways to be a `both`, then + the two single-sided answers — and moving one would change a verdict. """ locally_modified = local_digest != row.recorded upstream_changed = pinned_bytes is None or pinned_bytes != target_bytes @@ -1226,9 +1233,9 @@ class Rollback: already had is restored from the original bytes kept here; a file `--add` created has to be DELETED, along with any directory that had to be made to hold it. `cmd_apply` carried both as closures over two of its own locals - and the undo as two loops in an `except` arm, which is a third of what - Sonar scored that function at; split out for #133, with the same writes in - the same order and the same undo. + and the undo as two loops in an `except` arm — together about a third of + the 47 Sonar scored that function at. Split out for #133, with the same + writes in the same order and the same undo. """ def __init__(self) -> None: