Merge pull request #159 from eduralph/feat/158-pdca-revert

feat(cli): pdca revert — undo a published contribution (revert PR or withdraw)

Author: eduralph

docs/07-publish-and-act.md

@@ -56,6 +56,13 @@ f1092ca results(46): record published GraphView import-safety cycle
 That's one full cycle, [steps 03–07](03-plan.md), from tracker issue to merged
 record.
 
+**Undoing one.** If a landed fix turns out wrong, `pdca revert <id>` undoes the
+contribution from its recorded `publish.json`: a **merged** PR gets a draft **revert PR**
+(reverse-applying the bundle's own `patch.diff` onto the base — no guessing the merge
+commit), and an **open** one is **withdrawn** (`gh pr close --delete-branch`). It matters
+most under the opt-in `wave_mode = "merge"`, where the harness lands waves itself; STOP
+discipline holds — the revert PR opens as a draft for you to merge (issue #158).
+
 ---
 
 ## Act — improve the process, not the contribution

template/src/pdca_harness/cli.py

@@ -16,7 +16,7 @@
 from pathlib import Path
 
 from . import (act, brief, driver, flow, gates, merged, publish, queue, revalidate,
-               signoff, state, waves)
+               revert, signoff, state, waves)
 from .config import Config
 
 
@@ -134,6 +134,12 @@ def main(argv: list[str] | None = None) -> int:
                            help="no tracker id yet: relax T4 to a flag, record id_pending (vs a magic #0000)")
     p_publish.add_argument("--by", default="", help="who published (recorded in publish.json)")
 
+    p_revert = sub.add_parser("revert",
+                              help="undo a published contribution: a revert PR if merged, else withdraw the PR (#158)")
+    p_revert.add_argument("issue_id")
+    p_revert.add_argument("--dry-run", action="store_true", help="print the git/gh plan without mutating anything")
+    p_revert.add_argument("--by", default="", help="who reverted (recorded in revert.json)")
+
     args = parser.parse_args(argv)
     # --rehearse (#87): a dry-run of the SAME control flow with stub leaves + stub gates
     # in an isolated bundle root — set before Config.load reads the env. setdefault so an
@@ -178,6 +184,8 @@ def main(argv: list[str] | None = None) -> int:
     if args.cmd == "publish":
         return publish.publish(cfg, args.issue_id, dry_run=args.dry_run,
                                open_pr=not args.no_pr, by=args.by, pending_id=args.no_issue)
+    if args.cmd == "revert":
+        return revert.revert(cfg, args.issue_id, dry_run=args.dry_run, by=args.by)
     return 2
 
 

template/src/pdca_harness/revert.py

