Refactor: split Remote Script into handlers/, MCP server into tools/ + connection#63
Refactor: split Remote Script into handlers/, MCP server into tools/ + connection#63marcoloco23 wants to merge 7 commits intoahujasid:mainfrom
Conversation
…+ connection - Remote Script: extract 86 command implementations into 11 handler modules (session, tracks, clips, mixer, devices, browser, scenes, arrangement, audio, midi, automation); slim __init__.py to core + dispatch table. - MCP Server: extract AbletonConnection to connection.py; extract 43 tool definitions into 10 domain tool modules; slim server.py to FastMCP setup + get_ableton_connection + register_all(). - Behavior unchanged: same 43 active tools, same socket protocol. - Tested: get_session_info, get_all_tracks_info, get_track_info, get_return_tracks_info, create_scene, create_locator, set_track_color. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes command dispatch with a hot-reloadable handlers registry, splits domain logic into many handler modules, adds MODIFYING_COMMANDS and main-thread dispatching, introduces an AbletonConnection socket client and register_all tool registration, adds ASCII grid notation, CLI/tools, meter utility, and updates packaging and deployment metadata. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP_Server
participant AbletonConnection
participant RemoteScript
participant Dispatch
participant Handler
participant Live
Client->>MCP_Server: invoke mcp tool (e.g., create_midi_track)
MCP_Server->>AbletonConnection: get_ableton_connection()
AbletonConnection->>AbletonConnection: connect() if needed
MCP_Server->>AbletonConnection: send_command("create_midi_track", params)
AbletonConnection->>RemoteScript: send JSON over socket
RemoteScript->>Dispatch: classify command (is_known / is_modifying)
alt modifying
RemoteScript->>RemoteScript: schedule handler on main thread (response queue, timeout)
end
Dispatch->>Handler: execute(command, params, song, ctrl)
Handler->>Live: perform Live API calls (tracks/clips/devices)
Live-->>Handler: return result/state
Handler-->>Dispatch: return dict
Dispatch-->>RemoteScript: handler result
RemoteScript-->>AbletonConnection: send JSON response
AbletonConnection-->>MCP_Server: deliver response
MCP_Server-->>Client: return formatted result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Review Summary by QodoRefactor: Modularize Remote Script handlers and MCP Server tools with grid notation support
WalkthroughsDescriptionMajor refactoring to improve code maintainability and modularity: **Remote Script (AbletonMCP_Remote_Script/)** • Extracted 86 command implementations into 11 modular handler modules: session, tracks, clips, mixer, devices, browser, scenes, arrangement, audio, midi, automation • Reduced __init__.py from ~2000 lines to ~750 lines with core AbletonMCP class, socket server, and dispatch logic only • Centralized state-modifying command routing via _dispatch_modifying() function **MCP Server (MCP_Server/)** • Extracted AbletonConnection class to dedicated connection.py module for socket communication • Extracted 43 tool definitions into 10 domain-specific modules under tools/: session, tracks, clips, mixer, devices, browser, scenes, arrangement, audio, midi • Reduced server.py from ~400 lines to ~60 lines with FastMCP setup, lifespan management, and centralized tool registration **New Features** • Added grid_notation.py module providing ASCII grid notation system for MIDI pattern editing with bidirectional conversion • Comprehensive audio analysis with transient detection, frequency hints, and waveform characteristics • Support for multiple warping modes (beats, tones, texture, re_pitch, complex, complex_pro) **Project Updates** • Updated project name to ableton-mcp-macwhite and version to 2.0.0 • Updated CLI entry point and added Mac White as co-author All 43 active tools maintain same socket protocol and behavior. Verified against Live 12. Diagramflowchart LR
A["AbletonMCP<br/>Remote Script"] -->|"11 handler<br/>modules"| B["handlers/<br/>session, tracks,<br/>clips, mixer,<br/>devices, etc."]
C["MCP Server"] -->|"10 tool<br/>modules"| D["tools/<br/>session, tracks,<br/>clips, mixer,<br/>devices, etc."]
C -->|"connection<br/>management"| E["connection.py<br/>AbletonConnection"]
D -->|"MIDI grid<br/>conversion"| F["grid_notation.py<br/>ASCII patterns"]
B -->|"socket<br/>protocol"| E
File Changes1. AbletonMCP_Remote_Script/__init__.py
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyproject.toml (1)
31-33:⚠️ Potential issue | 🟡 MinorURLs point to the original repo, not the fork.
The Homepage and Bug Tracker still reference
ahujasid/ableton-mcp. If the package is truly rebranded, these should point to the fork. If contributing upstream, keep the original package identity instead.
🤖 Fix all issues with AI agents
In `@AbletonMCP_Remote_Script/handlers/audio.py`:
- Around line 478-517: The function export_track_audio currently accepts
start_time and end_time but ignores them; update export_track_audio to remove
these unused parameters (delete start_time and end_time from the signature and
all callers), update the docstring to reflect that it only freezes/unfreezes and
reports the expected frozen file path, and remove any messaging that implies a
time-range export; keep the freeze/unfreeze logic (track.freeze handling), the
returned result keys ("track_index", "output_path", "message"), and the
exception logging (ctrl.log_message/traceback) intact so behavior remains the
same aside from the cleaned API surface.
- Around line 434-455: The code incorrectly assigns to a non-existent writable
track.freeze property; replace direct assignments with the Song-level API calls
and read-only status: call song.freeze_track(track) to freeze and
song.unfreeze_track(track) to unfreeze (check for those methods with hasattr
before calling), then report status using track.is_frozen (not track.freeze);
similarly update export_track_audio() to invoke song.freeze_track/ unfreeze or
use the documented export API instead of setting track.freeze, and preserve the
existing validation, error logging (ctrl.log_message), and exception handling.
In `@AbletonMCP_Remote_Script/handlers/browser.py`:
- Around line 72-73: Fix the typo in the branch that checks path_parts: change
the string literal "nstruments" to "instruments" so the branch correctly sets
current_item = app.browser.instruments when path_parts[0].lower() equals
"instruments"; update the check that references path_parts[0].lower() and ensure
current_item remains app.browser.instruments (used later for child-walk logic)
so the else fallback no longer receives an extra "instruments" segment.
In `@AbletonMCP_Remote_Script/handlers/clips.py`:
- Around line 146-148: The clip loop update can raise a Live API error if you
set loop_start before loop_end when the new start is greater than the clip's
current loop_end (or vice versa); update set_clip_loop_points to read the
current clip via clip_slot.clip, compare desired loop_start and loop_end to the
clip's existing loop_start/loop_end, and assign the values in the safe order
(set loop_end first if increasing loop_start past current end, or set loop_start
first if decreasing loop_end below current start) so you never write a start
that exceeds the clip's current end (use the clip variable and its
loop_start/loop_end attributes to determine ordering).
In `@AbletonMCP_Remote_Script/handlers/devices.py`:
- Around line 142-172: The set_macro_value function assigns the incoming value
directly to macro_param.value which can be out-of-range; mirror the clamp used
in set_device_parameter by bounding the value with the parameter's min/max
before assignment (e.g., compute clamped = max(macro_param.min,
min(macro_param.max, value)) and assign macro_param.value = clamped), keep the
existing return shape using macro_param.value and preserve the existing
exception logging via ctrl.log_message.
In `@AbletonMCP_Remote_Script/handlers/midi.py`:
- Around line 134-164: The function duplicate_clip currently validates
dest_track/dest_clip but always calls
src_track.duplicate_clip_slot(source_clip), so duplication never goes to
dst_track; either remove the misleading destination params or implement real
cross-track duplication: in duplicate_clip, after obtaining src_slot =
src_track.clip_slots[source_clip] and dst_track = song.tracks[dest_track], get
dst_slot = dst_track.clip_slots[dest_clip], ensure src_slot.has_clip, then
create/replace a clip in dst_slot by copying the source clip's content (length,
notes, envelopes, loop) into dst_slot (use the Ableton API methods available in
your environment to create a clip and transfer notes/automation), set
song.view.highlighted_clip_slot to dst_slot and operate on dst_slot.clip (e.g.,
duplicate_loop) instead of the source; update logging in the exception block
(ctrl.log_message) accordingly and remove the src_track.duplicate_clip_slot call
when doing cross-track copying.
In `@AbletonMCP_Remote_Script/handlers/tracks.py`:
- Around line 206-223: The function group_tracks currently only sets
song.view.selected_track to song.tracks[track_indices[0]] and falsely reports
grouping; fix by implementing real grouping or renaming the function to reflect
selection. Specifically, in group_tracks validate track_indices as you already
do, then call the real grouping API (e.g., song.create_group(name) or
song.group_tracks(track_list) depending on your host API), move/assign the
specified song.tracks into the new group, set song.view.selected_track to the
new group object, and return an honest payload (e.g., {"grouped": True,
"group_name": name, "group_id": <id>, "track_count": len(track_indices)});
alternatively, if you cannot implement grouping now, rename group_tracks to
select_tracks or select_first_track and change the return to {"grouped": False,
"selected": True, "track_count": len(track_indices)} to avoid misleading
callers.
In `@MCP_Server/connection.py`:
- Around line 86-105: The modifying-command list in connection.py
(is_modifying_command) has diverged from the canonical MODIFYING_COMMANDS in
AbletonMCP_Remote_Script/__init__.py (missing load_browser_item, extra
create_locator), causing wrong timeout/delay behavior for load_browser_item;
extract the authoritative list into a shared module (e.g., shared_constants.py
or common/utils) and have both connection.py and
AbletonMCP_Remote_Script/__init__.py import MODIFYING_COMMANDS from there;
update the shared list to include load_browser_item (and remove or reconcile
create_locator to match intended semantics), and ensure
MCP_Server/tools/browser.py behavior (15s timeout, 0.1s pre-send delay) is
driven by that canonical set.
In `@MCP_Server/grid_notation.py`:
- Around line 82-97: PREFERRED_LABELS has an incorrect mapping for pitch 47
(currently 'LT'); update the dictionary entry for key 47 to map to 'MT' so it
matches DRUM_LABELS and correctly labels mid-tom hits; locate the
PREFERRED_LABELS dict and replace the 47: 'LT' pair with 47: 'MT' (ensure no
duplicate low-tom mapping remains).
- Around line 449-467: The call sites passing clip_length as the second
positional argument to notes_to_grid are misbinding clip_length to the is_drums
parameter (forcing drum mode); update the call that currently does
notes_to_grid(notes, clip_length) to omit the second positional arg or
explicitly pass is_drums=None (e.g. notes_to_grid(notes) or notes_to_grid(notes,
is_drums=None)), and if clip_length is required elsewhere, pass it to the
correct parameter or function instead (search for the notes_to_grid(...) call in
MIDI processing code and update that invocation).
In `@MCP_Server/tools/midi.py`:
- Line 40: The code calls notes_to_grid(notes, clip_length) which incorrectly
binds clip_length to the is_drums parameter; change the call to pass the correct
named arguments — either provide is_drums explicitly (e.g., notes_to_grid(notes,
is_drums=is_drums)) if you intended non-drum handling, or pass clip_length as
steps_per_beat by name (e.g., notes_to_grid(notes, steps_per_beat=clip_length))
if that was the intent; update the call that assigns grid to ensure
notes_to_grid receives the correct parameters.
In `@pyproject.toml`:
- Line 22: The console entry point key was renamed to "ableton-mcp-macwhite"
which is a breaking change; revert the pyproject.toml console_scripts entry back
to the original name "ableton-mcp" so it maps to MCP_Server.server:main (or if
you intended to add a non-breaking alias, keep the original "ableton-mcp =
MCP_Server.server:main" and add a second entry "ableton-mcp-macwhite =
MCP_Server.server:main" to preserve backward compatibility).
- Around line 2-4: Revert the identity-level changes in pyproject.toml: restore
the package name field from "ableton-mcp-macwhite" back to the original package
name (e.g., "ableton-mcp"), undo the version bump from "2.0.0" to the previous
upstream version, and put the original description back instead of the rewritten
string; ensure only non-functional refactor changes remain in this PR so the
package identity (name, version, description) is unchanged for upstream
consumers.
🟡 Minor comments (12)
AbletonMCP_Remote_Script/handlers/session.py-153-163 (1)
153-163:⚠️ Potential issue | 🟡 Minor
stop_arrangement_recordingalso stops playback — intentional?Lines 157-158 stop playback whenever the transport is running. A user may want to stop recording while keeping playback going (e.g., to audition what was just recorded). Consider making the stop-playback step optional or removing it entirely, since the caller can always stop playback separately.
AbletonMCP_Remote_Script/handlers/mixer.py-6-17 (1)
6-17:⚠️ Potential issue | 🟡 MinorNo input clamping on
volume— inconsistent withset_track_send.
set_track_send(line 76) clampsvalueto[0.0, 1.0], butset_track_volumepassesvolumedirectly to the Ableton API without any range check. The same applies toset_track_pan. If the intent is defensive validation, apply it uniformly; otherwise out-of-range values will surface as opaque Ableton API errors.MCP_Server/grid_notation.py-433-446 (1)
433-446:⚠️ Potential issue | 🟡 Minor
is_drum_trackheuristic may misclassify melodic content.The GM drum range (35–81) overlaps heavily with common melodic pitches (e.g., B2=59, C4=60, A5=81). A C-major scale from C3 to C5 would have all pitches in 48–72, yielding 100% drum-range overlap. The
len(pitches) > 1guard helps slightly, but a single-pitch melodic line (e.g., a bass drone) would returnFalseeven if it's a drum pitch.This is hard to solve perfectly, but worth noting that the heuristic can produce false positives for melodic content in the mid-range.
MCP_Server/connection.py-42-79 (1)
42-79:⚠️ Potential issue | 🟡 Minor
receive_full_responseoverrides the caller's socket timeout.Line 45 hardcodes
sock.settimeout(15.0), which silently overrides the differentiated timeout set bysend_commandat line 113 (15.0for modifying,10.0for read-only). Non-modifying commands will never use the intended 10-second timeout.Suggested fix: accept timeout as a parameter
- def receive_full_response(self, sock: socket.socket, buffer_size: int = 8192) -> bytes: + def receive_full_response(self, sock: socket.socket, buffer_size: int = 8192, timeout: float = 15.0) -> bytes: """Receive the complete response, potentially in multiple chunks.""" chunks = [] - sock.settimeout(15.0) + sock.settimeout(timeout)Then in
send_command:- self.sock.settimeout(15.0 if is_modifying_command else 10.0) - response_data = self.receive_full_response(self.sock) + timeout = 15.0 if is_modifying_command else 10.0 + response_data = self.receive_full_response(self.sock, timeout=timeout)MCP_Server/tools/arrangement.py-14-22 (1)
14-22:⚠️ Potential issue | 🟡 Minor
resultis assigned but never used.Line 18 captures the return value of
send_commandintoresult, but line 19 formats the return string using only the input parameters (name,position). If the server returns useful data (e.g., the actual locator index), it's being discarded.Either use values from
resultin the return string or drop the assignment.Proposed fix
- result = ableton.send_command("create_locator", {"position": position, "name": name}) - return f"Created locator '{name}' at position {position} beats" + ableton.send_command("create_locator", {"position": position, "name": name}) + return f"Created locator '{name}' at position {position} beats"MCP_Server/tools/arrangement.py-64-66 (1)
64-66:⚠️ Potential issue | 🟡 MinorUse the already-imported
jsonmodule instead of__import__("json").
jsonis imported at line 3 but line 66 uses__import__("json").loads(...)instead. This is unnecessary indirection.Proposed fix
- points = __import__("json").loads(automation_points) + points = json.loads(automation_points)AbletonMCP_Remote_Script/handlers/automation.py-81-112 (1)
81-112:⚠️ Potential issue | 🟡 MinorMissing validation that
start_time < end_time.
delete_time(line 118) andduplicate_time(line 135) both validatestart_time >= end_time, butclear_automationdoes not. Ifstart_time >= end_time, the computed duration (end_time - start_time) on line 101 will be zero or negative, which could produce unexpected behavior withinsert_step.Proposed fix
def clear_automation( song, track_index, parameter_name, start_time, end_time, ctrl=None ): """Clear automation for a parameter in a time range.""" try: + if start_time >= end_time: + raise ValueError("Start time must be less than end time") if track_index < 0 or track_index >= len(song.tracks): raise IndexError("Track index out of range")AbletonMCP_Remote_Script/handlers/automation.py-18-26 (1)
18-26:⚠️ Potential issue | 🟡 MinorSend parameter parsing is fragile for malformed input.
If the input is just
"send"(no letter suffix),parameter_path[-1]yields"d", silently resolving to Send D. Similarly, multi-character suffixes like"send 10"will pass"10"toord(), raising aTypeError.This is low-risk since callers typically pass well-formed names like
"Send A", but consider adding a guard:Proposed hardening
elif parameter_path.startswith("send"): send_char = ( parameter_path.split()[-1] if len(parameter_path.split()) > 1 else parameter_path[-1] ) - send_index = ord(send_char.upper()) - ord("A") + if len(send_char) != 1 or not send_char.isalpha(): + return None + send_index = ord(send_char.upper()) - ord("A") if 0 <= send_index < len(mixer.sends): parameter = mixer.sends[send_index]AbletonMCP_Remote_Script/handlers/midi.py-167-190 (1)
167-190:⚠️ Potential issue | 🟡 Minor
clip_indexis validated but unused incapture_midi.
capture_midivalidatestrack_indexandclip_index, butsong.capture_midi()is a global operation — it doesn't target a specific clip slot. The returnedhas_clipcheck on line 185 reads the slot after capture, but the capture itself may not place the clip there. This could mislead callers about where the captured MIDI ended up.AbletonMCP_Remote_Script/handlers/clips.py-28-49 (1)
28-49:⚠️ Potential issue | 🟡 Minor
add_notes_to_clipusesclip.set_notes()which replaces all notes — name implies appending.
set_notesoverwrites existing notes in the clip. Callers of the MCP tooladd_notes_to_clipmay expect additive behavior. If replacement is intentional, consider renaming toset_clip_notes; if additive behavior is intended, you'd need to read existing notes first and merge.AbletonMCP_Remote_Script/handlers/tracks.py-192-203 (1)
192-203:⚠️ Potential issue | 🟡 Minor
set_track_armdoesn't checkcan_be_armed, unlikearm_track.
arm_track(Line 158) guards withtrack.can_be_armed, butset_track_armskips this check. Arming a track that can't be armed will raise an opaque Live API error rather than a descriptive one.Proposed fix
def set_track_arm(song, track_index, arm, ctrl=None): """Set the arm state of a track.""" try: if track_index < 0 or track_index >= len(song.tracks): raise IndexError("Track index out of range") track = song.tracks[track_index] + if not track.can_be_armed: + raise Exception("Track cannot be armed") track.arm = bool(arm) return {"arm": track.arm}AbletonMCP_Remote_Script/handlers/midi.py-45-79 (1)
45-79:⚠️ Potential issue | 🟡 MinorMissing
is_audio_clipguard inget_notes_from_clip.
get_clip_notes(Line 18) checksclip.is_audio_clipbefore calling note APIs, butget_notes_from_clipdoes not. Callingget_notes_extendedon an audio clip will likely raise an opaque error from the Live API. Consider adding the same guard for consistency.Proposed fix
clip = clip_slot.clip + if clip.is_audio_clip: + raise Exception("Clip is not a MIDI clip") notes_data = clip.get_notes_extended(
🧹 Nitpick comments (14)
AbletonMCP_Remote_Script/handlers/browser.py (2)
194-260: Repetitive per-category try/except blocks could be consolidated.Each category (instruments, sounds, drums, audio_effects, midi_effects) follows the identical pattern: check type,
hasattr,_process_item, rename, append. A data-driven loop would cut ~60 lines down to ~10 and reduce the chance of copy-paste errors.♻️ Suggested consolidation
- if (category_type == "all" or category_type == "instruments") and hasattr( - app.browser, "instruments" - ): - try: - instruments = _process_item(app.browser.instruments) - if instruments: - instruments["name"] = "Instruments" - result["categories"].append(instruments) - except Exception as e: - if ctrl: - ctrl.log_message( - "Error processing instruments: {0}".format(str(e)) - ) - # ... (repeat for sounds, drums, audio_effects, midi_effects) + known_categories = [ + ("instruments", "Instruments"), + ("sounds", "Sounds"), + ("drums", "Drums"), + ("audio_effects", "Audio Effects"), + ("midi_effects", "MIDI Effects"), + ] + for attr, display_name in known_categories: + if (category_type == "all" or category_type == attr) and hasattr(app.browser, attr): + try: + item = _process_item(getattr(app.browser, attr)) + if item: + item["name"] = display_name + result["categories"].append(item) + except Exception as e: + if ctrl: + ctrl.log_message("Error processing {0}: {1}".format(attr, str(e)))
8-45:find_browser_item_by_uridepth tracking doesn't distinguish category iteration from child iteration.When recursing into the 5 top-level categories (lines 25-30),
current_depthis incremented the same way as when recursing into children. Withmax_depth=10, this is unlikely to be a problem in practice, but the depth semantics are imprecise — a single category hop plus 9 levels of children would hit the limit.MCP_Server/grid_notation.py (1)
270-273: UseOptional[int](orint | None) instead of implicitOptional.PEP 484 prohibits
int = Nonewithout an explicitOptionaltype. Since the file already imports fromtyping, this is straightforward.♻️ Fix type hints
+from typing import Optional + def notes_to_drum_grid( notes: list[dict], steps_per_beat: int = 4, - num_bars: int = None + num_bars: Optional[int] = None ) -> str:Apply the same change to
notes_to_melodic_grid,notes_to_grid, andparse_grid.Also applies to: 349-352
MCP_Server/tools/__init__.py (1)
9-31: Consider error isolation so a single broken module doesn't prevent all tools from registering.If any module's
importorregister()call raises (e.g., a syntax error in one tool file), all subsequent modules will be skipped. Wrapping each registration in a try/except with logging would improve resilience.♻️ Suggested resilient registration
+import logging + +logger = logging.getLogger("AbletonMCPServer") + +_MODULES = [ + "session", "tracks", "clips", "mixer", "devices", + "browser", "scenes", "arrangement", "audio", "midi", +] + def register_all(mcp: object, get_ableton_connection: GetAbletonConnection) -> None: """Register all tool modules with the given mcp and connection getter.""" - from . import session - from . import tracks - ... - session.register(mcp, get_ableton_connection) - tracks.register(mcp, get_ableton_connection) - ... + import importlib + for name in _MODULES: + try: + mod = importlib.import_module(f".{name}", __package__) + mod.register(mcp, get_ableton_connection) + except Exception: + logger.exception(f"Failed to register tool module: {name}")MCP_Server/tools/devices.py (1)
42-55:send_commandresult is discarded — success message won't reflect the actual clamped value.The handler returns
{"name": ..., "value": ..., "track_type": ...}with the actual value after clamping. Consider using it for a more accurate response.Suggested improvement
try: ableton = get_ableton_connection() - ableton.send_command("set_device_parameter", { + result = ableton.send_command("set_device_parameter", { "track_index": track_index, "device_index": device_index, "parameter_index": parameter_index, "value": value, "track_type": track_type, }) - return f"Set device {device_index} parameter {parameter_index} to {value}" + return f"Set parameter '{result.get('name', parameter_index)}' to {result.get('value', value)}"MCP_Server/connection.py (2)
18-30:connect()assumesself.sockbeing non-None means the connection is healthy.If a previous call set
self.sockto a socket object but the remote end closed the connection (without triggering an error on this side yet),connect()returnsTruewithout verifying the connection is actually alive. The send path does null outself.sockon errors, but there's a window where the socket is broken yet still truthy.This is mostly mitigated by
send_command's error handling (which nullsself.sockand raises), so a retry from the caller would reconnect. Noting for awareness.
125-140: Lost exception chains — useraise ... from efor proper tracebacks.All the
exceptblocks re-raise newExceptioninstances, discarding the original traceback chain. This makes debugging harder when issues occur in production.Example fix for one block (apply to all four)
except socket.timeout: logger.error("Socket timeout while waiting for response from Ableton") self.sock = None - raise Exception("Timeout waiting for Ableton response") + raise Exception("Timeout waiting for Ableton response") from None except (ConnectionError, BrokenPipeError, ConnectionResetError) as e: logger.error(f"Socket connection error: {str(e)}") self.sock = None - raise Exception(f"Connection to Ableton lost: {str(e)}") + raise Exception(f"Connection to Ableton lost: {str(e)}") from e except json.JSONDecodeError as e: logger.error(f"Invalid JSON response from Ableton: {str(e)}") self.sock = None - raise Exception(f"Invalid response from Ableton: {str(e)}") + raise Exception(f"Invalid response from Ableton: {str(e)}") from e except Exception as e: logger.error(f"Error communicating with Ableton: {str(e)}") self.sock = None - raise Exception(f"Communication error with Ableton: {str(e)}") + raise Exception(f"Communication error with Ableton: {str(e)}") from eAbletonMCP_Remote_Script/__init__.py (1)
393-751:_dispatch_modifying— consider a dispatch dictionary for maintainability.The 50+
if/returnchain is correct and comprehensive, but as the command set grows this will become harder to maintain and navigate. A dictionary-based dispatch mappingcommand_type → (handler_fn, param_extractor)would reduce boilerplate and make additions a one-liner. Not urgent for this refactor PR.MCP_Server/tools/tracks.py (1)
90-110:arm_trackandset_track_armoverlap — consider a note in the docstring.
arm_trackis effectivelyset_track_arm(..., arm=True). Both dispatch to different remote commands so this is fine functionally, but a brief cross-reference in the docstrings would help discoverability for tool consumers.Optional docstring tweak
`@mcp.tool`() def arm_track(ctx: Context, track_index: int) -> str: - """Arm a track for recording. Parameters: track_index.""" + """Arm a track for recording (always arms). See also set_track_arm to toggle. Parameters: track_index."""AbletonMCP_Remote_Script/handlers/devices.py (1)
100-139:get_macro_valuesandset_macro_valuedon't useresolve_track, unlike the other device functions.
get_device_parametersandset_device_parameteraccept atrack_typeparameter and resolve viaresolve_track, supporting return and master tracks. The macro functions are hardcoded tosong.tracksonly. If rack devices on return/master tracks need macro access in the future, this would need to change.Low impact since macro tools aren't currently exposed via MCP, but worth noting for consistency.
MCP_Server/tools/midi.py (1)
61-68: Silently swallowingcreate_clipfailure hides real errors.The intent is to tolerate "clip already exists," but this also swallows connection errors, timeouts, and other failures. At minimum, log the exception so debugging isn't a dead end.
Proposed fix
try: ableton.send_command("create_clip", { "track_index": track_index, "clip_index": clip_index, "length": length, }) - except Exception: - pass + except Exception as create_err: + logger.debug(f"create_clip skipped (may already exist): {create_err}")AbletonMCP_Remote_Script/handlers/audio.py (3)
79-86: Duplicatewarp_mode_mapinget_audio_clip_infoandanalyze_audio_clip.The same int→string mapping is defined in both functions. Extract to a module-level constant.
Proposed refactor
+WARP_MODE_MAP = { + 0: "beats", + 1: "tones", + 2: "texture", + 3: "re_pitch", + 4: "complex", + 5: "complex_pro", +} + + def get_audio_clip_info(song, track_index, clip_index, ctrl=None): ... - warp_mode_map = { - 0: "beats", - ... - } warp_mode = "unknown" if hasattr(clip, "warp_mode"): - warp_mode = warp_mode_map.get(clip.warp_mode, "unknown") + warp_mode = WARP_MODE_MAP.get(clip.warp_mode, "unknown")Apply the same change in
analyze_audio_clip.Also applies to: 231-238
488-494: Redundanthasattr+getattrforwas_frozen.
getattr(track, "freeze", False)already handles the missing-attribute case, making thehasattrcheck superfluous.Proposed simplification
- was_frozen = ( - getattr(track, "freeze", False) - if hasattr(track, "freeze") - else False - ) + was_frozen = getattr(track, "freeze", False)
217-431:analyze_audio_clipis ~200 lines — consider splitting into helpers.The function aggregates results from several independent analysis sections (transients, audio properties, frequency hints, waveform description). Each section could be a small helper function, improving readability and testability.
| def freeze_track(song, track_index, ctrl=None): | ||
| """Freeze a track.""" | ||
| try: | ||
| if track_index < 0 or track_index >= len(song.tracks): | ||
| raise IndexError("Track index out of range") | ||
| track = song.tracks[track_index] | ||
| if not getattr(track, "can_be_frozen", False) or not track.can_be_frozen: | ||
| raise Exception( | ||
| "Track cannot be frozen (may be a return or master track)" | ||
| ) | ||
| if not hasattr(track, "freeze"): | ||
| raise Exception("Freeze not available on this track") | ||
| track.freeze = True | ||
| return { | ||
| "track_index": track_index, | ||
| "frozen": True, | ||
| "track_name": track.name, | ||
| } | ||
| except Exception as e: | ||
| if ctrl: | ||
| ctrl.log_message("Error freezing track: " + str(e)) | ||
| raise |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Ableton Live Python track freeze method API documentation
💡 Result:
Ableton Live’s Python/Control Surface API (the same object model exposed to Max for Live as the Live Object Model / LOM) does not provide a Track.freeze() / freeze_track / unfreeze method you can call from Python.
What is documented/exposed for freezing on Track are only state/readiness properties:
can_be_frozen(read-only):1if the track can be frozen, else0[1]is_frozen(read-only, observable):1if the track is currently frozen [1]
And in the official Track function list, there is no freeze/unfreeze function (only things like create_audio_clip, create_midi_clip, stop_all_clips, etc.) [1].
For official context on what Live exposes via the API (LOM) and how it’s accessed (Max objects / JS LiveAPI—mirrors what Control Surface scripts use), see Ableton’s overview here [2].
Sources:
[1] Cycling ’74 Live Object Model — Track class reference (properties/functions list)
[2] Ableton Help — “Controlling Live using Max for Live” (LOM/API overview)
🏁 Script executed:
# Search the codebase for any other uses of track freeze or freeze-related code
rg -i "freeze" --type py -B 2 -A 2Repository: ahujasid/ableton-mcp
Length of output: 5548
The track.freeze = True assignment is not a valid Ableton Live API call and will not work.
According to Ableton's official Python/Control Surface API documentation, the Track object does not expose a writable freeze property or a freeze() method. The only documented freeze-related properties are:
can_be_frozen(read-only): whether the track can be frozenis_frozen(read-only): whether the track is currently frozen
The current implementation will fail silently or raise an error when executed. This also affects unfreeze_track() and export_track_audio(), which similarly attempt to set track.freeze.
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 438-438: Abstract raise to an inner function
(TRY301)
[warning] 438-438: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 441-443: Abstract raise to an inner function
(TRY301)
[warning] 441-443: Create your own exception
(TRY002)
[warning] 441-443: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 445-445: Abstract raise to an inner function
(TRY301)
[warning] 445-445: Create your own exception
(TRY002)
[warning] 445-445: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 447-451: Consider moving this statement to an else block
(TRY300)
🤖 Prompt for AI Agents
In `@AbletonMCP_Remote_Script/handlers/audio.py` around lines 434 - 455, The code
incorrectly assigns to a non-existent writable track.freeze property; replace
direct assignments with the Song-level API calls and read-only status: call
song.freeze_track(track) to freeze and song.unfreeze_track(track) to unfreeze
(check for those methods with hasattr before calling), then report status using
track.is_frozen (not track.freeze); similarly update export_track_audio() to
invoke song.freeze_track/ unfreeze or use the documented export API instead of
setting track.freeze, and preserve the existing validation, error logging
(ctrl.log_message), and exception handling.
| def export_track_audio( | ||
| song, track_index, output_path, start_time, end_time, ctrl=None | ||
| ): | ||
| """Export track audio to WAV file (freeze and report path).""" | ||
| try: | ||
| if track_index < 0 or track_index >= len(song.tracks): | ||
| raise IndexError("Track index out of range") | ||
| track = song.tracks[track_index] | ||
| if not getattr(track, "can_be_frozen", False) or not track.can_be_frozen: | ||
| raise Exception( | ||
| "Track cannot be frozen (may be a return or master track)" | ||
| ) | ||
| was_frozen = ( | ||
| getattr(track, "freeze", False) | ||
| if hasattr(track, "freeze") | ||
| else False | ||
| ) | ||
| if not was_frozen: | ||
| track.freeze = True | ||
| result = { | ||
| "track_index": track_index, | ||
| "output_path": output_path, | ||
| "message": ( | ||
| "Track frozen. Frozen audio file should be in: " | ||
| "Project/Samples/Frozen/ folder. " | ||
| "Copy it manually to: " | ||
| + str(output_path) | ||
| + ". " | ||
| "For fully automatic export, use Ableton's built-in " | ||
| "Export Audio/Video feature." | ||
| ), | ||
| } | ||
| if not was_frozen and hasattr(track, "freeze"): | ||
| track.freeze = False | ||
| return result | ||
| except Exception as e: | ||
| if ctrl: | ||
| ctrl.log_message("Error exporting track audio: " + str(e)) | ||
| ctrl.log_message(traceback.format_exc()) | ||
| raise |
There was a problem hiding this comment.
start_time and end_time are accepted but completely ignored.
These parameters suggest time-range export but the function only freezes/unfreezes the track. Callers will believe they've exported a specific range when they haven't. Either implement range support or remove these parameters to avoid misleading the API surface.
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 479-479: Unused function argument: start_time
(ARG001)
[warning] 479-479: Unused function argument: end_time
(ARG001)
[warning] 484-484: Abstract raise to an inner function
(TRY301)
[warning] 484-484: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 487-489: Abstract raise to an inner function
(TRY301)
[warning] 487-489: Create your own exception
(TRY002)
[warning] 487-489: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 512-512: Consider moving this statement to an else block
(TRY300)
🤖 Prompt for AI Agents
In `@AbletonMCP_Remote_Script/handlers/audio.py` around lines 478 - 517, The
function export_track_audio currently accepts start_time and end_time but
ignores them; update export_track_audio to remove these unused parameters
(delete start_time and end_time from the signature and all callers), update the
docstring to reflect that it only freezes/unfreezes and reports the expected
frozen file path, and remove any messaging that implies a time-range export;
keep the freeze/unfreeze logic (track.freeze handling), the returned result keys
("track_index", "output_path", "message"), and the exception logging
(ctrl.log_message/traceback) intact so behavior remains the same aside from the
cleaned API surface.
| clip = clip_slot.clip | ||
| clip.loop_start = loop_start | ||
| clip.loop_end = loop_end |
There was a problem hiding this comment.
set_clip_loop_points can fail when the new start exceeds the current end (or vice versa).
Setting loop_start before loop_end will raise a Live API error if the new loop_start is greater than the clip's current loop_end. The safe approach is to set the values in the correct order depending on direction of change.
Proposed fix
clip = clip_slot.clip
- clip.loop_start = loop_start
- clip.loop_end = loop_end
+ # Set in the correct order to avoid invalid intermediate state
+ if loop_end > clip.loop_end:
+ clip.loop_end = loop_end
+ clip.loop_start = loop_start
+ else:
+ clip.loop_start = loop_start
+ clip.loop_end = loop_end📝 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.
| clip = clip_slot.clip | |
| clip.loop_start = loop_start | |
| clip.loop_end = loop_end | |
| clip = clip_slot.clip | |
| # Set in the correct order to avoid invalid intermediate state | |
| if loop_end > clip.loop_end: | |
| clip.loop_end = loop_end | |
| clip.loop_start = loop_start | |
| else: | |
| clip.loop_start = loop_start | |
| clip.loop_end = loop_end |
🤖 Prompt for AI Agents
In `@AbletonMCP_Remote_Script/handlers/clips.py` around lines 146 - 148, The clip
loop update can raise a Live API error if you set loop_start before loop_end
when the new start is greater than the clip's current loop_end (or vice versa);
update set_clip_loop_points to read the current clip via clip_slot.clip, compare
desired loop_start and loop_end to the clip's existing loop_start/loop_end, and
assign the values in the safe order (set loop_end first if increasing loop_start
past current end, or set loop_start first if decreasing loop_end below current
start) so you never write a start that exceeds the clip's current end (use the
clip variable and its loop_start/loop_end attributes to determine ordering).
| def set_macro_value(song, track_index, device_index, macro_index, value, ctrl=None): | ||
| """Set the value of a specific macro control on a rack device.""" | ||
| try: | ||
| if track_index < 0 or track_index >= len(song.tracks): | ||
| raise IndexError("Track index out of range") | ||
| track = song.tracks[track_index] | ||
| if device_index < 0 or device_index >= len(track.devices): | ||
| raise IndexError("Device index out of range") | ||
| device = track.devices[device_index] | ||
| if not hasattr(device, "macros_mapped"): | ||
| raise Exception("Device is not a rack (no macros)") | ||
| if macro_index < 0 or macro_index > 7: | ||
| raise IndexError("Macro index must be 0-7") | ||
| param_index = macro_index + 1 | ||
| if param_index >= len(device.parameters): | ||
| raise Exception( | ||
| "Macro {0} not available on this device".format(macro_index + 1) | ||
| ) | ||
| macro_param = device.parameters[param_index] | ||
| macro_param.value = value | ||
| return { | ||
| "track_index": track_index, | ||
| "device_index": device_index, | ||
| "macro_index": macro_index, | ||
| "macro_name": macro_param.name, | ||
| "value": macro_param.value, | ||
| } | ||
| except Exception as e: | ||
| if ctrl: | ||
| ctrl.log_message("Error setting macro value: " + str(e)) | ||
| raise |
There was a problem hiding this comment.
set_macro_value does not clamp the value to the parameter's range.
set_device_parameter (line 88) properly clamps: max(param.min, min(param.max, value)), but set_macro_value (line 161) assigns value directly. An out-of-range value could cause unexpected behavior or an error from the Ableton API.
Proposed fix
macro_param = device.parameters[param_index]
- macro_param.value = value
+ macro_param.value = max(macro_param.min, min(macro_param.max, value))🧰 Tools
🪛 Ruff (0.14.14)
[warning] 146-146: Abstract raise to an inner function
(TRY301)
[warning] 146-146: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 149-149: Abstract raise to an inner function
(TRY301)
[warning] 149-149: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 152-152: Abstract raise to an inner function
(TRY301)
[warning] 152-152: Create your own exception
(TRY002)
[warning] 152-152: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 154-154: Abstract raise to an inner function
(TRY301)
[warning] 154-154: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 157-159: Abstract raise to an inner function
(TRY301)
[warning] 157-159: Create your own exception
(TRY002)
[warning] 162-168: Consider moving this statement to an else block
(TRY300)
🤖 Prompt for AI Agents
In `@AbletonMCP_Remote_Script/handlers/devices.py` around lines 142 - 172, The
set_macro_value function assigns the incoming value directly to
macro_param.value which can be out-of-range; mirror the clamp used in
set_device_parameter by bounding the value with the parameter's min/max before
assignment (e.g., compute clamped = max(macro_param.min, min(macro_param.max,
value)) and assign macro_param.value = clamped), keep the existing return shape
using macro_param.value and preserve the existing exception logging via
ctrl.log_message.
| PREFERRED_LABELS = { | ||
| 36: 'KK', 35: 'KK', # Kicks | ||
| 38: 'SN', 40: 'SN', # Snares | ||
| 37: 'SS', # Sidestick | ||
| 39: 'CL', # Clap | ||
| 42: 'HC', 44: 'HC', # Hi-hat closed | ||
| 46: 'HO', # Hi-hat open | ||
| 45: 'LT', 47: 'LT', # Low tom | ||
| 48: 'MT', # Mid tom | ||
| 50: 'HT', # High tom | ||
| 41: 'FT', 43: 'FT', # Floor tom | ||
| 49: 'CR', 57: 'CR', # Crash | ||
| 51: 'RD', 59: 'RD', # Ride | ||
| 53: 'RB', # Ride bell | ||
| 56: 'CB', # Cowbell | ||
| } |
There was a problem hiding this comment.
Bug: PREFERRED_LABELS maps pitch 47 to 'LT' — should be 'MT' (Mid tom).
DRUM_LABELS correctly maps 'MT': 47, but PREFERRED_LABELS at line 89 maps 47: 'LT', duplicating the Low tom label. This means mid-tom hits will be mislabeled in the grid output.
🐛 Fix the label mapping
- 45: 'LT', 47: 'LT', # Low tom
- 48: 'MT', # Mid tom
+ 45: 'LT', # Low tom
+ 47: 'MT', 48: 'MT', # Mid tom🤖 Prompt for AI Agents
In `@MCP_Server/grid_notation.py` around lines 82 - 97, PREFERRED_LABELS has an
incorrect mapping for pitch 47 (currently 'LT'); update the dictionary entry for
key 47 to map to 'MT' so it matches DRUM_LABELS and correctly labels mid-tom
hits; locate the PREFERRED_LABELS dict and replace the 47: 'LT' pair with 47:
'MT' (ensure no duplicate low-tom mapping remains).
| def notes_to_grid(notes: list[dict], is_drums: bool = None, steps_per_beat: int = 4) -> str: | ||
| """ | ||
| Convert notes to appropriate grid format. | ||
|
|
||
| Args: | ||
| notes: List of note dicts | ||
| is_drums: Force drum mode (auto-detect if None) | ||
| steps_per_beat: Grid resolution | ||
|
|
||
| Returns: | ||
| ASCII grid string | ||
| """ | ||
| if is_drums is None: | ||
| is_drums = is_drum_track(notes) | ||
|
|
||
| if is_drums: | ||
| return notes_to_drum_grid(notes, steps_per_beat) | ||
| else: | ||
| return notes_to_melodic_grid(notes, steps_per_beat) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'notes_to_grid' --type=py -C 3Repository: ahujasid/ableton-mcp
Length of output: 1712
notes_to_grid receives clip_length (a float) as the second positional argument, misinterpreting it as is_drums.
Line 40 in MCP_Server/tools/midi.py calls notes_to_grid(notes, clip_length), passing clip_length (a float defaulting to 4.0) to the is_drums: bool parameter. Since any non-zero float is truthy in Python, drum mode is forced on every clip, regardless of actual content.
Pass no second argument or use is_drums=None explicitly: notes_to_grid(notes, is_drums=None) (or omit it entirely since None is the default).
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 449-449: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 Prompt for AI Agents
In `@MCP_Server/grid_notation.py` around lines 449 - 467, The call sites passing
clip_length as the second positional argument to notes_to_grid are misbinding
clip_length to the is_drums parameter (forcing drum mode); update the call that
currently does notes_to_grid(notes, clip_length) to omit the second positional
arg or explicitly pass is_drums=None (e.g. notes_to_grid(notes) or
notes_to_grid(notes, is_drums=None)), and if clip_length is required elsewhere,
pass it to the correct parameter or function instead (search for the
notes_to_grid(...) call in MIDI processing code and update that invocation).
| notes = result.get("notes", []) | ||
| clip_length = result.get("clip_length", 4.0) | ||
| clip_name = result.get("clip_name", "Unknown") | ||
| grid = notes_to_grid(notes, clip_length) |
There was a problem hiding this comment.
Bug: clip_length is passed as is_drums parameter to notes_to_grid.
notes_to_grid signature (from grid_notation.py:448) is notes_to_grid(notes, is_drums=None, steps_per_beat=4). Passing clip_length (a float) as the second positional arg sets is_drums to a truthy value, forcing drum-grid format for every clip.
🐛 Proposed fix
- grid = notes_to_grid(notes, clip_length)
+ grid = notes_to_grid(notes)📝 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.
| grid = notes_to_grid(notes, clip_length) | |
| grid = notes_to_grid(notes) |
🤖 Prompt for AI Agents
In `@MCP_Server/tools/midi.py` at line 40, The code calls notes_to_grid(notes,
clip_length) which incorrectly binds clip_length to the is_drums parameter;
change the call to pass the correct named arguments — either provide is_drums
explicitly (e.g., notes_to_grid(notes, is_drums=is_drums)) if you intended
non-drum handling, or pass clip_length as steps_per_beat by name (e.g.,
notes_to_grid(notes, steps_per_beat=clip_length)) if that was the intent; update
the call that assigns grid to ensure notes_to_grid receives the correct
parameters.
| name = "ableton-mcp-macwhite" | ||
| version = "2.0.0" | ||
| description = "Extended Ableton Live MCP integration for Mac White production system" |
There was a problem hiding this comment.
Package rebranding doesn't align with the stated "non-functional refactor" scope.
Changing the package name to ableton-mcp-macwhite, bumping to 2.0.0, and rewriting the description are identity-level changes that go beyond a structural refactor. This will create a different package on PyPI and break any existing consumers that depend on ableton-mcp. If the intent is to contribute this refactor upstream, keep the original name/version and let the upstream maintainer decide on versioning.
🤖 Prompt for AI Agents
In `@pyproject.toml` around lines 2 - 4, Revert the identity-level changes in
pyproject.toml: restore the package name field from "ableton-mcp-macwhite" back
to the original package name (e.g., "ableton-mcp"), undo the version bump from
"2.0.0" to the previous upstream version, and put the original description back
instead of the rewritten string; ensure only non-functional refactor changes
remain in this PR so the package identity (name, version, description) is
unchanged for upstream consumers.
|
|
||
| [project.scripts] | ||
| ableton-mcp = "MCP_Server.server:main" | ||
| ableton-mcp-macwhite = "MCP_Server.server:main" |
There was a problem hiding this comment.
Console entry point renamed — this is a breaking change.
Renaming the script from ableton-mcp to ableton-mcp-macwhite breaks any existing user workflow or documentation that references the old command. If this PR is intended as a non-breaking refactor, this should remain ableton-mcp.
🤖 Prompt for AI Agents
In `@pyproject.toml` at line 22, The console entry point key was renamed to
"ableton-mcp-macwhite" which is a breaking change; revert the pyproject.toml
console_scripts entry back to the original name "ableton-mcp" so it maps to
MCP_Server.server:main (or if you intended to add a non-breaking alias, keep the
original "ableton-mcp = MCP_Server.server:main" and add a second entry
"ableton-mcp-macwhite = MCP_Server.server:main" to preserve backward
compatibility).
Code Review by Qodo
1. Tools echo raw exceptions
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| def get_track_info(ctx: Context, track_index: int) -> str: | ||
| """Get detailed information about a specific track. Parameters: track_index.""" | ||
| try: | ||
| ableton = get_ableton_connection() | ||
| result = ableton.send_command("get_track_info", {"track_index": track_index}) | ||
| return json.dumps(result, indent=2) | ||
| except Exception as e: | ||
| logger.error(f"Error getting track info from Ableton: {str(e)}") | ||
| return f"Error getting track info: {str(e)}" |
There was a problem hiding this comment.
1. Tools echo raw exceptions 📘 Rule violation ⛨ Security
• Multiple MCP tools return strings like Error ...: {str(e)} directly to the caller, exposing
internal exception details to end users.
• This conflicts with secure error handling requirements and may reveal system internals or
sensitive operational information.
• Detailed exception information should be logged internally, while the tool response should be
generic (optionally with an error code).
Agent Prompt
## Issue description
Tool functions return raw exception text to the user, which can leak internal system information.
## Issue Context
Compliance requires generic user-facing errors and detailed errors only in internal logs.
## Fix Focus Areas
- MCP_Server/tools/tracks.py[14-22]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| command = {"type": command_type, "params": params or {}} | ||
| 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", | ||
| "arm_track", "disarm_track", "set_arrangement_overdub", | ||
| "start_arrangement_recording", "stop_arrangement_recording", | ||
| "set_loop_start", "set_loop_end", "set_loop_length", "set_playback_position", | ||
| "create_scene", "delete_scene", "duplicate_scene", "trigger_scene", "set_scene_name", | ||
| "set_track_color", "set_clip_color", | ||
| "quantize_clip", "transpose_clip", "duplicate_clip", | ||
| "group_tracks", "set_track_volume", "set_track_pan", "set_track_mute", "set_track_solo", | ||
| "load_audio_sample", "set_warp_mode", "set_clip_warp", "crop_clip", "reverse_clip", | ||
| "set_clip_loop_points", "set_clip_start_marker", "set_clip_end_marker", "set_track_send", | ||
| "copy_clip_to_arrangement", "create_automation", "clear_automation", | ||
| "delete_time", "duplicate_time", "insert_silence", "create_locator", | ||
| "delete_clip", "set_metronome", "tap_tempo", "set_macro_value", "capture_midi", "apply_groove", | ||
| "freeze_track", "unfreeze_track", "export_track_audio", | ||
| "create_return_track", "delete_track", "duplicate_track", "set_track_arm", | ||
| ] | ||
| try: | ||
| logger.info(f"Sending command: {command_type} with params: {params}") | ||
| self.sock.sendall(json.dumps(command).encode("utf-8")) | ||
| logger.info("Command sent, waiting for response...") | ||
| if is_modifying_command: | ||
| import time | ||
| time.sleep(0.1) | ||
| self.sock.settimeout(15.0 if is_modifying_command else 10.0) | ||
| response_data = self.receive_full_response(self.sock) | ||
| logger.info(f"Received {len(response_data)} bytes of data") |
There was a problem hiding this comment.
2. send_command logs params values 📘 Rule violation ⛨ Security
• The connection layer logs full command parameters (params) at INFO level, which can include sensitive data such as filesystem paths (e.g., file_path) or other user-provided content. • Logs are also unstructured free-form strings, reducing auditability and increasing the risk of accidental sensitive-data leakage. • Logging should be structured and should redact or omit sensitive fields/values.
Agent Prompt
## Issue description
`send_command()` logs full `params` values, which can include sensitive data (e.g., local filesystem paths). Logging is also unstructured.
## Issue Context
Compliance requires avoiding sensitive data in logs and keeping logs useful for auditing.
## Fix Focus Areas
- MCP_Server/connection.py[85-115]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if path_parts[0].lower() == "nstruments": | ||
| current_item = app.browser.instruments | ||
| elif path_parts[0].lower() == "sounds": | ||
| current_item = app.browser.sounds | ||
| elif path_parts[0].lower() == "drums": | ||
| current_item = app.browser.drums | ||
| elif path_parts[0].lower() == "audio_effects": | ||
| current_item = app.browser.audio_effects | ||
| elif path_parts[0].lower() == "midi_effects": | ||
| current_item = app.browser.midi_effects | ||
| else: | ||
| current_item = app.browser.instruments | ||
| path_parts = ["instruments"] + path_parts |
There was a problem hiding this comment.
3. Browser instruments typo 🐞 Bug ✓ Correctness
• get_browser_item checks for the category string "nstruments" (missing the leading "i"), so paths starting with "instruments/..." won’t match the intended branch. • This can lead to incorrect path normalization (prefixing "instruments" again) and confusing/incorrect lookups when clients specify explicit instrument paths. • The command router calls this handler directly for get_browser_item, so the bug is user-facing.
Agent Prompt
## Issue description
`get_browser_item` incorrectly checks for the string `"nstruments"` instead of `"instruments"`, breaking explicit instrument-path routing.
## Issue Context
This handler is invoked for `get_browser_item` commands and uses the first path segment to select the browser root category.
## Fix Focus Areas
- AbletonMCP_Remote_Script/handlers/browser.py[69-84]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| def send_command(self, command_type: str, params: Dict[str, Any] = None) -> Dict[str, Any]: | ||
| """Send a command to Ableton and return the response.""" | ||
| if not self.sock and not self.connect(): | ||
| raise ConnectionError("Not connected to Ableton") | ||
| command = {"type": command_type, "params": params or {}} | ||
| 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", | ||
| "arm_track", "disarm_track", "set_arrangement_overdub", | ||
| "start_arrangement_recording", "stop_arrangement_recording", | ||
| "set_loop_start", "set_loop_end", "set_loop_length", "set_playback_position", | ||
| "create_scene", "delete_scene", "duplicate_scene", "trigger_scene", "set_scene_name", | ||
| "set_track_color", "set_clip_color", | ||
| "quantize_clip", "transpose_clip", "duplicate_clip", | ||
| "group_tracks", "set_track_volume", "set_track_pan", "set_track_mute", "set_track_solo", | ||
| "load_audio_sample", "set_warp_mode", "set_clip_warp", "crop_clip", "reverse_clip", | ||
| "set_clip_loop_points", "set_clip_start_marker", "set_clip_end_marker", "set_track_send", | ||
| "copy_clip_to_arrangement", "create_automation", "clear_automation", | ||
| "delete_time", "duplicate_time", "insert_silence", "create_locator", | ||
| "delete_clip", "set_metronome", "tap_tempo", "set_macro_value", "capture_midi", "apply_groove", | ||
| "freeze_track", "unfreeze_track", "export_track_audio", | ||
| "create_return_track", "delete_track", "duplicate_track", "set_track_arm", | ||
| ] | ||
| try: | ||
| logger.info(f"Sending command: {command_type} with params: {params}") | ||
| self.sock.sendall(json.dumps(command).encode("utf-8")) | ||
| logger.info("Command sent, waiting for response...") | ||
| if is_modifying_command: | ||
| import time | ||
| time.sleep(0.1) | ||
| self.sock.settimeout(15.0 if is_modifying_command else 10.0) | ||
| response_data = self.receive_full_response(self.sock) | ||
| logger.info(f"Received {len(response_data)} bytes of data") | ||
| response = json.loads(response_data.decode("utf-8")) | ||
| logger.info(f"Response parsed, status: {response.get('status', 'unknown')}") | ||
| if response.get("status") == "error": | ||
| logger.error(f"Ableton error: {response.get('message')}") | ||
| raise Exception(response.get("message", "Unknown error from Ableton")) | ||
| if is_modifying_command: | ||
| import time | ||
| time.sleep(0.1) | ||
| return response.get("result", {}) |
There was a problem hiding this comment.
4. Shared socket race 🐞 Bug ⛯ Reliability
• The MCP server keeps a single global AbletonConnection and all tools reuse its single underlying socket. • send_command() performs a request/response exchange on that socket without any locking, so concurrent tool calls can interleave writes/reads and corrupt the JSON stream. • When this happens, responses can be mismatched, parsing can fail, or callers can hang waiting for the “wrong” response.
Agent Prompt
## Issue description
All tool calls share a single global socket connection to Ableton, but the request/response protocol is not concurrency-safe. Concurrent calls can interleave and break message boundaries.
## Issue Context
FastMCP servers commonly handle multiple tool invocations concurrently. Without a lock, `send_command()` can overlap across requests.
## Fix Focus Areas
- MCP_Server/connection.py[81-140]
- MCP_Server/server.py[19-85]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
- Add handlers/dispatch.py for hot-reloadable command registry (no restart needed) - Add set_return_track_name and load_on_return_track handlers - Add delete_device, delete_chain_device, move_device support - Add device parameter get/set for chain devices inside racks - Add ableton-cli.py for direct command-line control of Ableton - Wire new commands into __init__.py dispatch and connection.py - Add DEPLOYMENT.md and server_config.json Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Introduce view switching and arrangement clip recording commands, add rack/device note-access improvements from upstream PRs, and add grid preview tooling to make clip authoring and inspection more intuitive. Co-authored-by: Cursor <cursoragent@cursor.com>
Include a standalone script to sample track/output meters over time and report relative peak/average levels with Ableton fader dB context for quick mix diagnostics. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (18)
AbletonMCP_Remote_Script/handlers/browser.py (2)
298-317: Enumerating all non-underscore browser attributes may surface non-category items.The loop at line 298 iterates every public attribute of
app.browserand attempts to treat it as a browsable category. This could include methods, properties, or other non-browsable objects. While_process_itemis defensive, this may produce noise in the output. Consider maintaining an explicit allowlist of known additional categories, or filtering for attributes that actually havechildren.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/handlers/browser.py` around lines 298 - 317, The loop over browser_attrs treating every public attribute of app.browser as a category can surface methods or non-browsable objects; restrict the set to only real categories by filtering attributes that actually expose children (or use an explicit allowlist) before calling _process_item. Update the block that iterates browser_attrs (and the getattr(app.browser, attr) usage) to skip attrs that lack a "children" attribute or are callable, or replace browser_attrs with a small explicit list of known extra categories, and keep the existing ctrl.log_message error handling for anything unexpected.
85-87: Silent fallback toinstrumentsfor unrecognized path roots may confuse users.When the first path segment doesn't match any known category, the code defaults to
app.browser.instrumentsand prepends"instruments"to the path. This means a typo like"insruments/bass"silently navigatesinstruments/instruments/insruments/bassand likely fails deeper with a misleading "Path part not found" error. Consider returning an explicit error instead.Proposed fix
else: - current_item = app.browser.instruments - path_parts = ["instruments"] + path_parts + return { + "uri": uri, + "path": path, + "found": False, + "error": "Unknown category: '{0}'. Try: instruments, sounds, drums, audio_effects, midi_effects, plugins".format(path_parts[0]), + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/handlers/browser.py` around lines 85 - 87, The code silently falls back to app.browser.instruments when the first path segment is unrecognized (setting current_item = app.browser.instruments and path_parts = ["instruments"] + path_parts); change this to validate the first path segment and return or raise an explicit error instead of mutating path_parts and defaulting to instruments. Locate the logic that branches on the first path segment (the block that sets current_item and path_parts) and replace the fallback branch with a clear error path (e.g., raise ValueError or return an error response) that includes the invalid root segment so callers see a precise "unrecognized root: <segment>" message rather than silently redirecting to app.browser.instruments.AbletonMCP_Remote_Script/__init__.py (2)
392-455: Duplicated main-thread scheduling boilerplate between static and dynamic dispatch paths.Lines 393–420 (static MODIFYING_COMMANDS) and 423–451 (dynamic modifying dispatch) are nearly identical: create a
response_queue, define a task closure,schedule_message,AssertionErrorfallback, andqueue.get(timeout=10.0). Consider extracting a helper like_run_on_main_thread(self, task_fn)to eliminate the duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/__init__.py` around lines 392 - 455, The two blocks handling main-thread work for MODIFYING_COMMANDS and handlers.dispatch.is_modifying duplicate scheduling/queue/timeout/exception logic; extract this into a helper (e.g., _run_on_main_thread(self, task_fn, timeout=10.0)) that creates the response queue, schedules the provided task_fn via self.schedule_message with the existing AssertionError fallback, waits on queue.get(timeout), and returns a standardized dict {"status":..., "result":..., "message":...}; then replace the duplicated code in the paths that call _dispatch_modifying(...) and handlers.dispatch.execute(...) with thin wrappers that build the task closure (calling those functions and putting the result/error into the queue) and call _run_on_main_thread to get and handle the response.
22-45:MODIFYING_COMMANDSis a list — O(n) lookup on every command.
command_type in MODIFYING_COMMANDSat line 393 performs a linear scan (~60 items) on every incoming command. Use afrozensetfor O(1) membership tests.Proposed fix
-MODIFYING_COMMANDS = [ +MODIFYING_COMMANDS = frozenset([ "create_midi_track", "create_audio_track", "set_track_name", ... "switch_to_view", "record_arrangement_clip", -] +])Also applies to: 393-393
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/__init__.py` around lines 22 - 45, MODIFYING_COMMANDS is currently a list causing O(n) membership checks; change its definition to a frozenset (e.g., MODIFYING_COMMANDS = frozenset([...])) so "command_type in MODIFYING_COMMANDS" becomes O(1); update any other similar lists used for membership tests to frozenset as well to avoid repeated linear scans.MCP_Server/connection.py (2)
129-144: Exception chaining lost on re-raise — original traceback is discarded.All
exceptblocks wrap the original exception in a newException(...)withoutfrom e, making root-cause debugging harder. Useraise ... from eto preserve the chain.Example fix for one block
except socket.timeout: logger.error("Socket timeout while waiting for response from Ableton") self.sock = None - raise Exception("Timeout waiting for Ableton response") + raise Exception("Timeout waiting for Ableton response") from None except (ConnectionError, BrokenPipeError, ConnectionResetError) as e: logger.error(f"Socket connection error: {str(e)}") self.sock = None - raise Exception(f"Connection to Ableton lost: {str(e)}") + raise Exception(f"Connection to Ableton lost: {str(e)}") from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCP_Server/connection.py` around lines 129 - 144, The except handlers in connection.py (around the block handling socket.timeout, ConnectionError/BrokenPipeError/ConnectionResetError, json.JSONDecodeError, and the generic Exception) currently re-raise new Exception instances and drop the original trace; update each raise to use exception chaining (e.g., raise Exception("...") from e) so the original exception is preserved, keep the existing logger.error(...) and self.sock = None behavior, and apply this change for all referenced handlers in the method that reads from Ableton.
42-42:receive_full_responsetakes asockparameter redundant withself.sock.This method is only ever called as
self.receive_full_response(self.sock)(line 118). The extra parameter adds confusion — consider usingself.sockdirectly or documenting why an external socket might be passed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCP_Server/connection.py` at line 42, The receive_full_response method currently accepts a sock parameter redundant with the instance attribute self.sock; change the implementation to use self.sock directly (remove the sock parameter from the signature of receive_full_response) and update all call sites that do self.receive_full_response(self.sock) to call self.receive_full_response() instead; ensure the type hints and any internal references are updated to refer to self.sock and run tests to confirm no other callers rely on passing an external socket (alternatively, if external sockets are required, make the parameter optional (sock: Optional[socket.socket] = None) and fall back to self.sock when None).DEPLOYMENT.md (1)
5-14: Add language identifiers to fenced code blocks.The architecture diagram (line 5) and deployment path blocks (lines 21, 26) lack language identifiers, flagged by markdownlint (MD040). Use
textorplaintextfor non-code blocks to satisfy linting.Also applies to: 21-23, 26-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DEPLOYMENT.md` around lines 5 - 14, Update the fenced code blocks in DEPLOYMENT.md that show the deployment paths (the block containing "production/ableton-mcp/" and the related blocks listing "AbletonMCP_Remote_Script/", "MCP_Server/", etc.) to include a language identifier such as ```text or ```plaintext after the opening backticks so markdownlint MD040 is satisfied; locate each triple-backtick block that contains the directory listings (the one starting with production/ableton-mcp/ and the other blocks around the same section) and change the opening fence to ```text (or ```plaintext) while leaving the block contents intact.AbletonMCP_Remote_Script/handlers/dispatch.py (1)
37-38:_get_registry()is rebuilt on every call — 2–3 times per dynamic command.
is_known,is_modifying, andexecuteeach independently call_get_registry(). During a single command dispatch from__init__.py, the registry dict is constructed 2–3 times. For hot-reload support this is by design, but you could build it once per dispatch by havingexecute(or a single entry point) return the result after a single build, or cache with a short TTL.Example: single-lookup entry point
+def lookup(command_type): + """Return the registry entry for command_type, or None.""" + return _get_registry().get(command_type)Then in
__init__.py:entry = handlers.dispatch.lookup(command_type) if entry is not None: if entry.get("modifying", False): # schedule on main thread ... result = entry["handler"](song, params, ctrl) else: response["result"] = entry["handler"](song, params, ctrl)Also applies to: 130-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/handlers/dispatch.py` around lines 37 - 38, The registry is being rebuilt multiple times per command because is_known, is_modifying, and execute each call _get_registry(); add a single-lookup entry point (e.g., lookup(command_type) or make execute return the built entry) that calls _get_registry() once and returns the handler metadata (entry dict with "handler" and "modifying"), then change callers (is_known/is_modifying/execute) to use that single lookup result (or pass the entry through the dispatch flow) so the registry is constructed only once per dispatch while preserving hot-reload behavior; reference functions: _get_registry, lookup (new), is_known, is_modifying, execute, and the entry dict keys ("handler","modifying").ableton-cli.py (1)
22-24:sys.pathmanipulation is fragile — breaks if script is moved or invoked from a different working directory.
os.path.dirname(__file__)resolves relative to the script location, which should be stable, but consider using a proper package structure or relative import instead. This is acceptable for a CLI utility but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ableton-cli.py` around lines 22 - 24, The current sys.path insertion (sys.path.insert(0, os.path.join(os.path.dirname(__file__), "MCP_Server"))) and the top-level import (from connection import AbletonConnection) is fragile; replace it by either turning MCP_Server into a proper package and using an explicit package import (e.g., from MCP_Server.connection import AbletonConnection) or, if you must manipulate sys.path, compute an absolute path via pathlib.Path(__file__).resolve().parent.joinpath("MCP_Server") and insert str(path) before importing; update the sys.path.insert line and the import statement (connection / AbletonConnection) accordingly so imports are stable regardless of working directory.AbletonMCP_Remote_Script/handlers/midi.py (2)
129-146:quantize_clipdoes not verify the clip is a MIDI clip.Unlike
get_clip_notes(line 72) andapply_groove(line 252),quantize_clipskips theis_midi_clip/is_audio_clipcheck. Whileclip.quantize()may work on audio clips in some Live versions, this inconsistency could lead to unexpected behavior. Consider adding a guard.Proposed fix
clip = clip_slot.clip + if not clip.is_midi_clip: + raise Exception("Clip is not a MIDI clip") clip.quantize(quantize_to, 1.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/handlers/midi.py` around lines 129 - 146, The quantize_clip function currently skips verifying the clip type; add a guard in quantize_clip to check clip.is_midi_clip (and reject audio clips) before calling clip.quantize(), and if the clip is not a MIDI clip use ctrl.log_message (if ctrl) to report "Cannot quantize non-MIDI clip" and raise an appropriate exception (e.g., TypeError or ValueError) so behavior matches get_clip_notes and apply_groove's clip-type checks.
214-237:capture_midivalidatestrack_index/clip_indexbutsong.capture_midi()is a global operation.The track/clip validation is misleading —
song.capture_midi()captures across all armed tracks, not into the specified slot. Theclip_slot.has_clipcheck on line 232 reads state after capture which is fine for reporting, but the function signature implies per-slot targeting that doesn't exist. Consider documenting this clearly or simplifying the signature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/handlers/midi.py` around lines 214 - 237, capture_midi has misleading per-slot parameters: track_index and clip_index suggest targeting a slot but song.capture_midi() is global; either document this behavior or change the API. Update the capture_midi function (and its docstring) to clarify that song.capture_midi() captures across all armed tracks and that track_index/clip_index are only used for reporting clip_slot.has_clip afterwards; alternatively remove track_index/clip_index from the signature and only return global capture status. Ensure references to song.capture_midi, capture_midi, clip_slot.has_clip and the ctrl parameter are updated accordingly (log message and returned payload) so callers and logs reflect the true global behavior.AbletonMCP_Remote_Script/handlers/audio.py (3)
36-44: Deadif ctrl:check —ctrlis guaranteed non-None at this point.Line 20–21 already raises
RuntimeErrorifctrl is None, so theif ctrl:guard on line 37 is alwaysTrue. Harmless but misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/handlers/audio.py` around lines 36 - 44, The guard around ctrl is redundant because a RuntimeError is raised earlier when ctrl is None; remove the unnecessary "if ctrl:" check in the block that logs the attempted load and call ctrl.log_message unconditionally (the code in the elif file_path branch that raises NotImplementedError should still log via ctrl.log_message and then raise). Update the block containing file_path and ctrl to directly use ctrl.log_message (no conditional) to reflect that ctrl is guaranteed non-None.
147-154:warp_mode_mapis duplicated betweenget_audio_clip_infoandanalyze_audio_clip.Extract the integer-to-string warp mode mapping to a module-level constant to avoid maintaining two copies.
Proposed fix
+WARP_MODE_MAP = { + 0: "beats", + 1: "tones", + 2: "texture", + 3: "re_pitch", + 4: "complex", + 5: "complex_pro", +} + + def get_audio_clip_info(song, track_index, clip_index, ctrl=None): ... - warp_mode_map = { - 0: "beats", - ... - } warp_mode = "unknown" if hasattr(clip, "warp_mode"): - warp_mode = warp_mode_map.get(clip.warp_mode, "unknown") + warp_mode = WARP_MODE_MAP.get(clip.warp_mode, "unknown")Apply the same change in
analyze_audio_clip.Also applies to: 299-306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/handlers/audio.py` around lines 147 - 154, warp_mode_map is duplicated in get_audio_clip_info and analyze_audio_clip; extract this integer->string mapping into a module-level constant (e.g., WARP_MODE_MAP) and replace the local warp_mode_map usages in both functions to reference that constant so there is a single source of truth; ensure both get_audio_clip_info and analyze_audio_clip import/use the new WARP_MODE_MAP name and remove the duplicated dicts.
285-499:analyze_audio_clipis ~200 lines of heuristic analysis — consider splitting into sub-functions.The function handles basic info, tempo/rhythm, transients, audio properties, frequency hints, waveform description, pitch info, and summary generation. Extracting each section into a helper (e.g.,
_analyze_transients,_analyze_frequency) would improve readability and testability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/handlers/audio.py` around lines 285 - 499, analyze_audio_clip is doing many responsibilities; split it into smaller helpers to improve readability and testability. Extract the transients block into a _analyze_transients(clip, ctrl) that returns transient dict and any errors; extract sample/audio properties into _analyze_audio_properties(clip, ctrl) returning audio_properties dict; extract tempo/warp logic into _analyze_tempo_rhythm(song, clip) returning tempo_rhythm dict; extract frequency/waveform/pitch into _analyze_frequency_and_waveform(clip) returning frequency_analysis, waveform_description, pitch_info; and extract summary construction into _build_summary(analysis) — then make analyze_audio_clip call these helpers, merge their returned dicts into analysis, and preserve existing error logging (ctrl.log_message) and exceptions behavior. Ensure helper names (_analyze_transients, _analyze_audio_properties, _analyze_tempo_rhythm, _analyze_frequency_and_waveform, _build_summary) match exactly so they’re easy to locate and unit-test.AbletonMCP_Remote_Script/handlers/session.py (1)
153-163:stop_arrangement_recordingunconditionally stops playback — may surprise callers.Setting
record_mode = Falseand then callingsong.stop_playing()couples two distinct operations. A caller may want to stop recording while keeping the transport running (e.g., to audition what was just recorded). Consider either making the playback stop conditional on a parameter or documenting this behavior prominently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/handlers/session.py` around lines 153 - 163, The function stop_arrangement_recording currently always stops playback after disabling record_mode; change its signature (stop_arrangement_recording) to accept a boolean parameter (e.g., stop_playback=False) and only call song.stop_playing() when that flag is True, updating the returned state accordingly; ensure callers that rely on the old behavior are updated or documented and preserve the existing ctrl logging/exception behavior in the except block.AbletonMCP_Remote_Script/handlers/arrangement.py (1)
83-133: Duplicated attribute introspection logic could be extracted into a helper.Lines 94–104 and 113–123 contain identical logic for enumerating object attributes. Consider extracting a small
_inspect_object(obj)helper to reduce duplication in this debugging utility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/handlers/arrangement.py` around lines 83 - 133, The attribute enumeration logic in inspect_arrangement_clip is duplicated for ClipSlot and arrangement clip; extract that block into a helper like _inspect_object(obj) which iterates dir(obj), skips privates, attempts getattr, and returns the attrs list with entries of types "method", "property" (with truncated value) or "inaccessible"; then replace the two duplicated loops in inspect_arrangement_clip by calling _inspect_object(slot) and _inspect_object(clip) and use the returned list when building the response (referencing inspect_arrangement_clip, slot, clip and the new _inspect_object helper).MCP_Server/tools/midi.py (1)
61-68: Silently swallowingcreate_clipfailure may mask real errors.If
create_clipfails for a reason other than "clip already exists" (e.g., wrong track type, invalid index), the subsequentadd_notes_to_clipwill also fail with a confusing error. Consider at least logging the exception.Proposed fix
try: ableton.send_command("create_clip", { "track_index": track_index, "clip_index": clip_index, "length": length, }) - except Exception: - pass + except Exception as create_err: + logger.debug(f"create_clip skipped (may already exist): {create_err}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCP_Server/tools/midi.py` around lines 61 - 68, The create_clip call currently swallows all exceptions (ableton.send_command("create_clip", ...)) which can hide real errors and cause add_notes_to_clip to fail later; change the except block to catch Exception as e and log the full exception (e.g., logging.exception or logger.exception) including context (track_index/clip_index/length), and only suppress the error if the exception message explicitly indicates the clip already exists (otherwise log and re-raise or return an error) so that genuine failures are visible and handled.AbletonMCP_Remote_Script/handlers/tracks.py (1)
415-501:relocate_trackis a non-atomic multi-step operation — partial failure leaves a duplicate track.If
song.delete_track(actual_source)on line 484 fails after the new track has been created (line 444/446) and configured, the session ends up with a duplicate. Consider wrapping the delete in a try/except that at minimum logs a clear warning about the inconsistent state so the user can recover manually.Proposed improvement
- # Delete old track - song.delete_track(actual_source) + # Delete old track — if this fails, the session has a duplicate + try: + song.delete_track(actual_source) + except Exception as del_err: + if ctrl: + ctrl.log_message( + "WARNING: New track created at %d but failed to delete " + "source at %d: %s" % (target_index, actual_source, str(del_err)) + ) + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/handlers/tracks.py` around lines 415 - 501, relocate_track currently creates new_track then calls song.delete_track(actual_source) which can fail and leave a duplicate; wrap the deletion in a try/except around the song.delete_track(actual_source) call (identify by symbol song.delete_track and variable actual_source) and on exception call ctrl.log_message with a clear warning that includes source_index, actual_source, target_index, new_track.name and the exception text; after logging, attempt a best-effort cleanup by trying to delete the new track (song.delete_track at the created track index) inside its own try/except and log any cleanup failure as well, then propagate the original error (re-raise) or return an error result so callers know relocation was incomplete.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ableton-cli.py`:
- Around line 104-105: The args defined with parser.add_argument("--mute",
type=bool) and parser.add_argument("--solo", type=bool) incorrectly convert
strings (e.g., "False") to True; replace the bool type with a custom boolean
parser (e.g., a parse_bool function referenced from the add_argument calls) that
accepts common true/false tokens ("true","1","yes"/"false","0","no") and raises
argparse.ArgumentTypeError for invalid values, and then use
parser.add_argument("--mute", type=parse_bool) and parser.add_argument("--solo",
type=parse_bool) to correctly parse explicit true/false inputs.
In `@AbletonMCP_Remote_Script/__init__.py`:
- Around line 144-151: The recv loop currently decodes each chunk with
data.decode("utf-8") into buffer which can raise UnicodeDecodeError when a
multi-byte UTF-8 character is split across recv boundaries; change the logic to
accumulate raw bytes (e.g., use a bytearray like raw_buffer or the existing data
variable) and only decode when attempting to parse JSON, or use an incremental
UTF-8 decoder (codecs.getincrementaldecoder("utf-8")()) to feed chunks and
append its output to buffer; then call json.loads on the decoded string (and
clear both raw_buffer/decoder state and buffer only after successful parse) so
partial multibyte sequences don’t cause decode errors.
- Around line 43-44: The entries "set_return_track_name" and
"load_on_return_track" are dead in dispatch.py because MODIFYING_COMMANDS (in
__init__.py) causes _process_command to route them to _dispatch_modifying;
remove these two command registrations from dispatch.py's dynamic registry so
they are no longer duplicated/unreachable, and run tests to ensure no other code
references those dispatch.py entries (search for set_return_track_name and
load_on_return_track) before committing.
In `@AbletonMCP_Remote_Script/handlers/arrangement.py`:
- Around line 136-199: The copy_arrangement_to_session function must validate
that the destination track supports audio and the API call exists before calling
target_slot.create_audio_clip(file_path): first check that the track is an audio
track (e.g., hasattr(track, "is_audio_track") and track.is_audio_track) and
raise a clear Exception if not; then verify the API exists on the slot
(hasattr(target_slot, "create_audio_clip")) and raise if it doesn't (to handle
Live 11); also check for a frozen track (e.g., hasattr(track, "is_frozen") and
track.is_frozen) and return/log a helpful error if frozen; finally wrap the call
to target_slot.create_audio_clip(file_path) in a try/except to catch and log
runtime errors (using ctrl.log_message if available) and re-raise with a
descriptive message so callers know the create_audio_clip call failed.
In `@AbletonMCP_Remote_Script/handlers/devices.py`:
- Around line 308-347: Add a track_type parameter (default "track") to
get_macro_values and set_macro_value and use resolve_track(song, track_index,
track_type) instead of directly indexing song.tracks so return/master/return
tracks are handled consistently; in get_macro_values remove the redundant check
"if i < len(device.parameters)" since the existing ternary that checks
len(device.parameters) > i + 1 already prevents out-of-range access, and
preserve the existing ctrl error logging behavior on exceptions.
In `@AbletonMCP_Remote_Script/handlers/tracks.py`:
- Around line 554-586: The code has off-by-one errors in relocate_track: when
inserting "after" the last member you must set the insertion index to one past
the last member (because target_index creates a new track at that index), so
update the "last" branch to set target = members[-1] + 1 (not members[-1]) and
in the "after:" branch change the cap logic so when computing target = ref_pos +
1 you clamp against members[-1] + 1 (not members[-1]); adjust any bounds checks
accordingly so target never exceeds the overall tracks length.
In `@DEPLOYMENT.md`:
- Around line 66-75: The example .mcp.json in DEPLOYMENT.md currently hardcodes
a developer-specific absolute path in the "mcpServers" -> "ableton" -> "args" --
replace the user-specific --directory value with a neutral placeholder like
"<path-to-repo>" or a relative path (e.g. "./path/to/ableton-mcp") and add a
short note that users must customize this before running; update the JSON
example and any accompanying text to reference the placeholder and mention it is
required to be replaced for different environments.
In `@MCP_Server/connection.py`:
- Around line 42-45: The receive_full_response method currently forces a 15s
socket timeout (sock.settimeout(15.0)), which overrides the timeout set by
send_command (self.sock.settimeout(15.0 if is_modifying_command else 10.0));
remove the redundant hardcoded timeout in receive_full_response and instead
accept a timeout parameter (e.g., timeout: float = None) or rely on the
caller-set socket timeout so read-only commands get the intended 10s; update the
call site(s) (send_command) to pass the desired timeout into
receive_full_response or ensure send_command sets the socket timeout immediately
before calling receive_full_response and no other settimeout exists in
receive_full_response.
In `@MCP_Server/server.py`:
- Around line 48-87: The current health-check in get_ableton_connection uses
_ableton_connection.sock.sendall(b"") which can falsely succeed on closed TCP
peers; replace that probe with a reliable check such as calling
_ableton_connection.sock.getpeername() (inside the existing try/except) or
performing a non-blocking recv() that would raise on a dead peer, and remove the
sendall(b"") call; if getpeername() (or recv) raises, log the error, call
_ableton_connection.disconnect(), set _ableton_connection = None, and let the
existing reconnect loop create a fresh AbletonConnection and validate it via
send_command("get_session_info") as already implemented.
In `@MCP_Server/tools/devices.py`:
- Around line 57-77: The get_rack_device_info tool returns a generic error
string without the exception details; update the except block in
get_rack_device_info to return the exception message (e.g. include str(e) or an
f-string like "Error getting rack device info: {e}") so the returned value
matches other tools and aids debugging while keeping the existing logger.error
call.
---
Duplicate comments:
In `@AbletonMCP_Remote_Script/handlers/audio.py`:
- Around line 502-523: The code incorrectly writes to a non-writable Track
property (track.freeze) — the Ableton Live API only exposes can_be_frozen and
is_frozen as read-only — so update freeze_track, unfreeze_track, and
export_track_audio to stop assigning to track.freeze or calling a non-existent
freeze() method; instead check track.can_be_frozen and track.is_frozen, then
either return a clear error/NotImplementedError (and log via ctrl.log_message)
stating that programmatic freezing/unfreezing/exporting is not supported by the
Live API, or implement an alternative export flow that uses a supported API call
if available; reference the functions freeze_track, unfreeze_track, and
export_track_audio when making the changes.
- Around line 546-585: export_track_audio currently ignores the start_time and
end_time parameters; either honor them or explicitly reject them — update
export_track_audio to validate start_time/end_time at the top (e.g., if
start_time is not None or end_time is not None) and if the implementation does
not support ranged exports, use ctrl.log_message to emit a clear error and raise
NotImplementedError (or ValueError) explaining that range exports are
unsupported and advising to use Ableton's Export Audio/Video; reference the
export_track_audio function, the start_time/end_time parameters, and the
ctrl.log_message calls so the change is made where the function validates inputs
before freezing/unfreezing the track.
In `@AbletonMCP_Remote_Script/handlers/devices.py`:
- Around line 350-380: The set_macro_value function currently uses song.tracks
directly and assigns value without clamping; change it to resolve the track the
same way as get_macro_values/get_device_parameter by honoring track_type and
calling resolve_track(track_index, song, track_type) (instead of
song.tracks[...]), then retrieve the device and macro parameter (param_index =
macro_index + 1) and clamp the incoming value to the macro_param.min and
macro_param.max before assigning (mirror the clamping logic used in
set_device_parameter and set_chain_device_parameter), keep the existing error
handling and ctrl.log_message usage but ensure you reference the same symbols:
set_macro_value, resolve_track, macro_param, param_index, macro_index.
In `@AbletonMCP_Remote_Script/handlers/midi.py`:
- Around line 181-211: The function duplicate_clip validates
dest_track/dest_clip but always duplicates on src_track (using
src_track.duplicate_clip_slot and source_clip+1), so cross-track duplication is
broken; update duplicate_clip to when dest_track != source_track copy the clip
data into the destination slot instead of calling src_track.duplicate_clip_slot:
locate src_slot = src_track.clip_slots[source_clip] and dst_slot =
dst_track.clip_slots[dest_clip], ensure src_slot.has_clip, then create or
replace the clip in dst_slot using the source clip's content/loop (e.g. use the
API to create/replace a clip from src_slot.clip data or use a dst_track API if
available), and only call src_track.duplicate_clip_slot when dest_track equals
source_track (preserving current behavior); keep the existing error handling and
ctrl.log_message behavior.
In `@AbletonMCP_Remote_Script/handlers/tracks.py`:
- Around line 221-238: The function group_tracks currently only selects the
first track (song.view.selected_track = song.tracks[track_indices[0]]) but
reports "grouped": True; change it to actually create a group, move the
specified tracks into it, set the group's name, and select the new group.
Specifically, in group_tracks: validate indices as already done, collect the
Track objects for track_indices, create a new GroupTrack (or call the host API
that creates a group, e.g., song.create_group or similar), add/move those Track
objects into the new group container, set the group's name to the name
parameter, update song.view.selected_track (or song.view.selected_group) to the
newly created group, and return the same result dict; keep the existing
exception handling and ctrl.log_message behavior.
In `@MCP_Server/connection.py`:
- Around line 86-109: The MODIFYING_COMMANDS list in connection.py is out of
sync with the canonical MODIFYING_COMMANDS in
AbletonMCP_Remote_Script/__init__.py: specifically add "load_browser_item" to
connection.py (so it uses the 15s timeout and pre-send delay) and remove or
treat "create_locator" as a special multi-tick command (do not include it in
MODIFYING_COMMANDS) to match the handling in __init__.py; better yet, refactor
by extracting the canonical MODIFYING_COMMANDS into a shared constant (e.g., a
new module or importing the list from AbletonMCP_Remote_Script.__init__) and
have both connection.py and other modules reference that single source to keep
them synchronized.
In `@MCP_Server/tools/midi.py`:
- Line 40: The call grid = notes_to_grid(notes, clip_length) incorrectly passes
clip_length as the is_drums positional argument (truthy float) and forces drum
format; change the call to not pass clip_length positionally—either call
notes_to_grid(notes) / notes_to_grid(notes, is_drums=None) if you meant default
behavior, or if you intended to set steps_per_beat pass it by name e.g.
notes_to_grid(notes, steps_per_beat=int(clip_length)) (and cast/validate as
needed).
---
Nitpick comments:
In `@ableton-cli.py`:
- Around line 22-24: The current sys.path insertion (sys.path.insert(0,
os.path.join(os.path.dirname(__file__), "MCP_Server"))) and the top-level import
(from connection import AbletonConnection) is fragile; replace it by either
turning MCP_Server into a proper package and using an explicit package import
(e.g., from MCP_Server.connection import AbletonConnection) or, if you must
manipulate sys.path, compute an absolute path via
pathlib.Path(__file__).resolve().parent.joinpath("MCP_Server") and insert
str(path) before importing; update the sys.path.insert line and the import
statement (connection / AbletonConnection) accordingly so imports are stable
regardless of working directory.
In `@AbletonMCP_Remote_Script/__init__.py`:
- Around line 392-455: The two blocks handling main-thread work for
MODIFYING_COMMANDS and handlers.dispatch.is_modifying duplicate
scheduling/queue/timeout/exception logic; extract this into a helper (e.g.,
_run_on_main_thread(self, task_fn, timeout=10.0)) that creates the response
queue, schedules the provided task_fn via self.schedule_message with the
existing AssertionError fallback, waits on queue.get(timeout), and returns a
standardized dict {"status":..., "result":..., "message":...}; then replace the
duplicated code in the paths that call _dispatch_modifying(...) and
handlers.dispatch.execute(...) with thin wrappers that build the task closure
(calling those functions and putting the result/error into the queue) and call
_run_on_main_thread to get and handle the response.
- Around line 22-45: MODIFYING_COMMANDS is currently a list causing O(n)
membership checks; change its definition to a frozenset (e.g.,
MODIFYING_COMMANDS = frozenset([...])) so "command_type in MODIFYING_COMMANDS"
becomes O(1); update any other similar lists used for membership tests to
frozenset as well to avoid repeated linear scans.
In `@AbletonMCP_Remote_Script/handlers/arrangement.py`:
- Around line 83-133: The attribute enumeration logic in
inspect_arrangement_clip is duplicated for ClipSlot and arrangement clip;
extract that block into a helper like _inspect_object(obj) which iterates
dir(obj), skips privates, attempts getattr, and returns the attrs list with
entries of types "method", "property" (with truncated value) or "inaccessible";
then replace the two duplicated loops in inspect_arrangement_clip by calling
_inspect_object(slot) and _inspect_object(clip) and use the returned list when
building the response (referencing inspect_arrangement_clip, slot, clip and the
new _inspect_object helper).
In `@AbletonMCP_Remote_Script/handlers/audio.py`:
- Around line 36-44: The guard around ctrl is redundant because a RuntimeError
is raised earlier when ctrl is None; remove the unnecessary "if ctrl:" check in
the block that logs the attempted load and call ctrl.log_message unconditionally
(the code in the elif file_path branch that raises NotImplementedError should
still log via ctrl.log_message and then raise). Update the block containing
file_path and ctrl to directly use ctrl.log_message (no conditional) to reflect
that ctrl is guaranteed non-None.
- Around line 147-154: warp_mode_map is duplicated in get_audio_clip_info and
analyze_audio_clip; extract this integer->string mapping into a module-level
constant (e.g., WARP_MODE_MAP) and replace the local warp_mode_map usages in
both functions to reference that constant so there is a single source of truth;
ensure both get_audio_clip_info and analyze_audio_clip import/use the new
WARP_MODE_MAP name and remove the duplicated dicts.
- Around line 285-499: analyze_audio_clip is doing many responsibilities; split
it into smaller helpers to improve readability and testability. Extract the
transients block into a _analyze_transients(clip, ctrl) that returns transient
dict and any errors; extract sample/audio properties into
_analyze_audio_properties(clip, ctrl) returning audio_properties dict; extract
tempo/warp logic into _analyze_tempo_rhythm(song, clip) returning tempo_rhythm
dict; extract frequency/waveform/pitch into
_analyze_frequency_and_waveform(clip) returning frequency_analysis,
waveform_description, pitch_info; and extract summary construction into
_build_summary(analysis) — then make analyze_audio_clip call these helpers,
merge their returned dicts into analysis, and preserve existing error logging
(ctrl.log_message) and exceptions behavior. Ensure helper names
(_analyze_transients, _analyze_audio_properties, _analyze_tempo_rhythm,
_analyze_frequency_and_waveform, _build_summary) match exactly so they’re easy
to locate and unit-test.
In `@AbletonMCP_Remote_Script/handlers/browser.py`:
- Around line 298-317: The loop over browser_attrs treating every public
attribute of app.browser as a category can surface methods or non-browsable
objects; restrict the set to only real categories by filtering attributes that
actually expose children (or use an explicit allowlist) before calling
_process_item. Update the block that iterates browser_attrs (and the
getattr(app.browser, attr) usage) to skip attrs that lack a "children" attribute
or are callable, or replace browser_attrs with a small explicit list of known
extra categories, and keep the existing ctrl.log_message error handling for
anything unexpected.
- Around line 85-87: The code silently falls back to app.browser.instruments
when the first path segment is unrecognized (setting current_item =
app.browser.instruments and path_parts = ["instruments"] + path_parts); change
this to validate the first path segment and return or raise an explicit error
instead of mutating path_parts and defaulting to instruments. Locate the logic
that branches on the first path segment (the block that sets current_item and
path_parts) and replace the fallback branch with a clear error path (e.g., raise
ValueError or return an error response) that includes the invalid root segment
so callers see a precise "unrecognized root: <segment>" message rather than
silently redirecting to app.browser.instruments.
In `@AbletonMCP_Remote_Script/handlers/dispatch.py`:
- Around line 37-38: The registry is being rebuilt multiple times per command
because is_known, is_modifying, and execute each call _get_registry(); add a
single-lookup entry point (e.g., lookup(command_type) or make execute return the
built entry) that calls _get_registry() once and returns the handler metadata
(entry dict with "handler" and "modifying"), then change callers
(is_known/is_modifying/execute) to use that single lookup result (or pass the
entry through the dispatch flow) so the registry is constructed only once per
dispatch while preserving hot-reload behavior; reference functions:
_get_registry, lookup (new), is_known, is_modifying, execute, and the entry dict
keys ("handler","modifying").
In `@AbletonMCP_Remote_Script/handlers/midi.py`:
- Around line 129-146: The quantize_clip function currently skips verifying the
clip type; add a guard in quantize_clip to check clip.is_midi_clip (and reject
audio clips) before calling clip.quantize(), and if the clip is not a MIDI clip
use ctrl.log_message (if ctrl) to report "Cannot quantize non-MIDI clip" and
raise an appropriate exception (e.g., TypeError or ValueError) so behavior
matches get_clip_notes and apply_groove's clip-type checks.
- Around line 214-237: capture_midi has misleading per-slot parameters:
track_index and clip_index suggest targeting a slot but song.capture_midi() is
global; either document this behavior or change the API. Update the capture_midi
function (and its docstring) to clarify that song.capture_midi() captures across
all armed tracks and that track_index/clip_index are only used for reporting
clip_slot.has_clip afterwards; alternatively remove track_index/clip_index from
the signature and only return global capture status. Ensure references to
song.capture_midi, capture_midi, clip_slot.has_clip and the ctrl parameter are
updated accordingly (log message and returned payload) so callers and logs
reflect the true global behavior.
In `@AbletonMCP_Remote_Script/handlers/session.py`:
- Around line 153-163: The function stop_arrangement_recording currently always
stops playback after disabling record_mode; change its signature
(stop_arrangement_recording) to accept a boolean parameter (e.g.,
stop_playback=False) and only call song.stop_playing() when that flag is True,
updating the returned state accordingly; ensure callers that rely on the old
behavior are updated or documented and preserve the existing ctrl
logging/exception behavior in the except block.
In `@AbletonMCP_Remote_Script/handlers/tracks.py`:
- Around line 415-501: relocate_track currently creates new_track then calls
song.delete_track(actual_source) which can fail and leave a duplicate; wrap the
deletion in a try/except around the song.delete_track(actual_source) call
(identify by symbol song.delete_track and variable actual_source) and on
exception call ctrl.log_message with a clear warning that includes source_index,
actual_source, target_index, new_track.name and the exception text; after
logging, attempt a best-effort cleanup by trying to delete the new track
(song.delete_track at the created track index) inside its own try/except and log
any cleanup failure as well, then propagate the original error (re-raise) or
return an error result so callers know relocation was incomplete.
In `@DEPLOYMENT.md`:
- Around line 5-14: Update the fenced code blocks in DEPLOYMENT.md that show the
deployment paths (the block containing "production/ableton-mcp/" and the related
blocks listing "AbletonMCP_Remote_Script/", "MCP_Server/", etc.) to include a
language identifier such as ```text or ```plaintext after the opening backticks
so markdownlint MD040 is satisfied; locate each triple-backtick block that
contains the directory listings (the one starting with production/ableton-mcp/
and the other blocks around the same section) and change the opening fence to
```text (or ```plaintext) while leaving the block contents intact.
In `@MCP_Server/connection.py`:
- Around line 129-144: The except handlers in connection.py (around the block
handling socket.timeout, ConnectionError/BrokenPipeError/ConnectionResetError,
json.JSONDecodeError, and the generic Exception) currently re-raise new
Exception instances and drop the original trace; update each raise to use
exception chaining (e.g., raise Exception("...") from e) so the original
exception is preserved, keep the existing logger.error(...) and self.sock = None
behavior, and apply this change for all referenced handlers in the method that
reads from Ableton.
- Line 42: The receive_full_response method currently accepts a sock parameter
redundant with the instance attribute self.sock; change the implementation to
use self.sock directly (remove the sock parameter from the signature of
receive_full_response) and update all call sites that do
self.receive_full_response(self.sock) to call self.receive_full_response()
instead; ensure the type hints and any internal references are updated to refer
to self.sock and run tests to confirm no other callers rely on passing an
external socket (alternatively, if external sockets are required, make the
parameter optional (sock: Optional[socket.socket] = None) and fall back to
self.sock when None).
In `@MCP_Server/tools/midi.py`:
- Around line 61-68: The create_clip call currently swallows all exceptions
(ableton.send_command("create_clip", ...)) which can hide real errors and cause
add_notes_to_clip to fail later; change the except block to catch Exception as e
and log the full exception (e.g., logging.exception or logger.exception)
including context (track_index/clip_index/length), and only suppress the error
if the exception message explicitly indicates the clip already exists (otherwise
log and re-raise or return an error) so that genuine failures are visible and
handled.
| parser.add_argument("--mute", type=bool) | ||
| parser.add_argument("--solo", type=bool) |
There was a problem hiding this comment.
type=bool in argparse does not work as intended — --mute False sets True.
argparse passes the string through bool(), so bool("False") evaluates to True. Any non-empty string becomes True.
Proposed fix
- parser.add_argument("--mute", type=bool)
- parser.add_argument("--solo", type=bool)
+ parser.add_argument("--mute", action="store_true")
+ parser.add_argument("--solo", action="store_true")If you need to support explicit --mute false, use a custom parser:
def parse_bool(v):
if v.lower() in ("true", "1", "yes"):
return True
if v.lower() in ("false", "0", "no"):
return False
raise argparse.ArgumentTypeError(f"Boolean value expected, got '{v}'")
parser.add_argument("--mute", type=parse_bool)
parser.add_argument("--solo", type=parse_bool)📝 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.
| parser.add_argument("--mute", type=bool) | |
| parser.add_argument("--solo", type=bool) | |
| parser.add_argument("--mute", action="store_true") | |
| parser.add_argument("--solo", action="store_true") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ableton-cli.py` around lines 104 - 105, The args defined with
parser.add_argument("--mute", type=bool) and parser.add_argument("--solo",
type=bool) incorrectly convert strings (e.g., "False") to True; replace the bool
type with a custom boolean parser (e.g., a parse_bool function referenced from
the add_argument calls) that accepts common true/false tokens
("true","1","yes"/"false","0","no") and raises argparse.ArgumentTypeError for
invalid values, and then use parser.add_argument("--mute", type=parse_bool) and
parser.add_argument("--solo", type=parse_bool) to correctly parse explicit
true/false inputs.
| break | ||
|
|
||
| # Accumulate data in buffer with explicit encoding/decoding | ||
| try: | ||
| # Python 3: data is bytes, decode to string | ||
| buffer += data.decode('utf-8') | ||
| buffer += data.decode("utf-8") | ||
| except AttributeError: | ||
| # Python 2: data is already string | ||
| buffer += data | ||
|
|
||
| try: | ||
| # Try to parse command from buffer | ||
| command = json.loads(buffer) # Removed decode('utf-8') | ||
| buffer = '' # Clear buffer after successful parse | ||
|
|
||
| command = json.loads(buffer) | ||
| buffer = "" |
There was a problem hiding this comment.
Potential UnicodeDecodeError if a multi-byte UTF-8 character is split across recv boundaries.
Line 146 decodes each recv chunk independently. If a multi-byte UTF-8 character (e.g., non-ASCII track name) is split between two 8192-byte chunks, data.decode("utf-8") will raise UnicodeDecodeError, which is caught by the outer except Exception at line 160 and breaks the connection.
Consider accumulating raw bytes and decoding only when attempting JSON parse, or use an incremental decoder.
Proposed fix — accumulate bytes, decode at parse time
- buffer = ""
+ raw_buffer = b""
try:
while self.running:
try:
data = client.recv(8192)
if not data:
self.log_message("Client disconnected")
break
- try:
- buffer += data.decode("utf-8")
- except AttributeError:
- buffer += data
+ raw_buffer += data
try:
- command = json.loads(buffer)
- buffer = ""
+ command = json.loads(raw_buffer.decode("utf-8"))
+ raw_buffer = b""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AbletonMCP_Remote_Script/__init__.py` around lines 144 - 151, The recv loop
currently decodes each chunk with data.decode("utf-8") into buffer which can
raise UnicodeDecodeError when a multi-byte UTF-8 character is split across recv
boundaries; change the logic to accumulate raw bytes (e.g., use a bytearray like
raw_buffer or the existing data variable) and only decode when attempting to
parse JSON, or use an incremental UTF-8 decoder
(codecs.getincrementaldecoder("utf-8")()) to feed chunks and append its output
to buffer; then call json.loads on the decoded string (and clear both
raw_buffer/decoder state and buffer only after successful parse) so partial
multibyte sequences don’t cause decode errors.
| def copy_arrangement_to_session(song, track_index, arrangement_clip_index, clip_slot_index, ctrl=None): | ||
| """Copy an arrangement clip into a session clip slot. | ||
|
|
||
| Uses create_audio_clip on the target slot with the arrangement clip's file_path. | ||
| """ | ||
| try: | ||
| if track_index < 0 or track_index >= len(song.tracks): | ||
| raise IndexError("Track index out of range") | ||
| track = song.tracks[track_index] | ||
|
|
||
| if not hasattr(track, "arrangement_clips"): | ||
| raise Exception("Track has no arrangement clips") | ||
|
|
||
| arr_clips = list(track.arrangement_clips) | ||
| if arrangement_clip_index < 0 or arrangement_clip_index >= len(arr_clips): | ||
| raise IndexError( | ||
| "Arrangement clip index %d out of range (track has %d)" | ||
| % (arrangement_clip_index, len(arr_clips)) | ||
| ) | ||
|
|
||
| if clip_slot_index < 0 or clip_slot_index >= len(track.clip_slots): | ||
| raise IndexError("Clip slot index out of range") | ||
|
|
||
| src = arr_clips[arrangement_clip_index] | ||
| target_slot = track.clip_slots[clip_slot_index] | ||
|
|
||
| if target_slot.has_clip: | ||
| raise Exception("Target clip slot already has a clip") | ||
|
|
||
| if not src.is_audio_clip: | ||
| raise Exception("Only audio clips supported") | ||
|
|
||
| file_path = src.file_path | ||
| if not file_path: | ||
| raise Exception("Arrangement audio clip has no file_path") | ||
|
|
||
| # Create audio clip in session slot from the same file | ||
| target_slot.create_audio_clip(file_path) | ||
|
|
||
| # Copy properties from arrangement clip | ||
| if target_slot.has_clip: | ||
| new_clip = target_slot.clip | ||
| new_clip.name = src.name | ||
| new_clip.looping = True | ||
| new_clip.loop_start = src.loop_start | ||
| new_clip.loop_end = src.loop_end | ||
| new_clip.warping = src.warping | ||
| if hasattr(src, "warp_mode"): | ||
| new_clip.warp_mode = src.warp_mode | ||
| if hasattr(src, "gain"): | ||
| new_clip.gain = src.gain | ||
|
|
||
| return { | ||
| "copied": True, | ||
| "track_index": track_index, | ||
| "clip_slot_index": clip_slot_index, | ||
| "clip_name": src.name, | ||
| "file_path": file_path, | ||
| } | ||
|
|
||
| except Exception as e: | ||
| if ctrl: | ||
| ctrl.log_message("Error copying arrangement to session: " + str(e)) | ||
| raise |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "create_audio_clip" --type py -C 3Repository: ahujasid/ableton-mcp
Length of output: 1402
🌐 Web query:
Ableton Live Python API ClipSlot create_audio_clip
💡 Result:
ClipSlot.create_audio_clip is a Live Object Model (LOM) function that creates an audio clip in a Session View clip slot from an absolute file path:
- Signature/behavior:
create_audio_clip(path)wherepathis an absolute path to a supported audio file; it errors if the slot isn’t on an audio track or the track is frozen. [1] - Version availability: although it appears in the LOM docs, it has historically not been present in some Live 12 trial/early builds (“no attribute
create_audio_clip”). It was added in the Live 12 beta line (release notes mentioncreate_audio_clip/create_midi_clipbeing added to the API). [2] The Max for Live forum confirms it worked for users only once they were on a newer Live 12 beta build. [3]
Minimal Python (Control Surface) usage
# assuming you already have a ClipSlot object, e.g. slot = track.clip_slots[i]
slot.create_audio_clip(r"C:\Samples\kick.wav") # Windows example (absolute path)
# slot.create_audio_clip("/Users/me/Samples/kick.wav") # macOS exampleIf you’re on Live 11 (or a Live 12 build that doesn’t include it), you’ll only have ClipSlot.create_clip(length) for MIDI clip creation, not audio importing. [1]
Sources: Ableton LOM ClipSlot docs [1], Ableton Live 12 beta release notes [2], Cycling ’74 Max for Live forum thread [3].
target_slot.create_audio_clip(file_path) is a valid Ableton Live API call, but the code lacks validation for its requirements.
The method exists in the Ableton Live Object Model (added in Live 12 beta) and accepts an absolute file path. However, create_audio_clip errors if:
- The track is not an audio track
- The track is frozen
Add validation before line 173 to check that the track is an audio track. Consider also handling potential errors from create_audio_clip in case the track is frozen. Note that this method is not available in Live 11, which may affect users on older versions.
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 143-143: Abstract raise to an inner function
(TRY301)
[warning] 143-143: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 147-147: Abstract raise to an inner function
(TRY301)
[warning] 147-147: Create your own exception
(TRY002)
[warning] 147-147: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 151-154: Abstract raise to an inner function
(TRY301)
[warning] 157-157: Abstract raise to an inner function
(TRY301)
[warning] 157-157: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 163-163: Abstract raise to an inner function
(TRY301)
[warning] 163-163: Create your own exception
(TRY002)
[warning] 163-163: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 166-166: Abstract raise to an inner function
(TRY301)
[warning] 166-166: Create your own exception
(TRY002)
[warning] 166-166: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 170-170: Abstract raise to an inner function
(TRY301)
[warning] 170-170: Create your own exception
(TRY002)
[warning] 170-170: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 188-194: Consider moving this statement to an else block
(TRY300)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AbletonMCP_Remote_Script/handlers/arrangement.py` around lines 136 - 199, The
copy_arrangement_to_session function must validate that the destination track
supports audio and the API call exists before calling
target_slot.create_audio_clip(file_path): first check that the track is an audio
track (e.g., hasattr(track, "is_audio_track") and track.is_audio_track) and
raise a clear Exception if not; then verify the API exists on the slot
(hasattr(target_slot, "create_audio_clip")) and raise if it doesn't (to handle
Live 11); also check for a frozen track (e.g., hasattr(track, "is_frozen") and
track.is_frozen) and return/log a helpful error if frozen; finally wrap the call
to target_slot.create_audio_clip(file_path) in a try/except to catch and log
runtime errors (using ctrl.log_message if available) and re-raise with a
descriptive message so callers know the create_audio_clip call failed.
| def get_macro_values(song, track_index, device_index, ctrl=None): | ||
| """Get the values of all 8 macro controls on a rack device.""" | ||
| try: | ||
| if track_index < 0 or track_index >= len(song.tracks): | ||
| raise IndexError("Track index out of range") | ||
| track = song.tracks[track_index] | ||
| if device_index < 0 or device_index >= len(track.devices): | ||
| raise IndexError("Device index out of range") | ||
| device = track.devices[device_index] | ||
| if not hasattr(device, "macros_mapped"): | ||
| raise Exception("Device is not a rack (no macros)") | ||
| macros = [] | ||
| for i in range(8): | ||
| if i < len(device.parameters): | ||
| macro_param = ( | ||
| device.parameters[i + 1] | ||
| if len(device.parameters) > i + 1 | ||
| else None | ||
| ) | ||
| if macro_param: | ||
| macros.append({ | ||
| "index": i, | ||
| "name": macro_param.name, | ||
| "value": macro_param.value, | ||
| "min": macro_param.min, | ||
| "max": macro_param.max, | ||
| "is_enabled": getattr( | ||
| macro_param, "is_enabled", True | ||
| ), | ||
| }) | ||
| return { | ||
| "track_index": track_index, | ||
| "device_index": device_index, | ||
| "device_name": device.name, | ||
| "macros": macros, | ||
| } | ||
| except Exception as e: | ||
| if ctrl: | ||
| ctrl.log_message("Error getting macro values: " + str(e)) | ||
| raise |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the get_macro_values function and surrounding context
cat -n AbletonMCP_Remote_Script/handlers/devices.py | head -360 | tail -80Repository: ahujasid/ableton-mcp
Length of output: 3881
🏁 Script executed:
# Check for resolve_track definition and usage
rg -n "resolve_track" AbletonMCP_Remote_Script/handlers/devices.py | head -20Repository: ahujasid/ableton-mcp
Length of output: 572
🏁 Script executed:
# Look for other functions in the file and their signatures to see track_type pattern
rg -n "^def " AbletonMCP_Remote_Script/handlers/devices.pyRepository: ahujasid/ableton-mcp
Length of output: 1349
🏁 Script executed:
# Search for callers of get_macro_values
rg -rn "get_macro_values" --type=pyRepository: ahujasid/ableton-mcp
Length of output: 328
🏁 Script executed:
# Search for set_macro_value as well to see if it has similar pattern
rg -n "def set_macro_value" AbletonMCP_Remote_Script/handlers/devices.pyRepository: ahujasid/ableton-mcp
Length of output: 152
🏁 Script executed:
# View the resolve_track function to understand what it enables
sed -n '6,18p' AbletonMCP_Remote_Script/handlers/devices.pyRepository: ahujasid/ableton-mcp
Length of output: 639
🏁 Script executed:
# Check the full context around set_macro_value to understand its pattern
sed -n '350,380p' AbletonMCP_Remote_Script/handlers/devices.pyRepository: ahujasid/ableton-mcp
Length of output: 1433
🏁 Script executed:
# Search for any router/handler code that calls get_macro_values or set_macro_value
rg -rn "get_macro_values|set_macro_value" --type=py -B 2 -A 2Repository: ahujasid/ableton-mcp
Length of output: 2750
Add track_type parameter to get_macro_values and set_macro_value; both currently bypass resolve_track.
Both functions hardcode song.tracks[track_index], preventing access to return or master tracks. Every other device-handling function in this module accepts track_type and uses resolve_track. Additionally, in get_macro_values, the bounds check on line 321 (if i < len(device.parameters)) is redundant — the ternary on lines 323-325 already guards the i + 1 access.
Proposed fix
-def get_macro_values(song, track_index, device_index, ctrl=None):
+def get_macro_values(song, track_index, device_index, track_type="track", ctrl=None):
"""Get the values of all 8 macro controls on a rack device."""
try:
- if track_index < 0 or track_index >= len(song.tracks):
- raise IndexError("Track index out of range")
- track = song.tracks[track_index]
+ track = resolve_track(song, track_index, track_type)
if device_index < 0 or device_index >= len(track.devices):
raise IndexError("Device index out of range")
device = track.devices[device_index]
if not hasattr(device, "macros_mapped"):
raise Exception("Device is not a rack (no macros)")
macros = []
for i in range(8):
- if i < len(device.parameters):
- macro_param = (
- device.parameters[i + 1]
- if len(device.parameters) > i + 1
- else None
- )
- if macro_param:
- macros.append({
- "index": i,
- "name": macro_param.name,
- "value": macro_param.value,
- "min": macro_param.min,
- "max": macro_param.max,
- "is_enabled": getattr(
- macro_param, "is_enabled", True
- ),
- })
+ param_index = i + 1
+ if param_index < len(device.parameters):
+ macro_param = device.parameters[param_index]
+ macros.append({
+ "index": i,
+ "name": macro_param.name,
+ "value": macro_param.value,
+ "min": macro_param.min,
+ "max": macro_param.max,
+ "is_enabled": getattr(macro_param, "is_enabled", True),
+ })Apply the same pattern to set_macro_value (line 350): add track_type="track" parameter and replace hardcoded track access with resolve_track(song, track_index, track_type).
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 312-312: Abstract raise to an inner function
(TRY301)
[warning] 312-312: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 315-315: Abstract raise to an inner function
(TRY301)
[warning] 315-315: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 318-318: Abstract raise to an inner function
(TRY301)
[warning] 318-318: Create your own exception
(TRY002)
[warning] 318-318: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 338-343: Consider moving this statement to an else block
(TRY300)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AbletonMCP_Remote_Script/handlers/devices.py` around lines 308 - 347, Add a
track_type parameter (default "track") to get_macro_values and set_macro_value
and use resolve_track(song, track_index, track_type) instead of directly
indexing song.tracks so return/master/return tracks are handled consistently; in
get_macro_values remove the redundant check "if i < len(device.parameters)"
since the existing ternary that checks len(device.parameters) > i + 1 already
prevents out-of-range access, and preserve the existing ctrl error logging
behavior on exceptions.
| if not members: | ||
| # Empty group — insert right after the bus | ||
| target = group_idx + 1 | ||
| elif position == "first": | ||
| target = members[0] | ||
| elif position == "last": | ||
| target = members[-1] | ||
| elif position.startswith("before:"): | ||
| ref_name = position[7:] | ||
| target = None | ||
| for m in members: | ||
| if tracks[m].name == ref_name: | ||
| target = m | ||
| break | ||
| if target is None: | ||
| raise ValueError("Reference track not found in group: " + ref_name) | ||
| elif position.startswith("after:"): | ||
| ref_name = position[6:] | ||
| target = None | ||
| for m in members: | ||
| if tracks[m].name == ref_name: | ||
| # Insert at this position pushes ref track down, | ||
| # but we want AFTER, so use the next member or last | ||
| ref_pos = m | ||
| target = ref_pos + 1 | ||
| # Check if target is still inside the group | ||
| if target > members[-1]: | ||
| target = members[-1] | ||
| break | ||
| if target is None: | ||
| raise ValueError("Reference track not found in group: " + ref_name) | ||
| else: | ||
| target = members[-1] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, get an overview of the file structure and locate the function
grep -n "def " AbletonMCP_Remote_Script/handlers/tracks.py | head -20Repository: ahujasid/ableton-mcp
Length of output: 1213
🏁 Script executed:
# Run the suggested search from the review
rg -n "move_to_group\|relocate_track" --type py -C 2 AbletonMCP_Remote_Script/handlers/tracks.pyRepository: ahujasid/ableton-mcp
Length of output: 46
🏁 Script executed:
# Get the full function containing lines 554-586
sed -n '500,650p' AbletonMCP_Remote_Script/handlers/tracks.pyRepository: ahujasid/ableton-mcp
Length of output: 4338
🏁 Script executed:
# Read the relocate_track function (line 415)
sed -n '415,503p' AbletonMCP_Remote_Script/handlers/tracks.pyRepository: ahujasid/ableton-mcp
Length of output: 3125
Off-by-one errors: position="last" and position="after:..." when applied to the last group member insert before the target position instead of after.
In relocate_track, the target_index parameter means "create the new track at this index", which pushes existing tracks forward. This makes setting target = members[-1] incorrect—it inserts the new track before the last member, not after.
The same issue affects position="after:NAME" when the reference track is last in the group: line 580–581 set target = ref_pos + 1, but then line 581–583 cap it back to members[-1], undoing the "+1" offset.
🐛 Proposed fix
elif position == "last":
- target = members[-1]
+ target = members[-1] + 1
elif position.startswith("before:"):And for the "after:" branch:
target = ref_pos + 1
- # Check if target is still inside the group
- if target > members[-1]:
- target = members[-1]
+ # No need to cap — target > members[-1] is valid and lands
+ # the new track after the last member, inside the group.📝 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 not members: | |
| # Empty group — insert right after the bus | |
| target = group_idx + 1 | |
| elif position == "first": | |
| target = members[0] | |
| elif position == "last": | |
| target = members[-1] | |
| elif position.startswith("before:"): | |
| ref_name = position[7:] | |
| target = None | |
| for m in members: | |
| if tracks[m].name == ref_name: | |
| target = m | |
| break | |
| if target is None: | |
| raise ValueError("Reference track not found in group: " + ref_name) | |
| elif position.startswith("after:"): | |
| ref_name = position[6:] | |
| target = None | |
| for m in members: | |
| if tracks[m].name == ref_name: | |
| # Insert at this position pushes ref track down, | |
| # but we want AFTER, so use the next member or last | |
| ref_pos = m | |
| target = ref_pos + 1 | |
| # Check if target is still inside the group | |
| if target > members[-1]: | |
| target = members[-1] | |
| break | |
| if target is None: | |
| raise ValueError("Reference track not found in group: " + ref_name) | |
| else: | |
| target = members[-1] | |
| if not members: | |
| # Empty group — insert right after the bus | |
| target = group_idx + 1 | |
| elif position == "first": | |
| target = members[0] | |
| elif position == "last": | |
| target = members[-1] + 1 | |
| elif position.startswith("before:"): | |
| ref_name = position[7:] | |
| target = None | |
| for m in members: | |
| if tracks[m].name == ref_name: | |
| target = m | |
| break | |
| if target is None: | |
| raise ValueError("Reference track not found in group: " + ref_name) | |
| elif position.startswith("after:"): | |
| ref_name = position[6:] | |
| target = None | |
| for m in members: | |
| if tracks[m].name == ref_name: | |
| # Insert at this position pushes ref track down, | |
| # but we want AFTER, so use the next member or last | |
| ref_pos = m | |
| target = ref_pos + 1 | |
| # No need to cap — target > members[-1] is valid and lands | |
| # the new track after the last member, inside the group. | |
| break | |
| if target is None: | |
| raise ValueError("Reference track not found in group: " + ref_name) | |
| else: | |
| target = members[-1] |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 569-569: Abstract raise to an inner function
(TRY301)
[warning] 584-584: Abstract raise to an inner function
(TRY301)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AbletonMCP_Remote_Script/handlers/tracks.py` around lines 554 - 586, The code
has off-by-one errors in relocate_track: when inserting "after" the last member
you must set the insertion index to one past the last member (because
target_index creates a new track at that index), so update the "last" branch to
set target = members[-1] + 1 (not members[-1]) and in the "after:" branch change
the cap logic so when computing target = ref_pos + 1 you clamp against
members[-1] + 1 (not members[-1]); adjust any bounds checks accordingly so
target never exceeds the overall tracks length.
| ```json | ||
| { | ||
| "mcpServers": { | ||
| "ableton": { | ||
| "command": "uv", | ||
| "args": ["run", "--directory", "/Users/marcsperzel/Music/MacWhite/production/ableton-mcp", "ableton-mcp-macwhite"] | ||
| } | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Hardcoded user-specific path in .mcp.json example.
The --directory path /Users/marcsperzel/Music/MacWhite/production/ableton-mcp is developer-specific. Other contributors or deployment targets will need to change this. Consider using a placeholder like <path-to-repo> or a relative path, and note that users must customize it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@DEPLOYMENT.md` around lines 66 - 75, The example .mcp.json in DEPLOYMENT.md
currently hardcodes a developer-specific absolute path in the "mcpServers" ->
"ableton" -> "args" -- replace the user-specific --directory value with a
neutral placeholder like "<path-to-repo>" or a relative path (e.g.
"./path/to/ableton-mcp") and add a short note that users must customize this
before running; update the JSON example and any accompanying text to reference
the placeholder and mention it is required to be replaced for different
environments.
| def receive_full_response(self, sock: socket.socket, buffer_size: int = 8192) -> bytes: | ||
| """Receive the complete response, potentially in multiple chunks.""" | ||
| chunks = [] | ||
| sock.settimeout(15.0) |
There was a problem hiding this comment.
receive_full_response always overrides the caller's timeout to 15 s.
send_command sets self.sock.settimeout(15.0 if is_modifying_command else 10.0) at line 117, but receive_full_response immediately overrides it with sock.settimeout(15.0) at line 45. This means read-only commands never get the intended 10-second timeout — they always get 15 seconds.
Either pass the desired timeout into receive_full_response or remove the redundant settimeout in one of the two locations.
Proposed fix
- def receive_full_response(self, sock: socket.socket, buffer_size: int = 8192) -> bytes:
+ def receive_full_response(self, sock: socket.socket, buffer_size: int = 8192, timeout: float = 15.0) -> bytes:
"""Receive the complete response, potentially in multiple chunks."""
chunks = []
- sock.settimeout(15.0)
+ sock.settimeout(timeout)Then at the call site:
- self.sock.settimeout(15.0 if is_modifying_command else 10.0)
- response_data = self.receive_full_response(self.sock)
+ timeout = 15.0 if is_modifying_command else 10.0
+ response_data = self.receive_full_response(self.sock, timeout=timeout)Also applies to: 117-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCP_Server/connection.py` around lines 42 - 45, The receive_full_response
method currently forces a 15s socket timeout (sock.settimeout(15.0)), which
overrides the timeout set by send_command (self.sock.settimeout(15.0 if
is_modifying_command else 10.0)); remove the redundant hardcoded timeout in
receive_full_response and instead accept a timeout parameter (e.g., timeout:
float = None) or rely on the caller-set socket timeout so read-only commands get
the intended 10s; update the call site(s) (send_command) to pass the desired
timeout into receive_full_response or ensure send_command sets the socket
timeout immediately before calling receive_full_response and no other settimeout
exists in receive_full_response.
| def get_ableton_connection() -> AbletonConnection: | ||
| """Get or create a persistent Ableton connection.""" | ||
| global _ableton_connection | ||
|
|
||
| if _ableton_connection is not None: | ||
| try: | ||
| # Test the connection with a simple ping | ||
| # We'll try to send an empty message, which should fail if the connection is dead | ||
| # but won't affect Ableton if it's alive | ||
| _ableton_connection.sock.settimeout(1.0) | ||
| _ableton_connection.sock.sendall(b'') | ||
| _ableton_connection.sock.sendall(b"") | ||
| return _ableton_connection | ||
| except Exception as e: | ||
| logger.warning(f"Existing connection is no longer valid: {str(e)}") | ||
| try: | ||
| _ableton_connection.disconnect() | ||
| except: | ||
| except Exception: | ||
| pass | ||
| _ableton_connection = None | ||
|
|
||
| # Connection doesn't exist or is invalid, create a new one | ||
| if _ableton_connection is None: | ||
| # Try to connect up to 3 times with a short delay between attempts | ||
| max_attempts = 3 | ||
| for attempt in range(1, max_attempts + 1): | ||
| try: | ||
| logger.info(f"Connecting to Ableton (attempt {attempt}/{max_attempts})...") | ||
| _ableton_connection = AbletonConnection(host="localhost", port=9877) | ||
| if _ableton_connection.connect(): | ||
| logger.info("Created new persistent connection to Ableton") | ||
|
|
||
| # Validate connection with a simple command | ||
| try: | ||
| # Get session info as a test | ||
| _ableton_connection.send_command("get_session_info") | ||
| logger.info("Connection validated successfully") | ||
| return _ableton_connection | ||
| except Exception as e: | ||
| logger.error(f"Connection validation failed: {str(e)}") | ||
| _ableton_connection.disconnect() | ||
| _ableton_connection = None | ||
| # Continue to next attempt | ||
| else: | ||
| _ableton_connection = None | ||
| except Exception as e: | ||
| logger.error(f"Connection attempt {attempt} failed: {str(e)}") | ||
| if _ableton_connection: | ||
| max_attempts = 3 | ||
| for attempt in range(1, max_attempts + 1): | ||
| try: | ||
| logger.info(f"Connecting to Ableton (attempt {attempt}/{max_attempts})...") | ||
| _ableton_connection = AbletonConnection(host="localhost", port=9877) | ||
| if _ableton_connection.connect(): | ||
| logger.info("Created new persistent connection to Ableton") | ||
| try: | ||
| _ableton_connection.send_command("get_session_info") | ||
| logger.info("Connection validated successfully") | ||
| return _ableton_connection | ||
| except Exception as e: | ||
| logger.error(f"Connection validation failed: {str(e)}") | ||
| _ableton_connection.disconnect() | ||
| _ableton_connection = None | ||
|
|
||
| # Wait before trying again, but only if we have more attempts left | ||
| if attempt < max_attempts: | ||
| import time | ||
| time.sleep(1.0) | ||
|
|
||
| # If we get here, all connection attempts failed | ||
| if _ableton_connection is None: | ||
| logger.error("Failed to connect to Ableton after multiple attempts") | ||
| raise Exception("Could not connect to Ableton. Make sure the Remote Script is running.") | ||
|
|
||
| return _ableton_connection | ||
|
|
||
|
|
||
| # Core Tool endpoints | ||
|
|
||
| @mcp.tool() | ||
| def get_session_info(ctx: Context) -> str: | ||
| """Get detailed information about the current Ableton session""" | ||
| try: | ||
| ableton = get_ableton_connection() | ||
| result = ableton.send_command("get_session_info") | ||
| return json.dumps(result, indent=2) | ||
| except Exception as e: | ||
| logger.error(f"Error getting session info from Ableton: {str(e)}") | ||
| return f"Error getting session info: {str(e)}" | ||
|
|
||
| @mcp.tool() | ||
| def get_track_info(ctx: Context, track_index: int) -> str: | ||
| """ | ||
| Get detailed information about a specific track in Ableton. | ||
|
|
||
| Parameters: | ||
| - track_index: The index of the track to get information about | ||
| """ | ||
| try: | ||
| ableton = get_ableton_connection() | ||
| result = ableton.send_command("get_track_info", {"track_index": track_index}) | ||
| return json.dumps(result, indent=2) | ||
| except Exception as e: | ||
| logger.error(f"Error getting track info from Ableton: {str(e)}") | ||
| return f"Error getting track info: {str(e)}" | ||
|
|
||
| @mcp.tool() | ||
| def create_midi_track(ctx: Context, index: int = -1) -> str: | ||
| """ | ||
| Create a new MIDI track in the Ableton session. | ||
|
|
||
| Parameters: | ||
| - index: The index to insert the track at (-1 = end of list) | ||
| """ | ||
| try: | ||
| ableton = get_ableton_connection() | ||
| result = ableton.send_command("create_midi_track", {"index": index}) | ||
| return f"Created new MIDI track: {result.get('name', 'unknown')}" | ||
| except Exception as e: | ||
| logger.error(f"Error creating MIDI track: {str(e)}") | ||
| return f"Error creating MIDI track: {str(e)}" | ||
|
|
||
|
|
||
| @mcp.tool() | ||
| def set_track_name(ctx: Context, track_index: int, name: str) -> str: | ||
| """ | ||
| Set the name of a track. | ||
|
|
||
| Parameters: | ||
| - track_index: The index of the track to rename | ||
| - name: The new name for the track | ||
| """ | ||
| try: | ||
| ableton = get_ableton_connection() | ||
| result = ableton.send_command("set_track_name", {"track_index": track_index, "name": name}) | ||
| return f"Renamed track to: {result.get('name', name)}" | ||
| except Exception as e: | ||
| logger.error(f"Error setting track name: {str(e)}") | ||
| return f"Error setting track name: {str(e)}" | ||
|
|
||
| @mcp.tool() | ||
| def create_clip(ctx: Context, track_index: int, clip_index: int, length: float = 4.0) -> str: | ||
| """ | ||
| Create a new MIDI clip in the specified track and clip slot. | ||
|
|
||
| Parameters: | ||
| - track_index: The index of the track to create the clip in | ||
| - clip_index: The index of the clip slot to create the clip in | ||
| - length: The length of the clip in beats (default: 4.0) | ||
| """ | ||
| try: | ||
| ableton = get_ableton_connection() | ||
| result = ableton.send_command("create_clip", { | ||
| "track_index": track_index, | ||
| "clip_index": clip_index, | ||
| "length": length | ||
| }) | ||
| return f"Created new clip at track {track_index}, slot {clip_index} with length {length} beats" | ||
| except Exception as e: | ||
| logger.error(f"Error creating clip: {str(e)}") | ||
| return f"Error creating clip: {str(e)}" | ||
|
|
||
| @mcp.tool() | ||
| def add_notes_to_clip( | ||
| ctx: Context, | ||
| track_index: int, | ||
| clip_index: int, | ||
| notes: List[Dict[str, Union[int, float, bool]]] | ||
| ) -> str: | ||
| """ | ||
| Add MIDI notes to a clip. | ||
|
|
||
| Parameters: | ||
| - track_index: The index of the track containing the clip | ||
| - clip_index: The index of the clip slot containing the clip | ||
| - notes: List of note dictionaries, each with pitch, start_time, duration, velocity, and mute | ||
| """ | ||
| try: | ||
| ableton = get_ableton_connection() | ||
| result = ableton.send_command("add_notes_to_clip", { | ||
| "track_index": track_index, | ||
| "clip_index": clip_index, | ||
| "notes": notes | ||
| }) | ||
| return f"Added {len(notes)} notes to clip at track {track_index}, slot {clip_index}" | ||
| except Exception as e: | ||
| logger.error(f"Error adding notes to clip: {str(e)}") | ||
| return f"Error adding notes to clip: {str(e)}" | ||
|
|
||
| @mcp.tool() | ||
| def set_clip_name(ctx: Context, track_index: int, clip_index: int, name: str) -> str: | ||
| """ | ||
| Set the name of a clip. | ||
|
|
||
| Parameters: | ||
| - track_index: The index of the track containing the clip | ||
| - clip_index: The index of the clip slot containing the clip | ||
| - name: The new name for the clip | ||
| """ | ||
| try: | ||
| ableton = get_ableton_connection() | ||
| result = ableton.send_command("set_clip_name", { | ||
| "track_index": track_index, | ||
| "clip_index": clip_index, | ||
| "name": name | ||
| }) | ||
| return f"Renamed clip at track {track_index}, slot {clip_index} to '{name}'" | ||
| except Exception as e: | ||
| logger.error(f"Error setting clip name: {str(e)}") | ||
| return f"Error setting clip name: {str(e)}" | ||
|
|
||
| @mcp.tool() | ||
| def set_tempo(ctx: Context, tempo: float) -> str: | ||
| """ | ||
| Set the tempo of the Ableton session. | ||
|
|
||
| Parameters: | ||
| - tempo: The new tempo in BPM | ||
| """ | ||
| try: | ||
| ableton = get_ableton_connection() | ||
| result = ableton.send_command("set_tempo", {"tempo": tempo}) | ||
| return f"Set tempo to {tempo} BPM" | ||
| except Exception as e: | ||
| logger.error(f"Error setting tempo: {str(e)}") | ||
| return f"Error setting tempo: {str(e)}" | ||
|
|
||
|
|
||
| @mcp.tool() | ||
| def load_instrument_or_effect(ctx: Context, track_index: int, uri: str) -> str: | ||
| """ | ||
| Load an instrument or effect onto a track using its URI. | ||
|
|
||
| Parameters: | ||
| - track_index: The index of the track to load the instrument on | ||
| - uri: The URI of the instrument or effect to load (e.g., 'query:Synths#Instrument%20Rack:Bass:FileId_5116') | ||
| """ | ||
| try: | ||
| ableton = get_ableton_connection() | ||
| result = ableton.send_command("load_browser_item", { | ||
| "track_index": track_index, | ||
| "item_uri": uri | ||
| }) | ||
|
|
||
| # Check if the instrument was loaded successfully | ||
| if result.get("loaded", False): | ||
| new_devices = result.get("new_devices", []) | ||
| if new_devices: | ||
| return f"Loaded instrument with URI '{uri}' on track {track_index}. New devices: {', '.join(new_devices)}" | ||
| else: | ||
| devices = result.get("devices_after", []) | ||
| return f"Loaded instrument with URI '{uri}' on track {track_index}. Devices on track: {', '.join(devices)}" | ||
| else: | ||
| return f"Failed to load instrument with URI '{uri}'" | ||
| except Exception as e: | ||
| logger.error(f"Error loading instrument by URI: {str(e)}") | ||
| return f"Error loading instrument by URI: {str(e)}" | ||
|
|
||
| @mcp.tool() | ||
| def fire_clip(ctx: Context, track_index: int, clip_index: int) -> str: | ||
| """ | ||
| Start playing a clip. | ||
|
|
||
| Parameters: | ||
| - track_index: The index of the track containing the clip | ||
| - clip_index: The index of the clip slot containing the clip | ||
| """ | ||
| try: | ||
| ableton = get_ableton_connection() | ||
| result = ableton.send_command("fire_clip", { | ||
| "track_index": track_index, | ||
| "clip_index": clip_index | ||
| }) | ||
| return f"Started playing clip at track {track_index}, slot {clip_index}" | ||
| except Exception as e: | ||
| logger.error(f"Error firing clip: {str(e)}") | ||
| return f"Error firing clip: {str(e)}" | ||
|
|
||
| @mcp.tool() | ||
| def stop_clip(ctx: Context, track_index: int, clip_index: int) -> str: | ||
| """ | ||
| Stop playing a clip. | ||
|
|
||
| Parameters: | ||
| - track_index: The index of the track containing the clip | ||
| - clip_index: The index of the clip slot containing the clip | ||
| """ | ||
| try: | ||
| ableton = get_ableton_connection() | ||
| result = ableton.send_command("stop_clip", { | ||
| "track_index": track_index, | ||
| "clip_index": clip_index | ||
| }) | ||
| return f"Stopped clip at track {track_index}, slot {clip_index}" | ||
| except Exception as e: | ||
| logger.error(f"Error stopping clip: {str(e)}") | ||
| return f"Error stopping clip: {str(e)}" | ||
|
|
||
| @mcp.tool() | ||
| def start_playback(ctx: Context) -> str: | ||
| """Start playing the Ableton session.""" | ||
| try: | ||
| ableton = get_ableton_connection() | ||
| result = ableton.send_command("start_playback") | ||
| return "Started playback" | ||
| except Exception as e: | ||
| logger.error(f"Error starting playback: {str(e)}") | ||
| return f"Error starting playback: {str(e)}" | ||
|
|
||
| @mcp.tool() | ||
| def stop_playback(ctx: Context) -> str: | ||
| """Stop playing the Ableton session.""" | ||
| try: | ||
| ableton = get_ableton_connection() | ||
| result = ableton.send_command("stop_playback") | ||
| return "Stopped playback" | ||
| except Exception as e: | ||
| logger.error(f"Error stopping playback: {str(e)}") | ||
| return f"Error stopping playback: {str(e)}" | ||
| except Exception as e: | ||
| logger.error(f"Connection attempt {attempt} failed: {str(e)}") | ||
| if _ableton_connection: | ||
| _ableton_connection.disconnect() | ||
| _ableton_connection = None | ||
| if attempt < max_attempts: | ||
| time.sleep(1.0) | ||
| raise Exception( | ||
| "Could not connect to Ableton. Make sure the Remote Script is running." | ||
| ) |
There was a problem hiding this comment.
sendall(b"") does not reliably detect dead TCP connections.
On most OS TCP stacks, sending zero bytes succeeds even when the peer has closed the connection (no actual data is transmitted on the wire). This means get_ableton_connection() can return a stale socket that will only fail on the next real send_command call.
A more reliable approach is to attempt a non-blocking recv or use sock.getpeername(), or simply rely on the send_command call itself to detect failures and reconnect.
Proposed fix — use getpeername() instead
try:
- _ableton_connection.sock.settimeout(1.0)
- _ableton_connection.sock.sendall(b"")
+ _ableton_connection.sock.getpeername()
return _ableton_connection🧰 Tools
🪛 Ruff (0.15.1)
[warning] 55-55: Consider moving this statement to an else block
(TRY300)
[warning] 56-56: Do not catch blind exception: Exception
(BLE001)
[warning] 57-57: Use explicit conversion flag
Replace with conversion flag
(RUF010)
[error] 60-61: try-except-pass detected, consider logging the exception
(S110)
[warning] 60-60: Do not catch blind exception: Exception
(BLE001)
[warning] 73-73: Consider moving this statement to an else block
(TRY300)
[warning] 74-74: Do not catch blind exception: Exception
(BLE001)
[warning] 75-75: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 75-75: Use explicit conversion flag
Replace with conversion flag
(RUF010)
[warning] 78-78: Do not catch blind exception: Exception
(BLE001)
[warning] 79-79: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 79-79: Use explicit conversion flag
Replace with conversion flag
(RUF010)
[warning] 85-87: Create your own exception
(TRY002)
[warning] 85-87: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCP_Server/server.py` around lines 48 - 87, The current health-check in
get_ableton_connection uses _ableton_connection.sock.sendall(b"") which can
falsely succeed on closed TCP peers; replace that probe with a reliable check
such as calling _ableton_connection.sock.getpeername() (inside the existing
try/except) or performing a non-blocking recv() that would raise on a dead peer,
and remove the sendall(b"") call; if getpeername() (or recv) raises, log the
error, call _ableton_connection.disconnect(), set _ableton_connection = None,
and let the existing reconnect loop create a fresh AbletonConnection and
validate it via send_command("get_session_info") as already implemented.
| @mcp.tool() | ||
| def get_rack_device_info( | ||
| ctx: Context, | ||
| track_index: int, | ||
| device_index: int, | ||
| track_type: str = "track", | ||
| ) -> str: | ||
| """Get detailed information about a rack device's chains and nested devices. | ||
| Recursively serializes all chains and devices. Parameters: track_index, | ||
| device_index (the rack), track_type ('track'|'return'|'master').""" | ||
| try: | ||
| ableton = get_ableton_connection() | ||
| result = ableton.send_command("get_rack_device_info", { | ||
| "track_index": track_index, | ||
| "device_index": device_index, | ||
| "track_type": track_type, | ||
| }) | ||
| return json.dumps(result, indent=2) | ||
| except Exception as e: | ||
| logger.error(f"Error getting rack device info: {str(e)}") | ||
| return "Error getting rack device info" |
There was a problem hiding this comment.
get_rack_device_info error message omits exception details, unlike all other tools.
Line 77 returns "Error getting rack device info" without str(e), while every other tool in this file includes the exception message. This makes debugging harder.
Proposed fix
- return "Error getting rack device info"
+ return f"Error getting rack device info: {str(e)}"📝 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.tool() | |
| def get_rack_device_info( | |
| ctx: Context, | |
| track_index: int, | |
| device_index: int, | |
| track_type: str = "track", | |
| ) -> str: | |
| """Get detailed information about a rack device's chains and nested devices. | |
| Recursively serializes all chains and devices. Parameters: track_index, | |
| device_index (the rack), track_type ('track'|'return'|'master').""" | |
| try: | |
| ableton = get_ableton_connection() | |
| result = ableton.send_command("get_rack_device_info", { | |
| "track_index": track_index, | |
| "device_index": device_index, | |
| "track_type": track_type, | |
| }) | |
| return json.dumps(result, indent=2) | |
| except Exception as e: | |
| logger.error(f"Error getting rack device info: {str(e)}") | |
| return "Error getting rack device info" | |
| `@mcp.tool`() | |
| def get_rack_device_info( | |
| ctx: Context, | |
| track_index: int, | |
| device_index: int, | |
| track_type: str = "track", | |
| ) -> str: | |
| """Get detailed information about a rack device's chains and nested devices. | |
| Recursively serializes all chains and devices. Parameters: track_index, | |
| device_index (the rack), track_type ('track'|'return'|'master').""" | |
| try: | |
| ableton = get_ableton_connection() | |
| result = ableton.send_command("get_rack_device_info", { | |
| "track_index": track_index, | |
| "device_index": device_index, | |
| "track_type": track_type, | |
| }) | |
| return json.dumps(result, indent=2) | |
| except Exception as e: | |
| logger.error(f"Error getting rack device info: {str(e)}") | |
| return f"Error getting rack device info: {str(e)}" |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 59-59: Unused function argument: ctx
(ARG001)
[warning] 75-75: Do not catch blind exception: Exception
(BLE001)
[warning] 76-76: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 76-76: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCP_Server/tools/devices.py` around lines 57 - 77, The get_rack_device_info
tool returns a generic error string without the exception details; update the
except block in get_rack_device_info to return the exception message (e.g.
include str(e) or an f-string like "Error getting rack device info: {e}") so the
returned value matches other tools and aids debugging while keeping the existing
logger.error call.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
meter.py (1)
160-186: Hardcoded track indices make the hierarchy section useless for other usersThe
FADER LEVELSblock is tied to specific track indices (6, 7, 22, 23) that only match the author's project. For any other user the block prints the heading but no content, with no indication why. Consider removing this section or making the indices configurable via CLI arguments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@meter.py` around lines 160 - 186, The FADER LEVELS block currently relies on hardcoded track indices (6,7,22,23) inside the results loop (variables kick, drums_grp, bass_track, bass_grp, master), which fails for other users; change this by making the indices configurable and only showing the heading when at least one configured item is found: add a CLI/config option (or environment variable) that supplies the mapping of logical names to track indices, replace the hardcoded index checks in the loop with lookups into that mapping (e.g., using keys "kick", "drums_grp", "bass_track", "bass_grp"), and print the FADER LEVELS heading and each line only if the corresponding entry was found in results (otherwise skip it or print a clear message about using the config).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@meter.py`:
- Around line 53-59: collect_samples currently ignores the CLI flags and always
uses sample_meters defaults; add boolean parameters include_returns and
include_master to collect_samples and pass them through to sample_meters (i.e.,
call sample_meters(conn, track_indices, include_returns=...,
include_master=...)), then update any callers (the CLI entry that parses
args.no_returns / args.no_master) to call collect_samples(...,
include_returns=not args.no_returns, include_master=not args.no_master) so the
flags actually take effect; reference functions collect_samples and
sample_meters and the CLI symbols args.no_returns / args.no_master when making
the changes.
- Around line 135-138: The meter_bar function can produce a bar longer than
width when value > 1.0 (or negative), so clamp the computed filled count to the
valid range [0, width] before building the string: compute filled = max(0,
min(width, int(value * width))) (or equivalent) and then return the
concatenation using that clamped filled value so the result never exceeds the
specified width; update meter_bar accordingly.
- Around line 83-125: The analyze function produces inconsistent dicts: silent
branch omits the "samples" key and the loop variable key is unused; update the
silent-entry dict to include "samples": len(values) (same as the non-silent
branch) and remove the unused loop variable by iterating over
all_samples.values() or renaming key to _, e.g., change "for key, data in
all_samples.items()" to "for data in all_samples.values()" (or "for _, data in
...") so Ruff B007 is resolved and all result entries share the same schema.
---
Nitpick comments:
In `@meter.py`:
- Around line 160-186: The FADER LEVELS block currently relies on hardcoded
track indices (6,7,22,23) inside the results loop (variables kick, drums_grp,
bass_track, bass_grp, master), which fails for other users; change this by
making the indices configurable and only showing the heading when at least one
configured item is found: add a CLI/config option (or environment variable) that
supplies the mapping of logical names to track indices, replace the hardcoded
index checks in the loop with lookups into that mapping (e.g., using keys
"kick", "drums_grp", "bass_track", "bass_grp"), and print the FADER LEVELS
heading and each line only if the corresponding entry was found in results
(otherwise skip it or print a clear message about using the config).
| def collect_samples(conn, num_samples, duration, track_indices=None): | ||
| """Collect multiple meter samples over a time window.""" | ||
| interval = duration / max(num_samples - 1, 1) | ||
| all_samples = {} | ||
|
|
||
| for i in range(num_samples): | ||
| meters = sample_meters(conn, track_indices) |
There was a problem hiding this comment.
--no-returns / --no-master flags are parsed but never take effect
collect_samples does not accept include_returns or include_master parameters, so it always calls sample_meters(conn, track_indices) with their defaults (True/True). The CLI flags at lines 205–206 are parsed into args.no_returns / args.no_master but are never forwarded — both flags are silently ignored.
🐛 Proposed fix
-def collect_samples(conn, num_samples, duration, track_indices=None):
+def collect_samples(conn, num_samples, duration, track_indices=None,
+ include_returns=True, include_master=True):
"""Collect multiple meter samples over a time window."""
interval = duration / max(num_samples - 1, 1)
all_samples = {}
for i in range(num_samples):
- meters = sample_meters(conn, track_indices)
+ meters = sample_meters(conn, track_indices,
+ include_returns=include_returns,
+ include_master=include_master)- all_samples = collect_samples(conn, args.samples, args.time, track_indices)
+ all_samples = collect_samples(
+ conn, args.samples, args.time, track_indices,
+ include_returns=not args.no_returns,
+ include_master=not args.no_master,
+ )Also applies to: 199-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@meter.py` around lines 53 - 59, collect_samples currently ignores the CLI
flags and always uses sample_meters defaults; add boolean parameters
include_returns and include_master to collect_samples and pass them through to
sample_meters (i.e., call sample_meters(conn, track_indices,
include_returns=..., include_master=...)), then update any callers (the CLI
entry that parses args.no_returns / args.no_master) to call collect_samples(...,
include_returns=not args.no_returns, include_master=not args.no_master) so the
flags actually take effect; reference functions collect_samples and
sample_meters and the CLI symbols args.no_returns / args.no_master when making
the changes.
| def analyze(all_samples): | ||
| """Compute statistics for each track.""" | ||
| results = [] | ||
| for key, data in all_samples.items(): | ||
| values = data["values"] | ||
| fader_db = parse_volume_db(data["volume_db"]) | ||
| is_muted = data.get("mute", False) | ||
|
|
||
| if not any(v > 0 for v in values) or is_muted: | ||
| results.append({ | ||
| "name": data["name"], | ||
| "type": data["type"], | ||
| "index": data["index"], | ||
| "fader": data["volume"], | ||
| "fader_db": fader_db, | ||
| "fader_db_str": data["volume_db"], | ||
| "meter_peak": 0.0, | ||
| "meter_avg": 0.0, | ||
| "silent": True, | ||
| "mute": is_muted, | ||
| }) | ||
| continue | ||
|
|
||
| peak = max(values) | ||
| avg = sum(values) / len(values) | ||
|
|
||
| results.append({ | ||
| "name": data["name"], | ||
| "type": data["type"], | ||
| "index": data["index"], | ||
| "fader": data["volume"], | ||
| "fader_db": fader_db, | ||
| "fader_db_str": data["volume_db"], | ||
| "meter_peak": peak, | ||
| "meter_avg": avg, | ||
| "samples": len(values), | ||
| "silent": False, | ||
| "mute": is_muted, | ||
| }) | ||
|
|
||
| type_order = {"track": 0, "return": 1, "master": 2} | ||
| results.sort(key=lambda r: (type_order.get(r["type"], 9), r["index"])) | ||
| return results |
There was a problem hiding this comment.
Silent-track result dict is missing the "samples" key — inconsistent schema
Non-silent entries include "samples": len(values) (line 118), but silent entries omit it entirely (lines 92–103). Any downstream consumer iterating over results uniformly (e.g., the JSON output path, or future analysis code) will encounter a KeyError. Additionally, the Ruff linter flags the loop variable key at line 86 as unused (B007).
🐛 Proposed fixes
if not any(v > 0 for v in values) or is_muted:
results.append({
"name": data["name"],
"type": data["type"],
"index": data["index"],
"fader": data["volume"],
"fader_db": fader_db,
"fader_db_str": data["volume_db"],
"meter_peak": 0.0,
"meter_avg": 0.0,
+ "samples": len(values),
"silent": True,
"mute": is_muted,
})- for key, data in all_samples.items():
+ for _key, data in all_samples.items():🧰 Tools
🪛 Ruff (0.15.1)
[warning] 86-86: Loop control variable key not used within loop body
Rename unused key to _key
(B007)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@meter.py` around lines 83 - 125, The analyze function produces inconsistent
dicts: silent branch omits the "samples" key and the loop variable key is
unused; update the silent-entry dict to include "samples": len(values) (same as
the non-silent branch) and remove the unused loop variable by iterating over
all_samples.values() or renaming key to _, e.g., change "for key, data in
all_samples.items()" to "for data in all_samples.values()" (or "for _, data in
...") so Ruff B007 is resolved and all result entries share the same schema.
| def meter_bar(value, width=20): | ||
| """ASCII meter bar for visual level comparison.""" | ||
| filled = int(value * width) | ||
| return "\u2588" * filled + "\u2591" * (width - filled) |
There was a problem hiding this comment.
meter_bar renders wider than width when value > 1.0
"\u2591" * (width - filled) silently evaluates to "" when filled > width, producing a bar that overflows the intended column width and breaks the table alignment.
🐛 Proposed fix
def meter_bar(value, width=20):
"""ASCII meter bar for visual level comparison."""
- filled = int(value * width)
+ filled = int(min(max(value, 0.0), 1.0) * width)
return "\u2588" * filled + "\u2591" * (width - filled)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@meter.py` around lines 135 - 138, The meter_bar function can produce a bar
longer than width when value > 1.0 (or negative), so clamp the computed filled
count to the valid range [0, width] before building the string: compute filled =
max(0, min(width, int(value * width))) (or equivalent) and then return the
concatenation using that clamped filled value so the result never exceeds the
specified width; update meter_bar accordingly.
- Add get_track_routing and set_track_routing to dispatch.py (inlined for hot-reload) - Log reload errors in create_instance instead of silently swallowing them - Add get_track_routing/set_track_routing to tracks.py for reference Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
AbletonMCP_Remote_Script/__init__.py (4)
392-455: The main-thread dispatch pattern is duplicated betweenMODIFYING_COMMANDS(Lines 393–420) and dynamic dispatch (Lines 422–455).Both branches create a
response_queue, define a task closure, callschedule_message, handleAssertionErrorfallback, and drain the queue with a 10-second timeout. Consider extracting a shared helper to reduce the ~30 lines of identical boilerplate.Example helper extraction
def _run_on_main_thread(self, task_fn): """Schedule task_fn on the main thread and return the result dict.""" response_queue = queue.Queue() def wrapper(): try: result = task_fn() response_queue.put({"status": "success", "result": result}) except Exception as e: self.log_message("Error in main thread task: " + str(e)) self.log_message(traceback.format_exc()) response_queue.put({"status": "error", "message": str(e)}) try: self.schedule_message(0, wrapper) except AssertionError: wrapper() return response_queue.get(timeout=10.0)Then in
_process_command:elif command_type in MODIFYING_COMMANDS: task_response = self._run_on_main_thread( lambda: _dispatch_modifying(command_type, params, song, ctrl) ) # handle task_response...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/__init__.py` around lines 392 - 455, Duplicate main-thread scheduling/response logic in _process_command for MODIFYING_COMMANDS and dynamic dispatch; extract a helper (e.g. _run_on_main_thread) that encapsulates creating response_queue, wrapping a task function with try/except logging (use self.log_message and traceback.format_exc()), calling self.schedule_message(0, wrapper) with the AssertionError fallback, and returning the task response dict (or raising/propagating queue.Empty). Replace both blocks to call this helper with a lambda that calls _dispatch_modifying(...) or handlers.dispatch.execute(...), then handle the returned dict the same way you already do (check "status" == "error" and set response fields or set response["result"]).
468-870:_dispatch_modifyingis a module-level function with a long if-chain — consider using a dispatch dictionary.This 400-line function is a series of
if command_type == .../return ...statements. A dictionary mapping command names to handler callables (similar todispatch.py's_get_registry) would be more maintainable and eliminate the linear scan.That said, this is a non-functional refactor PR, so this can be deferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/__init__.py` around lines 468 - 870, _long if-chain in module-level function _dispatch_modifying makes maintenance hard; replace it with a dispatch dictionary mapping command_type strings to callables to remove the long linear scan. Create a dict (e.g., MODIFYING_DISPATCH) where keys are the command strings and values are small wrapper callables or lambdas that accept (params, song, ctrl) and call the existing handlers (handlers.tracks.create_midi_track, handlers.clips.create_clip, handlers.session.set_tempo, etc.); then rewrite _dispatch_modifying to look up command_type in MODIFYING_DISPATCH, call the callable with (p, song, ctrl) and raise the same ValueError if missing. Keep existing parameter defaults by having each wrapper extract values from params with p.get(...) so behavior remains identical.
47-67: Hot-reload iterateshandlers.__dict__which may include non-module attributes.
sorted(handlers.__dict__)includes all attributes of thehandlerspackage — not just submodules but also any top-level variables, functions, or re-exported symbols. Thehasattr(submod, "__file__")guard filters well enough in practice, but iterating over all dict entries is slightly wasteful. Consider usingpkgutil.iter_modulesor a whitelist of known handler module names for clarity and to avoid reloading unexpected objects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/__init__.py` around lines 47 - 67, The hot-reload loop in create_instance currently iterates handlers.__dict__ which can contain non-module attributes; change it to explicitly discover and iterate actual submodules of the handlers package (e.g., using pkgutil.iter_modules(handlers.__path__) or a predefined whitelist) and import each module via importlib.import_module before calling importlib.reload(module); keep the existing error handling that logs to c_instance.log_message on reload failure and ensure you reference the same symbols (create_instance, handlers, importlib.reload, c_instance.log_message) when making the replacement.
305-390:create_locatorblocks the socket-handler thread waiting for main-thread ticks — verify timeout is sufficient.The locator workflow schedules three successive steps via
schedule_message(ticks 1, +2, +2), then blocks onlocator_result_queue.get(timeout=10.0). This is a total of ~5 ticks before the result is enqueued. If Ableton's main loop is under heavy load (large session, heavy GUI), ticks may be delayed, potentially causing a 10-second timeout even under normal conditions.This is acceptable for now but consider documenting or increasing the timeout if users report sporadic "Timeout waiting for locator creation" errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/__init__.py` around lines 305 - 390, The create_locator handler blocks the socket thread waiting on locator_result_queue.get(timeout=10.0) while schedule_message triggers locator_step1/2/3 across several ticks (locator_step1, locator_step2, locator_step3), so increase robustness by replacing the hard-coded 10.0 with a configurable constant (e.g., LOCATOR_TIMEOUT) or a larger value (e.g., 30s) and/or implement a retry/extended-wait loop; update the code around the locator_result_queue.get(timeout=10.0) call to use that constant, add a clear log message when a timeout occurs, and document the timeout choice so users can adjust it if they see sporadic "Timeout waiting for locator creation" errors.AbletonMCP_Remote_Script/handlers/tracks.py (1)
71-95: Inconsistent handling ofindex < 0betweencreate_midi_trackandcreate_audio_track.
create_midi_trackpassesindexdirectly to the API (including-1) and then infersnew_track_indexpost-hoc.create_audio_tracknormalizes negative indices tolen(song.tracks)before the API call. If the Ableton API treats-1as "append," both functions work, but the asymmetry is fragile — if someone passes-2tocreate_midi_trackit goes straight through, whilecreate_audio_trackwould clamp it tolen(song.tracks).Consider unifying the index normalization logic, e.g., always normalizing before the call.
Suggested normalization for create_midi_track
def create_midi_track(song, index, ctrl=None): """Create a new MIDI track at the specified index.""" try: + if index < 0: + index = len(song.tracks) song.create_midi_track(index) - new_track_index = len(song.tracks) - 1 if index == -1 else index - new_track = song.tracks[new_track_index] - return {"index": new_track_index, "name": new_track.name} + new_track = song.tracks[index] + return {"index": index, "name": new_track.name} except Exception as e:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/handlers/tracks.py` around lines 71 - 95, create_midi_track and create_audio_track handle negative indices inconsistently (create_midi_track passes negatives like -1 directly to song.create_midi_track while create_audio_track normalizes negatives to len(song.tracks)); unify by normalizing the index in create_midi_track the same way as create_audio_track (e.g., if index < 0 then set index = len(song.tracks)) before calling song.create_midi_track, then compute new_track_index from that normalized index and return it (update references to new_track_index/new_track accordingly) while preserving the existing exception logging via ctrl.log_message and re-raising.AbletonMCP_Remote_Script/handlers/dispatch.py (1)
240-259:_get_registry()is rebuilt on every call — three times per dispatched command.A single dynamic command dispatch goes through
is_known()→is_modifying()→execute(), each calling_get_registry()which builds a new dict with lambda closures. While the overhead is small, it's unnecessary.Consider caching or building the registry once per
executecall, or at minimum merging the three lookups:Example: single-lookup execute
def execute(command_type, params, song, ctrl): """Execute a dynamically registered command.""" reg = _get_registry() if command_type not in reg: raise ValueError("Unknown dynamic command: " + command_type) + entry = reg[command_type] + # Caller already checked is_modifying via __init__.py, so just execute - return reg[command_type]["handler"](song, params, ctrl) + return entry["handler"](song, params, ctrl)Or provide a combined lookup function to avoid redundant rebuilds:
def lookup(command_type): """Return (is_known, is_modifying) without rebuilding the registry multiple times.""" reg = _get_registry() if command_type not in reg: return False, False return True, reg[command_type].get("modifying", False)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/handlers/dispatch.py` around lines 240 - 259, The registry returned by _get_registry() is rebuilt on each call causing three redundant builds per dispatch; change the flow so the registry is built once per dispatch by adding a single lookup that calls _get_registry() once (e.g., a new helper like _lookup(command_type) that returns (found, entry) or by modifying execute() to build reg = _get_registry() and then perform the known/modifying checks and handler call using that reg), then update is_known, is_modifying, and execute to use that single lookup/registry to avoid repeated rebuilds while preserving existing return values and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AbletonMCP_Remote_Script/handlers/dispatch.py`:
- Around line 12-23: The _resolve_track function in dispatch.py duplicates
existing logic in devices.resolve_track; remove the local _resolve_track
definition and update all references to call devices.resolve_track instead
(e.g., replace calls that currently target _resolve_track at the sites mentioned
and anywhere else in this file). Ensure imports remain (devices is already
imported) and run tests to confirm behavior is unchanged after removing the
duplicate function.
- Around line 26-67: The _get_track_routing (and analogous _set_track_routing)
implementations duplicate logic already present in tracks.get_track_routing and
tracks.set_track_routing; replace the body of these wrapper functions to
delegate to the canonical implementations in tracks.py: import the tracks
module, map the incoming dict params (p.get("track_index"), p.get("track_type"),
plus any routing values for _set_) into the argument names expected by
tracks.get_track_routing / tracks.set_track_routing, call those functions, and
return their results (or propagate exceptions) so there is a single source of
truth for routing logic.
In `@AbletonMCP_Remote_Script/handlers/tracks.py`:
- Around line 350-363: The master track entry built when include_master is true
is missing the "mute" key (unlike regular tracks and return tracks), causing
KeyError when consumers access entry["mute"]; update the block that constructs
the master dict (using variables master, vol and appending to results) to
include "mute": master.mute alongside the existing keys and ensure consistency
with the track/return entries and _get_volume_db usage.
- Around line 595-711: The get_track_routing and set_track_routing functions in
tracks.py are duplicate, unused copies of _get_track_routing and
_set_track_routing in dispatch.py; remove the duplicated functions from
tracks.py or consolidate them into a single shared utility and have callers
delegate to it. Specifically, either delete get_track_routing and
set_track_routing from tracks.py (ensuring no external callers reference them)
or extract the routing logic into a new helper (e.g.,
track_routing_utils.get_track_routing / set_track_routing) and update
dispatch.py to call that helper instead of the local _get/_set implementations;
preserve behavior including handling of track_type ("track"/"return"/"master"),
index bounds checks, available_output_routing_types/channels iteration, result
dict keys, and ctrl logging on exceptions.
---
Duplicate comments:
In `@AbletonMCP_Remote_Script/__init__.py`:
- Around line 144-159: The code currently appends decoded strings to buffer
(buffer = "") after each client.recv call, which can raise UnicodeDecodeError
when a multibyte UTF-8 character is split across recv boundaries; change to
accumulate raw bytes (initialize buffer as b""), append the raw data from
client.recv to that bytes buffer, and only decode to str when attempting to
json.loads (or better: use an incremental UTF-8 decoder/bytes.decode at parse
time) so partial multibyte sequences are preserved until complete; update
references around client.recv, buffer, and json.loads to operate on bytes and
decode right before JSON parsing, clearing the bytes buffer only after a
successful json.loads.
In `@AbletonMCP_Remote_Script/handlers/dispatch.py`:
- Around line 143-154: Registry entries "set_return_track_name" and
"load_on_return_track" in handlers.dispatch are unreachable because
_process_command checks MODIFYING_COMMANDS before the dynamic handlers
(handlers.dispatch.is_known), so either delete these two entries from the
dispatch registry (remove the "set_return_track_name" and "load_on_return_track"
keys in the dispatch mapping) or remove them from MODIFYING_COMMANDS and the
related logic in _dispatch_modifying (and tests/use-sites) so the dynamic
registry entries will actually be used; update only one of these approaches to
avoid duplicate handling and adjust any callers/tests accordingly (refer to
symbols: set_return_track_name, load_on_return_track, MODIFYING_COMMANDS,
_dispatch_modifying, _process_command).
In `@AbletonMCP_Remote_Script/handlers/tracks.py`:
- Around line 221-238: The function group_tracks currently only selects the
first track via song.view.selected_track and does not perform any grouping;
update the function to reflect actual behavior by changing the docstring and
return payload instead of falsely returning "grouped": True—e.g., revise the
docstring to say it selects the first track, replace "grouped": True with
"selected": True (keep "track_count" and "name" if still relevant), and ensure
error logging via ctrl.log_message remains intact; alternatively, if real
grouping is required, implement grouping logic inside group_tracks (operate on
song.tracks and create a group/chain as the host API requires) and then return
"grouped": True only after successful grouping.
- Around line 554-586: relocate_track's target computation is off-by-one: when
position == "last" and when position.startswith("after:") the code picks
members[-1] (or caps to members[-1]) which inserts before the intended spot;
change the "last" branch to target = members[-1] + 1, and in the "after:" branch
compute target = ref_pos + 1 but cap against members[-1] + 1 (not members[-1])
or simply remove the cap that resets it back to members[-1]; adjust uses of
members, group_idx and position in relocate_track accordingly so inserting after
a member actually places the new track after it.
---
Nitpick comments:
In `@AbletonMCP_Remote_Script/__init__.py`:
- Around line 392-455: Duplicate main-thread scheduling/response logic in
_process_command for MODIFYING_COMMANDS and dynamic dispatch; extract a helper
(e.g. _run_on_main_thread) that encapsulates creating response_queue, wrapping a
task function with try/except logging (use self.log_message and
traceback.format_exc()), calling self.schedule_message(0, wrapper) with the
AssertionError fallback, and returning the task response dict (or
raising/propagating queue.Empty). Replace both blocks to call this helper with a
lambda that calls _dispatch_modifying(...) or handlers.dispatch.execute(...),
then handle the returned dict the same way you already do (check "status" ==
"error" and set response fields or set response["result"]).
- Around line 468-870: _long if-chain in module-level function
_dispatch_modifying makes maintenance hard; replace it with a dispatch
dictionary mapping command_type strings to callables to remove the long linear
scan. Create a dict (e.g., MODIFYING_DISPATCH) where keys are the command
strings and values are small wrapper callables or lambdas that accept (params,
song, ctrl) and call the existing handlers (handlers.tracks.create_midi_track,
handlers.clips.create_clip, handlers.session.set_tempo, etc.); then rewrite
_dispatch_modifying to look up command_type in MODIFYING_DISPATCH, call the
callable with (p, song, ctrl) and raise the same ValueError if missing. Keep
existing parameter defaults by having each wrapper extract values from params
with p.get(...) so behavior remains identical.
- Around line 47-67: The hot-reload loop in create_instance currently iterates
handlers.__dict__ which can contain non-module attributes; change it to
explicitly discover and iterate actual submodules of the handlers package (e.g.,
using pkgutil.iter_modules(handlers.__path__) or a predefined whitelist) and
import each module via importlib.import_module before calling
importlib.reload(module); keep the existing error handling that logs to
c_instance.log_message on reload failure and ensure you reference the same
symbols (create_instance, handlers, importlib.reload, c_instance.log_message)
when making the replacement.
- Around line 305-390: The create_locator handler blocks the socket thread
waiting on locator_result_queue.get(timeout=10.0) while schedule_message
triggers locator_step1/2/3 across several ticks (locator_step1, locator_step2,
locator_step3), so increase robustness by replacing the hard-coded 10.0 with a
configurable constant (e.g., LOCATOR_TIMEOUT) or a larger value (e.g., 30s)
and/or implement a retry/extended-wait loop; update the code around the
locator_result_queue.get(timeout=10.0) call to use that constant, add a clear
log message when a timeout occurs, and document the timeout choice so users can
adjust it if they see sporadic "Timeout waiting for locator creation" errors.
In `@AbletonMCP_Remote_Script/handlers/dispatch.py`:
- Around line 240-259: The registry returned by _get_registry() is rebuilt on
each call causing three redundant builds per dispatch; change the flow so the
registry is built once per dispatch by adding a single lookup that calls
_get_registry() once (e.g., a new helper like _lookup(command_type) that returns
(found, entry) or by modifying execute() to build reg = _get_registry() and then
perform the known/modifying checks and handler call using that reg), then update
is_known, is_modifying, and execute to use that single lookup/registry to avoid
repeated rebuilds while preserving existing return values and behavior.
In `@AbletonMCP_Remote_Script/handlers/tracks.py`:
- Around line 71-95: create_midi_track and create_audio_track handle negative
indices inconsistently (create_midi_track passes negatives like -1 directly to
song.create_midi_track while create_audio_track normalizes negatives to
len(song.tracks)); unify by normalizing the index in create_midi_track the same
way as create_audio_track (e.g., if index < 0 then set index = len(song.tracks))
before calling song.create_midi_track, then compute new_track_index from that
normalized index and return it (update references to new_track_index/new_track
accordingly) while preserving the existing exception logging via
ctrl.log_message and re-raising.
| def _resolve_track(song, track_index, track_type): | ||
| """Resolve a track object from index and type.""" | ||
| if track_type == "return": | ||
| if track_index < 0 or track_index >= len(song.return_tracks): | ||
| raise IndexError("Return track index out of range") | ||
| return song.return_tracks[track_index] | ||
| elif track_type == "master": | ||
| return song.master_track | ||
| else: | ||
| if track_index < 0 or track_index >= len(song.tracks): | ||
| raise IndexError("Track index out of range") | ||
| return song.tracks[track_index] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
_resolve_track duplicates devices.resolve_track — use the existing one.
This function is a verbatim copy of devices.resolve_track (shown in the relevant snippet from devices.py Lines 5–16). Since devices is already imported on line 9, replace calls to _resolve_track with devices.resolve_track and remove this duplicate.
Proposed fix
-def _resolve_track(song, track_index, track_type):
- """Resolve a track object from index and type."""
- if track_type == "return":
- if track_index < 0 or track_index >= len(song.return_tracks):
- raise IndexError("Return track index out of range")
- return song.return_tracks[track_index]
- elif track_type == "master":
- return song.master_track
- else:
- if track_index < 0 or track_index >= len(song.tracks):
- raise IndexError("Track index out of range")
- return song.tracks[track_index]Then update references at lines 30 and 76:
- track = _resolve_track(song, track_index, track_type)
+ track = devices.resolve_track(song, track_index, track_type)🧰 Tools
🪛 Ruff (0.15.1)
[warning] 16-16: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 22-22: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AbletonMCP_Remote_Script/handlers/dispatch.py` around lines 12 - 23, The
_resolve_track function in dispatch.py duplicates existing logic in
devices.resolve_track; remove the local _resolve_track definition and update all
references to call devices.resolve_track instead (e.g., replace calls that
currently target _resolve_track at the sites mentioned and anywhere else in this
file). Ensure imports remain (devices is already imported) and run tests to
confirm behavior is unchanged after removing the duplicate function.
| def _get_track_routing(song, p, ctrl): | ||
| """Get output routing info for a track.""" | ||
| track_index = p.get("track_index", 0) | ||
| track_type = p.get("track_type", "track") | ||
| track = _resolve_track(song, track_index, track_type) | ||
|
|
||
| current_type = None | ||
| current_channel = None | ||
| try: | ||
| rt = track.output_routing_type | ||
| current_type = rt.display_name if rt else None | ||
| except Exception: | ||
| pass | ||
| try: | ||
| rc = track.output_routing_channel | ||
| current_channel = rc.display_name if rc else None | ||
| except Exception: | ||
| pass | ||
|
|
||
| available_types = [] | ||
| try: | ||
| for rt in track.available_output_routing_types: | ||
| available_types.append(rt.display_name) | ||
| except Exception: | ||
| pass | ||
|
|
||
| available_channels = [] | ||
| try: | ||
| for rc in track.available_output_routing_channels: | ||
| available_channels.append(rc.display_name) | ||
| except Exception: | ||
| pass | ||
|
|
||
| return { | ||
| "index": track_index, | ||
| "name": track.name, | ||
| "track_type": track_type, | ||
| "output_routing_type": current_type, | ||
| "output_routing_channel": current_channel, | ||
| "available_output_routing_types": available_types, | ||
| "available_output_routing_channels": available_channels, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
_get_track_routing and _set_track_routing duplicate functions in tracks.py — consolidate.
As noted in the tracks.py review, the routing logic at lines 26–112 here is nearly identical to tracks.get_track_routing and tracks.set_track_routing (Lines 595–711 of tracks.py). The only difference is parameter extraction (dict p here vs explicit args there). Have these wrappers delegate to the tracks.py functions to maintain a single source of truth.
Proposed consolidated approach
def _get_track_routing(song, p, ctrl):
- """Get output routing info for a track."""
- track_index = p.get("track_index", 0)
- track_type = p.get("track_type", "track")
- track = _resolve_track(song, track_index, track_type)
- ... (30+ lines)
+ """Get output routing info for a track — delegates to tracks module."""
+ return tracks.get_track_routing(
+ song, p.get("track_index", 0), p.get("track_type", "track"), ctrl
+ )
def _set_track_routing(song, p, ctrl):
- """Set output routing for a track by display name."""
- ... (40+ lines)
+ """Set output routing for a track — delegates to tracks module."""
+ return tracks.set_track_routing(
+ song, p.get("track_index", 0),
+ p.get("output_type", None), p.get("output_channel", None),
+ p.get("track_type", "track"), ctrl
+ )🧰 Tools
🪛 Ruff (0.15.1)
[warning] 26-26: Unused function argument: ctrl
(ARG001)
[error] 37-38: try-except-pass detected, consider logging the exception
(S110)
[warning] 37-37: Do not catch blind exception: Exception
(BLE001)
[error] 42-43: try-except-pass detected, consider logging the exception
(S110)
[warning] 42-42: Do not catch blind exception: Exception
(BLE001)
[error] 49-50: try-except-pass detected, consider logging the exception
(S110)
[warning] 49-49: Do not catch blind exception: Exception
(BLE001)
[error] 56-57: try-except-pass detected, consider logging the exception
(S110)
[warning] 56-56: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AbletonMCP_Remote_Script/handlers/dispatch.py` around lines 26 - 67, The
_get_track_routing (and analogous _set_track_routing) implementations duplicate
logic already present in tracks.get_track_routing and tracks.set_track_routing;
replace the body of these wrapper functions to delegate to the canonical
implementations in tracks.py: import the tracks module, map the incoming dict
params (p.get("track_index"), p.get("track_type"), plus any routing values for
_set_) into the argument names expected by tracks.get_track_routing /
tracks.set_track_routing, call those functions, and return their results (or
propagate exceptions) so there is a single source of truth for routing logic.
| if include_master: | ||
| master = song.master_track | ||
| vol = master.mixer_device.volume | ||
| results.append({ | ||
| "index": 0, | ||
| "name": "Master", | ||
| "type": "master", | ||
| "left": master.output_meter_left, | ||
| "right": master.output_meter_right, | ||
| "volume": vol.value, | ||
| "volume_min": vol.min, | ||
| "volume_max": vol.max, | ||
| "volume_db": _get_volume_db(master), | ||
| }) |
There was a problem hiding this comment.
Master track meter entry is missing the "mute" key present in track/return entries.
Regular tracks (Line 334) and return tracks (Line 349) include "mute": track.mute, but the master track entry omits it. Consumers iterating meters and accessing entry["mute"] will get a KeyError for the master track entry.
Proposed fix
results.append({
"index": 0,
"name": "Master",
"type": "master",
"left": master.output_meter_left,
"right": master.output_meter_right,
"volume": vol.value,
"volume_min": vol.min,
"volume_max": vol.max,
"volume_db": _get_volume_db(master),
+ "mute": False,
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AbletonMCP_Remote_Script/handlers/tracks.py` around lines 350 - 363, The
master track entry built when include_master is true is missing the "mute" key
(unlike regular tracks and return tracks), causing KeyError when consumers
access entry["mute"]; update the block that constructs the master dict (using
variables master, vol and appending to results) to include "mute": master.mute
alongside the existing keys and ensure consistency with the track/return entries
and _get_volume_db usage.
| def get_track_routing(song, track_index, track_type="track", ctrl=None): | ||
| """Get output routing info for a track: current routing and available options.""" | ||
| try: | ||
| if track_type == "return": | ||
| if track_index < 0 or track_index >= len(song.return_tracks): | ||
| raise IndexError("Return track index out of range") | ||
| track = song.return_tracks[track_index] | ||
| elif track_type == "master": | ||
| track = song.master_track | ||
| else: | ||
| if track_index < 0 or track_index >= len(song.tracks): | ||
| raise IndexError("Track index out of range") | ||
| track = song.tracks[track_index] | ||
|
|
||
| current_type = None | ||
| current_channel = None | ||
| try: | ||
| rt = track.output_routing_type | ||
| current_type = rt.display_name if rt else None | ||
| except Exception: | ||
| pass | ||
| try: | ||
| rc = track.output_routing_channel | ||
| current_channel = rc.display_name if rc else None | ||
| except Exception: | ||
| pass | ||
|
|
||
| available_types = [] | ||
| try: | ||
| for rt in track.available_output_routing_types: | ||
| available_types.append(rt.display_name) | ||
| except Exception: | ||
| pass | ||
|
|
||
| available_channels = [] | ||
| try: | ||
| for rc in track.available_output_routing_channels: | ||
| available_channels.append(rc.display_name) | ||
| except Exception: | ||
| pass | ||
|
|
||
| return { | ||
| "index": track_index, | ||
| "name": track.name, | ||
| "track_type": track_type, | ||
| "output_routing_type": current_type, | ||
| "output_routing_channel": current_channel, | ||
| "available_output_routing_types": available_types, | ||
| "available_output_routing_channels": available_channels, | ||
| } | ||
| except Exception as e: | ||
| if ctrl: | ||
| ctrl.log_message("Error getting track routing: " + str(e)) | ||
| raise | ||
|
|
||
|
|
||
| def set_track_routing(song, track_index, output_type=None, output_channel=None, | ||
| track_type="track", ctrl=None): | ||
| """Set output routing for a track by display name. | ||
|
|
||
| Args: | ||
| track_index: Index of track. | ||
| output_type: Display name of output routing type (e.g. "SUB LIMITER"). | ||
| output_channel: Display name of output routing channel (e.g. "Track In"). | ||
| track_type: "track", "return", or "master". | ||
| """ | ||
| try: | ||
| if track_type == "return": | ||
| if track_index < 0 or track_index >= len(song.return_tracks): | ||
| raise IndexError("Return track index out of range") | ||
| track = song.return_tracks[track_index] | ||
| elif track_type == "master": | ||
| track = song.master_track | ||
| else: | ||
| if track_index < 0 or track_index >= len(song.tracks): | ||
| raise IndexError("Track index out of range") | ||
| track = song.tracks[track_index] | ||
|
|
||
| result = {"index": track_index, "name": track.name, "track_type": track_type} | ||
|
|
||
| if output_type is not None: | ||
| found = False | ||
| for rt in track.available_output_routing_types: | ||
| if rt.display_name == output_type: | ||
| track.output_routing_type = rt | ||
| result["output_routing_type"] = rt.display_name | ||
| found = True | ||
| break | ||
| if not found: | ||
| available = [rt.display_name for rt in track.available_output_routing_types] | ||
| raise ValueError( | ||
| "Output routing type '{0}' not found. Available: {1}".format( | ||
| output_type, ", ".join(available) | ||
| ) | ||
| ) | ||
|
|
||
| if output_channel is not None: | ||
| found = False | ||
| for rc in track.available_output_routing_channels: | ||
| if rc.display_name == output_channel: | ||
| track.output_routing_channel = rc | ||
| result["output_routing_channel"] = rc.display_name | ||
| found = True | ||
| break | ||
| if not found: | ||
| available = [rc.display_name for rc in track.available_output_routing_channels] | ||
| raise ValueError( | ||
| "Output routing channel '{0}' not found. Available: {1}".format( | ||
| output_channel, ", ".join(available) | ||
| ) | ||
| ) | ||
|
|
||
| return result | ||
| except Exception as e: | ||
| if ctrl: | ||
| ctrl.log_message("Error setting track routing: " + str(e)) | ||
| raise |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's verify the files exist and check the line counts
echo "=== File existence and structure ==="
wc -l AbletonMCP_Remote_Script/handlers/tracks.py AbletonMCP_Remote_Script/handlers/dispatch.py
echo ""
echo "=== Check dispatch.py lines 26-112 (_get_track_routing and _set_track_routing) ==="
sed -n '26,112p' AbletonMCP_Remote_Script/handlers/dispatch.py
echo ""
echo "=== Check tracks.py lines 595-711 (get_track_routing and set_track_routing) ==="
sed -n '595,711p' AbletonMCP_Remote_Script/handlers/tracks.pyRepository: ahujasid/ableton-mcp
Length of output: 7887
🏁 Script executed:
#!/bin/bash
echo "=== Usages of get_track_routing and set_track_routing (from tracks.py) ==="
rg "get_track_routing|set_track_routing" --type py -B 2 -A 2 | head -100
echo ""
echo "=== Usages of _get_track_routing and _set_track_routing (from dispatch.py) ==="
rg "_get_track_routing|_set_track_routing" --type py -B 2 -A 2 | head -100Repository: ahujasid/ableton-mcp
Length of output: 4040
🏁 Script executed:
#!/bin/bash
# Check which modules import from tracks and dispatch
echo "=== Files that import from tracks ==="
rg "from.*tracks import|import.*tracks" --type py
echo ""
echo "=== Files that import from dispatch ==="
rg "from.*dispatch import|import.*dispatch" --type pyRepository: ahujasid/ableton-mcp
Length of output: 437
🏁 Script executed:
#!/bin/bash
echo "=== _resolve_track helper in dispatch.py ==="
rg -n "_resolve_track" --type py -B 2 -A 10
echo ""
echo "=== Check if tracks.py functions are exported in __init__.py ==="
cat AbletonMCP_Remote_Script/handlers/__init__.pyRepository: ahujasid/ableton-mcp
Length of output: 4112
🏁 Script executed:
#!/bin/bash
# Check broader context of dispatch.py to see all helper functions
echo "=== First 200 lines of dispatch.py to see helper functions ==="
head -200 AbletonMCP_Remote_Script/handlers/dispatch.pyRepository: ahujasid/ableton-mcp
Length of output: 7152
🏁 Script executed:
#!/bin/bash
# Check if there are any external imports or usages of the tracks module functions
echo "=== Check for any imports of get_track_routing or set_track_routing ==="
rg "from.*tracks.*import.*routing|import.*tracks.*routing" --type py
echo ""
echo "=== Check MCP_Server/tools/__init__.py for usage ==="
head -50 MCP_Server/tools/__init__.py
echo ""
echo "=== Check if tracks.py is used as a public API elsewhere ==="
rg "tracks\.(get_track|set_track)" --type pyRepository: ahujasid/ableton-mcp
Length of output: 1843
🏁 Script executed:
#!/bin/bash
# Final check: Search for direct imports or calls to get_track_routing from tracks module
echo "=== Direct usage of tracks.get_track_routing or tracks.set_track_routing ==="
rg "tracks\.get_track_routing|tracks\.set_track_routing" --type py
echo ""
echo "=== Check if these functions are called in tests ==="
fd -e "test*.py" -o -e "*_test.py" | xargs rg "get_track_routing|set_track_routing" 2>/dev/null || echo "No test files found with these calls"Repository: ahujasid/ableton-mcp
Length of output: 1145
Remove or consolidate the unused get_track_routing and set_track_routing functions in tracks.py (lines 595–711).
These functions are nearly identical to _get_track_routing and _set_track_routing in dispatch.py (lines 26–112), with the core routing logic copied verbatim. However, the tracks.py versions have no callers anywhere in the codebase—only the dispatch.py versions are actively used by the command handler registry. Consider either removing the unused tracks.py functions entirely or consolidating both into a shared utility module with dispatch.py delegating to it, to eliminate the duplication and prevent future drift.
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 600-600: Abstract raise to an inner function
(TRY301)
[warning] 600-600: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 606-606: Abstract raise to an inner function
(TRY301)
[warning] 606-606: Avoid specifying long messages outside the exception class
(TRY003)
[error] 614-615: try-except-pass detected, consider logging the exception
(S110)
[warning] 614-614: Do not catch blind exception: Exception
(BLE001)
[error] 619-620: try-except-pass detected, consider logging the exception
(S110)
[warning] 619-619: Do not catch blind exception: Exception
(BLE001)
[error] 626-627: try-except-pass detected, consider logging the exception
(S110)
[warning] 626-626: Do not catch blind exception: Exception
(BLE001)
[error] 633-634: try-except-pass detected, consider logging the exception
(S110)
[warning] 633-633: Do not catch blind exception: Exception
(BLE001)
[warning] 636-644: Consider moving this statement to an else block
(TRY300)
[warning] 664-664: Abstract raise to an inner function
(TRY301)
[warning] 664-664: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 670-670: Abstract raise to an inner function
(TRY301)
[warning] 670-670: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 685-689: Abstract raise to an inner function
(TRY301)
[warning] 701-705: Abstract raise to an inner function
(TRY301)
[warning] 707-707: Consider moving this statement to an else block
(TRY300)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AbletonMCP_Remote_Script/handlers/tracks.py` around lines 595 - 711, The
get_track_routing and set_track_routing functions in tracks.py are duplicate,
unused copies of _get_track_routing and _set_track_routing in dispatch.py;
remove the duplicated functions from tracks.py or consolidate them into a single
shared utility and have callers delegate to it. Specifically, either delete
get_track_routing and set_track_routing from tracks.py (ensuring no external
callers reference them) or extract the routing logic into a new helper (e.g.,
track_routing_utils.get_track_routing / set_track_routing) and update
dispatch.py to call that helper instead of the local _get/_set implementations;
preserve behavior including handling of track_type ("track"/"return"/"master"),
index bounds checks, available_output_routing_types/channels iteration, result
dict keys, and ctrl logging on exceptions.
… summary Enhanced _build_arrangement: auto-names clips to track names during creation (eliminates 163 separate rename calls), auto-detects source loop length from arrangement clips, returns scene summary in response. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
AbletonMCP_Remote_Script/handlers/dispatch.py (1)
569-685: Registry is rebuilt up to 3× per command dispatch with no hot-reload benefit.
is_known(),is_modifying(), andexecute()each call_get_registry(), creating 17 lambdas and dicts per invocation. Since hot-reload re-imports the module anyway (control surface toggle), a module-level constant provides the same reload semantics without per-call overhead.Proposed simplification
-def _get_registry(): - """Build registry fresh each call so hot-reload picks up changes.""" - return { +_REGISTRY = { "set_return_track_name": { ... }, ... - } +} def is_known(command_type): - return command_type in _get_registry() + return command_type in _REGISTRY def is_modifying(command_type): - reg = _get_registry() - if command_type in reg: - return reg[command_type].get("modifying", False) + if command_type in _REGISTRY: + return _REGISTRY[command_type].get("modifying", False) return False def execute(command_type, params, song, ctrl): - reg = _get_registry() - if command_type not in reg: + if command_type not in _REGISTRY: raise ValueError("Unknown dynamic command: " + command_type) - return reg[command_type]["handler"](song, params, ctrl) + return _REGISTRY[command_type]["handler"](song, params, ctrl)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AbletonMCP_Remote_Script/handlers/dispatch.py` around lines 569 - 685, The registry is being rebuilt on every call via _get_registry(), causing redundant allocations; replace the dynamic builder by creating a module-level constant (e.g., REGISTRY) initialized once with the same mapping currently returned by _get_registry(), remove or keep _get_registry only if used for tests, and update callers (is_known, is_modifying, execute) to reference REGISTRY instead of calling _get_registry(); keep the same handler lambdas and keys so behavior and hot-reload (module re-import) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AbletonMCP_Remote_Script/handlers/dispatch.py`:
- Around line 340-362: The locators loop uses song.set_or_delete_cue() which
toggles cues so duplicate positions will delete instead of create; before
creating cues, deduplicate the locators list by position (using
float(loc["position"]) as the key) while preserving desired order or last-wins
semantics, then iterate the deduplicated list and call song.current_song_time /
song.set_or_delete_cue() once per unique position; after creation, assign names
by locating the cue with an exact time match (compare cp.time == position or
with a tiny epsilon) rather than nearest-cue logic that can pick an adjacent cue
— update references in this block (locators, created, song.current_song_time,
song.set_or_delete_cue, song.cue_points, best_cue) accordingly.
- Around line 534-540: The code may access section_tracks[0] when plan is empty,
causing IndexError; add an early-return guard like in _record_arrangement: check
if plan (or derived section_tracks) is empty immediately before the loop that
iterates over track_data (and before the block that references
section_tracks[0]) and return/exit that handler if empty so the subsequent
access to section_tracks[0] (and slot0.delete_clip()) cannot occur.
- Around line 233-307: The _record_arrangement handler must validate ctrl at the
top and fail fast if None (return an error like {"error": "ctrl required for
recording"}) because ctrl.schedule_message is used unguarded
(ctrl.schedule_message, ctrl.log_message); also replace the hardcoded
4-beats-per-bar with the song's time signature (use song.signature_numerator as
beats_per_bar) when computing scene_times and total_beats; and avoid permanently
mutating song.clip_trigger_quantization by saving its original value before
setting it and restoring it (and similarly ensure song.record_mode or other song
state is restored/cleaned up when stopping) so side effects do not persist after
_record_arrangement returns.
---
Duplicate comments:
In `@AbletonMCP_Remote_Script/handlers/dispatch.py`:
- Around line 12-23: The _resolve_track function duplicates
devices.resolve_track; remove the _resolve_track definition and update all call
sites in this module to call devices.resolve_track instead (e.g., replace usages
in the functions that currently call _resolve_track, including the logic
referenced by _move_device which already uses devices.resolve_track) so the
module consistently delegates track resolution to the devices.resolve_track
helper.
- Around line 26-112: Both _get_track_routing and _set_track_routing duplicate
logic already implemented in tracks.get_track_routing and
tracks.set_track_routing; replace the bodies so they extract the same params
from p (track_index, track_type, output_type, output_channel as needed) and
delegate to the corresponding tracks function, returning its result and allowing
exceptions from tracks to propagate. Update _get_track_routing to call
tracks.get_track_routing(song, track_index, track_type, ctrl) (or the signature
used by the tracks module) and return that value, and update _set_track_routing
to call tracks.set_track_routing(song, track_index, track_type, output_type,
output_channel, ctrl) (matching tracks signature), removing the duplicated
iteration and error construction code. Ensure you import the tracks module if
not already present and keep parameter extraction from p (and result return)
intact.
---
Nitpick comments:
In `@AbletonMCP_Remote_Script/handlers/dispatch.py`:
- Around line 569-685: The registry is being rebuilt on every call via
_get_registry(), causing redundant allocations; replace the dynamic builder by
creating a module-level constant (e.g., REGISTRY) initialized once with the same
mapping currently returned by _get_registry(), remove or keep _get_registry only
if used for tests, and update callers (is_known, is_modifying, execute) to
reference REGISTRY instead of calling _get_registry(); keep the same handler
lambdas and keys so behavior and hot-reload (module re-import) remain unchanged.
| def _record_arrangement(song, p, ctrl): | ||
| """Record session scenes to arrangement automatically. | ||
|
|
||
| Fires scenes at the right times based on the plan, recording | ||
| everything to arrangement view. | ||
|
|
||
| Params: | ||
| plan: list of sections (same format as build_arrangement) | ||
| stop_after: bool (default True) - stop recording after last section | ||
| """ | ||
| plan = p.get("plan", []) | ||
| stop_after = p.get("stop_after", True) | ||
|
|
||
| if not plan: | ||
| return {"error": "no plan provided"} | ||
|
|
||
| # Build scene fire times (in beats) | ||
| scene_times = [] | ||
| total_beats = 0 | ||
| for i, s in enumerate(plan): | ||
| beat = (s["bar"] - 1) * 4 | ||
| scene_times.append({"scene": i, "beat": beat, "name": s["name"]}) | ||
| end = beat + s["bars"] * 4 | ||
| if end > total_beats: | ||
| total_beats = end | ||
|
|
||
| next_scene = [1] # index into scene_times; scene 0 fires immediately | ||
|
|
||
| def poll_and_fire(): | ||
| idx = next_scene[0] | ||
|
|
||
| # Check if we're past the end | ||
| if song.current_song_time >= total_beats: | ||
| if stop_after: | ||
| song.record_mode = False | ||
| song.stop_playing() | ||
| if ctrl: | ||
| ctrl.log_message("record_arrangement: done at {0}".format( | ||
| song.current_song_time)) | ||
| return | ||
|
|
||
| # Fire next scene 2 beats early — global quantization (1 bar) | ||
| # will snap the actual launch to the correct bar line. | ||
| if idx < len(scene_times): | ||
| target = scene_times[idx]["beat"] | ||
| if song.current_song_time >= target - 2.0: | ||
| song.scenes[scene_times[idx]["scene"]].fire() | ||
| if ctrl: | ||
| ctrl.log_message("record_arrangement: fired {0} at {1}".format( | ||
| scene_times[idx]["name"], song.current_song_time)) | ||
| next_scene[0] += 1 | ||
|
|
||
| # Poll again (~100ms per tick) | ||
| ctrl.schedule_message(1, poll_and_fire) | ||
|
|
||
| # Setup: position at start, quantize to 1 bar, enable record, fire scene 0 | ||
| song.stop_playing() | ||
| song.current_song_time = 0.0 | ||
| song.clip_trigger_quantization = 4 # 4 = 1 Bar | ||
| song.record_mode = True | ||
| song.scenes[0].fire() | ||
| if ctrl: | ||
| ctrl.log_message("record_arrangement: started, {0} scenes".format( | ||
| len(scene_times))) | ||
|
|
||
| # Start polling for subsequent scenes | ||
| ctrl.schedule_message(1, poll_and_fire) | ||
|
|
||
| return { | ||
| "status": "recording_started", | ||
| "total_scenes": len(scene_times), | ||
| "total_beats": total_beats, | ||
| "total_bars": total_beats / 4, | ||
| "duration_seconds": total_beats * 60.0 / song.tempo, | ||
| } |
There was a problem hiding this comment.
ctrl is used unconditionally on lines 286 and 299 — AttributeError if None.
ctrl.schedule_message(...) is called without a None guard, but ctrl is an optional parameter across all handlers. Since _record_arrangement fundamentally cannot function without ctrl (polling depends on schedule_message), validate it upfront and fail fast with a clear error.
Additionally, line 253 hardcodes 4 beats/bar (4/4 time) and line 291 overwrites song.clip_trigger_quantization as a side effect that persists after return.
Proposed fix — validate ctrl upfront
def _record_arrangement(song, p, ctrl):
"""Record session scenes to arrangement automatically.
...
"""
+ if ctrl is None:
+ raise RuntimeError("record_arrangement requires ctrl for schedule_message()")
+
plan = p.get("plan", [])
stop_after = p.get("stop_after", True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AbletonMCP_Remote_Script/handlers/dispatch.py` around lines 233 - 307, The
_record_arrangement handler must validate ctrl at the top and fail fast if None
(return an error like {"error": "ctrl required for recording"}) because
ctrl.schedule_message is used unguarded (ctrl.schedule_message,
ctrl.log_message); also replace the hardcoded 4-beats-per-bar with the song's
time signature (use song.signature_numerator as beats_per_bar) when computing
scene_times and total_beats; and avoid permanently mutating
song.clip_trigger_quantization by saving its original value before setting it
and restoring it (and similarly ensure song.record_mode or other song state is
restored/cleaned up when stopping) so side effects do not persist after
_record_arrangement returns.
| locators = p.get("locators", []) | ||
| created = [] | ||
| for loc in locators: | ||
| pos = float(loc["position"]) | ||
| name = str(loc.get("name", "")) | ||
| song.current_song_time = pos | ||
| song.set_or_delete_cue() | ||
| # Find the cue we just created and name it | ||
| best_cue = None | ||
| best_dist = 999999.0 | ||
| for cp in song.cue_points: | ||
| dist = abs(cp.time - pos) | ||
| if dist < best_dist: | ||
| best_dist = dist | ||
| best_cue = cp | ||
| if best_cue and name: | ||
| best_cue.name = name | ||
| created.append({"name": name, "position": best_cue.time}) | ||
| elif best_cue: | ||
| created.append({"name": best_cue.name, "position": best_cue.time}) | ||
|
|
||
| song.current_song_time = original_time | ||
| return {"deleted": deleted, "created": created} |
There was a problem hiding this comment.
set_or_delete_cue() is a toggle — duplicate positions will delete instead of create.
If two entries in the locators list share the same position, the second call to set_or_delete_cue() at that position will delete the cue just created by the first call. The nearest-cue naming search (lines 348–354) is also fragile when locators are closely spaced, as it may rename a previously created cue rather than the intended one.
Consider deduplicating positions before creating, or at minimum documenting this limitation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AbletonMCP_Remote_Script/handlers/dispatch.py` around lines 340 - 362, The
locators loop uses song.set_or_delete_cue() which toggles cues so duplicate
positions will delete instead of create; before creating cues, deduplicate the
locators list by position (using float(loc["position"]) as the key) while
preserving desired order or last-wins semantics, then iterate the deduplicated
list and call song.current_song_time / song.set_or_delete_cue() once per unique
position; after creation, assign names by locating the cue with an exact time
match (compare cp.time == position or with a tiny epsilon) rather than
nearest-cue logic that can pick an adjacent cue — update references in this
block (locators, created, song.current_song_time, song.set_or_delete_cue,
song.cue_points, best_cue) accordingly.
|
|
||
| # If track NOT active in scene 0, remove clip from slot 0 | ||
| if tidx not in section_tracks[0]: | ||
| try: | ||
| slot0.delete_clip() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
IndexError when plan is empty — section_tracks[0] on an empty list.
If plan is [], then section_tracks is also []. The iteration over track_data (line 437) still executes, and line 536 accesses section_tracks[0], which raises IndexError.
Add an early return for an empty plan, similar to what _record_arrangement does.
Proposed fix
def _build_arrangement(song, p, ctrl):
...
plan = p.get("plan", [])
source_bars = p.get("source_bars", None)
+
+ if not plan:
+ return {"scenes_created": 0, "tracks_processed": 0,
+ "clips_placed": 0, "source_bars": 0,
+ "scenes": [], "skipped": [], "errors": []}🧰 Tools
🪛 Ruff (0.15.1)
[error] 539-540: try-except-pass detected, consider logging the exception
(S110)
[warning] 539-539: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AbletonMCP_Remote_Script/handlers/dispatch.py` around lines 534 - 540, The
code may access section_tracks[0] when plan is empty, causing IndexError; add an
early-return guard like in _record_arrangement: check if plan (or derived
section_tracks) is empty immediately before the loop that iterates over
track_data (and before the block that references section_tracks[0]) and
return/exit that handler if empty so the subsequent access to section_tracks[0]
(and slot0.delete_clip()) cannot occur.
- record_arrangement now accepts automation lanes with interpolated parameter changes during recording pass - Fixed create_automation_envelope (create before write) - Removed broken clip envelope approach, using live param writes instead Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Non-functional refactor to improve maintainability. Same 43 active tools, same socket protocol and behavior.
Remote Script (
AbletonMCP_Remote_Script/)session,tracks,clips,mixer,devices,browser,scenes,arrangement,audio,midi,automation.__init__.pyto ~750 lines: coreAbletonMCPclass, socket server,_process_commanddispatch table, and multi-tickcreate_locatorlogic only.MCP Server (
MCP_Server/)AbletonConnectiontoconnection.py.tools/:session,tracks,clips,mixer,devices,browser,scenes,arrangement,audio,midi.server.pyto ~100 lines: FastMCP setup, lifespan,get_ableton_connection(), andregister_all(mcp, get_ableton_connection).Benefits
Testing
Verified against Live 12:
get_session_info,get_all_tracks_info,get_track_info,get_return_tracks_info,create_scene,create_locator,set_track_color.Made with Cursor
Summary by CodeRabbit
New Features
Refactor
Chores
Documentation