From 6c540dd36cc97b8ce1fbf2693a0412ec63f483c3 Mon Sep 17 00:00:00 2001 From: "Eric A. Litman" Date: Tue, 8 Sep 2026 17:37:27 -0400 Subject: [PATCH 1/2] fix(pstack): make the upstream merge lever refuse drift and report unsafe rows The Gavel panel review of PR #60 raised five defects in the lever. It now refuses to run unless HEAD is the audited port commit and every mapped path is clean, reports excluded and non-regular entries instead of crashing on them, treats an already-matching addition as a no-op, writes its three-way inputs to a temporary directory instead of sibling files, and applies the upstream executable bit. upstream-merge-probe.py reproduces each case plus the real 0.15.0 range. Against the previous script it fails all eight checks, six on behavior and two only on the summary wording; against this one all eight pass. Co-Authored-By: Claude Fable 5.1 --- scripts/upstream-merge-probe.py | 119 ++++++++++++++++++++++++++++++++ scripts/upstream-merge.py | 96 ++++++++++++++++++-------- 2 files changed, 187 insertions(+), 28 deletions(-) create mode 100755 scripts/upstream-merge-probe.py diff --git a/scripts/upstream-merge-probe.py b/scripts/upstream-merge-probe.py new file mode 100755 index 0000000..38ae35f --- /dev/null +++ b/scripts/upstream-merge-probe.py @@ -0,0 +1,119 @@ +#!/usr/bin/env python3 +"""Prove scripts/upstream-merge.py refuses unsafe runs and applies safe ones. + +Builds throwaway worktrees at the audited port commit, feeds the real audit +and relabeled variants through the merge, and asserts what changed on disk. + + python3 scripts/upstream-audit.py --port --upstream > audit.json + python3 scripts/upstream-merge-probe.py audit.json +""" +import copy +import json +import os +import shutil +import subprocess +import sys +import tempfile + +ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +MERGE = os.path.join(ROOT, "scripts", "upstream-merge.py") + + +def sh(*args, cwd, check=True): + return subprocess.run(args, cwd=cwd, capture_output=True, text=True, check=check) + + +def worktree(commit): + path = tempfile.mkdtemp(prefix="merge-probe-") + os.rmdir(path) + sh("git", "worktree", "add", "--detach", "-q", path, commit, cwd=ROOT) + return path + + +def run(audit, tree): + audit_path = os.path.join(tree, "probe-audit.json") + json.dump(audit, open(audit_path, "w")) + result = sh("python3", MERGE, audit_path, cwd=tree, check=False) + os.remove(audit_path) + return result.returncode, result.stdout + result.stderr + + +def changed(tree): + return sh("git", "status", "--porcelain", "--untracked-files=all", cwd=tree).stdout.strip() + + +def check(name, condition, detail=""): + print(f"{'ok' if condition else 'FAIL'}: {name}{'' if condition else ' ' + detail}") + return condition + + +def main(audit_path): + audit = json.load(open(audit_path)) + port = audit["port_commit"] + results = [] + trees = [] + try: + tree = worktree(port); trees.append(tree) + code, out = run(audit, tree) + results.append(check("real range merges (exit 1 for hand review)", code == 1 and "verbatim 24, clean merge 32, needs review 32, removed 2" in out, out.splitlines()[0] if out else "")) + + tree = worktree(f"{port}~1"); trees.append(tree) + code, out = run(audit, tree) + results.append(check("stale HEAD refused, nothing written", code == 2 and changed(tree) == "", out)) + + tree = worktree(port); trees.append(tree) + dirty = next(c["port_path"] for c in audit["changes"] if c["comparison"] == "unchanged-since-base") + open(os.path.join(tree, dirty), "a").write("\nlocal edit\n") + code, out = run(audit, tree) + results.append(check("dirty mapped path refused, edit kept", code == 2 and "local edit" in open(os.path.join(tree, dirty)).read(), out)) + + tree = worktree(port); trees.append(tree) + variant = copy.deepcopy(audit) + variant["changes"] = [c for c in variant["changes"] if c["change"] != "modify"] + for c in variant["changes"]: + c["comparison"] = "port-diverged-review" + code, out = run(variant, tree) + results.append(check("diverged add/delete reported, nothing written", code == 1 and changed(tree) == "" and out.count("review upstream") == len(variant["changes"]), out)) + + tree = worktree(port); trees.append(tree) + variant = copy.deepcopy(audit) + row = copy.deepcopy(next(c for c in variant["changes"] if c["change"] == "modify" and c["port_path"])) + row["comparison"] = "absent-from-port-review-exclusion" + row["port_path"] = "plugins/pstack/skills/make-bot-ui/SKILL.md" + variant["changes"] = [row] + code, out = run(variant, tree) + results.append(check("excluded path reported without crash or directory", code == 1 and "excluded path" in out and not os.path.exists(os.path.join(tree, "plugins/pstack/skills/make-bot-ui")), out)) + + tree = worktree(port); trees.append(tree) + variant = copy.deepcopy(audit) + conflicted = next(c for c in variant["changes"] if c["comparison"] == "port-diverged-review" and c["change"] == "modify") + variant["changes"] = [conflicted] + sibling = os.path.join(tree, conflicted["port_path"] + ".upstream-base") + open(sibling, "w").write("keep me") + code, out = run(variant, tree) + results.append(check("sibling temp-name file untouched", os.path.exists(sibling) and open(sibling).read() == "keep me", out)) + + tree = worktree(port); trees.append(tree) + variant = copy.deepcopy(audit) + row = copy.deepcopy(next(c for c in variant["changes"] if c["comparison"] == "unchanged-since-base")) + row["target"]["mode"] = "100755" + variant["changes"] = [row] + code, out = run(variant, tree) + results.append(check("executable bit applied from target mode", code == 0 and os.access(os.path.join(tree, row["port_path"]), os.X_OK), out)) + + tree = worktree(port); trees.append(tree) + variant = copy.deepcopy(audit) + row = copy.deepcopy(next(c for c in variant["changes"] if c["change"] == "add")) + row["comparison"] = "already-matches-target" + variant["changes"] = [row] + code, out = run(variant, tree) + results.append(check("already-matching addition is a no-op", code == 0 and changed(tree) == "", out)) + finally: + for tree in trees: + sh("git", "worktree", "remove", "--force", tree, cwd=ROOT, check=False) + shutil.rmtree(tree, ignore_errors=True) + return 0 if all(results) else 1 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1])) diff --git a/scripts/upstream-merge.py b/scripts/upstream-merge.py index 4851a49..52a0017 100755 --- a/scripts/upstream-merge.py +++ b/scripts/upstream-merge.py @@ -1,11 +1,12 @@ #!/usr/bin/env python3 """Apply an upstream pstack range to the port tree mechanically. -Reads the JSON that scripts/upstream-audit.py prints. For each mapped -modification it either checks out the upstream blob (the port still matches -the old upstream blob) or runs a three-way `git merge-file` in place, leaving -conflict markers for the hand pass. Additions are copied; deletions are -removed. Unmapped paths are listed and left alone. +Reads the JSON that scripts/upstream-audit.py prints. For each mapped path it +either checks out the upstream blob (the port still matches the old upstream +blob), runs a three-way `git merge-file` in place and leaves conflict markers +for the hand pass, or reports the row for review without touching the tree. +Refuses to run unless HEAD is the audited port commit and the mapped paths are +clean, so an audit never overwrites work done after it was taken. python3 scripts/upstream-audit.py --port --upstream > audit.json python3 scripts/upstream-merge.py audit.json @@ -14,55 +15,94 @@ import os import subprocess import sys +import tempfile + +REGULAR_MODES = {"100644", "100755"} + + +def git(*args, **kwargs): + return subprocess.run(["git", *args], capture_output=True, check=True, **kwargs).stdout def blob(rev, path): - return subprocess.run(["git", "show", f"{rev}:{path}"], capture_output=True, check=True).stdout + return git("show", f"{rev}:{path}") + + +def apply_mode(path, mode): + executable = mode == "100755" + current = os.stat(path).st_mode + os.chmod(path, (current | 0o111) if executable else (current & ~0o111)) + + +def refuse_drift(audit, ports): + head = git("rev-parse", "HEAD").decode().strip() + if head != audit["port_commit"]: + return f"HEAD {head[:12]} is not the audited port commit {audit['port_commit'][:12]}" + dirty = git("status", "--porcelain", "--untracked-files=all", "--", *ports).decode().strip() + if dirty: + return "mapped paths have local changes:\n" + dirty + return None + + +def merge_three_way(port, old, new): + with tempfile.TemporaryDirectory(prefix="upstream-merge-") as tmp: + base_path, new_path = os.path.join(tmp, "base"), os.path.join(tmp, "target") + open(base_path, "wb").write(old) + open(new_path, "wb").write(new) + return subprocess.run( + ["git", "merge-file", "-L", "port", "-L", "upstream-base", "-L", "upstream-target", port, base_path, new_path] + ).returncode def main(audit_path): audit = json.load(open(audit_path)) base, target = audit["upstream_base"], audit["upstream_target"] - verbatim, clean, conflicted, removed, skipped = [], [], [], [], [] + mapped = [c for c in audit["changes"] if c["port_path"] is not None] + drift = refuse_drift(audit, [c["port_path"] for c in mapped]) + if drift: + print(f"refusing to run: {drift}") + return 2 + verbatim, clean, review, removed, skipped = [], [], [], [], [] for change in audit["changes"]: - up, port = change["upstream_path"], change["port_path"] + up, port, comparison = change["upstream_path"], change["port_path"], change["comparison"] if port is None: skipped.append(up) continue - comparison = change["comparison"] + if comparison == "already-matches-target": + continue + if comparison == "absent-from-port-review-exclusion": + review.append((port, f"upstream {change['change']}d an excluded path")) + continue if change["change"] == "delete": - if comparison not in ("unchanged-since-base", "already-matches-target"): - conflicted.append((port, f"upstream deleted a port-edited file ({comparison})")) + if comparison != "unchanged-since-base": + review.append((port, f"upstream deleted a port-edited file ({comparison})")) continue - if os.path.exists(port): - os.remove(port) + os.remove(port) removed.append(port) continue + mode = change["target"]["mode"] + if mode not in REGULAR_MODES: + review.append((port, f"upstream entry mode {mode} is not a regular file")) + continue new = blob(target, up) if change["change"] == "add" and comparison != "upstream-addition": - conflicted.append((port, f"upstream added a path the port already has ({comparison})")) + review.append((port, f"upstream added a path the port already has ({comparison})")) continue if change["change"] == "add" or comparison == "unchanged-since-base": os.makedirs(os.path.dirname(port) or ".", exist_ok=True) open(port, "wb").write(new) + apply_mode(port, mode) verbatim.append(port) continue - old = blob(base, up) - tmp_base, tmp_new = port + ".upstream-base", port + ".upstream-target" - open(tmp_base, "wb").write(old) - open(tmp_new, "wb").write(new) - result = subprocess.run( - ["git", "merge-file", "-L", "port", "-L", "upstream-base", "-L", "upstream-target", port, tmp_base, tmp_new] - ) - os.remove(tmp_base) - os.remove(tmp_new) - (clean if result.returncode == 0 else conflicted).append((port, result.returncode)) - print(f"verbatim {len(verbatim)}, clean merge {len(clean)}, conflicted {len(conflicted)}, removed {len(removed)}, unmapped {len(skipped)}") - for port, why in conflicted: - print(f"conflict {why} {port}") + hunks = merge_three_way(port, blob(base, up), new) + apply_mode(port, mode) + (clean if hunks == 0 else review).append((port, f"{hunks} conflict hunks") if hunks else port) + print(f"verbatim {len(verbatim)}, clean merge {len(clean)}, needs review {len(review)}, removed {len(removed)}, unmapped {len(skipped)}") + for port, why in review: + print(f"review {why}: {port}") for path in skipped: print(f"unmapped {path}") - return 1 if conflicted else 0 + return 1 if review else 0 if __name__ == "__main__": From 22df5758cea785a02fe1dd8df3e59276be9d7078 Mon Sep 17 00:00:00 2001 From: "Eric A. Litman" Date: Tue, 8 Sep 2026 18:54:31 -0400 Subject: [PATCH 2/2] fix(pstack): stop the merge lever refusing empty audits and chmodding failed merges Two review findings bind on this repository. The documented invocation writes audit.json next to the script, and with no mapped path the drift check ran a repository-wide status and refused the run; it now skips the status check when nothing is mapped. git merge-file returns 255 on a binary file such as the mapped logo, which the lever counted as hunks and then chmodded; exit codes outside 0..127 are now reported and the file is left alone. Two probe checks cover both. Co-Authored-By: Claude Fable 5.1 --- scripts/upstream-merge-probe.py | 17 +++++++++++++++++ scripts/upstream-merge.py | 10 +++++++++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/scripts/upstream-merge-probe.py b/scripts/upstream-merge-probe.py index 38ae35f..5647258 100755 --- a/scripts/upstream-merge-probe.py +++ b/scripts/upstream-merge-probe.py @@ -108,6 +108,23 @@ def main(audit_path): variant["changes"] = [row] code, out = run(variant, tree) results.append(check("already-matching addition is a no-op", code == 0 and changed(tree) == "", out)) + tree = worktree(port); trees.append(tree) + variant = copy.deepcopy(audit) + variant["changes"] = [c for c in variant["changes"] if c["port_path"] is None] + open(os.path.join(tree, "audit.json"), "w").write("{}") + code, out = run(variant, tree) + results.append(check("all-unmapped audit is not refused by the untracked audit file", code == 0 and "unmapped" in out, out)) + + tree = worktree(port); trees.append(tree) + variant = copy.deepcopy(audit) + row = copy.deepcopy(next(c for c in variant["changes"] if c["comparison"] == "port-diverged-review" and c["change"] == "modify")) + row["target"]["mode"] = "100755" + variant["changes"] = [row] + binary = os.path.join(tree, row["port_path"]) + open(binary, "wb").write(b"\x00\x01 binary local copy\n") + sh("git", "update-index", "--assume-unchanged", row["port_path"], cwd=tree) + code, out = run(variant, tree) + results.append(check("binary merge-file failure reported, no mode change", "merge-file failed" in out and not os.access(binary, os.X_OK), out)) finally: for tree in trees: sh("git", "worktree", "remove", "--force", tree, cwd=ROOT, check=False) diff --git a/scripts/upstream-merge.py b/scripts/upstream-merge.py index 52a0017..8e42b57 100755 --- a/scripts/upstream-merge.py +++ b/scripts/upstream-merge.py @@ -38,6 +38,8 @@ def refuse_drift(audit, ports): head = git("rev-parse", "HEAD").decode().strip() if head != audit["port_commit"]: return f"HEAD {head[:12]} is not the audited port commit {audit['port_commit'][:12]}" + if not ports: + return None dirty = git("status", "--porcelain", "--untracked-files=all", "--", *ports).decode().strip() if dirty: return "mapped paths have local changes:\n" + dirty @@ -95,8 +97,14 @@ def main(audit_path): verbatim.append(port) continue hunks = merge_three_way(port, blob(base, up), new) + if hunks < 0 or hunks > 127: + review.append((port, f"git merge-file failed with status {hunks}")) + continue apply_mode(port, mode) - (clean if hunks == 0 else review).append((port, f"{hunks} conflict hunks") if hunks else port) + if hunks: + review.append((port, f"{hunks} conflict hunks")) + else: + clean.append(port) print(f"verbatim {len(verbatim)}, clean merge {len(clean)}, needs review {len(review)}, removed {len(removed)}, unmapped {len(skipped)}") for port, why in review: print(f"review {why}: {port}")