@@ -0,0 +1,176 @@
+"""Revert — undo a published contribution (issue #158).
+
+``pdca revert <id>`` reads the bundle's recorded ``publish.json`` and undoes the
+contribution:
+
+- the PR is **MERGED** → open a **revert PR** that reverse-applies the bundle's own
+  ``patch.diff`` onto the base (``git apply --reverse``) — deterministic, no guessing the
+  merge commit or the ``-m`` mainline — pushed as a fresh **draft** PR.
+- the PR is **OPEN** (never landed) → **withdraw** it: ``gh pr close --delete-branch``.
+- the PR is already **CLOSED** → nothing to do.
+
+Records ``revert.json`` in the bundle. ``--dry-run`` prints the git/gh plan without
+mutating anything (it still reads the PR state). Fail-closed and loud, like publish/merge;
+STOP discipline holds — a revert PR opens as a draft for the human to merge. The mechanics
+are deterministic ``git``/``gh`` subprocesses (no model), reusing the publish helpers.
+"""
+
+from __future__ import annotations
+
+import datetime
+import json
+import shlex
+import subprocess
+import sys
+from pathlib import Path
+
+from . import publish
+from .config import Config
+
+REVERT_JSON = "revert.json"
+
+
+def revert(cfg: Config, issue_id: str, *, dry_run: bool = False, by: str = "",
+           today: str | None = None) -> int:
+    """Undo the bundle's published contribution; return a process code."""
+    d = cfg.bundle(issue_id)
+    today = today or datetime.date.today().isoformat()
+    rec = publish._publish_record(d)
+    pr_url = rec.get("pr_url") if rec else None
+    if not pr_url:
+        print(f"revert: {d.name} has no recorded PR (nothing published to revert)",
+              file=sys.stderr)
+        return 1
+    pr_state = _pr_state(pr_url)
+    if pr_state is None:
+        print(f"revert: could not read PR state for {pr_url}; aborting", file=sys.stderr)
+        return 1
+    if pr_state == "MERGED":
+        return _revert_merged(cfg, d, issue_id, rec, pr_url, dry_run=dry_run, by=by, today=today)
+    if pr_state == "OPEN":
+        # ``mode: "stacked"`` (Onto branch, #54) means the harness appended a commit to a
+        # PRE-EXISTING PR it did NOT create. Withdrawing it would `gh pr close
+        # --delete-branch` that collaborator's whole PR branch — never do that. (The merged
+        # path is still safe: it opens a *new* revert PR, leaving the original alone.)
+        if rec.get("mode") == "stacked":
+            print(f"revert: {d.name} was published as a commit onto an existing PR "
+                  f"({pr_url}, mode=stacked) the harness did not create — refusing to close "
+                  "it. Revert just that commit on the PR branch by hand.", file=sys.stderr)
+            return 1
+        return _withdraw(cfg, d, pr_url, dry_run=dry_run, by=by, today=today)
+    print(f"revert: {d.name}'s PR is {pr_state} — nothing to revert ({pr_url}).")
+    return 0
+
+
+def _pr_state(pr_url: str) -> str | None:
+    """The recorded PR's state via ``gh pr view`` (``MERGED`` / ``OPEN`` / ``CLOSED``), or
+    None on a gh failure (the caller aborts — never reverts blind)."""
+    r = subprocess.run(["gh", "pr", "view", str(pr_url), "--json", "state"],
+                       capture_output=True, text=True)
+    if r.returncode != 0:
+        print(r.stderr, file=sys.stderr)
+        return None
+    try:
+        return json.loads(r.stdout or "{}").get("state")
+    except ValueError:
+        return None
+
+
+def _commit_summary(d: Path, issue_id: str) -> str:
+    """The contribution's commit subject (for the revert title), or a fallback."""
+    msg = d / publish.COMMIT_MSG
+    if msg.is_file():
+        lines = msg.read_text(encoding="utf-8").splitlines()
+        if lines and lines[0].strip():
+            return lines[0].strip()
+    return f"contribution for {issue_id}"
+
+
+def _record(d: Path, rec: dict) -> None:
+    (d / REVERT_JSON).write_text(json.dumps(rec, indent=2) + "\n", encoding="utf-8")
+
+
+def _revert_merged(cfg: Config, d: Path, issue_id: str, pub: dict, pr_url: str, *,
+                   dry_run: bool, by: str, today: str) -> int:
+    """Open a draft revert PR that reverse-applies the bundle's patch.diff onto the base."""
+    patch = d / "patch.diff"
+    if not patch.is_file() or not patch.read_text(encoding="utf-8").strip():
+        print(f"revert: {d.name} has no patch.diff to reverse (close/no-fix had nothing to "
+              "land) — nothing to revert.", file=sys.stderr)
+        return 1
+    repo_spec = pub.get("repo", "")
+    base = pub.get("base", "")
+    repo = publish._checkout_path(cfg, repo_spec)
+    base_remote = cfg.base_remote
+    rev_branch = f"revert/{issue_id}"
+    summary = _commit_summary(d, issue_id)
+    git = lambda *a: ["git", "-C", str(repo), *a]
+    steps = [
+        git("fetch", base_remote),
+        git("checkout", "-B", rev_branch, f"{base_remote}/{base}"),
+        git("apply", "--reverse", str(patch.resolve())),
+        git("add", "--all"),
+        git("commit", "-s", "-m",
+            f"Revert: {summary}\n\nReverts the change contributed for {issue_id} ({pr_url})."),
+        git("push", "--force-with-lease", "-u", "origin", rev_branch),
+    ]
+    if dry_run:
+        print(f"revert --dry-run — {d.name}: open a draft revert PR on {repo_spec} "
+              f"({rev_branch} → {base}):")
+        for c in steps:
+            print("  " + " ".join(shlex.quote(x) for x in c))
+        print(f"  gh pr create --draft --repo {repo_spec} --base {base} "
+              f"--head <fork-owner>:{rev_branch} --title {shlex.quote('Revert: ' + summary)}")
+        return 0
+
+    rc = publish._check_repo(repo, repo_spec, required_remotes={base_remote, "origin"})
+    if rc != 0:
+        return rc
+    orig = publish._current_ref(repo)
+    stashed = publish._stash_worktree(repo)
+    try:
+        for c in steps:
+            print("→ " + " ".join(c[3:]))
+            if subprocess.run(c).returncode != 0:
+                print(f"revert: step failed: {' '.join(c)}", file=sys.stderr)
+                return 1
+    finally:
+        publish._restore_worktree(repo, orig, stashed)
+
+    head = f"{publish._fork_owner(repo) or repo_spec.split('/')[0]}:{rev_branch}"
+    pr_cmd = ["gh", "pr", "create", "--draft", "--repo", repo_spec, "--base", base,
+              "--head", head, "--title", f"Revert: {summary}",
+              "--body", f"Reverts the contribution for {issue_id} ({pr_url}) by "
+              f"reverse-applying the recorded patch onto `{base}`."]
+    r = subprocess.run(pr_cmd, capture_output=True, text=True)
+    if r.returncode != 0:
+        print(r.stderr, file=sys.stderr)
+        print("revert: branch pushed but `gh pr create` FAILED — open the revert PR by "
+              "hand. This is NOT done.", file=sys.stderr)
+        return 1
+    revert_pr = ((r.stdout or "").strip().splitlines() or [""])[-1]
+    _record(d, {"action": "revert-pr", "reverts": pr_url, "branch": rev_branch,
+                "revert_pr": revert_pr, "base": base, "repo": repo_spec,
+                "by": by or cfg.author or "unknown", "date": today})
+    print(f"\nDraft revert PR opened on {repo_spec} ({rev_branch} → {base}).\n  {revert_pr}")
+    print("  STOP: review CI, then mark it ready / merge yourself — the human's step.")
+    return 0
+
+
+def _withdraw(cfg: Config, d: Path, pr_url: str, *, dry_run: bool, by: str,
+              today: str) -> int:
+    """Withdraw an unmerged contribution: close the PR and delete its branch."""
+    cmd = ["gh", "pr", "close", str(pr_url), "--delete-branch"]
+    if dry_run:
+        print(f"revert --dry-run — {d.name}: withdraw the unmerged PR: {' '.join(cmd)}")
+        return 0
+    print(f"→ gh pr close {pr_url} --delete-branch")
+    r = subprocess.run(cmd, capture_output=True, text=True)
+    if r.returncode != 0:
+        print(r.stderr, file=sys.stderr)
+        print(f"revert: could not close {pr_url} — close it by hand.", file=sys.stderr)
+        return 1
+    _record(d, {"action": "withdraw", "reverts": pr_url,
+                "by": by or cfg.author or "unknown", "date": today})
+    print(f"Withdrew the unmerged PR {pr_url} (closed + branch deleted).")
+    return 0

