From 43e4319817cd33d64d64b1bb09109eef2c349836 Mon Sep 17 00:00:00 2001 From: istos Date: Thu, 30 Jul 2026 07:57:19 +0200 Subject: [PATCH] Surface PR merge conflicts on the review card The poller now reads GitHub's mergeable field alongside reviews and checks. A CONFLICTING PR drops any approved-green verdict (as changes-needed-by-you, not a CI failure), wears an alarm-coloured conflicts chip in the card's footer row, and narrates the flip in the ticker. GitHub computes mergeability lazily, so UNKNOWN keeps the previous reading instead of flapping the chip. The poll fold is now a pure function (_fold), so the verdict logic is testable without gh. Co-Authored-By: Claude Fable 5 --- AGENTS.md | 16 +++++-- manager/core/board.html | 4 ++ manager/core/github.py | 69 +++++++++++++++++++-------- tests/test_pr_conflicts.py | 95 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 162 insertions(+), 22 deletions(-) create mode 100644 tests/test_pr_conflicts.py diff --git a/AGENTS.md b/AGENTS.md index 6b7f70f..50a4849 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -285,10 +285,18 @@ review and line comment, addresses each point or says why not, commits, pushes so the PR updates, and appends a `## PR update` section to the task file. Then **◔ review PR** again, until it settles. -The board polls open PRs of review-stage cards (reviews + CI checks, every -60s — a plain thread in board.py, no agent involved, silent when review/ is -empty) and folds everything into one verdict — any changes-requested review -or failing check wins over any approval. Tool chips (CI, copilot, PR, drive) +The board polls open PRs of review-stage cards (reviews + CI checks + +GitHub's mergeable state, every 60s — a plain thread in board.py, no agent +involved, silent when review/ is empty) and folds everything into one +verdict — any changes-requested review or failing check wins over any +approval. A PR GitHub cannot merge cleanly wears an alarm-coloured +`conflicts` chip and counts as changes-needed-by-you (not a CI failure); +**↻ act on PR** resolves mechanical conflicts by merging main into the +branch — additively, never rebasing or force-pushing — in a dedicated +resolution commit, and refuses semantic ones, naming the collision for a +human to settle. GitHub computes mergeability lazily, so an UNKNOWN +reading keeps the chip's last state rather than flapping. +Tool chips (CI, copilot, PR, drive) are destinations, not statuses: they live in the card's footer row, never squeezed into the author row — `CI ✓` (pine), `CI ✕` (terracotta), `◌` while in flight, with hover actions staying in the status pill's slot. The card wears it in the design diff --git a/manager/core/board.html b/manager/core/board.html index 23c3c2c..aba5e90 100644 --- a/manager/core/board.html +++ b/manager/core/board.html @@ -881,6 +881,10 @@ function cardFor(task) { chips.push({ label: 'CI', glyph: { pass: '✓', fail: '✕', running: '◌' }[prState.ci], cls: { pass: 'ok', fail: 'bad', running: 'accent' }[prState.ci], title: detail }); } + if (prState && prState.conflicts) { + chips.push({ label: 'conflicts', glyph: '✕', cls: 'bad', + title: 'GitHub cannot merge this into main — ↻ act on PR can attempt a resolution merge' }); + } if (prState && prState.copilot) { chips.push({ label: 'copilot', glyph: { asked: '◌', approved: '✓', changes: '✕', commented: '·' }[prState.copilot], diff --git a/manager/core/github.py b/manager/core/github.py index ad44e03..b3c80c9 100644 --- a/manager/core/github.py +++ b/manager/core/github.py @@ -191,16 +191,21 @@ def _is_copilot(login) -> bool: return "copilot" in str(login or "").lower() -def _poll_pr(filename: str, url: str) -> None: - number = url.rstrip("/").rsplit("/", 1)[-1] - result = _run([config.GH_BIN, "pr", "view", number, - "--json", "reviews,reviewRequests,statusCheckRollup,state"], timeout=60) - if result.returncode != 0: - return - try: - data = json.loads(result.stdout) - except json.JSONDecodeError: - return +def _conflict_state(data: dict, prev: dict) -> bool | None: + """GitHub computes mergeability lazily: UNKNOWN means "not computed + yet", never "fine" — keep the previous reading so the chip does not + flap while GitHub thinks.""" + mergeable = str(data.get("mergeable") or "").upper() + if mergeable == "CONFLICTING": + return True + if mergeable == "MERGEABLE": + return False + return prev.get("conflicts") + + +def _fold(data: dict, prev: dict) -> dict: + """One gh pr-view payload + the previous snapshot → the new snapshot. + Pure fold: fetching, events and broadcasts stay in _poll_pr.""" reviews = data.get("reviews") or [] checks = data.get("statusCheckRollup") or [] changes = any(r.get("state") == "CHANGES_REQUESTED" for r in reviews) @@ -219,15 +224,18 @@ def _poll_pr(filename: str, url: str) -> None: elif cop_requested: copilot = "asked" else: - copilot = PR_STATE.get(filename, {}).get("copilot") - copilot = "asked" if copilot == "asked" else None + copilot = "asked" if prev.get("copilot") == "asked" else None states = [_check_state(c) for c in checks] ci = ("fail" if "fail" in states else "running" if "running" in states else "pass" if states else None) - verdict = ("red" if (changes or ci == "fail") + conflicts = _conflict_state(data, prev) + + # A conflict is changes-needed-by-you, not a CI failure: it beats any + # approval but leaves the CI chip telling its own story. + verdict = ("red" if (changes or ci == "fail" or conflicts) else "green" if approved else "pending") detail_bits = [] if reviews: @@ -235,14 +243,32 @@ def _poll_pr(filename: str, url: str) -> None: if ci: detail_bits.append({"fail": "checks failing", "running": "checks running", "pass": "checks ok"}[ci]) + if conflicts: + detail_bits.append("conflicts with main") if copilot: detail_bits.append("copilot " + {"asked": "asked", "approved": "approved", "changes": "asked for changes", "commented": "commented"}[copilot]) + return {"verdict": verdict, "ci": ci, "copilot": copilot, + "conflicts": conflicts, "detail": " · ".join(detail_bits)} + + +def _poll_pr(filename: str, url: str) -> None: + number = url.rstrip("/").rsplit("/", 1)[-1] + result = _run([config.GH_BIN, "pr", "view", number, + "--json", "reviews,reviewRequests,statusCheckRollup,state,mergeable"], + timeout=60) + if result.returncode != 0: + return + try: + data = json.loads(result.stdout) + except json.JSONDecodeError: + return prev = PR_STATE.get(filename, {}) - PR_STATE[filename] = {"verdict": verdict, "ci": ci, "copilot": copilot, - "url": url, "detail": " · ".join(detail_bits), - "ts": time.time()} + entry = _fold(data, prev) + entry.update({"url": url, "ts": time.time()}) + PR_STATE[filename] = entry + verdict, ci, copilot = entry["verdict"], entry["ci"], entry["copilot"] if prev.get("copilot") in (None, "asked") and copilot in ("approved", "changes", "commented"): word = {"approved": "approved it", "changes": "asked for changes", "commented": "commented"}[copilot] @@ -250,7 +276,14 @@ def _poll_pr(filename: str, url: str) -> None: "kind": "agent", "actor": "board", "file": filename, "summary": f"Copilot reviewed {filename}'s PR and {word}"}) state.broadcast({"type": "board"}) - if prev.get("verdict") != verdict and verdict != "pending": + if bool(prev.get("conflicts")) != bool(entry["conflicts"]): + state.record_board_event({ + "kind": "agent", "actor": "board", "file": filename, + "summary": (f"{filename}'s PR conflicts with main — ↻ act on PR " + f"can attempt the resolution" if entry["conflicts"] + else f"{filename}'s PR no longer conflicts with main")}) + state.broadcast({"type": "board"}) + elif prev.get("verdict") != verdict and verdict != "pending": word = "approved" if verdict == "green" else "changes asked" state.record_board_event({ "kind": "agent", "actor": "board", "file": filename, @@ -283,7 +316,7 @@ def poller() -> None: def public_state() -> dict: - return {f: {k: v.get(k) for k in ("verdict", "ci", "copilot", "detail", "url")} + return {f: {k: v.get(k) for k in ("verdict", "ci", "copilot", "conflicts", "detail", "url")} for f, v in PR_STATE.items()} diff --git a/tests/test_pr_conflicts.py b/tests/test_pr_conflicts.py new file mode 100644 index 0000000..17ff960 --- /dev/null +++ b/tests/test_pr_conflicts.py @@ -0,0 +1,95 @@ +"""Conflicted PRs become card state: the poller folds GitHub's +mergeable field into the PR snapshot, a conflict drops any +approved-green verdict as changes-needed-by-you (not a CI failure), +and GitHub's lazily-computed UNKNOWN keeps the previous reading so the +chip never flaps. The snapshot's `conflicts` key must reach the UI.""" + +import sys +import unittest +from pathlib import Path + +REPO = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(REPO / "manager" / "core")) + +import github # noqa: E402 + + +def payload(**overrides): + """A gh pr-view JSON payload with the fields _fold reads.""" + data = {"reviews": [], "reviewRequests": [], "statusCheckRollup": []} + data.update(overrides) + return data + + +APPROVED = [{"state": "APPROVED"}] + + +class ConflictFolding(unittest.TestCase): + def test_conflict_drops_green_even_when_approved(self): + entry = github._fold(payload(reviews=APPROVED, + mergeable="CONFLICTING"), {}) + self.assertTrue(entry["conflicts"]) + self.assertEqual(entry["verdict"], "red") + self.assertIn("conflicts with main", entry["detail"]) + + def test_conflict_is_not_a_ci_failure(self): + entry = github._fold(payload(mergeable="CONFLICTING"), {}) + self.assertIsNone(entry["ci"]) + self.assertEqual(entry["verdict"], "red") + + def test_mergeable_approved_pr_stays_green(self): + entry = github._fold(payload(reviews=APPROVED, + mergeable="MERGEABLE"), {}) + self.assertFalse(entry["conflicts"]) + self.assertEqual(entry["verdict"], "green") + + def test_unknown_keeps_the_previous_reading_both_ways(self): + # GitHub computes mergeability lazily after a push: UNKNOWN means + # "not yet", never "fine" — the chip must not flap. + still = github._fold(payload(mergeable="UNKNOWN"), + {"conflicts": True}) + self.assertTrue(still["conflicts"]) + self.assertEqual(still["verdict"], "red") + clean = github._fold(payload(reviews=APPROVED, mergeable="UNKNOWN"), + {"conflicts": False}) + self.assertFalse(clean["conflicts"]) + self.assertEqual(clean["verdict"], "green") + + def test_unknown_on_first_sight_alarms_nobody(self): + entry = github._fold(payload(mergeable="UNKNOWN"), {}) + self.assertIsNone(entry["conflicts"]) + self.assertEqual(entry["verdict"], "pending") + self.assertNotIn("conflicts", entry["detail"]) + + def test_resolution_lets_green_return(self): + entry = github._fold(payload(reviews=APPROVED, mergeable="MERGEABLE"), + {"conflicts": True, "verdict": "red"}) + self.assertFalse(entry["conflicts"]) + self.assertEqual(entry["verdict"], "green") + + +class SnapshotReachesTheUI(unittest.TestCase): + def test_public_state_carries_conflicts(self): + github.PR_STATE["x.md"] = {"verdict": "red", "ci": None, + "copilot": None, "conflicts": True, + "detail": "conflicts with main", + "url": "u", "ts": 1} + try: + self.assertTrue(github.public_state()["x.md"]["conflicts"]) + finally: + github.PR_STATE.pop("x.md", None) + + def test_the_card_wears_an_alarm_coloured_chip(self): + html = (REPO / "manager" / "core" / "board.html").read_text( + encoding="utf-8") + self.assertIn("prState.conflicts", html) + self.assertIn("label: 'conflicts', glyph: '✕', cls: 'bad'", html) + + def test_poller_asks_github_for_mergeable(self): + source = (REPO / "manager" / "core" / "github.py").read_text( + encoding="utf-8") + self.assertIn("statusCheckRollup,state,mergeable", source) + + +if __name__ == "__main__": + unittest.main()