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/adapters/README.md b/manager/core/adapters/README.md index f4cc8fd..e461823 100644 --- a/manager/core/adapters/README.md +++ b/manager/core/adapters/README.md @@ -40,9 +40,13 @@ side effects its prompt demands, and never a blanket allow-everything - `work` — implement, test, commit in an isolated worktree. May edit files, run local git bookkeeping (`git add/commit/status/diff`) and the project's `AGENT_COMMANDS`. No push. -- `act-pr` — the work stance, plus `git push` (the PR must update) and +- `act-pr` — the work stance, plus `git push` (the PR must update), reading the PR's reviews and line comments (`gh pr view`, `gh pr - diff`, `gh api`). + diff`, `gh api`), and `git fetch`/`git merge` so a conflicted PR can + be resolved by merging main into the branch. Resolution is additive + only — the branch is public — so `git rebase` and the force-push + spellings must be denied, not merely unlisted (a plain `git push` + allow would otherwise cover them). - `review` — read-only on the working tree: no edit tools, no commits. May read a PR (`gh pr view`, `gh pr diff`, read-only git) and post the verdict (`gh pr review`, `gh pr comment`). diff --git a/manager/core/adapters/claude/hook_settings.py b/manager/core/adapters/claude/hook_settings.py index 503c5d2..96a607d 100644 --- a/manager/core/adapters/claude/hook_settings.py +++ b/manager/core/adapters/claude/hook_settings.py @@ -13,7 +13,12 @@ is isolated, the shell is not. (add/commit/status/diff) + the project's test/check commands. No push. act-pr the work stance + `git push` (the PR must update) + reading - the PR's reviews and line comments through gh. + the PR's reviews and line comments through gh + `git fetch` + and `git merge` so a conflicted PR can be resolved by merging + main into the branch. The branch is public, so resolution is + additive only: rebase and the force-push spellings are denied + outright (deny beats allow, catching what the plain + `git push` prefix would otherwise cover). review read-only (edit tools disallowed in `run`) + reading the PR it judges + posting the verdict with gh pr review/comment. @@ -35,10 +40,19 @@ from pathlib import Path MODE_PREFIXES = { "work": ["git add", "git commit", "git status", "git diff"], "act-pr": ["git add", "git commit", "git status", "git diff", + "git fetch", "git merge", "git push", "gh pr view", "gh pr diff", "gh api"], "review": ["git status", "git diff", "git log", "git show", "gh pr view", "gh pr diff", "gh pr review", "gh pr comment"], } +# History must never rewrite under a public PR: deny the canonical force +# and rebase spellings even though nothing allows them — `git push` alone +# would otherwise cover `git push --force` by prefix. (Prefix rules can't +# catch a flag placed after the refspec; the prompt and the review loop +# guard the exotic spellings.) +MODE_DENY_PREFIXES = { + "act-pr": ["git push --force", "git push -f", "git rebase"], +} # Which intents run the project's own test/check commands. MODES_WITH_PROJECT_COMMANDS = {"work", "act-pr"} @@ -60,6 +74,14 @@ def allow_rules(mode: str, commands: list[str]) -> list[str]: return rules +def deny_rules(mode: str) -> list[str]: + """Bash() deny-rules for one intent — deny beats allow.""" + rules = [] + for prefix in MODE_DENY_PREFIXES.get(mode, []): + rules += [f"Bash({prefix})", f"Bash({prefix}:*)"] + return rules + + def settings(mode: str, commands: list[str]) -> dict: emit = Path(__file__).resolve().parent / "emit.py" hook = {"type": "command", "command": f'python3 "{emit}"', "timeout": 5} @@ -71,9 +93,15 @@ def settings(mode: str, commands: list[str]) -> dict: "PreToolUse": [{"matcher": "Bash", "hooks": [hook]}], "PostToolUse": [{"matcher": "*", "hooks": [hook]}], }} + perms = {} rules = allow_rules(mode, commands) if rules: - out["permissions"] = {"allow": rules} + perms["allow"] = rules + denies = deny_rules(mode) + if denies: + perms["deny"] = denies + if perms: + out["permissions"] = perms return out diff --git a/manager/core/adapters/opencode/permission_config.py b/manager/core/adapters/opencode/permission_config.py index bbac75f..617ba97 100644 --- a/manager/core/adapters/opencode/permission_config.py +++ b/manager/core/adapters/opencode/permission_config.py @@ -14,7 +14,11 @@ and never a blanket allow: the worktree is isolated, the shell is not. work "edit": "allow" + git bookkeeping (add/commit/status/diff) and the project's test/check commands. No push. act-pr the work stance + `git push` + reading the PR's reviews and - line comments through gh. + line comments through gh + `git fetch`/`git merge` so a + conflicted PR can be resolved by merging main into the + branch. The branch is public, so resolution is additive + only: rebase and the force-push spellings get explicit deny + rules, placed last so they win over the `git push *` allow. review "edit": "deny" + reading the PR it judges + posting the verdict with gh pr review/comment. Everything else denied. @@ -34,10 +38,20 @@ import sys MODE_PREFIXES = { "work": ["git add", "git commit", "git status", "git diff"], "act-pr": ["git add", "git commit", "git status", "git diff", + "git fetch", "git merge", "git push", "gh pr view", "gh pr diff", "gh api"], "review": ["git status", "git diff", "git log", "git show", "gh pr view", "gh pr diff", "gh pr review", "gh pr comment"], } +# History must never rewrite under a public PR: deny the force and rebase +# spellings even though nothing allows them — "git push *" would otherwise +# cover them. Globs run over the whole command line, so the flag is caught +# wherever it sits. +MODE_DENY_PATTERNS = { + "act-pr": ["git rebase", "git rebase *", + "git push --force*", "git push * --force*", + "git push -f", "git push -f *", "git push * -f *"], +} # Which intents run the project's own test/check commands. MODES_WITH_PROJECT_COMMANDS = {"work", "act-pr"} @@ -57,6 +71,8 @@ def bash_rules(mode: str, commands: list[str]) -> dict: for prefix in prefixes: rules[prefix] = "allow" rules[f"{prefix} *"] = "allow" + for pattern in MODE_DENY_PATTERNS.get(mode, []): + rules[pattern] = "deny" # last, so it wins over the allows return rules diff --git a/manager/core/board.html b/manager/core/board.html index 95ee577..b2b367f 100644 --- a/manager/core/board.html +++ b/manager/core/board.html @@ -909,6 +909,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/manager/core/prompts/act-pr.md b/manager/core/prompts/act-pr.md index ad9cbe3..4a8e6dd 100644 --- a/manager/core/prompts/act-pr.md +++ b/manager/core/prompts/act-pr.md @@ -14,6 +14,23 @@ Do this properly: repos/{{owner}}/{{repo}}/pulls//comments`. - Address each point in the code. If you disagree with a point, do not silently ignore it — leave it unchanged and say why in your summary. +- If the PR conflicts with main (check `gh pr view {branch} --json + mergeable` — CONFLICTING means yes), resolve it mechanically: + - `git fetch origin main`, then `git merge origin/main` in this + worktree. Never rebase and never force-push: the branch is public, + so the resolution must be additive. + - Resolve each conflicted file honouring both sides' intent, and run + the project's tests until they pass. + - Put the resolution in its own commit — never folded into other + changes — with a message naming the conflicted files and the choice + made in each. + - Cover the resolution explicitly in your closing report: which files + conflicted and what you chose. + - If both intents cannot hold at once — main has made this branch's + premise false — resolve nothing: run `git merge --abort`, leave the + branch as it was, and state in your report that a human must + decide, naming the specific collision. Guessing at a semantic + conflict is the one forbidden move. - Follow repo AGENTS.md: layering rules, definition of done. Run the tests that cover what you changed until they pass. - Commit in clear, reviewable commits and push the branch (`git push`) so diff --git a/tests/test_adapter_permissions.py b/tests/test_adapter_permissions.py index 69bb9b1..08ab969 100644 --- a/tests/test_adapter_permissions.py +++ b/tests/test_adapter_permissions.py @@ -53,6 +53,29 @@ class ClaudeAllowRules(unittest.TestCase): for prefix in ["git push", "gh pr view", "gh pr diff", "gh api"]: self.assertIn(f"Bash({prefix}:*)", rules) + def test_act_pr_resolves_conflicts_additively(self): + # Conflicted PRs: merging main into the branch is allowed; history + # rewriting is not, and not merely by omission — `git push` alone + # would cover the force spellings, so they are denied outright. + rules = hook_settings.allow_rules("act-pr", COMMANDS) + for prefix in ["git fetch", "git merge"]: + self.assertIn(f"Bash({prefix})", rules) + self.assertIn(f"Bash({prefix}:*)", rules) + joined = " ".join(rules) + self.assertNotIn("git rebase", joined) + self.assertNotIn("--force", joined) + deny = hook_settings.settings("act-pr", COMMANDS)["permissions"]["deny"] + for prefix in ["git push --force", "git push -f", "git rebase"]: + self.assertIn(f"Bash({prefix}:*)", deny) + + def test_only_act_pr_may_fetch_and_merge(self): + for mode in ("work", "review"): + joined = " ".join(hook_settings.allow_rules(mode, COMMANDS)) + self.assertNotIn("git fetch", joined) + self.assertNotIn("git merge", joined) + self.assertNotIn("deny", + hook_settings.settings(mode, COMMANDS)["permissions"]) + def test_review_posts_verdicts_but_writes_nothing_locally(self): rules = hook_settings.allow_rules("review", COMMANDS) for prefix in ["gh pr review", "gh pr comment", "gh pr view", @@ -90,6 +113,26 @@ class OpencodeConfig(unittest.TestCase): self.assertEqual(bash["git push *"], "allow") self.assertEqual(bash["gh pr view *"], "allow") + def test_act_pr_resolves_conflicts_additively(self): + bash = permission_config.build_config("act-pr", COMMANDS)["permission"]["bash"] + for prefix in ["git fetch", "git merge"]: + self.assertEqual(bash[prefix], "allow") + self.assertEqual(bash[f"{prefix} *"], "allow") + for pattern in ["git rebase", "git rebase *", "git push --force*", + "git push * --force*", "git push -f", "git push -f *", + "git push * -f *"]: + self.assertEqual(bash[pattern], "deny") + # last match wins: the denies must come after the push allow + keys = list(bash) + self.assertGreater(keys.index("git push --force*"), + keys.index("git push *")) + + def test_only_act_pr_may_fetch_and_merge(self): + for mode in ("work", "review"): + bash = permission_config.build_config(mode, COMMANDS)["permission"]["bash"] + for absent in ["git fetch", "git merge", "git rebase"]: + self.assertNotIn(absent, bash) # unlisted = denied by "*" + def test_review_cannot_edit_and_bash_default_denies(self): config = permission_config.build_config("review", COMMANDS) self.assertEqual(config["permission"]["edit"], "deny") 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()