From 1f8b6e33c9cc8385e895428c61e33997d92e221d Mon Sep 17 00:00:00 2001 From: GitHub CI Date: Mon, 6 Jul 2026 02:42:39 -0700 Subject: [PATCH] feat(review-tools): formal verdict surface with below-the-model guards (v0.2.0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR 0078 Phase B — the hands of the QA-review takeover: - github_review_comment / github_review_approve / github_review_request_changes post formal reviews via the GitHub Review API. The two blocking verdicts are GUARDED inside the tool, where a prompt can't reach: · CI-terminal (Quinn's #863): refused while any check run is queued/running, AND when CI can't be read at all (403/error says nothing about pass/fail — fail closed, never open). Refusals instruct comment-instead + never poll. · Self-review: refused when the PR author is the token's own identity (approve-your-own-work loops); commenting on own PRs stays allowed. · No checks at all = terminal by definition. - github_path_exists (read tool): the EXISTS/MISSING grounding probe for review claims about external references — cross-repo capable; a tool error reads as UNVERIFIED (a Gap), never a verdict. - Review tools ride the github.write gate with the other mutating tools. 13 new tests (guards impossible to bypass: pending CI, unreadable CI, own PR; COMMENT ungated; path probe EXISTS/MISSING/UNVERIFIED). 110 passed; ruff clean. v0.2.0 lockstep. Co-Authored-By: Claude Fable 5 --- __init__.py | 3 +- protoagent.plugin.yaml | 2 +- pyproject.toml | 2 +- read_tools.py | 42 +++++++ review_tools.py | 219 +++++++++++++++++++++++++++++++++++++ tests/test_read_tools.py | 30 +++++ tests/test_review_tools.py | 131 ++++++++++++++++++++++ 7 files changed, 426 insertions(+), 3 deletions(-) create mode 100644 review_tools.py create mode 100644 tests/test_review_tools.py diff --git a/__init__.py b/__init__.py index 28de758..bf1fb0c 100644 --- a/__init__.py +++ b/__init__.py @@ -55,9 +55,10 @@ def register(registry) -> None: n_write = 0 if write_enabled: try: + from .review_tools import get_review_tools from .write_tools import get_write_tools - write = get_write_tools(default_repo) + write = get_write_tools(default_repo) + get_review_tools(default_repo) for t in write: registry.register_tool(t) n_write = len(write) diff --git a/protoagent.plugin.yaml b/protoagent.plugin.yaml index 5cd1078..5c1f5b7 100644 --- a/protoagent.plugin.yaml +++ b/protoagent.plugin.yaml @@ -2,7 +2,7 @@ # Keep `version` in lockstep with pyproject.toml (tests/test_version.py asserts it). id: github name: GitHub (read/write tools) -version: 0.1.6 +version: 0.2.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 diff --git a/pyproject.toml b/pyproject.toml index ef8e888..b2581da 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "github-plugin" -version = "0.1.6" +version = "0.2.0" description = "Read/write GitHub tools for protoAgent over the gh CLI, with per-agent write gating." requires-python = ">=3.11" diff --git a/read_tools.py b/read_tools.py index 61f6b3e..a728278 100644 --- a/read_tools.py +++ b/read_tools.py @@ -286,6 +286,47 @@ async def github_read_file(path: str, repo: str = "", ref: str = "") -> str: out = out[:20000] + "\n… (truncated at 20000 chars)" return out + @tool + async def github_path_exists(path: str, repo: str = "", ref: str = "") -> str: + """Check whether a path exists in a GitHub repo — the grounding probe for + review claims about external references. + + A diff is often only correct if something OUTSIDE it exists (a cross-repo + `COPY --from` path, a workspace package, an action ref). The answer is + authoritative: EXISTS means the assumption holds (don't invent a problem + around it); MISSING means the dependency is really absent (that's the + finding). A tool error means UNVERIFIED — report a Gap, never a severity. + + Args: + repo: Repository as ``owner/name`` — may be a DIFFERENT repo than the + PR under review (that is the point). Omit for the default repo. + path: Repo-relative path to check (file or directory). + ref: Optional branch / tag / SHA (default: the repo's default branch). + """ + repo = resolve_repo(repo, default_repo) or "" + if err := bad_repo(repo): + return err + if not path.strip(): + return "Error: `path` is empty." + clean = path.strip().strip("/") + args = ["api", f"repos/{repo}/contents/{clean}"] + if ref.strip(): + args += ["-f", f"ref={ref.strip()}"] + rc, out, serr = await run_gh(args) + if rc == 0: + return f"EXISTS: {repo}/{clean}" + (f" @ {ref.strip()}" if ref.strip() else "") + blob = (serr or out or "").lower() + if "404" in blob or "not found" in blob: + return ( + f"MISSING: {repo}/{clean}" + + (f" @ {ref.strip()}" if ref.strip() else "") + + " — the path does not exist." + ) + return ( + check_gh_error(rc, serr) + or f"Error (gh exit {rc}): could not verify {repo}/{clean} — treat as UNVERIFIED (a Gap, not a finding)." + ) + @tool async def github_repo_contents(repo: str = "", path: str = "", ref: str = "") -> str: """List the contents (files + dirs) of a path in a GitHub repo. @@ -325,6 +366,7 @@ async def github_repo_contents(repo: str = "", path: str = "", ref: str = "") -> github_list_issues, github_get_commit_diff, github_pr_diff, + github_path_exists, github_ci_runs, github_run_failure, github_read_file, diff --git a/review_tools.py b/review_tools.py new file mode 100644 index 0000000..eb94ce0 --- /dev/null +++ b/review_tools.py @@ -0,0 +1,219 @@ +"""Formal PR-review verdict tools over `gh` — GATED behind `github.write: true`. + +The verdict surface for the QA-review takeover (protoAgent ADR 0078 Phase B): +three tools posting formal reviews via the GitHub Review API, with the guards +that made Quinn's reviewer reliable enforced INSIDE the tools — below the model, +so a prompt can never bypass them: + + - github_review_comment — non-blocking COMMENT review. No guards: a + comment is always safe to post. + - github_review_approve — formal APPROVE. Guarded. + - github_review_request_changes — formal REQUEST_CHANGES. Guarded. + +The guards (ported from protoWorkstacean's pr-inspector, her failure ledger): + + CI-TERMINAL (#863) — a blocking verdict locks in a judgement about the PR's + settled state, so APPROVE / REQUEST_CHANGES are refused while any check run + is still queued/in-progress, AND when CI cannot be read at all (a 403/error + says nothing about whether checks pass — fail closed, never fail open). The + refusal tells the model to post a review_comment instead. A repo with no + checks at all is terminal by definition. + + SELF-REVIEW — the tool refuses a blocking verdict on a PR authored by the + SAME identity the token authenticates as (approve-your-own-work loops). + Commenting on your own PR stays allowed. + +Keep tool docstrings PLAIN string literals (an f-string docstring → __doc__ is +None → the tool ships with no description). +""" + +from __future__ import annotations + +import json + +from langchain_core.tools import tool + +from .gh_cli import bad_repo, check_gh_error, run_gh +from .gh_issue import resolve_repo + +# Check-run states that count as still-running. GitHub check runs carry +# `status` (queued | in_progress | completed | waiting | requested | pending) +# and only a completed run has a `conclusion`. +_NON_TERMINAL = {"queued", "in_progress", "waiting", "requested", "pending"} + + +async def _viewer_login() -> str: + """The login the token authenticates as ('' on failure — guards fail closed + on their own signal, not on this helper).""" + rc, out, _err = await run_gh(["api", "user", "--jq", ".login"]) + return out.strip() if rc == 0 else "" + + +async def _pr_head_and_author(repo: str, number: int) -> tuple[str, str, str]: + """(head_sha, author_login, error). error is '' on success.""" + rc, out, serr = await run_gh(["pr", "view", str(number), "--repo", repo, "--json", "headRefOid,author"]) + if gh_err := check_gh_error(rc, serr): + return "", "", gh_err + try: + d = json.loads(out) + except json.JSONDecodeError: + return "", "", f"Error: could not parse gh output: {out[:200]}" + return str(d.get("headRefOid") or ""), str((d.get("author") or {}).get("login") or ""), "" + + +async def _ci_state(repo: str, head_sha: str) -> tuple[str, str]: + """('terminal' | 'pending' | 'unknown', detail). + + 'terminal' — every check run completed (or the commit has none at all). + 'pending' — at least one run is queued/in-progress. + 'unknown' — CI could not be read (403, network, parse) — treated exactly + like pending by the guards: you cannot verdict what you + cannot see (fail closed).""" + rc, out, serr = await run_gh( + ["api", f"repos/{repo}/commits/{head_sha}/check-runs", "--jq", "[.check_runs[].status]"] + ) + if rc != 0: + return "unknown", (serr or out).strip()[:200] + try: + statuses = json.loads(out or "[]") + except json.JSONDecodeError: + return "unknown", f"unparseable check-runs response: {out[:120]}" + pending = [s for s in statuses if str(s).lower() in _NON_TERMINAL] + if pending: + return "pending", f"{len(pending)} of {len(statuses)} check run(s) still running" + return "terminal", f"{len(statuses)} check run(s), all completed" + + +async def _post_review(repo: str, number: int, event: str, body: str) -> str: + rc, out, serr = await run_gh( + [ + "api", + f"repos/{repo}/pulls/{number}/reviews", + "-X", + "POST", + "-f", + f"event={event}", + "-f", + f"body={body}", + ] + ) + if gh_err := check_gh_error(rc, serr): + return gh_err + try: + d = json.loads(out) + url = d.get("html_url") or "" + except json.JSONDecodeError: + url = "" + return f"Posted {event} review on {repo}#{number}." + (f" {url}" if url else "") + + +async def _guard_blocking_verdict(repo: str, number: int, verdict: str) -> str: + """The two below-the-model guards. Returns '' to allow, or the refusal text. + + Order matters: the cheap identity guard first, then CI. Both refusals + instruct the model to use github_review_comment instead — the non-blocking + outcome is always available.""" + head_sha, author, err = await _pr_head_and_author(repo, number) + if err: + return ( + f"{err}\nHeld: cannot verify the PR's head/author, so a formal {verdict} " + "is refused (fail closed). Post your findings with github_review_comment instead." + ) + me = await _viewer_login() + if me and author and me == author: + return ( + f"Held: this PR was authored by {author!r} — the same identity this agent " + f"reviews as. A formal {verdict} on your own work is refused (self-approval " + "loops). Post observations with github_review_comment and escalate to a " + "human reviewer." + ) + state, detail = await _ci_state(repo, head_sha) + if state == "pending": + return ( + f"Held: CI is not terminal on {repo}#{number} ({detail}). A formal {verdict} " + "locks in a judgement about the PR's settled state, so it is refused while " + "checks run. Post your findings NOW with github_review_comment (non-blocking) " + "and finish — do not wait or poll; once checks settle, a later pass (or a " + "deterministic approve-on-green policy) takes it from there." + ) + if state == "unknown": + return ( + f"Held: CI on {repo}#{number} cannot be read ({detail}) — which says nothing " + f"about whether checks pass. A formal {verdict} on unverified CI is refused " + "(fail closed). Record the CI-access gap with github_review_comment " + "(non-blocking); do not treat unreadable CI as a failure." + ) + return "" + + +def get_review_tools(default_repo: str = "") -> list: + """Build the verdict tools. ``default_repo`` (``owner/name``) is used whenever a + tool's ``repo`` arg is omitted.""" + + @tool + async def github_review_comment(number: int, body: str, repo: str = "") -> str: + """Post a formal non-blocking COMMENT review on a pull request. + + The always-safe verdict: use it for findings while CI is still running, for + observations that shouldn't block a merge, or when a blocking verdict was + refused by its guards. + + Args: + repo: Repository as ``owner/name``. Omit to use the agent's configured default repo. + number: PR number. + body: The review body (findings, observations — markdown). + """ + repo = resolve_repo(repo, default_repo) or "" + if err := bad_repo(repo): + return err + if not body.strip(): + return "Error: `body` is empty — a review needs content." + return await _post_review(repo, number, "COMMENT", body) + + @tool + async def github_review_approve(number: int, body: str, repo: str = "") -> str: + """Post a formal APPROVE review on a pull request. GUARDED. + + Refused (with comment-instead guidance) while any check run is still + running, when CI cannot be read at all, or on a PR this agent authored. + An APPROVE clears the way to merge — it must reflect the PR's settled, + verified state, never a guess. + + Args: + repo: Repository as ``owner/name``. Omit to use the agent's configured default repo. + number: PR number. + body: The review body (what was verified, any non-blocking notes). + """ + repo = resolve_repo(repo, default_repo) or "" + if err := bad_repo(repo): + return err + if not body.strip(): + return "Error: `body` is empty — a review needs content." + if held := await _guard_blocking_verdict(repo, number, "APPROVE"): + return held + return await _post_review(repo, number, "APPROVE", body) + + @tool + async def github_review_request_changes(number: int, body: str, repo: str = "") -> str: + """Post a formal REQUEST_CHANGES review on a pull request. GUARDED. + + Refused (with comment-instead guidance) while any check run is still + running, when CI cannot be read at all, or on a PR this agent authored. + A broken-CI block must rest on a check that COMPLETED with a failure you + read — never on one still running or one you couldn't see. + + Args: + repo: Repository as ``owner/name``. Omit to use the agent's configured default repo. + number: PR number. + body: The review body — every blocking finding with file:line evidence. + """ + repo = resolve_repo(repo, default_repo) or "" + if err := bad_repo(repo): + return err + if not body.strip(): + return "Error: `body` is empty — a review needs content." + if held := await _guard_blocking_verdict(repo, number, "REQUEST_CHANGES"): + return held + return await _post_review(repo, number, "REQUEST_CHANGES", body) + + return [github_review_comment, github_review_approve, github_review_request_changes] diff --git a/tests/test_read_tools.py b/tests/test_read_tools.py index 18dc543..c4cb668 100644 --- a/tests/test_read_tools.py +++ b/tests/test_read_tools.py @@ -284,3 +284,33 @@ async def test_omitted_repo_no_default_errors(monkeypatch): monkeypatch.delenv("GH_REPO", raising=False) result = await _list_issues_tool("").ainvoke({}) # no repo, no default, no env assert result.startswith("Error: no usable repo") + + +# ── github_path_exists: the EXISTS/MISSING grounding probe (ADR 0078) ─────────── +def _path_exists_tool(): + for t in get_read_tools(): + if t.name == "github_path_exists": + return t + raise AssertionError("github_path_exists tool not found") + + +@pytest.mark.asyncio +async def test_path_exists_reports_exists(): + with patch("ghplugin.read_tools.run_gh", new=AsyncMock(return_value=(0, "{}", ""))): + out = await _path_exists_tool().ainvoke({"repo": "owner/name", "path": "packages/x"}) + assert out.startswith("EXISTS: owner/name/packages/x") + + +@pytest.mark.asyncio +async def test_path_exists_reports_missing_on_404(): + with patch("ghplugin.read_tools.run_gh", new=AsyncMock(return_value=(1, "", "HTTP 404: Not Found"))): + out = await _path_exists_tool().ainvoke({"repo": "owner/name", "path": "gone.txt", "ref": "main"}) + assert out.startswith("MISSING: owner/name/gone.txt @ main") + + +@pytest.mark.asyncio +async def test_path_exists_other_error_is_unverified_not_a_verdict(): + with patch("ghplugin.read_tools.run_gh", new=AsyncMock(return_value=(1, "", "HTTP 500"))): + out = await _path_exists_tool().ainvoke({"repo": "owner/name", "path": "x"}) + assert "EXISTS" not in out and "MISSING" not in out.split(":")[0] + assert out.startswith("Error") diff --git a/tests/test_review_tools.py b/tests/test_review_tools.py new file mode 100644 index 0000000..dc5b5aa --- /dev/null +++ b/tests/test_review_tools.py @@ -0,0 +1,131 @@ +"""Verdict tools (ADR 0078 Phase B) over a mocked `gh`. + +The point of these tests is the GUARDS: a blocking verdict must be impossible +against pending CI, unreadable CI, or the agent's own PR — refusals the model +cannot prompt its way around because they live below it, in the tool. +""" + +from __future__ import annotations + +import json +from unittest.mock import patch + +import pytest +from ghplugin.review_tools import get_review_tools + +PR_JSON = json.dumps({"headRefOid": "a" * 40, "author": {"login": "someone-else"}}) +OWN_PR_JSON = json.dumps({"headRefOid": "a" * 40, "author": {"login": "proto-bot"}}) + + +def _tool(name: str): + for t in get_review_tools("owner/name"): + if t.name == name: + return t + raise AssertionError(f"{name} not found") + + +def _gh(*, pr_json=PR_JSON, viewer="proto-bot", statuses='["completed","completed"]', review=None, checks_rc=0): + """A run_gh stub routing by argv shape.""" + + async def run_gh(args, timeout=None, **kw): + if args[:2] == ["pr", "view"]: + return (0, pr_json, "") + if args[:2] == ["api", "user"]: + return (0, viewer, "") + if args[0] == "api" and "/check-runs" in args[1]: + return (checks_rc, statuses if checks_rc == 0 else "", "HTTP 403" if checks_rc else "") + if args[0] == "api" and "/reviews" in args[1]: + if review is not None: + review.append(args) + return (0, json.dumps({"html_url": "https://example/review/1"}), "") + raise AssertionError(f"unexpected gh call: {args}") + + return run_gh + + +# ── COMMENT: always allowed, no guards ─────────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_comment_posts_without_any_guard_calls(): + posted = [] + with patch("ghplugin.review_tools.run_gh", new=_gh(review=posted)): + out = await _tool("github_review_comment").ainvoke({"number": 7, "body": "findings…"}) + assert "Posted COMMENT review" in out + assert any("event=COMMENT" in a for a in posted[0]) + + +# ── APPROVE / REQUEST_CHANGES: the CI-terminal guard (#863) ────────────────────── + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "tool_name,verdict", [("github_review_approve", "APPROVE"), ("github_review_request_changes", "REQUEST_CHANGES")] +) +async def test_blocking_verdict_refused_while_ci_pending(tool_name, verdict): + posted = [] + with patch( + "ghplugin.review_tools.run_gh", + new=_gh(statuses='["completed","in_progress"]', review=posted), + ): + out = await _tool(tool_name).ainvoke({"number": 7, "body": "verdict"}) + assert "Held" in out and "github_review_comment" in out + assert "do not wait or poll" in out or "do not wait" in out + assert not posted # nothing reached the Review API + + +@pytest.mark.asyncio +async def test_blocking_verdict_refused_when_ci_unreadable_fails_closed(): + posted = [] + with patch("ghplugin.review_tools.run_gh", new=_gh(checks_rc=1, review=posted)): + out = await _tool("github_review_approve").ainvoke({"number": 7, "body": "lgtm"}) + assert "Held" in out and "cannot be read" in out + assert not posted + + +@pytest.mark.asyncio +async def test_approve_posts_when_ci_terminal_and_author_differs(): + posted = [] + with patch("ghplugin.review_tools.run_gh", new=_gh(review=posted)): + out = await _tool("github_review_approve").ainvoke({"number": 7, "body": "verified"}) + assert "Posted APPROVE review" in out + assert any("event=APPROVE" in a for a in posted[0]) + + +@pytest.mark.asyncio +async def test_no_checks_at_all_is_terminal_by_definition(): + posted = [] + with patch("ghplugin.review_tools.run_gh", new=_gh(statuses="[]", review=posted)): + out = await _tool("github_review_approve").ainvoke({"number": 7, "body": "verified"}) + assert "Posted APPROVE review" in out + + +# ── the self-review guard ──────────────────────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_blocking_verdict_refused_on_own_pr(): + posted = [] + with patch("ghplugin.review_tools.run_gh", new=_gh(pr_json=OWN_PR_JSON, review=posted)): + out = await _tool("github_review_approve").ainvoke({"number": 7, "body": "ship it"}) + assert "Held" in out and "own work" in out + assert not posted + + +@pytest.mark.asyncio +async def test_comment_on_own_pr_stays_allowed(): + posted = [] + with patch("ghplugin.review_tools.run_gh", new=_gh(pr_json=OWN_PR_JSON, review=posted)): + out = await _tool("github_review_comment").ainvoke({"number": 7, "body": "notes"}) + assert "Posted COMMENT review" in out + + +# ── hygiene ────────────────────────────────────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_empty_body_and_bad_repo_are_rejected(): + with patch("ghplugin.review_tools.run_gh", new=_gh()): + assert "empty" in await _tool("github_review_approve").ainvoke({"number": 7, "body": " "}) + out = await _tool("github_review_comment").ainvoke({"number": 7, "body": "x", "repo": "bad"}) + assert out.startswith("Error")