Add managed Discord install flow for Milady agents#424
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Code Review: Add managed Discord install flow for Milady agentsOverall: solid architecture for the OAuth/install flow with good validation at boundaries. A few security, reliability, and test coverage issues worth addressing before merging. SecurityOAuth state Bot mention regex not escaped const botMentionRegex = new RegExp(`<@!?${botUserId}>`, "g");
CORS wildcard check Loopback origin list duplicated Bugs / Logic ErrorsRedundant null check after length guard Agent config updated before restart succeeds
Bridge failure returns HTTP 200 Error Handling
Race condition on guild conflict check Bridge response not schema-validated Silent JSON parse error swallowed .catch(() => ({}))This masks JSON parse failures. At minimum, log the error so it's visible in traces. PerformanceRegex recompiled on every message Two sequential DB queries in Test CoverageNo test for guild-owner enforcement No test for redirect validation Bridge error reason codes untested No test for agent restart on connect Code Quality
Magic string Vague conflict error message throw new Error("Discord server is already linked to another agent");Include the conflicting agent/guild IDs to make debugging easier: New bot permissions undocumented OAuth scope change is a breaking config change Posted by Claude Code |
Code ReviewOverall this is a solid, well-structured implementation of managed Discord installs. The HMAC-signed OAuth state is a notable improvement over the old plain base64 encoding. Test coverage is good. A few issues worth addressing before merging: Bug: Mutation in
|
PR Review: Add managed Discord install flow for Milady agentsOverall this is a well-structured PR with good input validation, HMAC-signed OAuth state, and solid test coverage. A few issues are worth addressing before merge. Bugs / Correctness Issues1.
|
Code Review: PR #424 — Add managed Discord install flow for Milady agentsThis is a substantial feature PR (5,573 additions / 485 deletions, 90+ files) introducing the managed Discord install OAuth flow, per-user OAuth connection scoping, and a managed Google connector for Milady cloud agents. The overall architecture is sound and the test coverage is notably good for a feature of this size. Below are findings organized by severity. Critical / Correctness Issues1. window.opener.postMessage(payload, "*");The 2. - owner_id: state.userId, // was the Eliza/cloud user ID
+ owner_id: identity.user.id, // now the Discord user's Snowflake IDThis is a breaking change to the 3. const conflictingGuildLinks = await miladySandboxesRepository.findByManagedDiscordGuildId(...)
const conflict = conflictingGuildLinks.find(...)
if (conflict) { throw ... }
// ... time passes ...
await miladySandboxesRepository.update(sandbox.id, { agent_config: nextConfig });Two concurrent install requests for the same guild and different agents can both pass the uniqueness check before either writes. This should use a database-level unique constraint or advisory lock rather than an application-level guard. Security Concerns4. const LOOPBACK_REDIRECT_ORIGINS = [
"https://localhost:*",
"https://127.0.0.1:*",
] as const;The 5. function sanitizeReturnPath(path: string | undefined): string {
if (!path || !path.startsWith("/")) {
return "/connected";
}
return path;
}This doesn't guard against Bugs / Potential Issues6. Dead private 7. const messages = await Promise.all(
(listed.messages ?? []).map(async (messageRef) => {
const response = await googleFetch({ url: `.../${messageId}?...` });
}),
);
8. if (!routed.handled) {
logger.debug("Managed Milady Discord message was not handled", { reason: routed.reason });
return; // no reply to user
}When routing returns 9. ".next-test-3303/types/**/*.ts",
".next-test-3300/types/**/*.ts",The generic glob 10. <title>${providerLabel} connected</title>
<h1>${providerLabel} connected.</h1>The Performance11. WHERE (agent_config -> '__miladyManagedDiscord' ->> 'guildId') = $1This expression query will do a full table scan on every guild message received. A partial index on Code Quality / Best Practices12. 13. 14. 15. 16. Missing Test CoverageThe test suite is a genuine strength of this PR —
Minor Nits
Overall: approve after addressing issues #1 (wildcard postMessage origin), #2 (owner_id semantics), and #11 (missing index). Issues #3 (TOCTOU race) and #5 (sanitizeReturnPath) should be fixed before or shortly after merge. |
|
PR 424 Review posted via separate message due to shell escaping constraints - see below. |
|
PR 424 Code Review - Part 1 of 2 (Security and Bugs) SECURITY
POTENTIAL BUGS
|
|
PR 424 Code Review - Part 2 of 2 (Quality, Performance, Tests, Summary) CODE QUALITY
PERFORMANCE
TEST COVERAGE
MINOR NITS
SUMMARY - Priority items before merging: [Security-High] Lock postMessage target origin instead of using wildcard. |
Code Review — PR #424: Managed Discord Install Flow for Milady AgentsOverall the architecture is sound and the test coverage is above average for a feature of this size. There are three high-priority items that should be addressed before merging out of draft, along with several medium and low concerns. Security[HIGH]
window.opener.postMessage(payload, "*");
window.opener.postMessage(payload, process.env.NEXT_PUBLIC_ELIZA_APP_ORIGIN);[MEDIUM] Loopback redirect origins are unconditional in production Both [MEDIUM]
function sanitizeReturnPath(path: string | undefined): string {
if (!path || !path.startsWith("/")) return "/connected";
return path;
}
[LOW] Guild owner check only enforced for The [LOW] HMAC comparison idiom is misleading
Bugs / Potential Issues[MEDIUM] Race condition in
CREATE UNIQUE INDEX milady_sandboxes_unique_guild
ON milady_sandboxes ((agent_config -> '__miladyManagedDiscord' ->> 'guildId'))
WHERE agent_config -> '__miladyManagedDiscord' IS NOT NULL;[MEDIUM] Provision failure after config write leaves agent in inconsistent state In both [LOW] Dead null-guard after length check
[LOW]
[LOW] Expired-connection message hardcodes "Google"
Performance[HIGH] Missing JSONB expression index — full table scan on every inbound Discord message
WHERE (agent_config -> '__miladyManagedDiscord' ->> 'guildId') = $1There is no index on this path. Every Discord message hitting CREATE INDEX milady_sandboxes_managed_discord_guild_id
ON milady_sandboxes ((agent_config -> '__miladyManagedDiscord' ->> 'guildId'))
WHERE agent_config -> '__miladyManagedDiscord' IS NOT NULL;[MEDIUM] When [LOW] Dynamic
Code Quality[MEDIUM] Every function getAuthUser() {
const ctx = authContextStorage.getStore();
if (!ctx) throw new Error("Not authenticated");
return ctx.user;
}Extract into the shared auth context module and re-export. [MEDIUM]
[LOW]
[LOW] Extract to [LOW] Moving [LOW]
[NITS]
Test CoverageThe test suite ( Gaps worth addressing:
Priority Summary
|
Code ReviewOverall this is a well-structured PR. The HMAC-signed OAuth state using 🔴 High — Reflected XSS in
|
1 participant
Summary
Testing