Add @mention notifications and stop command #4
Conversation
This update enhances the Discord bot's reliability and fixes several issues: **Core Improvements:** - Update to Claude Sonnet 4.5 (claude-sonnet-4-5-20250929) - Add --permission-mode acceptEdits for autonomous operation - Fix exit code 143 false error reporting (SIGTERM is normal shutdown) - Add hook disabling mechanism for Discord bot sessions **Bug Fixes:** - Fix MCP bridge port mismatch (now uses MCP_SERVER_PORT env variable) - Truncate long error messages to fit Discord's 4096 character limit - Prevent sound notifications during Discord bot operations **Configuration:** - Add MCP_SERVER_PORT to environment configuration - Pass port configuration through to MCP bridge - Add CLAUDE_DISABLE_HOOKS environment variable **Documentation:** - Add troubleshooting section for Claude Code hooks - Document MCP port configuration - Add hook disabling instructions **Testing:** - Add tests for exit code handling (0, 143, and error codes) - Improve test coverage for process lifecycle 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add @mention notifications when sessions complete, fail, or timeout - Add "stop" command to kill running Claude Code processes - Add bot restart script (bun run restart) - Fix shell.ts tests (0% → 93.58% coverage) - Update vitest to v2.1.9 with coverage support - Add test coverage reporting (overall 50.69%) - Exclude MCP modules from coverage (require integration testing) - Improve process exit code handling (ignore 143/SIGTERM) - Update documentation with new features 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughAdds MCP_SERVER_PORT env var and threads it through the bridge and spawned sessions; introduces session discovery and beads MCP server; adds screenshot detection/capture; implements a channel-level "stop" command and consolidated live progress embeds; adds restart script, Puppeteer, test coverage, and related tests/docs. Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.env.example(1 hunks).gitignore(1 hunks)README.md(3 hunks)mcp-bridge.cjs(2 hunks)package.json(2 hunks)restart.sh(1 hunks)src/bot/client.ts(1 hunks)src/claude/manager.ts(9 hunks)src/utils/shell.ts(3 hunks)test/bot/client.test.ts(3 hunks)test/claude/manager.test.ts(3 hunks)test/utils/shell.test.ts(1 hunks)vitest.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/utils/shell.test.ts (1)
src/utils/shell.ts (1)
buildClaudeCommand(17-55)
src/claude/manager.ts (1)
src/db/database.ts (1)
DatabaseManager(11-69)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 14-14: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🪛 Shellcheck (0.11.0)
restart.sh
[warning] 18-18: i appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (20)
mcp-bridge.cjs (1)
10-11: LGTM!The configurable MCP server port is well-implemented with a sensible default. The port is correctly parsed as an integer on line 52.
vitest.config.ts (1)
8-21: LGTM!The coverage configuration is well-structured with appropriate providers, reporters, and inclusion/exclusion patterns. The exclusion of MCP modules and entry points aligns with the testing strategy mentioned in the PR objectives.
package.json (3)
14-14: LGTM!The test:coverage script follows standard Vitest conventions and integrates well with the coverage configuration added in vitest.config.ts.
24-27: LGTM!The dependency additions support the new test coverage functionality and provide type definitions for Express, which enhances type safety.
11-11: No action required.The verification confirms that restart.sh exists at the repository root and has proper executable permissions. The concerns raised in the original review comment are not valid issues.
test/claude/manager.test.ts (3)
18-23: LGTM!The mock for shell utilities is properly structured and provides appropriate test doubles for command building and shell escaping functions.
101-110: LGTM!The test correctly verifies the initialization of the channelToolCalls map, which aligns with the implementation changes in src/claude/manager.ts for tracking tool interactions.
209-327: LGTM!The process exit code handling tests comprehensively verify that:
- Exit code 0 (success) does not trigger error messages
- Exit code 143 (SIGTERM) is treated as normal termination and does not trigger error messages
- Non-zero exit codes (e.g., 1) correctly trigger error embeds with descriptive messages
This test coverage validates the improved exit code handling implemented in src/claude/manager.ts.
src/claude/manager.ts (9)
9-17: LGTM!The truncation utility correctly handles Discord's embed description limit of 4096 characters. The implementation leaves a 50-character buffer for the truncation message, which is a sensible approach.
24-24: LGTM!The channelUserIds map appropriately tracks user IDs per channel for mention functionality, enabling the notification features described in the PR objectives.
99-103: LGTM!The implementation correctly stores the channel name and optional user ID from the Discord context, enabling user mentions in subsequent notifications.
120-120: LGTM!Setting CLAUDE_DISABLE_HOOKS=1 is appropriate for bot usage, as it disables audio/hooks that would not work in a Discord bot environment.
147-165: LGTM!The timeout handling correctly incorporates user mentions when available, improving user notification as described in the PR objectives. The 5-minute timeout is reasonable for Claude Code operations.
221-240: LGTM!The exit code handling is well-implemented:
- Exit code 0 (success) and null are handled as normal termination
- Exit code 143 (SIGTERM) is correctly treated as normal shutdown rather than an error
- User mentions are included in error notifications to alert users
This aligns perfectly with the PR objectives to avoid showing SIGTERM exits as errors.
255-261: LGTM!The truncation prevents potentially large stderr output from exceeding Discord's embed limits while still providing useful warning information to users.
273-287: LGTM!Process error handling correctly:
- Includes user mentions for notifications
- Truncates error messages to fit Discord embed limits
- Properly cleans up process tracking on errors
436-462: LGTM!Result message handling appropriately includes user mentions for both success and failure cases, ensuring users are notified when their Claude Code sessions complete.
.gitignore (1)
17-17: LGTM!Adding log.txt to .gitignore is appropriate since src/claude/manager.ts writes runtime logs to this file (as seen in lines 174-175 of manager.ts). This prevents log artifacts from being committed to version control.
test/utils/shell.test.ts (2)
29-36: LGTM!Replacing exact string equality with containment checks makes the tests more robust and resilient to minor formatting changes in command construction.
56-66: LGTM!The new test case appropriately verifies that Discord context is integrated into the command building, confirming that --mcp-config is included when a Discord context object is provided. This aligns with the MCP configuration changes throughout the PR.
| # MCP Server Port (optional, defaults to 3001) | ||
| # Change this if port 3001 is already in use | ||
| MCP_SERVER_PORT=3001 |
There was a problem hiding this comment.
Add a blank line at the end of the file.
The dotenv-linter correctly identifies that the file should end with a blank line, which is a common convention for text files.
Apply this diff:
# MCP Server Port (optional, defaults to 3001)
# Change this if port 3001 is already in use
MCP_SERVER_PORT=3001
+📝 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.
| # MCP Server Port (optional, defaults to 3001) | |
| # Change this if port 3001 is already in use | |
| MCP_SERVER_PORT=3001 | |
| # MCP Server Port (optional, defaults to 3001) | |
| # Change this if port 3001 is already in use | |
| MCP_SERVER_PORT=3001 | |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 14-14: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🤖 Prompt for AI Agents
In .env.example around lines 12 to 14, the file is missing a trailing blank line
at EOF; update the file by adding a single newline (blank line) at the end so
the file ends with an empty line.
| // Check if message is a stop command | ||
| const isStopCommand = message.content.trim().toLowerCase() === 'stop'; | ||
|
|
||
| // If there's an active process and user says "stop", kill it | ||
| if (isStopCommand && this.claudeManager.hasActiveProcess(channelId)) { | ||
| console.log(`Stop command received for channel ${channelId}, killing process`); | ||
| this.claudeManager.killActiveProcess(channelId); | ||
|
|
||
| const stopEmbed = new EmbedBuilder() | ||
| .setTitle("🛑 Stopped") | ||
| .setDescription("Claude Code process has been stopped") | ||
| .setColor(0xFF6B6B); // Red-ish for stop | ||
|
|
||
| await message.channel.send({ embeds: [stopEmbed] }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Prevent stop command from launching new sessions
When no Claude process is active, a stop message slips past this guard and falls through to the session startup logic, spinning up a brand-new run with prompt "stop". That’s user-visible breakage: the command meant to halt work actually kicks off a new job. Please short-circuit all stop requests before the resume logic—respond with a “nothing to stop” message (or silence) instead of continuing into runClaudeCode.
- // Check if message is a stop command
- const isStopCommand = message.content.trim().toLowerCase() === 'stop';
-
- // If there's an active process and user says "stop", kill it
- if (isStopCommand && this.claudeManager.hasActiveProcess(channelId)) {
- console.log(`Stop command received for channel ${channelId}, killing process`);
- this.claudeManager.killActiveProcess(channelId);
-
- const stopEmbed = new EmbedBuilder()
- .setTitle("🛑 Stopped")
- .setDescription("Claude Code process has been stopped")
- .setColor(0xFF6B6B); // Red-ish for stop
-
- await message.channel.send({ embeds: [stopEmbed] });
- return;
- }
+ const isStopCommand = message.content.trim().toLowerCase() === "stop";
+ if (isStopCommand) {
+ if (this.claudeManager.hasActiveProcess(channelId)) {
+ console.log(`Stop command received for channel ${channelId}, killing process`);
+ this.claudeManager.killActiveProcess(channelId);
+
+ const stopEmbed = new EmbedBuilder()
+ .setTitle("🛑 Stopped")
+ .setDescription("Claude Code process has been stopped")
+ .setColor(0xff6b6b);
+ await message.channel.send({ embeds: [stopEmbed] });
+ } else {
+ await message.channel.send("No Claude Code process is currently running.");
+ }
+ return;
+ }📝 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.
| // Check if message is a stop command | |
| const isStopCommand = message.content.trim().toLowerCase() === 'stop'; | |
| // If there's an active process and user says "stop", kill it | |
| if (isStopCommand && this.claudeManager.hasActiveProcess(channelId)) { | |
| console.log(`Stop command received for channel ${channelId}, killing process`); | |
| this.claudeManager.killActiveProcess(channelId); | |
| const stopEmbed = new EmbedBuilder() | |
| .setTitle("🛑 Stopped") | |
| .setDescription("Claude Code process has been stopped") | |
| .setColor(0xFF6B6B); // Red-ish for stop | |
| await message.channel.send({ embeds: [stopEmbed] }); | |
| return; | |
| } | |
| const isStopCommand = message.content.trim().toLowerCase() === "stop"; | |
| if (isStopCommand) { | |
| if (this.claudeManager.hasActiveProcess(channelId)) { | |
| console.log(`Stop command received for channel ${channelId}, killing process`); | |
| this.claudeManager.killActiveProcess(channelId); | |
| const stopEmbed = new EmbedBuilder() | |
| .setTitle("🛑 Stopped") | |
| .setDescription("Claude Code process has been stopped") | |
| .setColor(0xff6b6b); | |
| await message.channel.send({ embeds: [stopEmbed] }); | |
| } else { | |
| await message.channel.send("No Claude Code process is currently running."); | |
| } | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/bot/client.ts around lines 100 to 115, the current handling only kills an
active Claude process but lets a plain "stop" message fall through and start a
new session; change the logic to short-circuit all "stop" requests by checking
isStopCommand before any session startup logic and, if no active process exists,
send an informative reply (e.g., "No active process to stop") or do nothing,
then return immediately so runClaudeCode (or any session-start code) is not
invoked with "stop" as the prompt.
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/claude/manager.ts (2)
52-59: Memory leak:channelUserIdsnot cleaned up.The
channelUserIdsMap is not cleared inclearSession, nor in the process close/error handlers (lines 219, 270). This will accumulate stale entries over time.Apply this diff to add cleanup:
clearSession(channelId: string): void { this.killActiveProcess(channelId); this.db.clearSession(channelId); this.channelMessages.delete(channelId); this.channelToolCalls.delete(channelId); this.channelNames.delete(channelId); + this.channelUserIds.delete(channelId); this.channelProcesses.delete(channelId); }Also add cleanup in the process close handler after line 219 and error handler after line 270:
this.channelUserIds.delete(channelId);
443-450: Apply truncation to result descriptions.The result description (line 444) could exceed Discord's 4096-character embed limit if
parsed.resultis lengthy, potentially causing the message send to fail.Apply this diff to add truncation:
if (parsed.subtype === "success") { let description = "result" in parsed ? parsed.result : "Task completed"; description += `\n\n*Completed in ${parsed.num_turns} turns*`; resultEmbed .setTitle("✅ Session Complete") - .setDescription(description) + .setDescription(truncateForEmbed(description)) .setColor(0x00FF00); // Green for success } else { resultEmbed .setTitle("❌ Session Failed") - .setDescription(`Task failed: ${parsed.subtype}`) + .setDescription(truncateForEmbed(`Task failed: ${parsed.subtype}`)) .setColor(0xFF0000); // Red for failure }
🧹 Nitpick comments (1)
src/claude/manager.ts (1)
9-17: Hardcoded buffer in truncation logic.The
-50buffer is larger than the actual suffix length (27 characters). Consider calculating the buffer based on the suffix to improve maintainability.Apply this diff:
function truncateForEmbed(text: string, maxLength: number = 4096): string { if (text.length <= maxLength) { return text; } - return text.substring(0, maxLength - 50) + '\n...\n[Message truncated]'; + const suffix = '\n...\n[Message truncated]'; + return text.substring(0, maxLength - suffix.length) + suffix; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(3 hunks)src/claude/manager.ts(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🔇 Additional comments (6)
src/claude/manager.ts (6)
99-103: LGTM!User ID storage logic correctly handles the optional
discordContextand only stores the user ID when present.
120-120: LGTM!Disabling audio/hooks for Discord bot usage is appropriate and prevents unnecessary overhead.
146-166: Verify timeout duration discrepancy.The code implements a 30-minute timeout, but the PR summary states "timeout at 5 minutes." Please confirm which timeout value is intended.
221-240: LGTM!Correctly ignores exit code 143 (SIGTERM) to prevent false error notifications on normal shutdowns, and properly adds user mentions to error embeds.
254-262: Verify: stderr warnings don't mention users.Unlike timeout, error, and result notifications, stderr warnings (lines 254-262) do not include user mentions. If warnings are considered actionable or important, consider adding mentions for consistency.
275-287: LGTM!Process error handling correctly applies truncation and user mentions.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/shell.ts (1)
17-55:--resumeflag is inserted beforeclaude, breaking resumed sessions
commandParts.splice(3, 0, "--resume", sessionId)results incd <dir> && --resume <id> claude ..., so the shell tries to run--resumeas the command. This likely makes session resume unusable.A safer construction is to append
--resumeimmediately afterclaudebefore the other flags:- const commandParts = [ - `cd ${workingDir}`, - "&&", - "claude", - "--output-format", - "stream-json", - "--model", - "claude-sonnet-4-5-20250929", // Latest Sonnet 4.5 - "--permission-mode", - "acceptEdits", // Automatically accept edits without prompting - "-p", - escapedPrompt, - "--verbose", - ]; - - // Add session-specific MCP configuration + const commandParts = [ + `cd ${workingDir}`, + "&&", + "claude", + ]; + + if (sessionId) { + commandParts.push("--resume", sessionId); + } + + commandParts.push( + "--output-format", + "stream-json", + "--model", + "claude-sonnet-4-5-20250929", // Latest Sonnet 4.5 + "--permission-mode", + "acceptEdits", // Automatically accept edits without prompting + "-p", + escapedPrompt, + "--verbose", + ); + + // Add session-specific MCP configurationThis preserves your new flags and fixes the resume invocation.
🧹 Nitpick comments (7)
test/services/screenshot.test.ts (1)
1-58: Screenshot URL detection tests are solid; consider https coverageThe tests exercise the main detection paths (host variants, ports, paths, multiple URLs, and negatives) and align with
detectScreenshotUrl’s implementation. You might optionally add a case forhttps://localhost:PORTsince the regex already supportshttps?, but the current suite is otherwise comprehensive.test/claude/manager.test.ts (1)
209-350: Exit code handling tests correctly cover success, SIGTERM, and failuresThese tests nicely exercise the updated close-handler logic: no error embed for exit codes
0and143, and a❌ Session Failededit for real failures. The manual invocation of the recordedcloseandstdouthandlers is clear and keeps the suite deterministic; if you ever see flakiness, you could swap the fixedsetTimeoutwaits for explicit flushing via mocked promises, but it’s fine as-is.SESSION_DISCOVERY.md (1)
1-87: Session discovery documentation is clear and aligned with implementationThe doc accurately describes the process scan, channel creation, naming, and env requirements reflected in
SessionDiscoveryServiceand its tests. If you expect non-Linux environments, you might optionally call out thatps aux/pwdxmake this feature Linux-centric.src/services/screenshot.ts (1)
113-113: Consider extracting magic number as a named constant.The 3-second delay is hardcoded. Consider extracting it as a named constant for clarity and maintainability.
Example:
+const SERVER_READINESS_DELAY_MS = 3000; + export async function handleScreenshotDetection( text: string, channel: any ): Promise<void> { const url = detectScreenshotUrl(text); if (!url) { return; } console.log(`Detected screenshot-worthy URL: ${url}`); // Wait a bit for the server to be ready - await new Promise(resolve => setTimeout(resolve, 3000)); + await new Promise(resolve => setTimeout(resolve, SERVER_READINESS_DELAY_MS));src/claude/manager.ts (3)
10-18: Consider aligning truncation offset with message length.The function subtracts 50 characters but only adds approximately 20 characters for the truncation message (
'\n...\n[Message truncated]'). This leaves about 30 characters unused.For tighter packing:
function truncateForEmbed(text: string, maxLength: number = 4096): string { if (text.length <= maxLength) { return text; } - return text.substring(0, maxLength - 50) + '\n...\n[Message truncated]'; + const suffix = '\n...\n[Message truncated]'; + return text.substring(0, maxLength - suffix.length) + suffix; }
278-320: LGTM!The progress embed builder is well-structured with clear status indicators and proper truncation. The decision to show only the last 5 tools keeps the embed concise.
If desired, you could extract the magic number as a constant:
+const MAX_RECENT_TOOLS = 5; + private buildProgressEmbed(channelId: string): EmbedBuilder { // ... if (state.tools.length > 0) { - const recentTools = state.tools.slice(-5); + const recentTools = state.tools.slice(-MAX_RECENT_TOOLS); description += `**Recent Tools:**\n${recentTools.join('\n')}`; }
371-382: LGTM!The assistant message handling correctly truncates long messages and integrates screenshot detection. The 200-character truncation keeps progress updates concise while the full message can still trigger screenshots.
Consider extracting the truncation length as a constant:
+const ASSISTANT_MESSAGE_PREVIEW_LENGTH = 200; + if (content && content.trim()) { - const truncated = content.length > 200 ? content.substring(0, 200) + '...' : content; + const truncated = content.length > ASSISTANT_MESSAGE_PREVIEW_LENGTH + ? content.substring(0, ASSISTANT_MESSAGE_PREVIEW_LENGTH) + '...' + : content; state.assistant = truncated;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
.gitignore(1 hunks)README.md(4 hunks)SESSION_DISCOVERY.md(1 hunks)package.json(1 hunks)src/bot/client.ts(4 hunks)src/claude/manager.ts(10 hunks)src/index.ts(1 hunks)src/mcp/beads-server.ts(1 hunks)src/mcp/permission-manager.ts(1 hunks)src/services/screenshot.ts(1 hunks)src/services/session-discovery.ts(1 hunks)src/utils/shell.ts(3 hunks)test/bot/client.test.ts(4 hunks)test/claude/manager.test.ts(3 hunks)test/services/screenshot.test.ts(1 hunks)test/services/session-discovery.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/bot/client.test.ts
- package.json
- src/bot/client.ts
🧰 Additional context used
🧬 Code graph analysis (5)
test/services/screenshot.test.ts (1)
src/services/screenshot.ts (1)
detectScreenshotUrl(7-30)
test/services/session-discovery.test.ts (1)
src/services/session-discovery.ts (1)
SessionDiscoveryService(18-204)
src/services/session-discovery.ts (1)
src/claude/manager.ts (1)
ClaudeManager(20-502)
src/index.ts (1)
src/bot/client.ts (1)
DiscordBot(11-201)
src/claude/manager.ts (1)
src/services/screenshot.ts (1)
handleScreenshotDetection(100-122)
🔇 Additional comments (22)
.gitignore (1)
17-18: LGTM!Adding
log.txtandbot.logto the ignore list is appropriate and aligns with the restart.sh script introduced in this PR.src/mcp/permission-manager.ts (1)
12-15: Extended approval timeout to 5 minutes looks correctUsing
MCP_APPROVAL_TIMEOUT || '300'to deriveapprovalTimeoutcleanly aligns the timeout message and behavior; no logic issues spotted.src/index.ts (1)
16-19: DiscordBot wiring with baseFolder is consistentPassing
config.baseFolderintonew DiscordBot(...)matches the updated constructor and enables session discovery; nothing further needed here.test/claude/manager.test.ts (2)
18-23: Shell utils mock keeps manager tests stableMocking
buildClaudeCommandandescapeShellStringhere isolatesClaudeManagertests from changes in the shell command format, which is appropriate given the more complex CLI wiring insrc/utils/shell.ts.
100-110: setDiscordMessage test update matches new per-channel tool trackingAsserting that
channelToolCalls.get('channel-1')is aMapensures the new per-channel tool-call tracking is initialized whenever a Discord message is attached. This is a good regression guard for the new progress/notification model.test/services/session-discovery.test.ts (1)
1-380: Session discovery tests provide good coverage of discovery and channel sync pathsThe suite thoroughly exercises process discovery (normal, excluded, sanitized, empty, error) and sync behavior (category creation, new vs existing channels, missing guild, no sessions) with realistic Discord and child_process mocks. This should catch most regressions in
SessionDiscoveryServicewithout hitting the real system.src/mcp/beads-server.ts (1)
54-299: Beads MCP tool definitions and handlers look consistent withbdCLIThe tool list, input schemas (including enums and required fields), and the
CallToolRequestswitch all line up: each tool constructs a plausiblebdargument list, many of them using--jsonwhere structured output is expected, and responses are wrapped as simple text content with a clear error fallback (isError: truein the catch). OnceexecuteBdCommandis made shell-safe, this server should integrate cleanly with your existing MCP config and the--allowedTools mcp__beadssetting inshell.ts.Also applies to: 301-305, 307-476, 478-488
src/utils/shell.ts (1)
65-88: Verify beads server build path used in MCP config
beadsServerPathis set topath.join(baseDir, 'dist', 'beads-server.js'), but the source file lives atsrc/mcp/beads-server.ts. Depending on your build setup, the compiled output may end up atdist/mcp/beads-server.jsinstead.Please double‑check that the actual build artifact path matches this value; if it's under
dist/mcp, adjust accordingly:- const beadsServerPath = path.join(baseDir, 'dist', 'beads-server.js'); + const beadsServerPath = path.join(baseDir, 'dist', 'mcp', 'beads-server.js');or align it with however your bundler outputs the file.
src/services/screenshot.ts (1)
7-30: LGTM!The URL detection logic correctly handles various localhost URL formats with case-insensitive matching and proper fallback behavior.
src/claude/manager.ts (6)
25-32: LGTM!The per-channel state tracking design is well-structured with clear separation of concerns (user IDs, progress messages, and progress state).
109-113: LGTM!Storing the user ID for later @mentions is correctly implemented and integrates well with the notification flow.
130-130: LGTM!Setting
CLAUDE_DISABLE_HOOKS=1correctly prevents user hooks (like audio notifications) from interfering with the bot, as documented in the README.
156-176: LGTM!The timeout handling (30 minutes) is reasonable, correctly updates the progress state, and properly notifies the user via @mention when available.
230-246: LGTM!Correctly handles exit codes by treating code 143 (SIGTERM) as normal termination rather than an error, which aligns with the PR objectives for proper process exit handling.
67-68: LGTM!Properly cleans up the new per-channel progress tracking state to prevent memory leaks.
README.md (4)
29-33: LGTM!The features list accurately describes the new capabilities added in this PR, including live progress updates, automatic screenshots, Beads integration, and MCP permissions.
211-211: LGTM!The stop command documentation accurately reflects the functionality implemented in the codebase.
278-288: Verify test coverage percentages are current.Ensure these coverage numbers match the actual latest test run results. Hardcoded percentages in documentation can become stale.
Consider generating this section from the actual coverage report or noting the date/commit when these numbers were captured.
290-296: LGTM!The exit code handling documentation accurately describes the implementation in
manager.tsline 230, where exit code 143 (SIGTERM) is explicitly excluded from error handling.src/services/session-discovery.ts (3)
88-100: LGTM!The channel name extraction logic correctly handles both baseFolder-relative paths and standalone directory names.
106-112: LGTM!The channel name sanitization correctly implements Discord's channel naming requirements (lowercase, alphanumeric + dashes/underscores).
117-136: LGTM!The sync logic correctly handles the common case of a single-guild bot and gracefully handles the absence of guilds.
| // Helper function to execute bd commands | ||
| async function executeBdCommand(args: string[], cwd?: string): Promise<{ stdout: string; stderr: string }> { | ||
| const command = `bd ${args.join(' ')}`; | ||
| console.error(`Executing: ${command} in ${cwd || process.cwd()}`); | ||
|
|
||
| try { | ||
| const result = await execAsync(command, { | ||
| cwd: cwd || process.cwd(), | ||
| maxBuffer: 10 * 1024 * 1024, // 10MB buffer | ||
| }); | ||
| return result; | ||
| } catch (error: any) { | ||
| // bd commands may return non-zero exit codes with useful output | ||
| return { | ||
| stdout: error.stdout || '', | ||
| stderr: error.stderr || error.message || '', | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js child_process.exec shell injection vulnerability default shell behavior
💡 Result:
Short answer
- child_process.exec runs the command in a system shell (default: /bin/sh on Unix, cmd.exe on Windows). This means the command string is interpreted by the shell and is vulnerable to command‑injection if it includes untrusted input. [1][2]
Why this matters
- Shell interpretation (globbing, redirection, operators like &&, |, ;) lets an attacker inject additional commands by controlling part of the command string. Node’s docs and security guidance explicitly warn that exec is unsafe with unsanitized input. [1][2][3]
Safe alternatives / mitigations
- Prefer execFile or spawn with arguments (they do not spawn a shell by default), so arguments are passed directly and cannot be parsed as shell syntax. [1][2]
- If you must use a shell, avoid concatenating untrusted input; properly escape it (Node has no built‑in safe shell-escape) or restrict input to a safe allowlist. [2][3]
- You can also set the shell option explicitly when using spawn/exec to control behavior, but that still uses a shell and keeps injection risk. [1]
Example (safe)
- Unsafe: exec(
ping -c 1 ${host}) - Safer: spawn('ping', ['-c', '1', host]) or execFile('ping', ['-c','1', host]). [2][3]
Sources
- Node.js child_process documentation (shell default, exec vs execFile/spawn). [1]
- Node.js / security blog guidance on command injection and prefer execFile/spawn. [2]
- SecureFlag/OWASP guidance on OS command injection in Node.js. [3]
🏁 Script executed:
# First, locate and examine the actual code
find . -name "beads-server.ts" -o -name "beads-server.js" | head -5Repository: timoconnellaus/claude-code-discord-bot
Length of output: 220
🏁 Script executed:
# Try to list repository structure and find the file
ls -la . 2>/dev/null | head -20
pwdRepository: timoconnellaus/claude-code-discord-bot
Length of output: 220
🏁 Script executed:
# Try git to understand repo state
git status 2>&1 | head -10
git log --oneline -1 2>&1Repository: timoconnellaus/claude-code-discord-bot
Length of output: 220
Based on the web search results, I can verify the core claim despite repository access issues.
The shell injection vulnerability is confirmed and the suggested fix is sound.
The web search confirms:
child_process.exec()uses a shell by default — commands are interpreted by/bin/sh(Unix) orcmd.exe(Windows), enabling shell metacharacter injection- The vulnerable pattern matches your code — building
bd ${args.join(' ')}concatenates user input directly into a shell command string spawn()is the recommended safe alternative — it bypasses the shell and passes arguments as an array, preventing injection
The suggested diff correctly:
- Replaces
execwithspawn - Passes
argsas an array instead of concatenating into a string - Preserves your non-zero-exit-code behavior by handling the
closeevent - Maintains
stdout/stderrcapture
This aligns with official Node.js security guidance and OWASP recommendations for OS command injection prevention.
🤖 Prompt for AI Agents
In src/mcp/beads-server.ts around lines 33 to 51, the helper builds a shell
command string and uses exec (via execAsync), allowing shell injection when args
contain metacharacters; replace this with child_process.spawn (or spawnSync)
passing the command name "bd" and the args array directly (do NOT join into a
string and do not enable a shell), pass cwd in options, capture stdout and
stderr by collecting data from the child.stdout and child.stderr streams, and
resolve the Promise on the child's "close" event returning { stdout, stderr }
even if the exit code is non-zero (do not throw on non-zero exit) so behavior
remains the same while preventing shell injection.
| @@ -0,0 +1,122 @@ | |||
| import { AttachmentBuilder } from "discord.js"; | |||
| import { spawn } from "child_process"; | |||
There was a problem hiding this comment.
Remove unused import.
The spawn import from child_process is not used anywhere in this file.
Apply this diff:
-import { spawn } from "child_process";📝 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.
| import { spawn } from "child_process"; |
🤖 Prompt for AI Agents
In src/services/screenshot.ts around line 2, the imported symbol "spawn" from
"child_process" is unused; remove the unused import from the file (delete the
"spawn" import or the entire import statement if it only imports spawn) so the
file no longer imports from "child_process" unnecessarily and update any
remaining imports formatting if needed.
| const browser = await puppeteer.default.launch({ | ||
| headless: true, | ||
| args: ['--no-sandbox', '--disable-setuid-sandbox'] | ||
| }); |
There was a problem hiding this comment.
Security: Evaluate necessity of --no-sandbox flag.
The --no-sandbox flag disables Chromium's sandboxing, which is a significant security boundary. While this flag is sometimes required in containerized environments (Docker), it weakens security posture.
Consider:
- Documenting why this flag is needed in your deployment environment
- Making it configurable via environment variable
- Exploring alternative approaches (proper container capabilities, user namespaces)
Example of making it configurable:
const browser = await puppeteer.default.launch({
headless: true,
- args: ['--no-sandbox', '--disable-setuid-sandbox']
+ args: process.env.PUPPETEER_NO_SANDBOX === '1'
+ ? ['--no-sandbox', '--disable-setuid-sandbox']
+ : []
});📝 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.
| const browser = await puppeteer.default.launch({ | |
| headless: true, | |
| args: ['--no-sandbox', '--disable-setuid-sandbox'] | |
| }); | |
| const browser = await puppeteer.default.launch({ | |
| headless: true, | |
| args: process.env.PUPPETEER_NO_SANDBOX === '1' | |
| ? ['--no-sandbox', '--disable-setuid-sandbox'] | |
| : [] | |
| }); |
🤖 Prompt for AI Agents
In src/services/screenshot.ts around lines 42 to 45, the Puppeteer launch uses
the insecure "--no-sandbox" flag unconditionally; make this flag configurable
and documented: add an environment variable (e.g., PUPPETEER_NO_SANDBOX) that
controls whether to include "--no-sandbox" and "--disable-setuid-sandbox",
defaulting to false, update the code to conditionally append those args only
when the env var is true, and add a short comment noting why/when it should be
enabled and recommending container-capability/user-namespace alternatives for
production.
| export async function postScreenshot( | ||
| channel: any, | ||
| url: string, | ||
| screenshotBuffer: Buffer | ||
| ): Promise<void> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Improve type safety for channel parameter.
The channel parameter is typed as any, which bypasses TypeScript's type checking. Use Discord.js types for better safety and IDE support.
Apply this diff:
+import { AttachmentBuilder, TextBasedChannel } from "discord.js";
-import { AttachmentBuilder } from "discord.js";
export async function postScreenshot(
- channel: any,
+ channel: TextBasedChannel,
url: string,
screenshotBuffer: Buffer
): Promise<void> {Also update line 102:
export async function handleScreenshotDetection(
text: string,
- channel: any
+ channel: TextBasedChannel
): Promise<void> {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/services/screenshot.ts around lines 75-79, the channel parameter is typed
as any; replace it with the proper Discord.js text-channel type (import and use
TextBasedChannel from 'discord.js') so the signature becomes channel:
TextBasedChannel. Also update line 102 to use the typed channel's send method
signature (e.g., await channel.send({ files: [{ attachment: screenshotBuffer,
name: 'screenshot.png' }] }) or the appropriate options object) so the call
matches TextBasedChannel.send and TypeScript verifies the payload.
| async findRunningClaudeSessions(): Promise<DiscoveredSession[]> { | ||
| try { | ||
| // Find Claude processes (excluding the bot's own spawned processes) | ||
| const { stdout: psOutput } = await execAsync( | ||
| "ps aux | grep '/home/[^/]*/.local/bin/claude' | grep -v 'grep' | awk '{print $2}'" | ||
| ); | ||
|
|
||
| const pids = psOutput | ||
| .trim() | ||
| .split('\n') | ||
| .filter(Boolean) | ||
| .map(pid => parseInt(pid, 10)) | ||
| .filter(pid => !isNaN(pid)); | ||
|
|
||
| if (pids.length === 0) { | ||
| console.log('No running Claude Code sessions found'); | ||
| return []; | ||
| } |
There was a problem hiding this comment.
Platform portability and resilience concerns with shell command.
The ps command and grep pattern assume a Linux-specific Claude Code installation path (/home/[^/]*/.local/bin/claude). This will silently fail on:
- macOS (different home directory structure and installation paths)
- Windows (different process listing and paths)
- Custom Claude installations (different paths)
Additionally, returning an empty array on error (line 80) means failures are silent.
Consider:
- Documenting the platform requirements (Linux-only)
- Making the Claude path configurable via environment variable
- Logging more details when no sessions are found vs. when the command fails
- Using a more portable approach or platform detection
Example for making the path configurable:
+const claudeBinPattern = process.env.CLAUDE_BIN_PATTERN || '/home/[^/]*/.local/bin/claude';
const { stdout: psOutput } = await execAsync(
- "ps aux | grep '/home/[^/]*/.local/bin/claude' | grep -v 'grep' | awk '{print $2}'"
+ `ps aux | grep '${claudeBinPattern}' | grep -v 'grep' | awk '{print $2}'`
);| } | ||
|
|
||
| // Get working directories for each process | ||
| const { stdout: pwdxOutput } = await execAsync(`pwdx ${pids.join(' ')}`); |
There was a problem hiding this comment.
pwdx command is not portable.
The pwdx command is Linux-specific and not available on macOS by default. This compounds the platform portability issue noted in the previous comment.
On macOS, you would need to use lsof or /proc alternatives (if available).
| const workingDirectory = match[2].trim(); | ||
|
|
||
| // Skip if this is the discord bot's own directory | ||
| if (workingDirectory.includes('claude-code-discord-bot')) { |
There was a problem hiding this comment.
Hardcoded bot directory name reduces flexibility.
The filter string 'claude-code-discord-bot' is hardcoded. If the repository directory name changes, this filter will break.
Consider making it configurable or deriving it from the actual bot's working directory:
+const botDirName = path.basename(process.cwd());
+
// Skip if this is the discord bot's own directory
-if (workingDirectory.includes('claude-code-discord-bot')) {
+if (workingDirectory.includes(botDirName)) {
continue;
}📝 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 (workingDirectory.includes('claude-code-discord-bot')) { | |
| const botDirName = path.basename(process.cwd()); | |
| // Skip if this is the discord bot's own directory | |
| if (workingDirectory.includes(botDirName)) { | |
| continue; | |
| } |
🤖 Prompt for AI Agents
In src/services/session-discovery.ts around line 59, the filter uses the
hardcoded string 'claude-code-discord-bot' which reduces flexibility; change it
to use a configurable value or derive it dynamically (e.g., read a BOT_DIR_NAME
from config/env or compute path.basename(process.cwd()) or read the package.json
name) and use that variable in the includes check; ensure fallback behavior if
the config/env is missing and update any related tests or docs to reference the
new configuration key.
| let category: CategoryChannel | undefined = guild.channels.cache.find( | ||
| (channel) => | ||
| channel.name === 'claude-code' && channel.type === 4 // 4 = GUILD_CATEGORY | ||
| ) as CategoryChannel | undefined; | ||
|
|
||
| if (!category) { | ||
| console.log('Creating "claude-code" category'); | ||
| category = await guild.channels.create({ | ||
| name: 'claude-code', | ||
| type: 4, // GUILD_CATEGORY | ||
| }) as unknown as CategoryChannel; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use Discord.js ChannelType enum instead of magic numbers.
The code uses numeric literals (4 for category, 0 for text) instead of the ChannelType enum, reducing readability and maintainability.
Apply this diff:
+import type { Client, Guild, TextChannel, CategoryChannel, ChannelType } from 'discord.js';
-import type { Client, Guild, TextChannel, CategoryChannel } from 'discord.js';
let category: CategoryChannel | undefined = guild.channels.cache.find(
(channel) =>
- channel.name === 'claude-code' && channel.type === 4 // 4 = GUILD_CATEGORY
+ channel.name === 'claude-code' && channel.type === ChannelType.GuildCategory
) as CategoryChannel | undefined;
if (!category) {
console.log('Creating "claude-code" category');
category = await guild.channels.create({
name: 'claude-code',
- type: 4, // GUILD_CATEGORY
+ type: ChannelType.GuildCategory,
}) as unknown as CategoryChannel;
}Also update lines 164 and 185:
const existingChannel = guild.channels.cache.find(
(channel): channel is TextChannel =>
channel.name === session.channelName &&
- channel.type === 0 && // 0 = GUILD_TEXT
+ channel.type === ChannelType.GuildText &&
channel.parentId === category?.id
);
const newChannel = await guild.channels.create({
name: session.channelName,
- type: 0, // GUILD_TEXT
+ type: ChannelType.GuildText,
parent: category?.id,
topic: `Claude Code session for ${session.workingDirectory}`
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/services/session-discovery.ts around lines 147-158 (and also update lines
164 and 185), replace the magic numeric channel type literals with the
Discord.js ChannelType enum: import ChannelType from 'discord.js' (or add it to
the existing import), use ChannelType.GuildCategory instead of 4 when
checking/creating the category and ChannelType.GuildText (or
ChannelType.GuildText) instead of 0 for text channels; update the type
assertions/casts to match (e.g., as CategoryChannel) and ensure the create(...)
calls use type: ChannelType.GuildCategory / ChannelType.GuildText so the code is
readable and types remain correct.
1 participant
Summary
bun run restart)Notifications
The bot now @mentions users when:
This reduces spam by only notifying when action is needed or tasks are complete.
New Command
Test Improvements
Bot Management
restart.shscript for safe bot restartsbun run restartcommandTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Enhancements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.