Feat/update connection protocol #40
Conversation
… and the Ableton Remote Script to make it more robust. The previous implementation was fragile and could fail if multiple JSON messages were received in a single TCP packet, leading to parsing errors and connection hangs. This has been addressed by implementing a newline-delimited JSON protocol: - The `AbletonMCP_Remote_Script` now buffers incoming data and processes one command per line, separated by a newline character (`\n`). It also appends a newline to all its responses. - The `MCP_Server` is updated to append a newline to all outgoing commands and reads incoming responses until a newline is received. This ensures that message boundaries are clearly defined, making the connection significantly more reliable.
…s the socket communication protocol. New Feature: A `get_device_parameters` tool has been added, allowing the AI to read the parameters (name, value, min, max) of any device on any track in the Ableton Live session. This is the foundational step for enabling more advanced device analysis and manipulation. Protocol Enhancement: The underlying socket communication protocol has been updated to use newline-delimited JSON. Both the server and the remote script now append a newline character to every message, ensuring clear and reliable message boundaries. This replaces a more fragile implementation and makes the entire connection more robust and less prone to hanging.
WalkthroughSwitched client/server protocol to newline-delimited JSON over a persistent socket, updated per-command buffering and error handling, added a device-parameters retrieval command and helper, adjusted server send/receive framing and timeouts, and expanded the public API with get_device_parameters. Minor docstrings updated; receive_full_response default buffer size changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP_Server as MCP Server
participant Ableton as AbletonMCP Script
Client->>MCP_Server: send_command(JSON + "\n")
MCP_Server->>Ableton: TCP send(JSON + "\n")
Ableton-->>Ableton: Buffer, split by newline, parse JSON
Ableton->>Ableton: _process_command / get_device_parameters
Ableton-->>MCP_Server: JSON response + "\n"
MCP_Server-->>Client: receive_full_response (line-based)
sequenceDiagram
participant Client
participant MCP_Server as MCP Server
participant Ableton as AbletonMCP Script
Client->>MCP_Server: get_device_parameters(track_idx, device_idx)
MCP_Server->>Ableton: {"type":"get_device_parameters", ...}\n
Ableton->>Ableton: _get_device_parameters(track_idx, device_idx)
Ableton-->>MCP_Server: {status, data:{device_name, parameters[]}}\n
MCP_Server-->>Client: JSON string result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
MCP_Server/server.py (2)
104-106: Pass the chosen timeout to receive_full_responsePropagate the per-command timeout so non-modifying commands use 10s and modifying use 15s as intended.
- response_data = self.receive_full_response(self.sock) + response_data = self.receive_full_response(self.sock, buffer_size=4096, timeout=timeout)
77-85: Treat 'load_browser_item' as a modifying commandYou send the 'load_browser_item' command (e.g., from load_instrument_or_effect), but it’s not in the modifying list. This skips the deliberate delays and uses a shorter timeout, increasing the chance of timeouts and flakiness when loading devices.
is_modifying_command = command_type in [ "create_midi_track", "create_audio_track", "set_track_name", "create_clip", "add_notes_to_clip", "set_clip_name", "set_tempo", "fire_clip", "stop_clip", "set_device_parameter", - "start_playback", "stop_playback", "load_instrument_or_effect" + "start_playback", "stop_playback", "load_instrument_or_effect", + "load_browser_item" ]
🧹 Nitpick comments (5)
AbletonMCP_Remote_Script/__init__.py (4)
136-146: Avoid indefinite blocking on client sockets to enable clean shutdowns
client.settimeout(None)can leave client threads blocked on recv() during shutdown, since you don't close client sockets indisconnect(). Use a finite timeout and handlesocket.timeoutto periodically checkself.running.Apply this diff:
- client.settimeout(None) + # Use a finite timeout so we can periodically check self.running and exit cleanly + client.settimeout(1.0)
194-197: Handle socket timeout explicitly in client loopWith a finite timeout, add an
except socket.timeoutto continue the loop instead of treating it as an error.- except (socket.error, IOError) as e: + except socket.timeout: + # Periodic wake-up to re-check self.running + continue + except (socket.error, IOError) as e: self.log_message("Socket error in client handler: " + str(e)) break # Exit the loop on socket errors
154-159: Trim CR from lines to be robust against CRLF line endingsWhen splitting on newline, Windows clients may send CRLF. Safely strip a trailing '\r' before JSON parsing.
- command_str, buffer = buffer.split('\n', 1) + command_str, buffer = buffer.split('\n', 1) + # Normalize CRLF endings + if command_str.endswith('\r'): + command_str = command_str[:-1]
200-206: Replace bare except with explicit exception handlingBare
except:hides non-Exception control-flow (e.g., KeyboardInterrupt) and trips linters. CatchExceptionexplicitly and log.- except: - pass + except Exception as send_err: + self.log_message("Failed to send error response to client: " + str(send_err))MCP_Server/server.py (1)
60-66: Preserve original exception context using exception chainingUse
raise ... from eto retain the original traceback and satisfy Ruff B904.- except socket.timeout: - logger.error("Socket timeout while waiting for response from Ableton") - raise TimeoutError("Timeout waiting for Ableton response") - except Exception as e: - logger.error(f"Error receiving data: {str(e)}") - raise + except socket.timeout as e: + logger.error("Socket timeout while waiting for response from Ableton") + raise TimeoutError("Timeout waiting for Ableton response") from e + except Exception as e: + logger.error(f"Error receiving data: {str(e)}") + raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
AbletonMCP_Remote_Script/__init__.py(3 hunks)MCP_Server/server.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
AbletonMCP_Remote_Script/__init__.py
204-204: Do not use bare except
(E722)
MCP_Server/server.py
62-62: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (4)
AbletonMCP_Remote_Script/__init__.py (2)
229-239: New command route looks goodCommand routing for
get_device_parametersis consistent with existing patterns and returns a structured payload as expected.
428-462: Device parameter retrieval is correct and defensiveIndex validation and parameter extraction look solid. Returning name/value/min/max is a useful, stable surface for clients.
MCP_Server/server.py (2)
88-93: LGTM: newline-delimited framing on sendAppending a newline and sending UTF-8 encoded JSON matches the new protocol and aligns with the remote script.
265-284: LGTM: device parameters tool endpointThe public MCP tool delegates to the new remote command and returns a formatted JSON string with errors handled appropriately.
| def receive_full_response(self, sock, buffer_size=4096): | ||
| """Receive a newline-delimited JSON response from the socket.""" | ||
| buffer = b'' | ||
| sock.settimeout(15.0) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t override caller-selected timeouts inside receive_full_response
receive_full_response sets a hard 15s timeout, which overrides the per-command timeout set in send_command. Accept a timeout parameter and only set it if provided.
- def receive_full_response(self, sock, buffer_size=4096):
+ def receive_full_response(self, sock, buffer_size=4096, timeout=None):
"""Receive a newline-delimited JSON response from the socket."""
- buffer = b''
- sock.settimeout(15.0)
+ buffer = b''
+ if timeout is not None:
+ sock.settimeout(timeout)📝 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.
| def receive_full_response(self, sock, buffer_size=4096): | |
| """Receive a newline-delimited JSON response from the socket.""" | |
| buffer = b'' | |
| sock.settimeout(15.0) | |
| def receive_full_response(self, sock, buffer_size=4096, timeout=None): | |
| """Receive a newline-delimited JSON response from the socket.""" | |
| buffer = b'' | |
| if timeout is not None: | |
| sock.settimeout(timeout) | |
| # …rest of method… |
🤖 Prompt for AI Agents
In MCP_Server/server.py around lines 46 to 49, receive_full_response currently
unconditionally sets sock.settimeout(15.0); change the function signature to
accept a timeout parameter (e.g. timeout=None) and only call
sock.settimeout(timeout) if timeout is not None (remove the hardcoded 15s);
update callers (notably send_command) to pass their per-command timeout through
to receive_full_response so caller-selected timeouts are honored.
PR Type
Enhancement
Description
Implement newline-delimited JSON protocol for robust communication
Add
get_device_parameterstool for device parameter inspectionEnhance error handling and message buffering
Fix connection reliability issues
Diagram Walkthrough
File Walkthrough
__init__.py
Implement robust communication protocol and device parametersAbletonMCP_Remote_Script/init.py
_get_device_parametersmethod for device inspectionserver.py
Add device parameters tool and protocol updatesMCP_Server/server.py
get_device_parametersMCP tool functionSummary by CodeRabbit