fix: proper 401 handling with session reset and redirect#286
Open
mahek2016 wants to merge 10 commits intoAOSSIE-Org:mainfrom
Open
fix: proper 401 handling with session reset and redirect#286mahek2016 wants to merge 10 commits intoAOSSIE-Org:mainfrom
mahek2016 wants to merge 10 commits intoAOSSIE-Org:mainfrom
Conversation
…itHub API integration
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request refactors the backend architecture from a queue-based orchestration model with LLM-driven intent classification to a simplified rule-based approach. A new GitHub API endpoint for beginner-friendly issues is introduced, complex agent orchestration components are removed, and frontend authentication handling for 401 responses is enhanced. Changes
Sequence DiagramsequenceDiagram
participant User
participant Discord as Discord Bot
participant Toolkit as GitHubToolkit
participant Classifier as Rule-based<br/>Classifier
participant Handler as Issue Handler
participant Service as IssueSuggestionService
participant GitHub as GitHub API
User->>Discord: send message
Discord->>Toolkit: execute(query)
Toolkit->>Classifier: classify_intent(query)
Classifier->>Classifier: check keywords
Classifier-->>Toolkit: {intent: 'find_good_first_issues'}
Toolkit->>Handler: find_good_first_issues path
Handler->>Service: fetch_beginner_issues(language, limit)
Service->>GitHub: GET /search/issues?q=language:...
GitHub-->>Service: {items: [...]}
Service-->>Handler: [{number, title, url, repo}, ...]
Handler->>Handler: format results
Handler-->>Toolkit: {status: 'success', issues: [...]}
Toolkit-->>Discord: structured payload
Discord->>User: stream response in chunks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 4❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (10)
backend/app/classification/classification_router.py (3)
22-25: Missing return type annotation on_simple_pattern_matchThe docstring documents the return type (
dictorNone), but there's no formal annotation. Adding-> Optional[Dict[str, Any]]makes the contract explicit and consistent with the rest of the module.♻️ Proposed annotation
- def _simple_pattern_match(self, message: str): + def _simple_pattern_match(self, message: str) -> Optional[Dict[str, Any]]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/classification/classification_router.py` around lines 22 - 25, Update the _simple_pattern_match signature to explicitly annotate its return type as Optional[Dict[str, Any]] (i.e., def _simple_pattern_match(self, message: str) -> Optional[Dict[str, Any]]:), and add the necessary typing imports (Optional, Dict, Any) at the top of the module if they are not already present so the type contract matches the docstring and other functions in the module.
30-35: Greeting detection uses exact-match while other branches use substring-match
msg in greetingschecks whether the entire message is exactly"hi","hello", or"hey". Inputs like"hi there"or"hello everyone"silently fall through to the LLM, inconsistent with the substring-styleany(k in msg for k in ...)used for onboarding/issue/thanks branches.♻️ Proposed fix for consistency
- if msg in greetings: + if any(g in msg for g in greetings):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/classification/classification_router.py` around lines 30 - 35, The greeting check uses exact-match (if msg in greetings) so messages like "hi there" won't be detected; change the condition to a substring-style check consistent with the other branches (use any(k in msg for k in greetings)) and ensure msg is normalized (lowercased/trimmed) the same way as the onboarding/issue/thanks checks; update the greeting variable reference (greetings) and the condition where msg is evaluated so it matches the pattern used for onboarding_keywords and issue_keywords.
104-105: Moveimport jsonto module levelPlacing a stdlib import inside a function body is non-idiomatic; it re-runs the import machinery on every call (even though Python caches modules). Move it to the top of the file with the other imports.
♻️ Proposed refactor
+import json import logging from typing import Dict, Any from langchain_google_genai import ChatGoogleGenerativeAI- import json result = json.loads(json_str)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/classification/classification_router.py` around lines 104 - 105, Move the local "import json" out of the function and add it with the other module imports at the top of classification_router.py; then remove the in-function "import json" so the call result = json.loads(json_str) uses the top-level import (locate the place where json_str is parsed in the function that contains result = json.loads(json_str) and delete the in-function import).backend/integrations/discord/bot.py (1)
58-60:GitHubToolkitis instantiated on every message — move to__init__.Creating a new
GitHubToolkit()per message is wasteful. Since it holds no per-request state, initialize it once in__init__and reuse it.Proposed fix
def __init__(self, **kwargs): # ... self.active_threads: Dict[str, str] = {} + self.toolkit = GitHubToolkit() async def on_message(self, message): # ... - toolkit = GitHubToolkit() - result = await toolkit.execute(message.content) + result = await self.toolkit.execute(message.content)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/integrations/discord/bot.py` around lines 58 - 60, The code currently instantiates GitHubToolkit inside the message handler each time (toolkit = GitHubToolkit()), which is wasteful; move the creation into the bot class constructor (define self.toolkit = GitHubToolkit() in __init__) and change the handler to call await self.toolkit.execute(message.content) instead of creating a new instance; update any references to the local toolkit variable to use self.toolkit and remove the per-message instantiation to reuse the single GitHubToolkit instance.backend/app/agents/devrel/github/github_toolkit.py (2)
120-125: NewIssueSuggestionServiceinstance created on every call.A new service is instantiated per
execute()invocation. Consider initializing it once in__init__to avoid redundant object creation.Proposed fix
class GitHubToolkit: def __init__(self): self.tools = [...] + self.issue_service = IssueSuggestionService(settings.github_token) # ... in execute(): elif classification == "find_good_first_issues": - service = IssueSuggestionService(settings.github_token) - issues = await service.fetch_global_beginner_issues(query) + issues = await self.issue_service.fetch_global_beginner_issues(query)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/agents/devrel/github/github_toolkit.py` around lines 120 - 125, The code creates a new IssueSuggestionService on every execute() call (see IssueSuggestionService and fetch_global_beginner_issues usage); move initialization into the class constructor (__init__) so a single IssueSuggestionService instance is stored as an instance attribute (e.g., self.issue_service) and then reuse that attribute in execute() when classification == "find_good_first_issues" to call fetch_global_beginner_issues(query).
47-76: Rule-based classifier is fragile — keyword ordering and generic matches will cause misrouting.
"repo"(Line 57) is very generic and will match queries like "What repositories have good first issues?" before thefind_good_first_issuesbranch can be reached. Similarly,"search"(Line 63) will match broadly. The precedence order of the conditions matters significantly, and common queries may be misclassified. Consider refining the keyword rules or using more specific phrase matching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/agents/devrel/github/github_toolkit.py` around lines 47 - 76, The rule-based classifier in classify_intent is too brittle because broad substring checks like "repo" and "search" can capture queries intended for more specific intents (e.g., find_good_first_issues); update classify_intent to check more specific phrases first and tighten matches by using whole-word/phrase matching or regex boundaries (e.g., match "good first issue", "beginner", or "good first issues" before testing for "repo" or "search"), or alternatively normalize/tokenize the user_query and match against token sets to avoid partial matches; ensure the branches for find_good_first_issues, contributor_recommendation, and github_support are evaluated before generic "repo" and "search" checks and adjust the confidence/reasoning if falling back to general_github_help.backend/services/github/issue_suggestion_service.py (1)
29-29: Uselogginginstead ofprint()for production code.
print()statements bypass log-level configuration, structured logging, and log routing.Proposed fix
+import logging + +logger = logging.getLogger(__name__) + # ... - print("GitHub search failed:", response.text) + logger.error("GitHub search failed: %s", response.text)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/github/issue_suggestion_service.py` at line 29, The print("GitHub search failed:", response.text) call should be replaced with structured logging: import the logging module, create or reuse a module logger via logging.getLogger(__name__), and replace the print with an appropriate logger call (e.g., logger.error) that includes the response.text and contextual info; update the same location in the GitHub search handling code in issue_suggestion_service.py (where the GitHub search failure is handled) to use the logger instead of print.backend/app/agents/devrel/github/services/issue_suggestion_service.py (1)
28-34: Hardcoded keyword filters are brittle and non-extensible.Only "python" and "django" are recognized. Any other language or org in the user query is silently ignored. Consider accepting structured filter parameters instead of parsing free text with substring matching, or at least document this limitation clearly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/agents/devrel/github/services/issue_suggestion_service.py` around lines 28 - 34, The current hardcoded substring checks on query_lower (checking for "python" and "django" and appending to search_query) are brittle; update the IssueSuggestionService method that builds search_query to accept explicit, optional structured filter parameters (e.g., language and org) instead of relying on free-text parsing of query_lower, and use those parameters to append "language:{language}" and "org:{org}" to search_query; if you must support free-text, implement a simple parser that looks for "language:<value>" and "org:<value>" tokens in query_lower and map them to the same search_query additions (using the existing search_query variable), and remove the hardcoded "python"/"django" checks so other languages/orgs are handled.backend/main.py (2)
53-53: Prefix unusedappparameter with_to signal intentional non-use♻️ Proposed fix
-async def lifespan(app: FastAPI): +async def lifespan(_app: FastAPI):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/main.py` at line 53, The lifespan function declares an unused parameter named app; rename it to _app to indicate intentional non-use (change async def lifespan(app: FastAPI): to async def lifespan(_app: FastAPI):) and update any internal references to that parameter (or remove references if none) so the symbol match is consistent for the lifespan function.
44-45: Uselogger.exceptionto preserve the traceback on shutdown errors
logger.errordiscards the stack trace. Replacing it withlogger.exceptioncosts nothing and makes debugging Discord bot shutdown failures much easier.♻️ Proposed fix
- logger.error(f"Error closing Discord bot: {e}") + logger.exception(f"Error closing Discord bot: {e}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/main.py` around lines 44 - 45, In the except block in backend/main.py that currently catches Exception as e and calls logger.error(f"Error closing Discord bot: {e}"), replace the logger.error call with logger.exception to include the full traceback; locate the shutdown/cleanup code where the Discord bot is closed (the except Exception as e handler) and change the logging invocation from logger.error(...) to logger.exception(...) so the stack trace is preserved without altering control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/agents/devrel/github/github_toolkit.py`:
- Around line 152-153: The fallback call passes None as the llm to
handle_general_github_help which leads to AttributeError when that function (or
its helper _extract_search_query) calls llm.ainvoke; fix by passing a valid LLM
instance instead of None (e.g., use the existing llm variable in the surrounding
scope) or, if no LLM is available, construct and pass a minimal fallback object
that implements ainvoke (a DummyLLM/NoOp wrapper) so handle_general_github_help
and _extract_search_query can safely call ainvoke.
In `@backend/app/agents/devrel/github/services/issue_suggestion_service.py`:
- Around line 36-42: The code builds the GitHub search URL by interpolating
search_query into url and then calls AsyncClient.get without a timeout; update
the call in issue_suggestion_service.py to stop string-interpolating the query
and instead pass the query parts as a params dict (e.g., use the existing
search_query as the q param and per_page/limit as params) to client.get so httpx
will handle proper URL-encoding, and instantiate or call
httpx.AsyncClient/Client.get with a timeout (or pass timeout=HTTPX_TIMEOUT) to
avoid hanging requests; change the lines around url construction and the call
site where response = await client.get(url, headers=headers) to use
params=params and timeout configured on the client or request.
In `@backend/app/api/router.py`:
- Around line 9-13: There are two duplicate endpoints providing identical
behavior at /github/beginner-issues (backend/routes.py) and
/v1/github/beginner-issues (backend/app/api/v1/github.py) — remove the
duplication by choosing one canonical implementation (recommend keeping the
versioned /v1/github endpoint in backend/app/api/v1/github.py) and deleting the
other handler in backend/routes.py; update backend/app/api/router.py
(api_router.include_router) to ensure the chosen router (github_router) is the
one exposing the retained endpoint, remove any duplicate import or registration
of the removed handler, and run/update any tests or docs referencing the old
path to point to the single kept endpoint.
In `@backend/app/api/v1/github.py`:
- Around line 18-22: The endpoint currently calls a non-existent method
issue_service.fetch_beginner_issues(owner=GITHUB_ORG, repo=repo); replace this
call with the correct service method fetch_global_beginner_issues and pass the
proper parameters (language and limit) matching
IssueSuggestionService.fetch_global_beginner_issues; update the API handler (in
backend/app/api/v1/github.py) to accept a language query parameter (and a
sensible default) and forward it to
issue_service.fetch_global_beginner_issues(language=language, limit=...) so the
call signature matches the service.
- Around line 30-34: The except Exception as e handler in
backend/app/api/v1/github.py currently returns the raw str(e) in
HTTPException.detail and should instead return a generic client-safe message and
chain the original exception; replace detail=f"Failed to fetch issues: {str(e)}"
with a non-sensitive message like "Failed to fetch issues" (or include an error
code) and re-raise using raise HTTPException(status_code=500, detail="Failed to
fetch issues") from e; also log the full exception server-side (e.g.,
logger.exception or similar) before raising so internal details are preserved in
logs but not leaked to the client.
In `@backend/app/classification/classification_router.py`:
- Around line 62-69: The returned dict for the acknowledgment branch (the if
any(t in msg for t in thanks) check) incorrectly sets "needs_devrel": True while
the "reasoning" says no processing is needed; change "needs_devrel" to False in
that return value so acknowledgment messages are not forwarded to DevRel (keep
other keys like "priority", "reasoning", "original_message", and
"proactive_type": "acknowledgment" unchanged).
- Line 76: The parameter type hint for context is implicitly allowing None;
change the annotation from context: Dict[str, Any] = None to use an explicit
optional type (e.g., context: Optional[Dict[str, Any]] = None or context:
Dict[str, Any] | None = None for Python 3.10+), and add Optional to the typing
imports (ensure the module import list includes Optional). Update the signature
where the symbol context appears (in the function or method that defines
context) and run a quick lint/type-check to confirm the Ruff RUF013 warning is
resolved.
- Around line 83-85: The call to the pattern matcher is dead-coded by the
literal `if False` so _simple_pattern_match and its ~50 lines are never used;
replace the hardcoded guard with a real feature flag (e.g. check a boolean like
ENABLE_PATTERN_MATCHING from your config or settings) so the branch becomes `if
ENABLE_PATTERN_MATCHING and pattern_result:` (or equivalent), ensure
ENABLE_PATTERN_MATCHING defaults to False in config and is documented, and keep
the existing logger.info and return pattern_result behavior when the flag is
enabled; alternatively, if the matcher isn’t ready, remove the call to
_simple_pattern_match and delete the unused method to avoid unreachable code.
In `@backend/integrations/discord/bot.py`:
- Around line 58-69: The try/except around GitHubToolkit execution currently
only logs exceptions (logger.error) so users who saw "Processing your request…"
never receive a follow-up; update the except block handling exceptions from
GitHubToolkit and toolkit.execute to also send an error message to the Discord
thread (use thread.send or channel.send depending on context) informing the user
that processing failed and optionally include a short error summary, while still
logging the full exception via logger.error; target the code paths around
GitHubToolkit, toolkit.execute, thread.send, and logger.error to implement this
change.
In `@backend/main.py`:
- Around line 33-35: The created background task from
asyncio.create_task(self.discord_bot.start(settings.discord_bot_token)) is
discarded; store the returned Task on self (e.g., self.discord_bot_task) so it
isn't garbage-collected and can be managed; modify the startup site where
asyncio.create_task is called to assign the Task to self.discord_bot_task and
update stop_background_tasks to properly close the bot (use
self.discord_bot.close()/await as appropriate) and cancel/await
self.discord_bot_task if it exists and is not done, handling exceptions and
cleanup.
- Around line 63-64: The CORS setup currently uses allow_origins=["*"] together
with allow_credentials=True which is insecure and incompatible; update the
CORSMiddleware configuration so allow_origins is a specific list (or loaded from
an env var like ALLOWED_ORIGINS) containing only trusted origins (e.g., your
frontend dev/prod URLs) and keep allow_credentials=True only when those explicit
origins are set; locate the CORSMiddleware instantiation and replace the
wildcard allow_origins with a concrete list (or logic to parse an env var) and
ensure the code that sets allow_credentials remains unchanged but guarded by
explicit origins.
In `@backend/requirements.txt`:
- Around line 1-19: requirements.txt is missing many runtime dependencies
referenced across the codebase (e.g., imports in backend/integrations/discord/,
backend/app/database/supabase/client.py, and agent/service modules), so update
backend/requirements.txt to include the confirmed missing packages (at minimum:
aio_pika, aiohttp, ddgs, discord, falkordb, git, graphrag_sdk, langchain_core,
langchain_google_genai, langgraph, langsmith, multilspy, pygit2, redis,
requests, sentence_transformers, supabase, tavily, toml, torch, tree_sitter,
validators, weaviate and any other packages referenced by modules under
backend/integrations/discord/ and agent/service code), pin versions where
appropriate, and regenerate/lock dependencies (pip-compile or poetry lock) and
run tests/import checks to ensure no further import-time failures.
In `@backend/routes.py`:
- Around line 110-127: The route is calling a nonexistent
IssueSuggestionService.fetch_beginner_issues(owner, repo) which will raise
AttributeError; change the handler to call the correct service method
IssueSuggestionService.fetch_global_beginner_issues(language, limit) or refactor
to reuse the existing endpoint logic from backend/app/api/v1/github.py
(consolidate duplicate behavior) so you pass the expected parameters
(language/limit) to that method; also replace logging.error with
logging.exception and re-raise the HTTPException using "raise ... from e" to
preserve the original exception (refer to fetch_beginner_issues,
fetch_global_beginner_issues, IssueSuggestionService, and the exception handling
block in this file).
In `@backend/services/github/issue_suggestion_service.py`:
- Around line 25-26: The HTTP call to GitHub uses httpx.AsyncClient without any
timeout, so requests may hang; update the call in issue_suggestion_service.py to
set a reasonable timeout (either by constructing httpx.AsyncClient(timeout=...)
or by passing timeout=... to client.get) and ensure the timeout value is applied
to the request to url with headers; pick a sensible default (e.g., several
seconds) and use httpx.Timeout if you need separate connect/read/write limits.
- Around line 22-23: The constructed GitHub search URL embeds an un-encoded
query string (query and url variables) which can break requests; instead build
the search query string (e.g., label:"good first issue" language:{language}
state:open) as a plain Python string and call the HTTP client with params={'q':
query, 'per_page': limit} (use the GITHUB_API_BASE as the base URL and pass
params to httpx/requests) so the client handles URL-encoding; update the code
paths that currently form url =
f"{GITHUB_API_BASE}/search/issues?q={query}&per_page={limit}" to use a base
endpoint like f"{GITHUB_API_BASE}/search/issues" and pass params via the params
kwarg.
- Around line 1-46: The class IssueSuggestionService defines
fetch_global_beginner_issues but callers expect fetch_beginner_issues with
owner/repo parameters; add (or rename to) a method fetch_beginner_issues(self,
owner: str, repo: str, language: str = "python", limit: int = 5) that builds a
repo-scoped GitHub search query (repo:{owner}/{repo} label:"good first issue"
language:{language}) and returns the same dict shape as the existing loop,
reusing the existing headers and AsyncClient logic from
fetch_global_beginner_issues; consolidate the near-duplicate implementation in
backend/app/agents/... by merging its semantic parsing logic into this single
IssueSuggestionService and remove the duplicate file so callers
(fetch_beginner_issues) use the unified service.
In `@frontend/src/lib/api.ts`:
- Line 93: The guard using currentPath.includes('/login') can false-match other
routes; update the conditional in frontend/src/lib/api.ts (the line with if
(status === 401 && !currentPath.includes('/login'))) to use a strict or
start-of-path check instead—for example replace includes with currentPath ===
'/login' if you only want the exact login page, or with currentPath === '/login'
|| currentPath.startsWith('/login/') (or currentPath.startsWith('/login')) if
you want the login page and its legitimate subpaths; keep the rest of the 401
handling unchanged.
- Around line 89-111: Add a module-level boolean guard (e.g., isLoggingOut) and
use an atomic check/set before running the logout sequence in the interceptor so
concurrent 401 responses only trigger signOut and redirect once; call
supabase.auth.signOut() inside that guarded block, await it, then set
window.location.href = '/login' a single time. Replace localStorage.clear() with
targeted removals of known auth keys (e.g., AUTH_TOKEN_KEY, REFRESH_TOKEN_KEY,
USER_SESSION_KEY) and keep sessionStorage cleanup scoped similarly instead of
wiping everything. Ensure you reset isLoggingOut appropriately only if needed
and that the interceptor still returns Promise.reject(error) for non-handled
flows.
---
Nitpick comments:
In `@backend/app/agents/devrel/github/github_toolkit.py`:
- Around line 120-125: The code creates a new IssueSuggestionService on every
execute() call (see IssueSuggestionService and fetch_global_beginner_issues
usage); move initialization into the class constructor (__init__) so a single
IssueSuggestionService instance is stored as an instance attribute (e.g.,
self.issue_service) and then reuse that attribute in execute() when
classification == "find_good_first_issues" to call
fetch_global_beginner_issues(query).
- Around line 47-76: The rule-based classifier in classify_intent is too brittle
because broad substring checks like "repo" and "search" can capture queries
intended for more specific intents (e.g., find_good_first_issues); update
classify_intent to check more specific phrases first and tighten matches by
using whole-word/phrase matching or regex boundaries (e.g., match "good first
issue", "beginner", or "good first issues" before testing for "repo" or
"search"), or alternatively normalize/tokenize the user_query and match against
token sets to avoid partial matches; ensure the branches for
find_good_first_issues, contributor_recommendation, and github_support are
evaluated before generic "repo" and "search" checks and adjust the
confidence/reasoning if falling back to general_github_help.
In `@backend/app/agents/devrel/github/services/issue_suggestion_service.py`:
- Around line 28-34: The current hardcoded substring checks on query_lower
(checking for "python" and "django" and appending to search_query) are brittle;
update the IssueSuggestionService method that builds search_query to accept
explicit, optional structured filter parameters (e.g., language and org) instead
of relying on free-text parsing of query_lower, and use those parameters to
append "language:{language}" and "org:{org}" to search_query; if you must
support free-text, implement a simple parser that looks for "language:<value>"
and "org:<value>" tokens in query_lower and map them to the same search_query
additions (using the existing search_query variable), and remove the hardcoded
"python"/"django" checks so other languages/orgs are handled.
In `@backend/app/classification/classification_router.py`:
- Around line 22-25: Update the _simple_pattern_match signature to explicitly
annotate its return type as Optional[Dict[str, Any]] (i.e., def
_simple_pattern_match(self, message: str) -> Optional[Dict[str, Any]]:), and add
the necessary typing imports (Optional, Dict, Any) at the top of the module if
they are not already present so the type contract matches the docstring and
other functions in the module.
- Around line 30-35: The greeting check uses exact-match (if msg in greetings)
so messages like "hi there" won't be detected; change the condition to a
substring-style check consistent with the other branches (use any(k in msg for k
in greetings)) and ensure msg is normalized (lowercased/trimmed) the same way as
the onboarding/issue/thanks checks; update the greeting variable reference
(greetings) and the condition where msg is evaluated so it matches the pattern
used for onboarding_keywords and issue_keywords.
- Around line 104-105: Move the local "import json" out of the function and add
it with the other module imports at the top of classification_router.py; then
remove the in-function "import json" so the call result = json.loads(json_str)
uses the top-level import (locate the place where json_str is parsed in the
function that contains result = json.loads(json_str) and delete the in-function
import).
In `@backend/integrations/discord/bot.py`:
- Around line 58-60: The code currently instantiates GitHubToolkit inside the
message handler each time (toolkit = GitHubToolkit()), which is wasteful; move
the creation into the bot class constructor (define self.toolkit =
GitHubToolkit() in __init__) and change the handler to call await
self.toolkit.execute(message.content) instead of creating a new instance; update
any references to the local toolkit variable to use self.toolkit and remove the
per-message instantiation to reuse the single GitHubToolkit instance.
In `@backend/main.py`:
- Line 53: The lifespan function declares an unused parameter named app; rename
it to _app to indicate intentional non-use (change async def lifespan(app:
FastAPI): to async def lifespan(_app: FastAPI):) and update any internal
references to that parameter (or remove references if none) so the symbol match
is consistent for the lifespan function.
- Around line 44-45: In the except block in backend/main.py that currently
catches Exception as e and calls logger.error(f"Error closing Discord bot:
{e}"), replace the logger.error call with logger.exception to include the full
traceback; locate the shutdown/cleanup code where the Discord bot is closed (the
except Exception as e handler) and change the logging invocation from
logger.error(...) to logger.exception(...) so the stack trace is preserved
without altering control flow.
In `@backend/services/github/issue_suggestion_service.py`:
- Line 29: The print("GitHub search failed:", response.text) call should be
replaced with structured logging: import the logging module, create or reuse a
module logger via logging.getLogger(__name__), and replace the print with an
appropriate logger call (e.g., logger.error) that includes the response.text and
contextual info; update the same location in the GitHub search handling code in
issue_suggestion_service.py (where the GitHub search failure is handled) to use
the logger instead of print.
backend/app/agents/devrel/github/services/issue_suggestion_service.py
Outdated
Show resolved
Hide resolved
Comment on lines
+9
to
+13
| api_router.include_router( | ||
| github_router, | ||
| prefix="/v1/github", | ||
| tags=["GitHub"] | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all routes that serve beginner-issues
echo "=== Searching for beginner-issues registrations ==="
rg -n 'beginner.issues' --type=py -B2 -A2
echo -e "\n=== Checking backend/routes.py existence ==="
find . -name "routes.py" -type f | head -20
echo -e "\n=== Checking for github router in app/api ==="
find . -path "*/app/api*" -name "*.py" -type f | head -20Repository: AOSSIE-Org/Devr.AI
Length of output: 3781
🏁 Script executed:
#!/bin/bash
echo "=== backend/routes.py: beginner-issues endpoint ==="
sed -n '102,127p' backend/routes.py
echo -e "\n=== backend/app/api/v1/github.py: beginner-issues endpoint ==="
sed -n '10,35p' backend/app/api/v1/github.pyRepository: AOSSIE-Org/Devr.AI
Length of output: 1476
Remove or consolidate the duplicate beginner-issues endpoint.
Two endpoints serve identical functionality: /github/beginner-issues (from backend/routes.py) and /v1/github/beginner-issues (from backend/app/api/v1/github.py included in backend/app/api/router.py). Both call the same service method with identical parameters and nearly identical implementations. This duplication creates maintenance burden and inconsistency risk. Consolidate to a single endpoint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/router.py` around lines 9 - 13, There are two duplicate
endpoints providing identical behavior at /github/beginner-issues
(backend/routes.py) and /v1/github/beginner-issues
(backend/app/api/v1/github.py) — remove the duplication by choosing one
canonical implementation (recommend keeping the versioned /v1/github endpoint in
backend/app/api/v1/github.py) and deleting the other handler in
backend/routes.py; update backend/app/api/router.py (api_router.include_router)
to ensure the chosen router (github_router) is the one exposing the retained
endpoint, remove any duplicate import or registration of the removed handler,
and run/update any tests or docs referencing the old path to point to the single
kept endpoint.
Comment on lines
18
to
22
| try: | ||
| issues = await issue_service.fetch_beginner_issues( | ||
| owner=GITHUB_ORG, | ||
| repo=repo | ||
| ) |
Contributor
There was a problem hiding this comment.
fetch_beginner_issues does not exist — this will raise AttributeError at runtime.
The imported IssueSuggestionService (from services.github.issue_suggestion_service) only defines fetch_global_beginner_issues(self, language, limit). There is no fetch_beginner_issues method, and the call signature (owner=..., repo=...) doesn't match any method either. This endpoint is completely broken.
Proposed fix (adjust to the actual method signature)
issues = await issue_service.fetch_beginner_issues(
- owner=GITHUB_ORG,
- repo=repo
+ issues = await issue_service.fetch_global_beginner_issues(
+ language="python", # or derive from query param
+ limit=10
)You will also need to add language as a query parameter to the endpoint if you want it to be configurable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/v1/github.py` around lines 18 - 22, The endpoint currently
calls a non-existent method
issue_service.fetch_beginner_issues(owner=GITHUB_ORG, repo=repo); replace this
call with the correct service method fetch_global_beginner_issues and pass the
proper parameters (language and limit) matching
IssueSuggestionService.fetch_global_beginner_issues; update the API handler (in
backend/app/api/v1/github.py) to accept a language query parameter (and a
sensible default) and forward it to
issue_service.fetch_global_beginner_issues(language=language, limit=...) so the
call signature matches the service.
Comment on lines
+89
to
111
| async (error) => { | ||
| const status = error.response?.status; | ||
| const currentPath = window.location.pathname; | ||
|
|
||
| if (status === 401 && !currentPath.includes('/login')) { | ||
| console.warn('🔒 Session expired. Logging out...'); | ||
|
|
||
| try { | ||
| await supabase.auth.signOut(); | ||
| } catch (signOutError) { | ||
| console.error('Error during signOut:', signOutError); | ||
| } | ||
|
|
||
| // Optional: Clear any remaining storage | ||
| localStorage.clear(); | ||
| sessionStorage.clear(); | ||
|
|
||
| // Redirect to login page | ||
| window.location.href = '/login'; | ||
| } | ||
|
|
||
| return Promise.reject(error); | ||
| } |
Contributor
There was a problem hiding this comment.
Concurrent 401 responses can trigger multiple sign-outs and redirects.
If several API calls are in-flight and all return 401, this interceptor runs for each one — calling signOut(), clearing storage, and assigning window.location.href multiple times concurrently. Introduce a module-level guard (e.g., a boolean flag) to ensure the logout-and-redirect sequence only executes once.
Also, localStorage.clear() on Line 103 is aggressive — it wipes all keys, not just auth-related ones, which can destroy unrelated persisted app state (user preferences, cached data, etc.). Consider only removing known auth keys.
Proposed fix with a redirect guard and scoped storage cleanup
+let isLoggingOut = false;
+
class ApiClient {
private client: AxiosInstance;
constructor() {
// ... (axios.create stays the same)
this.client.interceptors.response.use(
(response) => response,
async (error) => {
const status = error.response?.status;
const currentPath = window.location.pathname;
- if (status === 401 && !currentPath.includes('/login')) {
+ if (status === 401 && !currentPath.includes('/login') && !isLoggingOut) {
+ isLoggingOut = true;
console.warn('🔒 Session expired. Logging out...');
try {
await supabase.auth.signOut();
} catch (signOutError) {
console.error('Error during signOut:', signOutError);
}
- // Optional: Clear any remaining storage
- localStorage.clear();
- sessionStorage.clear();
+ // Clear only auth-related storage
+ localStorage.removeItem('supabase.auth.token');
+ sessionStorage.clear();
// Redirect to login page
window.location.href = '/login';
}
return Promise.reject(error);
}
);
}📝 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.
Suggested change
| async (error) => { | |
| const status = error.response?.status; | |
| const currentPath = window.location.pathname; | |
| if (status === 401 && !currentPath.includes('/login')) { | |
| console.warn('🔒 Session expired. Logging out...'); | |
| try { | |
| await supabase.auth.signOut(); | |
| } catch (signOutError) { | |
| console.error('Error during signOut:', signOutError); | |
| } | |
| // Optional: Clear any remaining storage | |
| localStorage.clear(); | |
| sessionStorage.clear(); | |
| // Redirect to login page | |
| window.location.href = '/login'; | |
| } | |
| return Promise.reject(error); | |
| } | |
| async (error) => { | |
| const status = error.response?.status; | |
| const currentPath = window.location.pathname; | |
| if (status === 401 && !currentPath.includes('/login') && !isLoggingOut) { | |
| isLoggingOut = true; | |
| console.warn('🔒 Session expired. Logging out...'); | |
| try { | |
| await supabase.auth.signOut(); | |
| } catch (signOutError) { | |
| console.error('Error during signOut:', signOutError); | |
| } | |
| // Clear only auth-related storage | |
| localStorage.removeItem('supabase.auth.token'); | |
| sessionStorage.clear(); | |
| // Redirect to login page | |
| window.location.href = '/login'; | |
| } | |
| return Promise.reject(error); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/lib/api.ts` around lines 89 - 111, Add a module-level boolean
guard (e.g., isLoggingOut) and use an atomic check/set before running the logout
sequence in the interceptor so concurrent 401 responses only trigger signOut and
redirect once; call supabase.auth.signOut() inside that guarded block, await it,
then set window.location.href = '/login' a single time. Replace
localStorage.clear() with targeted removals of known auth keys (e.g.,
AUTH_TOKEN_KEY, REFRESH_TOKEN_KEY, USER_SESSION_KEY) and keep sessionStorage
cleanup scoped similarly instead of wiping everything. Ensure you reset
isLoggingOut appropriately only if needed and that the interceptor still returns
Promise.reject(error) for non-handled flows.
| const status = error.response?.status; | ||
| const currentPath = window.location.pathname; | ||
|
|
||
| if (status === 401 && !currentPath.includes('/login')) { |
Contributor
There was a problem hiding this comment.
Path guard with includes('/login') can false-match.
currentPath.includes('/login') will match unrelated paths like /admin/login-settings or /blog/login-flow. Use a strict equality check or a start-of-path match.
Proposed fix
- if (status === 401 && !currentPath.includes('/login')) {
+ if (status === 401 && currentPath !== '/login') {📝 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.
Suggested change
| if (status === 401 && !currentPath.includes('/login')) { | |
| if (status === 401 && currentPath !== '/login') { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/lib/api.ts` at line 93, The guard using
currentPath.includes('/login') can false-match other routes; update the
conditional in frontend/src/lib/api.ts (the line with if (status === 401 &&
!currentPath.includes('/login'))) to use a strict or start-of-path check
instead—for example replace includes with currentPath === '/login' if you only
want the exact login page, or with currentPath === '/login' ||
currentPath.startsWith('/login/') (or currentPath.startsWith('/login')) if you
want the login page and its legitimate subpaths; keep the rest of the 401
handling unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/services/github/issue_suggestion_service.py (2)
29-37: Per-requestAsyncClientinstantiation bypasses connection pooling.Creating a new
httpx.AsyncClienton every call prevents connection reuse and re-performs TLS handshakes. Consider sharing a single client instance at the service level (initialized in__init__and closed on disposal).♻️ Proposed refactor
class IssueSuggestionService: def __init__(self, token: str): self.token = token + self._client = httpx.AsyncClient(timeout=10.0) + + async def aclose(self): + await self._client.aclose() async def fetch_beginner_issues(self, language: str = "python", limit: int = 5) -> List[Dict]: ... - async with httpx.AsyncClient(timeout=10.0) as client: - response = await client.get(...) + response = await self._client.get( + f"{GITHUB_API_BASE}/search/issues", + headers=headers, + params={"q": query, "per_page": limit}, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/github/issue_suggestion_service.py` around lines 29 - 37, Per-request AsyncClient creation in the issue suggestion code prevents connection pooling; refactor by creating a single httpx.AsyncClient on the service instance (e.g., in __init__ as self._client with timeout=10.0) and replace the local "async with httpx.AsyncClient(...)" usage with self._client.get(...) in the method that currently builds the request (the block calling client.get f"{GITHUB_API_BASE}/search/issues" with params q and per_page). Add a proper disposal path (an async close/shutdown method or implement __aenter__/__aexit__ / __del__ to call await self._client.aclose()) so the client is closed when the service is torn down.
39-40: Silent failure on both non-200 responses and exceptions makes debugging impossible.The non-200 branch (Line 39) and the broad
except Exception(Line 56) both return[]without logging. This swallows GitHub API errors (401, 403, 429), network timeouts, and even programming errors likeAttributeError. Narrow the exception to httpx-specific types and add logging to both paths.♻️ Proposed fix
+ import logging + logger = logging.getLogger(__name__) ... if response.status_code != 200: + logger.warning( + "GitHub API returned %s: %s", + response.status_code, response.text[:200] + ) return [] ... - except Exception: - # Fail gracefully — do not crash API - return [] + except (httpx.TimeoutException, httpx.RequestError) as exc: + logger.exception("GitHub request failed: %s", exc) + return []Also applies to: 56-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/github/issue_suggestion_service.py` around lines 39 - 40, The code currently returns [] silently on non-200 responses and on a broad except Exception; change the non-200 branch that checks response.status_code to log the status code, URL/endpoint and response body (or response.text) before returning [], and replace the broad "except Exception" with a narrowed httpx exception catch (e.g., httpx.HTTPError or httpx.RequestError) that logs the exception details and stacktrace; leave other unexpected exceptions to propagate (or re-raise) so real bugs aren't swallowed. Ensure you update the blocks around the response variable and the current except Exception handler to use the logger and the specific httpx exception classes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/services/github/issue_suggestion_service.py`:
- Line 26: The constructed query uses the unvalidated variable `language` (used
in the `query = f'label:"good first issue" language:{language} state:open'`
line), allowing injection like `"python state:closed"` or empty strings; fix by
validating/sanitizing `language` before building `query`: either whitelist
acceptable language identifiers (e.g., a set of known languages) or
reject/normalize inputs containing spaces, colons, or other special chars, and
ensure empty values are skipped or defaulted; apply this check where `language`
is accepted (the function that builds `query`) and only interpolate a validated
single-token language into `query`.
- Around line 11-15: The method signature for fetch_beginner_issues does not
accept the owner and repo kwargs used by its callers; update the
fetch_beginner_issues definition to accept owner: str and repo: str (e.g., def
fetch_beginner_issues(self, owner: str, repo: str, language: str = "python",
limit: int = 5) -> List[Dict]) so the callers that pass owner/repo won't raise
TypeError, and adjust any internal references to use these parameters (also
update the method docstring to reflect the new parameters).
---
Duplicate comments:
In `@backend/services/github/issue_suggestion_service.py`:
- Around line 7-15: The file contains a duplicate implementation of
IssueSuggestionService with fetch_beginner_issues that mirrors the class in the
other module which defines fetch_global_beginner_issues; remove the duplicate by
consolidating shared behavior into a single service and updating callers to use
that unified class/method: extract common logic into one class (e.g.,
IssueSuggestionService) or a shared helper, migrate/rename
fetch_global_beginner_issues or fetch_beginner_issues to the canonical method,
and delete the redundant class in this file; ensure token handling and method
signature (language, limit) are preserved and update any imports/usages to
reference the consolidated class/method.
---
Nitpick comments:
In `@backend/services/github/issue_suggestion_service.py`:
- Around line 29-37: Per-request AsyncClient creation in the issue suggestion
code prevents connection pooling; refactor by creating a single
httpx.AsyncClient on the service instance (e.g., in __init__ as self._client
with timeout=10.0) and replace the local "async with httpx.AsyncClient(...)"
usage with self._client.get(...) in the method that currently builds the request
(the block calling client.get f"{GITHUB_API_BASE}/search/issues" with params q
and per_page). Add a proper disposal path (an async close/shutdown method or
implement __aenter__/__aexit__ / __del__ to call await self._client.aclose()) so
the client is closed when the service is torn down.
- Around line 39-40: The code currently returns [] silently on non-200 responses
and on a broad except Exception; change the non-200 branch that checks
response.status_code to log the status code, URL/endpoint and response body (or
response.text) before returning [], and replace the broad "except Exception"
with a narrowed httpx exception catch (e.g., httpx.HTTPError or
httpx.RequestError) that logs the exception details and stacktrace; leave other
unexpected exceptions to propagate (or re-raise) so real bugs aren't swallowed.
Ensure you update the blocks around the response variable and the current except
Exception handler to use the logger and the specific httpx exception classes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/routes.py (1)
153-153: Replacelogging.errorwithlogging.exceptionto capture the traceback.
logging.exceptionautomatically attaches the current exception's traceback, which is strictly more useful for diagnosing failures here.logging.erroronly records the formatted message.♻️ Proposed fix
- logging.error(f"Error fetching beginner issues: {e}") + logging.exception("Error fetching beginner issues")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes.py` at line 153, Replace the current logging call logging.error(f"Error fetching beginner issues: {e}") with logging.exception so the traceback is recorded; locate the logging statement (logging.error(f"Error fetching beginner issues: {e}")) in backend/routes.py and change it to use logging.exception (and you can omit the explicit exception interpolation since logging.exception will include the exception and stack trace).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/routes.py`:
- Around line 59-60: The github_webhook handler currently accepts payloads
without verifying GitHub signatures; update the github_webhook(Request) function
to read the raw request body, compute an HMAC-SHA256 using the configured
webhook secret, and compare it securely (use hmac.compare_digest) against the
incoming X-Hub-Signature-256 header; if the header is missing or the signature
verification fails, return an HTTP 401/403 and do not dispatch the event. Ensure
you reference the same Request object used in github_webhook and fail-fast
before any decoding/processing of the payload.
- Line 1: The import for IssueSuggestionService in backend/routes.py fails
because backend/services and backend/services/github are not packages; either
add __init__.py files to backend/services/ and backend/services/github/ so the
existing import from services.github.issue_suggestion_service import
IssueSuggestionService works, or move issue_suggestion_service.py into the
existing package path backend/app/services/github/ and update the import in
backend/routes.py to from app.services.github.issue_suggestion_service import
IssueSuggestionService; ensure the referenced symbol IssueSuggestionService is
imported from the chosen location and that the package init files exist for the
chosen approach.
---
Nitpick comments:
In `@backend/routes.py`:
- Line 153: Replace the current logging call logging.error(f"Error fetching
beginner issues: {e}") with logging.exception so the traceback is recorded;
locate the logging statement (logging.error(f"Error fetching beginner issues:
{e}")) in backend/routes.py and change it to use logging.exception (and you can
omit the explicit exception interpolation since logging.exception will include
the exception and stack trace).
backend/routes.py
Outdated
| @@ -1,4 +1,6 @@ | |||
| import asyncio | |||
| from services.github.issue_suggestion_service import IssueSuggestionService | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find issue_suggestion_service.py file location
fd "issue_suggestion_service.py" --type f
# Check for services directories under backend/
fd --max-depth 2 --type d "services" backend/
# Also check git structure to understand the layout
git ls-files | grep -E "(routes\.py|issue_suggestion_service)" | head -20Repository: AOSSIE-Org/Devr.AI
Length of output: 363
🏁 Script executed:
# Show the imports section at the top of backend/routes.py
head -15 backend/routes.py
# Check what's in each issue_suggestion_service.py file (headers only)
echo "=== backend/services/github/issue_suggestion_service.py ===" && head -30 backend/services/github/issue_suggestion_service.py
echo -e "\n=== backend/app/agents/devrel/github/services/issue_suggestion_service.py ===" && head -30 backend/app/agents/devrel/github/services/issue_suggestion_service.pyRepository: AOSSIE-Org/Devr.AI
Length of output: 2188
🏁 Script executed:
# Check for __init__.py files in the services hierarchy
find backend/services -name "__init__.py" -type f
# Also check the app/services hierarchy
find backend/app/services -name "__init__.py" -type fRepository: AOSSIE-Org/Devr.AI
Length of output: 271
🏁 Script executed:
# Check if there's a sys.path setup or PYTHONPATH configuration in backend
cat backend/routes.py | head -30
# Check root level main/entry point
find backend -maxdepth 1 -name "*.py" -type fRepository: AOSSIE-Org/Devr.AI
Length of output: 989
🏁 Script executed:
# Check the main.py to understand how the app is configured
cat backend/main.py
# Check if backend/__init__.py has any sys.path setup
cat backend/__init__.pyRepository: AOSSIE-Org/Devr.AI
Length of output: 1969
🏁 Script executed:
# Check if backend/routes.py is actually imported/used anywhere
rg "from.*routes import|import.*routes" --type py
# Verify once more that backend/services/__init__.py does NOT exist
ls -la backend/services/__init__.py 2>&1
# Check the full directory structure under backend/services
ls -la backend/services/github/Repository: AOSSIE-Org/Devr.AI
Length of output: 325
🏁 Script executed:
# Check if backend/app/services/github/ directory exists
ls -la backend/app/services/github/ 2>&1
# Also check what files are in backend/app/services/
find backend/app/services -type f -name "*.py" | head -20Repository: AOSSIE-Org/Devr.AI
Length of output: 951
Wrong import path for IssueSuggestionService — will cause ModuleNotFoundError at startup.
The import from services.github.issue_suggestion_service import IssueSuggestionService fails because backend/services/ is not a proper Python package—it's missing __init__.py files in both backend/services/ and backend/services/github/ directories. The file exists at backend/services/github/issue_suggestion_service.py but Python cannot import from it without the package marker files.
Additionally, the suggested fix to use from app.services.github.issue_suggestion_service won't work because that file doesn't exist at that location.
To fix: Either add __init__.py files to backend/services/ and backend/services/github/, or move issue_suggestion_service.py to backend/app/services/github/issue_suggestion_service.py (which already has proper __init__.py files in place).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/routes.py` at line 1, The import for IssueSuggestionService in
backend/routes.py fails because backend/services and backend/services/github are
not packages; either add __init__.py files to backend/services/ and
backend/services/github/ so the existing import from
services.github.issue_suggestion_service import IssueSuggestionService works, or
move issue_suggestion_service.py into the existing package path
backend/app/services/github/ and update the import in backend/routes.py to from
app.services.github.issue_suggestion_service import IssueSuggestionService;
ensure the referenced symbol IssueSuggestionService is imported from the chosen
location and that the package init files exist for the chosen approach.
Comment on lines
59
to
60
| @router.post("/github/webhook") | ||
| async def github_webhook(request: Request): |
Contributor
There was a problem hiding this comment.
Missing GitHub webhook signature verification — endpoint accepts forged payloads.
The /github/webhook handler reads and dispatches events without verifying the X-Hub-Signature-256 HMAC sent by GitHub. Any unauthenticated caller can POST crafted payloads, inject arbitrary actor_id values, and trigger downstream event handlers.
Add a HMAC-SHA256 verification step using the webhook secret before processing the payload:
🔒 Proposed fix
+import hmac
+import hashlib
+from backend.config import GITHUB_WEBHOOK_SECRET # add this to config
`@router.post`("/github/webhook")
async def github_webhook(request: Request):
+ body = await request.body()
+ sig_header = request.headers.get("X-Hub-Signature-256", "")
+ if GITHUB_WEBHOOK_SECRET:
+ expected = "sha256=" + hmac.new(
+ GITHUB_WEBHOOK_SECRET.encode(), body, hashlib.sha256
+ ).hexdigest()
+ if not hmac.compare_digest(expected, sig_header):
+ raise HTTPException(status_code=401, detail="Invalid webhook signature")
+ payload = await request.json()
- payload = await request.json()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/routes.py` around lines 59 - 60, The github_webhook handler currently
accepts payloads without verifying GitHub signatures; update the
github_webhook(Request) function to read the raw request body, compute an
HMAC-SHA256 using the configured webhook secret, and compare it securely (use
hmac.compare_digest) against the incoming X-Hub-Signature-256 header; if the
header is missing or the signature verification fails, return an HTTP 401/403
and do not dispatch the event. Ensure you reference the same Request object used
in github_webhook and fail-fast before any decoding/processing of the payload.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
backend/app/api/v1/github.py (1)
37-41: No logger defined in this module — exceptions are swallowed silently.There is no
import loggingorlogger = logging.getLogger(__name__)in this file. Theexceptblock raises a safeHTTPException, but the original exception is never logged anywhere, making debugging impossible.♻️ Proposed fix
from fastapi import APIRouter, HTTPException from services.github.issue_suggestion_service import IssueSuggestionService from config import GITHUB_TOKEN +import logging + +logger = logging.getLogger(__name__) ... except Exception as e: + logger.exception("Failed to fetch beginner issues") raise HTTPException( status_code=500, detail="Failed to fetch beginner issues" ) from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/v1/github.py` around lines 37 - 41, The except block in github.py that catches Exception e and raises HTTPException("Failed to fetch beginner issues") swallows the original error; add logging by importing logging and creating logger = logging.getLogger(__name__), then in the except block call logger.exception or logger.error with the exception details (e.g., logger.exception("Failed to fetch beginner issues") or logger.error("Failed to fetch beginner issues: %s", e)) before re-raising the HTTPException so the original stacktrace is recorded while still returning the safe HTTP response.backend/routes.py (2)
153-153: Uselogging.exceptionto preserve the traceback (Ruff TRY400).♻️ Proposed fix
- logging.error(f"Error fetching beginner issues: {e}") + logging.exception("Error fetching beginner issues")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes.py` at line 153, Replace the plain error log inside the except block that currently calls logging.error(f"Error fetching beginner issues: {e}") with logging.exception to preserve the traceback; locate the logging call (logging.error(...)) in backend/routes.py where beginner issues are fetched and swap it to logging.exception("Error fetching beginner issues") (ensure it remains inside the except block so the current exception context is captured).
125-157: Duplicate endpoint withbackend/app/api/v1/github.py.Both
routes.py(GET /beginner-issues) andapp/api/v1/github.py(GET /github/beginner-issues) instantiateIssueSuggestionServiceand implement identical token validation + service call + response shaping logic. This should be consolidated into a single endpoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes.py` around lines 125 - 157, Duplicate beginner-issues endpoints exist: get_beginner_issues in routes.py and the handler in app/api/v1/github.py both repeat token checks, instantiate IssueSuggestionService, and shape the same response; remove the duplication by keeping a single endpoint handler and delegating all logic to a shared service function. Refactor by extracting the common logic into a single function (e.g., fetch_beginner_issues or use the existing IssueSuggestionService method) and have only one route (either keep get_beginner_issues or the /github/beginner-issues handler) call that shared function; delete the redundant route implementation and ensure token validation uses the same centralized check so only one place (the retained handler) constructs the response and handles exceptions. Ensure you reference get_beginner_issues, IssueSuggestionService, and the existing /github/beginner-issues handler when making these changes so routing and imports are adjusted accordingly.backend/app/agents/devrel/github/github_toolkit.py (1)
159-160: Uselogger.exceptionand move success path toelseblock (Ruff TRY300/TRY400).
logger.errordoes not capture the traceback;logger.exceptionshould be used insideexceptblocks. Ruff also suggests that lines 154–155 (result["intent_analysis"]assignment) belong in anelseblock to avoid executing them whenresultmight be unset.♻️ Proposed fix
+ else: result["intent_analysis"] = intent_result result["type"] = "github_toolkit" return result except Exception as e: - logger.error(f"GitHub toolkit execution error: {str(e)}") + logger.exception("GitHub toolkit execution error: %s", e)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/agents/devrel/github/github_toolkit.py` around lines 159 - 160, Replace the logger.error call in the except block with logger.exception to record the traceback (i.e., change logger.error(f"GitHub toolkit execution error: {str(e)}") to logger.exception(...)), and move the assignment to result["intent_analysis"] into an else block paired with the try/except so it only runs when no exception occurred; ensure the try block sets result, the except uses logger.exception(e) and the else performs result["intent_analysis"] assignment to avoid referencing an unset result.backend/app/agents/devrel/github/tools/general_github_help.py (1)
11-11:llmparameter is unused — Ruff ARG001.Since the function no longer invokes any LLM path, the
llmparameter is dead code. Remove it from the signature and update the single call site ingithub_toolkit.py(handle_general_github_help(query, None)→handle_general_github_help(query)).♻️ Proposed fix
-async def handle_general_github_help(query: str, llm=None) -> Dict[str, Any]: +async def handle_general_github_help(query: str) -> Dict[str, Any]:And in
backend/app/agents/devrel/github/github_toolkit.pyline 152:- result = await handle_general_github_help(query, None) + result = await handle_general_github_help(query)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/agents/devrel/github/tools/general_github_help.py` at line 11, Remove the unused llm parameter from the function signature of handle_general_github_help (change async def handle_general_github_help(query: str, llm=None) -> Dict[str, Any]: to accept only query) and update its single call site in github_toolkit.py by removing the None argument (change handle_general_github_help(query, None) to handle_general_github_help(query)). Ensure any type hints/imports referencing llm are cleaned up and run tests/lint to confirm Ruff ARG001 is resolved.backend/services/github/issue_suggestion_service.py (1)
58-60: Prefer specific exception types over a blanketexcept Exception.Catching all exceptions hides bugs (e.g.,
KeyErrorin the list comprehension,JSONDecodeError) and makes debugging harder. The static analysis tool (Ruff BLE001) also flags this.♻️ Proposed refactor
- except Exception: - # Fail gracefully - return [] + except (httpx.TimeoutException, httpx.RequestError, httpx.HTTPStatusError): + return []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/github/issue_suggestion_service.py` around lines 58 - 60, Replace the blanket "except Exception:" in backend/services/github/issue_suggestion_service.py with specific exception handlers for the actual failure modes (e.g., catch json.JSONDecodeError, KeyError for the list comprehension, requests.RequestException or whatever HTTP client error type is used, and ValueError if parsing can fail); for each caught exception log the full exception (logger.exception or similar) and return [] as before, and do not swallow other unexpected exceptions—let them propagate so they surface in CI/logs (this removes the broad "except Exception" and narrows handling to the specific error types around the failing code path).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/agents/devrel/github/github_toolkit.py`:
- Around line 119-124: In the "find_good_first_issues" branch replace the
non-existent call to service.fetch_global_beginner_issues(query) with the
existing IssueSuggestionService method: call
service.fetch_beginner_issues(language, limit) (e.g., pass the query as the
language argument or map the query to a language and choose a sensible default
limit like 10), or alternatively add a new wrapper method
fetch_global_beginner_issues(query) to IssueSuggestionService that adapts the
free-text query into the (language, limit) signature; locate
IssueSuggestionService.fetch_beginner_issues and the classification branch where
service.fetch_global_beginner_issues is invoked to make the change.
- Line 121: The instantiation of IssueSuggestionService in github_toolkit.py
uses settings.github_token which is inconsistent with other callers using
config.GITHUB_TOKEN; update the IssueSuggestionService construction to use the
same token source as the rest of the codebase (config.GITHUB_TOKEN) or modify
the Settings model so settings.github_token implements the same fallback
(os.getenv("GITHUB_TOKEN") or os.getenv("GH_TOKEN"))—ensure the change touches
the IssueSuggestionService(...) call in github_toolkit.py and aligns it with the
token used in routes.py and app/api/v1/github.py to avoid divergent values.
- Line 1: The import in github_toolkit.py using "from
services.github.issue_suggestion_service import IssueSuggestionService" will
raise ModuleNotFoundError because backend/services and backend/services/github
are not Python packages; fix by either adding __init__.py files to
backend/services and backend/services/github so the import resolves, or move
issue_suggestion_service.py into the packaged backend/app/services/github tree
and update the import in github_toolkit.py accordingly; ensure the import path
matches the chosen location and that IssueSuggestionService is reachable from
that module.
In `@backend/app/api/v1/github.py`:
- Line 10: The route decorator currently declares the full path
"/github/beginner-issues" which causes a duplicated segment when this router is
mounted under the "/v1/github" prefix; change the route on the router.get
decorator to use the relative path "/beginner-issues" (i.e., update the
`@router.get`(...) call) so the endpoint becomes "/v1/github/beginner-issues" when
mounted.
In `@backend/routes.py`:
- Line 15: Replace the module-level call to logging.basicConfig with a
module-scoped named logger: remove the logging.basicConfig(level=logging.INFO)
line and add logger = logging.getLogger(__name__) (optionally
logger.setLevel(logging.INFO) if you need a default level), but do not attach
handlers here — let the application/entry-point configure logging handlers and
formatters; update any uses of the root logger to call this module logger
instead.
In `@backend/services/github/issue_suggestion_service.py`:
- Around line 27-28: The current guard using language.isalnum() in
backend/services/github/issue_suggestion_service.py incorrectly rejects valid
language identifiers (e.g., "C++", "C#", "Objective-C", "Jupyter Notebook");
change the validation to: trim/strip the incoming language, then treat it as
valid unless it contains obvious shell-injection or delimiter characters by
using a regex check (for example reject if re.search(r'[ \t\n\r;:"\'`|&$<>]',
language) or similar), and only fall back to "python" if language is empty or
fails that forbidden-character test; update the code that sets the language
variable accordingly (refer to the language variable assignment in this file) so
valid non-alphanumeric identifiers are preserved while blocking injection-prone
characters.
---
Duplicate comments:
In `@backend/app/api/v1/github.py`:
- Line 2: The import for IssueSuggestionService causes ModuleNotFoundError at
startup; replace the bare absolute import with the project's package-aware
import that other modules use (e.g., use the package-prefixed import or a
relative import) so the module can be resolved at runtime — update the import of
IssueSuggestionService in github.py (from
services.github.issue_suggestion_service import IssueSuggestionService) to the
correct package import (for example, from
app.services.github.issue_suggestion_service import IssueSuggestionService or a
relative import like from ...services.github.issue_suggestion_service import
IssueSuggestionService) to match how routes.py and other modules import
services.
In `@backend/routes.py`:
- Line 1: The import in routes.py (from services.github.issue_suggestion_service
import IssueSuggestionService) fails because backend/services and
backend/services/github are not Python packages; fix by either adding empty
__init__.py files into the backend/services and backend/services/github
directories so the import path becomes valid, or move
issue_suggestion_service.py into an existing package (e.g.,
backend/app/services/github/) and update the import in backend/routes.py to the
new package path to match the project package layout.
---
Nitpick comments:
In `@backend/app/agents/devrel/github/github_toolkit.py`:
- Around line 159-160: Replace the logger.error call in the except block with
logger.exception to record the traceback (i.e., change logger.error(f"GitHub
toolkit execution error: {str(e)}") to logger.exception(...)), and move the
assignment to result["intent_analysis"] into an else block paired with the
try/except so it only runs when no exception occurred; ensure the try block sets
result, the except uses logger.exception(e) and the else performs
result["intent_analysis"] assignment to avoid referencing an unset result.
In `@backend/app/agents/devrel/github/tools/general_github_help.py`:
- Line 11: Remove the unused llm parameter from the function signature of
handle_general_github_help (change async def handle_general_github_help(query:
str, llm=None) -> Dict[str, Any]: to accept only query) and update its single
call site in github_toolkit.py by removing the None argument (change
handle_general_github_help(query, None) to handle_general_github_help(query)).
Ensure any type hints/imports referencing llm are cleaned up and run tests/lint
to confirm Ruff ARG001 is resolved.
In `@backend/app/api/v1/github.py`:
- Around line 37-41: The except block in github.py that catches Exception e and
raises HTTPException("Failed to fetch beginner issues") swallows the original
error; add logging by importing logging and creating logger =
logging.getLogger(__name__), then in the except block call logger.exception or
logger.error with the exception details (e.g., logger.exception("Failed to fetch
beginner issues") or logger.error("Failed to fetch beginner issues: %s", e))
before re-raising the HTTPException so the original stacktrace is recorded while
still returning the safe HTTP response.
In `@backend/routes.py`:
- Line 153: Replace the plain error log inside the except block that currently
calls logging.error(f"Error fetching beginner issues: {e}") with
logging.exception to preserve the traceback; locate the logging call
(logging.error(...)) in backend/routes.py where beginner issues are fetched and
swap it to logging.exception("Error fetching beginner issues") (ensure it
remains inside the except block so the current exception context is captured).
- Around line 125-157: Duplicate beginner-issues endpoints exist:
get_beginner_issues in routes.py and the handler in app/api/v1/github.py both
repeat token checks, instantiate IssueSuggestionService, and shape the same
response; remove the duplication by keeping a single endpoint handler and
delegating all logic to a shared service function. Refactor by extracting the
common logic into a single function (e.g., fetch_beginner_issues or use the
existing IssueSuggestionService method) and have only one route (either keep
get_beginner_issues or the /github/beginner-issues handler) call that shared
function; delete the redundant route implementation and ensure token validation
uses the same centralized check so only one place (the retained handler)
constructs the response and handles exceptions. Ensure you reference
get_beginner_issues, IssueSuggestionService, and the existing
/github/beginner-issues handler when making these changes so routing and imports
are adjusted accordingly.
In `@backend/services/github/issue_suggestion_service.py`:
- Around line 58-60: Replace the blanket "except Exception:" in
backend/services/github/issue_suggestion_service.py with specific exception
handlers for the actual failure modes (e.g., catch json.JSONDecodeError,
KeyError for the list comprehension, requests.RequestException or whatever HTTP
client error type is used, and ValueError if parsing can fail); for each caught
exception log the full exception (logger.exception or similar) and return [] as
before, and do not swallow other unexpected exceptions—let them propagate so
they surface in CI/logs (this removes the broad "except Exception" and narrows
handling to the specific error types around the failing code path).
| @@ -1,12 +1,9 @@ | |||
| from services.github.issue_suggestion_service import IssueSuggestionService | |||
Contributor
There was a problem hiding this comment.
Same ModuleNotFoundError import risk as in routes.py.
backend/services/ and backend/services/github/ lack __init__.py files, so from services.github.issue_suggestion_service import IssueSuggestionService will fail at startup. The fix is the same as previously flagged for routes.py: either add the missing __init__.py files or move issue_suggestion_service.py into the properly-packaged backend/app/services/github/ tree.
#!/bin/bash
# Verify package structure for services.github
find backend/services -type f -name "*.py" | sort
find backend/app/services -type f -name "*.py" | sort🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/agents/devrel/github/github_toolkit.py` at line 1, The import in
github_toolkit.py using "from services.github.issue_suggestion_service import
IssueSuggestionService" will raise ModuleNotFoundError because backend/services
and backend/services/github are not Python packages; fix by either adding
__init__.py files to backend/services and backend/services/github so the import
resolves, or move issue_suggestion_service.py into the packaged
backend/app/services/github tree and update the import in github_toolkit.py
accordingly; ensure the import path matches the chosen location and that
IssueSuggestionService is reachable from that module.
Comment on lines
119
to
124
| elif classification == "find_good_first_issues": | ||
|
|
||
| service = IssueSuggestionService(settings.github_token) | ||
|
|
||
| # ✅ FIXED — passing query argument | ||
| issues = await service.fetch_global_beginner_issues(query) |
Contributor
There was a problem hiding this comment.
fetch_global_beginner_issues does not exist — this will raise AttributeError at runtime.
IssueSuggestionService (imported from services.github.issue_suggestion_service) only defines fetch_beginner_issues(self, language, limit). There is no fetch_global_beginner_issues method, and passing a free-text query string to it wouldn't match the (language, limit) signature anyway.
🐛 Proposed fix
- # ✅ FIXED — passing query argument
- issues = await service.fetch_global_beginner_issues(query)
+ # Extract language from query if possible, or default to "python"
+ issues = await service.fetch_beginner_issues(language="python", limit=5)📝 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.
Suggested change
| elif classification == "find_good_first_issues": | |
| service = IssueSuggestionService(settings.github_token) | |
| # ✅ FIXED — passing query argument | |
| issues = await service.fetch_global_beginner_issues(query) | |
| elif classification == "find_good_first_issues": | |
| service = IssueSuggestionService(settings.github_token) | |
| # Extract language from query if possible, or default to "python" | |
| issues = await service.fetch_beginner_issues(language="python", limit=5) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/agents/devrel/github/github_toolkit.py` around lines 119 - 124,
In the "find_good_first_issues" branch replace the non-existent call to
service.fetch_global_beginner_issues(query) with the existing
IssueSuggestionService method: call service.fetch_beginner_issues(language,
limit) (e.g., pass the query as the language argument or map the query to a
language and choose a sensible default limit like 10), or alternatively add a
new wrapper method fetch_global_beginner_issues(query) to IssueSuggestionService
that adapts the free-text query into the (language, limit) signature; locate
IssueSuggestionService.fetch_beginner_issues and the classification branch where
service.fetch_global_beginner_issues is invoked to make the change.
|
|
||
| elif classification == "find_good_first_issues": | ||
|
|
||
| service = IssueSuggestionService(settings.github_token) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for github_token in app/core/config.py and GITHUB_TOKEN in config files
echo "=== Searching for github_token in app/core/config.py ==="
rg -n "github_token" backend/app/core/config.py
echo -e "\n=== Searching for GITHUB_TOKEN in config files ==="
rg -n "GITHUB_TOKEN" backend/config.py 2>/dev/null || echo "backend/config.py not found"
echo -e "\n=== Searching for all config references ==="
fd "config.py" backend/ --type f
echo -e "\n=== Check full app/core/config.py structure ==="
cat backend/app/core/config.py | head -50Repository: AOSSIE-Org/Devr.AI
Length of output: 494
🏁 Script executed:
#!/bin/bash
# Search for IssueSuggestionService instantiation in routes.py and app/api/v1/github.py
echo "=== Searching for IssueSuggestionService in routes.py ==="
rg -n "IssueSuggestionService" backend/app/routes.py -A 1 -B 1
echo -e "\n=== Searching for IssueSuggestionService in app/api/v1/github.py ==="
rg -n "IssueSuggestionService" backend/app/api/v1/github.py -A 1 -B 1
echo -e "\n=== Searching for all IssueSuggestionService usages ==="
rg -n "IssueSuggestionService" backend/ -A 1 -B 1Repository: AOSSIE-Org/Devr.AI
Length of output: 1891
🏁 Script executed:
#!/bin/bash
# Check full github_toolkit.py for settings import
echo "=== First 20 lines of github_toolkit.py ==="
head -20 backend/app/agents/devrel/github/github_toolkit.py
echo -e "\n=== Search for settings import in github_toolkit.py ==="
rg -n "^from|^import" backend/app/agents/devrel/github/github_toolkit.py | head -20
echo -e "\n=== Look for all settings references ==="
rg -n "settings\." backend/app/agents/devrel/github/github_toolkit.pyRepository: AOSSIE-Org/Devr.AI
Length of output: 1446
🏁 Script executed:
#!/bin/bash
# Find where settings object is defined
echo "=== Search for Settings class definition ==="
rg -n "class Settings" backend/ -A 3
echo -e "\n=== Search for settings attribute assignments ==="
rg -n "github_token\s*=" backend/ -B 2 -A 1
echo -e "\n=== Check if settings module exists ==="
fd "settings" backend/ --type fRepository: AOSSIE-Org/Devr.AI
Length of output: 473
🏁 Script executed:
#!/bin/bash
# Check the full Settings class definition
echo "=== Full Settings class in app/core/config/settings.py ==="
cat backend/app/core/config/settings.py
echo -e "\n=== Check app/core/config/__init__.py ==="
cat backend/app/core/config/__init__.py 2>/dev/null || echo "File not found"Repository: AOSSIE-Org/Devr.AI
Length of output: 1654
🏁 Script executed:
#!/bin/bash
# Check config.py for full token configuration
echo "=== Full backend/config.py ==="
cat backend/config.py
echo -e "\n=== Check if github_token is in any config ==="
rg -n "github_token" backend/Repository: AOSSIE-Org/Devr.AI
Length of output: 1017
Inconsistent token source — settings.github_token vs config.GITHUB_TOKEN.
routes.py and app/api/v1/github.py instantiate IssueSuggestionService(GITHUB_TOKEN) from config (direct os.getenv() with fallback to GH_TOKEN), while github_toolkit.py uses settings.github_token from app.core.config (Pydantic BaseSettings). Although both attributes exist, they read from environment variables through different mechanisms with different fallback behaviors:
config.GITHUB_TOKEN:os.getenv("GITHUB_TOKEN") or os.getenv("GH_TOKEN")settings.github_token: Pydantic BaseSettings (defaults to empty string if not set)
This inconsistency could cause IssueSuggestionService to receive different token values or fail silently if one configuration path is properly set while the other isn't.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/agents/devrel/github/github_toolkit.py` at line 121, The
instantiation of IssueSuggestionService in github_toolkit.py uses
settings.github_token which is inconsistent with other callers using
config.GITHUB_TOKEN; update the IssueSuggestionService construction to use the
same token source as the rest of the codebase (config.GITHUB_TOKEN) or modify
the Settings model so settings.github_token implements the same fallback
(os.getenv("GITHUB_TOKEN") or os.getenv("GH_TOKEN"))—ensure the change touches
the IssueSuggestionService(...) call in github_toolkit.py and aligns it with the
token used in routes.py and app/api/v1/github.py to avoid divergent values.
backend/app/api/v1/github.py
Outdated
| issue_service = IssueSuggestionService(GITHUB_TOKEN) | ||
|
|
||
|
|
||
| @router.get("/github/beginner-issues") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "router.py" backend/app/api/ --type f -exec cat {} \;Repository: AOSSIE-Org/Devr.AI
Length of output: 208
🏁 Script executed:
find backend/app/api -name "router.py" -type fRepository: AOSSIE-Org/Devr.AI
Length of output: 87
🏁 Script executed:
cat backend/app/api/router.pyRepository: AOSSIE-Org/Devr.AI
Length of output: 731
🏁 Script executed:
cat backend/app/api/v1/github.pyRepository: AOSSIE-Org/Devr.AI
Length of output: 1039
🏁 Script executed:
grep -n "@router.get" backend/app/api/v1/github.pyRepository: AOSSIE-Org/Devr.AI
Length of output: 103
Route path /github/beginner-issues produces a double /github/ prefix when mounted.
This router is included under the prefix /v1/github (in backend/app/api/router.py), making the effective URL /v1/github/github/beginner-issues. The route path should be /beginner-issues to yield /v1/github/beginner-issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/v1/github.py` at line 10, The route decorator currently
declares the full path "/github/beginner-issues" which causes a duplicated
segment when this router is mounted under the "/v1/github" prefix; change the
route on the router.get decorator to use the relative path "/beginner-issues"
(i.e., update the `@router.get`(...) call) so the endpoint becomes
"/v1/github/beginner-issues" when mounted.
backend/routes.py
Outdated
|
|
||
| router = APIRouter() | ||
|
|
||
| logging.basicConfig(level=logging.INFO) |
Contributor
There was a problem hiding this comment.
logging.basicConfig at module scope silently hijacks the root logger.
Calling basicConfig in a module that is imported by the application (rather than in the entry-point if __name__ == "__main__" guard or the app factory) configures the root logger for the whole process. Any logging configuration set by the host application before or after this import is either ignored or overridden.
♻️ Proposed fix
-logging.basicConfig(level=logging.INFO)Use a module-level named logger instead:
+logger = logging.getLogger(__name__)📝 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.
Suggested change
| logging.basicConfig(level=logging.INFO) | |
| logger = logging.getLogger(__name__) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/routes.py` at line 15, Replace the module-level call to
logging.basicConfig with a module-scoped named logger: remove the
logging.basicConfig(level=logging.INFO) line and add logger =
logging.getLogger(__name__) (optionally logger.setLevel(logging.INFO) if you
need a default level), but do not attach handlers here — let the
application/entry-point configure logging handlers and formatters; update any
uses of the root logger to call this module logger instead.
Comment on lines
+27
to
+28
| if not language or not language.isalnum(): | ||
| language = "python" |
Contributor
There was a problem hiding this comment.
isalnum() silently rejects valid multi-word and symbol language identifiers.
Common GitHub-searchable languages like C++, C#, Objective-C, and Jupyter Notebook all fail isalnum() and silently fall back to "python" with no indication to the caller. Consider a less restrictive guard that only blocks shell-injection characters (spaces, colons, quotes) while preserving these valid identifiers.
🛡️ Proposed fix
- if not language or not language.isalnum():
- language = "python"
+ # Allow alphanumeric, +, #, space (e.g. C++, C#, Jupyter Notebook)
+ # Reject chars that could alter the search query syntax
+ import re
+ if not language or re.search(r'[:"\'\\]', language):
+ language = "python"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/services/github/issue_suggestion_service.py` around lines 27 - 28,
The current guard using language.isalnum() in
backend/services/github/issue_suggestion_service.py incorrectly rejects valid
language identifiers (e.g., "C++", "C#", "Objective-C", "Jupyter Notebook");
change the validation to: trim/strip the incoming language, then treat it as
valid unless it contains obvious shell-injection or delimiter characters by
using a regex check (for example reject if re.search(r'[ \t\n\r;:"\'`|&$<>]',
language) or similar), and only fall back to "python" if language is empty or
fails that forbidden-character test; update the code that sets the language
variable accordingly (refer to the language variable assignment in this file) so
valid non-alphanumeric identifiers are preserved while blocking injection-prone
characters.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/app/services/github/issue_suggestion_service.py (2)
43-44: Silent failure on non-200 responses loses observability.Both the non-200 path (line 44) and the exception path (line 60) return
[]with no log. In production this makes it impossible to distinguish a legitimate "no results" from a rate-limit (429), an auth failure (401), or a network error.Consider adding a structured log at
WARNINGlevel in both paths — at minimum loggingresponse.status_codeand the caught exception type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/github/issue_suggestion_service.py` around lines 43 - 44, The non-200 branch (where `response.status_code != 200`) and the exception handler currently return [] silently; update the HTTP error path and the exception catch in the issue suggestion routine (the function that performs the external request in issue_suggestion_service.py) to emit a structured WARNING-level log including the `response.status_code` (and response body or url if available) for non-200s, and the exception type/name and message in the except block; use the module/class logger (e.g., `logger` or `self.logger`) so callers still return [] but operators can distinguish 429/401/network errors from genuine no-results.
58-60: Narrow the bareexcept Exceptionto specific httpx and JSON exceptions.Catching all exceptions (Ruff BLE001) silently swallows unexpected programming errors (e.g.,
AttributeError,TypeError), making bugs invisible. In this code, the two failure modes are network/timeout errors from httpx and invalid JSON parsing — both have precise types.Suggested refactor
- except Exception: - # Fail gracefully - return [] + except (httpx.RequestError, ValueError): + return []
httpx.RequestErroris the correct base exception for all network and timeout errors in httpx 0.28.1, includingTimeoutExceptionand its subtypes.ValueErrorcovers JSON parsing failures fromresponse.json().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/github/issue_suggestion_service.py` around lines 58 - 60, The bare except in the issue suggestion flow should be narrowed to only handle HTTP/network and JSON errors: catch httpx.RequestError for network/timeout failures coming from the httpx client calls and ValueError for invalid JSON from response.json(); update the except block in the function that calls the httpx client and response.json() (look for response = client.get/... and response.json() in IssueSuggestionService/issue_suggestion_service functions) to except (httpx.RequestError, ValueError) and ensure httpx is imported at top of the module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/services/github/issue_suggestion_service.py`:
- Around line 13-14: The function parameter "limit" in issue_suggestion_service
(the signature with language: str = "python", limit: int = 5) needs an
upper-bound guard to avoid GitHub Search API 422 errors; validate and sanitize
"limit" at the start of the method (or in the constructor/path where it's used)
by clamping it to a maximum of 100 (and minimum of 1) before building
per_page/page params, and return or raise a clear error if an out-of-range value
is provided so callers know the input was adjusted/rejected rather than silently
getting an empty result.
---
Duplicate comments:
In `@backend/app/services/github/issue_suggestion_service.py`:
- Around line 27-28: The current guard using language.isalnum() silently rejects
valid language identifiers (e.g., "C++", "C#", "Objective-C", "Jupyter
Notebook") and defaults to "python"; update the validation in
issue_suggestion_service.py to accept common non-alphanumeric characters or map
known aliases instead of using isalnum(): validate/normalize the incoming
language variable (e.g., strip, lower, replace common variants) and check
against an allowlist of known language identifiers (or a regex that permits +,
#, -, and spaces) before falling back to "python", referencing the language
variable and the existing if-not-language condition to locate where to change
the logic.
---
Nitpick comments:
In `@backend/app/services/github/issue_suggestion_service.py`:
- Around line 43-44: The non-200 branch (where `response.status_code != 200`)
and the exception handler currently return [] silently; update the HTTP error
path and the exception catch in the issue suggestion routine (the function that
performs the external request in issue_suggestion_service.py) to emit a
structured WARNING-level log including the `response.status_code` (and response
body or url if available) for non-200s, and the exception type/name and message
in the except block; use the module/class logger (e.g., `logger` or
`self.logger`) so callers still return [] but operators can distinguish
429/401/network errors from genuine no-results.
- Around line 58-60: The bare except in the issue suggestion flow should be
narrowed to only handle HTTP/network and JSON errors: catch httpx.RequestError
for network/timeout failures coming from the httpx client calls and ValueError
for invalid JSON from response.json(); update the except block in the function
that calls the httpx client and response.json() (look for response =
client.get/... and response.json() in
IssueSuggestionService/issue_suggestion_service functions) to except
(httpx.RequestError, ValueError) and ensure httpx is imported at top of the
module.
Comment on lines
+13
to
+14
| language: str = "python", | ||
| limit: int = 5 |
Contributor
There was a problem hiding this comment.
limit has no upper-bound guard; values > 100 yield a silent 422 from GitHub.
GitHub's search API caps per_page at 100. A caller passing limit=200 will receive a 422 Unprocessable Entity, which is silently swallowed as [].
🛡️ Proposed fix
async def fetch_beginner_issues(
self,
language: str = "python",
- limit: int = 5
+ limit: int = 5,
) -> List[Dict]:+ limit = max(1, min(limit, 100)) # GitHub per_page cap
if not language or not language.isalnum():📝 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.
Suggested change
| language: str = "python", | |
| limit: int = 5 | |
| async def fetch_beginner_issues( | |
| self, | |
| owner: str, | |
| repo: str, | |
| language: str = "python", | |
| limit: int = 5, | |
| ) -> List[Dict]: | |
| limit = max(1, min(limit, 100)) # GitHub per_page cap | |
| if not language or not language.isalnum(): | |
| language = "python" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/github/issue_suggestion_service.py` around lines 13 -
14, The function parameter "limit" in issue_suggestion_service (the signature
with language: str = "python", limit: int = 5) needs an upper-bound guard to
avoid GitHub Search API 422 errors; validate and sanitize "limit" at the start
of the method (or in the constructor/path where it's used) by clamping it to a
maximum of 100 (and minimum of 1) before building per_page/page params, and
return or raise a clear error if an out-of-range value is provided so callers
know the input was adjusted/rejected rather than silently getting an empty
result.
Author
|
All the requested changes have been implemented.
Kindly review again. Thank you. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #285
📝 Description
This PR fixes the silent handling of 401 Unauthorized responses in the Axios response interceptor.
Previously, when an API returned a 401 error:
This update ensures proper session handling and improves production readiness.
🔧 Changes Made
signOut()on 401 response/login📷 Screenshots or Visual Changes (if applicable)
Before:
After:
/login🤝 Collaboration
Collaborated with:
N/A✅ Checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements
Chores