Merge pull request #98 from eduralph/feat/97-publish-on-accept-visibility

feat(publish): accept→publish by default + publish visibility

Author: eduralph

docs/07-publish-and-act.md

@@ -11,14 +11,22 @@ closes the PDCA loop by improving the *process*, not the contribution.
 ## Publish — contribute the accepted fix
 
 Once a bundle is `COMPLETE`, publish opens it as a **draft** PR on the target
-branch from the brief. In `pdca flow` this runs automatically on an accept; you
-can also run it standalone:
+branch from the brief. **Accept publishes by default everywhere** (#97): both
+`pdca flow` and a standalone `pdca signoff <id> --accept` run publish on an accept,
+so "approved" doesn't silently stay "unpublished"; pass `--no-publish` to opt out
+(then it's *deliberately* unpublished, not by accident). A publish that fails is
+**loud** — never swallowed. You can also run it standalone:
 
 ```bash
 pdca publish 11589              # open the draft PR
 pdca publish 11589 --dry-run    # print the git/gh plan without pushing
 ```
 
+`pdca status` (and bare `pdca`) shows each `COMPLETE` bundle's publish state —
+`[PR <url>]` when a PR was opened, `[unpublished]` when it wasn't (dry-run /
+no-target / failed / not-yet-run), `[close: no PR]` for a close/no-fix bundle — so
+an accepted-but-unpublished cycle is visible at a glance.
+
 It writes `publish.json` into the bundle and uses the project's PR conventions
 from INTEGRATION §8. For gramps that's a four-section body (Root cause / Fix /
 Verified against / Test), and — because 11589 touches an addon — an `## Affected

template/src/pdca_harness/cli.py

@@ -9,6 +9,7 @@
 
 import argparse
 import datetime
+import json
 import os
 import shutil
 import sys
@@ -106,6 +107,8 @@ def main(argv: list[str] | None = None) -> int:
                    help="discontinue — record §9, no transition, drop from the pending set")
     p_signoff.add_argument("--by", default="", help="who signed off")
     p_signoff.add_argument("--delta", default="", help="iteration delta note")
+    p_signoff.add_argument("--no-publish", action="store_true",
+                           help="don't publish-on-accept (record §9, stop at COMPLETE)")
 
     p_publish = sub.add_parser("publish", help="Check's closing work: contribute an accepted fix as a draft PR")
     p_publish.add_argument("issue_id")
@@ -278,13 +281,31 @@ def _status(cfg: Config, issue_id: str | None) -> int:
         if s == state.AWAITING_SIGNOFF:
             n = len(signoff.open_needs_human(d / "SUMMARY.md"))
             flag = "  [cheap: confirm]" if n == 0 else f"  [{n} NEEDS-HUMAN]"
+        if s == state.COMPLETE:  # publish visibility (#97): is the accepted fix actually out?
+            flag += _publish_flag(d)
         blocked = _blocked_by(cfg, d) if s != state.COMPLETE else []
         if blocked:
             flag += f"  [blocked-by: {', '.join(blocked)}]"
         print(f"{s:18}{d.name}{flag}")
     return 0
 
 
+def _publish_flag(d: Path) -> str:
+    """A COMPLETE bundle's publish state (#97): a real publish writes publish.json with the
+    PR url; absent ⇒ accepted-but-unpublished (dry-run / no-target / failed / not-yet-run),
+    so it's visible instead of looking published. A close/no-fix bundle has no patch to ship."""
+    pj = d / "publish.json"
+    if not pj.exists():
+        if not (d / "patch.diff").is_file() or not (d / "patch.diff").read_text(encoding="utf-8").strip():
+            return "  [close: no PR]"
+        return "  [unpublished]"
+    try:
+        url = json.loads(pj.read_text(encoding="utf-8")).get("pr_url")
+    except (ValueError, OSError):
+        return "  [published]"
+    return f"  [PR {url}]" if url else "  [published]"
+
+
 def _blocked_by(cfg: Config, d: Path) -> list[str]:
     """Declared `Depends on` ids of bundle ``d`` that are not yet COMPLETE (issue #36)."""
     bp = d / "brief.md"
@@ -416,6 +437,17 @@ def _signoff(cfg: Config, args: argparse.Namespace) -> int:
     # Apply the transition: accept freezes; iterate clears and re-runs the body.
     final = driver.run_issue(d, cfg)
     print(f"{final}\t{d}")
+
+    # Accept → publish by default, like `flow`'s closing step (#97): a standalone
+    # `signoff --accept` otherwise left bundles COMPLETE-but-unpublished with no signal.
+    # `--no-publish` opts out (then the bundle is deliberately, not silently, unpublished).
+    if action == "accept" and final == state.COMPLETE and not getattr(args, "no_publish", False):
+        rc = publish.publish(cfg, args.issue_id, dry_run=cfg.publisher.mode == "stub",
+                             by=args.by, skip_if_no_target=True)
+        if rc != 0:
+            print(f"  publish did not complete (rc {rc}) — {d.name} is COMPLETE but NOT "
+                  f"published; fix and re-run `pdca publish {args.issue_id}`.", file=sys.stderr)
+            return rc
     return 0
 
 

template/src/pdca_harness/flow.py

@@ -159,9 +159,13 @@ def flow(
     final = state.state(d)
     if do_publish and final == state.COMPLETE:
         # Closing step of Check. Dry-run when the publisher leaf is stubbed (offline
-        # rehearse / CI) so the flow never pushes without a live model.
-        publish.publish(cfg, issue_id, dry_run=cfg.publisher.mode == "stub",
-                        by=by, today=today, skip_if_no_target=True)
+        # rehearse / CI) so the flow never pushes without a live model. A real failure
+        # is LOUD (#97) — never silently leave a COMPLETE bundle unpublished.
+        rc = publish.publish(cfg, issue_id, dry_run=cfg.publisher.mode == "stub",
+                             by=by, today=today, skip_if_no_target=True)
+        if rc:
+            print(f"flow: issue_{issue_id} is COMPLETE but publish did not complete "
+                  f"(rc {rc}) — NOT published; run `pdca publish {issue_id}`.", file=sys.stderr)
     if do_act and final == state.COMPLETE:
         leaves.run_act(cfg, today)
     return final
@@ -388,10 +392,16 @@ def _drive_and_act(
         # must not abort the batch return / Act for the rest (testbed issue #3).
         for d in bundles:
             if state.state(d) == state.COMPLETE:
-                _isolate(d, "publish", lambda d=d: publish.publish(
+                rc = _isolate(d, "publish", lambda d=d: publish.publish(
                     cfg, d.name.removeprefix("issue_"),
                     dry_run=cfg.publisher.mode == "stub", by=by, today=today,
                     skip_if_no_target=True))
+                # rc != 0 (and not None — None means _isolate already logged an exception):
+                # a publish that returned failure must not pass silently (#97).
+                if rc not in (0, None):
+                    print(f"flow: {d.name} is COMPLETE but publish did not complete "
+                          f"(rc {rc}) — NOT published; run `pdca publish "
+                          f"{d.name.removeprefix('issue_')}`.", file=sys.stderr)
     if do_act and any(s == state.COMPLETE for s in results.values()):
         leaves.run_act(cfg, today)
     return results

