Skip to content

Fix bugs, improve validation, and expand test coverage#7

Open
Keramikus-97 wants to merge 1 commit into
mainfrom
devin/1780764654-code-review-fixes
Open

Fix bugs, improve validation, and expand test coverage#7
Keramikus-97 wants to merge 1 commit into
mainfrom
devin/1780764654-code-review-fixes

Conversation

@Keramikus-97

Copy link
Copy Markdown
Owner

Summary

Implements all fixes identified during a thorough code review of the codebase. The three most impactful changes:

1. list_issue_comments now paginates — previously only fetched the first page (30 comments), silently dropping data on active issues. Now iterates up to max_pages (default 10 → 1000 comments at 100/page):

# before: single request, max 30 results
data = await self._request("GET", f"/.../comments")

# after: follows pagination
while page <= max_pages:
    data = await self._request("GET", ..., params={"per_page": 100, "page": page})
    if len(data) < 100: break

2. _request now rejects 3xx and invalid JSON — previously a 301 redirect was treated as success and its body parsed as JSON (likely crashing). Now raises GitHubAPIError for both cases. follow_redirects=False is set on the httpx client.

3. Config.from_env validates timeout boundsOPENCODE_TIMEOUT=-5 or 999 was silently accepted and passed to httpx. Now clamped to [1, 300] with a logged warning.

Other fixes:

  • parse_payload rejects webhooks with comment_id=0, empty sender_login, or issue_number=0 instead of creating unusable WebhookEvent objects
  • extract_commands regex: \b(?=\s|$) so triggers ending in non-word chars (e.g. /oc+) work
  • 17 new tests covering all the above + edge cases (empty responses, unmatched quotes, etc.)
  • README documents usage, configuration env vars, and dev setup

Note: The workflow file (.github/workflows/opencode.yml) also needs hardening (actor filtering, concurrency, timeout, full clone). The changes are ready locally but couldn't be pushed due to OAuth workflow scope restrictions. The recommended workflow diff:

+concurrency:
+  group: opencode-${{ github.event.issue.number || ... }}
+  cancel-in-progress: true
+
 jobs:
   opencode:
-    if: contains(...)
+    if: |
+      (...) &&
+      contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)
+    timeout-minutes: 15
     ...
-          fetch-depth: 1
-          persist-credentials: false
+          fetch-depth: 0
+          persist-credentials: true

Link to Devin session: https://app.devin.ai/sessions/40cdfcb7b4614be1934ac754a3c1d013
Requested by: @Keramikus-97

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 <dominicpape@gmx.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant