feat: add onboarding Discord and email raw tools#57
Conversation
Add 9 new onboarding tools that proxy to the member-onboarding service API for Discord and email operations: Discord (onboarding_tools_discord_*): - get_config, list_roles, find_role, find_user, check_user_role, assign_role Email (onboarding_tools_email_*): - list_templates, render_template, send Rename onboarding_list_memberships to onboarding_guided_list_memberships to distinguish guided-flow tools from raw tools. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Joan Reyero <joan@reyero.io>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Pull request overview
Adds new “raw” onboarding tool surface area for Discord and email by proxying requests through the member-onboarding service, and renames the existing memberships tool to a “guided” namespace to separate future guided vs raw functionality.
Changes:
- Renames
onboarding_list_membershipstoonboarding_guided_list_memberships(tool name + registration). - Adds Discord onboarding tools for config lookup, role/user discovery, role checks, and role assignment (service-proxied).
- Adds email onboarding tools for listing templates, rendering previews, and sending emails (service-proxied).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/tools/onboarding.go | Renames the guided memberships tool registration and updates related comments. |
| internal/tools/onboarding_tools_discord.go | Introduces Discord “raw tools” that proxy to /member-onboarding/tools/discord/.... |
| internal/tools/onboarding_tools_email.go | Introduces email “raw tools” that proxy to /member-onboarding/tools/email/.... |
| cmd/lfx-mcp-server/main.go | Enables the renamed tool and adds the new tools to defaults + server registration. |
Comments suppressed due to low confidence (1)
internal/tools/onboarding.go:49
- The tool was renamed to
onboarding_guided_list_memberships, but the associated Go identifiers remain generic (OnboardingListMembershipsArgs,handleOnboardingListMemberships). If a non-guided/raw memberships tool is added later (as implied by the new namespace split), these names will collide conceptually and make the code harder to navigate. Consider renaming the args/handler to includeGuidedfor clarity.
// OnboardingListMembershipsArgs defines the input for onboarding_guided_list_memberships.
type OnboardingListMembershipsArgs struct {
ProjectSlug string `json:"project_slug" jsonschema:"Project slug from search_projects (e.g. 'agentic-ai-foundation')"`
Status string `json:"status,omitempty" jsonschema:"Filter by status,enum=all,enum=pending,enum=in_progress,enum=closed"`
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rr-5gcm-pp58 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Joan Reyero <joan@reyero.io>
Align all tool names with verb-first naming used by the rest of the
codebase and the GitHub MCP server:
- onboarding_tools_discord_* → {verb}_onboarding_discord_*
- onboarding_tools_email_* → {verb}_onboarding_email_*
- onboarding_guided_list_memberships → list_membership_actions
- lfx_lens_query → query_lfx_lens
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Joan Reyero <joan@reyero.io>
…ctive - PathEscape user_id and role_id in check_user_role to prevent path injection - Set DestructiveHint to true on send_onboarding_email (irreversible) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Joan Reyero <joan@reyero.io>
…riteScopes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Joan Reyero <joan@reyero.io>
emsearcy
left a comment
emsearcy
left a comment
There was a problem hiding this comment.
I have some substantial questions around providing what appears to be Discord IT Services Management capability but calling it part of the onboarding API.
| // --- Tool args --- | ||
|
|
||
| // LFXLensQueryArgs defines the input for lfx_lens_query. | ||
| // LFXLensQueryArgs defines the input for query_lfx_lens. |
There was a problem hiding this comment.
shall we also rename the structs to match? (similar to #56, which I've closed out in favor of this PR)
There was a problem hiding this comment.
Done — renamed LFXLensQueryArgs to QueryLFXLensArgs to match the tool name convention.
| // --- Tool args --- | ||
|
|
||
| // OnboardingListMembershipsArgs defines the input for onboarding_list_memberships. | ||
| // OnboardingListMembershipsArgs defines the input for list_membership_actions. |
There was a problem hiding this comment.
also missed renaming the struct here
There was a problem hiding this comment.
Moot now — the tool and its struct have been removed entirely (see reply to the other comment).
| Annotations: &mcp.ToolAnnotations{ | ||
| Title: "List Onboarding Memberships", | ||
| ReadOnlyHint: true, | ||
| DestructiveHint: boolPtr(false), |
There was a problem hiding this comment.
I don't understand this (and perhaps, it might even be better if we just removed this tool until we have the actual working use case?)
If this is a "invoke onboarding actions" (maybe with a dry-run?), then it would be destructive (destructive is anything that cannot be reverted/undone). If this is more of a "see status of previously-invoked actions: pending/success/failure" ... then how would it not be readonly?
There was a problem hiding this comment.
Agreed — removed the stub tool entirely. The registration, args struct, and handler are all gone. Left a TODO comment pointing to the member-onboarding service endpoint for when we're ready to add it back.
| func RegisterGetOnboardingDiscordConfig(server *mcp.Server) { | ||
| AddServiceTool(server, &mcp.Tool{ | ||
| Name: "get_onboarding_discord_config", | ||
| Description: "Check if Discord is configured for a project. IMPORTANT: Always call this before using any other *_onboarding_discord_* tool. If configured is false, tell the user they need to set up Discord in the LFX Project Control Center (PCC) first.", |
There was a problem hiding this comment.
I'm not wholly against this, but it doesn't seem like a typical pattern I've seen in other MCPs. Why not just let the user attempt to call the onboarding tool and have it fail with a helpful message: "Discord onboarding not available; please set up Discord in LFX Project Control Panel first"?
Also—and I realize we can change this later—but this would also get into "shouldn't we just have IT Collaboration Services tools that provide information and management of PCC-connected services" as their own tools, rather than calling them "onboarding config" tools.
There was a problem hiding this comment.
Agreed — removed get_discord_config entirely. The API now returns a 404 with a helpful detail message when Discord/Email isn't configured for a project, so every tool handles this gracefully:
- 404 responses are parsed for the FastAPI
{"detail": "..."}format and returned as anIsErrortool result, so the LLM can relay the message directly to the user (e.g. "Discord not available; please set up Discord in LFX Project Control Panel first.") - All "Depends on: get_discord_config" references removed from descriptions
- Same 404 handling added to email tools
This keeps the tool surface area smaller and follows the fail-with-helpful-message pattern you suggested.
| func RegisterListOnboardingDiscordRoles(server *mcp.Server) { | ||
| AddServiceTool(server, &mcp.Tool{ | ||
| Name: "list_onboarding_discord_roles", | ||
| Description: "List all roles in the project's Discord guild. Use this to discover what roles exist before assigning one, or when the user asks about the role structure. Depends on: get_onboarding_discord_config (call first).", |
There was a problem hiding this comment.
this doesn't sound like an onboarding tool. this just sounds like a Discord "IT Services" abstraction capability. Why is the onboarding API providing this? Is it wrapping v1 Project Infrastructure Service? Can we present it to the user as an "IT Service" feature rather than an "onboarding" feature?
There was a problem hiding this comment.
Agreed — dropped the "onboarding" prefix from all user-facing tool names. They're now presented as IT service capabilities:
Discord: get_discord_config, list_discord_roles, find_discord_role, find_discord_user, check_discord_user_role, assign_discord_role
Email: list_email_templates, render_email_template, send_email
Files renamed: onboarding_tools_discord.go → discord.go, onboarding_tools_email.go → email.go. Go function names updated to match. Internal implementation still references the onboarding service (since that's the backing API), but the tool surface area is now service-agnostic.
There was a problem hiding this comment.
not seeing these changes (yet) in the PR
| func RegisterSendOnboardingEmail(server *mcp.Server) { | ||
| AddServiceTool(server, &mcp.Tool{ | ||
| Name: "send_onboarding_email", | ||
| Description: "Send a templated email to a recipient via AWS SES. Use this to send onboarding or welcome emails to key contacts or members. Always render the template first to confirm content before sending. Depends on: render_onboarding_email_template (call first to preview), list_onboarding_email_templates (for template_name).", |
There was a problem hiding this comment.
is the template purely deterministic? even if it isn't it seems like it's not deterministic that the tool calls will be deterministic, since the agent calling them is not. IMO the only way you could really have that is by having a "draft" action which stages the email, and returns both the preview and some kind of hash to the user, and then a "send". this might even be able to be done with a single tool: mode: draft/send, where "send" requires the draft_hash from mode:draft.
There was a problem hiding this comment.
Also, we don't need to say "via AWS SES". Just "sends via LFX mail servers" or something.
There was a problem hiding this comment.
Done — changed to "via LFX mail servers".
There was a problem hiding this comment.
Agreed — collapsed render_email_template and send_email into a single send_email tool with a mode field (draft / send):
mode=draft: renders a preview via the render endpoint, no email sentmode=send: delivers the email (requiresto_emailandto_name)
The description instructs the caller to always draft first. This reduces the tool surface by one and keeps the same two-step safety flow. The hash-based confirmation is a known limitation of proxying through the onboarding API as-is — we can revisit when we have more control over the API contract.
There was a problem hiding this comment.
your comment is good, but I don't see the code that actually implements this change
Resolve go.mod conflicts by taking newer OTel versions from main and promoting go.opentelemetry.io/otel/trace to a direct dependency. 🤖 Generated with [GitHub Copilot](https://github.com/features/copilot) (via OpenCode) Signed-off-by: Eric Searcy <eric@linuxfoundation.org>
- Drop "onboarding" prefix from all Discord/email tool names and present them as IT service capabilities (discord.go, email.go) - Remove get_discord_config pre-check tool; all tools now handle 404 with the FastAPI detail message (e.g. "Discord not available; please set up Discord in LFX Project Control Panel first") - Remove list_membership_actions stub tool entirely (re-add when guided onboarding flow is ready) - Collapse render_email_template + send_email into a single send_email tool with mode=draft/send - Rename LFXLensQueryArgs → QueryLFXLensArgs to match tool naming - Change "via AWS SES" → "via LFX mail servers" in email description Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Joan Reyero <joan@reyero.io>
Summary
onboarding_tools_discord_*): get_config, list_roles, find_role, find_user, check_user_role, assign_roleonboarding_tools_email_*): list_templates, render_template, sendonboarding_list_memberships→onboarding_guided_list_membershipsto separate guided vs raw tool namespacesTest plan
member-onboarding/tools/discord/endpointsmember-onboarding/tools/email/endpointsonboarding_guided_list_membershipsstill returns dry-run stub🤖 Generated with Claude Code