From 68e14efa8cfd844e197d243577856887ed69ea8a Mon Sep 17 00:00:00 2001 From: Josh Mabry Date: Tue, 23 Jun 2026 21:45:54 -0700 Subject: [PATCH] feat: complete the write toolset (edit/merge/close/labels/assignees) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends write_tools.py from 3 to 8 gated write tools so the plugin covers the full GitHub write surface (still behind the per-agent github.write gate): - github_edit_pr — gh pr edit (+ gh pr ready [--undo]) for title/body/draft - github_merge_pr — gh pr merge; IRREVERSIBLE, so it refuses without confirm=true and offers a dry_run preview (defence in depth on top of the write gate) - github_close — close/reopen an issue or PR (kind-aware subcommands) - github_set_labels — gh {issue,pr} edit --add/--remove-label - github_set_assignees— gh {issue,pr} edit --add/--remove-assignee Each validates bad_repo(), builds explicit argv, runs via run_gh(), and degrades to a readable Error string. test_register's WRITE_TOOLS set now covers all 8 (so the gate test verifies they're all withheld read-only and present when write=true); test_write_tools adds argv + guard coverage for each. PROTO.md + manifest updated. Co-Authored-By: Claude Opus 4.8 (1M context) --- PROTO.md | 29 ++++-- protoagent.plugin.yaml | 9 +- tests/test_register.py | 11 +- tests/test_write_tools.py | 172 +++++++++++++++++++++++++++++++ write_tools.py | 210 ++++++++++++++++++++++++++++++++++++-- 5 files changed, 406 insertions(+), 25 deletions(-) diff --git a/PROTO.md b/PROTO.md index d9b783c..cd8a2ca 100644 --- a/PROTO.md +++ b/PROTO.md @@ -34,8 +34,8 @@ There is no other runner. `ruff` + `pytest` are the sole gate. protoagent.plugin.yaml # manifest (id: github, config_section: github, write: bool) __init__.py # register() — the GATING wiring (read always; write iff github.write) gh_cli.py # vendored async `gh` runner (run_gh, check_gh_error, bad_repo) -read_tools.py # 6 ported read tools (done) + read_file/repo_contents (STUBS) -write_tools.py # create_issue / comment / create_pr (STUBS — gated) +read_tools.py # 8 read tools (6 ported core + read_file/repo_contents) +write_tools.py # 8 write tools (create/edit/merge/close/comment/labels/assignees) — gated tests/ # host-free pytest (gating + version coherence) ``` @@ -46,14 +46,25 @@ tests/ # host-free pytest (gating + version coherence) agent stays read-only; a coding/PM agent gets write. `tests/test_register.py` asserts both halves — keep it green. -## 5. What to build (the stubs) +## 5. Tools (all implemented) -Each stub has a `TODO(team)` with the exact `gh` command. Implement, then add a test -that mocks `run_gh` and asserts the tool formats the result (and errors readably): -- **`github_read_file`** — `gh api repos/{repo}/contents/{path}?ref={ref}` (raw media type). -- **`github_repo_contents`** — `gh api repos/{repo}/contents/{path}` → list of entries. -- **`github_create_issue` / `github_comment` / `github_create_pr`** — `gh issue create` / - `gh issue comment` / `gh pr create`; return the new URL. +Each tool mocks `run_gh` in its test and asserts the exact argv + readable errors. + +**Read (always on)** — 6 ported core tools (`github_get_pr`, `github_get_issue`, +`github_list_issues`, `github_get_commit_diff`, `github_ci_runs`, `github_run_failure`) +plus `github_read_file` (`gh api .../contents/{path}`, raw) and `github_repo_contents` +(directory listing). + +**Write (gated on `github.write`)** — +`github_create_issue` / `github_comment` / `github_create_pr` (return the new URL), +`github_edit_pr` (`gh pr edit` + `gh pr ready [--undo]`), +`github_merge_pr` (`gh pr merge` — **refuses without `confirm=true`**, offers `dry_run`), +`github_close` (close/reopen issue|pr), and `github_set_labels` / `github_set_assignees` +(`gh {issue,pr} edit --add/--remove-{label,assignee}`). Issue-vs-PR ops take `kind`. + +New write op? Validate `bad_repo()`, build argv, `run_gh()`, degrade to `Error: ...`, +add it to `get_write_tools()`'s return list **and** `WRITE_TOOLS` in `test_register.py`, +and mirror an existing test. Anything irreversible (merge) must be `confirm`-guarded. ## 6. Rules diff --git a/protoagent.plugin.yaml b/protoagent.plugin.yaml index 633a653..7b2ffa3 100644 --- a/protoagent.plugin.yaml +++ b/protoagent.plugin.yaml @@ -6,10 +6,11 @@ version: 0.1.0 description: >- Read AND write GitHub tools over the `gh` CLI, with PER-AGENT write gating. The read tools (PRs, issues, diffs, CI, repo files/contents) are always on; the write - tools (create issue, comment, create PR) load ONLY when `github.write: true` — so a - research/Lead agent stays read-only while a coding/PM agent gets write, purely by - its own per-instance config (ADR 0019). Supersedes the read-only in-tree `github` - plugin. Tools degrade to a readable error if `gh` isn't installed/authenticated. + tools (create/edit/merge/close issues & PRs, comment, labels, assignees) load ONLY + when `github.write: true` — so a research/Lead agent stays read-only while a + coding/PM agent gets write, purely by its own per-instance config (ADR 0019). + Merging is `confirm`-guarded. Supersedes the read-only in-tree `github` plugin. + Tools degrade to a readable error if `gh` isn't installed/authenticated. enabled: false # SHIP DISABLED — enabling is the operator's trust decision repository: https://github.com/protoLabsAI/github-plugin min_protoagent_version: "0.27.0" diff --git a/tests/test_register.py b/tests/test_register.py index 30ea7c7..87facb7 100644 --- a/tests/test_register.py +++ b/tests/test_register.py @@ -18,7 +18,16 @@ "github_read_file", "github_repo_contents", } -WRITE_TOOLS = {"github_create_issue", "github_comment", "github_create_pr"} +WRITE_TOOLS = { + "github_create_issue", + "github_comment", + "github_create_pr", + "github_edit_pr", + "github_merge_pr", + "github_close", + "github_set_labels", + "github_set_assignees", +} def test_read_only_by_default(make_registry): diff --git a/tests/test_write_tools.py b/tests/test_write_tools.py index 4c82c52..fd1775a 100644 --- a/tests/test_write_tools.py +++ b/tests/test_write_tools.py @@ -175,3 +175,175 @@ async def test_create_pr_gh_failure_returns_check_gh_error(): with patch("ghplugin.write_tools.run_gh", fake): out = await tool.ainvoke({"repo": "o/n", "head": "feature", "title": "t"}) assert out == "Error (gh exit 1): could not create pr: forbidden" + + +def _by_name(name: str): + return {t.name: t for t in get_write_tools()}[name] + + +# --- github_edit_pr ---------------------------------------------------------- + + +async def test_edit_pr_title_and_body_via_pr_edit(): + """Title/body changes go through `gh pr edit` and return the PR URL.""" + tool = _by_name("github_edit_pr") + fake = AsyncMock(return_value=(0, "https://github.com/o/n/pull/3\n", "")) + with patch("ghplugin.write_tools.run_gh", fake): + out = await tool.ainvoke({"repo": "o/n", "number": 3, "title": "New", "body": "Updated"}) + assert out == "https://github.com/o/n/pull/3" + args = fake.call_args.args[0] + assert args[:5] == ["pr", "edit", "3", "--repo", "o/n"] + assert args[args.index("--title") + 1] == "New" + assert args[args.index("--body") + 1] == "Updated" + + +async def test_edit_pr_state_ready_and_draft(): + """state=ready → `gh pr ready`; state=draft → `gh pr ready --undo`.""" + tool = _by_name("github_edit_pr") + fake = AsyncMock(return_value=(0, "", "")) + with patch("ghplugin.write_tools.run_gh", fake): + await tool.ainvoke({"repo": "o/n", "number": 4, "state": "ready"}) + assert fake.call_args.args[0] == ["pr", "ready", "4", "--repo", "o/n"] + with patch("ghplugin.write_tools.run_gh", fake): + await tool.ainvoke({"repo": "o/n", "number": 4, "state": "draft"}) + assert fake.call_args.args[0] == ["pr", "ready", "4", "--repo", "o/n", "--undo"] + + +async def test_edit_pr_requires_a_change_and_validates_state(): + tool = _by_name("github_edit_pr") + fake = AsyncMock() + with patch("ghplugin.write_tools.run_gh", fake): + assert (await tool.ainvoke({"repo": "o/n", "number": 1})).startswith("Error: nothing to change") + assert "state must be" in await tool.ainvoke({"repo": "o/n", "number": 1, "state": "merged"}) + fake.assert_not_called() + + +# --- github_merge_pr (guarded) ---------------------------------------------- + + +async def test_merge_pr_refuses_without_confirm(): + """The default call refuses and never shells out — merging is irreversible.""" + tool = _by_name("github_merge_pr") + fake = AsyncMock() + with patch("ghplugin.write_tools.run_gh", fake): + out = await tool.ainvoke({"repo": "o/n", "number": 9}) + assert "confirm=true" in out and "Refusing" in out + fake.assert_not_called() + + +async def test_merge_pr_dry_run_previews_without_merging(): + tool = _by_name("github_merge_pr") + fake = AsyncMock() + with patch("ghplugin.write_tools.run_gh", fake): + out = await tool.ainvoke({"repo": "o/n", "number": 9, "dry_run": True}) + assert out.startswith("DRY RUN") and "via squash" in out + fake.assert_not_called() + + +async def test_merge_pr_confirm_runs_gh_merge(): + tool = _by_name("github_merge_pr") + fake = AsyncMock(return_value=(0, "Merged", "")) + with patch("ghplugin.write_tools.run_gh", fake): + await tool.ainvoke({"repo": "o/n", "number": 9, "method": "rebase", "delete_branch": True, "confirm": True}) + args = fake.call_args.args[0] + assert args == ["pr", "merge", "9", "--repo", "o/n", "--rebase", "--delete-branch"] + + +async def test_merge_pr_validates_method(): + tool = _by_name("github_merge_pr") + fake = AsyncMock() + with patch("ghplugin.write_tools.run_gh", fake): + out = await tool.ainvoke({"repo": "o/n", "number": 9, "method": "fast-forward", "confirm": True}) + assert "method must be" in out + fake.assert_not_called() + + +# --- github_close / reopen --------------------------------------------------- + + +async def test_close_issue_with_comment(): + tool = _by_name("github_close") + fake = AsyncMock(return_value=(0, "", "")) + with patch("ghplugin.write_tools.run_gh", fake): + await tool.ainvoke({"repo": "o/n", "number": 5, "comment": "dupe"}) + args = fake.call_args.args[0] + assert args[:5] == ["issue", "close", "5", "--repo", "o/n"] + assert args[args.index("--comment") + 1] == "dupe" + + +async def test_close_pr_uses_pr_subcommand(): + tool = _by_name("github_close") + fake = AsyncMock(return_value=(0, "", "")) + with patch("ghplugin.write_tools.run_gh", fake): + await tool.ainvoke({"repo": "o/n", "number": 5, "kind": "pr"}) + assert fake.call_args.args[0][:2] == ["pr", "close"] + + +async def test_reopen_ignores_comment_and_uses_reopen(): + tool = _by_name("github_close") + fake = AsyncMock(return_value=(0, "", "")) + with patch("ghplugin.write_tools.run_gh", fake): + await tool.ainvoke({"repo": "o/n", "number": 5, "reopen": True, "comment": "ignored"}) + args = fake.call_args.args[0] + assert args[:2] == ["issue", "reopen"] + assert "--comment" not in args + + +async def test_close_validates_kind(): + tool = _by_name("github_close") + fake = AsyncMock() + with patch("ghplugin.write_tools.run_gh", fake): + out = await tool.ainvoke({"repo": "o/n", "number": 5, "kind": "discussion"}) + assert "kind must be" in out + fake.assert_not_called() + + +# --- github_set_labels / github_set_assignees ------------------------------- + + +async def test_set_labels_add_and_remove(): + tool = _by_name("github_set_labels") + fake = AsyncMock(return_value=(0, "", "")) + with patch("ghplugin.write_tools.run_gh", fake): + await tool.ainvoke({"repo": "o/n", "number": 2, "add": "bug, p0", "remove": "stale"}) + args = fake.call_args.args[0] + assert args[:5] == ["issue", "edit", "2", "--repo", "o/n"] + assert [args[i + 1] for i, a in enumerate(args) if a == "--add-label"] == ["bug", "p0"] + assert [args[i + 1] for i, a in enumerate(args) if a == "--remove-label"] == ["stale"] + + +async def test_set_labels_requires_at_least_one(): + tool = _by_name("github_set_labels") + fake = AsyncMock() + with patch("ghplugin.write_tools.run_gh", fake): + out = await tool.ainvoke({"repo": "o/n", "number": 2}) + assert out.startswith("Error:") and "label" in out + fake.assert_not_called() + + +async def test_set_assignees_pr_kind_add_and_remove(): + tool = _by_name("github_set_assignees") + fake = AsyncMock(return_value=(0, "", "")) + with patch("ghplugin.write_tools.run_gh", fake): + await tool.ainvoke({"repo": "o/n", "number": 8, "add": "alice,bob", "remove": "carol", "kind": "pr"}) + args = fake.call_args.args[0] + assert args[:5] == ["pr", "edit", "8", "--repo", "o/n"] + assert [args[i + 1] for i, a in enumerate(args) if a == "--add-assignee"] == ["alice", "bob"] + assert [args[i + 1] for i, a in enumerate(args) if a == "--remove-assignee"] == ["carol"] + + +async def test_new_write_tools_bad_repo_short_circuits(): + """Every new write tool validates the repo before shelling out.""" + fake = AsyncMock() + cases = [ + ("github_edit_pr", {"repo": "x", "number": 1, "title": "t"}), + ("github_merge_pr", {"repo": "x", "number": 1, "confirm": True}), + ("github_close", {"repo": "x", "number": 1}), + ("github_set_labels", {"repo": "x", "number": 1, "add": "bug"}), + ("github_set_assignees", {"repo": "x", "number": 1, "add": "alice"}), + ] + for name, kwargs in cases: + with patch("ghplugin.write_tools.run_gh", fake): + out = await _by_name(name).ainvoke(kwargs) + assert out.startswith("Error:") and "owner/name" in out, name + fake.assert_not_called() diff --git a/write_tools.py b/write_tools.py index 80e3149..5616055 100644 --- a/write_tools.py +++ b/write_tools.py @@ -1,18 +1,23 @@ """GitHub WRITE tools over `gh` — GATED. Registered ONLY when `github.write: true`. These mutate GitHub, so they're behind the per-agent write gate (see __init__.py). -All are STUBBED — the team builds them out (see the TODOs). When implemented, each -must validate its repo with `bad_repo()`, run via `run_gh()`, and degrade to a +Each validates its repo with `bad_repo()`, runs via `run_gh()`, and degrades to a readable `Error: ...` string. -Build guidance: - - github_create_issue → `gh issue create --repo {repo} --title ... --body ...` - (return the new issue URL). - - github_comment → `gh issue comment {number} --repo {repo} --body ...` - (works for PRs too — a PR is an issue). Take an `is_pr`/number and post a comment. - - github_create_pr → `gh pr create --repo {repo} --head {branch} --base {base} - --title ... --body ...` (return the new PR URL). Note: the projectBoard loop opens - its own PRs; this tool is for ad-hoc PRs the agent raises directly. +The set: + - github_create_issue — `gh issue create` (returns the new issue URL). + - github_comment — `gh issue comment` (works for PRs too — a PR is an issue). + - github_create_pr — `gh pr create` (returns the new PR URL). + - github_edit_pr — `gh pr edit` (+ `gh pr ready`) to change title/body/draft. + - github_merge_pr — `gh pr merge`. IRREVERSIBLE, so it refuses without an + explicit ``confirm=true`` and offers a ``dry_run`` preview (defence-in-depth on + top of the per-agent write gate). + - github_close — close/reopen an issue or PR (`gh {issue,pr} close|reopen`). + - github_set_labels — add/remove labels (`gh {issue,pr} edit --add/--remove-label`). + - github_set_assignees — add/remove assignees (`gh {issue,pr} edit --add/--remove-assignee`). + +Issue-vs-PR matters for close/labels/assignees (different `gh` subcommands), so those +take a ``kind`` ("issue" | "pr"); commenting does not (a PR is an issue for comments). Keep tool docstrings PLAIN string literals (an f-string docstring → __doc__ is None → the tool ships with no description). @@ -25,6 +30,11 @@ from .gh_cli import bad_repo, check_gh_error, run_gh +def _csv(value: str) -> list[str]: + """Split a comma-separated arg into cleaned, non-empty tokens (order-preserving).""" + return [tok.strip() for tok in (value or "").split(",") if tok.strip()] + + def get_write_tools() -> list: @tool async def github_create_issue(repo: str, title: str, body: str = "", labels: str = "") -> str: @@ -103,4 +113,182 @@ async def github_create_pr(repo: str, head: str, title: str, body: str = "", bas return gh_err return out.strip() - return [github_create_issue, github_comment, github_create_pr] + @tool + async def github_edit_pr(repo: str, number: int, title: str = "", body: str = "", state: str = "") -> str: + """Edit a pull request's title/body and/or its draft state. + + Args: + repo: Repository as ``owner/name`` (required, no default). + number: PR number. + title: New title (omit to leave unchanged). + body: New body / description (omit to leave unchanged). + state: ``ready`` to mark ready for review, ``draft`` to convert back to a + draft, or empty to leave the state unchanged. + + Returns the PR URL (or a short summary of what changed). + """ + if err := bad_repo(repo): + return err + if not (title or body or state): + return "Error: nothing to change — pass title, body, and/or state." + if state and state not in ("ready", "draft"): + return f"Error: state must be 'ready' or 'draft' (got {state!r})." + url = "" + if title or body: + args = ["pr", "edit", str(number), "--repo", repo] + if title: + args += ["--title", title] + if body: + args += ["--body", body] + rc, out, serr = await run_gh(args) + if gh_err := check_gh_error(rc, serr): + return gh_err + url = out.strip() + if state: + args = ["pr", "ready", str(number), "--repo", repo] + (["--undo"] if state == "draft" else []) + rc, out, serr = await run_gh(args) + if gh_err := check_gh_error(rc, serr): + return gh_err + url = url or out.strip() + return url or f"Edited PR #{number} in {repo}." + + @tool + async def github_merge_pr( + repo: str, + number: int, + method: str = "squash", + delete_branch: bool = False, + dry_run: bool = False, + confirm: bool = False, + ) -> str: + """Merge a pull request. IRREVERSIBLE — guarded. + + Args: + repo: Repository as ``owner/name`` (required, no default). + number: PR number. + method: ``squash`` (default), ``merge``, or ``rebase``. + delete_branch: Delete the head branch after merging (default False). + dry_run: If true, report what WOULD be merged without merging. + confirm: Must be true to actually merge — a guard against accidental or + autonomous merges even when the per-agent write gate is on. + + Returns the merge result, a dry-run preview, or a refusal asking for confirm. + """ + if err := bad_repo(repo): + return err + if method not in ("squash", "merge", "rebase"): + return f"Error: method must be 'squash', 'merge', or 'rebase' (got {method!r})." + if dry_run: + extra = " and delete its branch" if delete_branch else "" + return ( + f"DRY RUN — would merge PR #{number} in {repo} via {method}{extra}. Re-call with confirm=true to do it." + ) + if not confirm: + return ( + f"Refusing to merge PR #{number} in {repo} without confirm=true — merging is irreversible. " + "Re-call with confirm=true, or dry_run=true to preview." + ) + args = ["pr", "merge", str(number), "--repo", repo, f"--{method}"] + if delete_branch: + args.append("--delete-branch") + rc, out, serr = await run_gh(args) + if gh_err := check_gh_error(rc, serr): + return gh_err + return out.strip() or f"Merged PR #{number} in {repo} via {method}." + + @tool + async def github_close(repo: str, number: int, kind: str = "issue", reopen: bool = False, comment: str = "") -> str: + """Close (or reopen) an issue or pull request. + + Args: + repo: Repository as ``owner/name`` (required, no default). + number: Issue or PR number. + kind: ``issue`` (default) or ``pr`` — they use different gh subcommands. + reopen: If true, reopen instead of close. + comment: Optional comment to post alongside a close (ignored on reopen). + + Returns a short confirmation. + """ + if err := bad_repo(repo): + return err + if kind not in ("issue", "pr"): + return f"Error: kind must be 'issue' or 'pr' (got {kind!r})." + action = "reopen" if reopen else "close" + args = [kind, action, str(number), "--repo", repo] + if comment and not reopen: + args += ["--comment", comment] + rc, out, serr = await run_gh(args) + if gh_err := check_gh_error(rc, serr): + return gh_err + return out.strip() or f"{'Reopened' if reopen else 'Closed'} {kind} #{number} in {repo}." + + @tool + async def github_set_labels(repo: str, number: int, add: str = "", remove: str = "", kind: str = "issue") -> str: + """Add and/or remove labels on an issue or pull request. + + Args: + repo: Repository as ``owner/name`` (required, no default). + number: Issue or PR number. + add: Comma-separated label names to add. + remove: Comma-separated label names to remove. + kind: ``issue`` (default) or ``pr``. + + Returns a short confirmation. + """ + if err := bad_repo(repo): + return err + if kind not in ("issue", "pr"): + return f"Error: kind must be 'issue' or 'pr' (got {kind!r})." + adds, removes = _csv(add), _csv(remove) + if not adds and not removes: + return "Error: pass at least one label to add or remove." + args = [kind, "edit", str(number), "--repo", repo] + for label in adds: + args += ["--add-label", label] + for label in removes: + args += ["--remove-label", label] + rc, out, serr = await run_gh(args) + if gh_err := check_gh_error(rc, serr): + return gh_err + return out.strip() or f"Updated labels on {kind} #{number} in {repo}." + + @tool + async def github_set_assignees(repo: str, number: int, add: str = "", remove: str = "", kind: str = "issue") -> str: + """Add and/or remove assignees on an issue or pull request. + + Args: + repo: Repository as ``owner/name`` (required, no default). + number: Issue or PR number. + add: Comma-separated GitHub usernames to assign. + remove: Comma-separated GitHub usernames to unassign. + kind: ``issue`` (default) or ``pr``. + + Returns a short confirmation. + """ + if err := bad_repo(repo): + return err + if kind not in ("issue", "pr"): + return f"Error: kind must be 'issue' or 'pr' (got {kind!r})." + adds, removes = _csv(add), _csv(remove) + if not adds and not removes: + return "Error: pass at least one assignee to add or remove." + args = [kind, "edit", str(number), "--repo", repo] + for user in adds: + args += ["--add-assignee", user] + for user in removes: + args += ["--remove-assignee", user] + rc, out, serr = await run_gh(args) + if gh_err := check_gh_error(rc, serr): + return gh_err + return out.strip() or f"Updated assignees on {kind} #{number} in {repo}." + + return [ + github_create_issue, + github_comment, + github_create_pr, + github_edit_pr, + github_merge_pr, + github_close, + github_set_labels, + github_set_assignees, + ]