Beep Boop MCP

# Redundancy and Refactor Recommendations for Beep/Boop MCP Server Date: 2025-08-22 This document summarizes redundant code, duplicated logic, unused configuration, and workflow redundancies in the repository, with concrete, low-risk refactor recommendations. No code was changed; this is a recommendations report only. ## Table of contents - Executive summary - Scope and method - Findings and recommendations - 1. Duplicated logic and functions - 2. Unused or misleading configuration - 3. Redundant configs/scripts and minor bugs - 4. CI/CD workflow redundancies and dead steps - 5. Minor inconsistencies and polish - Refactor roadmap (prioritized) - Appendix: Code references --- ## Executive summary Top opportunities to simplify and de-duplicate: - Extract a single agent ID validation helper used in 3 places to remove repetition and avoid rule drift (~60+ lines consolidated). - DRY “check_status” output formatting shared by MCP tools and the HTTP listener; centralize next-steps formatter (~60 lines consolidated). - Unify Slack/Discord posting and “wait-for-reply” polling shared across MCP and listener implementations (hundreds of lines consolidated and behavior aligned). - Remove or implement unused config options (backups, metrics, timezone, escalation, etc.) to reduce cognitive load and misconfiguration risk. - Fix and DRY CI workflows; remove a dead Discord notification step that references non-existent outputs. - Clean up duplicate .gitignore patterns and centralize repo-wide ignores. These changes reduce maintenance overhead, improve correctness, and make future enhancements safer. --- ## Scope and method - Non-destructive review of the repository’s TypeScript sources, example configs, docs, and CI workflows. - Focus on redundant/duplicated implementations, unused configuration, and drift between docs and code. - Provide exact file:line references and example snippets to illustrate each finding. --- ## Findings and recommendations ### 1) Duplicated logic and functions 1.1 Agent ID validation “reasons” logic repeated 3x - Repeated blocks building reasons for invalid agent IDs in: - handleUpdateBoop - handleEndWork - handleCheckStatus (validating newAgentId) Example (one occurrence): // Validate agent ID with configuration if (!validateAgentIdWithConfig(agentId, config)) { const reasons = []; if (agentId.length > config.maxAgentIdLength) { reasons.push(`exceeds maximum length of ${config.maxAgentIdLength}`); } if (config.requireTeamPrefix && config.teamPrefixes.length > 0) { const hasValidPrefix = config.teamPrefixes.some(prefix => agentId.startsWith(prefix)); if (!hasValidPrefix) { reasons.push(`must start with one of: ${config.teamPrefixes.join(', ')}`); } } if (!/^[a-zA-Z0-9._-]+$/.test(agentId)) { reasons.push('contains invalid characters (only alphanumeric, hyphens, underscores, dots allowed)'); } return { content: [{ type: "text", text: `❌ Invalid agent ID "${agentId}": ${reasons.join(', ')}` }], isError: true }; } - Why redundant: Business rules can drift; error text consistency can break. - Recommendation: Extract a single helper, e.g. buildAgentIdValidationError(agentId, config): string | null (or a { valid: boolean; reasons: string[] }), and call it from all three places. - Impact: Removes repetition and future-proofs policy changes. 1.2 “check_status” output formatting duplicated (status emoji/text, timestamps, next steps) - Both MCP tools and the HTTP listener build similar human-readable responses with emojis, timestamps, and “next steps.” MCP tools formatter: // Choose appropriate emoji and status text let statusEmoji = ''; let statusText = ''; switch (status.status) { case WorkState.WORK_ALLOWED: statusEmoji = '✅'; statusText = 'WORK ALLOWED'; break; case WorkState.WORK_IN_PROGRESS: statusEmoji = '🚧'; statusText = 'WORK IN PROGRESS'; break; case WorkState.NO_COORDINATION: statusEmoji = '⭕'; statusText = 'NO COORDINATION'; break; case WorkState.INVALID_STATE: statusEmoji = '⚠️'; statusText = 'INVALID STATE'; break; } Listener formatter: let statusEmoji = ''; let statusText = ''; switch (status.status) { case WorkState.WORK_ALLOWED: statusEmoji = '✅'; statusText = 'WORK ALLOWED'; break; case WorkState.WORK_IN_PROGRESS: statusEmoji = '🚧'; statusText = 'WORK IN PROGRESS'; break; case WorkState.NO_COORDINATION: statusEmoji = '⭕'; statusText = 'NO COORDINATION'; break; case WorkState.INVALID_STATE: statusEmoji = '⚠️'; statusText = 'INVALID STATE'; break; } - Why redundant: Increases chance of divergence; doubles maintenance. - Recommendation: Create a shared formatter (e.g., formatStatusResponse(status, opts)) used by both paths; keep next-steps logic in one place. 1.3 Slack/Discord posting and “wait for user reply” duplicated in both MCP tool handlers and HTTP listener - update_user, initiate_conversation, and the reply-polling loop are implemented twice with nearly identical logic. - Example (Slack/Discord post in tools.ts): if (msg.platform === 'slack') { if (!config.slackBotToken) { return { content: [{ type: 'text', text: '❌ Slack bot token not configured' }], isError: true }; } const { WebClient } = await import('@slack/web-api'); const web = new WebClient(config.slackBotToken); const channel = msg.context.channelId as string; const thread_ts = msg.context.threadTs as string | undefined; await web.chat.postMessage({ channel, thread_ts, text: updateContent }); } else if (msg.platform === 'discord') { if (!config.discordBotToken) { return { content: [{ type: 'text', text: '❌ Discord bot token not configured' }], isError: true }; } const { REST, Routes } = await import('discord.js'); const rest = new (REST as any)({ version: '10' }).setToken(config.discordBotToken); const threadId = (msg.context as any).threadId as string | undefined; if (threadId) { await rest.post((Routes as any).channelMessages(threadId), { body: { content: updateContent } }); } else { const channelId = msg.context.channelId as string; await rest.post((Routes as any).channelMessages(channelId), { body: { content: updateContent, message_reference: { message_id: (msg.context as any).messageId } } }); } } else { return { content: [{ type: 'text', text: `❌ Unsupported platform: ${(msg as any).platform}` }], isError: true }; } - Why redundant: Posting and polling code is complex; doubling it multiplies maintenance and bug surface. - Recommendation: Extract: - postToSlack({token, channelId, text, threadTs?}) and postToDiscord({token, channelId, content}) helpers - createDiscordThread helper - waitForUserReply(inbox, {platform, threadIds…, since}) utility - Then use these in both MCP tool and listener paths. 1.4 Directory-access validation try/catch repeated across multiple tool handlers - handleCreateBeep, handleUpdateBoop, handleEndWork all wrap validateDirectoryAccess in similar try/catch to return ToolResponse. - Recommendation: introduce withValidatedDirectoryAccess(directory, config, async () => ToolResponse) to reduce repetition and keep error translation consistent. 1.5 Inconsistent validation helper usage (config-aware vs legacy) - cleanupStaleBoopAndClaim uses validateAgentId (a legacy, fixed-100 length rule) instead of validateAgentIdWithConfig (respects team prefixes and configurable length): // If new agent info provided, claim the directory if (newAgentId && validateAgentId(newAgentId)) { await createBoopFile(directory, newAgentId, workDescription || 'Claimed after stale cleanup'); claimed = true; message += ` and claimed for agent "${newAgentId}"`; } - Recommendation: Switch to validateAgentIdWithConfig (or accept config param for consistency). This aligns all validation policy in one place. --- ### 2) Unused or misleading configuration 2.1 Config helpers that are defined but unused - getEnvironmentDefaults and validateWebhookUrls exist but are not used anywhere. - Consider removing, or call validateWebhookUrls(config) inside loadConfig (warn in debug log). 2.2 Configuration fields present in BeepBoopConfig but not currently enforced or used - Likely unused in code paths: - backupEnabled, backupDir - enableMetrics - timezone - filePermissions - maxWorkDurationHours, warnThresholdHours, escalationEnabled, escalationAfterHours - devMode, ciMode, watchMode, forceCleanupOnStart, failOnStale - maxConcurrentOperations (validated and printed, but not used operationally) - auditLogPath (auditLogEnabled is used to add audit-like outputs, but file path is unused) - notificationTimeoutMs (no explicit timeout used in NotificationManager) - Recommendation: - If these are planned features, annotate as TODO and document as not yet implemented. - Otherwise, remove to reduce confusion and the chance of unexpected “no-op” configs. 2.3 Naming vs behavior: notificationRetryAttempts - Acts as a circuit breaker open threshold (no retry loop). Either implement real retries with backoff or rename to circuitBreakerThreshold to match behavior. 2.4 Example configs include unsupported keys - Example production/enterprise include health check, error recovery, encryption, and rate limit keys not modeled in BeepBoopConfig (e.g., BEEP_BOOP_HEALTH_CHECK_ENABLED). - Recommendation: Remove or comment as placeholders to avoid user confusion. --- ### 3) Redundant configs/scripts and minor bugs 3.1 package.json “config” scripts point to the wrong path - Current scripts reference ./select-config.sh, but script is under example-configs/select-config.sh. "test:webhooks": "npm run build && tsx test-webhooks.ts", "config": "./select-config.sh", "config:dev": "./select-config.sh development", "config:prod": "./select-config.sh production", - Recommendation: Fix script paths to example-configs/select-config.sh. Optionally collapse config:dev/prod/ci/enterprise to a single “config” script that passes an argument. 3.2 .gitignore duplicate entries and distributed ignores - Root .gitignore contains duplicate patterns (coverage/, *.lcov, dist). - First occurrence: # Coverage directory used by tools like istanbul coverage/ *.lcov - Second occurrence: # Test files (keep source test files in repo, but ignore test outputs) coverage/ *.lcov .nyc_output/ - src/.gitignore and docs/.gitignore also ignore beep, boop, and inbox. These can be centralized at root for consistency: .beep-boop-inbox/ # Beep/Boop coordination files beep boop - Recommendation: Deduplicate root patterns and remove per-subdir ignores if they add no additional value. 3.3 Legacy/demo inbox layout vs current InboxStore - There are demo files under .beep-boop-inbox/messages/, but InboxStore reads/writes at the inbox root and moves to processed/. The messages/ subfolder appears obsolete. Clean it up or update store to match docs if desired. 3.4 test-blocking-conversation.js is ad-hoc and not wired to scripts - Keep as a manual test utility or move to a docs/examples folder; clarify its purpose in README. --- ### 4) CI/CD workflow redundancies and dead steps 4.1 Duplicate test logic across two workflows - .github/workflows/test.yml and build-and-publish.yml both install, build, test, and tsc --noEmit. This leads to drift. - Recommendation: Extract a reusable workflow called by both (workflow_call) for test/build steps. 4.2 Dead notify-discord job references non-existent job outputs - notify-discord references needs.publish.outputs.published and needs.publish.outputs.version, but the publish job does not expose those as job outputs. notify-discord: needs: [publish] runs-on: ubuntu-latest if: github.ref == 'refs/heads/main' && github.event_name == 'push' steps: - name: Send Discord notification if: needs.publish.outputs.published == 'true' run: | curl -H "Content-Type: application/json" \ -X POST \ -d "{\"content\":\"🚀 **New release published!**\n\n📦 **beep-boop-mcp-server** v${{ needs.publish.outputs.version }}\n\n✅ Successfully built and published to npm\n🔗 [View on GitHub](https://github.com/thesammykins/beep_boop_mcp/releases/latest)\"}" \ ${{ secrets.DISCORD_WEBHOOK_URL }} || echo "Discord notification failed (webhook not configured)" - Recommendation: Either export proper outputs from publish (job-level outputs) and set them, or remove this job. 4.3 Changelog/version bump logic is verbose inline bash - Consider a maintained action or a small repo script to reduce complexity and risk of drift. --- ### 5) Minor inconsistencies and polish 5.1 Server version mismatch - src/index.ts hardcodes version '1.0.0' while package.json is 1.1.3. const server = new McpServer({ name: 'beep-boop-coordination', version: '1.0.0' }); - Recommendation: Source from package.json at build time or keep a single canonical version constant. 5.2 Noisy debug logs not gated - handleInitiateConversation prints several console.error debug lines unconditionally. Gate behind config.logLevel === 'debug' for consistency. 5.3 Docs vs implementation drift (inbox structure) - README/docs mention .beep-boop-inbox/messages/ but current InboxStore doesn’t create/use this subdirectory. Align documentation or store behavior. --- ## Refactor roadmap (prioritized) Quick wins (< 1 hour each) - Extract and reuse agent ID validation reasons helper; replace 3 occurrences. - DRY the status/next-steps formatter for check_status responses (shared module). - Gate debug logs in handleInitiateConversation by log level. - Remove or wire unused helpers (validateWebhookUrls, getEnvironmentDefaults). - Deduplicate .gitignore entries; centralize ignores. - Fix package.json config script paths to example-configs/select-config.sh. - Mark unsupported example-config keys as placeholders or remove them. Medium effort (1–4 hours) - Extract platform posting and thread-creation helpers; unify code paths in tools.ts and ingress/index.ts. - Extract a single waitForUserReply helper for both code paths. - Replace validateAgentId with validateAgentIdWithConfig inside cleanupStaleBoopAndClaim (pass config). - DRY CI: introduce a reusable workflow for test/build and remove dead notify-discord job (or add outputs). Larger improvements (> 4 hours) - Decide on unused BeepBoopConfig fields: implement or remove. If implementing: - Add metrics hooks, backups to backupDir, work-duration enforcement and escalations, notification timeouts, and auditLogPath outputs. - Implement actual retry/backoff in NotificationManager (rename retryAttempts or implement retries). Risk assessment - Most items are Low risk (pure refactors and DRYing). - Config removal is Medium risk (breaking user expectations); mitigate with a deprecation note and docs updates. - CI workflow consolidation is Low risk if tested in a branch. --- ## Appendix: code references Agent validation repeated blocks: - tools.ts handleUpdateBoop (lines ~181–205), handleEndWork (lines ~323–346), handleCheckStatus (lines ~467–489). Status formatter duplication: - tools.ts (emoji/text lines ~587–607; response builder ~609–636) - ingress/index.ts (emoji/text ~125–132; response builder ~134–145) Slack/Discord posting duplication: - tools.ts update_user (lines ~727–751), initiate_conversation (lines ~826–861, ~867–975 for inbox and polling) - ingress/index.ts update_user (lines ~174–216), initiate_conversation (lines ~250–395, and ~337–381 for polling) validateAgentId vs validateAgentIdWithConfig: export function validateAgentId(agentId: string): boolean { if (!agentId || typeof agentId !== 'string') return false; const trimmed = agentId.trim(); if (trimmed.length === 0) return false; if (trimmed.length > 100) return false; // Reasonable length limit // Allow alphanumeric, hyphens, underscores, and dots const validPattern = /^[a-zA-Z0-9._-]+$/; return validPattern.test(trimmed); } Unused config helpers: export function getEnvironmentDefaults(): Partial<BeepBoopConfig> { const env = process.env.NODE_ENV || 'development'; switch (env) { case 'development': return { ... }; case 'test': return { ... }; case 'production': return { ... }; default: return {}; } } export function validateWebhookUrls(config: BeepBoopConfig): string[] { const errors: string[] = []; if (config.discordWebhookUrl && !config.discordWebhookUrl.includes('discord.com/api/webhooks')) { errors.push('Discord webhook URL must be a valid Discord webhook URL'); } if (config.slackWebhookUrl && !config.slackWebhookUrl.includes('hooks.slack.com')) { errors.push('Slack webhook URL must be a valid Slack webhook URL'); } return errors; } Server version mismatch: const server = new McpServer({ name: 'beep-boop-coordination', version: '1.0.0' }); CI notify-discord dead outputs: notify-discord: needs: [publish] ... - name: Send Discord notification if: needs.publish.outputs.published == 'true' run: | curl ... v${{ needs.publish.outputs.version }} ... .gitignore duplicates: # Coverage directory used by tools like istanbul coverage/ *.lcov # Test files (keep source test files in repo, but ignore test outputs) coverage/ *.lcov .nyc_output/ Package config script path bug: "config": "./select-config.sh", "config:dev": "./select-config.sh development", "config:prod": "./select-config.sh production", If you’d like, I can draft the shared helpers (formatter, platform posting, reply waiter) and a small PR plan to implement the quick wins next.