Conversation
…checks/{issue_number}
Co-authored-by: honzajavorek <283441+honzajavorek@users.noreply.github.com>
honzajavorek
left a comment
honzajavorek
left a comment
There was a problem hiding this comment.
Look at my comments. Also:
- When your work is done, always run
uv run ruff format .,uv run ruff check --fix ., anduv run pytestbefore you really call it done.
Add this to AGENTS.md (if not present already, create it).
…prove error handling, parse JSON from comments Co-authored-by: honzajavorek <283441+honzajavorek@users.noreply.github.com>
Created AGENTS.md with instructions to run ruff format, ruff check --fix, and pytest before completing tasks. All commands have been run - 48 tests passing. (d8b35cf) |
honzajavorek
left a comment
honzajavorek
left a comment
There was a problem hiding this comment.
See my comments. Also see this advice on how to test githubkit code from the creator of the library - source yanyongyu/githubkit#98 (comment):
As you mentioned, the easiest way is to record the response data and save it (dict, JSON file, etc.). You can export the api response data by
resp.text(raw str data) orresp.json()(json parsed dict/list).After save the response data, you could rebuild the validated data with powerful Pydantic methods (Docs). For example:
from pydantic import TypeAdapter
from githubkit.versions.latest.models import SocialAccountaccounts: list[SocialAccount] = TypeAdapter(list[SocialAccount]).validate_json(json_content)
accounts: list[SocialAccount] = TypeAdapter(list[SocialAccount]).validate_python(python_list)
Record this approach to AGENTS.md as preferred over mocking and use it in tests where possible.
|
Also don't forget about #161 (comment) |
…gging, add fixtures Co-authored-by: honzajavorek <283441+honzajavorek@users.noreply.github.com>
Recorded githubkit testing approach to AGENTS.md with guidance to prefer fixtures over mocking, including Pydantic TypeAdapter examples from library creator. (c683f98) |
…thub_client, fixture as markdown Co-authored-by: honzajavorek <283441+honzajavorek@users.noreply.github.com>
The referenced comment (2773332879) appears to be about using pytest.mark.parametrize, which has been fully addressed in this commit. (43bc415) |
honzajavorek
left a comment
honzajavorek
left a comment
There was a problem hiding this comment.
I tried to remove some commas to see if the formatter squashes the lines, so apply those suggestions and let's see, but if the code folds and the formatter re-adds the commas, it's okay.
Let's rename github_client to github everywhere.
|
|
||
|
|
||
| def parse_eggtray_url(url: str) -> tuple[str, str]: | ||
| if not ( |
There was a problem hiding this comment.
Man, this is laughable. Please. I'll write it for you, but put it in your AGENTS.md somehow and learn how to use it.
if match := ...:
return match.groups()
raise ValueError(...)|
|
||
| def extract_actions_url(comment_body: str) -> str | None: | ||
| if actions_match := re.search( | ||
| r"https://github\.com/juniorguru/eggtray/actions/runs/(\d+)", |
There was a problem hiding this comment.
I just realized this won't work if we set different eggtray repo url. But it would be good enough if we just treat owner and repo as placeholders, we don't need to match them exactly. So don't add any arguments, just allow the owner and repo be variable in the regular expression.
| response_data = {"status": CheckStatus.COMPLETE} | ||
|
|
||
| if summary_data := parse_summary_json(comment_body): | ||
| response_data["data"] = summary_data |
There was a problem hiding this comment.
It's ok to set these properties even if they evaluate as None, so simplify it like this:
response_data = {"status": ..., "summary": parse_summary_json(comment_body), "actions_url": extract_actions_url(comment_body)}
Note that I also suggest renaming "data" to "summary".
| ) | ||
| return json_response( | ||
| {"status": CheckStatus.PENDING}, | ||
| status=202, |
There was a problem hiding this comment.
| status=202, | |
| status=202 |
| raise | ||
| return json_response( | ||
| {"error": "Internal server error", "debug_id": error_hash}, | ||
| status=500, |
There was a problem hiding this comment.
| status=500, | |
| status=500 |
| assert call_kwargs["owner"] == "juniorguru" | ||
| assert call_kwargs["repo"] == "eggtray" | ||
| assert "testuser" in call_kwargs["title"] | ||
| assert "testuser" in call_kwargs["body"] |
There was a problem hiding this comment.
| assert "testuser" in call_kwargs["body"] | |
| assert "@testuser" in call_kwargs["body"] |
This is actually important to test.
| issue_data = MagicMock(state="open") | ||
| github_client.rest.issues.get.return_value = MagicMock(parsed_data=issue_data) | ||
| github_client.rest.issues.list_comments.return_value = MagicMock( | ||
| parsed_data=[MagicMock(body="Just a regular comment")] |
There was a problem hiding this comment.
Maybe we could have it as a pytest fixture:
@pytest.fixture
def regular_comment():
return MagicMock(body="Just a regular comment")
Then use it across the tests.
| issue_data = MagicMock(state="closed") | ||
| github_client.rest.issues.get.return_value = MagicMock(parsed_data=issue_data) | ||
| github_client.rest.issues.list_comments.return_value = MagicMock( | ||
| parsed_data=[MagicMock(body=fixture_content.read_text())] |
There was a problem hiding this comment.
Could be another pytest fixture maybe? summary_comment?
| @@ -0,0 +1,37 @@ | |||
| Tak jsem si poctivě prošlo celý profil [github.com/bashynx](https://github.com/bashynx) a tady je moje zpětná vazba 🔬 | |||
|
|
|||
There was a problem hiding this comment.
Rename this file to summary_comment.md
| async def test_get_check_status_complete_with_fixture(api): | ||
| fixture_content = Path(__file__).parent / "fixtures" / "eggtray_comment.md" | ||
| github_client = api.app["github_client"] | ||
| issue_data = MagicMock(state="closed") |
There was a problem hiding this comment.
Let's parametrize this test with just two values, and let's test that it works completely the same with state="closed" as with state="open". That way we'll verify the mechanics are dependent on summary comment and not state of the issue.
|
@copilot Address my comments. |
There was a problem hiding this comment.
Pull request overview
This PR repurposes the chick project from a pure Discord bot into a Discord bot + API, adding two new API endpoints for GitHub profile reviews. The API provides progressive enhancement for the static junior.guru website, allowing users to request and check GitHub profile reviews via the eggtray automation system.
Changes:
- Added POST /checks/{github_username} endpoint to create GitHub issues for profile review requests
- Added GET /checks/{issue_number} endpoint to check the status and retrieve results of profile reviews
- Updated project description and README to reflect the bot + API architecture
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Added githubkit and pytest-aiohttp dependencies |
| uv.lock | Dependency lock file updates for new packages |
| README.md | Updated description to reflect "Discord bot and API" pivot, added API endpoints to features list |
| AGENTS.md | New file documenting code style conventions for AI agents (logging, walrus operators, minimal docstrings, test structure) |
| src/jg/chick/main.py | Added GitHub API key and eggtray URL configuration options, web app initialization |
| src/jg/chick/web.py | Removed (functionality moved to api.py) |
| src/jg/chick/api.py | New API module with uptime endpoint and two GitHub profile review endpoints, plus helper functions for parsing and error handling |
| tests/test_api.py | Comprehensive test coverage for all new API functionality using pytest parametrize and realistic fixtures |
| tests/fixtures/eggtray_comment.md | Real GitHub comment fixture with JSON summary data for testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| web = Application() | ||
| routes = RouteTableDef() | ||
|
|
||
|
|
||
| def parse_eggtray_url(url: str) -> tuple[str, str]: | ||
| if not ( | ||
| match := re.match( | ||
| r"https?://github\.com/([^/]+)/([^/]+)(?:/.*)?$", url.rstrip("/") | ||
| ) | ||
| ): | ||
| raise ValueError(f"Invalid GitHub repository URL: {url}") | ||
| return match.groups() | ||
|
|
||
|
|
||
| def find_summary_comment(comments: list) -> str | None: | ||
| for comment in reversed(comments): | ||
| if "```json" in (comment.body or ""): | ||
| return comment.body | ||
| return None | ||
|
|
||
|
|
||
| def parse_summary_json(comment_body: str) -> dict | None: | ||
| if json_match := re.search(r"```json\s*\n(.*?)\n```", comment_body, re.DOTALL): | ||
| return json.loads(json_match.group(1)) | ||
| return None | ||
|
|
||
|
|
||
| def extract_actions_url(comment_body: str) -> str | None: | ||
| if actions_match := re.search( | ||
| r"https://github\.com/juniorguru/eggtray/actions/runs/(\d+)", | ||
| comment_body, | ||
| ): | ||
| return actions_match.group(0) | ||
| return None | ||
|
|
||
|
|
||
| def generate_error_hash(error: Exception) -> str: | ||
| """Generate a unique hash for an error for tracking purposes.""" | ||
| error_string = ( | ||
| f"{datetime.now(UTC).isoformat()}-{type(error).__name__}-{str(error)}" | ||
| ) | ||
| return hashlib.sha256(error_string.encode()).hexdigest()[:8] | ||
|
|
||
|
|
||
| @routes.get("/") | ||
| async def index(request: Request) -> Response: | ||
| logger.info(f"Received {request!r}") | ||
| return json_response( | ||
| { | ||
| "status": "OK", | ||
| "launch_at": LAUNCH_AT.isoformat(), | ||
| "uptime_sec": (datetime.now(UTC) - LAUNCH_AT).total_seconds(), | ||
| } | ||
| ) | ||
|
|
||
|
|
||
| @routes.post("/checks/{github_username}") | ||
| async def create_check(request: Request) -> Response: | ||
| github_username = request.match_info["github_username"] | ||
| logger.info(f"Creating check for GitHub username: {github_username}") | ||
|
|
||
| github_client = request.app["github_client"] | ||
| eggtray_url = request.app["eggtray_url"] | ||
| debug = request.app["debug"] | ||
|
|
||
| try: | ||
| eggtray_owner, eggtray_repo = parse_eggtray_url(eggtray_url) | ||
|
|
||
| title = f"Zpětná vazba na profil @{github_username}" | ||
| body = f"Z webu junior.guru přišel požadavek na zpětnou vazbu k profilu @{github_username}." | ||
|
|
||
| logger.debug(f"Creating issue in {eggtray_owner}/{eggtray_repo}") | ||
| issue = github_client.rest.issues.create( | ||
| owner=eggtray_owner, | ||
| repo=eggtray_repo, | ||
| title=title, | ||
| body=body, | ||
| labels=["check"], | ||
| ) | ||
|
|
||
| issue_data = issue.parsed_data | ||
| logger.info(f"Created issue #{issue_data.number}: {issue_data.html_url}") | ||
|
|
||
| return json_response( | ||
| { | ||
| "url": issue_data.html_url, | ||
| "number": issue_data.number, | ||
| } | ||
| ) | ||
|
|
||
| except Exception as e: | ||
| error_hash = generate_error_hash(e) | ||
| logger.error(f"Error creating check [{error_hash}]: {e}") | ||
| if debug: | ||
| raise | ||
| return json_response( | ||
| {"error": "Internal server error", "debug_id": error_hash}, | ||
| status=500, | ||
| ) | ||
|
|
||
|
|
||
| @routes.get("/checks/{issue_number}") | ||
| async def get_check_status(request: Request) -> Response: | ||
| try: | ||
| issue_number = int(request.match_info["issue_number"]) | ||
| except ValueError: | ||
| return json_response( | ||
| {"error": "Invalid issue number"}, | ||
| status=400, | ||
| ) | ||
|
|
||
| logger.info(f"Getting check status for issue #{issue_number}") | ||
|
|
||
| github_client = request.app["github_client"] | ||
| eggtray_url = request.app["eggtray_url"] | ||
| debug = request.app["debug"] | ||
|
|
||
| try: | ||
| eggtray_owner, eggtray_repo = parse_eggtray_url(eggtray_url) | ||
|
|
||
| logger.info( | ||
| f"Fetching issue #{issue_number} from {eggtray_owner}/{eggtray_repo}" | ||
| ) | ||
| try: | ||
| issue = github_client.rest.issues.get( | ||
| owner=eggtray_owner, | ||
| repo=eggtray_repo, | ||
| issue_number=issue_number, | ||
| ) | ||
| except RequestFailed as e: | ||
| if e.response.status_code == 404: | ||
| logger.info(f"Issue #{issue_number} not found") | ||
| return json_response( | ||
| {"error": "Issue not found"}, | ||
| status=404, | ||
| ) | ||
| raise | ||
|
|
||
| issue_data = issue.parsed_data | ||
| logger.info( | ||
| f"Getting comments of issue #{issue_number} ({issue_data.state}) to check for summary" | ||
| ) | ||
|
|
||
| comments = github_client.rest.issues.list_comments( | ||
| owner=eggtray_owner, | ||
| repo=eggtray_repo, | ||
| issue_number=issue_number, | ||
| ) | ||
|
|
||
| if comment_body := find_summary_comment(comments.parsed_data): | ||
| logger.info(f"Check #{issue_number} is complete, parsing summary data") | ||
|
|
||
| response_data = {"status": CheckStatus.COMPLETE} | ||
|
|
||
| if summary_data := parse_summary_json(comment_body): | ||
| response_data["data"] = summary_data | ||
|
|
||
| if actions_url := extract_actions_url(comment_body): | ||
| response_data["actions_url"] = actions_url | ||
|
|
||
| return json_response(response_data, status=200) | ||
| else: | ||
| logger.info( | ||
| f"Check #{issue_number} is still in progress (no summary found)" | ||
| ) | ||
| return json_response( | ||
| {"status": CheckStatus.PENDING}, | ||
| status=202, | ||
| ) | ||
|
|
||
| except Exception as e: | ||
| error_hash = generate_error_hash(e) | ||
| logger.error(f"Error getting check status [{error_hash}]: {e}") | ||
| if debug: | ||
| raise | ||
| return json_response( | ||
| {"error": "Internal server error", "debug_id": error_hash}, | ||
| status=500, | ||
| ) | ||
|
|
||
|
|
||
| web.add_routes(routes) |
There was a problem hiding this comment.
The API endpoints lack CORS (Cross-Origin Resource Sharing) configuration. According to the README, these endpoints will be called from the junior.guru static website. Without CORS headers, browsers will block these cross-origin API requests. Consider adding aiohttp-cors middleware or manually setting appropriate CORS headers (Access-Control-Allow-Origin, etc.) to enable the static website to call these endpoints.
| @routes.post("/checks/{github_username}") | ||
| async def create_check(request: Request) -> Response: | ||
| github_username = request.match_info["github_username"] | ||
| logger.info(f"Creating check for GitHub username: {github_username}") |
There was a problem hiding this comment.
The github_username parameter lacks input validation. Malicious or malformed usernames (e.g., containing special characters, being excessively long, or empty) could be passed directly to the GitHub API or used in issue titles/bodies, potentially causing issues or abuse. Consider validating that the username matches GitHub's username format (alphanumeric plus hyphens, 1-39 characters, cannot start/end with hyphen).
| def extract_actions_url(comment_body: str) -> str | None: | ||
| if actions_match := re.search( | ||
| r"https://github\.com/juniorguru/eggtray/actions/runs/(\d+)", | ||
| comment_body, | ||
| ): | ||
| return actions_match.group(0) | ||
| return None |
There was a problem hiding this comment.
The extract_actions_url function is hardcoded to only match URLs from the juniorguru/eggtray repository, but eggtray_url is configurable. If the eggtray_url is changed to a different repository, the actions URL extraction will fail silently. Consider making the regex pattern dynamic based on the configured eggtray_url, or document this limitation clearly.
| error_string = ( | ||
| f"{datetime.now(UTC).isoformat()}-{type(error).__name__}-{str(error)}" | ||
| ) |
There was a problem hiding this comment.
The generate_error_hash function includes the current timestamp in the error hash. This means the same error occurring at different times will have different hashes, making it harder to track recurring issues. Consider removing the timestamp from the hash calculation if the goal is to identify similar errors, or keep only the date portion (without time) for daily grouping.
| error_string = ( | |
| f"{datetime.now(UTC).isoformat()}-{type(error).__name__}-{str(error)}" | |
| ) | |
| error_string = f"{type(error).__name__}-{str(error)}" |
| def test_parse_summary_json_invalid_json(): | ||
| comment_body = "```json\n{invalid json}\n```" | ||
|
|
||
| with pytest.raises(Exception): | ||
| parse_summary_json(comment_body) |
There was a problem hiding this comment.
The test expects any Exception to be raised when parsing invalid JSON, but parse_summary_json will specifically raise json.JSONDecodeError (a subclass of ValueError). The test should be more specific about which exception type is expected, e.g., pytest.raises(json.JSONDecodeError), to properly document and verify the actual behavior.
|
@copilot Address comments from both my and copilot review. |
All Latest PR Feedback Addressed
API Code Improvements:
Test Improvements:
clitoapiDependencies:
Documentation (AGENTS.md):
All 62 tests passing ✓
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.