template/tests/test_revert.py

@@ -0,0 +1,131 @@
+"""Slice for `pdca revert` (issue #158) — undo a published contribution.
+
+Routing by the recorded PR state: MERGED → a draft revert PR (reverse-apply patch.diff);
+OPEN → withdraw (`gh pr close --delete-branch`); CLOSED → no-op. Dry-run prints the plan
+and mutates nothing. `gh` state + subprocess are mocked — no network. Run from the project
+root:
+    PYTHONPATH=src python -m unittest discover -s tests
+"""
+
+from __future__ import annotations
+
+import io
+import json
+import shutil
+import tempfile
+import unittest
+from contextlib import redirect_stderr, redirect_stdout
+from pathlib import Path
+from types import SimpleNamespace
+from unittest import mock
+
+from pdca_harness import revert
+from pdca_harness.config import Config, LeafConfig
+
+
+def _cfg(root: Path) -> Config:
+    return Config(
+        root=root, bundle_root=root / "results", process_dir=root / "process",
+        templates_dir=root / "templates", default_branch="main", tracker_system="github",
+        tracker_url="", issue_id_example="#1",
+        builder=LeafConfig(mode="stub"), reviewer=LeafConfig(mode="stub"),
+        base_remote="origin", repo_checkouts={"org/repo": str(root / "repo")})
+
+
+class Revert(unittest.TestCase):
+    def setUp(self) -> None:
+        self.tmp = Path(tempfile.mkdtemp())
+        self.cfg = _cfg(self.tmp)
+
+    def tearDown(self) -> None:
+        shutil.rmtree(self.tmp, ignore_errors=True)
+
+    def _bundle(self, iid: str, *, pr_url: str | None = "https://gh/pr/1",
+                patch: str | None = "diff --git a/f.py b/f.py\n@@ -1 +1 @@\n-x\n+y\n") -> Path:
+        d = self.cfg.bundle(iid)
+        d.mkdir(parents=True)
+        if pr_url is not None:
+            (d / "publish.json").write_text(json.dumps(
+                {"pr_url": pr_url, "repo": "org/repo", "base": "main", "branch": f"fix/{iid}"}),
+                encoding="utf-8")
+        if patch is not None:
+            (d / "patch.diff").write_text(patch, encoding="utf-8")
+        (d / "commit-msg.txt").write_text("Fix the thing\n", encoding="utf-8")
+        return d
+
+    def test_no_publish_record_fails(self) -> None:
+        d = self.cfg.bundle("NP")
+        d.mkdir(parents=True)
+        with redirect_stderr(io.StringIO()):
+            self.assertEqual(revert.revert(self.cfg, "NP"), 1)
+
+    def test_merged_dry_run_plans_revert_pr(self) -> None:
+        self._bundle("M")
+        with mock.patch.object(revert, "_pr_state", return_value="MERGED"), \
+                mock.patch.object(revert.subprocess, "run") as run, \
+                redirect_stdout(io.StringIO()) as out:
+            rc = revert.revert(self.cfg, "M", dry_run=True)
+        self.assertEqual(rc, 0)
+        run.assert_not_called()                       # dry-run mutates nothing
+        self.assertIn("apply --reverse", out.getvalue())
+        self.assertIn("gh pr create", out.getvalue())
+
+    def test_open_dry_run_plans_withdraw(self) -> None:
+        self._bundle("O")
+        with mock.patch.object(revert, "_pr_state", return_value="OPEN"), \
+                mock.patch.object(revert.subprocess, "run") as run, \
+                redirect_stdout(io.StringIO()) as out:
+            rc = revert.revert(self.cfg, "O", dry_run=True)
+        self.assertEqual(rc, 0)
+        run.assert_not_called()
+        self.assertIn("gh pr close", out.getvalue())
+        self.assertIn("--delete-branch", out.getvalue())
+
+    def test_closed_pr_is_noop(self) -> None:
+        self._bundle("C")
+        with mock.patch.object(revert, "_pr_state", return_value="CLOSED"), \
+                redirect_stdout(io.StringIO()):
+            self.assertEqual(revert.revert(self.cfg, "C"), 0)
+
+    def test_merged_without_patch_fails(self) -> None:
+        self._bundle("MP", patch=None)
+        with mock.patch.object(revert, "_pr_state", return_value="MERGED"), \
+                redirect_stderr(io.StringIO()):
+            self.assertEqual(revert.revert(self.cfg, "MP"), 1)
+
+    def test_withdraw_real_closes_and_records(self) -> None:
+        d = self._bundle("W")
+        calls: list[list[str]] = []
+
+        def fake_run(cmd, **kw):
+            calls.append(cmd)
+            return SimpleNamespace(returncode=0, stdout="", stderr="")
+
+        with mock.patch.object(revert, "_pr_state", return_value="OPEN"), \
+                mock.patch.object(revert.subprocess, "run", side_effect=fake_run), \
+                redirect_stdout(io.StringIO()):
+            rc = revert.revert(self.cfg, "W")
+        self.assertEqual(rc, 0)
+        self.assertIn(["gh", "pr", "close", "https://gh/pr/1", "--delete-branch"], calls)
+        rec = json.loads((d / "revert.json").read_text(encoding="utf-8"))
+        self.assertEqual(rec["action"], "withdraw")
+        self.assertEqual(rec["reverts"], "https://gh/pr/1")
+
+    def test_open_stacked_pr_is_refused(self) -> None:
+        # mode="stacked" (Onto branch #54) = a commit on a PRE-EXISTING PR the harness did
+        # not create — revert must NOT close/delete it.
+        d = self._bundle("S")
+        pj = json.loads((d / "publish.json").read_text(encoding="utf-8"))
+        pj["mode"] = "stacked"
+        (d / "publish.json").write_text(json.dumps(pj), encoding="utf-8")
+        with mock.patch.object(revert, "_pr_state", return_value="OPEN"), \
+                mock.patch.object(revert.subprocess, "run") as run, \
+                redirect_stderr(io.StringIO()) as err:
+            rc = revert.revert(self.cfg, "S")
+        self.assertEqual(rc, 1)
+        run.assert_not_called()                  # never touched the collaborator's PR
+        self.assertIn("refusing to close", err.getvalue())
+
+
+if __name__ == "__main__":
+    unittest.main()