template/tests/test_flow_slice.py

@@ -849,5 +849,79 @@ def test_prog_name_resolution(self) -> None:
             sys.argv = orig
 
 
+class PublishOnAccept(unittest.TestCase):
+    """Accept → publish by default + publish visibility (issue #97)."""
+
+    def setUp(self) -> None:
+        self.tmp = Path(tempfile.mkdtemp())
+        self.cfg = _stub_config(self.tmp)
+
+    def tearDown(self) -> None:
+        shutil.rmtree(self.tmp, ignore_errors=True)
+
+    def _accepted_ready(self, iid: str) -> Path:
+        d = self.cfg.bundle(iid)
+        leaves.do_plan(d, self.cfg)
+        driver.run_issue(d, self.cfg)  # → AWAITING_SIGNOFF (§6 open from the stub reviewer)
+        summ = d / "SUMMARY.md"
+        summ.write_text(summ.read_text().replace("- [ ]", "- [x]"), encoding="utf-8")  # clear §6
+        return d
+
+    def _accept_args(self, iid: str, no_publish: bool = False) -> SimpleNamespace:
+        return SimpleNamespace(issue_id=iid, accept=True, iterate_do=False,
+                               iterate_plan=False, discontinue=False, by="", delta="",
+                               no_publish=no_publish)
+
+    def test_accept_publishes_by_default(self) -> None:
+        from pdca_harness import publish
+        calls, orig = [], publish.publish
+        publish.publish = lambda cfg, iid, **kw: calls.append(iid) or 0
+        try:
+            self._accepted_ready("ACC")
+            self.assertEqual(cli._signoff(self.cfg, self._accept_args("ACC")), 0)
+        finally:
+            publish.publish = orig
+        self.assertEqual(calls, ["ACC"])  # standalone accept publishes (#97)
+
+    def test_no_publish_opts_out(self) -> None:
+        from pdca_harness import publish
+        calls, orig = [], publish.publish
+        publish.publish = lambda cfg, iid, **kw: calls.append(iid) or 0
+        try:
+            self._accepted_ready("NOP")
+            cli._signoff(self.cfg, self._accept_args("NOP", no_publish=True))
+        finally:
+            publish.publish = orig
+        self.assertEqual(calls, [])  # --no-publish ⇒ deliberately unpublished
+
+    def test_accept_publish_failure_is_loud(self) -> None:
+        import io
+        from contextlib import redirect_stderr
+        from pdca_harness import publish
+        orig = publish.publish
+        publish.publish = lambda cfg, iid, **kw: 1  # publish fails
+        try:
+            self._accepted_ready("FAILP")
+            buf = io.StringIO()
+            with redirect_stderr(buf):
+                rc = cli._signoff(self.cfg, self._accept_args("FAILP"))
+        finally:
+            publish.publish = orig
+        self.assertEqual(rc, 1)                       # failure surfaced as the return
+        self.assertIn("NOT", buf.getvalue())          # and printed loudly
+
+    def test_status_publish_flag(self) -> None:
+        d = self.cfg.bundle("ST")
+        d.mkdir(parents=True)
+        (d / "patch.diff").write_text("diff --git a/x b/x\n", encoding="utf-8")
+        self.assertEqual(cli._publish_flag(d), "  [unpublished]")  # no publish.json
+        (d / "publish.json").write_text('{"pr_url": "https://x/pr/1"}', encoding="utf-8")
+        self.assertEqual(cli._publish_flag(d), "  [PR https://x/pr/1]")
+        d2 = self.cfg.bundle("ST2")
+        d2.mkdir(parents=True)
+        (d2 / "patch.diff").write_text("", encoding="utf-8")  # close/no-fix → no PR expected
+        self.assertEqual(cli._publish_flag(d2), "  [close: no PR]")
+
+
 if __name__ == "__main__":
     unittest.main()