Add GitHub MCP microservice for repository queries #131
Conversation
Introduces a new GitHub MCP microservice with FastAPI for handling repository metadata queries. Adds client, server, and service modules for the MCP, updates the intent analysis prompt and toolkit to route repo_support queries to the new service, and implements the repository_query tool. Also updates Discord cog lifecycle methods and bumps aio-pika and aiohttp dependencies.
|
Caution Review failedThe pull request is closed. WalkthroughAdds a GitHub MCP stack (service, FastAPI server, aiohttp client, startup script), a new github_support tool and intent routing in the GitHub toolkit and prompts, parsing/handler module for GitHub queries, pyproject and env updates, and a Discord cog lifecycle adjustment. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Agent as DevRel Agent
participant GT as GitHubToolkit
participant Tool as github_support
participant MCP_Client as GitHubMCPClient
participant MCP_Server as FastAPI MCP Server
participant Svc as GitHubMCPService
User->>Agent: "Repo stats for owner/repo"
Agent->>GT: classify_intent(query)
GT-->>Agent: {"classification":"github_support", ...}
Agent->>Tool: handle_github_supp(query, org=normalize_org())
Tool->>MCP_Client: POST /github_support {owner, repo}
MCP_Client->>MCP_Server: proxy POST /github_support
MCP_Server->>Svc: repo_query(owner, repo) (async thread)
Svc-->>MCP_Server: repo details
MCP_Server-->>MCP_Client: {"status":"success","data":...}
MCP_Client-->>Tool: {"status":"success","data":...}
Tool-->>Agent: structured result
Agent-->>User: formatted response
sequenceDiagram
autonumber
participant Client as GitHubMCPClient
participant Server as FastAPI MCP Server
Client->>Server: GET /health
Server-->>Client: 200 {"status":"healthy"}
Client->>Server: POST /list_org_repos {org}
Server->>Server: validate & call GitHubMCPService.list_org_repos thread
Server-->>Client: {"status":"success","data":[...]} or {"status":"error","error":...}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/agents/devrel/github/github_toolkit.py (1)
124-129: Prevent runtime TypeError: result is a string for unimplemented toolsWhen classification is
issue_creationordocumentation_generation,resultis set to a string. A few lines later the code writesresult["intent_analysis"], which will raiseTypeError: 'str' object does not support item assignment. Return a dict for unimplemented tools.Apply this diff:
- elif classification == "issue_creation": - result = "Not implemented" + elif classification == "issue_creation": + result = { + "status": "not_implemented", + "message": "issue_creation is not implemented yet" + } # result = await handle_issue_creation(query) elif classification == "documentation_generation": - result = "Not implemented" + result = { + "status": "not_implemented", + "message": "documentation_generation is not implemented yet" + } # result = await handle_documentation_generation(query)Also applies to: 134-137
🧹 Nitpick comments (21)
backend/integrations/discord/cogs.py (5)
27-36: Avoid name shadowing between the loop method and the imported cleanup function.cleanup_expired_tokens is both a class task and an imported coroutine; the unqualified call inside the loop can confuse readers and invites future mistakes. Alias the import (or rename the loop) for clarity.
-from app.services.auth.verification import create_verification_session, cleanup_expired_tokens +from app.services.auth.verification import ( + create_verification_session, + cleanup_expired_tokens as perform_token_cleanup, +) @@ async def cleanup_expired_tokens(self): """Periodic cleanup of expired verification tokens""" try: - print("--> Running token cleanup task...") - await cleanup_expired_tokens() - print("--> Token cleanup task finished.") + logger.info("Running token cleanup task...") + await perform_token_cleanup() + logger.info("Token cleanup task finished.") except Exception as e: - logger.error(f"Error during token cleanup: {e}") + logger.exception("Error during token cleanup")Also applies to: 8-8
31-36: Replace prints with structured logging and include stack traces on failure.Use logger.info and logger.exception for consistency and observability.
- print("--> Running token cleanup task...") + logger.info("Running token cleanup task...") @@ - print("--> Token cleanup task finished.") + logger.info("Token cleanup task finished.") @@ - except Exception as e: - logger.error(f"Error during token cleanup: {e}") + except Exception: + logger.exception("Error during token cleanup")
20-22: Make the loop start idempotent to guard against accidental double-starts.Not strictly necessary, but cheap protection if this cog ever reloads without a clean cancel.
def cog_load(self): """Called when the cog is loaded""" - self.cleanup_expired_tokens.start() + if not self.cleanup_expired_tokens.is_running(): + self.cleanup_expired_tokens.start()Also applies to: 24-25
55-73: Consider sending /help as an ephemeral response to reduce channel noise.Helps keep public channels clean; users can resurface help anytime.
- await interaction.response.send_message(embed=embed) + await interaction.response.send_message(embed=embed, ephemeral=True)
1-4: Tighten typing in setup to reflect DiscordBot and avoid mypy attribute errors.setup must accept commands.Bot for the loader, but you can cast internally to access queue_manager safely.
import discord from discord import app_commands from discord.ext import commands, tasks import logging +from typing import cast @@ async def setup(bot: commands.Bot): """This function is called by the bot to load the cog.""" - await bot.add_cog(DevRelCommands(bot, bot.queue_manager)) + dbot = cast(DiscordBot, bot) + await dbot.add_cog(DevRelCommands(dbot, dbot.queue_manager))Also applies to: 179-181
backend/app/agents/devrel/github/services/github_mcp_service.py (2)
5-7: Avoid import-time side effects; broaden .env lookup.Calling load_dotenv at import time has side effects and complicates testing. Prefer loading in init and consider checking both backend/.env and repo-root .env for flexibility.
Proposed sketch (apply outside this hunk as needed):
- dotenv_path = os.path.join(os.path.dirname(__file__), '..', '..', '..', '..', '..', '.env') - load_dotenv(dotenv_path=dotenv_path) + _backend_env = os.path.join(os.path.dirname(__file__), '..', '..', '..', '..', '..', '.env') + _root_env = os.path.abspath(os.path.join(os.path.dirname(__file__), *(['..'] * 6), '.env')) + # Defer .env loading to __init__ to avoid import-time side effects + # and load both locations if present (backend/.env first, then root).Then in init:
def __init__(self, token: str = None): + load_dotenv(dotenv_path=_backend_env) + load_dotenv(dotenv_path=_root_env)
43-48: Clarify that open_issues_count includes PRs; consider separate fields.GitHub’s open_issues_count includes both issues and PRs. If the agent surfaces “open issues” to users, this can be misleading.
Option A (rename field to reflect semantics):
- "open_issues": data.get("open_issues_count"), + "open_issues_and_prs": data.get("open_issues_count"),Option B (expose both with an extra call or GraphQL later):
- Keep "open_issues_and_prs" and, when needed, compute true issues-only via search/issues with is:issue. Can be a follow-up.
Also applies to: 55-59
backend/app/agents/devrel/github/prompts/intent_analysis.py (1)
31-37: Keep the JSON-only contract strict in tests and runtime checks.Given the CRITICAL instruction, add/confirm unit tests that validate the classifier returns raw JSON without surrounding text/markdown.
I can draft a small test that asserts the first non-whitespace char is "{" and the last is "}" for representative queries.
backend/app/agents/devrel/github/README.md (2)
41-47: Document .env lookup location(s) precisely.Service code currently prefers backend/.env; README only mentions environment variable export. Clarify that both GITHUB_TOKEN in env and backend/.env (or repo-root .env, if you adopt the refactor) are supported, and note required token scopes.
Proposed wording:
- Set GITHUB_TOKEN in your environment or in backend/.env (KEY=VALUE format). Recommended scopes: repo:read (for private repos), public_repo (for public), and read:org if needed.
98-106: Error handling section is solid; add rate limiting note.Consider a short note that 403 with X-RateLimit-Remaining: 0 is handled with a user-friendly message and retry guidance.
I can add a brief paragraph plus an example error payload.
backend/app/agents/devrel/github/services/github_mcp_client.py (1)
38-42: Add per-request timeout for POST to /repo_info.Defensive timeouts on critical calls keep the tool responsive under server load.
Apply this diff:
- async with self.session.post( + async with self.session.post( f"{self.mcp_server_url}/repo_info", json=payload, - headers={"Content-Type": "application/json"} + headers={"Content-Type": "application/json"}, + timeout=15 ) as response:backend/app/agents/devrel/github/test_repo_query.py (1)
1-21: This is an ad-hoc demo, not an automated test; consider moving out of tests or adding assertionsAs written, this file prints results and requires a running MCP server. It won’t be executed by pytest and provides no assertions. Consider moving it under
backend/examples/or converting it into a real async test with mocks.I can help convert this into a
pytestasync test that monkeypatches the MCP client to avoid network/server dependencies. Want me to draft it?backend/start_github_mcp_server.py (1)
20-28: Improve startup ergonomics: configurable host/port and clearer ImportError handlingDifferentiate missing
uvicornfrom path/import issues, and allow overriding host/port via env vars for containers/CI.Apply this diff:
- print("Starting GitHub MCP Server...") - print("Server will be available at: http://localhost:8001") - print("Health check: http://localhost:8001/health") + host = os.getenv("GITHUB_MCP_HOST", "0.0.0.0") + port = int(os.getenv("GITHUB_MCP_PORT", "8001")) + print("Starting GitHub MCP Server...") + print(f"Server will be available at: http://{host}:{port}") + print(f"Health check: http://{host}:{port}/health") print("Press Ctrl+C to stop the server") - uvicorn.run(app, host="0.0.0.0", port=8001) + uvicorn.run(app, host=host, port=port) - except ImportError as e: - print(f"Import error: {e}") - print("Make sure you're running this from the backend directory") - sys.exit(1) + except ModuleNotFoundError as e: + if e.name == "uvicorn": + print("Missing dependency 'uvicorn'. Install via: pip install 'uvicorn[standard]'") + sys.exit(1) + print(f"Import error: {e}") + print("Make sure you're running this from the backend directory (PYTHONPATH should include 'backend').") + sys.exit(1)backend/test_github_integration.py (4)
12-13: Avoid hard-coded sys.path manipulation; resolve absolute path and guard duplicatessys.path hacking is brittle in CI and IDEs. If you must do it, use an absolute path, guard duplicates, and prefer project-root resolution.
Apply this diff:
-import sys -import os +import sys +import os +from pathlib import Path @@ -# Add the current directory to the Python path -sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) +# Add backend/ (this file's directory) to sys.path only if needed +backend_dir = str(Path(__file__).resolve().parent) +if backend_dir not in sys.path: + sys.path.insert(0, backend_dir)
26-33: Prevent KeyError and print-safe owner/repo when response shape variesAccessing result["status"] and other keys can raise KeyError if the tool changes its schema or on unexpected errors. Use defensive access and defaults.
Apply this diff:
- result = await handle_repo_query("Please fetch AOSSIE-Org/Devr.AI repository details.") + result = await handle_repo_query("Please fetch AOSSIE-Org/Devr.AI repository details.") @@ - if result["status"] == "success": - print("✓ Repository query successful!") - print(f"Repository: {result['owner']}/{result['repo']}") + status = result.get("status") if isinstance(result, dict) else None + if status == "success": + print("✓ Repository query successful!") + owner = (result.get("owner") if isinstance(result, dict) else None) or "unknown-owner" + repo = (result.get("repo") if isinstance(result, dict) else None) or "unknown-repo" + print(f"Repository: {owner}/{repo}")
47-49: Make topics printing resilient to non-list payloadsSome backends return topics as None or a string. Normalize before joining to avoid TypeError.
Apply this diff:
- # Join the list of topics into a comma-separated string for clean printing - topics = ", ".join(data.get('topics', [])) - print(f"Topics: {topics if topics else 'N/A'}") + # Normalize topics into a list of strings before joining + raw_topics = data.get('topics') or [] + if not isinstance(raw_topics, (list, tuple)): + raw_topics = [str(raw_topics)] + topics_str = ", ".join(map(str, raw_topics)) + print(f"Topics: {topics_str or 'N/A'}")
15-23: Add a preflight health check and configurable target repoAs an integration script, add a quick /health probe and allow overriding owner/repo via CLI/env so CI can skip or point to a fork.
Example snippet (outside the shown lines) to add above the repo query:
import os import aiohttp owner = os.getenv("GITHUB_OWNER", "AOSSIE-Org") repo = os.getenv("GITHUB_REPO", "Devr.AI") server_url = os.getenv("GITHUB_MCP_URL", "http://localhost:8001") async with aiohttp.ClientSession() as sess: try: async with sess.get(f"{server_url}/health", timeout=5) as resp: if resp.status != 200: print(f"✗ MCP server unhealthy: HTTP {resp.status}") return False except Exception as e: print(f"✗ MCP server not reachable at {server_url}/health: {e}") return False result = await handle_repo_query(f"Please fetch {owner}/{repo} repository details.")I can turn this into a minimal argparse-based CLI if you want.
backend/app/agents/devrel/github/services/github_mcp_server.py (4)
26-30: Tighten response typing with Optional and Dict[str, Any]Make the schema explicit and future-proof for OpenAPI. Using Optional and concrete mappings improves validation and docs.
Apply this diff:
-from pydantic import BaseModel +from pydantic import BaseModel +from typing import Any, Dict, Optional @@ class RepoInfoResponse(BaseModel): status: str - data: dict - error: str = None + data: Optional[Dict[str, Any]] = None + error: Optional[str] = None
79-81: Chain exception when re-raising to satisfy Ruff B904 and preserve contextRaising from the caught exception clarifies the origin in logs and tools.
Apply this diff:
- logger.error(f"Error getting repo info: {e}") - raise HTTPException(status_code=500, detail=str(e)) + logger.error(f"Error getting repo info: {e}") + raise HTTPException(status_code=500, detail=str(e)) from e
72-77: Avoid blocking the event loop if repo_query is synchronousIf GitHubMCPService.repo_query performs blocking I/O, run it in a thread to keep FastAPI responsive under load.
Example change (requires
import asyncioat top):# Replace: result = github_service.repo_query(request.owner, request.repo) # With: import asyncio # at top result = await asyncio.to_thread(github_service.repo_query, request.owner, request.repo)I can propose a small adapter to make the service async-first if you prefer.
84-85: Bind to localhost by default or add auth for productionExposing 0.0.0.0 without any authentication invites misuse. For local dev, bind to 127.0.0.1; for shared environments, add an API key or network ACLs.
Suggested minimal tweak:
- uvicorn.run(app, host="0.0.0.0", port=8001) + uvicorn.run(app, host=os.getenv("GITHUB_MCP_HOST", "127.0.0.1"), port=int(os.getenv("GITHUB_MCP_PORT", "8001")))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
backend/app/agents/devrel/github/README.md(1 hunks)backend/app/agents/devrel/github/github_toolkit.py(3 hunks)backend/app/agents/devrel/github/prompts/intent_analysis.py(2 hunks)backend/app/agents/devrel/github/services/github_mcp_client.py(1 hunks)backend/app/agents/devrel/github/services/github_mcp_server.py(1 hunks)backend/app/agents/devrel/github/services/github_mcp_service.py(1 hunks)backend/app/agents/devrel/github/test_repo_query.py(1 hunks)backend/app/agents/devrel/github/tools/repository_query.py(1 hunks)backend/integrations/discord/cogs.py(1 hunks)backend/start_github_mcp_server.py(1 hunks)backend/test_github_integration.py(1 hunks)backend/test_mcp_server.py(1 hunks)pyproject.toml(2 hunks)
🧰 Additional context used
🪛 LanguageTool
backend/app/agents/devrel/github/README.md
[grammar] ~9-~9: There might be a mistake here.
Context: ...* approach: ### MCP Server Architecture - File: services/github_mcp_server.py ...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...### MCP Server Architecture - File: services/github_mcp_server.py - How it works: Runs as a separate FastA...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...he MCP server separately ### MCP Client - File: services/github_mcp_client.py ...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ... separately ### MCP Client - File: services/github_mcp_client.py - Class: GitHubMCPClient - **How it wo...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...ices/github_mcp_client.py- **Class**:GitHubMCPClient` - How it works: Communicates with the MC...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ...ub API service using the GitHub REST API - github_mcp_client.py: Client for both direct and MCP server ...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...both direct and MCP server communication - tools/repository_query.py: Tool that handles repository queries f...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...es repository queries from user requests - github_toolkit.py: Main entry point that routes queries t...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ...AI repository details" The system will: 1. Parse "AOSSIE-Org" as owner and "Devr.AI...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ...ion 3. Return structured data including: - Repository name and description - Sta...
(QB_NEW_EN)
[grammar] ~85-~85: There might be a mistake here.
Context: ...ng: - Repository name and description - Star count, fork count, open issues -...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ... - Star count, fork count, open issues - Repository URL ## Integration with Devr...
(QB_NEW_EN)
[grammar] ~100-~100: There might be a mistake here.
Context: ...n includes comprehensive error handling: - Invalid repository format - GitHub API e...
(QB_NEW_EN)
[grammar] ~101-~101: There might be a mistake here.
Context: ...or handling: - Invalid repository format - GitHub API errors - Missing authenticati...
(QB_NEW_EN)
[grammar] ~102-~102: There might be a mistake here.
Context: ...id repository format - GitHub API errors - Missing authentication tokens - Network ...
(QB_NEW_EN)
[grammar] ~103-~103: There might be a mistake here.
Context: ...I errors - Missing authentication tokens - Network connectivity issues All errors ...
(QB_NEW_EN)
🪛 Ruff (0.12.2)
backend/app/agents/devrel/github/services/github_mcp_client.py
68-68: Do not use bare except
(E722)
backend/app/agents/devrel/github/services/github_mcp_server.py
81-81: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (10)
backend/integrations/discord/cogs.py (1)
20-22: Good move: start the task in cog_load (not on_ready).This avoids multiple starts on reconnects, and the before_loop guard still ensures readiness.
pyproject.toml (1)
28-28: Confirm and Remove Unused fastmcp DependencySearches across the repository did not reveal any
import fastmcporfastmcp.usage in Python source files. If this PR isn’t actually leveraging fastmcp, it should be removed to avoid an unused dependency.• pyproject.toml (line 28):
- "fastmcp>=2.11.3,<3.0.0",Please verify whether fastmcp is required (e.g. as a plugin or dynamic import). If not, remove it from your dependencies.
backend/app/agents/devrel/github/services/github_mcp_service.py (1)
51-53: Topics often require the correct Accept header.With the Accept header set (see earlier diff), topics should populate reliably. Without it, topics can be empty on some org policies.
backend/app/agents/devrel/github/prompts/intent_analysis.py (2)
4-6: Prompt taxonomy updates look good and better align routing to repo_support.The distinctions between repo_support and web_search are clear and should reduce misrouting.
Also applies to: 15-16, 26-27, 29-29
10-10: general_github_help handler wired correctlyAll required wiring for the new
general_github_helpoption is present—no missing routes or fall-throughs to the default:
- Imported in
github_toolkit.py(line 14):
from .tools.general_github_help import handle_general_github_help- Included in the toolkit’s supported tools list (lines 37–40):
"general_github_help"- Used as the default/fallback classification (lines 76, 92, 100):
– Invalid classifications default to"general_github_help"
– Parsing and classification errors return this as the intent- Routed correctly when executed (line 132):
result = await handle_general_github_help(query, self.llm)- Implementation exists in
tools/general_github_help.pywithhandle_general_github_helpNo further changes needed.
backend/app/agents/devrel/github/README.md (1)
56-61: No binding mismatches foundI’ve verified that both the entrypoint script and the service module use the same host and port:
- In backend/app/agents/devrel/github/services/github_mcp_server.py (line 85):
uvicorn.run(app, host="0.0.0.0", port=8001)- In backend/start_github_mcp_server.py (line 20):
uvicorn.run(app, host="0.0.0.0", port=8001)Binding to 0.0.0.0:8001 covers requests to http://localhost:8001 as documented. No changes needed here.
backend/app/agents/devrel/github/services/github_mcp_client.py (1)
26-31: Good guardrail for context misuse.Raising when used outside the async context prevents subtle resource leaks.
backend/app/agents/devrel/github/github_toolkit.py (2)
8-8: Wire-up of repo_support to MCP handler looks goodImporting
handle_repo_queryis correct and aligns the toolkit with the new MCP flow.
121-121: Correct execution path for repo_supportDelegating repo queries to
handle_repo_queryis the right call and matches the new MCP-based architecture.backend/app/agents/devrel/github/tools/repository_query.py (1)
21-36: Consistent error schema and context are goodGood use of a context manager for the client, health-check before query, and consistent response shapes for success/error cases.
| @@ -0,0 +1,21 @@ | |||
| import asyncio | |||
| from .tools.repository_query import handle_repo_query | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Relative import will fail when executed as a script; use absolute import
Running this module with python .../test_repo_query.py will raise an ImportError due to the relative import. Use an absolute import that works with PYTHONPATH=backend or instruct running as a module.
Apply this diff:
-from .tools.repository_query import handle_repo_query
+from app.agents.devrel.github.tools.repository_query import handle_repo_query📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from .tools.repository_query import handle_repo_query | |
| from app.agents.devrel.github.tools.repository_query import handle_repo_query |
🤖 Prompt for AI Agents
In backend/app/agents/devrel/github/test_repo_query.py around line 2, the file
uses a relative import ("from .tools.repository_query import handle_repo_query")
which will raise ImportError when the file is executed as a script; change it to
an absolute import (e.g., "from
backend.app.agents.devrel.github.tools.repository_query import
handle_repo_query") so it works with PYTHONPATH=backend, or alternatively update
the comment/docstring to instruct running the file as a module (python -m
backend.app.agents.devrel.github.test_repo_query) if you prefer keeping the
relative import.
backend/test_mcp_server.py
Outdated
| def test_mcp_server_import(): | ||
| """Test that the MCP server can be imported""" | ||
| try: | ||
| print("Testing MCP server import...") | ||
|
|
||
| # Test importing the MCP server | ||
| from app.agents.devrel.github.services.github_mcp_server import app | ||
| print("✓ MCP server import successful") | ||
|
|
||
| # Test that the app has the expected endpoints | ||
| routes = [route.path for route in app.routes] | ||
| expected_routes = ["/health", "/mcp", "/repo_info"] | ||
|
|
||
| for route in expected_routes: | ||
| if route in routes: | ||
| print(f"✓ Route {route} found") | ||
| else: | ||
| print(f"✗ Route {route} not found") | ||
| return False | ||
|
|
||
| print("\nAll MCP server tests passed!") | ||
| print("You can now start the server with: python start_github_mcp_server.py") | ||
| return True | ||
|
|
||
| except ImportError as e: | ||
| print(f"✗ Import error: {e}") | ||
| return False | ||
| except Exception as e: | ||
| print(f"✗ Unexpected error: {e}") | ||
| import traceback | ||
| traceback.print_exc() | ||
| return False | ||
|
|
||
| if __name__ == "__main__": | ||
| success = test_mcp_server_import() | ||
| sys.exit(0 if success else 1) |
There was a problem hiding this comment.
Test returns booleans and prints instead of asserting; pytest will treat failures as passes
Returning False does not fail a pytest test; you need assertions or raised exceptions. Convert route checks to assertions and fail on import errors to avoid false positives.
Apply this diff:
def test_mcp_server_import():
"""Test that the MCP server can be imported"""
try:
print("Testing MCP server import...")
# Test importing the MCP server
from app.agents.devrel.github.services.github_mcp_server import app
print("✓ MCP server import successful")
# Test that the app has the expected endpoints
routes = [route.path for route in app.routes]
expected_routes = ["/health", "/mcp", "/repo_info"]
-
- for route in expected_routes:
- if route in routes:
- print(f"✓ Route {route} found")
- else:
- print(f"✗ Route {route} not found")
- return False
-
- print("\nAll MCP server tests passed!")
- print("You can now start the server with: python start_github_mcp_server.py")
- return True
+ missing = [r for r in expected_routes if r not in routes]
+ for r in expected_routes:
+ print(("✓" if r in routes else "✗"), f"Route {r}", "found" if r in routes else "not found")
+ assert not missing, f"Missing routes: {missing}. Found: {routes}"
+
+ print("\nAll MCP server import/route tests passed!")
+ print("You can now start the server with: python start_github_mcp_server.py")
except ImportError as e:
- print(f"✗ Import error: {e}")
- return False
+ print(f"✗ Import error: {e}")
+ assert False, f"Import error: {e}"
except Exception as e:
- print(f"✗ Unexpected error: {e}")
- import traceback
- traceback.print_exc()
- return False
+ print(f"✗ Unexpected error: {e}")
+ import traceback
+ traceback.print_exc()
+ assert False, f"Unexpected error: {e}"
@@
-if __name__ == "__main__":
- success = test_mcp_server_import()
- sys.exit(0 if success else 1)
+if __name__ == "__main__":
+ # Allow running the check manually outside pytest
+ ok = True
+ try:
+ test_mcp_server_import()
+ except AssertionError as ae:
+ print(ae)
+ ok = False
+ sys.exit(0 if ok else 1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_mcp_server_import(): | |
| """Test that the MCP server can be imported""" | |
| try: | |
| print("Testing MCP server import...") | |
| # Test importing the MCP server | |
| from app.agents.devrel.github.services.github_mcp_server import app | |
| print("✓ MCP server import successful") | |
| # Test that the app has the expected endpoints | |
| routes = [route.path for route in app.routes] | |
| expected_routes = ["/health", "/mcp", "/repo_info"] | |
| for route in expected_routes: | |
| if route in routes: | |
| print(f"✓ Route {route} found") | |
| else: | |
| print(f"✗ Route {route} not found") | |
| return False | |
| print("\nAll MCP server tests passed!") | |
| print("You can now start the server with: python start_github_mcp_server.py") | |
| return True | |
| except ImportError as e: | |
| print(f"✗ Import error: {e}") | |
| return False | |
| except Exception as e: | |
| print(f"✗ Unexpected error: {e}") | |
| import traceback | |
| traceback.print_exc() | |
| return False | |
| if __name__ == "__main__": | |
| success = test_mcp_server_import() | |
| sys.exit(0 if success else 1) | |
| def test_mcp_server_import(): | |
| """Test that the MCP server can be imported""" | |
| try: | |
| print("Testing MCP server import...") | |
| # Test importing the MCP server | |
| from app.agents.devrel.github.services.github_mcp_server import app | |
| print("✓ MCP server import successful") | |
| # Test that the app has the expected endpoints | |
| routes = [route.path for route in app.routes] | |
| expected_routes = ["/health", "/mcp", "/repo_info"] | |
| missing = [r for r in expected_routes if r not in routes] | |
| for r in expected_routes: | |
| print(("✓" if r in routes else "✗"), f"Route {r}", "found" if r in routes else "not found") | |
| assert not missing, f"Missing routes: {missing}. Found: {routes}" | |
| print("\nAll MCP server import/route tests passed!") | |
| print("You can now start the server with: python start_github_mcp_server.py") | |
| except ImportError as e: | |
| print(f"✗ Import error: {e}") | |
| assert False, f"Import error: {e}" | |
| except Exception as e: | |
| print(f"✗ Unexpected error: {e}") | |
| import traceback | |
| traceback.print_exc() | |
| assert False, f"Unexpected error: {e}" | |
| if __name__ == "__main__": | |
| # Allow running the check manually outside pytest | |
| ok = True | |
| try: | |
| test_mcp_server_import() | |
| except AssertionError as ae: | |
| print(ae) | |
| ok = False | |
| sys.exit(0 if ok else 1) |
🤖 Prompt for AI Agents
In backend/test_mcp_server.py around lines 12 to 47, the test currently prints
and returns booleans instead of using assertions so pytest can’t detect
failures; replace the return/print-based checks with pytest-style assertions
(e.g., assert import succeeds, assert each expected route is in routes) and let
exceptions propagate (or re-raise after logging) so import errors fail the test;
remove sys.exit usage and the boolean return, use plain assertions inside the
test function so pytest will report failures correctly.
| authors = [{ name = "Your Name", email = "[email protected]" }] | ||
| readme = "README.md" | ||
| requires-python = ">=3.9, <4.0" | ||
| requires-python = ">=3.10,<4.0" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm Python 3.10 availability across all runtimes.
Bumping requires-python to ">=3.10,<4.0" is fine, but please verify the Docker base image(s), CI runners, and deployment targets are already on 3.10+ to avoid runtime mismatches.
Run:
🏁 Script executed:
#!/bin/bash
# Check for pinned Python versions that could conflict with 3.10+
rg -n -C2 -i 'python:? ?3\.\d+|pyversion|setup-python|actions/setup-python' -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**'
# Look for Dockerfiles or runtime images
fd -H -a Dockerfile | xargs -I{} sh -c 'echo "---- {} ----"; sed -n "1,80p" "{}" | rg -n -i "from .*python|pyenv|asdf"'Length of output: 1012
Please ensure all environments are on Python 3.10+
I’ve checked the codebase and found:
- No Dockerfiles are present, so there’s nothing to update for container images.
- No CI configuration (e.g.
actions/setup-python) pins a Python version—please confirm your workflows are using Python 3.10 or newer. - The install guide in
docs/INSTALL_GUIDE.mdalready usespython3.10, but the project README still says “Python 3.9+”.
• UpdateREADME.md:404to “Python 3.10+” (or matching your newrequires-python). - Finally, please manually verify that any deployment targets (servers, PaaS, etc.) support Python 3.10 or above to avoid runtime mismatches.
🤖 Prompt for AI Agents
In pyproject.toml around line 7 (requires-python = ">=3.10,<4.0"), ensure all
project references and environments reflect Python 3.10+: update README.md at
line 404 to "Python 3.10+", scan and update any other docs that still say
"Python 3.9+" to "Python 3.10+" to match pyproject, add or update CI workflows
to pin actions/setup-python to 3.10 (or a compatible matrix that includes
3.10+), and verify/record that deployment targets (servers, PaaS) support Python
3.10 or above so runtime environments match the declared requirement.
pyproject.toml
Outdated
| "fastmcp>=2.11.3,<3.0.0", | ||
| "PyGithub>=2.5.0,<3.0.0", |
There was a problem hiding this comment.
Fix duplicate/case-variant PyGithub dependency and unify constraint.
You now declare both "pygithub" and "PyGithub". Package names are case-insensitively normalized (PEP 503), so this is a duplicate that can lead to resolver churn or confusing lockfile diffs. Also, the constraints differ (>=2.6.1 vs >=2.5.0).
Unify to a single entry (preferred canonical name: PyGithub) and a single constraint.
Apply this diff:
- "pygithub (>=2.6.1,<3.0.0)",
+ "PyGithub (>=2.6.1,<3.0.0)",
@@
- "PyGithub>=2.5.0,<3.0.0",Also applies to: 12-12
🤖 Prompt for AI Agents
In pyproject.toml around lines 28-29 (and also update the duplicate at line 12),
you currently list both "pygithub" and "PyGithub" with differing constraints;
remove the duplicate and unify to the canonical package name "PyGithub" with a
single constraint (use the stricter range) e.g. "PyGithub>=2.6.1,<3.0.0",
replacing any other pygithub/ PyGithub entries.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
pyproject.toml (1)
7-7: Confirm Python 3.10+ across CI, deploy targets, and docs.Bumping
requires-pythonto>=3.10,<4.0is good—ensure all environments and docs are aligned (CI runner versions, container images, READMEs). This was flagged previously; re-stating here to close the loop.Run:
#!/bin/bash # Find Python version pins in CI and docs rg -n -C2 -i 'python:? ?3\.\d+|setup-python|actions/setup-python|pyenv|asdf|python3\.[0-9]+' -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**'backend/app/agents/devrel/github/tools/repository_query.py (1)
4-14: Nice fix: URL-aware parsing preventsgithub.comfrom being captured as owner.The
GH_URL_RE+ stricterOWNER_REPO_REresolves the mis-parse issue for full GitHub URLs and bareowner/repo. Regexes look solid.
🧹 Nitpick comments (4)
backend/app/agents/devrel/github/tools/repository_query.py (1)
16-46: Minor: add a docstring and clarify error response contract.Documenting the function and its return shape will help downstream callers and tests stay consistent.
Apply:
async def handle_repo_query(user_query: str) -> dict: + """ + Parse an owner/repo (or GitHub URL) from user_query, call the GitHub MCP server, + and return a normalized response: + - {"status": "success", "owner": str, "repo": str, "data": dict} + - {"status": "error", "message": str, ...optionally owner/repo...} + """Optional follow-up: If you plan to support GitHub Enterprise later, consider parameterizing the host instead of hard-coding
github.cominGH_URL_RE.backend/app/agents/devrel/github/services/github_mcp_service.py (2)
5-8: Make.envloading robust with discovery rather than a fragile relative path.Hardcoding
../../../../..is brittle. Usefind_dotenv()so local runs and various working directories behave predictably.Apply:
-from dotenv import load_dotenv +from dotenv import load_dotenv, find_dotenv @@ -dotenv_path = os.path.join(os.path.dirname(__file__), '..', '..', '..', '..', '..', '.env') - -load_dotenv(dotenv_path=dotenv_path) +dotenv_path = find_dotenv(usecwd=True) +if dotenv_path: + load_dotenv(dotenv_path=dotenv_path) +else: + load_dotenv()
41-64: Verify “watchers” Semantics – Usesubscribers_countfor True WatchersGitHub’s REST API treats
watchers_count(andwatchers) as an alias for stars (stargazers_count), while the true “watching” (notification subscription) metric is exposed viasubscribers_count.• In the
/repos/{owner}/{repo}response,
–watchers,watchers_count, andstargazers_countall reflect the number of stars a repo has (docs.github.com, github.com)
–subscribers_countreflects the number of users actually subscribed to watch for notifications (docs.github.com)• To expose the intended “watchers” metric (users receiving notifications), replace the legacy field:
- "watchers": data.get("watchers_count"), + "watchers": data.get("subscribers_count"),Please update
backend/app/agents/devrel/github/services/github_mcp_service.pyaccordingly.backend/app/agents/devrel/github/services/github_mcp_server.py (1)
35-39: Type the optional error field explicitly for Pydantic v2.Declare
errorasOptional[str] = Nonefor clarity and better schema generation.Apply:
from pydantic import BaseModel @@ class RepoInfoResponse(BaseModel): status: str data: dict - error: str = None + from typing import Optional + error: Optional[str] = NoneIf you prefer not to import inside the model block, add
from typing import Optionalnear the top and use it here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
backend/app/agents/devrel/github/github_toolkit.py(3 hunks)backend/app/agents/devrel/github/services/github_mcp_client.py(1 hunks)backend/app/agents/devrel/github/services/github_mcp_server.py(1 hunks)backend/app/agents/devrel/github/services/github_mcp_service.py(1 hunks)backend/app/agents/devrel/github/tools/repository_query.py(1 hunks)pyproject.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/agents/devrel/github/services/github_mcp_client.py
- backend/app/agents/devrel/github/github_toolkit.py
🧰 Additional context used
🪛 Ruff (0.12.2)
backend/app/agents/devrel/github/services/github_mcp_server.py
92-92: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (1)
backend/app/agents/devrel/github/services/github_mcp_server.py (1)
20-30: Good: token fallback and diagnostics are in place.Using
GITHUB_TOKENwithGH_TOKENfallback and logging a warning when missing makes initialization resilient.
| @app.post("/repo_info") | ||
| async def get_repo_info(request: RepoInfoRequest): | ||
|
|
||
| try: | ||
| if not github_service: | ||
| raise HTTPException(status_code=500, detail="GitHub service not available") | ||
|
|
||
| result = github_service.repo_query(request.owner, request.repo) | ||
|
|
||
| if "error" in result: | ||
| return RepoInfoResponse(status="error", data={}, error=result["error"]) | ||
|
|
||
| return RepoInfoResponse(status="success", data=result) | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Error getting repo info: {e}") | ||
| raise HTTPException(status_code=500, detail=str(e)) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Harden error handling and avoid blocking in /repo_info.
- Offload the sync service call.
- Prefer 503 for service unavailability.
- Log full tracebacks and chain exceptions (
B904) while returning a sanitized message.
Apply:
@@
- if not github_service:
- raise HTTPException(status_code=500, detail="GitHub service not available")
+ if not github_service:
+ raise HTTPException(status_code=503, detail="GitHub service not available")
@@
- result = github_service.repo_query(request.owner, request.repo)
+ import asyncio
+ result = await asyncio.to_thread(github_service.repo_query, request.owner, request.repo)
@@
- except Exception as e:
- logger.error(f"Error getting repo info: {e}")
- raise HTTPException(status_code=500, detail=str(e))
+ except Exception as e:
+ logger.exception("Error getting repo info")
+ raise HTTPException(status_code=502, detail="Upstream GitHub service failed") from eIf you adopt the async service refactor, replace await asyncio.to_thread(...) with await github_service.repo_query(...).
I’d like to verify whether repo_query is synchronous and how github_service is defined/imported. Please run:
🏁 Script executed:
#!/bin/bash
# Locate the definition of repo_query to determine if it’s sync or async
rg -nP "def repo_query" -C3 --glob="*.py"
# Find where github_service is instantiated or imported
rg -n "github_service" -C3 --glob="*.py"Length of output: 4539
Harden /repo_info endpoint: offload blocking calls, correct status codes, and improve error logging
- Location:
backend/app/agents/devrel/github/services/github_mcp_server.py(lines 76–93) - Issue 1:
github_service.repo_queryis a synchronous method (def repo_query(...)), which will block the event loop. - Issue 2: Returning HTTP 500 for an unavailable service conflates internal errors with temporary unavailability—use HTTP 503 instead.
- Issue 3: Current error logging (
logger.error(...)) omits stack traces and doesn’t chain exceptions; responses leak raw exception messages.
Apply this refactor:
@@ async def get_repo_info(request: RepoInfoRequest):
- if not github_service:
- raise HTTPException(status_code=500, detail="GitHub service not available")
+ if not github_service:
+ # Service not ready—respond with 503 Service Unavailable
+ raise HTTPException(status_code=503, detail="GitHub service not available")
- result = github_service.repo_query(request.owner, request.repo)
+ # Offload blocking I/O to thread pool
+ import asyncio
+ result = await asyncio.to_thread(
+ github_service.repo_query,
+ request.owner,
+ request.repo,
+ )
@@
- except Exception as e:
- logger.error(f"Error getting repo info: {e}")
- raise HTTPException(status_code=500, detail=str(e))
+ except Exception as e:
+ # Log full traceback and chain exception
+ logger.exception("Error fetching GitHub repo info")
+ # Upstream failure—respond with 502 Bad Gateway
+ raise HTTPException(
+ status_code=502,
+ detail="Failed to fetch repository information from GitHub",
+ ) from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @app.post("/repo_info") | |
| async def get_repo_info(request: RepoInfoRequest): | |
| try: | |
| if not github_service: | |
| raise HTTPException(status_code=500, detail="GitHub service not available") | |
| result = github_service.repo_query(request.owner, request.repo) | |
| if "error" in result: | |
| return RepoInfoResponse(status="error", data={}, error=result["error"]) | |
| return RepoInfoResponse(status="success", data=result) | |
| except Exception as e: | |
| logger.error(f"Error getting repo info: {e}") | |
| raise HTTPException(status_code=500, detail=str(e)) | |
| @app.post("/repo_info") | |
| async def get_repo_info(request: RepoInfoRequest): | |
| try: | |
| if not github_service: | |
| # Service not ready—respond with 503 Service Unavailable | |
| raise HTTPException(status_code=503, detail="GitHub service not available") | |
| # Offload blocking I/O to thread pool | |
| import asyncio | |
| result = await asyncio.to_thread( | |
| github_service.repo_query, | |
| request.owner, | |
| request.repo, | |
| ) | |
| if "error" in result: | |
| return RepoInfoResponse(status="error", data={}, error=result["error"]) | |
| return RepoInfoResponse(status="success", data=result) | |
| except Exception as e: | |
| # Log full traceback and chain exception | |
| logger.exception("Error fetching GitHub repo info") | |
| # Upstream failure—respond with 502 Bad Gateway | |
| raise HTTPException( | |
| status_code=502, | |
| detail="Failed to fetch repository information from GitHub", | |
| ) from e |
🧰 Tools
🪛 Ruff (0.12.2)
92-92: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
There was a problem hiding this comment.
your current fix blocks the /repo_info to perform action leading to error 502
RuntimeWarning: coroutine 'GitHubMCPService.repo_query' was never awaited
will keep using the same code
There was a problem hiding this comment.
@DhruvK278, you're absolutely right to reject my suggestion. I apologize for the error - my analysis incorrectly assumed repo_query was synchronous when it's actually an async coroutine that needs to be awaited. Using asyncio.to_thread() on an already async method would indeed cause the "coroutine was never awaited" error you encountered.
Your original synchronous code is working correctly, and you should definitely keep it as is. The current implementation handles the GitHub API calls appropriately for your use case.
✏️ Learnings added
Learnt from: DhruvK278
PR: AOSSIE-Org/Devr.AI#131
File: backend/app/agents/devrel/github/services/github_mcp_server.py:76-93
Timestamp: 2025-08-26T08:41:38.464Z
Learning: In the GitHub MCP service implementation in backend/app/agents/devrel/github/services/github_mcp_service.py, the repo_query method is actually asynchronous and returns a coroutine, despite appearing to have a synchronous signature. Direct await should be used instead of asyncio.to_thread().
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| "fastmcp>=2.11.3,<3.0.0", | ||
| "discord-py (>=2.5.2,<3.0.0)", | ||
| ] |
There was a problem hiding this comment.
Critical: Add HTTP client dependency (service uses requests, but it’s not declared).
backend/app/agents/devrel/github/services/github_mcp_service.py imports requests, so runtime will fail in environments that install strictly from pyproject.toml. Add a pinned constraint.
Apply one of the following:
Option A — keep current sync implementation:
dependencies = [
@@
- "ddgs (>=9.0.2,<10.0.0)",
- "fastmcp>=2.11.3,<3.0.0",
+ "ddgs (>=9.0.2,<10.0.0)",
+ "fastmcp>=2.11.3,<3.0.0",
+ "requests (>=2.32.0,<3.0.0)",Option B — if you adopt the async refactor I suggest for the service (httpx), add httpx instead and remove requests:
dependencies = [
@@
- "ddgs (>=9.0.2,<10.0.0)",
- "fastmcp>=2.11.3,<3.0.0",
- "requests (>=2.32.0,<3.0.0)",
+ "ddgs (>=9.0.2,<10.0.0)",
+ "fastmcp>=2.11.3,<3.0.0",
+ "httpx (>=0.27.0,<0.28.0)",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "fastmcp>=2.11.3,<3.0.0", | |
| "discord-py (>=2.5.2,<3.0.0)", | |
| ] | |
| dependencies = [ | |
| "ddgs (>=9.0.2,<10.0.0)", | |
| "fastmcp>=2.11.3,<3.0.0", | |
| "requests (>=2.32.0,<3.0.0)", | |
| "discord-py (>=2.5.2,<3.0.0)", | |
| ] |
🤖 Prompt for AI Agents
In pyproject.toml around lines 28–30, the project uses requests in
backend/app/agents/devrel/github/services/github_mcp_service.py but doesn't
declare it; either add a pinned requests dependency (e.g.
"requests>=2.31.0,<3.0.0") to the dependencies array to keep the current sync
implementation, or if you adopt the async refactor using httpx, remove requests
and add a pinned httpx dependency (e.g. "httpx>=0.24.0,<1.0.0") and update
imports in the service accordingly.
|
current coderabbit suggestion were wrong and causes code to break. |
|
@smokeyScraper Please have a look with my fixed changes :) |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/agents/devrel/github/services/github_mcp_server.py (1)
98-115: /repo_content: offload sync call and improve error handling.Mirror /repo_info fixes.
- try: - if not github_service: - raise HTTPException(status_code=500, detail="GitHub service not available") - - result = github_service.get_repo_content(request.owner, request.repo) + try: + if not github_service: + raise HTTPException(status_code=503, detail="GitHub service not available") + + result = await asyncio.to_thread(github_service.get_repo_content, request.owner, request.repo) @@ - except Exception as e: - logger.error(f"Error getting repo content: {e}") - raise HTTPException(status_code=500, detail=str(e)) + except Exception as e: + logger.exception("Error getting repo content") + raise HTTPException(status_code=502, detail="Upstream GitHub service failed") from e
♻️ Duplicate comments (2)
backend/app/agents/devrel/github/services/github_mcp_server.py (2)
45-75: Avoid blocking FastAPI’s event loop when calling the sync service.repo_query is synchronous (requests). Offload to a thread; minimal change. Also import asyncio.
-from fastapi import FastAPI, HTTPException +from fastapi import FastAPI, HTTPException +import asyncio @@ - result = github_service.repo_query(owner, repo) + result = await asyncio.to_thread(github_service.repo_query, owner, repo) return {"result": result}
76-93: /repo_info: offload sync call, use 503 for service unavailable, and log tracebacks.Prevents event-loop blocking and improves status codes and logging.
- try: - if not github_service: - raise HTTPException(status_code=500, detail="GitHub service not available") - - result = github_service.repo_query(request.owner, request.repo) + try: + if not github_service: + raise HTTPException(status_code=503, detail="GitHub service not available") + + result = await asyncio.to_thread(github_service.repo_query, request.owner, request.repo) @@ - except Exception as e: - logger.error(f"Error getting repo info: {e}") - raise HTTPException(status_code=500, detail=str(e)) + except Exception as e: + logger.exception("Error getting repo info") + raise HTTPException(status_code=502, detail="Upstream GitHub service failed") from e
🧹 Nitpick comments (22)
backend/main.py (1)
65-83: Use exponential backoff with jitter and clearer attempt indexing.Fixed 5s intervals can cause thundering herd; attempt+1 arithmetic is noisy.
- max_retries = 5 - retry_delay = 5 + max_retries = 5 + base_delay = 2 @@ - for attempt in range(max_retries): + import random + for attempt in range(1, max_retries + 1): @@ - logger.warning("Weaviate client connected, but the service is not ready yet.") + logger.warning("Weaviate client connected, but the service is not ready yet.") @@ - if attempt < max_retries - 1: - logger.info(f"Retrying in {retry_delay} seconds...") - await asyncio.sleep(retry_delay) + if attempt < max_retries: + # Exponential backoff with jitter + delay = min(base_delay * (2 ** (attempt - 1)), 30) + random.uniform(0, 1.0) + logger.info("Retrying in %.1f seconds...", delay) + await asyncio.sleep(delay)backend/app/agents/devrel/github/services/github_mcp_client.py (3)
57-63: Log exceptions with traceback and avoid eager string formatting.Use logger.exception to retain stack traces; defer formatting to logging.
- except aiohttp.ClientError as e: - logger.error(f"Error communicating with MCP server: {e}") - return {"error": f"Communication error: {str(e)}"} - except Exception as e: - logger.error(f"Unexpected error: {e}") - return {"error": f"Unexpected error: {str(e)}"} + except aiohttp.ClientError: + logger.exception("Error communicating with MCP server") + return {"error": "Communication error"} + except Exception: + logger.exception("Unexpected error while calling MCP server") + return {"error": "Unexpected error"}
54-56: Capture error payload for non-200 responses.Surface server-provided details to aid debugging.
- else: - logger.error(f"MCP server error: {response.status}") - return {"error": f"MCP server error: {response.status}"} + else: + try: + detail = await response.text() + except Exception: + detail = "" + logger.error("MCP server error: %s %s", response.status, detail[:200]) + return {"error": f"MCP server error: {response.status}", "detail": detail}
89-91: Same here: preserve traceback on repo_content failures.- except aiohttp.ClientError as e: - logger.error(f"Error communicating with MCP server for repo content: {e}") - return {"error": f"Communication error: {str(e)}"} + except aiohttp.ClientError: + logger.exception("Error communicating with MCP server for repo content") + return {"error": "Communication error"}backend/app/agents/devrel/github/github_toolkit.py (2)
69-78: Don’t raise then immediately catch JSONDecodeError; simplify control flow.Return the fallback directly when no candidate parses.
- result = None - for payload in candidates: - try: - result = json.loads(payload) - break - except json.JSONDecodeError: - continue - if result is None: - raise json.JSONDecodeError("No valid JSON object found in LLM response", content, 0) + result = None + for payload in candidates: + try: + result = json.loads(payload) + break + except json.JSONDecodeError: + continue + if result is None: + logger.warning("No valid JSON object found in LLM response") + return { + "classification": "general_github_help", + "reasoning": "Failed to parse LLM response", + "confidence": "low", + "query": user_query, + }
87-89: Minor log wording.- logger.info(f"Classified intent as for query: {user_query} is: {classification}") + logger.info("Classified intent for query '%s' as: %s", user_query[:100], classification) - logger.info(f"Reasoning: {result.get('reasoning', 'No reasoning provided')}") + logger.info("Reasoning: %s", result.get("reasoning", "No reasoning provided"))backend/app/agents/devrel/github/tools/documentation_generation.py (5)
50-55: Fail fast if GITHUB_TOKEN is missing.Avoid an opaque 401 later; return a clear error early.
token = settings.github_token + if not token: + logger.error("GITHUB_TOKEN is not configured") + return {"status": "error", "message": "GITHUB_TOKEN is not configured on the server."} headers = { "Authorization": f"Bearer {token}", "Accept": "application/vnd.github+json", }
79-82: Add explicit strict=False to zip (ruff B905) and guard mismatches.- files = [ - {"name": item["name"], "content": content} - for item, content in zip(files_to_download, file_contents_list) if content - ] + files = [ + {"name": item["name"], "content": content} + for item, content in zip(files_to_download, file_contents_list, strict=False) + if content + ]
87-92: Preserve traceback in logs; narrow exceptions where possible.Use logger.exception; optionally catch ClientResponseError to surface status.
- except aiohttp.ClientError as e: - logger.error(f"GitHub API request failed: {e}") - return {"status": "error", "message": f"An error occurred while fetching repository data: {e}"} - except Exception as e: - logger.error(f"An unexpected error occurred: {e}") - return {"status": "error", "message": f"An unexpected error occurred: {e}"} + except aiohttp.ClientResponseError as e: + logger.exception("GitHub API request failed (%s)", e.status) + return {"status": "error", "message": f"GitHub API request failed ({e.status})"} + except aiohttp.ClientError: + logger.exception("GitHub API request failed") + return {"status": "error", "message": "An error occurred while fetching repository data"} + except Exception: + logger.exception("Unexpected error while fetching repository data") + return {"status": "error", "message": "An unexpected error occurred"}
32-36: Route through the MCP for content to centralize auth/rate limiting.This tool calls GitHub directly, diverging from the PR’s MCP centralization goal. Consider fetching repo contents via the MCP server (new repo_content endpoint) and keeping GitHub API interactions in one place.
101-110: Bound prompt size to avoid LLM overrun/cost spikes.Consider truncating each file to N KB and/or limiting total tokens before building file_contents_str.
- file_contents_str = "\n\n".join( - f"File: {file['name']}\n\n```\n{file['content']}\n```" - for file in files - ) + MAX_PER_FILE = 20_000 # chars ~20KB + file_contents_str = "\n\n".join( + f"File: {f['name']}\n\n```\n{f['content'][:MAX_PER_FILE]}\n```" + for f in files + )backend/app/agents/devrel/github/services/github_mcp_service.py (8)
5-8: Use dotenv discovery instead of a fragile relative path.Hardcoded "../../../../.env" paths are brittle across run contexts. Prefer find_dotenv() with fallback.
-from dotenv import load_dotenv +from dotenv import load_dotenv, find_dotenv @@ -dotenv_path = os.path.join(os.path.dirname(__file__), '..', '..', '..', '..', '..', '.env') - -load_dotenv(dotenv_path=dotenv_path) +dotenv_path = find_dotenv(usecwd=True) +if dotenv_path: + load_dotenv(dotenv_path=dotenv_path) +else: + load_dotenv()
10-18: Type Optional correctly and support GH_TOKEN fallback.
- Use PEP 604 union for optional type.
- Fall back to GH_TOKEN to match common setups.
- def __init__(self, token: str = None): + def __init__(self, token: str | None = None): @@ - self.token = token or os.getenv("GITHUB_TOKEN") + self.token = token or os.getenv("GITHUB_TOKEN") or os.getenv("GH_TOKEN") if not self.token: raise ValueError("GitHub token required; export as GITHUB_TOKEN or place in backend/.env file")
20-35: Return structured HTTP error details from repo_query.Current handler collapses HTTP errors to a generic message. Include status and API message for better diagnostics.
- try: - resp = requests.get(url, headers=headers, timeout=15) - resp.raise_for_status() - except requests.exceptions.RequestException as e: - return {"error": "Request failed", "message": str(e)} + try: + resp = requests.get(url, headers=headers, timeout=15) + resp.raise_for_status() + except requests.exceptions.HTTPError as e: + status = e.response.status_code if getattr(e, "response", None) else None + msg = None + try: + msg = e.response.json().get("message") + except Exception: + msg = str(e) + return {"error": "http_error", "status": status, "message": msg or str(e)} + except requests.exceptions.RequestException as e: + return {"error": "request_failed", "message": str(e)}
1-4: Replace prints with structured logging (and add a module logger).The service uses print() extensively; switch to logging for consistency and observability.
-import os -import requests -from dotenv import load_dotenv +import os +import logging +import requests +from dotenv import load_dotenv + +logger = logging.getLogger(__name__)
66-79: Log with logger instead of print; add missing headers for consistency.Use the module logger and align headers (User-Agent, API version) with repo_query.
- print(f"SERVICE: Getting repo content for {owner}/{repo}") + logger.info("Getting repo content for %s/%s", owner, repo) @@ - headers = { - "Authorization": f"Bearer {self.token}", - "Accept": "application/vnd.github+json", - } + headers = { + "Authorization": f"Bearer {self.token}", + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + "User-Agent": "DevrAI-GitHubMCPService/0.1", + }
79-86: Preserve HTTP status and message when content listing fails.Return a structured error object and log the exception with context.
- except requests.exceptions.RequestException as e: - print(f"SERVICE ERROR: Failed to fetch repository contents: {e}") - return {"error": "Failed to fetch repository contents", "message": str(e)} + except requests.exceptions.HTTPError as e: + status = e.response.status_code if getattr(e, "response", None) else None + api_msg = None + try: + api_msg = e.response.json().get("message") + except Exception: + api_msg = str(e) + logger.error("Failed to fetch contents for %s/%s: %s", owner, repo, api_msg) + return {"error": "http_error", "status": status, "message": api_msg} + except requests.exceptions.RequestException as e: + logger.error("Request failure fetching contents for %s/%s: %s", owner, repo, e) + return {"error": "request_failed", "message": str(e)}
96-105: Prefer logger for info/warn messages.Minor cleanup for consistency.
- print("SERVICE INFO: No key files found in root. Grabbing first 5 files.") + logger.info("No key files in root; grabbing first 5 files.") @@ - print("SERVICE ERROR: No files found in the root directory.") + logger.error("No files found in the root directory.")
106-123: Use logging and carry on per-file errors without noisy prints.Also safe-guard item['name'] access.
- files.append({"name": item["name"], "content": content}) - print(f"SERVICE INFO: Successfully downloaded {item['name']}") + fname = item.get("name", "unknown") + files.append({"name": fname, "content": content}) + logger.info("Downloaded %s", fname) @@ - except requests.exceptions.RequestException as e: - print(f"SERVICE WARNING: Skipping file {item['name']}: {e}") + except requests.exceptions.RequestException as e: + logger.warning("Skipping file %s due to error: %s", item.get("name", "unknown"), e)backend/app/agents/devrel/github/services/github_mcp_server.py (3)
21-29: Log initialization failures with traceback.Use logger.exception to capture stack traces.
-except Exception as e: - logger.error(f"Failed to initialize GitHub service: {e}") - github_service = None +except Exception: + logger.exception("Failed to initialize GitHub service") + github_service = None
35-39: Annotate optional field properly in the response model.class RepoInfoResponse(BaseModel): status: str data: dict - error: str = None + error: str | None = None
45-115: Optional: make the service fully async with httpx and await directly.If you prefer not to offload, convert GitHubMCPService to httpx.AsyncClient (then remove to_thread here and await the service). I can provide a patch if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
backend/app/agents/devrel/github/github_toolkit.py(3 hunks)backend/app/agents/devrel/github/services/github_mcp_client.py(1 hunks)backend/app/agents/devrel/github/services/github_mcp_server.py(1 hunks)backend/app/agents/devrel/github/services/github_mcp_service.py(1 hunks)backend/app/agents/devrel/github/tools/documentation_generation.py(1 hunks)backend/main.py(1 hunks)pyproject.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T08:41:38.494Z
Learnt from: DhruvK278
PR: AOSSIE-Org/Devr.AI#131
File: backend/app/agents/devrel/github/services/github_mcp_server.py:76-93
Timestamp: 2025-08-26T08:41:38.494Z
Learning: In the GitHub MCP service implementation in backend/app/agents/devrel/github/services/github_mcp_service.py, the repo_query method is actually asynchronous and returns a coroutine, despite appearing to have a synchronous signature. Direct await should be used instead of asyncio.to_thread().
Applied to files:
backend/app/agents/devrel/github/services/github_mcp_service.pybackend/app/agents/devrel/github/services/github_mcp_client.pybackend/app/agents/devrel/github/services/github_mcp_server.py
🪛 Ruff (0.12.2)
backend/main.py
77-77: Do not catch blind exception: Exception
(BLE001)
85-85: Avoid specifying long messages outside the exception class
(TRY003)
backend/app/agents/devrel/github/services/github_mcp_service.py
10-10: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
17-17: Avoid specifying long messages outside the exception class
(TRY003)
backend/app/agents/devrel/github/tools/documentation_generation.py
81-81: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
88-88: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
90-90: Do not catch blind exception: Exception
(BLE001)
91-91: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
113-113: Consider moving this statement to an else block
(TRY300)
115-115: Do not catch blind exception: Exception
(BLE001)
116-116: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
backend/app/agents/devrel/github/github_toolkit.py
77-77: Abstract raise to an inner function
(TRY301)
77-77: Avoid specifying long messages outside the exception class
(TRY003)
backend/app/agents/devrel/github/services/github_mcp_client.py
34-34: Avoid specifying long messages outside the exception class
(TRY003)
58-58: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
59-59: Use explicit conversion flag
Replace with conversion flag
(RUF010)
60-60: Do not catch blind exception: Exception
(BLE001)
61-61: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
62-62: Use explicit conversion flag
Replace with conversion flag
(RUF010)
80-80: Avoid specifying long messages outside the exception class
(TRY003)
90-90: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
91-91: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/app/agents/devrel/github/services/github_mcp_server.py
27-27: Do not catch blind exception: Exception
(BLE001)
28-28: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
81-81: Abstract raise to an inner function
(TRY301)
90-90: Do not catch blind exception: Exception
(BLE001)
91-91: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
92-92: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
96-96: Possible binding to all interfaces
(S104)
103-103: Abstract raise to an inner function
(TRY301)
110-110: Consider moving this statement to an else block
(TRY300)
112-112: Do not catch blind exception: Exception
(BLE001)
113-113: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
114-114: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (1)
backend/app/agents/devrel/github/services/github_mcp_server.py (1)
76-115: No async/sync mismatches detected
Verified thatrepo_queryandget_repo_contentare synchronous methods and that noawait github_service.repo_queryorawait github_service.get_repo_contentcalls exist elsewhere in the codebase.
| async with self.session.get(f"{self.mcp_server_url}/health", timeout=5) as response: | ||
| return response.status == 200 |
There was a problem hiding this comment.
Request timeout misuse: aiohttp expects ClientTimeout, not an int.
Passing timeout=5 raises TypeError at runtime. Use ClientTimeout or rely on the session default.
- async with self.session.get(f"{self.mcp_server_url}/health", timeout=5) as response:
+ async with self.session.get(
+ f"{self.mcp_server_url}/health",
+ timeout=aiohttp.ClientTimeout(total=5),
+ ) as response:
return response.status == 200📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async with self.session.get(f"{self.mcp_server_url}/health", timeout=5) as response: | |
| return response.status == 200 | |
| async with self.session.get( | |
| f"{self.mcp_server_url}/health", | |
| timeout=aiohttp.ClientTimeout(total=5), | |
| ) as response: | |
| return response.status == 200 |
🤖 Prompt for AI Agents
In backend/app/agents/devrel/github/services/github_mcp_client.py around lines
70 to 71, the call to self.session.get(..., timeout=5) is incorrect because
aiohttp expects a ClientTimeout object (or no timeout) not an int; update the
call to pass aiohttp.ClientTimeout(total=5) by importing ClientTimeout from
aiohttp (or remove the timeout argument to use the session default) so the
request uses a proper ClientTimeout instance and avoids the TypeError.
backend/main.py
Outdated
| for attempt in range(max_retries): | ||
| try: | ||
| async with get_weaviate_client() as client: | ||
| if await client.is_ready(): | ||
| logger.info("Successfully connected to Weaviate.") | ||
| return | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid indefinite await on is_ready(); wrap with a timeout.
If client.is_ready() hangs, startup blocks despite retries. Use asyncio.wait_for to cap per-attempt wait.
- async with get_weaviate_client() as client:
- if await client.is_ready():
+ async with get_weaviate_client() as client:
+ # Cap the readiness probe to avoid indefinite waits
+ if await asyncio.wait_for(client.is_ready(), timeout=3):
logger.info("Successfully connected to Weaviate.")
return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for attempt in range(max_retries): | |
| try: | |
| async with get_weaviate_client() as client: | |
| if await client.is_ready(): | |
| logger.info("Successfully connected to Weaviate.") | |
| return | |
| for attempt in range(max_retries): | |
| try: | |
| async with get_weaviate_client() as client: | |
| # Cap the readiness probe to avoid indefinite waits | |
| if await asyncio.wait_for(client.is_ready(), timeout=3): | |
| logger.info("Successfully connected to Weaviate.") | |
| return |
🤖 Prompt for AI Agents
In backend/main.py around lines 68 to 74, the call await client.is_ready() can
hang and block startup across retries; wrap that await in asyncio.wait_for with
a sensible per-attempt timeout (e.g., 5–15 seconds), import asyncio if missing,
catch asyncio.TimeoutError (and other exceptions) to log a timeout/failure and
continue to the next retry, and ensure you close the client context on timeout
so resources aren’t leaked.
backend/main.py
Outdated
| except Exception as e: | ||
| logger.warning(f"Attempt {attempt + 1}/{max_retries} failed to connect to Weaviate: {e}") | ||
|
|
||
| if attempt < max_retries - 1: | ||
| logger.info(f"Retrying in {retry_delay} seconds...") | ||
| await asyncio.sleep(retry_delay) | ||
| else: | ||
| logger.error("Could not connect to Weaviate after several retries. Please ensure Docker services are running and accessible.") | ||
| raise ConnectionError("Failed to connect to Weaviate after multiple attempts.") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Broadened catch and lost traceback; improve exception handling and re-raise with context.
Catching Exception hides details and drops cause. Preserve traceback and link the failure when giving up.
- except Exception as e:
- logger.warning(f"Attempt {attempt + 1}/{max_retries} failed to connect to Weaviate: {e}")
+ except asyncio.CancelledError:
+ raise
+ except Exception:
+ logger.exception("Attempt %s/%s failed to connect to Weaviate", attempt + 1, max_retries)
@@
- else:
- logger.error("Could not connect to Weaviate after several retries. Please ensure Docker services are running and accessible.")
- raise ConnectionError("Failed to connect to Weaviate after multiple attempts.")
+ else:
+ logger.error("Could not connect to Weaviate after several retries. Please ensure Docker services are running and accessible.")
+ raise ConnectionError("Failed to connect to Weaviate after multiple attempts.") from eCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.12.2)
77-77: Do not catch blind exception: Exception
(BLE001)
85-85: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In backend/main.py around lines 77 to 85, the current broad except swallows
traceback and context; change the handler to capture the original exception
(e.g., except Exception as exc:), log it with traceback (logger.exception(...)
or include exc and stack), and when giving up re-raise a new ConnectionError
that chains the original exception (raise ConnectionError("Failed to connect to
Weaviate after multiple attempts.") from exc) so the original cause and
traceback are preserved for debugging.
DhruvK278
force-pushed
the
feat/github-MCP-server
branch
from
2bf5fe2 to
ea97cbe
Compare
smokeyScraper
left a comment
smokeyScraper
left a comment
There was a problem hiding this comment.
I was a bit constrained over to the part that users do not need to specify the organization, and we'll be passing the organization name in the .env itself, as one deployed agent will work for an org only, right?
could you please align it in this way? so, yeah, it'll help simply answer the queries like,
"What all repositories does this organization have?"
The rest of everything seems fine to me. Response too is great!
Thanks a lot for your contribution @DhruvK278 !
PS: Please do once consider all the coderrabit suggestions, a few of them seem important to me.
| - web_search: Search the web for information | ||
| - contributor_recommendation: Finding the right people to review PRs, assign issues, or collaborate (supports both issue URLs and general queries) | ||
| - repo_support: Questions about codebase structure, dependencies, impact analysis, architecture | ||
| - web_search: Search the web for general information not available through GitHub API |
There was a problem hiding this comment.
No need to specify "not available through GitHub API", this might lead the agent to use GitHub API first, maybe, and might lead to hallucinations. It's best if it decides on its own.
| - repo_support: Questions about codebase structure, dependencies, impact analysis, architecture | ||
| - web_search: Search the web for general information not available through GitHub API | ||
| - contributor_recommendation: Finding the right people to review PRs, assign issues, or collaborate | ||
| - repo_support: Questions about repository information, structure, stats, issues, stars, forks, description, or any repository metadata |
There was a problem hiding this comment.
could you please rename this to somewhat like GitHub support? repo_support would be using Falkor DB and will be very specific to codebase-related queries.
Replaces the old repository_query tool with github_support for handling repository metadata queries. Updates prompt classification, toolkit logic, MCP client/server endpoints, and introduces github_support.py with improved naming and separation of concerns.
GitHub support tools and services now default to using the organization from environment variables when only a repo name is provided. Added endpoints and logic for listing organization repositories and stats, improved intent classification for repo-related queries, and updated prompts to clarify GitHub toolkit usage. Updated env.example to document new environment variables.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/app/agents/devrel/github/github_toolkit.py (2)
141-149: Bug: result is sometimes a string; later treated as dict.Branches that set result = "Not implemented" will crash when adding intent_analysis.
- elif classification == "repo_support": - result = "Not implemented" + elif classification == "repo_support": + result = {"status": "not_implemented", "message": "repo_support is not implemented yet"} @@ - elif classification == "issue_creation": - result = "Not implemented" + elif classification == "issue_creation": + result = {"status": "not_implemented", "message": "issue_creation is not implemented yet"} @@ - elif classification == "documentation_generation": - result = "Not implemented" + elif classification == "documentation_generation": + result = {"status": "not_implemented", "message": "documentation_generation is not implemented yet"}Also applies to: 155-158
47-52: Classification advertises find_good_first_issues but execute has no branch.Either add a handler or remove it from the tools list/prompt to avoid misrouting.
self.tools = [ @@ - "find_good_first_issues", + "find_good_first_issues", @@ async def execute(self, query: str) -> Dict[str, Any]: @@ - elif classification == "web_search": + elif classification == "find_good_first_issues": + result = {"status": "not_implemented", "message": "find_good_first_issues is not implemented yet"} + elif classification == "web_search": result = await handle_web_search(query)Also applies to: 123-153
backend/app/agents/devrel/github/prompts/intent_analysis.py (1)
10-11: Ensure functions advertised are actually executable.Prompt exposes find_good_first_issues; toolkit doesn’t implement it yet. Either implement or remove from AVAILABLE FUNCTIONS to prevent misclassification.
Also applies to: 41-42
♻️ Duplicate comments (3)
backend/app/agents/devrel/github/prompts/intent_analysis.py (1)
5-6: Avoid tying web_search to “cannot be found through GitHub API”.As noted earlier by @smokeyScraper, this can mislead routing. Suggest general guidance: prefer github_support for repo-specific questions; use web_search for news/blogs/vendor docs.
-- web_search: Search the web for general information + - web_search: Search the web for non-GitHub sources (news, blogs, vendor docs); prefer github_support for repo-specific questions @@ -- web_search: Only for information that cannot be found through GitHub API (like news, articles, external documentation) + - web_search: Use for external sources (news, articles, vendor docs). Prefer github_support for repository-specific information.Also applies to: 38-39
backend/app/agents/devrel/github/services/github_mcp_client.py (1)
100-105: Health-check timeout bug: aiohttp expects ClientTimeout, not int.This raises TypeError at runtime.
- async with self.session.get(f"{self.mcp_server_url}/health", timeout=5) as response: + async with self.session.get( + f"{self.mcp_server_url}/health", + timeout=aiohttp.ClientTimeout(total=5), + ) as response:backend/app/agents/devrel/github/services/github_mcp_service.py (1)
15-23: Convert blocking GitHub HTTP calls to async + add retries and paginationrepo_query (backend/app/agents/devrel/github/services/github_mcp_service.py:15), list_repo_issues (:46), list_org_repos (:77) use synchronous requests and are invoked from async endpoints in backend/app/agents/devrel/github/services/github_mcp_server.py (lines 53, 78, 128, 132). Refactor these service methods to use httpx.AsyncClient (or explicitly offload to run_in_threadpool), add retry/backoff, and implement proper pagination by following the Link header (per_page=100 alone is insufficient).
🧹 Nitpick comments (9)
backend/.env.example (1)
11-13: Ordering nit in .env.example (dotenv-linter).Place the org key before GITHUB_TOKEN to satisfy UnorderedKey warning.
-GITHUB_TOKEN= -# Default GitHub organization (used when user doesn't specify one) -GITHUB_ORG= +# Default GitHub organization (used when user doesn't specify one) +GITHUB_ORG= +GITHUB_TOKEN=backend/app/agents/devrel/github/services/github_mcp_client.py (2)
30-37: Validate required args early.Guard against empty repo to fail fast.
async def get_github_supp(self, repo: str, owner: Optional[str] = None) -> Dict[str, Any]: @@ - if not self.session: + if not self.session: raise RuntimeError("Client not initialized. Use async context manager.") + if not repo or not repo.strip(): + raise ValueError("Parameter 'repo' is required.")
30-31: Naming nit: get_github_supp → get_github_support.Improves readability/autocomplete.
backend/app/agents/devrel/prompts/react_prompt.py (1)
16-23: Tighten routing guidance without implying API knowledge.Rephrase to avoid requiring the model to know “what GitHub API can/can’t do” while still preferring GitHub tooling for repo queries.
-1. web_search - Search the web for external information (ONLY if you cannot find info in GitHub or FAQs) +1. web_search - Search the web for external information (prefer GitHub/FAQs for repo-specific questions; use web for news/blogs/external docs) @@ -4. github_toolkit - Handle ANY GitHub-related queries such as: +4. github_toolkit - Prefer for ANY GitHub-related queries such as:backend/app/agents/devrel/github/github_toolkit.py (1)
105-116: Prefer logging.exception for easier triage.Keeps stack traces without extra code.
- except json.JSONDecodeError as e: - logger.error(f"Error parsing JSON response from LLM: {str(e)}") + except json.JSONDecodeError as e: + logger.exception("Error parsing JSON response from LLM: %s", e) logger.error(f"Raw response: {response.content}") @@ - except Exception as e: - logger.error(f"Error in intent classification: {str(e)}") + except Exception as e: + logger.exception("Error in intent classification: %s", e) @@ - except Exception as e: - logger.error(f"Error in GitHub toolkit execution: {str(e)}") + except Exception as e: + logger.exception("Error in GitHub toolkit execution: %s", e)Also applies to: 160-168
backend/app/agents/devrel/github/services/github_mcp_server.py (2)
22-24: Clarify log message.Message says “defaulting to manual owner input” but the endpoint didn’t accept owner. With the contract fix, reword to reflect fallback semantics.
-if not GITHUB_ORG: - logger.warning("GITHUB_ORG not set in .env — defaulting to manual owner input") +if not GITHUB_ORG: + logger.warning("GITHUB_ORG not set; will require 'owner' in requests")
86-88: Bind to all interfaces only when intended.Defaulting to 0.0.0.0 can be risky outside containers. Gate by env.
-if __name__ == "__main__": - import uvicorn - uvicorn.run(app, host="0.0.0.0", port=8001) +if __name__ == "__main__": + import uvicorn, os + host = "0.0.0.0" if os.getenv("BIND_ALL", "false").lower() in {"1","true","yes"} else "127.0.0.1" + uvicorn.run(app, host=host, port=8001)backend/app/agents/devrel/github/services/github_mcp_service.py (2)
9-13: Tighten typing and trim exception text.Prefer explicit optional type and a concise message.
- def __init__(self, token: str = None): + def __init__(self, token: str | None = None): self.token = token or os.getenv("GITHUB_TOKEN") if not self.token: - raise ValueError("GitHub token required; export as GITHUB_TOKEN or place in backend/.env file") + raise ValueError("GITHUB_TOKEN is required")
77-85: Pagination note (optional).Org repos may exceed 100; consider following Link headers to collect all pages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/.env.example(1 hunks)backend/app/agents/devrel/github/github_toolkit.py(5 hunks)backend/app/agents/devrel/github/prompts/intent_analysis.py(2 hunks)backend/app/agents/devrel/github/services/github_mcp_client.py(1 hunks)backend/app/agents/devrel/github/services/github_mcp_server.py(1 hunks)backend/app/agents/devrel/github/services/github_mcp_service.py(1 hunks)backend/app/agents/devrel/github/tools/github_support.py(1 hunks)backend/app/agents/devrel/prompts/react_prompt.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T08:41:38.494Z
Learnt from: DhruvK278
PR: AOSSIE-Org/Devr.AI#131
File: backend/app/agents/devrel/github/services/github_mcp_server.py:76-93
Timestamp: 2025-08-26T08:41:38.494Z
Learning: In the GitHub MCP service implementation in backend/app/agents/devrel/github/services/github_mcp_service.py, the repo_query method is actually asynchronous and returns a coroutine, despite appearing to have a synchronous signature. Direct await should be used instead of asyncio.to_thread().
Applied to files:
backend/app/agents/devrel/github/services/github_mcp_client.pybackend/app/agents/devrel/github/services/github_mcp_server.pybackend/app/agents/devrel/github/services/github_mcp_service.py
🪛 dotenv-linter (3.3.0)
backend/.env.example
[warning] 13-13: [UnorderedKey] The GITHUB_DEFAULT_ORG key should go before the GITHUB_TOKEN key
(UnorderedKey)
🪛 Ruff (0.12.2)
backend/app/agents/devrel/github/services/github_mcp_client.py
36-36: Avoid specifying long messages outside the exception class
(TRY003)
59-59: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
60-60: Use explicit conversion flag
Replace with conversion flag
(RUF010)
61-61: Do not catch blind exception: Exception
(BLE001)
62-62: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
63-63: Use explicit conversion flag
Replace with conversion flag
(RUF010)
67-67: Avoid specifying long messages outside the exception class
(TRY003)
86-86: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
87-87: Use explicit conversion flag
Replace with conversion flag
(RUF010)
88-88: Do not catch blind exception: Exception
(BLE001)
89-89: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
90-90: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/app/agents/devrel/github/tools/github_support.py
27-27: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
107-111: Consider moving this statement to an else block
(TRY300)
113-113: Do not catch blind exception: Exception
(BLE001)
114-114: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
backend/app/agents/devrel/github/services/github_mcp_server.py
32-32: Do not catch blind exception: Exception
(BLE001)
33-33: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
56-56: Abstract raise to an inner function
(TRY301)
63-63: Consider moving this statement to an else block
(TRY300)
65-65: Do not catch blind exception: Exception
(BLE001)
66-66: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
67-67: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
82-82: Do not catch blind exception: Exception
(BLE001)
83-83: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
84-84: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
88-88: Possible binding to all interfaces
(S104)
backend/app/agents/devrel/github/github_toolkit.py
20-20: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
89-89: Abstract raise to an inner function
(TRY301)
89-89: Avoid specifying long messages outside the exception class
(TRY003)
backend/app/agents/devrel/github/services/github_mcp_service.py
9-9: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
12-12: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (6)
backend/app/agents/devrel/github/services/github_mcp_client.py (1)
20-29: Session lifecycle LGTM.Timeout on session is set; context-managed close implemented.
backend/app/agents/devrel/github/github_toolkit.py (1)
73-90: Robust JSON extraction LGTM.Non-greedy candidates with code-block preference addresses earlier parse issues.
backend/app/agents/devrel/github/tools/github_support.py (2)
10-25: Regexes for owner/repo and GH URLs look solid.Good coverage for https and SSH formats; owner constraints align with GitHub rules.
55-63: Incorrect — MCP service already normalizes to "stars"
github_mcp_service maps GitHub's stargazers_count to the "stars" field and get_org_repositories returns repos with "stars" (see backend/app/agents/devrel/github/services/github_mcp_service.py), so sorting by r["stars"] is valid.Likely an incorrect or invalid review comment.
backend/app/agents/devrel/github/prompts/intent_analysis.py (1)
16-26: Routing priority to github_support is clear. LGTM.The “ALWAYS use github_support” directive for repo info should reduce noisy web calls.
Also applies to: 41-41
backend/app/agents/devrel/github/services/github_mcp_server.py (1)
58-59: Make GitHub service calls non‑blocking in async endpoints — convert the service to async or offload sync calls to threads.github_mcp_service.repo_query (backend/app/agents/devrel/github/services/github_mcp_service.py:15) and list_org_repos (…:77) are synchronous (def) and are called directly from async FastAPI routes in backend/app/agents/devrel/github/services/github_mcp_server.py (list_org_repos endpoint: lines 52–59; get_github_supp: lines 76–79) — this will block the event loop.
Short-term fix: wrap calls with await asyncio.to_thread(...). Example: replace
result = github_service.list_org_repos(request.org)
with
result = await asyncio.to_thread(github_service.list_org_repos, request.org)Long-term fix: make GitHubMCPService async (use httpx.AsyncClient or aiohttp) and await calls; also update the async wrappers in backend/app/agents/devrel/github/services/github_mcp_service.py (async helpers at ~lines 111–131) that currently call sync methods.
⛔ Skipped due to learnings
Learnt from: DhruvK278 PR: AOSSIE-Org/Devr.AI#131 File: backend/app/agents/devrel/github/services/github_mcp_server.py:76-93 Timestamp: 2025-08-26T08:41:38.494Z Learning: In the GitHub MCP service implementation in backend/app/agents/devrel/github/services/github_mcp_service.py, the repo_query method is actually asynchronous and returns a coroutine, despite appearing to have a synchronous signature. Direct await should be used instead of asyncio.to_thread().
backend/.env.example
Outdated
| # Add Org Name here | ||
| GITHUB_DEFAULT_ORG= |
There was a problem hiding this comment.
Env var mismatch: code reads GITHUB_ORG but .env.example adds GITHUB_DEFAULT_ORG.
Client/tooling use GITHUB_ORG (with fallback hardcoded elsewhere). Ship one canonical key or document both; today, setting only GITHUB_DEFAULT_ORG won’t be picked up by github_mcp_client and github_toolkit.
Apply this diff to standardize on GITHUB_ORG (keep DEFAULT as optional alias in comments):
-# Add Org Name here
-GITHUB_DEFAULT_ORG=
+# Default GitHub organization (used when user doesn't specify one)
+GITHUB_ORG=
+# (Optional) Back-compat alias; prefer GITHUB_ORG if both are set
+# GITHUB_DEFAULT_ORG=📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Add Org Name here | |
| GITHUB_DEFAULT_ORG= | |
| # Default GitHub organization (used when user doesn't specify one) | |
| GITHUB_ORG= | |
| # (Optional) Back-compat alias; prefer GITHUB_ORG if both are set | |
| # GITHUB_DEFAULT_ORG= |
🧰 Tools
🪛 dotenv-linter (3.3.0)
[warning] 13-13: [UnorderedKey] The GITHUB_DEFAULT_ORG key should go before the GITHUB_TOKEN key
(UnorderedKey)
🤖 Prompt for AI Agents
In backend/.env.example around lines 12-13, the example uses GITHUB_DEFAULT_ORG
but the code expects GITHUB_ORG; update the example to use GITHUB_ORG as the
canonical key (you may add a short comment noting GITHUB_DEFAULT_ORG is an
optional legacy alias if desired), ensuring the example matches what
github_mcp_client and github_toolkit read so the env value is actually picked
up.
| DEFAULT_ORG = os.getenv("GITHUB_ORG") | ||
|
|
||
| def normalize_org(org_from_user: str = None) -> str: | ||
| """ | ||
| Always fallback to env org if user does not specify one. | ||
| """ | ||
| if org_from_user and org_from_user.strip(): | ||
| return org_from_user.strip() | ||
| return DEFAULT_ORG | ||
|
|
There was a problem hiding this comment.
DEFAULT_ORG may be None; align env keys and guarantee a string.
Make DEFAULT_ORG robust and consistent with .env.
-DEFAULT_ORG = os.getenv("GITHUB_ORG")
+DEFAULT_ORG = (
+ os.getenv("GITHUB_ORG")
+ or os.getenv("GITHUB_DEFAULT_ORG")
+ or "Aossie-org"
+)
@@
-def normalize_org(org_from_user: str = None) -> str:
+from typing import Optional
+
+def normalize_org(org_from_user: Optional[str] = None) -> str:
@@
- if org_from_user and org_from_user.strip():
- return org_from_user.strip()
- return DEFAULT_ORG
+ user_org = (org_from_user or "").strip() if org_from_user else ""
+ return user_org or DEFAULT_ORG📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DEFAULT_ORG = os.getenv("GITHUB_ORG") | |
| def normalize_org(org_from_user: str = None) -> str: | |
| """ | |
| Always fallback to env org if user does not specify one. | |
| """ | |
| if org_from_user and org_from_user.strip(): | |
| return org_from_user.strip() | |
| return DEFAULT_ORG | |
| DEFAULT_ORG = ( | |
| os.getenv("GITHUB_ORG") | |
| or os.getenv("GITHUB_DEFAULT_ORG") | |
| or "Aossie-org" | |
| ) | |
| from typing import Optional | |
| def normalize_org(org_from_user: Optional[str] = None) -> str: | |
| """ | |
| Always fallback to env org if user does not specify one. | |
| """ | |
| user_org = (org_from_user or "").strip() if org_from_user else "" | |
| return user_org or DEFAULT_ORG |
🧰 Tools
🪛 Ruff (0.12.2)
20-20: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 Prompt for AI Agents
In backend/app/agents/devrel/github/github_toolkit.py around lines 18 to 27,
DEFAULT_ORG can be None and may not match .env expectations — change its
initialization to read the exact .env key and guarantee a string (e.g.,
DEFAULT_ORG = os.getenv("GITHUB_ORG", "") then .strip()), and update
normalize_org to return DEFAULT_ORG (stripped) when the user value is empty so
the function always returns a string; ensure you trim whitespace on both sources
before returning.
| except aiohttp.ClientError as e: | ||
| logger.error(f"Error communicating with MCP server: {e}") | ||
| return {"error": f"Communication error: {str(e)}"} | ||
| except Exception as e: | ||
| logger.error(f"Unexpected error: {e}") | ||
| return {"error": f"Unexpected error: {str(e)}"} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use logging.exception and narrow excepts.
Preserve tracebacks; avoid blanket Exception unless re-raised.
- except aiohttp.ClientError as e:
- logger.error(f"Error communicating with MCP server: {e}")
+ except aiohttp.ClientError as e:
+ logger.exception("Error communicating with MCP server: %s", e)
return {"error": f"Communication error: {str(e)}"}
- except Exception as e:
- logger.error(f"Unexpected error: {e}")
+ except Exception as e:
+ logger.exception("Unexpected error: %s", e)
return {"error": f"Unexpected error: {str(e)}"}Apply the same pattern to list_org_repos.
Also applies to: 85-90
🧰 Tools
🪛 Ruff (0.12.2)
59-59: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
60-60: Use explicit conversion flag
Replace with conversion flag
(RUF010)
61-61: Do not catch blind exception: Exception
(BLE001)
62-62: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
63-63: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In backend/app/agents/devrel/github/services/github_mcp_client.py around lines
58-63 (and similarly update 85-90 and the list_org_repos function), the broad
except Exception and logger.error calls swallow tracebacks; replace these with
more specific exception handlers and use logger.exception to preserve
tracebacks. Concretely, catch aiohttp.ClientError where appropriate and call
logger.exception with a clear message before returning the error payload, and
remove or narrow the generic Exception handler (or re-raise after logging) so
unexpected errors are not silently swallowed.
backend/app/agents/devrel/github/services/github_mcp_service.py
Outdated
Show resolved
Hide resolved
| except Exception as e: | ||
| logger.error(f"GitHub support error: {e}") | ||
| return {"status": "error", "message": str(e)} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer logging.exception for full trace; avoid blanket Exception.
Improves debuggability; optionally narrow exceptions where feasible.
- except Exception as e:
- logger.error(f"GitHub support error: {e}")
+ except Exception as e:
+ logger.exception("GitHub support error: %s", e)
return {"status": "error", "message": str(e)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| logger.error(f"GitHub support error: {e}") | |
| return {"status": "error", "message": str(e)} | |
| except Exception as e: | |
| logger.exception("GitHub support error: %s", e) | |
| return {"status": "error", "message": str(e)} |
🧰 Tools
🪛 Ruff (0.12.2)
113-113: Do not catch blind exception: Exception
(BLE001)
114-114: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In backend/app/agents/devrel/github/tools/github_support.py around lines 113 to
115, the code currently catches a blanket Exception and logs only the message;
change the handler to log the full traceback using logger.exception(...) so the
stack is captured, and where practical replace the broad except with more
specific exception types for known failure modes (or re-raise after logging if
the caller should handle it); ensure the returned error dict still contains a
concise message (e.g., str(e)) while the full exception context is written via
logger.exception.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
backend/app/agents/devrel/github/services/github_mcp_client.py (3)
58-63: Use logger.exception without formatting the exception.Remove the redundant %s arg; the traceback is included automatically.
- except aiohttp.ClientError as e: - logger.exception("Error communicating with MCP server: %s", e) + except aiohttp.ClientError: + logger.exception("Error communicating with MCP server") return {"error": f"Communication error: {str(e)}"} - except Exception as e: - logger.exception("Unexpected error: %s", e) + except Exception: + logger.exception("Unexpected error while calling MCP server") return {"error": f"Unexpected error: {str(e)}"}
85-90: Preserve tracebacks on failures in list_org_repos.Prefer logger.exception for full stack; same message applies.
- except aiohttp.ClientError as e: - logger.error(f"Error communicating with MCP server: {e}") + except aiohttp.ClientError: + logger.exception("Error communicating with MCP server") return {"error": f"Communication error: {str(e)}"} - except Exception as e: - logger.error(f"Unexpected error: {e}") + except Exception: + logger.exception("Unexpected error while listing org repos") return {"error": f"Unexpected error: {str(e)}"}
100-105: Fix aiohttp timeout misuse (int is invalid).Passing timeout=5 raises at runtime; use ClientTimeout or rely on the session default.
- async with self.session.get(f"{self.mcp_server_url}/health", timeout=5) as response: + async with self.session.get( + f"{self.mcp_server_url}/health", + timeout=aiohttp.ClientTimeout(total=5), + ) as response:
🧹 Nitpick comments (9)
backend/.env.example (1)
12-13: Standardize ordering; fix dotenv-linter warning.Move GITHUB_ORG before GITHUB_TOKEN and clarify the comment.
-GITHUB_TOKEN= -# Add Org Name here -GITHUB_ORG= +# Default GitHub organization (used when an owner isn’t provided) +GITHUB_ORG= +GITHUB_TOKEN=backend/app/agents/devrel/github/tools/github_support.py (2)
80-86: Handle errors from get_org_stats.Return a standardized error when stats fetch fails.
- if any(w in q for w in ["stat", "stats"]): - stats = await github_mcp_service.get_org_stats(org) - return { - "status": "success", - "message": f"Stats for {org}", - "stats": stats, - } + if any(w in q for w in ["stat", "stats"]): + stats = await github_mcp_service.get_org_stats(org) + if isinstance(stats, dict) and "error" in stats: + return {"status": "error", "message": f"Failed to fetch stats for {org}", "details": stats} + return {"status": "success", "message": f"Stats for {org}", "stats": stats}
108-110: Drop redundant exception arg in logger.exception.Slight cleanup; keeps full traceback.
- except Exception as e: - logger.exception("GitHub support error: %s", e) + except Exception: + logger.exception("GitHub support error") return {"status": "error", "message": str(e)}backend/app/agents/devrel/github/services/github_mcp_service.py (3)
14-18: Type annotation: make token explicitly optional.Avoid implicit Optional per PEP 484/Ruff.
-class GitHubMCPService: - def __init__(self, token: str = None): +class GitHubMCPService: + def __init__(self, token: Optional[str] = None):
116-118: Honor GH_TOKEN as a fallback in helpers.Aligns with server initialization.
-def _get_service(token: Optional[str] = None) -> GitHubMCPService: - return GitHubMCPService(token=token or os.getenv("GITHUB_TOKEN")) +def _get_service(token: Optional[str] = None) -> GitHubMCPService: + return GitHubMCPService(token=token or os.getenv("GITHUB_TOKEN") or os.getenv("GH_TOKEN"))
119-125: Narrow broad exception handling in async helpers.Catch ValueError from missing token separately; keep unexpected errors visible.
-async def get_org_repositories(org: str): - try: - svc = _get_service() - return await asyncio.to_thread(svc.list_org_repos, org) - except Exception as e: - return {"error": "config_or_request_failed", "message": str(e)} +async def get_org_repositories(org: str): + try: + svc = _get_service() + return await asyncio.to_thread(svc.list_org_repos, org) + except ValueError as e: + return {"error": "config_error", "message": str(e)}Apply the same pattern to get_org_stats, get_repo_details, and get_repo_issues.
Also applies to: 137-139, 140-146, 147-152
backend/app/agents/devrel/github/services/github_mcp_server.py (3)
33-35: Remove unused exception variable.Tiny lint fix.
-except Exception as e: - logger.exception("Failed to initialize GitHub service") +except Exception: + logger.exception("Failed to initialize GitHub service") github_service = None
67-69: Chain exceptions for clearer causality.Use “raise … from e” and avoid leaking raw traces to clients.
- except Exception as e: - logger.exception("Error listing org repos") - raise HTTPException(status_code=500, detail=str(e)) + except Exception as e: + logger.exception("Error listing org repos") + raise HTTPException(status_code=502, detail="Upstream GitHub call failed") from e
84-87: Normalize error response and chain exception.Consistent 502 for upstream errors; keep traceback in logs.
- except Exception as e: - logger.exception("Error getting repo info") - raise HTTPException(status_code=500, detail=str(e)) + except Exception as e: + logger.exception("Error getting repo info") + raise HTTPException(status_code=502, detail="Upstream GitHub call failed") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/.env.example(1 hunks)backend/app/agents/devrel/github/services/github_mcp_client.py(1 hunks)backend/app/agents/devrel/github/services/github_mcp_server.py(1 hunks)backend/app/agents/devrel/github/services/github_mcp_service.py(1 hunks)backend/app/agents/devrel/github/tools/github_support.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T08:41:38.494Z
Learnt from: DhruvK278
PR: AOSSIE-Org/Devr.AI#131
File: backend/app/agents/devrel/github/services/github_mcp_server.py:76-93
Timestamp: 2025-08-26T08:41:38.494Z
Learning: In the GitHub MCP service implementation in backend/app/agents/devrel/github/services/github_mcp_service.py, the repo_query method is actually asynchronous and returns a coroutine, despite appearing to have a synchronous signature. Direct await should be used instead of asyncio.to_thread().
Applied to files:
backend/app/agents/devrel/github/services/github_mcp_server.pybackend/app/agents/devrel/github/services/github_mcp_service.pybackend/app/agents/devrel/github/services/github_mcp_client.py
🪛 Ruff (0.12.2)
backend/app/agents/devrel/github/services/github_mcp_server.py
33-33: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
58-58: Abstract raise to an inner function
(TRY301)
65-65: Consider moving this statement to an else block
(TRY300)
69-69: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
87-87: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
91-91: Possible binding to all interfaces
(S104)
backend/app/agents/devrel/github/services/github_mcp_service.py
14-14: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
17-17: Avoid specifying long messages outside the exception class
(TRY003)
123-123: Do not catch blind exception: Exception
(BLE001)
137-137: Do not catch blind exception: Exception
(BLE001)
144-144: Do not catch blind exception: Exception
(BLE001)
151-151: Do not catch blind exception: Exception
(BLE001)
backend/app/agents/devrel/github/services/github_mcp_client.py
36-36: Avoid specifying long messages outside the exception class
(TRY003)
59-59: Redundant exception object included in logging.exception call
(TRY401)
60-60: Use explicit conversion flag
Replace with conversion flag
(RUF010)
62-62: Redundant exception object included in logging.exception call
(TRY401)
63-63: Use explicit conversion flag
Replace with conversion flag
(RUF010)
67-67: Avoid specifying long messages outside the exception class
(TRY003)
86-86: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
87-87: Use explicit conversion flag
Replace with conversion flag
(RUF010)
88-88: Do not catch blind exception: Exception
(BLE001)
89-89: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
90-90: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/app/agents/devrel/github/tools/github_support.py
102-106: Consider moving this statement to an else block
(TRY300)
109-109: Redundant exception object included in logging.exception call
(TRY401)
🪛 dotenv-linter (3.3.0)
backend/.env.example
[warning] 13-13: [UnorderedKey] The GITHUB_ORG key should go before the GITHUB_TOKEN key
(UnorderedKey)
Changed HTTP status code to 503 when GitHub service is unavailable and made repo listing run in a separate thread for better performance. Enhanced error handling in github_support.py to return error details if repository fetching fails and made repo sorting more robust.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/app/agents/devrel/github/services/github_mcp_server.py (2)
67-69: Chain exceptions and use 502 for upstream failuresReturn a gateway error and preserve the original exception context. (Similar to prior feedback.)
- except Exception as e: - logger.exception("Error listing org repos") - raise HTTPException(status_code=500, detail=str(e)) + except Exception as e: + logger.exception("Error listing org repos") + raise HTTPException(status_code=502, detail="Upstream GitHub call failed") from e @@ - except Exception as e: - logger.exception("Error getting repo info") - raise HTTPException(status_code=500, detail=str(e)) + except Exception as e: + logger.exception("Error getting repo info") + raise HTTPException(status_code=502, detail="Upstream GitHub call failed") from eAlso applies to: 85-87
80-84: Bug: repo_query is async; don’t wrap it in to_thread—await directlyPer retrieved learnings for this PR, GitHubMCPService.repo_query is a coroutine. to_thread here causes “coroutine was never awaited”.
- result = await asyncio.to_thread(github_service.repo_query, owner, request.repo) + result = await github_service.repo_query(owner, request.repo)
🧹 Nitpick comments (5)
backend/app/agents/devrel/github/tools/github_support.py (2)
110-112: Use logger.exception without redundant exception argumentlogger.exception already captures the traceback; passing e is unnecessary and flagged by TRY401.
- except Exception as e: - logger.exception("GitHub support error: %s", e) - return {"status": "error", "message": str(e)} + except Exception: + logger.exception("GitHub support error") + return {"status": "error", "message": "Unhandled error in GitHub support"}
9-10: Unify DEFAULT_ORG with server config (minor)Server warns if GITHUB_ORG is unset; here a hardcoded fallback may cause divergence. Align casing with org name used elsewhere.
-DEFAULT_ORG = os.getenv("GITHUB_ORG", "Aossie-org") +DEFAULT_ORG = os.getenv("GITHUB_ORG", "AOSSIE-Org")backend/app/agents/devrel/github/services/github_mcp_server.py (3)
33-35: Remove unused exception variableF841: variable ‘e’ is unused; logger.exception doesn’t need it.
-except Exception as e: - logger.exception("Failed to initialize GitHub service") - github_service = None +except Exception: + logger.exception("Failed to initialize GitHub service") + github_service = None
41-45: Type the optional error field explicitlyPydantic accepts None, but the hint should be Optional[str] for clarity.
class RepoInfoResponse(BaseModel): status: str data: dict - error: str = None + error: Optional[str] = None
91-91: Bind host via env for safer defaultsAvoid binding to all interfaces by default; gate via env with sensible defaults.
- uvicorn.run(app, host="0.0.0.0", port=8001) + uvicorn.run( + app, + host=os.getenv("HOST", "127.0.0.1"), + port=int(os.getenv("PORT", "8001")), + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/agents/devrel/github/services/github_mcp_server.py(1 hunks)backend/app/agents/devrel/github/tools/github_support.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T08:41:38.494Z
Learnt from: DhruvK278
PR: AOSSIE-Org/Devr.AI#131
File: backend/app/agents/devrel/github/services/github_mcp_server.py:76-93
Timestamp: 2025-08-26T08:41:38.494Z
Learning: In the GitHub MCP service implementation in backend/app/agents/devrel/github/services/github_mcp_service.py, the repo_query method is actually asynchronous and returns a coroutine, despite appearing to have a synchronous signature. Direct await should be used instead of asyncio.to_thread().
Applied to files:
backend/app/agents/devrel/github/services/github_mcp_server.py
🪛 Ruff (0.12.2)
backend/app/agents/devrel/github/services/github_mcp_server.py
33-33: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
58-58: Abstract raise to an inner function
(TRY301)
65-65: Consider moving this statement to an else block
(TRY300)
69-69: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
87-87: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
91-91: Possible binding to all interfaces
(S104)
backend/app/agents/devrel/github/tools/github_support.py
104-108: Consider moving this statement to an else block
(TRY300)
111-111: Redundant exception object included in logging.exception call
(TRY401)
🔇 Additional comments (1)
backend/app/agents/devrel/github/services/github_mcp_server.py (1)
60-60: Keep asyncio.to_thread — list_org_repos is synchronous. backend/app/agents/devrel/github/services/github_mcp_service.py definesdef list_org_repos(self, org: str) -> list(line ~82); usingawait asyncio.to_thread(github_service.list_org_repos, request.org)is appropriate.
| if any(w in q for w in ["stat", "stats"]): | ||
| stats = await github_mcp_service.get_org_stats(org) | ||
| return { | ||
| "status": "success", | ||
| "message": f"Stats for {org}", | ||
| "stats": stats, | ||
| } |
There was a problem hiding this comment.
Guard get_org_stats error to keep response shape consistent
If the service returns an error dict, this path still returns status=success.
Apply:
- if any(w in q for w in ["stat", "stats"]):
- stats = await github_mcp_service.get_org_stats(org)
- return {
- "status": "success",
- "message": f"Stats for {org}",
- "stats": stats,
- }
+ if any(w in q for w in ["stat", "stats"]):
+ stats = await github_mcp_service.get_org_stats(org)
+ if isinstance(stats, dict) and "error" in stats:
+ return {"status": "error", "message": f"Failed to fetch stats for {org}", "details": stats}
+ return {
+ "status": "success",
+ "message": f"Stats for {org}",
+ "stats": stats,
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if any(w in q for w in ["stat", "stats"]): | |
| stats = await github_mcp_service.get_org_stats(org) | |
| return { | |
| "status": "success", | |
| "message": f"Stats for {org}", | |
| "stats": stats, | |
| } | |
| if any(w in q for w in ["stat", "stats"]): | |
| stats = await github_mcp_service.get_org_stats(org) | |
| if isinstance(stats, dict) and "error" in stats: | |
| return {"status": "error", "message": f"Failed to fetch stats for {org}", "details": stats} | |
| return { | |
| "status": "success", | |
| "message": f"Stats for {org}", | |
| "stats": stats, | |
| } |
🤖 Prompt for AI Agents
In backend/app/agents/devrel/github/tools/github_support.py around lines 82-88,
the branch that handles "stat"/"stats" unconditionally treats the result of
github_mcp_service.get_org_stats(org) as a success; guard against the service
returning an error dict by checking the returned value before constructing the
success response. Specifically, after awaiting get_org_stats, detect if the
result is an error (e.g., isinstance(stats, dict) and stats.get("status") !=
"success" or contains an "error" key) and if so return that error dict unchanged
(or normalize it to {"status":"error", "message":...}), otherwise proceed to
return the existing {"status":"success","message":...,"stats":stats} shape.
|
@smokeyScraper I have done all the changes please have a look :) |
smokeyScraper
left a comment
smokeyScraper
left a comment
There was a problem hiding this comment.
LGTM!
A few clarifications on the extensive use of Regex part and reviewing the coderabbit suggestion would be enough!
Thanks a lot!
| AVAILABLE ACTIONS: | ||
| 1. web_search - Search the web for external information | ||
| 1. web_search - Search the web for external information (ONLY if you cannot find info in GitHub or FAQs) |
There was a problem hiding this comment.
Please try not to make anything very specific. This might make the agent think on preferring GitHub/FAQs first.
| import re | ||
|
|
||
| content = response.content.strip() | ||
|
|
||
| candidates = [] | ||
| cb = re.search(r'```(?:json)?\s*({[\s\S]*?})\s*```', content, flags=re.IGNORECASE) | ||
| if cb: | ||
| candidates.append(cb.group(1)) | ||
| candidates.extend(m.group(0) for m in re.finditer(r'\{[\s\S]*?\}', content)) | ||
|
|
||
| result = None | ||
| for payload in candidates: | ||
| try: | ||
| result = json.loads(payload) | ||
| break | ||
| except json.JSONDecodeError: | ||
| continue | ||
| if result is None: | ||
| raise json.JSONDecodeError("No valid JSON object found in LLM response", content, 0) |
There was a problem hiding this comment.
can you please elaborate on what this regex is for? seems to me like for extracting repo/org from the user query.
But won't it work without regex as used specifically?
A bit confused cuz github_support.py too has regex defined.
There was a problem hiding this comment.
actually this regex is different than github_support.py, github support regex extracts repo/org from user query but, this toolkit regex takes LLM response (react supervisor) and extract json payloads from it. without this the pipeline would fail if there is a slightly malformed response basically it is a safeguard for structured output parsing.
There was a problem hiding this comment.
Oh I get it. But the previous JSON logic works pretty well. Did you face any case where this didn't work? Initially, I faced cuz the model used to output ```json{} somewhat like a structure, but later it was fixed after changing the prompt. So, I guess no need for this. Can you please revert this change @DhruvK278 ?
minimized regex use in github_toolkit.py and some changes in react_prompt
smokeyScraper
left a comment
smokeyScraper
left a comment
There was a problem hiding this comment.
Thanks a lot @DhruvK278!!
This took a long way... Thanks for staying in..
|
Merged!!! Cheers @DhruvK278! |
2 participants
Introduces a new GitHub MCP microservice with FastAPI for handling repository metadata queries. Adds client, server, and service modules for the MCP, updates the intent analysis prompt and toolkit to route repo_support queries and to the new service, and implements the repository_query tool. Also updates Discord cog lifecycle methods and bumps aio-pika and aiohttp dependencies.
feat: Add GitHub MCP service
Closes #117
📝 Description
This PR adds a new GitHub MCP (Microservice Control Plane) service that allows the agent to query GitHub repository metadata (stars, forks, open issues, etc.) in a structured way.
The service is implemented using FastAPI and is integrated with the MCP server-client pattern. It securely fetches data using a GitHub Personal Access Token (PAT) stored in environment variables.
🔧 Changes Made
This PR introduces a new GitHub MCP microservice with FastAPI for handling repository metadata queries.
The following changes were made:
github_mcp_service.py→ service layer with GitHub API calls.github_mcp_server.py→ FastAPI MCP server exposing endpoints.repo_supportqueries.repository_querytool for agent use.aio-pikaandaiohttpdependencies for improved stability.📷 Screenshots or Visual Changes (if applicable)
Repo_query
🤝 Collaboration
Guidance under @smokeyScraper
✅ Checklist
Summary by CodeRabbit
New Features
Refactor
Chores