From 9a727189b8717bfa3fa384a7b51137eb5432f471 Mon Sep 17 00:00:00 2001 From: amirbena Date: Sat, 26 Sep 2026 16:56:49 +0300 Subject: [PATCH 1/3] Add GitHub integration execution surface and shared call boundary (#547) Co-Authored-By: Claude Sonnet 5 --- docs/github-integration/execution-surface.md | 46 +++++ scripts/github_integration/__init__.py | 0 scripts/github_integration/boundary.py | 171 ++++++++++++++++++ tests/unit/github_integration/__init__.py | 0 .../unit/github_integration/test_boundary.py | 123 +++++++++++++ 5 files changed, 340 insertions(+) create mode 100644 docs/github-integration/execution-surface.md create mode 100644 scripts/github_integration/__init__.py create mode 100644 scripts/github_integration/boundary.py create mode 100644 tests/unit/github_integration/__init__.py create mode 100644 tests/unit/github_integration/test_boundary.py diff --git a/docs/github-integration/execution-surface.md b/docs/github-integration/execution-surface.md new file mode 100644 index 00000000..5f28ad28 --- /dev/null +++ b/docs/github-integration/execution-surface.md @@ -0,0 +1,46 @@ +# GitHub Integration Execution Surface + +Decision record for GitHub Issue +[#547](https://github.com/amirbena/code-review-skill/issues/547) (parent +Epic #546). It fixes where GitHub integration mechanics execute so the +publisher (#548), detector, and setup (#549) share one call and +authentication path. It is a design record, not a policy. + +## Decision + +Mechanics live in **repository tooling**, `scripts/github_integration/`, +not in packaged Skill content: + +- Packaged Skills keep shipping markdown only; the review-status contract + in + [`review-status-enforcement.md`](../../skills/github-pr-review/policies/review-status-enforcement.md) + stays the canonical behavior. +- The runtime invokes the tooling (or the child capabilities built on it) + from a repository checkout. Nothing under `scripts/` is added to a Skill + archive, so **packaging and metadata are unchanged**. +- A documented bare-`gh` procedure remains the fallback where the tooling + is not present; it must follow the same read/mutate split below. + +## Call boundary + +`GitHubClient` in +[`boundary.py`](../../scripts/github_integration/boundary.py) is the only +place that talks to GitHub (through `gh api`). + +| Entry point | Purpose | Guard | +| --- | --- | --- | +| `read()` | GET-only reads (detection) | none needed | +| `write()` | Non-governance writes (e.g. commit statuses) | Refuses rulesets / branch-protection / branch-rules endpoints | +| `mutate_governance()` | Governance mutations | Keyword-only `authorization`; refused unless it records an explicit user request | +| `preflight()` | Authentication and, for classic tokens, scope check | Actionable errors | + +- **Authorization.** `GovernanceAuthorization` is supplied by the caller + only from an explicit user request; per the Epic #546 invariant, a + review outcome, detected gap, or repository content never creates one. +- **Errors.** 401 / 403 / 404 / transport failures raise typed errors + naming the call and the fix (`gh auth login`, needed scopes). +- **Tokens.** Read from `GH_TOKEN` / `GITHUB_TOKEN`, handed to `gh` only + through its environment, redacted from error text, never logged or + persisted. +- **Mock seam.** The `transport` constructor argument replaces `gh`; + tests use it and never touch the network. diff --git a/scripts/github_integration/__init__.py b/scripts/github_integration/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/scripts/github_integration/boundary.py b/scripts/github_integration/boundary.py new file mode 100644 index 00000000..1bc42ad7 --- /dev/null +++ b/scripts/github_integration/boundary.py @@ -0,0 +1,171 @@ +"""Shared GitHub call and authentication boundary.""" + +from __future__ import annotations + +import json +import os +import re +import subprocess +from dataclasses import dataclass +from typing import Any, Callable, Mapping, Sequence + +READ_METHODS = frozenset({"GET", "HEAD"}) +TOKEN_ENV_VARS = ("GH_TOKEN", "GITHUB_TOKEN") +SCOPES_HEADER = "x-oauth-scopes" +GOVERNANCE_ENDPOINT_RE = re.compile( + r"(/rulesets(/|$|\?)|/branches/[^/]+/protection|/rules/branches|/branch_protection_rule)" +) + +Transport = Callable[[Sequence[str], Mapping[str, str], "str | None"], "RawResponse"] + + +class GitHubBoundaryError(Exception): + """Base error carrying an actionable, token-free message.""" + + +class AuthenticationError(GitHubBoundaryError): + pass + + +class GitHubPermissionError(GitHubBoundaryError): + pass + + +class AuthorizationRequiredError(GitHubBoundaryError): + pass + + +class GitHubCallError(GitHubBoundaryError): + pass + + +@dataclass(frozen=True) +class RawResponse: + status: int + body: str = "" + headers: Mapping[str, str] | None = None + + +@dataclass(frozen=True) +class GovernanceAuthorization: + """Explicit user request naming the governance change being authorized.""" + + requested_by_user: bool + description: str + + def is_valid(self) -> bool: + return self.requested_by_user is True and bool(self.description.strip()) + + +def _redact(text: str, token: str | None) -> str: + return text.replace(token, "***") if token else text + + +def _gh_transport(args: Sequence[str], env: Mapping[str, str], stdin: str | None) -> RawResponse: + proc = subprocess.run( + ["gh", "api", "--include", *args], + input=stdin, + capture_output=True, + text=True, + env={**os.environ, **env}, + check=False, + ) + head, _, body = proc.stdout.partition("\r\n\r\n") + lines = head.splitlines() + status = int(lines[0].split()[1]) if lines and lines[0].startswith("HTTP") else 0 + headers = { + k.strip().lower(): v.strip() + for k, _, v in (ln.partition(":") for ln in lines[1:]) + } + if status == 0: + return RawResponse(0, proc.stderr, headers) + return RawResponse(status, body, headers) + + +class GitHubClient: + """Single seam for GitHub reads and governance-mutating writes.""" + + def __init__(self, transport: Transport | None = None, env: Mapping[str, str] | None = None): + self._transport = transport or _gh_transport + self._env = os.environ if env is None else env + + def _token(self) -> str | None: + return next((self._env[v] for v in TOKEN_ENV_VARS if self._env.get(v)), None) + + def _auth_env(self) -> dict[str, str]: + token = self._token() + return {"GH_TOKEN": token} if token else {} + + def _call(self, method: str, endpoint: str, payload: Mapping[str, Any] | None) -> Any: + token = self._token() + args = ["-X", method, endpoint] + (["--input", "-"] if payload is not None else []) + body = json.dumps(payload) if payload is not None else None + resp = self._transport(args, self._auth_env(), body) + return self._interpret(method, endpoint, resp, token) + + def _interpret(self, method: str, endpoint: str, resp: RawResponse, token: str | None) -> Any: + if resp.status in (200, 201, 202, 204): + return json.loads(resp.body) if resp.body.strip() else None + detail = _redact(resp.body[:300], token) + where = f"{method} {endpoint}" + if resp.status == 0: + raise GitHubCallError(f"{where}: gh unavailable or unreachable: {detail}") + if resp.status == 401: + raise AuthenticationError( + f"{where}: not authenticated. Run `gh auth login` or set GH_TOKEN." + ) + if resp.status in (403, 404): + needed = (resp.headers or {}).get("x-accepted-oauth-scopes", "").strip() + hint = f" Token needs: {needed}." if needed else "" + raise GitHubPermissionError( + f"{where}: HTTP {resp.status}; token lacks access or resource not visible." + f"{hint} {detail}".strip() + ) + raise GitHubCallError(f"{where}: HTTP {resp.status}: {detail}") + + def preflight(self, required_scopes: Sequence[str] = ()) -> None: + """Verify authentication and, for classic tokens, required scopes.""" + resp = self._transport(["-X", "GET", "user"], self._auth_env(), None) + self._interpret("GET", "user", resp, self._token()) + header = (resp.headers or {}).get(SCOPES_HEADER) + if header is None or not required_scopes: + return + have = {s.strip() for s in header.split(",") if s.strip()} + missing = [s for s in required_scopes if s not in have] + if missing: + raise GitHubPermissionError( + f"Token missing scopes: {', '.join(missing)}. Re-authenticate with them." + ) + + def read(self, endpoint: str) -> Any: + return self._call("GET", endpoint, None) + + def write(self, method: str, endpoint: str, payload: Mapping[str, Any] | None = None) -> Any: + """Non-governance write (e.g. commit statuses); governance endpoints are refused.""" + method = method.upper() + if method in READ_METHODS: + raise GitHubBoundaryError("Use read() for read-only calls.") + if GOVERNANCE_ENDPOINT_RE.search(endpoint): + raise AuthorizationRequiredError( + f"Refusing {method} {endpoint}: governance endpoint; use mutate_governance()." + ) + return self._call(method, endpoint, payload) + + def mutate_governance( + self, + method: str, + endpoint: str, + payload: Mapping[str, Any] | None, + *, + authorization: GovernanceAuthorization | None, + ) -> Any: + """Governance write; refused unless the user explicitly authorized it.""" + method = method.upper() + if method in READ_METHODS: + raise GitHubBoundaryError("Use read() for read-only calls.") + if authorization is None or not authorization.is_valid(): + raise AuthorizationRequiredError( + f"Refusing {method} {endpoint}: governance mutation needs an explicit " + "user request (GovernanceAuthorization)." + ) + return self._call(method, endpoint, payload) diff --git a/tests/unit/github_integration/__init__.py b/tests/unit/github_integration/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/unit/github_integration/test_boundary.py b/tests/unit/github_integration/test_boundary.py new file mode 100644 index 00000000..f3447cb3 --- /dev/null +++ b/tests/unit/github_integration/test_boundary.py @@ -0,0 +1,123 @@ +"""Tests for the shared GitHub call boundary against a mocked transport.""" + +from __future__ import annotations + +import json +import unittest + +from scripts.github_integration import boundary as b + + +class Recorder: + def __init__(self, *responses: b.RawResponse): + self.responses = list(responses) + self.calls: list[tuple] = [] + + def __call__(self, args, env, stdin): + self.calls.append((list(args), dict(env), stdin)) + return self.responses.pop(0) + + +def client(*responses, env=None): + rec = Recorder(*responses) + return b.GitHubClient(rec, env if env is not None else {"GH_TOKEN": "sekret"}), rec + + +AUTH = b.GovernanceAuthorization(True, "add review context as required check") + + +class ReadTests(unittest.TestCase): + def test_read_returns_json_and_passes_token_via_env(self): + c, rec = client(b.RawResponse(200, '{"a": 1}')) + self.assertEqual(c.read("repos/o/r"), {"a": 1}) + self.assertEqual(rec.calls[0][1], {"GH_TOKEN": "sekret"}) + self.assertNotIn("sekret", " ".join(rec.calls[0][0])) + + def test_401_is_actionable(self): + c, _ = client(b.RawResponse(401, "bad")) + with self.assertRaisesRegex(b.AuthenticationError, "gh auth login"): + c.read("user") + + def test_403_reports_needed_scopes_and_redacts_token(self): + c, _ = client( + b.RawResponse(403, "denied sekret", {"x-accepted-oauth-scopes": "repo"}) + ) + with self.assertRaises(b.GitHubPermissionError) as ctx: + c.read("repos/o/r") + self.assertIn("repo", str(ctx.exception)) + self.assertNotIn("sekret", str(ctx.exception)) + + def test_unreachable_gh(self): + c, _ = client(b.RawResponse(0, "no gh")) + with self.assertRaises(b.GitHubCallError): + c.read("user") + + +class PreflightTests(unittest.TestCase): + def test_missing_scope_fails(self): + c, _ = client(b.RawResponse(200, "{}", {"x-oauth-scopes": "read:org"})) + with self.assertRaisesRegex(b.GitHubPermissionError, "repo:status"): + c.preflight(["repo:status"]) + + def test_fine_grained_token_without_scope_header_passes(self): + c, _ = client(b.RawResponse(200, "{}", {})) + c.preflight(["repo:status"]) + + +class WriteTests(unittest.TestCase): + def test_write_sends_payload_on_stdin(self): + c, rec = client(b.RawResponse(201, "{}")) + c.write("POST", "repos/o/r/statuses/abc", {"state": "success"}) + self.assertEqual(json.loads(rec.calls[0][2]), {"state": "success"}) + + def test_write_refuses_governance_endpoints(self): + for ep in ( + "repos/o/r/rulesets/1", + "repos/o/r/branches/main/protection/required_status_checks", + ): + c, rec = client() + with self.assertRaises(b.AuthorizationRequiredError): + c.write("PUT", ep, {}) + self.assertEqual(rec.calls, []) + + def test_write_rejects_read_method(self): + c, _ = client() + with self.assertRaises(b.GitHubBoundaryError): + c.write("GET", "user") + + +class GovernanceTests(unittest.TestCase): + def test_refused_without_authorization(self): + c, rec = client() + with self.assertRaises(b.AuthorizationRequiredError): + c.mutate_governance("PUT", "repos/o/r/rulesets/1", {}, authorization=None) + self.assertEqual(rec.calls, []) + + def test_refused_with_invalid_authorization(self): + c, rec = client() + for auth in ( + b.GovernanceAuthorization(False, "x"), + b.GovernanceAuthorization(True, " "), + ): + with self.assertRaises(b.AuthorizationRequiredError): + c.mutate_governance("PUT", "repos/o/r/rulesets/1", {}, authorization=auth) + self.assertEqual(rec.calls, []) + + def test_authorization_is_keyword_only(self): + c, _ = client() + with self.assertRaises(TypeError): + c.mutate_governance("PUT", "repos/o/r/rulesets/1", {}, AUTH) # type: ignore[misc] + + def test_authorized_call_goes_through(self): + c, rec = client(b.RawResponse(200, "{}")) + c.mutate_governance("PUT", "repos/o/r/rulesets/1", {"a": 1}, authorization=AUTH) + self.assertEqual(len(rec.calls), 1) + + def test_read_method_rejected(self): + c, _ = client() + with self.assertRaises(b.GitHubBoundaryError): + c.mutate_governance("GET", "repos/o/r/rulesets", None, authorization=AUTH) + + +if __name__ == "__main__": + unittest.main() From 4e69b7c23f86801654c34273413ada39f439fbf8 Mon Sep 17 00:00:00 2001 From: amirbena Date: Sat, 26 Sep 2026 16:59:33 +0300 Subject: [PATCH 2/3] Harden GitHub boundary: allowlist writes, typed errors, redact before truncate (#547) Co-Authored-By: Claude Sonnet 5 --- docs/github-integration/execution-surface.md | 2 +- scripts/github_integration/boundary.py | 39 ++++++++++++------- .../unit/github_integration/test_boundary.py | 35 ++++++++++++++++- 3 files changed, 59 insertions(+), 17 deletions(-) diff --git a/docs/github-integration/execution-surface.md b/docs/github-integration/execution-surface.md index 5f28ad28..0a74e980 100644 --- a/docs/github-integration/execution-surface.md +++ b/docs/github-integration/execution-surface.md @@ -30,7 +30,7 @@ place that talks to GitHub (through `gh api`). | Entry point | Purpose | Guard | | --- | --- | --- | | `read()` | GET-only reads (detection) | none needed | -| `write()` | Non-governance writes (e.g. commit statuses) | Refuses rulesets / branch-protection / branch-rules endpoints | +| `write()` | Non-governance writes (e.g. commit statuses) | Allowlist only (statuses, PR/issue comments, PR reviews); everything else, including `graphql`, is refused | | `mutate_governance()` | Governance mutations | Keyword-only `authorization`; refused unless it records an explicit user request | | `preflight()` | Authentication and, for classic tokens, scope check | Actionable errors | diff --git a/scripts/github_integration/boundary.py b/scripts/github_integration/boundary.py index 1bc42ad7..3c1bf65c 100644 --- a/scripts/github_integration/boundary.py +++ b/scripts/github_integration/boundary.py @@ -12,8 +12,10 @@ READ_METHODS = frozenset({"GET", "HEAD"}) TOKEN_ENV_VARS = ("GH_TOKEN", "GITHUB_TOKEN") SCOPES_HEADER = "x-oauth-scopes" -GOVERNANCE_ENDPOINT_RE = re.compile( - r"(/rulesets(/|$|\?)|/branches/[^/]+/protection|/rules/branches|/branch_protection_rule)" +NON_GOVERNANCE_WRITE_RE = re.compile( + r"^repos/[^/]+/[^/]+/(statuses/[0-9a-f]{7,40}" + r"|issues/\d+/comments(/\d+)?" + r"|pulls/\d+/(reviews|comments)(/\d+)?)$" ) Transport = Callable[[Sequence[str], Mapping[str, str], "str | None"], "RawResponse"] @@ -62,14 +64,17 @@ def _redact(text: str, token: str | None) -> str: def _gh_transport(args: Sequence[str], env: Mapping[str, str], stdin: str | None) -> RawResponse: - proc = subprocess.run( - ["gh", "api", "--include", *args], - input=stdin, - capture_output=True, - text=True, - env={**os.environ, **env}, - check=False, - ) + try: + proc = subprocess.run( + ["gh", "api", "--include", *args], + input=stdin, + capture_output=True, + text=True, + env={**os.environ, **env}, + check=False, + ) + except OSError as exc: + return RawResponse(0, f"gh CLI not runnable ({exc}); install it or add it to PATH") head, _, body = proc.stdout.partition("\r\n\r\n") lines = head.splitlines() status = int(lines[0].split()[1]) if lines and lines[0].startswith("HTTP") else 0 @@ -105,8 +110,11 @@ def _call(self, method: str, endpoint: str, payload: Mapping[str, Any] | None) - def _interpret(self, method: str, endpoint: str, resp: RawResponse, token: str | None) -> Any: if resp.status in (200, 201, 202, 204): - return json.loads(resp.body) if resp.body.strip() else None - detail = _redact(resp.body[:300], token) + try: + return json.loads(resp.body) if resp.body.strip() else None + except ValueError as exc: + raise GitHubCallError(f"{method} {endpoint}: non-JSON response body") from exc + detail = _redact(resp.body, token)[:300] where = f"{method} {endpoint}" if resp.status == 0: raise GitHubCallError(f"{where}: gh unavailable or unreachable: {detail}") @@ -141,13 +149,14 @@ def read(self, endpoint: str) -> Any: return self._call("GET", endpoint, None) def write(self, method: str, endpoint: str, payload: Mapping[str, Any] | None = None) -> Any: - """Non-governance write (e.g. commit statuses); governance endpoints are refused.""" + """Write to an allowlisted non-governance endpoint; anything else is refused.""" method = method.upper() if method in READ_METHODS: raise GitHubBoundaryError("Use read() for read-only calls.") - if GOVERNANCE_ENDPOINT_RE.search(endpoint): + if not NON_GOVERNANCE_WRITE_RE.fullmatch(endpoint): raise AuthorizationRequiredError( - f"Refusing {method} {endpoint}: governance endpoint; use mutate_governance()." + f"Refusing {method} {endpoint}: not an allowlisted non-governance write; " + "use mutate_governance()." ) return self._call(method, endpoint, payload) diff --git a/tests/unit/github_integration/test_boundary.py b/tests/unit/github_integration/test_boundary.py index f3447cb3..d6e1efcc 100644 --- a/tests/unit/github_integration/test_boundary.py +++ b/tests/unit/github_integration/test_boundary.py @@ -4,6 +4,7 @@ import json import unittest +from unittest import mock from scripts.github_integration import boundary as b @@ -67,13 +68,17 @@ def test_fine_grained_token_without_scope_header_passes(self): class WriteTests(unittest.TestCase): def test_write_sends_payload_on_stdin(self): c, rec = client(b.RawResponse(201, "{}")) - c.write("POST", "repos/o/r/statuses/abc", {"state": "success"}) + c.write("POST", "repos/o/r/statuses/abc1234", {"state": "success"}) self.assertEqual(json.loads(rec.calls[0][2]), {"state": "success"}) def test_write_refuses_governance_endpoints(self): for ep in ( "repos/o/r/rulesets/1", "repos/o/r/branches/main/protection/required_status_checks", + "graphql", + "repos/o", + "repos/o/r", + "repos/o/r/statuses/abc1234/../../rulesets", ): c, rec = client() with self.assertRaises(b.AuthorizationRequiredError): @@ -86,6 +91,34 @@ def test_write_rejects_read_method(self): c.write("GET", "user") +class HardeningTests(unittest.TestCase): + def test_token_straddling_truncation_is_fully_redacted(self): + token = "tok" + "X" * 20 + c, _ = client(b.RawResponse(500, "a" * 290 + token), env={"GH_TOKEN": token}) + with self.assertRaises(b.GitHubCallError) as ctx: + c.read("user") + self.assertNotIn("XXX", str(ctx.exception)) + + def test_non_json_success_body_is_typed_error(self): + c, _ = client(b.RawResponse(200, "")) + with self.assertRaises(b.GitHubCallError): + c.read("user") + + def test_missing_gh_binary_is_actionable(self): + with mock.patch.object(b.subprocess, "run", side_effect=FileNotFoundError("gh")): + resp = b._gh_transport(["user"], {}, None) + self.assertEqual(resp.status, 0) + self.assertIn("PATH", resp.body) + + def test_gh_transport_parses_status_headers_and_body(self): + out = "HTTP/2.0 403 Forbidden\r\nX-Accepted-Oauth-Scopes: repo\r\n\r\n{\"m\": 1}" + proc = mock.Mock(stdout=out, stderr="") + with mock.patch.object(b.subprocess, "run", return_value=proc): + resp = b._gh_transport(["user"], {}, None) + self.assertEqual((resp.status, resp.body), (403, '{"m": 1}')) + self.assertEqual(resp.headers["x-accepted-oauth-scopes"], "repo") + + class GovernanceTests(unittest.TestCase): def test_refused_without_authorization(self): c, rec = client() From c422b83541f32eebe373d93865287ebb64ed8cca Mon Sep 17 00:00:00 2001 From: amirbena Date: Sat, 26 Sep 2026 17:01:17 +0300 Subject: [PATCH 3/3] Fix gh transport body loss and validate HTTP methods (#547) Co-Authored-By: Claude Sonnet 5 --- scripts/github_integration/boundary.py | 15 ++++++++--- .../unit/github_integration/test_boundary.py | 25 +++++++++++++++++-- 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/scripts/github_integration/boundary.py b/scripts/github_integration/boundary.py index 3c1bf65c..ff1b34d3 100644 --- a/scripts/github_integration/boundary.py +++ b/scripts/github_integration/boundary.py @@ -10,6 +10,8 @@ from typing import Any, Callable, Mapping, Sequence READ_METHODS = frozenset({"GET", "HEAD"}) +WRITE_METHODS = frozenset({"POST", "PATCH", "PUT"}) +GOVERNANCE_METHODS = WRITE_METHODS | {"DELETE"} TOKEN_ENV_VARS = ("GH_TOKEN", "GITHUB_TOKEN") SCOPES_HEADER = "x-oauth-scopes" NON_GOVERNANCE_WRITE_RE = re.compile( @@ -67,15 +69,16 @@ def _gh_transport(args: Sequence[str], env: Mapping[str, str], stdin: str | None try: proc = subprocess.run( ["gh", "api", "--include", *args], - input=stdin, + input=stdin.encode() if stdin is not None else None, capture_output=True, - text=True, env={**os.environ, **env}, check=False, ) except OSError as exc: return RawResponse(0, f"gh CLI not runnable ({exc}); install it or add it to PATH") - head, _, body = proc.stdout.partition("\r\n\r\n") + stdout = proc.stdout.decode("utf-8", "replace") + stderr = proc.stderr.decode("utf-8", "replace") + head, _, body = stdout.partition("\r\n\r\n") lines = head.splitlines() status = int(lines[0].split()[1]) if lines and lines[0].startswith("HTTP") else 0 headers = { @@ -83,7 +86,7 @@ def _gh_transport(args: Sequence[str], env: Mapping[str, str], stdin: str | None for k, _, v in (ln.partition(":") for ln in lines[1:]) } if status == 0: - return RawResponse(0, proc.stderr, headers) + return RawResponse(0, stderr, headers) return RawResponse(status, body, headers) @@ -153,6 +156,8 @@ def write(self, method: str, endpoint: str, payload: Mapping[str, Any] | None = method = method.upper() if method in READ_METHODS: raise GitHubBoundaryError("Use read() for read-only calls.") + if method not in WRITE_METHODS: + raise GitHubBoundaryError(f"Unsupported write method: {method!r}.") if not NON_GOVERNANCE_WRITE_RE.fullmatch(endpoint): raise AuthorizationRequiredError( f"Refusing {method} {endpoint}: not an allowlisted non-governance write; " @@ -172,6 +177,8 @@ def mutate_governance( method = method.upper() if method in READ_METHODS: raise GitHubBoundaryError("Use read() for read-only calls.") + if method not in GOVERNANCE_METHODS: + raise GitHubBoundaryError(f"Unsupported governance method: {method!r}.") if authorization is None or not authorization.is_valid(): raise AuthorizationRequiredError( f"Refusing {method} {endpoint}: governance mutation needs an explicit " diff --git a/tests/unit/github_integration/test_boundary.py b/tests/unit/github_integration/test_boundary.py index d6e1efcc..e7021c47 100644 --- a/tests/unit/github_integration/test_boundary.py +++ b/tests/unit/github_integration/test_boundary.py @@ -111,14 +111,35 @@ def test_missing_gh_binary_is_actionable(self): self.assertIn("PATH", resp.body) def test_gh_transport_parses_status_headers_and_body(self): - out = "HTTP/2.0 403 Forbidden\r\nX-Accepted-Oauth-Scopes: repo\r\n\r\n{\"m\": 1}" - proc = mock.Mock(stdout=out, stderr="") + out = b'HTTP/2.0 403 Forbidden\nX-Accepted-Oauth-Scopes: repo\r\n\r\n{"m": 1}' + proc = mock.Mock(stdout=out, stderr=b"") with mock.patch.object(b.subprocess, "run", return_value=proc): resp = b._gh_transport(["user"], {}, None) self.assertEqual((resp.status, resp.body), (403, '{"m": 1}')) self.assertEqual(resp.headers["x-accepted-oauth-scopes"], "repo") +class MethodTests(unittest.TestCase): + def test_invalid_methods_refused_before_transport(self): + c, rec = client() + for m in ("TRACE", " GET", "DELETE"): + with self.assertRaises(b.GitHubBoundaryError): + c.write(m, "repos/o/r/statuses/abc1234", {}) + with self.assertRaises(b.GitHubBoundaryError): + c.mutate_governance("TRACE", "repos/o/r/rulesets/1", {}, authorization=AUTH) + self.assertEqual(rec.calls, []) + + def test_governance_delete_allowed_when_authorized(self): + c, rec = client(b.RawResponse(204, "")) + c.mutate_governance("DELETE", "repos/o/r/rulesets/1", None, authorization=AUTH) + self.assertEqual(len(rec.calls), 1) + + def test_real_gh_read_returns_body(self): + out = b'HTTP/2.0 200 OK\nX-A: b\r\n\r\n{"ok": true}' + with mock.patch.object(b.subprocess, "run", return_value=mock.Mock(stdout=out, stderr=b"")): + self.assertEqual(b.GitHubClient(env={}).read("rate_limit"), {"ok": True}) + + class GovernanceTests(unittest.TestCase): def test_refused_without_authorization(self): c, rec = client()