From 1bcca77a412bfdb397c445020b15f9a8b608fd3c Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Sat, 6 Jun 2026 16:55:13 +0000 Subject: [PATCH] Fix bugs, improve validation, and expand test coverage config.py: - Validate timeout bounds [1, 300], clamp out-of-range values - Log warnings for invalid/out-of-range OPENCODE_TIMEOUT github_client.py: - Add pagination to list_issue_comments (up to 10 pages / 1000 comments) - Reject 3xx redirects explicitly instead of silently accepting - Catch JSON decode errors and wrap in GitHubAPIError - Disable follow_redirects on httpx client webhook_handler.py: - Reject payloads with missing comment_id, sender_login, or issue_number comment_parser.py: - Replace word-boundary with lookahead to support triggers ending in non-word characters (e.g. /oc+) - Document unmatched-quote behavior in split_arguments Tests (17 new): - Pagination, redirects, JSON errors, timeout clamping, field validation, unmatched quotes, special-char triggers, empty responses README.md: - Document usage, configuration, and dev setup Co-Authored-By: dominicpape --- README.md | 29 ++++++++++- src/opencode_github/comment_parser.py | 7 ++- src/opencode_github/config.py | 25 ++++++++- src/opencode_github/github_client.py | 58 +++++++++++++++++---- src/opencode_github/webhook_handler.py | 10 +++- tests/test_comment_parser.py | 24 +++++++++ tests/test_config.py | 29 ++++++++++- tests/test_github_client.py | 71 ++++++++++++++++++++++++++ tests/test_webhook_handler.py | 34 ++++++++++-- 9 files changed, 263 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index 6934471..75597bd 100644 --- a/README.md +++ b/README.md @@ -1,3 +1,30 @@ # OpenCode GitHub Integration -Repository for OpenCode GitHub Actions integration. +Python helpers and a GitHub Actions workflow for triggering [OpenCode](https://github.com/anomalyco/opencode) from issue and PR comments. + +## Quick start + +1. Add the `ANTHROPIC_API_KEY` secret to your repository (Settings → Secrets and variables → Actions). +2. Copy `.github/workflows/opencode.yml` into your repo. +3. Comment `/oc ` or `/opencode ` on any issue or PR. + +Only repository **owners**, **members**, and **collaborators** can trigger the bot. + +## Configuration + +| Environment variable | Default | Description | +|---|---|---| +| `ANTHROPIC_API_KEY` | *(required)* | Anthropic API key | +| `GITHUB_TOKEN` | *(auto-provided)* | GitHub token for API calls | +| `OPENCODE_MODEL` | `anthropic/claude-sonnet-4-20250514` | Model identifier | +| `GITHUB_API_URL` | `https://api.github.com` | GitHub API base URL (for GHES) | +| `OPENCODE_COMMANDS` | `/oc,/opencode` | Comma-separated trigger prefixes | +| `OPENCODE_TIMEOUT` | `30` | HTTP request timeout in seconds (1–300) | + +## Development + +```bash +pip install -e ".[dev]" +pytest +ruff check . +``` diff --git a/src/opencode_github/comment_parser.py b/src/opencode_github/comment_parser.py index 9f5e44c..9689c77 100644 --- a/src/opencode_github/comment_parser.py +++ b/src/opencode_github/comment_parser.py @@ -39,7 +39,7 @@ def extract_commands(body: str, allowed_triggers: list[str]) -> list[ParsedComma escaped = [re.escape(t) for t in sorted(allowed_triggers, key=len, reverse=True)] pattern = re.compile( - r"^\s*(" + "|".join(escaped) + r")\b\s*(.*?)\s*$", + r"^\s*(" + "|".join(escaped) + r")(?=\s|$)\s*(.*?)\s*$", re.MULTILINE, ) @@ -63,8 +63,13 @@ def is_command_comment(body: str, allowed_triggers: list[str]) -> bool: def split_arguments(arguments: str) -> list[str]: """Split an argument string into tokens respecting double-quoted groups. + Unmatched quotes are treated as literal characters — the remaining text + is collected into the final token. + >>> split_arguments('fix bug --verbose "hello world"') ['fix', 'bug', '--verbose', 'hello world'] + >>> split_arguments('fix "unterminated') + ['fix', 'unterminated'] """ tokens: list[str] = [] current: list[str] = [] diff --git a/src/opencode_github/config.py b/src/opencode_github/config.py index 705b120..7607771 100644 --- a/src/opencode_github/config.py +++ b/src/opencode_github/config.py @@ -2,9 +2,16 @@ from __future__ import annotations +import logging import os from dataclasses import dataclass, field +logger = logging.getLogger(__name__) + +MIN_TIMEOUT = 1 +MAX_TIMEOUT = 300 +DEFAULT_TIMEOUT = 30 + @dataclass(frozen=True) class Config: @@ -47,11 +54,25 @@ def from_env(cls, environ: dict[str, str] | None = None) -> Config: allowed_raw = env.get("OPENCODE_COMMANDS", "/oc,/opencode").strip() allowed_commands = [cmd.strip() for cmd in allowed_raw.split(",") if cmd.strip()] - timeout_raw = env.get("OPENCODE_TIMEOUT", "30").strip() + timeout_raw = env.get("OPENCODE_TIMEOUT", str(DEFAULT_TIMEOUT)).strip() try: request_timeout = int(timeout_raw) except ValueError: - request_timeout = 30 + logger.warning( + "Invalid OPENCODE_TIMEOUT value %r, falling back to %d", + timeout_raw, + DEFAULT_TIMEOUT, + ) + request_timeout = DEFAULT_TIMEOUT + + if request_timeout < MIN_TIMEOUT or request_timeout > MAX_TIMEOUT: + logger.warning( + "OPENCODE_TIMEOUT=%d out of range [%d, %d], clamping", + request_timeout, + MIN_TIMEOUT, + MAX_TIMEOUT, + ) + request_timeout = max(MIN_TIMEOUT, min(request_timeout, MAX_TIMEOUT)) return cls( github_token=github_token, diff --git a/src/opencode_github/github_client.py b/src/opencode_github/github_client.py index dfb357e..3ff9c20 100644 --- a/src/opencode_github/github_client.py +++ b/src/opencode_github/github_client.py @@ -2,11 +2,14 @@ from __future__ import annotations +import logging from dataclasses import dataclass from typing import Any import httpx +logger = logging.getLogger(__name__) + @dataclass(frozen=True) class PullRequest: @@ -67,6 +70,7 @@ def __init__( "X-GitHub-Api-Version": "2022-11-28", }, timeout=timeout, + follow_redirects=False, ) async def close(self) -> None: @@ -82,9 +86,20 @@ async def _request(self, method: str, path: str, **kwargs: Any) -> Any: resp = await self._client.request(method, path, **kwargs) if resp.status_code >= 400: raise GitHubAPIError(resp.status_code, resp.text) + if resp.is_redirect or resp.status_code >= 300: + raise GitHubAPIError( + resp.status_code, + f"Unexpected redirect to {resp.headers.get('location', '?')}", + ) if resp.status_code == 204: return None - return resp.json() + try: + return resp.json() + except ValueError as exc: + raise GitHubAPIError( + resp.status_code, + f"Invalid JSON in response body: {exc}", + ) from exc async def get_pull_request(self, owner: str, repo: str, number: int) -> PullRequest: data = await self._request("GET", f"/repos/{owner}/{repo}/pulls/{number}") @@ -97,18 +112,39 @@ async def get_pull_request(self, owner: str, repo: str, number: int) -> PullRequ ) async def list_issue_comments( - self, owner: str, repo: str, issue_number: int + self, owner: str, repo: str, issue_number: int, *, max_pages: int = 10 ) -> list[IssueComment]: - data = await self._request("GET", f"/repos/{owner}/{repo}/issues/{issue_number}/comments") - return [ - IssueComment( - id=c["id"], - body=c.get("body") or "", - user_login=c["user"]["login"], - html_url=c["html_url"], + """Fetch all comments for an issue, following pagination. + + Parameters + ---------- + max_pages: + Safety limit to prevent runaway pagination. Defaults to 10 + (up to 1000 comments at 100 per page). + """ + comments: list[IssueComment] = [] + page = 1 + while page <= max_pages: + data = await self._request( + "GET", + f"/repos/{owner}/{repo}/issues/{issue_number}/comments", + params={"per_page": 100, "page": page}, + ) + if not data: + break + comments.extend( + IssueComment( + id=c["id"], + body=c.get("body") or "", + user_login=c["user"]["login"], + html_url=c["html_url"], + ) + for c in data ) - for c in data - ] + if len(data) < 100: + break + page += 1 + return comments async def create_issue_comment( self, owner: str, repo: str, issue_number: int, body: str diff --git a/src/opencode_github/webhook_handler.py b/src/opencode_github/webhook_handler.py index 9e3fa68..2a5b93f 100644 --- a/src/opencode_github/webhook_handler.py +++ b/src/opencode_github/webhook_handler.py @@ -102,12 +102,18 @@ def parse_payload(event_type: EventType, payload: dict[str, Any]) -> WebhookEven elif event_type == EventType.PR_REVIEW_COMMENT: issue_number = payload.get("pull_request", {}).get("number", 0) + comment_id = comment.get("id", 0) + sender_login = comment.get("user", {}).get("login", "") + + if not comment_id or not sender_login or not issue_number: + return None + return WebhookEvent( event_type=event_type, action=action, comment_body=comment.get("body", ""), - comment_id=comment.get("id", 0), - sender_login=comment.get("user", {}).get("login", ""), + comment_id=comment_id, + sender_login=sender_login, repo_owner=owner_data.get("login", ""), repo_name=repo_data.get("name", ""), issue_number=issue_number, diff --git a/tests/test_comment_parser.py b/tests/test_comment_parser.py index 4de0335..8b8e1d1 100644 --- a/tests/test_comment_parser.py +++ b/tests/test_comment_parser.py @@ -64,6 +64,16 @@ def test_trigger_prefix_not_partial_word(self) -> None: """/ocean should NOT match /oc.""" assert extract_commands("/ocean voyage", TRIGGERS) == [] + def test_none_body(self) -> None: + """Passing None-ish empty string should return empty.""" + assert extract_commands("", TRIGGERS) == [] + + def test_trigger_with_special_regex_chars(self) -> None: + """Triggers with regex metacharacters are escaped properly.""" + cmds = extract_commands("/oc+ hello", ["/oc+"]) + assert len(cmds) == 1 + assert cmds[0].trigger == "/oc+" + class TestIsCommandComment: def test_true_for_matching(self) -> None: @@ -94,3 +104,17 @@ def test_adjacent_quotes(self) -> None: def test_no_quotes(self) -> None: assert split_arguments("a b c") == ["a", "b", "c"] + + def test_unmatched_quote(self) -> None: + """Unmatched opening quote collects remaining text as one token.""" + result = split_arguments('fix "hello world') + assert result == ["fix", "hello world"] + + def test_empty_quoted_string(self) -> None: + """Empty quotes produce no token (quotes stripped, nothing inside).""" + result = split_arguments('a "" b') + assert result == ["a", "b"] + + def test_consecutive_spaces(self) -> None: + result = split_arguments("a b c") + assert result == ["a", "b", "c"] diff --git a/tests/test_config.py b/tests/test_config.py index 183deff..a5f1be0 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -4,7 +4,7 @@ import pytest -from opencode_github.config import Config +from opencode_github.config import DEFAULT_TIMEOUT, MAX_TIMEOUT, MIN_TIMEOUT, Config class TestConfigFromEnv: @@ -42,7 +42,32 @@ def test_custom_timeout(self, minimal_env: dict[str, str]) -> None: def test_invalid_timeout_falls_back(self, minimal_env: dict[str, str]) -> None: minimal_env["OPENCODE_TIMEOUT"] = "not_a_number" cfg = Config.from_env(minimal_env) - assert cfg.request_timeout == 30 + assert cfg.request_timeout == DEFAULT_TIMEOUT + + def test_negative_timeout_clamped(self, minimal_env: dict[str, str]) -> None: + minimal_env["OPENCODE_TIMEOUT"] = "-5" + cfg = Config.from_env(minimal_env) + assert cfg.request_timeout == MIN_TIMEOUT + + def test_zero_timeout_clamped(self, minimal_env: dict[str, str]) -> None: + minimal_env["OPENCODE_TIMEOUT"] = "0" + cfg = Config.from_env(minimal_env) + assert cfg.request_timeout == MIN_TIMEOUT + + def test_excessive_timeout_clamped(self, minimal_env: dict[str, str]) -> None: + minimal_env["OPENCODE_TIMEOUT"] = "9999" + cfg = Config.from_env(minimal_env) + assert cfg.request_timeout == MAX_TIMEOUT + + def test_boundary_min_timeout(self, minimal_env: dict[str, str]) -> None: + minimal_env["OPENCODE_TIMEOUT"] = str(MIN_TIMEOUT) + cfg = Config.from_env(minimal_env) + assert cfg.request_timeout == MIN_TIMEOUT + + def test_boundary_max_timeout(self, minimal_env: dict[str, str]) -> None: + minimal_env["OPENCODE_TIMEOUT"] = str(MAX_TIMEOUT) + cfg = Config.from_env(minimal_env) + assert cfg.request_timeout == MAX_TIMEOUT def test_whitespace_stripped(self, minimal_env: dict[str, str]) -> None: minimal_env["GITHUB_TOKEN"] = " token_with_spaces " diff --git a/tests/test_github_client.py b/tests/test_github_client.py index a1ed8f9..748aca0 100644 --- a/tests/test_github_client.py +++ b/tests/test_github_client.py @@ -105,6 +105,52 @@ async def test_success(self, mock_router: respx.MockRouter, client: GitHubClient ) assert comments[1].body == "" + async def test_empty_response( + self, mock_router: respx.MockRouter, client: GitHubClient + ) -> None: + mock_router.get("/repos/owner/repo/issues/1/comments").mock( + return_value=httpx.Response(200, json=[]) + ) + comments = await client.list_issue_comments("owner", "repo", 1) + assert comments == [] + + async def test_pagination(self, mock_router: respx.MockRouter, client: GitHubClient) -> None: + """Verify the client follows pagination when a full page is returned.""" + page1 = [ + { + "id": i, + "body": f"comment {i}", + "user": {"login": "user"}, + "html_url": f"https://github.com/o/r/issues/1#issuecomment-{i}", + } + for i in range(100) + ] + page2 = [ + { + "id": 200, + "body": "last", + "user": {"login": "user"}, + "html_url": "https://github.com/o/r/issues/1#issuecomment-200", + } + ] + + call_count = 0 + + def side_effect(request: httpx.Request) -> httpx.Response: + nonlocal call_count + call_count += 1 + page = int(request.url.params.get("page", "1")) + if page == 1: + return httpx.Response(200, json=page1) + return httpx.Response(200, json=page2) + + mock_router.get("/repos/o/r/issues/1/comments").mock(side_effect=side_effect) + + comments = await client.list_issue_comments("o", "r", 1) + assert len(comments) == 101 + assert comments[-1].body == "last" + assert call_count == 2 + class TestCreateIssueComment: async def test_success(self, mock_router: respx.MockRouter, client: GitHubClient) -> None: @@ -153,3 +199,28 @@ class TestContextManager: async def test_async_with(self) -> None: async with GitHubClient(token="tok") as c: assert c._token == "tok" + + +class TestRedirectHandling: + async def test_3xx_raises(self, mock_router: respx.MockRouter, client: GitHubClient) -> None: + mock_router.get("/repos/owner/repo").mock( + return_value=httpx.Response( + 301, headers={"location": "https://api.github.com/repos/new-owner/repo"} + ) + ) + with pytest.raises(GitHubAPIError) as exc_info: + await client.get_repo("owner", "repo") + assert exc_info.value.status_code == 301 + assert "redirect" in exc_info.value.detail.lower() + + +class TestInvalidJsonResponse: + async def test_invalid_json_raises( + self, mock_router: respx.MockRouter, client: GitHubClient + ) -> None: + mock_router.get("/repos/owner/repo").mock( + return_value=httpx.Response(200, text="not json at all") + ) + with pytest.raises(GitHubAPIError) as exc_info: + await client.get_repo("owner", "repo") + assert "Invalid JSON" in exc_info.value.detail diff --git a/tests/test_webhook_handler.py b/tests/test_webhook_handler.py index 6b5b99a..0f303bc 100644 --- a/tests/test_webhook_handler.py +++ b/tests/test_webhook_handler.py @@ -94,16 +94,40 @@ def test_missing_comment(self) -> None: payload = {"action": "created", "repository": {"name": "r", "owner": {"login": "o"}}} assert parse_payload(EventType.ISSUE_COMMENT, payload) is None - def test_missing_repo_info(self) -> None: + def test_missing_repo_info_still_returns_none_if_no_issue(self) -> None: + """Without an issue number, parse_payload returns None (validation).""" payload = { "action": "created", "comment": {"id": 1, "body": "x", "user": {"login": "u"}, "html_url": ""}, + } + assert parse_payload(EventType.ISSUE_COMMENT, payload) is None + + def test_missing_comment_id_returns_none(self) -> None: + payload = { + "action": "created", + "comment": {"id": 0, "body": "/oc hi", "user": {"login": "u"}, "html_url": ""}, "issue": {"number": 1}, + "repository": {"name": "r", "owner": {"login": "o"}}, } - event = parse_payload(EventType.ISSUE_COMMENT, payload) - assert event is not None - assert event.repo_owner == "" - assert event.repo_name == "" + assert parse_payload(EventType.ISSUE_COMMENT, payload) is None + + def test_missing_sender_login_returns_none(self) -> None: + payload = { + "action": "created", + "comment": {"id": 1, "body": "/oc hi", "user": {"login": ""}, "html_url": ""}, + "issue": {"number": 1}, + "repository": {"name": "r", "owner": {"login": "o"}}, + } + assert parse_payload(EventType.ISSUE_COMMENT, payload) is None + + def test_missing_issue_number_returns_none(self) -> None: + payload = { + "action": "created", + "comment": {"id": 1, "body": "/oc hi", "user": {"login": "u"}, "html_url": ""}, + "issue": {"number": 0}, + "repository": {"name": "r", "owner": {"login": "o"}}, + } + assert parse_payload(EventType.ISSUE_COMMENT, payload) is None class TestParseRaw: