feat: route onboarding flow through MCP-backed GitHub toolkit #137
Conversation
WalkthroughAdds a multi-stage onboarding state machine, messaging and Discord UI; changes onboarding handler signatures and node return shapes to return rich task payloads; propagates onboarding_state through tool wrappers and AgentState; adds Discord cog/view for DMs and OAuth; and extends auth callback to optionally send a post‑verification DM. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Discord User
participant Bot as Discord Bot
participant Cog as OnboardingCog
participant Workflow as OnboardingWorkflow
participant Agent as DevRel Agent Node
participant Auth as Auth Callback
participant DM as Discord DM
User->>Bot: joins server
Bot->>Cog: on_member_join(member)
Cog->>Cog: get_or_create_user_by_discord()
Cog->>Workflow: run_onboarding_flow(state, latest_message, is_verified, github_username)
Workflow-->>Cog: OnboardingFlowResult + onboarding_state
Cog->>DM: send DM with OnboardingView (optional OAuth link)
alt User clicks "Verify" -> OAuth completes
User->>Auth: /auth/github/callback
Auth->>Auth: finalize verification
Auth->>DM: send_final_handoff_dm(user) %% (optional DM)
DM->>User: final handoff embed
else User uses View (verify/skip) or chooses actions
User->>DM: interact with OnboardingView
DM->>Workflow: (via Agent) update onboarding_state / next tool
DM->>User: updated messages / final handoff
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/app/agents/devrel/github/tools/general_github_help.py (1)
11-12: Update docstring to document the hint parameter.The function signature correctly adds the optional
hintparameter, but the docstring should be updated to explain its purpose and behavior.Apply this diff to enhance the docstring:
async def handle_general_github_help(query: str, llm, hint: Optional[str] = None) -> Dict[str, Any]: - """Execute general GitHub help with web search and LLM knowledge""" + """ + Execute general GitHub help with web search and LLM knowledge. + + Args: + query: The user's GitHub-related question + llm: The language model to use for generating responses + hint: Optional assistant hint to augment the prompt context + + Returns: + Dict containing status, response, and metadata + """backend/app/agents/devrel/onboarding/messages.py (1)
65-74: Consider sanitizing the github_username parameter for markdown safety.The
github_usernameparameter is wrapped in backticks for markdown formatting (lines 69, 92) without validation. If a username contains backticks or other markdown special characters, it could break the formatting or potentially cause injection issues in the rendered message.Consider adding a helper function to sanitize usernames before embedding them in formatted strings:
def _sanitize_username(username: str) -> str: """Escape markdown special characters in username.""" # Escape backticks and other markdown characters return username.replace("`", "\\`").replace("*", "\\*").replace("_", "\\_")Then use it when building messages:
def build_verified_welcome(github_username: Optional[str] = None) -> str: """Welcome copy for returning verified contributors.""" greeting = "👋 Welcome back to the Devr.AI community!" if github_username: - greeting += f" I see `{github_username}` is already linked, which is great." + greeting += f" I see `{_sanitize_username(github_username)}` is already linked, which is great."Also applies to: 88-98
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/app/agents/devrel/github/tools/general_github_help.py(3 hunks)backend/app/agents/devrel/nodes/handlers/onboarding.py(1 hunks)backend/app/agents/devrel/nodes/react_supervisor.py(1 hunks)backend/app/agents/devrel/onboarding/messages.py(1 hunks)backend/app/agents/devrel/onboarding/workflow.py(1 hunks)backend/app/agents/devrel/tool_wrappers.py(1 hunks)backend/app/agents/state.py(1 hunks)backend/app/api/v1/auth.py(5 hunks)backend/app/core/config/settings.py(1 hunks)backend/app/integrations/mcp/client.py(1 hunks)backend/integrations/discord/cogs.py(2 hunks)backend/integrations/discord/views.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
backend/app/agents/devrel/tool_wrappers.py (1)
backend/app/agents/devrel/nodes/react_supervisor.py (1)
add_tool_result(98-114)
backend/app/agents/devrel/github/tools/general_github_help.py (1)
backend/app/services/embedding_service/service.py (1)
llm(55-68)
backend/integrations/discord/cogs.py (5)
backend/app/agents/devrel/onboarding/messages.py (4)
build_encourage_verification_message(77-85)build_new_user_welcome(51-62)build_verified_capabilities_intro(88-98)build_verified_welcome(65-74)backend/app/services/auth/management.py (1)
get_or_create_user_by_discord(10-41)backend/app/services/auth/supabase.py (1)
login_with_github(28-30)backend/app/services/auth/verification.py (1)
create_verification_session(33-61)backend/integrations/discord/views.py (3)
OAuthView(36-49)OnboardingView(52-120)build_final_handoff_embed(11-24)
backend/app/agents/devrel/onboarding/workflow.py (2)
backend/app/agents/state.py (1)
AgentState(18-76)backend/app/agents/devrel/onboarding/messages.py (5)
build_verified_capabilities_intro(88-98)render_capabilities_text(39-48)build_new_user_welcome(51-62)build_encourage_verification_message(77-85)build_verified_welcome(65-74)
backend/integrations/discord/views.py (2)
backend/app/services/auth/management.py (1)
get_or_create_user_by_discord(10-41)backend/app/models/database/supabase.py (1)
User(7-69)
backend/app/api/v1/auth.py (6)
backend/app/database/supabase/client.py (1)
get_supabase_client(9-13)backend/app/services/auth/verification.py (2)
find_user_by_session_and_verify(63-135)get_verification_session_info(156-176)backend/app/services/github/user/profiling.py (2)
profile_user_from_github(301-333)request(77-79)backend/app/core/dependencies.py (1)
get_app_instance(7-12)backend/integrations/discord/views.py (1)
send_final_handoff_dm(27-34)backend/main.py (1)
DevRAIApplication(26-87)
backend/app/agents/devrel/nodes/handlers/onboarding.py (2)
backend/app/agents/devrel/onboarding/workflow.py (2)
OnboardingStage(12-19)run_onboarding_flow(87-276)backend/app/agents/state.py (1)
AgentState(18-76)
🪛 Ruff (0.13.3)
backend/integrations/discord/cogs.py
228-228: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
264-265: try-except-pass detected, consider logging the exception
(S110)
264-264: Do not catch blind exception: Exception
(BLE001)
280-281: try-except-pass detected, consider logging the exception
(S110)
280-280: Do not catch blind exception: Exception
(BLE001)
293-294: try-except-pass detected, consider logging the exception
(S110)
293-293: Do not catch blind exception: Exception
(BLE001)
300-300: Consider moving this statement to an else block
(TRY300)
303-303: Do not catch blind exception: Exception
(BLE001)
304-304: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
308-309: try-except-pass detected, consider logging the exception
(S110)
308-308: Do not catch blind exception: Exception
(BLE001)
backend/app/integrations/mcp/client.py
8-8: Do not catch blind exception: Exception
(BLE001)
31-31: Avoid specifying long messages outside the exception class
(TRY003)
43-43: Avoid specifying long messages outside the exception class
(TRY003)
47-47: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
47-47: Avoid specifying long messages outside the exception class
(TRY003)
50-50: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
50-50: Avoid specifying long messages outside the exception class
(TRY003)
50-50: Use explicit conversion flag
Replace with conversion flag
(RUF010)
backend/integrations/discord/views.py
32-34: try-except-pass detected, consider logging the exception
(S110)
32-32: Do not catch blind exception: Exception
(BLE001)
73-73: Unused method argument: button
(ARG002)
84-84: Do not catch blind exception: Exception
(BLE001)
102-103: try-except-pass detected, consider logging the exception
(S110)
102-102: Do not catch blind exception: Exception
(BLE001)
111-111: Unused method argument: button
(ARG002)
118-120: try-except-pass detected, consider logging the exception
(S110)
118-118: Do not catch blind exception: Exception
(BLE001)
backend/app/api/v1/auth.py
21-21: Unused function argument: request
(ARG001)
24-24: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
98-98: Do not catch blind exception: Exception
(BLE001)
backend/app/agents/devrel/nodes/handlers/onboarding.py
31-32: try-except-pass detected, consider logging the exception
(S110)
31-31: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (8)
backend/app/agents/devrel/github/tools/general_github_help.py (1)
1-1: LGTM!The
Optionalimport is correctly added to support the newhintparameter's type annotation.backend/app/agents/state.py (1)
29-31: Onboarding state persistence looks good.Defaulting to an empty dict matches the rest of the state model and cleanly unlocks the new workflow.
backend/app/core/config/settings.py (1)
42-48: Configuration surface extension is consistent.The new fields follow existing conventions, so wiring them through env vars should be seamless.
backend/app/agents/devrel/tool_wrappers.py (1)
33-44: Onboarding wrapper propagation looks solid.State updates now carry onboarding_state and force the next tool when present—nice alignment with the supervisor shortcut.
backend/app/agents/devrel/nodes/react_supervisor.py (1)
20-37: Force-next-tool shortcut is clean.The auto-route path removes the flag, records the decision, and bumps the iteration counter—exactly what the flow needs.
backend/app/agents/devrel/onboarding/messages.py (3)
39-48: LGTM!The capability text rendering logic is clear and produces well-formatted output. The function correctly iterates through sections and formats examples with proper spacing.
12-12: Verify repository name: ensure the hardcoded'Devr.AI-backend'matches the actual GitHub repository’s name (including exact capitalization, punctuation, and hyphenation) to avoid confusion.
58-60: Referenced Discord commands exist and are registered
All commands (/verify_github, /verification_status, /help) are defined in backend/integrations/discord/cogs.py.
| if hint: | ||
| help_prompt = GENERAL_GITHUB_HELP_PROMPT.format( | ||
| query=f"{query}\n\nAssistant hint: {hint}", | ||
| search_context=search_context | ||
| ) | ||
| else: | ||
| help_prompt = GENERAL_GITHUB_HELP_PROMPT.format( | ||
| query=query, | ||
| search_context=search_context | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Eliminate code duplication in prompt construction.
The GENERAL_GITHUB_HELP_PROMPT.format() call is duplicated in both branches, violating the DRY principle. Construct the augmented query first, then format once.
Apply this diff to refactor the conditional logic:
- if hint:
- help_prompt = GENERAL_GITHUB_HELP_PROMPT.format(
- query=f"{query}\n\nAssistant hint: {hint}",
- search_context=search_context
- )
- else:
- help_prompt = GENERAL_GITHUB_HELP_PROMPT.format(
- query=query,
- search_context=search_context
- )
+ augmented_query = f"{query}\n\nAssistant hint: {hint}" if hint else query
+ help_prompt = GENERAL_GITHUB_HELP_PROMPT.format(
+ query=augmented_query,
+ search_context=search_context
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if hint: | |
| help_prompt = GENERAL_GITHUB_HELP_PROMPT.format( | |
| query=f"{query}\n\nAssistant hint: {hint}", | |
| search_context=search_context | |
| ) | |
| else: | |
| help_prompt = GENERAL_GITHUB_HELP_PROMPT.format( | |
| query=query, | |
| search_context=search_context | |
| ) | |
| augmented_query = f"{query}\n\nAssistant hint: {hint}" if hint else query | |
| help_prompt = GENERAL_GITHUB_HELP_PROMPT.format( | |
| query=augmented_query, | |
| search_context=search_context | |
| ) |
🤖 Prompt for AI Agents
In backend/app/agents/devrel/github/tools/general_github_help.py around lines 26
to 35, the GENERAL_GITHUB_HELP_PROMPT.format() call is duplicated; refactor by
first building an augmented query string (include the hint when present, e.g.,
augmented_query = f"{query}\n\nAssistant hint: {hint}" if hint else query) and
then call GENERAL_GITHUB_HELP_PROMPT.format(query=augmented_query,
search_context=search_context) once to eliminate duplication.
| async with aiohttp.ClientSession() as session: | ||
| try: | ||
| async with session.post(self.server_url, headers=headers, json=payload, timeout=timeout) as resp: | ||
| text = await resp.text() | ||
| if resp.status >= 400: | ||
| raise MCPClientError(f"MCP HTTP {resp.status}: {text}") | ||
| try: | ||
| data = json.loads(text) | ||
| except json.JSONDecodeError: | ||
| raise MCPClientError("MCP returned invalid JSON") | ||
| return data | ||
| except asyncio.TimeoutError as e: | ||
| raise MCPClientError(f"MCP timeout: {str(e)}") |
There was a problem hiding this comment.
Wrap aiohttp transport errors in MCPClientError.
Right now any aiohttp.ClientError (DNS failure, refused connection, etc.) bubbles up directly, so callers stop seeing the unified MCPClientError your docstring implies. Please catch those and re-raise as MCPClientError (ideally using raise … from e) to keep error handling consistent.
Apply this diff:
async with aiohttp.ClientSession() as session:
try:
async with session.post(self.server_url, headers=headers, json=payload, timeout=timeout) as resp:
text = await resp.text()
if resp.status >= 400:
raise MCPClientError(f"MCP HTTP {resp.status}: {text}")
try:
data = json.loads(text)
except json.JSONDecodeError as e:
- raise MCPClientError("MCP returned invalid JSON")
+ raise MCPClientError("MCP returned invalid JSON") from e
return data
- except asyncio.TimeoutError as e:
- raise MCPClientError(f"MCP timeout: {str(e)}")
+ except asyncio.TimeoutError as e:
+ raise MCPClientError(f"MCP timeout: {e}") from e
+ except aiohttp.ClientError as e:
+ raise MCPClientError(f"MCP transport error: {e}") from eCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.13.3)
43-43: Avoid specifying long messages outside the exception class
(TRY003)
47-47: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
47-47: Avoid specifying long messages outside the exception class
(TRY003)
50-50: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
50-50: Avoid specifying long messages outside the exception class
(TRY003)
50-50: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In backend/app/integrations/mcp/client.py around lines 38 to 50, transport-level
aiohttp.ClientError exceptions (DNS failures, connection refused, etc.) are not
being caught and thus escape as raw aiohttp errors; catch aiohttp.ClientError in
the existing try/except block (or add an except clause) and re-raise as an
MCPClientError using "raise MCPClientError(... ) from e" so callers always
receive MCPClientError while preserving the original error context.
it was committed by mistake ,i was experimenting with MCPs
|
Any updates @Dharya4242 ? |
|
Hey @smokeyScraper |
|
cool man, actually i was preferring to close all the current issues by working on them myself, but you can give it a try and align it as per your way and then can share your branch's access to me as a collaborator so that I can make commits on top of yours if I feel like a few changes over yours. This way you too would learn from your mistakes in a better way (if any) as I'd be correcting them out. Thanks! |
|
got it , sounds great. Really appreciate the opportunity to learn and your guidance |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
backend/app/agents/devrel/nodes/gather_context.py (2)
28-46: Discord avatar URL construction, PII logging, and exception scope
author['avatar']is a hash, not a URL. Build the CDN URL when only a hash is available.- Redact
discord_idin logs.- Consider narrowing the catch or documenting why a broad catch is acceptable (to address BLE001), and optionally throttle DB calls if profile is already fresh.
- avatar_url = author.get("avatar") or author.get("avatar_url") + avatar_hash = author.get("avatar") + avatar_url = author.get("avatar_url") + if not avatar_url and avatar_hash and discord_id: + # Discord CDN avatar URL format; default to 256px + avatar_url = f"https://cdn.discordapp.com/avatars/{discord_id}/{avatar_hash}.png?size=256" @@ - if discord_id: - try: + if discord_id: + try: user = await get_or_create_user_by_discord( @@ ) profile_data = user.model_dump() - except Exception as exc: # pragma: no cover - graceful degradation - logger.warning("Failed to refresh Discord user profile for %s: %s", discord_id, exc) + except Exception as exc: # noqa: BLE001 # Graceful degradation; service raises heterogeneous errors + redacted = str(discord_id) + if len(redacted) > 6: + redacted = f"{redacted[:2]}***{redacted[-2:]}" + logger.warning("Failed to refresh Discord user profile for id=%s: %s", redacted, exc)Optional (future): cache a
last_profile_refreshtimestamp instate.contextand skip refresh within N minutes to reduce DB load.
55-63: Minor: timezones for last_interaction_time (optional)If/when the codebase moves to TZ-aware datetimes, switch to
datetime.now(tz=timezone.utc)for consistency; current usage matches AgentState defaults.backend/app/agents/devrel/onboarding/workflow.py (4)
39-46: Broaden intent detection to cover common phrasingsImprove coverage for “verify/verification/gh” and skip phrasings.
-_INTENT_VERIFIED = re.compile(r"\b(i\s*(have)?\s*)?(linked|connected|verified)\b.*github", re.IGNORECASE) +_INTENT_VERIFIED = re.compile(r"\b(i\s*(have|am)?\s*)?(verify|verified|link(?:ed)?|connect(?:ed)?)\b.*\b(github|gh)\b", re.IGNORECASE) -_INTENT_SKIP = re.compile(r"\b(skip|later|not\s+now)\b", re.IGNORECASE) +_INTENT_SKIP = re.compile(r"\b(skip|later|not\s+now|skip\s+for\s+now)\b", re.IGNORECASE) -_INTENT_HELP = re.compile(r"\b(how|help|can't|cannot|stuck)\b.*verify", re.IGNORECASE) +_INTENT_HELP = re.compile(r"\b(how|help|can.?t|cannot|stuck)\b.*verif(y|ication|y(ing)?)", re.IGNORECASE)
107-127: DRY up verified-capabilities/completed branchesFour nearly identical blocks construct verified responses. Extract a small helper to build the result to reduce maintenance risk.
+def _verified_response(github_username: Optional[str], capability_sections) -> OnboardingFlowResult: + return OnboardingFlowResult( + stage=OnboardingStage.VERIFIED_CAPABILITIES, + status="completed", + welcome_message=messages.build_verified_capabilities_intro(github_username), + final_message=messages.render_capabilities_text(), + actions=_exploration_suggestions(), + is_verified=True, + capability_sections=capability_sections, + route_hint="onboarding", + handoff="github_toolkit", + next_tool="github_toolkit", + ) @@ - intro = messages.build_verified_capabilities_intro(github_username) - return ( - OnboardingFlowResult( - stage=OnboardingStage.VERIFIED_CAPABILITIES, - status="completed", - welcome_message=intro, - final_message=messages.render_capabilities_text(), - actions=_exploration_suggestions(), - is_verified=True, - capability_sections=capability_sections, - route_hint="onboarding", - handoff="github_toolkit", - next_tool="github_toolkit", - ), - onboarding_state, - ) + return (_verified_response(github_username, capability_sections), onboarding_state)Apply similarly to the other verified transitions.
Also applies to: 145-165, 204-224, 245-261
226-241: Cap reminders to avoid unbounded growthConsider a
max_reminderscap (e.g., 3) and stop incrementing, to prevent ever-growing counters in persisted state.- onboarding_state["reminders_sent"] = reminders_sent + 1 + max_reminders = 3 + onboarding_state["reminders_sent"] = min(reminders_sent + 1, max_reminders)
22-37: Consider addingto_dict()as defensive serialization, though current handler already convertsstage.valueVerification shows the handler at
backend/app/agents/devrel/nodes/handlers/onboarding.py:43already convertsflow_result.stage.valueto string. The serialization concern is already addressed. Addingto_dict()to the dataclass would be a defensive measure to prevent future bugs if the handler pattern changes or ifOnboardingFlowResultis used elsewhere without explicit serialization. Current code is safe as-is.backend/app/agents/devrel/tool_wrappers.py (1)
33-46: Gate complete_after_forced_tool to supported toolThe stage value is already serialized as a string (
.valueinhandle_onboarding_node), so the type check is safe. However, scopecomplete_after_forced_toolto the specific tool where it's cleared to avoid leaving stale routing flags. Only set it whennext_tool == "github_toolkit".next_tool = tool_result.get("next_tool") if next_tool: context["force_next_tool"] = next_tool - if tool_result.get("stage") in {"verified_capabilities", "completed"}: - context["complete_after_forced_tool"] = next_tool + if next_tool == "github_toolkit" and tool_result.get("stage") in {"verified_capabilities", "completed"}: + context["complete_after_forced_tool"] = next_tool
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/agents/devrel/github/tools/general_github_help.py(1 hunks)backend/app/agents/devrel/nodes/gather_context.py(2 hunks)backend/app/agents/devrel/nodes/react_supervisor.py(1 hunks)backend/app/agents/devrel/onboarding/workflow.py(1 hunks)backend/app/agents/devrel/tool_wrappers.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/agents/devrel/nodes/react_supervisor.py
- backend/app/agents/devrel/github/tools/general_github_help.py
🧰 Additional context used
🧬 Code graph analysis (3)
backend/app/agents/devrel/tool_wrappers.py (1)
backend/app/agents/devrel/nodes/react_supervisor.py (1)
add_tool_result(114-130)
backend/app/agents/devrel/nodes/gather_context.py (2)
backend/app/agents/state.py (1)
AgentState(18-76)backend/app/services/auth/management.py (1)
get_or_create_user_by_discord(10-41)
backend/app/agents/devrel/onboarding/workflow.py (2)
backend/app/agents/state.py (1)
AgentState(18-76)backend/app/agents/devrel/onboarding/messages.py (5)
build_verified_capabilities_intro(88-98)render_capabilities_text(39-48)build_new_user_welcome(51-62)build_encourage_verification_message(77-85)build_verified_welcome(65-74)
🪛 Ruff (0.14.0)
backend/app/agents/devrel/nodes/gather_context.py
44-44: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (3)
backend/app/agents/devrel/nodes/gather_context.py (2)
26-27: Good: initialize profile_data from persisted stateKeeps prior profile fields when refresh fails.
48-48: Context default for user_profile is sensibleFallback ensures downstream code always has minimal profile metadata.
backend/app/agents/devrel/tool_wrappers.py (1)
70-78: Verify whether the additional clearing is necessary given existing supervisor-level logicThe script output shows
force_next_toolis already being cleared inreact_supervisor.py:31after the supervisor processes a forced action. The suggested addition intool_wrappers.pywould clear it earlier, but the existing mechanism in the supervisor node already handles removal.To confirm if the suggested refactor is necessary or if the current flow already prevents re-routing loops, verify:
- Whether the supervisor clears
force_next_toolbefore re-evaluating on the next iteration (confirming the existing logic is sufficient), or- Whether there's a race condition where
force_next_toolpersists long enough to cause unwanted re-routing despite the completion flag being setWithout tracing the full state machine execution order between tool completion and supervisor re-evaluation, the necessity of the refactor is unclear.
|
Could you please resolve the merge conflicts @Dharya4242 ? |
|
Sure @smokeyScraper , I will resolve the merge conflicts and update the PR shortly |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
backend/integrations/discord/cogs.py (1)
411-428: Consider simplifying embed construction.The string manipulation to extract and reformat the welcome message is somewhat fragile—it assumes the welcome text starts with "👋 " and splits on "\n\n". If
build_new_user_welcome()changes format, this parsing could break.Consider either:
- Refactoring
build_new_user_welcome()to return structured data (title, paragraphs) instead of raw text, or- Making this parsing more defensive with fallbacks if the expected format isn't found.
For example:
def _build_welcome_embed(self, member: discord.abc.User) -> discord.Embed: welcome_text = build_new_user_welcome() - first_paragraph, _, remainder = welcome_text.partition("\n\n") - if first_paragraph.startswith("👋 "): - first_paragraph = first_paragraph[2:] - - description_blocks = [first_paragraph] - if remainder: - description_blocks.append(remainder.strip()) + # Remove emoji prefix if present + text = welcome_text.lstrip("👋 ").strip() + # Split into paragraphs, filter empty ones + paragraphs = [p.strip() for p in text.split("\n\n") if p.strip()] embed = discord.Embed( title="Welcome to Devr.AI! 👋", - description="\n\n".join(description_blocks).strip(), + description="\n\n".join(paragraphs) if paragraphs else text, color=discord.Color.blue(), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/agents/state.py(1 hunks)backend/app/core/config/settings.py(1 hunks)backend/integrations/discord/cogs.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/integrations/discord/cogs.py (5)
backend/app/agents/devrel/onboarding/messages.py (4)
build_encourage_verification_message(77-85)build_new_user_welcome(51-62)build_verified_capabilities_intro(88-98)build_verified_welcome(65-74)backend/app/services/auth/management.py (1)
get_or_create_user_by_discord(10-41)backend/app/services/auth/supabase.py (1)
login_with_github(28-30)backend/app/services/auth/verification.py (1)
create_verification_session(33-61)backend/integrations/discord/views.py (3)
OAuthView(36-49)OnboardingView(52-120)build_final_handoff_embed(11-24)
🪛 Ruff (0.14.1)
backend/integrations/discord/cogs.py
440-440: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
476-477: try-except-pass detected, consider logging the exception
(S110)
476-476: Do not catch blind exception: Exception
(BLE001)
492-493: try-except-pass detected, consider logging the exception
(S110)
492-492: Do not catch blind exception: Exception
(BLE001)
505-506: try-except-pass detected, consider logging the exception
(S110)
505-505: Do not catch blind exception: Exception
(BLE001)
512-512: Consider moving this statement to an else block
(TRY300)
515-515: Do not catch blind exception: Exception
(BLE001)
516-516: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
520-521: try-except-pass detected, consider logging the exception
(S110)
520-520: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (4)
backend/app/core/config/settings.py (1)
42-43: LGTM: Onboarding toggle added.The
onboarding_show_oauth_buttonflag provides a clean way to control OAuth button visibility in the onboarding flow, with a sensible default.backend/app/agents/state.py (1)
29-30: LGTM: Onboarding state field added.The
onboarding_statefield is properly typed and usesdefault_factory=dictto avoid mutable default pitfalls. This provides a clean way to track per-channel onboarding progress.backend/integrations/discord/cogs.py (2)
1-21: LGTM: Onboarding integration wired properly.The imports are well-organized and the
OnboardingCogis correctly registered in thesetupfunction alongside the existingDevRelCommandscog.Also applies to: 399-403
445-448: LGTM: Clean event listener delegation.The
on_member_joinhandler properly delegates to the shared_send_onboarding_flowmethod, avoiding code duplication.
| except discord.Forbidden: | ||
| return "dm_forbidden" | ||
| except Exception as e: | ||
| logger.error(f"onboarding flow error: {e}") | ||
| try: | ||
| await user.send("I hit an error. You can still run /verify_github and /help.") | ||
| await user.send(build_encourage_verification_message(reminder_count=1)) | ||
| except Exception: | ||
| pass | ||
| return "error" |
There was a problem hiding this comment.
Improve exception logging and handling.
Two issues with the exception handling:
- Line 516 uses
logging.errorinstead oflogging.exception, which omits the traceback and makes debugging harder. - Lines 520-521 silently swallow exceptions when attempting to send error DMs.
As per static analysis
Apply this diff:
except discord.Forbidden:
return "dm_forbidden"
except Exception as e:
- logger.error(f"onboarding flow error: {e}")
+ logger.exception(f"onboarding flow error: {e}")
try:
await user.send("I hit an error. You can still run /verify_github and /help.")
await user.send(build_encourage_verification_message(reminder_count=1))
- except Exception:
- pass
+ except discord.Forbidden:
+ logger.warning(f"Cannot DM user {user.id} after onboarding error (DMs disabled)")
+ except Exception as send_error:
+ logger.exception(f"Failed to send error fallback DM to user {user.id}: {send_error}")
return "error"🧰 Tools
🪛 Ruff (0.14.1)
515-515: Do not catch blind exception: Exception
(BLE001)
516-516: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
520-521: try-except-pass detected, consider logging the exception
(S110)
520-520: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In backend/integrations/discord/cogs.py around lines 513 to 522, replace the
logger.error call with logger.exception so the traceback is included when the
onboarding flow hits an unexpected Exception, and update the inner except that
currently does a silent pass to instead catch the exception as e and log it
(e.g., logger.warning or logger.exception) with context about failing to send
the DM before continuing to return "error"; keep behavior of ignoring
discord.Forbidden earlier intact.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/core/config/settings.py(2 hunks)backend/integrations/discord/cogs.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/integrations/discord/cogs.py (5)
backend/app/agents/devrel/onboarding/messages.py (4)
build_encourage_verification_message(77-85)build_new_user_welcome(51-62)build_verified_capabilities_intro(88-98)build_verified_welcome(65-74)backend/app/services/auth/management.py (1)
get_or_create_user_by_discord(10-41)backend/app/services/auth/supabase.py (1)
login_with_github(28-30)backend/app/services/auth/verification.py (1)
create_verification_session(33-61)backend/integrations/discord/views.py (3)
OAuthView(36-49)OnboardingView(52-120)build_final_handoff_embed(11-24)
🪛 Ruff (0.14.1)
backend/integrations/discord/cogs.py
440-440: Expected an expression or a '}'
(invalid-syntax)
backend/app/core/config/settings.py
49-49: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (16)
backend/app/core/config/settings.py (3)
3-3: LGTM: Import formatting improved.Adding a space after the comma follows PEP 8 style conventions.
52-55: LGTM: Model config formatting enhanced.The multi-line formatting of
model_configimproves readability without changing behavior.
42-44: Field is properly consumed—verification passed.The script confirms
onboarding_show_oauth_buttonis actively used inbackend/integrations/discord/cogs.py(line 483) to control OAuth button visibility during onboarding. The implementation correctly retrieves the setting with a sensible default fallback. Unlike themcp_api_keyconcern, this field serves its intended purpose in the Discord integration flow.backend/integrations/discord/cogs.py (13)
1-21: LGTM: Imports are well-organized.The new imports for onboarding messages, authentication, and Discord views are appropriately structured and align with the onboarding flow implementation.
399-403: LGTM: Cog registration follows established pattern.The OnboardingCog is properly registered alongside DevRelCommands in the setup function.
405-409: LGTM: Clean cog initialization.The OnboardingCog follows the standard pattern for Discord cog initialization.
411-428: LGTM: Welcome embed construction is clear.The method appropriately extracts and formats the welcome message, removing the emoji prefix to avoid duplication with the embed title.
445-448: LGTM: Event listener properly delegates to shared flow.The
on_member_joinlistener appropriately delegates to the shared_send_onboarding_flowmethod.
450-461: LGTM: Comprehensive documentation.The method signature and docstring clearly document the onboarding flow's purpose and return values.
462-468: LGTM: Defensive attribute access.The use of
getattrwith defaults appropriately handles variations in Discord user object attributes.
470-480: LGTM: Verified user handling with proper logging.The code now includes appropriate logging for DM failures, addressing the previous review concern. The exception handling correctly distinguishes between permission issues and other failures.
482-485: LGTM: Defensive settings access.The use of
getattrwith a default value appropriately handles cases where theonboarding_show_oauth_buttonsetting might not be configured.
486-498: LGTM: Session failure handling with proper logging.The fallback path for session creation failures now includes appropriate logging, addressing the previous review concern.
500-513: LGTM: OAuth URL failure handling with proper logging.The fallback path for auth URL generation failures now includes appropriate logging, addressing the previous review concern.
515-518: LGTM: Welcome DM sent with appropriate view.The code correctly sends the welcome embed with the OnboardingView, handling the optional auth_url parameter.
519-530: LGTM: Comprehensive exception handling with proper logging.The exception handling now uses
logger.exceptionto capture tracebacks and includes logging for fallback DM failures, addressing all previous review concerns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/integrations/discord/cogs.py (2)
476-479: Remove redundant exception info from logger.exception.The
logger.exceptioncall automatically includes exception details and traceback, so appending: {e}to the message is redundant.Apply this diff:
except Exception as e: - logger.exception(f"Failed to send verified welcome DM to user {user.id}: {e}") + logger.exception(f"Failed to send verified welcome DM to user {user.id}")As per static analysis
482-530: Optional: Clean up exception logging and control flow.The exception handling is comprehensive and correct, but there are a few minor style improvements that would align with best practices:
Redundant exception info in
logger.exceptioncalls (lines 497, 512, 522, 529): Thelogger.exceptionmethod automatically includes exception details and traceback, so appending: {e}or: {send_error}to the message is redundant.Return statement placement (line 518): The return statement could be moved to an
elseblock for cleaner control flow structure.Apply this diff to address these style points:
except Exception as e: - logger.exception(f"Failed to send session failure fallback DM to user {user.id}: {e}") + logger.exception(f"Failed to send session failure fallback DM to user {user.id}") return "session_unavailable" # Generate GitHub OAuth URL via Supabase callback_url = f"{settings.backend_url}/v1/auth/callback?session={session_id}" auth = await login_with_github(redirect_to=callback_url) auth_url = auth.get("url") if not auth_url: try: await user.send("Couldn't generate a verification link. Please use /verify_github.") await user.send(build_encourage_verification_message(reminder_count=1)) await user.send(embed=build_final_handoff_embed()) except discord.Forbidden: logger.warning(f"Cannot DM user {user.id} after auth URL failure (DMs disabled)") except Exception as e: - logger.exception(f"Failed to send auth failure fallback DM to user {user.id}: {e}") + logger.exception(f"Failed to send auth failure fallback DM to user {user.id}") return "auth_unavailable" # Send welcome DM with actions (auth_url may be None when button disabled) embed = self._build_welcome_embed(user) await user.send(embed=embed, view=OnboardingView(auth_url)) return "onboarding_sent" - except discord.Forbidden: + except discord.Forbidden: return "dm_forbidden" - except Exception as e: + except Exception: - logger.exception(f"onboarding flow error: {e}") + logger.exception("onboarding flow error") try: await user.send("I hit an error. You can still run /verify_github and /help.") await user.send(build_encourage_verification_message(reminder_count=1)) except discord.Forbidden: logger.warning(f"Cannot DM user {user.id} after onboarding error (DMs disabled)") except Exception as send_error: - logger.exception(f"Failed to send error fallback DM to user {user.id}: {send_error}") + logger.exception(f"Failed to send error fallback DM to user {user.id}") return "error"As per static analysis
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/integrations/discord/cogs.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/integrations/discord/cogs.py (5)
backend/app/agents/devrel/onboarding/messages.py (4)
build_encourage_verification_message(77-85)build_new_user_welcome(51-62)build_verified_capabilities_intro(88-98)build_verified_welcome(65-74)backend/app/services/auth/management.py (1)
get_or_create_user_by_discord(10-41)backend/app/services/auth/supabase.py (1)
login_with_github(28-30)backend/app/services/auth/verification.py (1)
create_verification_session(33-61)backend/integrations/discord/views.py (3)
OAuthView(36-49)OnboardingView(52-120)build_final_handoff_embed(11-24)
🪛 Ruff (0.14.1)
backend/integrations/discord/cogs.py
479-479: Redundant exception object included in logging.exception call
(TRY401)
497-497: Redundant exception object included in logging.exception call
(TRY401)
512-512: Redundant exception object included in logging.exception call
(TRY401)
518-518: Consider moving this statement to an else block
(TRY300)
522-522: Redundant exception object included in logging.exception call
(TRY401)
529-529: Redundant exception object included in logging.exception call
(TRY401)
🔇 Additional comments (3)
backend/integrations/discord/cogs.py (3)
1-21: LGTM!The imports are well-organized and the setup function correctly registers the new OnboardingCog alongside the existing DevRelCommands cog.
Also applies to: 399-403
405-428: LGTM!The class initialization is straightforward, and the
_build_welcome_embedhelper cleanly formats the welcome message into a Discord embed with appropriate styling and metadata.
430-448: LGTM!The test command provides useful feedback for each flow outcome, and the member join listener correctly delegates to the shared flow method. Good separation of concerns.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/agents/devrel/nodes/gather_context.py (1)
71-79: Missing top-level user_profile in early return causes inconsistent state updatesEarly-return path doesn’t update AgentState.user_profile; only context.user_profile is set. Add it here for parity with the non-DB path.
if prev_context: logger.info(f"Retrieved previous conversation context from database") context_data["previous_conversation"] = prev_context # Populate state with previous conversation summary and topics return { "messages": [new_message], "context": {**state.context, **context_data}, "conversation_summary": prev_context.get("conversation_summary"), "key_topics": prev_context.get("key_topics", []), "current_task": "context_gathered", - "last_interaction_time": datetime.now() + "last_interaction_time": datetime.now(), + "user_profile": profile_data, }
🧹 Nitpick comments (5)
backend/app/agents/devrel/nodes/gather_context.py (5)
38-41: Normalize Discord avatar hash to a full CDN URLauthor["avatar"] can be a hash; persist a usable URL.
- avatar_url = author.get("avatar") or author.get("avatar_url") + avatar = author.get("avatar") + avatar_url = author.get("avatar_url") + # Normalize to CDN URL if only a hash is provided + if not avatar_url and avatar and discord_id: + ext = "gif" if str(avatar).startswith("a_") else "png" + avatar_url = f"https://cdn.discordapp.com/avatars/{discord_id}/{avatar}.{ext}"
27-31: Use timezone-aware UTC timestampsPrefer UTC for consistency across services.
-from datetime import datetime +from datetime import datetime, timezone @@ - "timestamp": datetime.now().isoformat() + "timestamp": datetime.now(timezone.utc).isoformat() @@ - "last_interaction_time": datetime.now(), + "last_interaction_time": datetime.now(timezone.utc), @@ - "last_interaction_time": datetime.now(), + "last_interaction_time": datetime.now(timezone.utc),Also applies to: 78-79, 93-94
62-62: Gate DB fetch explicitly on NoneAvoid treating empty strings as absent summaries.
- should_fetch_from_db = not state.conversation_summary and not state.key_topics + should_fetch_from_db = state.conversation_summary is None and not state.key_topics
55-55: Name might mislead: conversation_context is a countIf this is a turn count, consider renaming to message_count or turn_index for clarity. No functional change.
35-37: Minor: unify author extractionYou already have author_info earlier; consider reusing it to avoid divergence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/agents/devrel/nodes/gather_context.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/agents/devrel/nodes/gather_context.py (2)
backend/app/agents/state.py (1)
AgentState(18-77)backend/app/services/auth/management.py (1)
get_or_create_user_by_discord(10-41)
🪛 Ruff (0.14.1)
backend/app/agents/devrel/nodes/gather_context.py
51-51: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (3)
backend/app/agents/devrel/nodes/gather_context.py (3)
3-7: Imports: OKTyping and service import additions look correct and scoped.
89-99: Overall shape and delta semantics look goodmessages is additive; context merges; top-level user_profile returned on non-DB path. With the early-return fix above, this is consistent.
Please run the unit/integration tests covering both branches (DB hit vs. miss) to confirm user_profile updates in state.
19-25: Remove review comment; both calls query the same users table and use identical user_id lookupsThe review comment incorrectly assumes the functions operate on different tables. Both
ensure_user_existsandget_or_create_user_by_discordquery the sameuserstable (confirmed by schema:discord_id TEXT UNIQUE).More importantly,
gather_context_nodeis the workflow entry point, sostate.contextis empty when it executes. This means both database calls use the identicalstate.user_idfor lookups—not differing values. The first call ensures the user exists, and the second call retrieves the same record. No duplication risk exists; the UNIQUE constraint ondiscord_idfurther prevents any accidental duplicates.Likely an incorrect or invalid review comment.
| except Exception as exc: # pragma: no cover - graceful degradation | ||
| logger.warning("Failed to refresh Discord user profile for %s: %s", discord_id, exc) | ||
|
|
There was a problem hiding this comment.
Don’t swallow asyncio.CancelledError; narrow the exception handling
Catching Exception will absorb cancellations in async tasks. Re-raise CancelledError and keep graceful degradation for others.
+import asyncio
@@
- except Exception as exc: # pragma: no cover - graceful degradation
- logger.warning("Failed to refresh Discord user profile for %s: %s", discord_id, exc)
+ except asyncio.CancelledError:
+ raise
+ except Exception as exc: # pragma: no cover - graceful degradation
+ logger.warning("Failed to refresh Discord user profile", extra={"discord_id": str(discord_id)}, exc_info=exc)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as exc: # pragma: no cover - graceful degradation | |
| logger.warning("Failed to refresh Discord user profile for %s: %s", discord_id, exc) | |
| except asyncio.CancelledError: | |
| raise | |
| except Exception as exc: # pragma: no cover - graceful degradation | |
| logger.warning("Failed to refresh Discord user profile", extra={"discord_id": str(discord_id)}, exc_info=exc) |
🧰 Tools
🪛 Ruff (0.14.1)
51-51: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In backend/app/agents/devrel/nodes/gather_context.py around lines 51 to 53, the
current broad except Exception block will swallow asyncio.CancelledError; modify
the handler to re-raise CancelledError while keeping graceful degradation for
other exceptions — import asyncio if not present, change the exception handling
to either explicitly catch asyncio.CancelledError and re-raise it, or inspect
the caught exception and re-raise if isinstance(exc, asyncio.CancelledError),
and then log other exceptions as before.
| ) | ||
| profile_data = user.model_dump() | ||
| except Exception as exc: # pragma: no cover - graceful degradation | ||
| logger.warning("Failed to refresh Discord user profile for %s: %s", discord_id, exc) |
There was a problem hiding this comment.
PII in logs: redact or move to structured fields
Avoid emitting raw discord_id in message text; keep it in extras and rely on log sinks for filtering.
- logger.warning("Failed to refresh Discord user profile for %s: %s", discord_id, exc)
+ logger.warning("Failed to refresh Discord user profile", extra={"discord_id": str(discord_id)}, exc_info=exc)🤖 Prompt for AI Agents
In backend/app/agents/devrel/nodes/gather_context.py around line 52, the
logger.warning call embeds the raw discord_id into the message text; instead
remove the discord_id from the formatted message and pass it as a structured
field (e.g., via the logger's extra/context parameter) or redact/mask it if you
must include it in the message, and include the exception via exc_info or a
separate field so the log message reads a generic error and the PII is stored
only in structured metadata handled by the log sink.
smokeyScraper
left a comment
smokeyScraper
left a comment
There was a problem hiding this comment.
Very well aligned! Thanks @Dharya4242
|
Merged! |
|
Great! This was my first open-source contribution ,really enjoyed it and learned a lot. Thanks for the guidance 😄 |
2 participants
Closes #116
📝 Description
Graph-based onboarding flow that branches on GitHub verification and routes exploration into the GitHub toolkit (MCP-backed). Centralizes onboarding copy, persists minimal onboarding state, and aligns Discord DM onboarding with the same messages and stages.
Goals:
github_toolkitso MCP-powered repo/org queries answer user prompts.🔧 Changes Made
Onboarding state machine and copy
backend/app/agents/devrel/onboarding/messages.pyintro,awaiting_choice,encourage_verification,verified_capabilities,completed):backend/app/agents/devrel/onboarding/workflow.pynext_tool, andcapability_sections:backend/app/agents/devrel/nodes/handlers/onboarding.pyState + routing integration
onboarding_stateinAgentState:backend/app/agents/state.pyforce_next_tool:backend/app/agents/devrel/tool_wrappers.pyforce_next_toolto jump straight togithub_toolkit:backend/app/agents/devrel/nodes/react_supervisor.pyDiscord UX alignment
Reuse shared messages; verified users get capability intro + final handoff:
backend/integrations/discord/cogs.pyCapability embed mirrors shared sections; add “I’ve verified” button to re-check linkage:
backend/integrations/discord/views.pyOnboarding now hands off to
github_toolkit, which integrates with the MCP-backed GitHub service viagithub_support.🤝 Collaboration
Guidance from: @smokeyScraper
✅ Checklist
Summary by CodeRabbit
New Features
Enhancements
Chores