feat: Add rack chain tools and improve type safety#50
Conversation
- Add create_rack_chain tool using insert_chain() API - Add load_effect_to_chain tool using move_device() API - Add get_device_parameters and set_device_parameter with rack_device_index support - Add duplicate_clip, remove_clip, move_clip tools - Add set_track_volume, set_master_volume tools - Fix Pylance type errors with proper type annotations - Add copilot instructions with cross-platform commands - Document Live API insights (insert_chain, move_device patterns)
WalkthroughAdds developer documentation and ignores; expands MCP_Server with many new tool endpoints and typing updates; and extends AbletonMCP Remote Script with socket lifecycle, thread safety, numerous main-thread handlers for clips/volumes/racks/devices, and safer browser utilities. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant MCPSrv as MCP Server
participant Socket as Socket Transport
participant Remote as AbletonMCP (Remote Script)
participant Live as Ableton Live API
Client->>MCPSrv: POST JSON tool command
MCPSrv->>Socket: send command over socket
Socket->>Remote: receive & parse command
alt requires main-thread work
Remote->>Remote: enqueue handler for main thread
Remote->>Live: main-thread handler invokes Live API (clips/volumes/racks/devices/browser)
else can run off main-thread
Remote->>Live: invoke Live API directly
end
Live-->>Remote: return result/status
Remote-->>Socket: send structured response
Socket-->>MCPSrv: forward response
MCPSrv-->>Client: return JSON success/error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||||
- Add comments explaining why time.sleep() is necessary after browser.load_item() - Simplify chain creation logic by removing unused chain_created variable - Note: sleeps are required due to Live API's async browser.load_item() behavior
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
.gitignore (1)
12-12: Confirm intention to ignore allscripts/contentIgnoring the entire
scripts/directory is fine for local tooling, but it will also hide any future shared helper scripts from version control. Just confirm this is intentional and not meant to hold checked‑in utilities..github/docs/running-local-server.md (1)
7-62: Local‑server workflow matches implementationThe
uv run --directoryconfiguration and Remote Script copy steps correctly reflect howMCP_Server/server.pyandAbletonMCP_Remote_Script/__init__.pyare wired. Optionally, you might mention creating theAbletonMCPfolder under “Remote Scripts” if it doesn’t exist yet, but the main flow is solid.AbletonMCP_Remote_Script/__init__.py (3)
739-769: Consider clamping volume to device parameter bounds
_set_track_volumeand_set_master_volumedirectly assignvolumeto.mixer_device.volume.valuewithout checking that it’s within the parameter’s[min, max]. Passing an out‑of‑range value from the MCP side could yield surprising behavior depending on Live’s clamping/validation.You could defensively clamp via
param = track.mixer_device.volume; param.value = max(param.min, min(param.max, volume))to keep behavior predictable.
811-865: Minor cleanup in_create_rack_chain
chain_createdis written but never read, and the comment about “Method 1” implies a now‑removed fallback. Since you always useinsert_chainand validate viachains_after, you can safely drop the flag and related comments to reduce noise.
914-959: Parameter resolution by name is good; consider more specific error type
_set_device_param’s name‑based lookup is robust (case‑insensitive), but on failure it raises a genericException("Parameter '...' not found"). For downstream handling and clearer client messages, consider a dedicated exception type or at leastValueErrorhere, consistent with other “bad argument” cases in this file.MCP_Server/server.py (4)
93-170:send_commanderror handling is solid; consider serialization guard and concurrencyThe assertion that
sockis notNone, tailored timeouts, and detailed logging (including raw bytes on JSON errors) are all strong improvements. Two possible follow‑ups:
- Add a simple serialization guard (e.g., a threading.Lock) around send/receive to prevent interleaved responses if multiple MCP requests hit this process concurrently.
- Optionally wrap the generic
Exceptionrethrows in a customAbletonConnectionErrorto distinguish connectivity problems from logical errors returned by Live.Both would tighten behavior if this is ever used in a multi‑client context.
103-112:is_modifying_commandlist contains some unused/legacy namesYou include
"set_device_parameter"and"set_chain_name"alongside the actual commands ("set_device_param","create_rack_chain", etc.). This is harmless but slightly confusing since there’s no corresponding Live‑side handler for"set_device_parameter"nor"set_chain_name"in the Remote Script.Consider trimming unused entries so the list is a 1:1 reflection of real command types.
399-473: New clip tools map cleanly to Remote Script commands; clarify MIDI-only semantics
duplicate_clip,empty_clip_slot(→remove_clip), andrelocate_clip(→move_clip) all send the expected command payloads and return human‑readable summaries, which is great.Given the Live‑side implementation only truly supports MIDI clips (and errors out for audio), it may be worth briefly noting that limitation in these docstrings or messages so users don’t assume audio clip support.
623-646:load_effect_to_chainwiring looks correct; could surface more detailThe tool correctly forwards
track_index,rack_device_index,chain_index, andeffect_uriand returns a succinct success/failure message. Since the Live‑side handler returns richer data (chain_name,devices_in_chain, etc.), you might consider incorporating some of that into the string (e.g., mentioning the chain name) to aid debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/copilot-instructions.md(1 hunks).github/docs/adding-new-tools.md(1 hunks).github/docs/debugging.md(1 hunks).github/docs/live-api-insights.md(1 hunks).github/docs/running-local-server.md(1 hunks).gitignore(1 hunks)AbletonMCP_Remote_Script/__init__.py(9 hunks)MCP_Server/server.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
AbletonMCP_Remote_Script/__init__.py (1)
MCP_Server/server.py (1)
create_clip(329-348)
🪛 Ruff (0.14.6)
AbletonMCP_Remote_Script/__init__.py
617-617: Abstract raise to an inner function
(TRY301)
617-617: Avoid specifying long messages outside the exception class
(TRY003)
622-622: Abstract raise to an inner function
(TRY301)
622-622: Avoid specifying long messages outside the exception class
(TRY003)
627-627: Abstract raise to an inner function
(TRY301)
627-627: Create your own exception
(TRY002)
627-627: Avoid specifying long messages outside the exception class
(TRY003)
633-633: Abstract raise to an inner function
(TRY301)
633-633: Avoid specifying long messages outside the exception class
(TRY003)
638-638: Abstract raise to an inner function
(TRY301)
638-638: Avoid specifying long messages outside the exception class
(TRY003)
643-643: Abstract raise to an inner function
(TRY301)
643-643: Create your own exception
(TRY002)
643-643: Avoid specifying long messages outside the exception class
(TRY003)
671-671: Consider moving this statement to an else block
(TRY300)
680-680: Abstract raise to an inner function
(TRY301)
680-680: Avoid specifying long messages outside the exception class
(TRY003)
685-685: Abstract raise to an inner function
(TRY301)
685-685: Avoid specifying long messages outside the exception class
(TRY003)
690-690: Abstract raise to an inner function
(TRY301)
690-690: Create your own exception
(TRY002)
690-690: Avoid specifying long messages outside the exception class
(TRY003)
700-700: Consider moving this statement to an else block
(TRY300)
721-721: Consider moving this statement to an else block
(TRY300)
743-743: Abstract raise to an inner function
(TRY301)
743-743: Avoid specifying long messages outside the exception class
(TRY003)
752-752: Consider moving this statement to an else block
(TRY300)
765-765: Consider moving this statement to an else block
(TRY300)
770-770: Unused method argument: device_index
(ARG002)
774-774: Abstract raise to an inner function
(TRY301)
774-774: Avoid specifying long messages outside the exception class
(TRY003)
793-793: Abstract raise to an inner function
(TRY301)
793-793: Create your own exception
(TRY002)
793-793: Avoid specifying long messages outside the exception class
(TRY003)
806-806: Consider moving this statement to an else block
(TRY300)
815-815: Abstract raise to an inner function
(TRY301)
815-815: Avoid specifying long messages outside the exception class
(TRY003)
820-820: Abstract raise to an inner function
(TRY301)
820-820: Avoid specifying long messages outside the exception class
(TRY003)
826-826: Abstract raise to an inner function
(TRY301)
826-826: Create your own exception
(TRY002)
826-826: Avoid specifying long messages outside the exception class
(TRY003)
838-838: Local variable chain_created is assigned to but never used
Remove assignment to unused variable chain_created
(F841)
846-846: Abstract raise to an inner function
(TRY301)
846-846: Create your own exception
(TRY002)
861-861: Consider moving this statement to an else block
(TRY300)
870-870: Abstract raise to an inner function
(TRY301)
870-870: Avoid specifying long messages outside the exception class
(TRY003)
879-879: Abstract raise to an inner function
(TRY301)
879-879: Create your own exception
(TRY002)
879-879: Avoid specifying long messages outside the exception class
(TRY003)
881-881: Abstract raise to an inner function
(TRY301)
881-881: Avoid specifying long messages outside the exception class
(TRY003)
884-884: Abstract raise to an inner function
(TRY301)
888-888: Abstract raise to an inner function
(TRY301)
888-888: Avoid specifying long messages outside the exception class
(TRY003)
909-909: Consider moving this statement to an else block
(TRY300)
918-918: Abstract raise to an inner function
(TRY301)
918-918: Avoid specifying long messages outside the exception class
(TRY003)
926-926: Abstract raise to an inner function
(TRY301)
926-926: Create your own exception
(TRY002)
926-926: Avoid specifying long messages outside the exception class
(TRY003)
928-928: Abstract raise to an inner function
(TRY301)
928-928: Avoid specifying long messages outside the exception class
(TRY003)
931-931: Abstract raise to an inner function
(TRY301)
931-931: Avoid specifying long messages outside the exception class
(TRY003)
935-935: Abstract raise to an inner function
(TRY301)
935-935: Avoid specifying long messages outside the exception class
(TRY003)
946-946: Abstract raise to an inner function
(TRY301)
946-946: Create your own exception
(TRY002)
955-955: Consider moving this statement to an else block
(TRY300)
964-964: Abstract raise to an inner function
(TRY301)
964-964: Avoid specifying long messages outside the exception class
(TRY003)
969-969: Abstract raise to an inner function
(TRY301)
969-969: Avoid specifying long messages outside the exception class
(TRY003)
975-975: Abstract raise to an inner function
(TRY301)
975-975: Create your own exception
(TRY002)
975-975: Avoid specifying long messages outside the exception class
(TRY003)
978-978: Abstract raise to an inner function
(TRY301)
991-991: Abstract raise to an inner function
(TRY301)
991-991: Create your own exception
(TRY002)
1042-1042: Consider moving this statement to an else block
(TRY300)
MCP_Server/server.py
401-401: Unused function argument: ctx
(ARG001)
418-418: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
424-424: Consider moving this statement to an else block
(TRY300)
425-425: Do not catch blind exception: Exception
(BLE001)
426-426: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
426-426: Use explicit conversion flag
Replace with conversion flag
(RUF010)
427-427: Use explicit conversion flag
Replace with conversion flag
(RUF010)
430-430: Unused function argument: ctx
(ARG001)
440-440: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
444-444: Consider moving this statement to an else block
(TRY300)
445-445: Do not catch blind exception: Exception
(BLE001)
446-446: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
446-446: Use explicit conversion flag
Replace with conversion flag
(RUF010)
447-447: Use explicit conversion flag
Replace with conversion flag
(RUF010)
450-450: Unused function argument: ctx
(ARG001)
463-463: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
469-469: Consider moving this statement to an else block
(TRY300)
470-470: Do not catch blind exception: Exception
(BLE001)
471-471: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
471-471: Use explicit conversion flag
Replace with conversion flag
(RUF010)
472-472: Use explicit conversion flag
Replace with conversion flag
(RUF010)
491-491: Unused function argument: ctx
(ARG001)
501-501: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
502-502: Consider moving this statement to an else block
(TRY300)
503-503: Do not catch blind exception: Exception
(BLE001)
504-504: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
504-504: Use explicit conversion flag
Replace with conversion flag
(RUF010)
505-505: Use explicit conversion flag
Replace with conversion flag
(RUF010)
508-508: Unused function argument: ctx
(ARG001)
517-517: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
518-518: Consider moving this statement to an else block
(TRY300)
519-519: Do not catch blind exception: Exception
(BLE001)
520-520: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
520-520: Use explicit conversion flag
Replace with conversion flag
(RUF010)
521-521: Use explicit conversion flag
Replace with conversion flag
(RUF010)
525-525: Unused function argument: ctx
(ARG001)
535-535: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
539-539: Consider moving this statement to an else block
(TRY300)
540-540: Do not catch blind exception: Exception
(BLE001)
541-541: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
541-541: Use explicit conversion flag
Replace with conversion flag
(RUF010)
542-542: Use explicit conversion flag
Replace with conversion flag
(RUF010)
546-546: Unused function argument: ctx
(ARG001)
563-563: Consider moving this statement to an else block
(TRY300)
564-564: Do not catch blind exception: Exception
(BLE001)
565-565: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
565-565: Use explicit conversion flag
Replace with conversion flag
(RUF010)
566-566: Use explicit conversion flag
Replace with conversion flag
(RUF010)
570-570: Unused function argument: ctx
(ARG001)
589-589: Do not catch blind exception: Exception
(BLE001)
590-590: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
590-590: Use explicit conversion flag
Replace with conversion flag
(RUF010)
591-591: Use explicit conversion flag
Replace with conversion flag
(RUF010)
595-595: Unused function argument: ctx
(ARG001)
609-609: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
617-617: Consider moving this statement to an else block
(TRY300)
618-618: Do not catch blind exception: Exception
(BLE001)
619-619: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
619-619: Use explicit conversion flag
Replace with conversion flag
(RUF010)
620-620: Use explicit conversion flag
Replace with conversion flag
(RUF010)
624-624: Unused function argument: ctx
(ARG001)
636-636: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
642-642: Consider moving this statement to an else block
(TRY300)
643-643: Do not catch blind exception: Exception
(BLE001)
644-644: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
644-644: Use explicit conversion flag
Replace with conversion flag
(RUF010)
645-645: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (11)
.github/docs/debugging.md (1)
7-37: Debugging guide is clear and actionableThe log‑tail commands and
self.log_messageexample align well with how the Remote Script behaves, and the structured success/error responses are consistent with the MCP server expectations..github/copilot-instructions.md (1)
5-17: Good high‑level entrypoint for contributorsQuick reference to
MCP_Server/server.pyandAbletonMCP_Remote_Script/__init__.pyplus the topic‑specific docs is a solid map for anyone extending tools or debugging..github/docs/adding-new-tools.md (1)
7-55: Tool‑extension guide matches current code patternsThe documented flow (MCP tool →
is_modifying_command→ Remote Script routing + handler) mirrors howduplicate_clip, rack tools, and device parameter tools are implemented, so this should stay accurate as a reference..github/docs/live-api-insights.md (1)
5-69: Live API notes correctly match the implementationThe guidance on
insert_chain,song.move_device, and addressing devices viatrack_index+rack_device_index+chain_index+device_indexmatches how the new rack/parameter helpers are written in the Remote Script.AbletonMCP_Remote_Script/__init__.py (2)
235-372: Main-thread scheduling for mutating commands looks solidRouting all state‑changing commands through
schedule_messagewith aQueueand a bounded timeout is consistent with Live’s threading expectations and should avoid unsafe background thread mutations.
1531-1536: Extra metadata inget_browser_items_at_pathresponse is usefulAdding
name,uri, andis_folder/is_device/is_loadablefor the current node makes the path API more self‑describing and lines up with how browser items are modeled elsewhere in this file.MCP_Server/server.py (5)
15-35: MakingsockOptional and lazy-initialized is a good robustness improvementUsing
Optional[socket.socket]with afield(default=None)plus theconnect()guard makes reconnect logic clearer and safer compared to assuming a live socket.
46-92:receive_full_responseis a reasonable JSON framing strategyChunking until
json.loadssucceeds, then returning the full buffer, matches how the Remote Script sends single JSON objects per command. The fallback that still returns partial data (or errors clearly) on timeout is also helpful for debugging malformed responses.
202-262: Connection retry and validation flow is well thought outThe 3‑attempt retry loop with a short backoff and a validation
get_session_infocall gives clear logs and avoids leaving a half‑open_ableton_connectionaround. This should help a lot when the Remote Script isn’t running yet.
490-522: Volume tools are consistent with Live-side handlers
set_track_volumeandset_master_volumecorrectly forwardvolumeto the corresponding Remote Script commands and document the normalized 0.0–1.0 range (with 0.85≈0dB). This lines up with the mixer parameter behavior.
569-621: Device parameter tools align with Live-side addressing model
get_device_parametersandset_device_parametercorrectly pass throughtrack_index,device_index,chain_index, andrack_device_indexso the Remote Script can resolve devices in chains vs. on the track. The JSON‑formatted response forget_device_parametersis especially useful for downstream tooling.
- Add MIDI-only guard for duplicate_clip with clear error message - Add negative index validation for device access in chains and tracks - Add success validation for load_effect_to_chain (raises error if effect didn't load) - Update docstrings to clarify device_index is reserved for future use
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
MCP_Server/server.py (1)
524-542: Docstring now matches Live‑side behavior fordevice_indexExplicitly documenting
device_indexas “reserved for future use” and noting that the rack is always appended resolves the earlier API/behavior mismatch noted in past reviews. Once the Remote Script grows insertion support, you can update the docstring without any breaking change.AbletonMCP_Remote_Script/__init__.py (4)
612-728: MIDI‑only duplicate/move behavior is now explicit; consider dest‑track guardThe updated
_duplicate_clipnow raises a clear error whensource_clip.is_midi_clipis false, which addresses the earlier concern about silent failures on audio clips and makes_move_clipinherit that behavior. One remaining edge case: duplicating a MIDI clip into an audio track will still calldest_clip_slot.create_clip, which Live is likely to reject with a lower‑level error; you might optionally pre‑checkdest_track.has_midi_inputand raise a more descriptive message in that case.
774-819:_create_audio_effect_rackdocumentation now matches behaviorThe handler still ignores
device_indexfunctionally, but the docstring explicitly marks it as “reserved for future use” and states that the rack is appended to the end of the device chain, which aligns the Live‑side behavior with the MCP API docs. The browser lookup, selection, and short sleep to allow the rack to appear mirror patterns used elsewhere.
873-920: Device-parameter reads guard negative indices and support rack chains
_get_device_parametersnow explicitly rejectsdevice_index < 0both for chain devices and track‑level devices, fixing the negative‑indexing concern from the earlier review. Using the(chain_index >= 0 and rack_device_index >= 0)gate for the rack path plus detailed error strings (with requested vs. available device counts) should make mis‑indexed calls much easier to debug.
967-1055:_load_effect_to_chainvalidates success well; tighten index checks and revisit sleepsThe new implementation fixes the earlier “false success” issue by raising if
devices_after <= devices_before, and the diagnostic logging of track devices on failure is very helpful. Two follow‑ups to consider:
- Align index validation with other helpers by rejecting negative
rack_device_indexandchain_indexinstead of relying on Python’s negative indexing (currently onlyrack_device_index >= len(track.devices)andchain_index >= len(rack.chains)are checked).- The
time.sleep(0.3)andtime.sleep(0.2)still run on the Live main thread, which can briefly freeze the UI; if this becomes noticeable, a non‑blocking pattern using additionalschedule_messagecallbacks to poll for the loaded/moved device would be preferable.- if rack_device_index >= len(track.devices): - raise IndexError("Rack device index out of range") + if rack_device_index < 0 or rack_device_index >= len(track.devices): + raise IndexError("Rack device index out of range") @@ - if chain_index >= len(rack.chains): - raise IndexError("Chain index out of range (have {}, requested {})".format(len(rack.chains), chain_index)) + if chain_index < 0 or chain_index >= len(rack.chains): + raise IndexError( + "Chain index out of range (have {}, requested {})".format(len(rack.chains), chain_index) + )
🧹 Nitpick comments (4)
MCP_Server/server.py (4)
93-169:send_commandrobustness is good; consider trimming the “modifying” setThe stronger typing,
assert self.sock is not None, raw-response logging on JSON decode errors, and unified timeout handling all look solid and should make failures easier to debug. One minor thing:get_device_parametersis included inis_modifying_commandeven though it’s read‑only, which unnecessarily adds sleeps and a longer timeout for parameter reads; consider removing it from that list for slightly snappier tooling.
399-472: Clip tools map cleanly to Remote Script commands
duplicate_clip,empty_clip_slot(→remove_clip), andrelocate_clip(→move_clip) all forward the expected indices and have clear docstrings that match the Remote Script handlers. Only nit is that each assignsresult = ableton.send_command(...)without usingresult; you can drop the local variable to avoid Ruff’s F841 and keep things consistent with simple “fire-and-report” tools.- result = ableton.send_command("duplicate_clip", { + ableton.send_command("duplicate_clip", { @@ - result = ableton.send_command("remove_clip", { + ableton.send_command("remove_clip", { @@ - result = ableton.send_command("move_clip", { + ableton.send_command("move_clip", {
490-522: Volume tools are straightforward; consider optional value guardingBoth
set_track_volumeandset_master_volumecorrectly forward parameters and document the 0.0–1.0 / 0.85=0dB convention. If you want slightly safer behavior, you could clampvolumeinto[0.0, 1.0]on the MCP side so obviously invalid inputs don’t get through to Live, but this is not strictly required.
594-621:set_device_parameter→set_device_parammapping is easy to useRenaming the MCP tool to
set_device_parameterwhile still sending theset_device_paramcommand to Live is a nice public‑API improvement. The optionalchain_index/rack_device_indexarguments mirrorget_device_parameters, so usage is consistent. Similar to clip tools,resultis currently unused and can be dropped if you want to quiet Ruff.- result = ableton.send_command("set_device_param", { + ableton.send_command("set_device_param", { @@ - return f"Set '{parameter_name}' to {value} on device {device_index}" + return f"Set '{parameter_name}' to {value} on device {device_index}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
AbletonMCP_Remote_Script/__init__.py(9 hunks)MCP_Server/server.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
AbletonMCP_Remote_Script/__init__.py (1)
MCP_Server/server.py (1)
create_clip(329-348)
🪛 Ruff (0.14.6)
AbletonMCP_Remote_Script/__init__.py
617-617: Abstract raise to an inner function
(TRY301)
617-617: Avoid specifying long messages outside the exception class
(TRY003)
622-622: Abstract raise to an inner function
(TRY301)
622-622: Avoid specifying long messages outside the exception class
(TRY003)
627-627: Abstract raise to an inner function
(TRY301)
627-627: Create your own exception
(TRY002)
627-627: Avoid specifying long messages outside the exception class
(TRY003)
633-633: Abstract raise to an inner function
(TRY301)
633-633: Create your own exception
(TRY002)
633-633: Avoid specifying long messages outside the exception class
(TRY003)
637-637: Abstract raise to an inner function
(TRY301)
637-637: Avoid specifying long messages outside the exception class
(TRY003)
642-642: Abstract raise to an inner function
(TRY301)
642-642: Avoid specifying long messages outside the exception class
(TRY003)
647-647: Abstract raise to an inner function
(TRY301)
647-647: Create your own exception
(TRY002)
647-647: Avoid specifying long messages outside the exception class
(TRY003)
675-675: Consider moving this statement to an else block
(TRY300)
684-684: Abstract raise to an inner function
(TRY301)
684-684: Avoid specifying long messages outside the exception class
(TRY003)
689-689: Abstract raise to an inner function
(TRY301)
689-689: Avoid specifying long messages outside the exception class
(TRY003)
694-694: Abstract raise to an inner function
(TRY301)
694-694: Create your own exception
(TRY002)
694-694: Avoid specifying long messages outside the exception class
(TRY003)
704-704: Consider moving this statement to an else block
(TRY300)
725-725: Consider moving this statement to an else block
(TRY300)
747-747: Abstract raise to an inner function
(TRY301)
747-747: Avoid specifying long messages outside the exception class
(TRY003)
756-756: Consider moving this statement to an else block
(TRY300)
769-769: Consider moving this statement to an else block
(TRY300)
774-774: Unused method argument: device_index
(ARG002)
782-782: Abstract raise to an inner function
(TRY301)
782-782: Avoid specifying long messages outside the exception class
(TRY003)
801-801: Abstract raise to an inner function
(TRY301)
801-801: Create your own exception
(TRY002)
801-801: Avoid specifying long messages outside the exception class
(TRY003)
815-815: Consider moving this statement to an else block
(TRY300)
824-824: Abstract raise to an inner function
(TRY301)
824-824: Avoid specifying long messages outside the exception class
(TRY003)
829-829: Abstract raise to an inner function
(TRY301)
829-829: Avoid specifying long messages outside the exception class
(TRY003)
835-835: Abstract raise to an inner function
(TRY301)
835-835: Create your own exception
(TRY002)
835-835: Avoid specifying long messages outside the exception class
(TRY003)
843-843: Abstract raise to an inner function
(TRY301)
843-843: Create your own exception
(TRY002)
843-843: Avoid specifying long messages outside the exception class
(TRY003)
853-853: Abstract raise to an inner function
(TRY301)
853-853: Create your own exception
(TRY002)
868-868: Consider moving this statement to an else block
(TRY300)
877-877: Abstract raise to an inner function
(TRY301)
877-877: Avoid specifying long messages outside the exception class
(TRY003)
886-886: Abstract raise to an inner function
(TRY301)
886-886: Create your own exception
(TRY002)
886-886: Avoid specifying long messages outside the exception class
(TRY003)
888-888: Abstract raise to an inner function
(TRY301)
888-888: Avoid specifying long messages outside the exception class
(TRY003)
891-891: Abstract raise to an inner function
(TRY301)
895-895: Abstract raise to an inner function
(TRY301)
895-895: Avoid specifying long messages outside the exception class
(TRY003)
916-916: Consider moving this statement to an else block
(TRY300)
925-925: Abstract raise to an inner function
(TRY301)
925-925: Avoid specifying long messages outside the exception class
(TRY003)
933-933: Abstract raise to an inner function
(TRY301)
933-933: Create your own exception
(TRY002)
933-933: Avoid specifying long messages outside the exception class
(TRY003)
935-935: Abstract raise to an inner function
(TRY301)
935-935: Avoid specifying long messages outside the exception class
(TRY003)
938-938: Abstract raise to an inner function
(TRY301)
938-938: Avoid specifying long messages outside the exception class
(TRY003)
942-942: Abstract raise to an inner function
(TRY301)
942-942: Avoid specifying long messages outside the exception class
(TRY003)
953-953: Abstract raise to an inner function
(TRY301)
953-953: Create your own exception
(TRY002)
962-962: Consider moving this statement to an else block
(TRY300)
971-971: Abstract raise to an inner function
(TRY301)
971-971: Avoid specifying long messages outside the exception class
(TRY003)
976-976: Abstract raise to an inner function
(TRY301)
976-976: Avoid specifying long messages outside the exception class
(TRY003)
982-982: Abstract raise to an inner function
(TRY301)
982-982: Create your own exception
(TRY002)
982-982: Avoid specifying long messages outside the exception class
(TRY003)
985-985: Abstract raise to an inner function
(TRY301)
998-998: Abstract raise to an inner function
(TRY301)
998-998: Create your own exception
(TRY002)
1042-1042: Abstract raise to an inner function
(TRY301)
1042-1042: Create your own exception
(TRY002)
1042-1042: Avoid specifying long messages outside the exception class
(TRY003)
1052-1052: Consider moving this statement to an else block
(TRY300)
MCP_Server/server.py
401-401: Unused function argument: ctx
(ARG001)
418-418: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
424-424: Consider moving this statement to an else block
(TRY300)
425-425: Do not catch blind exception: Exception
(BLE001)
426-426: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
426-426: Use explicit conversion flag
Replace with conversion flag
(RUF010)
427-427: Use explicit conversion flag
Replace with conversion flag
(RUF010)
430-430: Unused function argument: ctx
(ARG001)
440-440: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
444-444: Consider moving this statement to an else block
(TRY300)
445-445: Do not catch blind exception: Exception
(BLE001)
446-446: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
446-446: Use explicit conversion flag
Replace with conversion flag
(RUF010)
447-447: Use explicit conversion flag
Replace with conversion flag
(RUF010)
450-450: Unused function argument: ctx
(ARG001)
463-463: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
469-469: Consider moving this statement to an else block
(TRY300)
470-470: Do not catch blind exception: Exception
(BLE001)
471-471: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
471-471: Use explicit conversion flag
Replace with conversion flag
(RUF010)
472-472: Use explicit conversion flag
Replace with conversion flag
(RUF010)
491-491: Unused function argument: ctx
(ARG001)
501-501: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
502-502: Consider moving this statement to an else block
(TRY300)
503-503: Do not catch blind exception: Exception
(BLE001)
504-504: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
504-504: Use explicit conversion flag
Replace with conversion flag
(RUF010)
505-505: Use explicit conversion flag
Replace with conversion flag
(RUF010)
508-508: Unused function argument: ctx
(ARG001)
517-517: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
518-518: Consider moving this statement to an else block
(TRY300)
519-519: Do not catch blind exception: Exception
(BLE001)
520-520: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
520-520: Use explicit conversion flag
Replace with conversion flag
(RUF010)
521-521: Use explicit conversion flag
Replace with conversion flag
(RUF010)
525-525: Unused function argument: ctx
(ARG001)
535-535: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
539-539: Consider moving this statement to an else block
(TRY300)
540-540: Do not catch blind exception: Exception
(BLE001)
541-541: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
541-541: Use explicit conversion flag
Replace with conversion flag
(RUF010)
542-542: Use explicit conversion flag
Replace with conversion flag
(RUF010)
546-546: Unused function argument: ctx
(ARG001)
563-563: Consider moving this statement to an else block
(TRY300)
564-564: Do not catch blind exception: Exception
(BLE001)
565-565: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
565-565: Use explicit conversion flag
Replace with conversion flag
(RUF010)
566-566: Use explicit conversion flag
Replace with conversion flag
(RUF010)
570-570: Unused function argument: ctx
(ARG001)
589-589: Do not catch blind exception: Exception
(BLE001)
590-590: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
590-590: Use explicit conversion flag
Replace with conversion flag
(RUF010)
591-591: Use explicit conversion flag
Replace with conversion flag
(RUF010)
595-595: Unused function argument: ctx
(ARG001)
609-609: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
617-617: Consider moving this statement to an else block
(TRY300)
618-618: Do not catch blind exception: Exception
(BLE001)
619-619: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
619-619: Use explicit conversion flag
Replace with conversion flag
(RUF010)
620-620: Use explicit conversion flag
Replace with conversion flag
(RUF010)
624-624: Unused function argument: ctx
(ARG001)
636-636: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
642-642: Consider moving this statement to an else block
(TRY300)
643-643: Do not catch blind exception: Exception
(BLE001)
644-644: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
644-644: Use explicit conversion flag
Replace with conversion flag
(RUF010)
645-645: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (10)
MCP_Server/server.py (3)
2-20: Type-safety and Optional socket usage look consistentImporting
Contextwithtype: ignore[import-not-found], usingfield(default=None)andOptional[socket.socket]forsockis coherent with the rest of the file and aligns with howget_ableton_connectionalready toleratessockbeingNone. No issues here.
545-592: Rack‑chain creation and parameter reads are wired correctly
create_rack_chainforwardstrack_index,device_index, andchain_nameas expected and returns thechain_indexfrom the Live side, which matches the Remote Script contract.get_device_parametersexposes thechain_index/rack_device_indexpair and just JSON‑dumps the result, which is appropriate for a tooling endpoint. No functional issues here.
623-645:load_effect_to_chainserver tool aligns with Remote Script handlerThe tool’s signature and payload keys (
track_index,rack_device_index,chain_index,effect_uri) match the Remote Script’s_load_effect_to_chainexpectations, and the user‑facing message is clear. No behavioral problems from the MCP side.AbletonMCP_Remote_Script/__init__.py (7)
26-50: ControlSurface typing and server fields are well‑structuredAnnotating
AbletonMCPwithControlSurface # type: ignore[misc], adding typedserver,client_threads,server_thread, andrunning, and starting the server in__init__are all coherent with the existing lifecycle. This solidifies type checking without changing runtime behavior.
94-138: Extra guard forself.server is Nonein_server_threadis a good safety netThe early return when
self.serverisNoneprevents obscure attribute errors if startup failed but the thread somehow runs. This matches the logging strategy used elsewhere and is harmless in normal flows.
235-372: Command routing cleanly extends to new Live operationsThe expanded
command_typeswitch inside_process_commandcorrectly enqueues state‑modifying operations (clips, volume, racks, parameters, load_effect_to_chain) onto the main thread viaschedule_message, with a response queue to propagate results or errors. This keeps Live mutations on the main thread and preserves the existing error/reporting pattern. No routing mismatches spotted.
743-773: Track/master volume helpers are straightforward
_set_track_volumeand_set_master_volumeboth perform simple bounds checks (for track index) and then write directly to the Live mixer parameters, returning the actual values applied. This is a clean, minimal implementation; no issues.
820-872: Rack‑chain creation viainsert_chainis correct and defensive
_create_rack_chainvalidates the track and device indices, checkscan_have_chains, ensuresinsert_chainexists, and verifies thatchains_after > chains_beforebefore returning the new chain index/name. That gives clear errors in the common failure modes and uses the right Live API call.
921-966:_set_device_parammirrors the same index rules and name-matchingThe setter mirrors
_get_device_parameters’s branching between rack chains and track devices, includes lower/upper‑bound checks ondevice_index, and does a case‑insensitive search overdevice.parametersbefore settingtarget_param.value. Exception messages are explicit enough for tooling consumers. Implementation looks good.
1541-1546: Extra metadata onget_browser_items_at_pathresults is usefulAdding
name,uri, and theis_folder/is_device/is_loadableflags for the resolvedcurrent_itemmakes the API much easier to consume, especially when tooling wants to annotate the container as well as its children. This is a nice, backward‑compatible enhancement.
The device_index parameter for create_audio_effect_rack was already implemented in the Remote Script but the docstrings incorrectly stated it was reserved for future use.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
AbletonMCP_Remote_Script/__init__.py (1)
235-246: Includeload_instrument_or_effectin the main‑thread state‑modifying command setThe comment explicitly says commands that modify Live’s state should be scheduled on the main thread, and you do that for most write operations. However,
load_instrument_or_effectis handled below in the same_process_commandmethod but is not part of thecommand_type in [...]block, so it runs on the server thread and calls Live APIs off the main thread.To avoid threading issues with Live, it should be scheduled just like
load_browser_itemand the other mutating commands.- # Commands that modify Live's state should be scheduled on the main thread - elif command_type in ["create_midi_track", "set_track_name", - "create_clip", "add_notes_to_clip", "set_clip_name", "duplicate_clip", "remove_clip", "move_clip", - "set_tempo", "fire_clip", "stop_clip", - "start_playback", "stop_playback", "load_browser_item", + # Commands that modify Live's state should be scheduled on the main thread + elif command_type in ["create_midi_track", "set_track_name", + "create_clip", "add_notes_to_clip", "set_clip_name", "duplicate_clip", "remove_clip", "move_clip", + "set_tempo", "fire_clip", "stop_clip", + "start_playback", "stop_playback", "load_instrument_or_effect", "load_browser_item", "set_track_volume", "set_master_volume", "create_audio_effect_rack", "create_rack_chain", "get_device_parameters", "set_device_param", "load_effect_to_chain"]:Also applies to: 257-346
🧹 Nitpick comments (7)
AbletonMCP_Remote_Script/__init__.py (2)
680-728:_move_clipsemantics are fine but not atomic on failureUsing
_duplicate_clipthen_remove_clipis a reasonable implementation of “move by copy + delete”. Be aware that if deletion fails after a successful duplicate (e.g., transient Live error), you’ll end up with both source and destination populated.If you care about stronger guarantees later, you could return the result from
_duplicate_clipand use it to attempt a rollback on failure, but for now the behavior is acceptable and clearly logged.
743-772: Volume setters rely on Live for range validation
_set_track_volumeand_set_master_volumeset.mixer_device.volume.valuedirectly without guardingvolume(docstring suggests 0.0–1.0). That’s acceptable if you trust callers and Live’s own clamping, but if you want stricter guarantees you could enforce the range and raise a clearer error.- def _set_track_volume(self, track_index, volume): + def _set_track_volume(self, track_index, volume): @@ - track = self._song.tracks[track_index] - track.mixer_device.volume.value = volume + track = self._song.tracks[track_index] + if not 0.0 <= volume <= 1.0: + raise ValueError("Volume must be between 0.0 and 1.0") + track.mixer_device.volume.value = volume @@ - def _set_master_volume(self, volume): + def _set_master_volume(self, volume): @@ - self._song.master_track.mixer_device.volume.value = volume + if not 0.0 <= volume <= 1.0: + raise ValueError("Volume must be between 0.0 and 1.0") + self._song.master_track.mixer_device.volume.value = volumeMCP_Server/server.py (5)
93-112:send_command: nice robustness; consider trimming unused command names inis_modifying_commandThe new signature with
params: Optional[Dict[str, Any]], assertion thatsockis notNone, and logging of rawresponse_dataon JSON errors are all solid improvements.The
is_modifying_commandlist does include some legacy strings ("set_device_parameter","set_chain_name") that are no longer sent anywhere; they only affect delay/timeout selection. Not urgent, but you could prune them to keep the list aligned with actual command types.- is_modifying_command = command_type in [ - "create_midi_track", "create_audio_track", "set_track_name", - "create_clip", "add_notes_to_clip", "set_clip_name", "duplicate_clip", "remove_clip", "move_clip", - "set_tempo", "fire_clip", "stop_clip", "set_device_parameter", - "start_playback", "stop_playback", "load_instrument_or_effect", - "set_track_volume", "set_master_volume", - "create_audio_effect_rack", "create_rack_chain", "set_chain_name", - "load_effect_to_chain", "get_device_parameters", "set_device_param" - ] + is_modifying_command = command_type in [ + "create_midi_track", "create_audio_track", "set_track_name", + "create_clip", "add_notes_to_clip", "set_clip_name", "duplicate_clip", "remove_clip", "move_clip", + "set_tempo", "fire_clip", "stop_clip", + "start_playback", "stop_playback", "load_instrument_or_effect", + "set_track_volume", "set_master_volume", + "create_audio_effect_rack", "create_rack_chain", + "load_effect_to_chain", "get_device_parameters", "set_device_param", + ]Also applies to: 114-169
490-522: Track/master volume tools are straightforward; optional client‑side range checksThe MCP
set_track_volumeandset_master_volumetools call the corresponding commands and return simple status strings. Behavior matches the Remote Script.If you want extra safety on the client side, you could mirror the 0.0–1.0 range enforcement here and fail fast before sending clearly invalid values, but that’s optional since Live will clamp/complain anyway.
524-543: Rack tools align with Live handlers; consider exposing returned indicesThese tools correctly forward
track_index,device_index, andchain_nameto the Remote Script and format nice summaries. Now that the Live side returnschain_indexand (with the earlier suggested fix) the actual rackdevice_index, it might be useful to include those in the human‑readable strings so callers don’t have to inspect the raw JSON.- result = ableton.send_command("create_audio_effect_rack", { + result = ableton.send_command("create_audio_effect_rack", { "track_index": track_index, "device_index": device_index }) - return f"Created Audio Effect Rack on track {track_index}" + created_index = result.get("device_index", device_index) + return f"Created Audio Effect Rack on track {track_index} at device index {created_index}" @@ - chain_idx = result.get("chain_index", "?") - return f"Created chain '{chain_name}' (index {chain_idx}) in rack at track {track_index}, device {device_index}" + chain_idx = result.get("chain_index", "?") + chain_label = chain_name or result.get("chain_name", f"Chain {chain_idx}") + return f"Created chain '{chain_label}' (index {chain_idx}) in rack at track {track_index}, device {device_index}"Also applies to: 545-567
569-592: Device parameter tools match Remote Script API; index semantics should mirror stricter checksThe MCP
get_device_parameters/set_device_parametertools match the Live‑side_get_device_parameters/_set_device_paramsignatures, includingchain_indexandrack_device_index, and just forward/format results. That’s good.Once you tighten the chain/rack index validation on the Remote Script side (as suggested earlier), these tools will naturally inherit the clearer error messages, so nothing to change here beyond possibly echoing the “rack index required when chain_index >= 0” rule into the docstrings later if you want the contract to be explicit at this level too.
Also applies to: 594-621
623-645:load_effect_to_chaintool looks correct; you might include the loaded effect nameThe MCP wrapper cleanly forwards
track_index,rack_device_index,chain_index, andeffect_urito the Live command and returns a concise summary string. Since the Remote Script now returnseffect_loadedand updateddevices_in_chain, you could include the effect name for a bit more feedback.- result = ableton.send_command("load_effect_to_chain", { + result = ableton.send_command("load_effect_to_chain", { "track_index": track_index, "rack_device_index": rack_device_index, "chain_index": chain_index, "effect_uri": effect_uri }) - return f"Loaded effect into chain {chain_index} of rack on track {track_index}" + effect_name = result.get("effect_loaded", "effect") + return f"Loaded '{effect_name}' into chain {chain_index} of rack on track {track_index}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
AbletonMCP_Remote_Script/__init__.py(9 hunks)MCP_Server/server.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
AbletonMCP_Remote_Script/__init__.py (1)
MCP_Server/server.py (1)
create_clip(329-348)
🪛 Ruff (0.14.6)
MCP_Server/server.py
401-401: Unused function argument: ctx
(ARG001)
418-418: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
424-424: Consider moving this statement to an else block
(TRY300)
425-425: Do not catch blind exception: Exception
(BLE001)
426-426: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
426-426: Use explicit conversion flag
Replace with conversion flag
(RUF010)
427-427: Use explicit conversion flag
Replace with conversion flag
(RUF010)
430-430: Unused function argument: ctx
(ARG001)
440-440: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
444-444: Consider moving this statement to an else block
(TRY300)
445-445: Do not catch blind exception: Exception
(BLE001)
446-446: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
446-446: Use explicit conversion flag
Replace with conversion flag
(RUF010)
447-447: Use explicit conversion flag
Replace with conversion flag
(RUF010)
450-450: Unused function argument: ctx
(ARG001)
463-463: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
469-469: Consider moving this statement to an else block
(TRY300)
470-470: Do not catch blind exception: Exception
(BLE001)
471-471: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
471-471: Use explicit conversion flag
Replace with conversion flag
(RUF010)
472-472: Use explicit conversion flag
Replace with conversion flag
(RUF010)
491-491: Unused function argument: ctx
(ARG001)
501-501: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
502-502: Consider moving this statement to an else block
(TRY300)
503-503: Do not catch blind exception: Exception
(BLE001)
504-504: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
504-504: Use explicit conversion flag
Replace with conversion flag
(RUF010)
505-505: Use explicit conversion flag
Replace with conversion flag
(RUF010)
508-508: Unused function argument: ctx
(ARG001)
517-517: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
518-518: Consider moving this statement to an else block
(TRY300)
519-519: Do not catch blind exception: Exception
(BLE001)
520-520: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
520-520: Use explicit conversion flag
Replace with conversion flag
(RUF010)
521-521: Use explicit conversion flag
Replace with conversion flag
(RUF010)
525-525: Unused function argument: ctx
(ARG001)
535-535: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
539-539: Consider moving this statement to an else block
(TRY300)
540-540: Do not catch blind exception: Exception
(BLE001)
541-541: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
541-541: Use explicit conversion flag
Replace with conversion flag
(RUF010)
542-542: Use explicit conversion flag
Replace with conversion flag
(RUF010)
546-546: Unused function argument: ctx
(ARG001)
563-563: Consider moving this statement to an else block
(TRY300)
564-564: Do not catch blind exception: Exception
(BLE001)
565-565: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
565-565: Use explicit conversion flag
Replace with conversion flag
(RUF010)
566-566: Use explicit conversion flag
Replace with conversion flag
(RUF010)
570-570: Unused function argument: ctx
(ARG001)
589-589: Do not catch blind exception: Exception
(BLE001)
590-590: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
590-590: Use explicit conversion flag
Replace with conversion flag
(RUF010)
591-591: Use explicit conversion flag
Replace with conversion flag
(RUF010)
595-595: Unused function argument: ctx
(ARG001)
609-609: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
617-617: Consider moving this statement to an else block
(TRY300)
618-618: Do not catch blind exception: Exception
(BLE001)
619-619: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
619-619: Use explicit conversion flag
Replace with conversion flag
(RUF010)
620-620: Use explicit conversion flag
Replace with conversion flag
(RUF010)
624-624: Unused function argument: ctx
(ARG001)
636-636: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
642-642: Consider moving this statement to an else block
(TRY300)
643-643: Do not catch blind exception: Exception
(BLE001)
644-644: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
644-644: Use explicit conversion flag
Replace with conversion flag
(RUF010)
645-645: Use explicit conversion flag
Replace with conversion flag
(RUF010)
AbletonMCP_Remote_Script/__init__.py
617-617: Abstract raise to an inner function
(TRY301)
617-617: Avoid specifying long messages outside the exception class
(TRY003)
622-622: Abstract raise to an inner function
(TRY301)
622-622: Avoid specifying long messages outside the exception class
(TRY003)
627-627: Abstract raise to an inner function
(TRY301)
627-627: Create your own exception
(TRY002)
627-627: Avoid specifying long messages outside the exception class
(TRY003)
633-633: Abstract raise to an inner function
(TRY301)
633-633: Create your own exception
(TRY002)
633-633: Avoid specifying long messages outside the exception class
(TRY003)
637-637: Abstract raise to an inner function
(TRY301)
637-637: Avoid specifying long messages outside the exception class
(TRY003)
642-642: Abstract raise to an inner function
(TRY301)
642-642: Avoid specifying long messages outside the exception class
(TRY003)
647-647: Abstract raise to an inner function
(TRY301)
647-647: Create your own exception
(TRY002)
647-647: Avoid specifying long messages outside the exception class
(TRY003)
675-675: Consider moving this statement to an else block
(TRY300)
684-684: Abstract raise to an inner function
(TRY301)
684-684: Avoid specifying long messages outside the exception class
(TRY003)
689-689: Abstract raise to an inner function
(TRY301)
689-689: Avoid specifying long messages outside the exception class
(TRY003)
694-694: Abstract raise to an inner function
(TRY301)
694-694: Create your own exception
(TRY002)
694-694: Avoid specifying long messages outside the exception class
(TRY003)
704-704: Consider moving this statement to an else block
(TRY300)
725-725: Consider moving this statement to an else block
(TRY300)
747-747: Abstract raise to an inner function
(TRY301)
747-747: Avoid specifying long messages outside the exception class
(TRY003)
756-756: Consider moving this statement to an else block
(TRY300)
769-769: Consider moving this statement to an else block
(TRY300)
783-783: Abstract raise to an inner function
(TRY301)
783-783: Avoid specifying long messages outside the exception class
(TRY003)
803-803: Abstract raise to an inner function
(TRY301)
803-803: Create your own exception
(TRY002)
803-803: Avoid specifying long messages outside the exception class
(TRY003)
826-826: Consider moving this statement to an else block
(TRY300)
835-835: Abstract raise to an inner function
(TRY301)
835-835: Avoid specifying long messages outside the exception class
(TRY003)
840-840: Abstract raise to an inner function
(TRY301)
840-840: Avoid specifying long messages outside the exception class
(TRY003)
846-846: Abstract raise to an inner function
(TRY301)
846-846: Create your own exception
(TRY002)
846-846: Avoid specifying long messages outside the exception class
(TRY003)
854-854: Abstract raise to an inner function
(TRY301)
854-854: Create your own exception
(TRY002)
854-854: Avoid specifying long messages outside the exception class
(TRY003)
864-864: Abstract raise to an inner function
(TRY301)
864-864: Create your own exception
(TRY002)
879-879: Consider moving this statement to an else block
(TRY300)
888-888: Abstract raise to an inner function
(TRY301)
888-888: Avoid specifying long messages outside the exception class
(TRY003)
897-897: Abstract raise to an inner function
(TRY301)
897-897: Create your own exception
(TRY002)
897-897: Avoid specifying long messages outside the exception class
(TRY003)
899-899: Abstract raise to an inner function
(TRY301)
899-899: Avoid specifying long messages outside the exception class
(TRY003)
902-902: Abstract raise to an inner function
(TRY301)
906-906: Abstract raise to an inner function
(TRY301)
906-906: Avoid specifying long messages outside the exception class
(TRY003)
927-927: Consider moving this statement to an else block
(TRY300)
936-936: Abstract raise to an inner function
(TRY301)
936-936: Avoid specifying long messages outside the exception class
(TRY003)
944-944: Abstract raise to an inner function
(TRY301)
944-944: Create your own exception
(TRY002)
944-944: Avoid specifying long messages outside the exception class
(TRY003)
946-946: Abstract raise to an inner function
(TRY301)
946-946: Avoid specifying long messages outside the exception class
(TRY003)
949-949: Abstract raise to an inner function
(TRY301)
949-949: Avoid specifying long messages outside the exception class
(TRY003)
953-953: Abstract raise to an inner function
(TRY301)
953-953: Avoid specifying long messages outside the exception class
(TRY003)
964-964: Abstract raise to an inner function
(TRY301)
964-964: Create your own exception
(TRY002)
973-973: Consider moving this statement to an else block
(TRY300)
982-982: Abstract raise to an inner function
(TRY301)
982-982: Avoid specifying long messages outside the exception class
(TRY003)
987-987: Abstract raise to an inner function
(TRY301)
987-987: Avoid specifying long messages outside the exception class
(TRY003)
993-993: Abstract raise to an inner function
(TRY301)
993-993: Create your own exception
(TRY002)
993-993: Avoid specifying long messages outside the exception class
(TRY003)
996-996: Abstract raise to an inner function
(TRY301)
1009-1009: Abstract raise to an inner function
(TRY301)
1009-1009: Create your own exception
(TRY002)
1053-1053: Abstract raise to an inner function
(TRY301)
1053-1053: Create your own exception
(TRY002)
1053-1053: Avoid specifying long messages outside the exception class
(TRY003)
1063-1063: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (5)
AbletonMCP_Remote_Script/__init__.py (4)
4-15: Type annotations and server lifecycle fields look consistentUsing
ControlSurface # type: ignore,Optional[socket.socket], andlist[threading.Thread]here is coherent with the rest of the module and makes the socket lifecycle easier to reason about. No issues from a correctness standpoint.Also applies to: 26-39
94-138: Defensiveself.server is Noneguard in_server_threadis sensibleThe early return when
self.serverisNoneavoids hard‑to‑debug attribute errors inside the server loop and matches the typedOptional[socket.socket]field. The rest of the loop still respectsself.runningand socket timeouts. Looks good.
831-882: Rack chain creation flow looks robustIndex checks,
can_have_chains,hasattr(device, 'insert_chain'), and the before/after chain count validation together make_create_rack_chainwell‑guarded. Returningchain_index,chain_name, andtotal_chainsis exactly what the MCP side needs. No issues here.
1552-1557: Safer browser item metadata extraction is a nice improvementUsing
getattr(current_item, 'name', "Unknown"),getattr(..., 'uri', None), and guardedhasattrchecks for booleans makesget_browser_items_at_pathmore robust against missing properties and browser differences. This should reduce runtime errors in odd browser states.MCP_Server/server.py (1)
15-21: Optionalsockfield and connect/disconnect logic look goodAnnotating
sockasOptional[socket.socket]and centralizing the connect/disconnect behavior inAbletonConnectionmakes the lifecycle clearer. Theconnectguard (if self.sock) plusdisconnectnulling the field are consistent with the laterassert self.sock is not Noneinsend_command.Also applies to: 19-45
| def _duplicate_clip(self, source_track_index, source_clip_index, dest_track_index, dest_clip_index): | ||
| """Duplicate a clip from one slot to another""" | ||
| try: | ||
| # Validate source track and clip | ||
| if source_track_index < 0 or source_track_index >= len(self._song.tracks): | ||
| raise IndexError("Source track index out of range") | ||
|
|
||
| source_track = self._song.tracks[source_track_index] | ||
|
|
||
| if source_clip_index < 0 or source_clip_index >= len(source_track.clip_slots): | ||
| raise IndexError("Source clip index out of range") | ||
|
|
||
| source_clip_slot = source_track.clip_slots[source_clip_index] | ||
|
|
||
| if not source_clip_slot.has_clip: | ||
| raise Exception("No clip in source slot") | ||
|
|
||
| source_clip = source_clip_slot.clip | ||
|
|
||
| # Only MIDI clips are supported for duplication | ||
| if not source_clip.is_midi_clip: | ||
| raise Exception("duplicate_clip only supports MIDI clips. Audio clip duplication is not yet implemented.") | ||
|
|
||
| # Validate destination track and clip slot | ||
| if dest_track_index < 0 or dest_track_index >= len(self._song.tracks): | ||
| raise IndexError("Destination track index out of range") | ||
|
|
||
| dest_track = self._song.tracks[dest_track_index] | ||
|
|
||
| if dest_clip_index < 0 or dest_clip_index >= len(dest_track.clip_slots): | ||
| raise IndexError("Destination clip index out of range") | ||
|
|
||
| dest_clip_slot = dest_track.clip_slots[dest_clip_index] | ||
|
|
||
| if dest_clip_slot.has_clip: | ||
| raise Exception("Destination clip slot already has a clip") | ||
|
|
||
| # Get source clip properties | ||
| clip_length = source_clip.length | ||
| clip_name = source_clip.name | ||
|
|
||
| # Create a new clip in the destination | ||
| dest_clip_slot.create_clip(clip_length) | ||
| dest_clip = dest_clip_slot.clip | ||
| dest_clip.name = clip_name | ||
|
|
||
| # Copy MIDI notes if it's a MIDI clip | ||
| if source_clip.is_midi_clip: | ||
| # Get all notes from source clip | ||
| notes = source_clip.get_notes(0, 0, clip_length, 128) | ||
|
|
||
| # set_notes expects a tuple of tuples: ((pitch, start_time, duration, velocity, mute), ...) | ||
| if notes and len(notes) > 0: | ||
| dest_clip.set_notes(notes) | ||
|
|
||
| result = { | ||
| "source_track": source_track_index, | ||
| "source_clip": source_clip_index, | ||
| "dest_track": dest_track_index, | ||
| "dest_clip": dest_clip_index, | ||
| "clip_name": clip_name, | ||
| "clip_length": clip_length | ||
| } | ||
| return result | ||
| except Exception as e: | ||
| self.log_message("Error duplicating clip: " + str(e)) | ||
| raise |
There was a problem hiding this comment.
MIDI‑only duplication behavior is correct but should be mirrored in higher‑level docs
The early is_midi_clip check and explicit error make the MIDI‑only limitation explicit and fix the earlier audio‑clip failure mode. The redundant if source_clip.is_midi_clip before copying notes is harmless.
It would be good to ensure the MCP tool docstring in MCP_Server/server.py::duplicate_clip also calls out the MIDI‑only behavior so callers aren’t surprised by the error message.
def _duplicate_clip(self, source_track_index, source_clip_index, dest_track_index, dest_clip_index):
@@
- # Only MIDI clips are supported for duplication
+ # Only MIDI clips are supported for duplication
if not source_clip.is_midi_clip:
raise Exception("duplicate_clip only supports MIDI clips. Audio clip duplication is not yet implemented.")
@@
- # Copy MIDI notes if it's a MIDI clip
- if source_clip.is_midi_clip:
+ # Copy MIDI notes (we already checked is_midi_clip above)
+ if source_clip.is_midi_clip:
# Get all notes from source clip
notes = source_clip.get_notes(0, 0, clip_length, 128)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.6)
617-617: Abstract raise to an inner function
(TRY301)
617-617: Avoid specifying long messages outside the exception class
(TRY003)
622-622: Abstract raise to an inner function
(TRY301)
622-622: Avoid specifying long messages outside the exception class
(TRY003)
627-627: Abstract raise to an inner function
(TRY301)
627-627: Create your own exception
(TRY002)
627-627: Avoid specifying long messages outside the exception class
(TRY003)
633-633: Abstract raise to an inner function
(TRY301)
633-633: Create your own exception
(TRY002)
633-633: Avoid specifying long messages outside the exception class
(TRY003)
637-637: Abstract raise to an inner function
(TRY301)
637-637: Avoid specifying long messages outside the exception class
(TRY003)
642-642: Abstract raise to an inner function
(TRY301)
642-642: Avoid specifying long messages outside the exception class
(TRY003)
647-647: Abstract raise to an inner function
(TRY301)
647-647: Create your own exception
(TRY002)
647-647: Avoid specifying long messages outside the exception class
(TRY003)
675-675: Consider moving this statement to an else block
(TRY300)
🤖 Prompt for AI Agents
In MCP_Server/server.py locate the duplicate_clip function docstring (around the
function definition for duplicate_clip) and update it to explicitly state that
duplication only supports MIDI clips (audio clip duplication is not
implemented), mention that an exception is raised for non‑MIDI clips, and update
the Raises/Returns sections to reflect this behavior so callers are not
surprised by the error thrown from
AbletonMCP_Remote_Script.__init__._duplicate_clip.
| def _create_audio_effect_rack(self, track_index, device_index=-1): | ||
| """Create an empty Audio Effect Rack on a track. | ||
|
|
||
| Parameters: | ||
| - track_index: The track to add the rack to | ||
| - device_index: Position in device chain to insert the rack (-1 = append to end) | ||
| """ | ||
| try: | ||
| if track_index < 0 or track_index >= len(self._song.tracks): | ||
| raise IndexError("Track index out of range") | ||
|
|
||
| track = self._song.tracks[track_index] | ||
| devices_before = len(track.devices) | ||
|
|
||
| # Select the track first | ||
| self._song.view.selected_track = track | ||
|
|
||
| # Find Audio Effect Rack in browser | ||
| app = self.application() | ||
| browser = app.browser | ||
|
|
||
| # Navigate to Audio Effect Rack | ||
| rack_item = None | ||
| for item in browser.audio_effects.children: | ||
| if item.name == "Audio Effect Rack": | ||
| rack_item = item | ||
| break | ||
|
|
||
| if not rack_item: | ||
| raise Exception("Could not find Audio Effect Rack in browser") | ||
|
|
||
| # Load the rack (loads to end of device chain) | ||
| browser.load_item(rack_item) | ||
|
|
||
| # browser.load_item() is asynchronous - brief delay needed for device to appear | ||
| # This is a known limitation of the Live API | ||
| import time | ||
| time.sleep(0.1) | ||
|
|
||
| # If device_index is specified and valid, move the rack to that position | ||
| devices_after = len(track.devices) | ||
| if devices_after > devices_before and device_index >= 0: | ||
| new_rack = track.devices[devices_after - 1] # The newly loaded rack | ||
| if device_index < devices_after - 1: # Only move if not already at desired position | ||
| self._song.move_device(new_rack, track, device_index) | ||
| self.log_message("Moved rack to device index: " + str(device_index)) | ||
|
|
||
| result = { | ||
| "track_index": track_index, | ||
| "device_index": device_index if device_index >= 0 else len(track.devices) - 1, | ||
| "device_count": len(track.devices) | ||
| } | ||
| return result | ||
| except Exception as e: | ||
| self.log_message("Error creating audio effect rack: " + str(e)) | ||
| raise |
There was a problem hiding this comment.
_create_audio_effect_rack: tighten device_index handling and success validation
Two issues here:
-
Out‑of‑range positive
device_indexis not validated and mis‑reported
Ifdevice_indexis larger than the final device count, you skipmove_deviceand then return that out‑of‑rangedevice_index, even though the rack actually lives at the end of the chain. That makes the reported index incorrect and can break follow‑up calls that rely on it. -
No error when the rack fails to load
Ifbrowser.load_itemdoesn’t add a device (e.g., browser state issues), you still return success with unchangeddevice_count.
@@
- track = self._song.tracks[track_index]
- devices_before = len(track.devices)
+ track = self._song.tracks[track_index]
+ devices_before = len(track.devices)
@@
- # Load the rack (loads to end of device chain)
+ # Load the rack (loads to end of device chain)
browser.load_item(rack_item)
@@
- # browser.load_item() is asynchronous - brief delay needed for device to appear
- # This is a known limitation of the Live API
- import time
- time.sleep(0.1)
-
- # If device_index is specified and valid, move the rack to that position
- devices_after = len(track.devices)
- if devices_after > devices_before and device_index >= 0:
- new_rack = track.devices[devices_after - 1] # The newly loaded rack
- if device_index < devices_after - 1: # Only move if not already at desired position
- self._song.move_device(new_rack, track, device_index)
- self.log_message("Moved rack to device index: " + str(device_index))
-
- result = {
- "track_index": track_index,
- "device_index": device_index if device_index >= 0 else len(track.devices) - 1,
- "device_count": len(track.devices)
- }
+ # browser.load_item() is asynchronous - brief delay needed for device to appear
+ # This is a known limitation of the Live API
+ import time
+ time.sleep(0.1)
+
+ devices_after = len(track.devices)
+ if devices_after <= devices_before:
+ raise Exception(
+ "Audio Effect Rack did not load onto track (devices: {} -> {})".format(
+ devices_before, devices_after
+ )
+ )
+
+ # Newly loaded rack is assumed to be last
+ new_rack_index = devices_after - 1
+ new_rack = track.devices[new_rack_index]
+
+ # If a valid insertion index was requested, move the rack there
+ if 0 <= device_index < new_rack_index:
+ self._song.move_device(new_rack, track, device_index)
+ self.log_message("Moved rack to device index: " + str(device_index))
+ new_rack_index = device_index
+
+ result = {
+ "track_index": track_index,
+ "device_index": new_rack_index,
+ "device_count": len(track.devices)
+ }(Separately, if you find the time.sleep noticeable in the UI, consider replacing the blocking sleep with a short polling loop scheduled via schedule_message instead of blocking the main thread.)
🧰 Tools
🪛 Ruff (0.14.6)
783-783: Abstract raise to an inner function
(TRY301)
783-783: Avoid specifying long messages outside the exception class
(TRY003)
803-803: Abstract raise to an inner function
(TRY301)
803-803: Create your own exception
(TRY002)
803-803: Avoid specifying long messages outside the exception class
(TRY003)
826-826: Consider moving this statement to an else block
(TRY300)
🤖 Prompt for AI Agents
In AbletonMCP_Remote_Script/__init__.py around lines 774-829, tighten
device_index handling and validate successful load: after calling
browser.load_item(rack_item) replace the single blocking sleep with a short
polling loop (or schedule_message-based retry) that checks track.devices until
either a new device appears or a timeout is reached; if no new device appears
raise an Exception("Failed to load Audio Effect Rack into track"). Once a new
device is detected compute the actual_index = index of that new device (which
will be len(track.devices)-1 initially) and if the requested device_index is >=0
and <= actual max index then move the device and set
result["device_index"]=device_index, otherwise set
result["device_index"]=actual_index (do not return an out-of-range requested
index). Also ensure result["device_count"] reflects the post-load device count
and surface any load/move errors via raised exceptions rather than silently
returning success.
| def _get_device_parameters(self, track_index, device_index, chain_index=-1, rack_device_index=-1): | ||
| """Get all parameters of a device""" | ||
| try: | ||
| if track_index < 0 or track_index >= len(self._song.tracks): | ||
| raise IndexError("Track index out of range") | ||
|
|
||
| track = self._song.tracks[track_index] | ||
|
|
||
| # Get the device (either from track or from a chain) | ||
| if chain_index >= 0 and rack_device_index >= 0: | ||
| # Device is inside a rack chain | ||
| rack = track.devices[rack_device_index] | ||
| if not rack.can_have_chains: | ||
| raise Exception("Device is not a rack") | ||
| if chain_index >= len(rack.chains): | ||
| raise IndexError("Chain index out of range") | ||
| chain = rack.chains[chain_index] | ||
| if device_index < 0 or device_index >= len(chain.devices): | ||
| raise IndexError("Device index out of range in chain (have {}, requested {})".format(len(chain.devices), device_index)) | ||
| device = chain.devices[device_index] | ||
| else: | ||
| if device_index < 0 or device_index >= len(track.devices): | ||
| raise IndexError("Device index out of range") | ||
| device = track.devices[device_index] | ||
|
|
||
| # Get all parameters | ||
| params = [] | ||
| for i, param in enumerate(device.parameters): | ||
| param_info = { | ||
| "index": i, | ||
| "name": param.name, | ||
| "value": param.value, | ||
| "min": param.min, | ||
| "max": param.max, | ||
| "is_quantized": param.is_quantized | ||
| } | ||
| params.append(param_info) | ||
|
|
||
| result = { | ||
| "device_name": device.name, | ||
| "device_class": device.class_name, | ||
| "parameters": params | ||
| } | ||
| return result | ||
| except Exception as e: | ||
| self.log_message("Error getting device parameters: " + str(e)) | ||
| raise |
There was a problem hiding this comment.
Clarify chain/rack index requirements and validate rack index explicitly
The chain_index/rack_device_index pair is a bit easy to misuse:
- If
chain_index >= 0butrack_device_index < 0, you silently fall back to the track‑level device path and ignore the chain, which can target the wrong device. - In the chained path,
rack_device_indexisn’t range‑checked before indexingtrack.devices[rack_device_index], so an out‑of‑range value surfaces as a genericIndexErrorrather than a clear “rack index out of range” message.
Same pattern exists in _set_device_param.
def _get_device_parameters(self, track_index, device_index, chain_index=-1, rack_device_index=-1):
@@
- track = self._song.tracks[track_index]
-
- # Get the device (either from track or from a chain)
- if chain_index >= 0 and rack_device_index >= 0:
+ track = self._song.tracks[track_index]
+
+ # Validate chain/rack combination
+ if chain_index >= 0 and rack_device_index < 0:
+ raise ValueError("rack_device_index is required when chain_index is specified")
+
+ # Get the device (either from track or from a chain)
+ if chain_index >= 0 and rack_device_index >= 0:
# Device is inside a rack chain
- rack = track.devices[rack_device_index]
+ if rack_device_index < 0 or rack_device_index >= len(track.devices):
+ raise IndexError("Rack device index out of range")
+ rack = track.devices[rack_device_index]
@@
chain = rack.chains[chain_index]
@@
- except Exception as e:
+ except Exception as e:
self.log_message("Error getting device parameters: " + str(e))
raiseApply the same pattern in _set_device_param:
- track = self._song.tracks[track_index]
-
- # Get the device
- if chain_index >= 0 and rack_device_index >= 0:
- rack = track.devices[rack_device_index]
+ track = self._song.tracks[track_index]
+
+ if chain_index >= 0 and rack_device_index < 0:
+ raise ValueError("rack_device_index is required when chain_index is specified")
+
+ # Get the device
+ if chain_index >= 0 and rack_device_index >= 0:
+ if rack_device_index < 0 or rack_device_index >= len(track.devices):
+ raise IndexError("Rack device index out of range")
+ rack = track.devices[rack_device_index]Also applies to: 932-976
🧰 Tools
🪛 Ruff (0.14.6)
888-888: Abstract raise to an inner function
(TRY301)
888-888: Avoid specifying long messages outside the exception class
(TRY003)
897-897: Abstract raise to an inner function
(TRY301)
897-897: Create your own exception
(TRY002)
897-897: Avoid specifying long messages outside the exception class
(TRY003)
899-899: Abstract raise to an inner function
(TRY301)
899-899: Avoid specifying long messages outside the exception class
(TRY003)
902-902: Abstract raise to an inner function
(TRY301)
906-906: Abstract raise to an inner function
(TRY301)
906-906: Avoid specifying long messages outside the exception class
(TRY003)
927-927: Consider moving this statement to an else block
(TRY300)
🤖 Prompt for AI Agents
In AbletonMCP_Remote_Script/__init__.py around lines 884-930 (and similarly for
_set_device_param at 932-976), the chained-device branch silently falls back
when chain_index is set but rack_device_index is not, and the rack_device_index
is used without range checks; update both functions to require that chain_index
and rack_device_index are provided together (i.e. if one is >=0 the other must
also be >=0) and raise a clear IndexError/ValueError if that contract is
violated, then add an explicit range check for rack_device_index against
len(track.devices) with a descriptive "rack index out of range" error before
indexing track.devices[rack_device_index], keep existing checks for chain_index
and device_index within the chained-path, and mirror these exact validation and
error messages in _set_device_param as well.
| def _load_effect_to_chain(self, track_index, rack_device_index, chain_index, effect_uri): | ||
| """Load an effect into a specific chain of a rack using move_device""" | ||
| try: | ||
| if track_index < 0 or track_index >= len(self._song.tracks): | ||
| raise IndexError("Track index out of range") | ||
|
|
||
| track = self._song.tracks[track_index] | ||
|
|
||
| if rack_device_index >= len(track.devices): | ||
| raise IndexError("Rack device index out of range") | ||
|
|
||
| rack = track.devices[rack_device_index] | ||
| self.log_message("Rack name: " + rack.name) | ||
|
|
||
| if not rack.can_have_chains: | ||
| raise Exception("Device is not a rack") | ||
|
|
||
| if chain_index >= len(rack.chains): | ||
| raise IndexError("Chain index out of range (have {}, requested {})".format(len(rack.chains), chain_index)) | ||
|
|
||
| chain = rack.chains[chain_index] | ||
| self.log_message("Chain name: " + chain.name) | ||
| devices_before = len(chain.devices) | ||
| self.log_message("Devices in chain before: " + str(devices_before)) | ||
|
|
||
| # Find the browser item first | ||
| app = self.application() | ||
| browser = app.browser | ||
|
|
||
| effect_item = self._find_browser_item_by_uri(browser, effect_uri) | ||
| if not effect_item: | ||
| raise Exception("Could not find effect with URI: {}".format(effect_uri)) | ||
| self.log_message("Found effect: " + effect_item.name) | ||
|
|
||
| # Count track devices before loading | ||
| track_devices_before = len(track.devices) | ||
| self.log_message("Track devices before: " + str(track_devices_before)) | ||
|
|
||
| # Select the track and load the effect (it will go to the end of the track) | ||
| self._song.view.selected_track = track | ||
| browser.load_item(effect_item) | ||
|
|
||
| # browser.load_item() is asynchronous - delay needed for device to appear | ||
| # This is a known limitation of the Live API | ||
| import time | ||
| time.sleep(0.3) | ||
|
|
||
| # Check if a new device was added to the track | ||
| track_devices_after = len(track.devices) | ||
| self.log_message("Track devices after: " + str(track_devices_after)) | ||
|
|
||
| if track_devices_after > track_devices_before: | ||
| # The effect was loaded to the track, now move it into the chain | ||
| new_device = track.devices[track_devices_after - 1] | ||
| self.log_message("New device loaded: " + new_device.name) | ||
|
|
||
| # Use move_device to move it into the chain | ||
| # The chain is a valid target (it's a DeviceContainer) | ||
| target_position = len(chain.devices) # Add to end of chain | ||
| self.log_message("Moving device to chain at position: " + str(target_position)) | ||
|
|
||
| result_position = self._song.move_device(new_device, chain, target_position) | ||
| self.log_message("Device moved to position: " + str(result_position)) | ||
|
|
||
| time.sleep(0.2) | ||
|
|
||
| # Check result | ||
| devices_after = len(chain.devices) | ||
| self.log_message("Devices in chain after: " + str(devices_after)) | ||
|
|
||
| # Verify the effect is now in the chain | ||
| if devices_after <= devices_before: | ||
| self.log_message("Effect failed to load into chain. Track devices: " + str(len(track.devices))) | ||
| for i, d in enumerate(track.devices): | ||
| self.log_message(" Device {}: {}".format(i, d.name)) | ||
| raise Exception("Failed to load effect into chain. The effect may have loaded to the track instead.") | ||
|
|
||
| result = { | ||
| "track_index": track_index, | ||
| "rack_device_index": rack_device_index, | ||
| "chain_index": chain_index, | ||
| "chain_name": chain.name, | ||
| "devices_in_chain": devices_after, | ||
| "effect_loaded": effect_item.name | ||
| } | ||
| return result | ||
| except Exception as e: | ||
| self.log_message("Error loading effect to chain: " + str(e)) | ||
| raise |
There was a problem hiding this comment.
_load_effect_to_chain: validate indices and consider avoiding main‑thread sleeps
This implementation does a good job of:
- Verifying that an effect was actually moved into the chain (
devices_after > devices_before), and - Raising a clear error when the device didn’t end up in the chain.
Two sets of refinements would make it safer:
-
Index validation for rack/chain
As written, negativerack_device_indexandchain_indexvalues are accepted due to Python’s negative indexing semantics, which can unexpectedly target the wrong rack/chain. For this API, indices seem intended to be non‑negative. -
Blocking
time.sleepcalls on Live’s main thread
The 0.3s + 0.2s sleeps are inside aschedule_messagecallback, so they block the Live UI thread for up to ~0.5s per call. That’s often noticeable if the tool is used repeatedly.
- track = self._song.tracks[track_index]
-
- if rack_device_index >= len(track.devices):
- raise IndexError("Rack device index out of range")
-
- rack = track.devices[rack_device_index]
+ track = self._song.tracks[track_index]
+
+ if rack_device_index < 0 or rack_device_index >= len(track.devices):
+ raise IndexError("Rack device index out of range")
+
+ rack = track.devices[rack_device_index]
@@
- if chain_index >= len(rack.chains):
- raise IndexError("Chain index out of range (have {}, requested {})".format(len(rack.chains), chain_index))
+ if chain_index < 0 or chain_index >= len(rack.chains):
+ raise IndexError(
+ "Chain index out of range (have {}, requested {})".format(
+ len(rack.chains), chain_index
+ )
+ )
@@
- # browser.load_item() is asynchronous - delay needed for device to appear
- # This is a known limitation of the Live API
- import time
- time.sleep(0.3)
+ # browser.load_item() is asynchronous - brief delay needed for device to appear.
+ # NOTE: consider replacing this blocking sleep with a scheduled poll using
+ # schedule_message to avoid freezing Live's UI thread.
+ import time
+ time.sleep(0.3)
@@
- time.sleep(0.2)
+ # NOTE: likewise, consider a non-blocking follow-up instead of sleep.
+ time.sleep(0.2)🧰 Tools
🪛 Ruff (0.14.6)
982-982: Abstract raise to an inner function
(TRY301)
982-982: Avoid specifying long messages outside the exception class
(TRY003)
987-987: Abstract raise to an inner function
(TRY301)
987-987: Avoid specifying long messages outside the exception class
(TRY003)
993-993: Abstract raise to an inner function
(TRY301)
993-993: Create your own exception
(TRY002)
993-993: Avoid specifying long messages outside the exception class
(TRY003)
996-996: Abstract raise to an inner function
(TRY301)
1009-1009: Abstract raise to an inner function
(TRY301)
1009-1009: Create your own exception
(TRY002)
1053-1053: Abstract raise to an inner function
(TRY301)
1053-1053: Create your own exception
(TRY002)
1053-1053: Avoid specifying long messages outside the exception class
(TRY003)
1063-1063: Consider moving this statement to an else block
(TRY300)
🤖 Prompt for AI Agents
In AbletonMCP_Remote_Script/__init__.py around lines 978-1066, the method
accepts negative rack_device_index and chain_index (relying on Python negative
indexing) and blocks the Live main thread with time.sleep calls; update the
validation to reject negative indices (raise IndexError if rack_device_index < 0
or chain_index < 0) and replace synchronous time.sleep waits with a non‑blocking
retry/poll approach using Live's scheduling (e.g., schedule_message or a
deferred callback) that polls for the new device to appear with a short interval
and overall timeout, logging and raising on timeout.
| @mcp.tool() | ||
| def duplicate_clip( | ||
| ctx: Context, | ||
| source_track_index: int, | ||
| source_clip_index: int, | ||
| dest_track_index: int, | ||
| dest_clip_index: int | ||
| ) -> str: | ||
| """ | ||
| Duplicate a clip from one slot to another (can be on the same or different track). | ||
|
|
||
| Parameters: | ||
| - source_track_index: The index of the track containing the source clip | ||
| - source_clip_index: The index of the clip slot containing the source clip | ||
| - dest_track_index: The index of the destination track | ||
| - dest_clip_index: The index of the destination clip slot (must be empty) | ||
| """ | ||
| try: | ||
| ableton = get_ableton_connection() | ||
| result = ableton.send_command("duplicate_clip", { | ||
| "source_track_index": source_track_index, | ||
| "source_clip_index": source_clip_index, | ||
| "dest_track_index": dest_track_index, | ||
| "dest_clip_index": dest_clip_index | ||
| }) | ||
| return f"Duplicated clip from track {source_track_index}, slot {source_clip_index} to track {dest_track_index}, slot {dest_clip_index}" | ||
| except Exception as e: | ||
| logger.error(f"Error duplicating clip: {str(e)}") | ||
| return f"Error duplicating clip: {str(e)}" | ||
|
|
||
| @mcp.tool() | ||
| def empty_clip_slot(ctx: Context, track_index: int, clip_index: int) -> str: | ||
| """ | ||
| Empty a clip slot. | ||
|
|
||
| Parameters: | ||
| - track_index: The index of the track containing the clip | ||
| - clip_index: The index of the clip slot to empty | ||
| """ | ||
| try: | ||
| ableton = get_ableton_connection() | ||
| result = ableton.send_command("remove_clip", { | ||
| "track_index": track_index, | ||
| "clip_index": clip_index | ||
| }) | ||
| return f"Emptied clip slot at track {track_index}, slot {clip_index}" | ||
| except Exception as e: | ||
| logger.error(f"Error emptying clip slot: {str(e)}") | ||
| return f"Error emptying clip slot: {str(e)}" | ||
|
|
||
| @mcp.tool() | ||
| def relocate_clip(ctx: Context, source_track_index: int, source_clip_index: int, dest_track_index: int, dest_clip_index: int) -> str: | ||
| """ | ||
| Relocate a clip from one slot to another (can be on the same or different track). | ||
| This duplicates the clip to the destination and empties the source. | ||
|
|
||
| Parameters: | ||
| - source_track_index: The index of the track containing the source clip | ||
| - source_clip_index: The index of the clip slot containing the source clip | ||
| - dest_track_index: The index of the destination track | ||
| - dest_clip_index: The index of the destination clip slot (must be empty) | ||
| """ | ||
| try: | ||
| ableton = get_ableton_connection() | ||
| result = ableton.send_command("move_clip", { | ||
| "source_track_index": source_track_index, | ||
| "source_clip_index": source_clip_index, | ||
| "dest_track_index": dest_track_index, | ||
| "dest_clip_index": dest_clip_index | ||
| }) | ||
| return f"Relocated clip from track {source_track_index}, slot {source_clip_index} to track {dest_track_index}, slot {dest_clip_index}" | ||
| except Exception as e: | ||
| logger.error(f"Error relocating clip: {str(e)}") | ||
| return f"Error relocating clip: {str(e)}" | ||
|
|
There was a problem hiding this comment.
Clip tools map cleanly to Remote Script, but document MIDI‑only limitation
duplicate_clip, empty_clip_slot, and relocate_clip correctly forward parameters to the corresponding Remote Script commands and return clear, human‑readable summaries.
Given _duplicate_clip on the Live side explicitly errors on non‑MIDI clips, it would be helpful to surface that constraint in the tool docstrings so users know audio clips aren’t supported yet.
@mcp.tool()
def duplicate_clip(
@@
- """
- Duplicate a clip from one slot to another (can be on the same or different track).
-
- Parameters:
+ """
+ Duplicate a MIDI clip from one slot to another (can be on the same or different track).
+
+ Note: This currently supports only MIDI clips; attempting to use it on audio clips
+ will return an error from the Ableton Remote Script.
+
+ Parameters:
@@
@mcp.tool()
def empty_clip_slot(ctx: Context, track_index: int, clip_index: int) -> str:
@@
- """
- Empty a clip slot.
+ """
+ Empty a clip slot.
@@
@mcp.tool()
def relocate_clip(ctx: Context, source_track_index: int, source_clip_index: int, dest_track_index: int, dest_clip_index: int) -> str:
@@
- """
- Relocate a clip from one slot to another (can be on the same or different track).
+ """
+ Relocate a clip from one slot to another (can be on the same or different track).
@@
- This duplicates the clip to the destination and empties the source.
+ This duplicates the clip to the destination and then empties the source.
+ Only MIDI clips are supported currently, matching `duplicate_clip`.🧰 Tools
🪛 Ruff (0.14.6)
401-401: Unused function argument: ctx
(ARG001)
418-418: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
424-424: Consider moving this statement to an else block
(TRY300)
425-425: Do not catch blind exception: Exception
(BLE001)
426-426: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
426-426: Use explicit conversion flag
Replace with conversion flag
(RUF010)
427-427: Use explicit conversion flag
Replace with conversion flag
(RUF010)
430-430: Unused function argument: ctx
(ARG001)
440-440: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
444-444: Consider moving this statement to an else block
(TRY300)
445-445: Do not catch blind exception: Exception
(BLE001)
446-446: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
446-446: Use explicit conversion flag
Replace with conversion flag
(RUF010)
447-447: Use explicit conversion flag
Replace with conversion flag
(RUF010)
450-450: Unused function argument: ctx
(ARG001)
463-463: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
469-469: Consider moving this statement to an else block
(TRY300)
470-470: Do not catch blind exception: Exception
(BLE001)
471-471: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
471-471: Use explicit conversion flag
Replace with conversion flag
(RUF010)
472-472: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In MCP_Server/server.py around lines 399 to 473, update the docstrings for
duplicate_clip, empty_clip_slot, and relocate_clip to explicitly state these
operations are MIDI-only and will error on non‑MIDI/audio clips (matching
Live-side _duplicate_clip behavior); change each function's docstring top
section to include one sentence like “Note: this tool only supports MIDI clips —
attempting to operate on audio/non‑MIDI clips will result in an error.” and keep
the existing parameter descriptions unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
AbletonMCP_Remote_Script/__init__.py (2)
924-971: Add explicit validation for rack_device_index and chain_index.When
chain_index >= 0butrack_device_index < 0, the code silently falls through to track-level device lookup (line 945), which can target the wrong device. Additionally,rack_device_index(line 935) andchain_index(line 938) aren't validated against negative values, allowing Python's negative indexing to silently target unexpected racks/chains.Apply this diff:
track = self._song.tracks[track_index] + # Validate chain/rack combination + if chain_index >= 0 and rack_device_index < 0: + raise ValueError("rack_device_index is required when chain_index is specified") + # Get the device (either from track or from a chain) if chain_index >= 0 and rack_device_index >= 0: # Device is inside a rack chain + if rack_device_index < 0 or rack_device_index >= len(track.devices): + raise IndexError("Rack device index out of range") rack = track.devices[rack_device_index] if not rack.can_have_chains: raise Exception("Device is not a rack") - if chain_index >= len(rack.chains): + if chain_index < 0 or chain_index >= len(rack.chains): raise IndexError("Chain index out of range")Apply the same pattern to
_set_device_param(lines 972-1017).
1018-1107: Validate negative indices and consider non-blocking delays.Two refinements needed:
Index validation allows negative values
Lines 1026 and 1035 only check upper bounds. Negativerack_device_indexorchain_indexwill use Python's negative indexing, potentially targeting the wrong rack/chain.Blocking sleeps freeze Live's UI
Lines 1063 and 1082 calltime.sleepwithin aschedule_messagecallback, blocking Live's main thread for up to 0.5s total. This is noticeable when used repeatedly.Apply this diff to add validation:
track = self._song.tracks[track_index] - if rack_device_index >= len(track.devices): + if rack_device_index < 0 or rack_device_index >= len(track.devices): raise IndexError("Rack device index out of range") rack = track.devices[rack_device_index] self.log_message("Rack name: " + rack.name) if not rack.can_have_chains: raise Exception("Device is not a rack") - if chain_index >= len(rack.chains): + if chain_index < 0 or chain_index >= len(rack.chains): raise IndexError("Chain index out of range (have {}, requested {})".format(len(rack.chains), chain_index))Regarding the blocking sleeps: consider replacing them with a scheduled polling approach using
schedule_messageto check for device appearance without freezing the UI. For now, you may add a comment noting this limitation.
🧹 Nitpick comments (1)
AbletonMCP_Remote_Script/__init__.py (1)
636-670: Redundant MIDI clip check after early guard.Line 663 checks
if source_clip.is_midi_clip:but this is redundant since line 637 already raises an exception for non-MIDI clips. The second check can never be false at this point.Apply this diff to simplify:
# Only MIDI clips are supported for duplication if not source_clip.is_midi_clip: raise Exception("duplicate_clip only supports MIDI clips. Audio clip duplication is not yet implemented.") # Validate destination track and clip slot if dest_track_index < 0 or dest_track_index >= len(self._song.tracks): raise IndexError("Destination track index out of range") dest_track = self._song.tracks[dest_track_index] if dest_clip_index < 0 or dest_clip_index >= len(dest_track.clip_slots): raise IndexError("Destination clip index out of range") dest_clip_slot = dest_track.clip_slots[dest_clip_index] if dest_clip_slot.has_clip: raise Exception("Destination clip slot already has a clip") # Get source clip properties clip_length = source_clip.length clip_name = source_clip.name # Create a new clip in the destination dest_clip_slot.create_clip(clip_length) dest_clip = dest_clip_slot.clip dest_clip.name = clip_name - # Copy MIDI notes if it's a MIDI clip - if source_clip.is_midi_clip: - # Get all notes from source clip - notes = source_clip.get_notes(0, 0, clip_length, 128) - - # set_notes expects a tuple of tuples: ((pitch, start_time, duration, velocity, mute), ...) - if notes and len(notes) > 0: - dest_clip.set_notes(notes) + # Copy MIDI notes (we already verified is_midi_clip above) + notes = source_clip.get_notes(0, 0, clip_length, 128) + # set_notes expects a tuple of tuples: ((pitch, start_time, duration, velocity, mute), ...) + if notes and len(notes) > 0: + dest_clip.set_notes(notes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/docs/adding-new-tools.md(1 hunks).github/docs/debugging.md(1 hunks)AbletonMCP_Remote_Script/__init__.py(9 hunks)MCP_Server/server.py(7 hunks)TODO.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- TODO.md
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/docs/debugging.md
- .github/docs/adding-new-tools.md
🧰 Additional context used
🧬 Code graph analysis (1)
AbletonMCP_Remote_Script/__init__.py (1)
MCP_Server/server.py (1)
create_clip(329-348)
🪛 Ruff (0.14.7)
MCP_Server/server.py
401-401: Unused function argument: ctx
(ARG001)
418-418: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
424-424: Consider moving this statement to an else block
(TRY300)
425-425: Do not catch blind exception: Exception
(BLE001)
426-426: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
426-426: Use explicit conversion flag
Replace with conversion flag
(RUF010)
427-427: Use explicit conversion flag
Replace with conversion flag
(RUF010)
430-430: Unused function argument: ctx
(ARG001)
440-440: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
444-444: Consider moving this statement to an else block
(TRY300)
445-445: Do not catch blind exception: Exception
(BLE001)
446-446: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
446-446: Use explicit conversion flag
Replace with conversion flag
(RUF010)
447-447: Use explicit conversion flag
Replace with conversion flag
(RUF010)
450-450: Unused function argument: ctx
(ARG001)
463-463: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
469-469: Consider moving this statement to an else block
(TRY300)
470-470: Do not catch blind exception: Exception
(BLE001)
471-471: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
471-471: Use explicit conversion flag
Replace with conversion flag
(RUF010)
472-472: Use explicit conversion flag
Replace with conversion flag
(RUF010)
491-491: Unused function argument: ctx
(ARG001)
501-501: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
502-502: Consider moving this statement to an else block
(TRY300)
503-503: Do not catch blind exception: Exception
(BLE001)
504-504: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
504-504: Use explicit conversion flag
Replace with conversion flag
(RUF010)
505-505: Use explicit conversion flag
Replace with conversion flag
(RUF010)
508-508: Unused function argument: ctx
(ARG001)
517-517: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
518-518: Consider moving this statement to an else block
(TRY300)
519-519: Do not catch blind exception: Exception
(BLE001)
520-520: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
520-520: Use explicit conversion flag
Replace with conversion flag
(RUF010)
521-521: Use explicit conversion flag
Replace with conversion flag
(RUF010)
525-525: Unused function argument: ctx
(ARG001)
536-536: Do not catch blind exception: Exception
(BLE001)
537-537: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
537-537: Use explicit conversion flag
Replace with conversion flag
(RUF010)
538-538: Use explicit conversion flag
Replace with conversion flag
(RUF010)
542-542: Unused function argument: ctx
(ARG001)
552-552: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
556-556: Consider moving this statement to an else block
(TRY300)
557-557: Do not catch blind exception: Exception
(BLE001)
558-558: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
558-558: Use explicit conversion flag
Replace with conversion flag
(RUF010)
559-559: Use explicit conversion flag
Replace with conversion flag
(RUF010)
563-563: Unused function argument: ctx
(ARG001)
580-580: Consider moving this statement to an else block
(TRY300)
581-581: Do not catch blind exception: Exception
(BLE001)
582-582: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
582-582: Use explicit conversion flag
Replace with conversion flag
(RUF010)
583-583: Use explicit conversion flag
Replace with conversion flag
(RUF010)
587-587: Unused function argument: ctx
(ARG001)
606-606: Do not catch blind exception: Exception
(BLE001)
607-607: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
607-607: Use explicit conversion flag
Replace with conversion flag
(RUF010)
608-608: Use explicit conversion flag
Replace with conversion flag
(RUF010)
612-612: Unused function argument: ctx
(ARG001)
626-626: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
634-634: Consider moving this statement to an else block
(TRY300)
635-635: Do not catch blind exception: Exception
(BLE001)
636-636: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
636-636: Use explicit conversion flag
Replace with conversion flag
(RUF010)
637-637: Use explicit conversion flag
Replace with conversion flag
(RUF010)
641-641: Unused function argument: ctx
(ARG001)
653-653: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
659-659: Consider moving this statement to an else block
(TRY300)
660-660: Do not catch blind exception: Exception
(BLE001)
661-661: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
661-661: Use explicit conversion flag
Replace with conversion flag
(RUF010)
662-662: Use explicit conversion flag
Replace with conversion flag
(RUF010)
AbletonMCP_Remote_Script/__init__.py
621-621: Abstract raise to an inner function
(TRY301)
621-621: Avoid specifying long messages outside the exception class
(TRY003)
626-626: Abstract raise to an inner function
(TRY301)
626-626: Avoid specifying long messages outside the exception class
(TRY003)
631-631: Abstract raise to an inner function
(TRY301)
631-631: Create your own exception
(TRY002)
631-631: Avoid specifying long messages outside the exception class
(TRY003)
637-637: Abstract raise to an inner function
(TRY301)
637-637: Create your own exception
(TRY002)
637-637: Avoid specifying long messages outside the exception class
(TRY003)
641-641: Abstract raise to an inner function
(TRY301)
641-641: Avoid specifying long messages outside the exception class
(TRY003)
646-646: Abstract raise to an inner function
(TRY301)
646-646: Avoid specifying long messages outside the exception class
(TRY003)
651-651: Abstract raise to an inner function
(TRY301)
651-651: Create your own exception
(TRY002)
651-651: Avoid specifying long messages outside the exception class
(TRY003)
679-679: Consider moving this statement to an else block
(TRY300)
688-688: Abstract raise to an inner function
(TRY301)
688-688: Avoid specifying long messages outside the exception class
(TRY003)
693-693: Abstract raise to an inner function
(TRY301)
693-693: Avoid specifying long messages outside the exception class
(TRY003)
698-698: Abstract raise to an inner function
(TRY301)
698-698: Create your own exception
(TRY002)
698-698: Avoid specifying long messages outside the exception class
(TRY003)
708-708: Consider moving this statement to an else block
(TRY300)
729-729: Consider moving this statement to an else block
(TRY300)
751-751: Abstract raise to an inner function
(TRY301)
751-751: Avoid specifying long messages outside the exception class
(TRY003)
760-760: Consider moving this statement to an else block
(TRY300)
773-773: Consider moving this statement to an else block
(TRY300)
790-790: Abstract raise to an inner function
(TRY301)
809-809: Consider moving this statement to an else block
(TRY300)
823-823: Abstract raise to an inner function
(TRY301)
823-823: Avoid specifying long messages outside the exception class
(TRY003)
843-843: Abstract raise to an inner function
(TRY301)
843-843: Create your own exception
(TRY002)
843-843: Avoid specifying long messages outside the exception class
(TRY003)
866-866: Consider moving this statement to an else block
(TRY300)
875-875: Abstract raise to an inner function
(TRY301)
875-875: Avoid specifying long messages outside the exception class
(TRY003)
880-880: Abstract raise to an inner function
(TRY301)
880-880: Avoid specifying long messages outside the exception class
(TRY003)
886-886: Abstract raise to an inner function
(TRY301)
886-886: Create your own exception
(TRY002)
886-886: Avoid specifying long messages outside the exception class
(TRY003)
894-894: Abstract raise to an inner function
(TRY301)
894-894: Create your own exception
(TRY002)
894-894: Avoid specifying long messages outside the exception class
(TRY003)
904-904: Abstract raise to an inner function
(TRY301)
904-904: Create your own exception
(TRY002)
919-919: Consider moving this statement to an else block
(TRY300)
928-928: Abstract raise to an inner function
(TRY301)
928-928: Avoid specifying long messages outside the exception class
(TRY003)
937-937: Abstract raise to an inner function
(TRY301)
937-937: Create your own exception
(TRY002)
937-937: Avoid specifying long messages outside the exception class
(TRY003)
939-939: Abstract raise to an inner function
(TRY301)
939-939: Avoid specifying long messages outside the exception class
(TRY003)
942-942: Abstract raise to an inner function
(TRY301)
946-946: Abstract raise to an inner function
(TRY301)
946-946: Avoid specifying long messages outside the exception class
(TRY003)
967-967: Consider moving this statement to an else block
(TRY300)
976-976: Abstract raise to an inner function
(TRY301)
976-976: Avoid specifying long messages outside the exception class
(TRY003)
984-984: Abstract raise to an inner function
(TRY301)
984-984: Create your own exception
(TRY002)
984-984: Avoid specifying long messages outside the exception class
(TRY003)
986-986: Abstract raise to an inner function
(TRY301)
986-986: Avoid specifying long messages outside the exception class
(TRY003)
989-989: Abstract raise to an inner function
(TRY301)
989-989: Avoid specifying long messages outside the exception class
(TRY003)
993-993: Abstract raise to an inner function
(TRY301)
993-993: Avoid specifying long messages outside the exception class
(TRY003)
1004-1004: Abstract raise to an inner function
(TRY301)
1004-1004: Create your own exception
(TRY002)
1013-1013: Consider moving this statement to an else block
(TRY300)
1022-1022: Abstract raise to an inner function
(TRY301)
1022-1022: Avoid specifying long messages outside the exception class
(TRY003)
1027-1027: Abstract raise to an inner function
(TRY301)
1027-1027: Avoid specifying long messages outside the exception class
(TRY003)
1033-1033: Abstract raise to an inner function
(TRY301)
1033-1033: Create your own exception
(TRY002)
1033-1033: Avoid specifying long messages outside the exception class
(TRY003)
1036-1036: Abstract raise to an inner function
(TRY301)
1049-1049: Abstract raise to an inner function
(TRY301)
1049-1049: Create your own exception
(TRY002)
1093-1093: Abstract raise to an inner function
(TRY301)
1093-1093: Create your own exception
(TRY002)
1093-1093: Avoid specifying long messages outside the exception class
(TRY003)
1103-1103: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (7)
AbletonMCP_Remote_Script/__init__.py (3)
4-4: LGTM: Type annotations properly added for Python 2/3 compatibility.The type hints and
type: ignorecomments appropriately handle Live API imports and maintain compatibility with both Python 2 and 3 environments.Also applies to: 10-10, 14-14, 26-26, 29-29, 35-38
98-102: LGTM: Defensive guard prevents potential AttributeError.The None check for
self.serverbefore starting the server thread is good defensive programming.
1592-1597: LGTM: Safer attribute access with getattr.Using
getattrwith default values prevents AttributeError when browser items have inconsistent attribute availability. This is solid defensive programming.MCP_Server/server.py (4)
6-6: LGTM: Type safety improved with Optional and assertions.The changes properly handle the optional socket field with type hints and runtime assertions. Line 119's assertion ensures
sockis not None after the connection check on line 95.Also applies to: 8-8, 19-19, 93-93, 118-120
490-521: LGTM: Volume control tools are well-documented.Both functions correctly document that 0.85 = 0dB, which helps users understand the volume scale.
524-663: LGTM: Rack and chain tools properly expose Remote Script functionality.The tool implementations correctly forward parameters and provide clear docstrings. The underlying device_index handling issues in the Remote Script are addressed in a separate comment on
__init__.py.
104-112: LGTM: New state-modifying commands properly registered.All new commands (duplicate_clip, move_clip, set_track_volume, create_audio_effect_rack, etc.) are correctly classified as state-modifying, ensuring appropriate delays and timeouts.
| def _create_audio_effect_rack(self, track_index, device_index=-1): | ||
| """Create an empty Audio Effect Rack on a track. | ||
|
|
||
| Parameters: | ||
| - track_index: The track to add the rack to | ||
| - device_index: Position in device chain to insert the rack (-1 = append to end) | ||
| """ | ||
| try: | ||
| if track_index < 0 or track_index >= len(self._song.tracks): | ||
| raise IndexError("Track index out of range") | ||
|
|
||
| track = self._song.tracks[track_index] | ||
| devices_before = len(track.devices) | ||
|
|
||
| # Select the track first | ||
| self._song.view.selected_track = track | ||
|
|
||
| # Find Audio Effect Rack in browser | ||
| app = self.application() | ||
| browser = app.browser | ||
|
|
||
| # Navigate to Audio Effect Rack | ||
| rack_item = None | ||
| for item in browser.audio_effects.children: | ||
| if item.name == "Audio Effect Rack": | ||
| rack_item = item | ||
| break | ||
|
|
||
| if not rack_item: | ||
| raise Exception("Could not find Audio Effect Rack in browser") | ||
|
|
||
| # Load the rack (loads to end of device chain) | ||
| browser.load_item(rack_item) | ||
|
|
||
| # browser.load_item() is asynchronous - brief delay needed for device to appear | ||
| # This is a known limitation of the Live API | ||
| import time | ||
| time.sleep(0.1) | ||
|
|
||
| # If device_index is specified and valid, move the rack to that position | ||
| devices_after = len(track.devices) | ||
| if devices_after > devices_before and device_index >= 0: | ||
| new_rack = track.devices[devices_after - 1] # The newly loaded rack | ||
| if device_index < devices_after - 1: # Only move if not already at desired position | ||
| self._song.move_device(new_rack, track, device_index) | ||
| self.log_message("Moved rack to device index: " + str(device_index)) | ||
|
|
||
| result = { | ||
| "track_index": track_index, | ||
| "device_index": device_index if device_index >= 0 else len(track.devices) - 1, | ||
| "device_count": len(track.devices) | ||
| } | ||
| return result | ||
| except Exception as e: | ||
| self.log_message("Error creating audio effect rack: " + str(e)) | ||
| raise | ||
|
|
There was a problem hiding this comment.
Tighten device_index handling and validate successful rack load.
Two issues remain:
-
Out-of-range device_index is silently ignored and mis-reported
Ifdevice_indexexceeds the device chain length (e.g., 10 when only 3 devices exist), line 857's condition fails, the rack stays at the end, but line 863 returns the out-of-rangedevice_indexanyway. This makes the reported position incorrect. -
No validation that the rack actually loaded
Ifbrowser.load_itemfails silently (e.g., browser state issues),devices_aftermay equaldevices_beforebut the function still returns success.
Apply this diff to fix both issues:
# Load the rack (loads to end of device chain)
browser.load_item(rack_item)
# browser.load_item() is asynchronous - brief delay needed for device to appear
# This is a known limitation of the Live API
import time
time.sleep(0.1)
- # If device_index is specified and valid, move the rack to that position
devices_after = len(track.devices)
- if devices_after > devices_before and device_index >= 0:
- new_rack = track.devices[devices_after - 1] # The newly loaded rack
- if device_index < devices_after - 1: # Only move if not already at desired position
- self._song.move_device(new_rack, track, device_index)
- self.log_message("Moved rack to device index: " + str(device_index))
+
+ # Verify rack was loaded
+ if devices_after <= devices_before:
+ raise Exception("Audio Effect Rack did not load (devices: {} -> {})".format(
+ devices_before, devices_after
+ ))
+
+ # The newly loaded rack is at the end
+ new_rack_index = devices_after - 1
+ new_rack = track.devices[new_rack_index]
+
+ # If a valid insertion index was requested, move the rack there
+ if 0 <= device_index < new_rack_index:
+ self._song.move_device(new_rack, track, device_index)
+ self.log_message("Moved rack to device index: " + str(device_index))
+ new_rack_index = device_index
result = {
"track_index": track_index,
- "device_index": device_index if device_index >= 0 else len(track.devices) - 1,
+ "device_index": new_rack_index,
"device_count": len(track.devices)
}🧰 Tools
🪛 Ruff (0.14.7)
823-823: Abstract raise to an inner function
(TRY301)
823-823: Avoid specifying long messages outside the exception class
(TRY003)
843-843: Abstract raise to an inner function
(TRY301)
843-843: Create your own exception
(TRY002)
843-843: Avoid specifying long messages outside the exception class
(TRY003)
866-866: Consider moving this statement to an else block
(TRY300)
🤖 Prompt for AI Agents
In AbletonMCP_Remote_Script/__init__.py around lines 814 to 870, tighten
device_index handling and validate the rack actually loaded: after loading the
rack and computing devices_after, first verify devices_after > devices_before
and if not raise an exception (or return an error) so we don’t claim success
when nothing was added; then handle device_index by checking against the actual
new device count (clamp it to the valid range or raise if out-of-range), only
attempt move when the desired index is within the new range, and finally set the
returned device_index to the actual position of the newly added rack (the end
index or the target index after a successful move) instead of echoing an
out-of-range requested value.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCP_Server/server.py (1)
872-903: Check final drum kit load result before claiming success
load_drum_kitverifies that the rack load succeeded and that a loadable kit exists, but it doesn’t check whether the finalload_browser_itemcall forkit_uriactually succeeded. If that step fails, the function will still return"Loaded drum rack and kit ..."which is misleading.You can reuse the same
loadedflag pattern you used earlier:- kit_uri = loadable_kits[0].get("uri") - load_result = ableton.send_command("load_browser_item", { - "track_index": track_index, - "item_uri": kit_uri - }) - - return f"Loaded drum rack and kit '{loadable_kits[0].get('name')}' on track {track_index}" + kit = loadable_kits[0] + kit_uri = kit.get("uri") + if not kit_uri: + return f"Loaded drum rack but selected kit '{kit.get('name')}' has no URI and cannot be loaded." + + load_result = ableton.send_command("load_browser_item", { + "track_index": track_index, + "item_uri": kit_uri + }) + + if not load_result.get("loaded", False): + return f"Loaded drum rack but failed to load kit '{kit.get('name')}' on track {track_index}" + + return f"Loaded drum rack and kit '{kit.get('name')}' on track {track_index}"This keeps the user‑facing string aligned with what actually happened and guards against a missing
uri.
♻️ Duplicate comments (1)
MCP_Server/server.py (1)
398-405: Docstrings still don’t mention MIDI‑only limitation for clip toolsPrevious review notes mentioned that the Remote Script’s
_duplicate_clipimplementation errors on non‑MIDI clips, makingduplicate_clipandrelocate_clipeffectively MIDI‑only. The current docstrings still read as if they work for any clip type, which can surprise users. (This is the same concern as the earlier review.)Suggested doc tweaks:
def duplicate_clip( @@ - """ - Duplicate a clip from one slot to another (can be on the same or different track). + """ + Duplicate a MIDI clip from one slot to another (can be on the same or different track). + + Note: Only MIDI clips are supported. Attempting to duplicate audio/non‑MIDI clips + will result in an error from the Ableton Remote Script. @@ def relocate_clip(source_track_index: int, source_clip_index: int, dest_track_index: int, dest_clip_index: int) -> str: @@ - """ - Relocate a clip from one slot to another (can be on the same or different track). - This duplicates the clip to the destination and empties the source. + """ + Relocate a MIDI clip from one slot to another (can be on the same or different track). + This duplicates the clip to the destination and then empties the source. + + Note: Only MIDI clips are supported, matching the limitation of duplicate_clip.Also applies to: 448-452
🧹 Nitpick comments (5)
MCP_Server/server.py (5)
152-169: Consider a dedicated exception type and preserving original error contextThe generic
Exceptionrethrows insend_commandlose the original exception type and currently don’t useraise ... from e, which makes debugging and distinguishing failure modes harder for callers, even though you log the details.Consider introducing a small custom exception and chaining from the original:
+class AbletonCommunicationError(Exception): + """Raised when communication with Ableton fails.""" @@ - except socket.timeout: - logger.error("Socket timeout while waiting for response from Ableton") - self.sock = None - raise Exception("Timeout waiting for Ableton response") - except (ConnectionError, BrokenPipeError, ConnectionResetError) as e: - logger.error(f"Socket connection error: {e}") - self.sock = None - raise Exception(f"Connection to Ableton lost: {e}") - except json.JSONDecodeError as e: - logger.error(f"Invalid JSON response from Ableton: {e}") + except socket.timeout as e: + logger.exception("Socket timeout while waiting for response from Ableton") + self.sock = None + raise AbletonCommunicationError("Timeout waiting for Ableton response") from e + except (ConnectionError, BrokenPipeError, ConnectionResetError) as e: + logger.exception("Socket connection error") + self.sock = None + raise AbletonCommunicationError("Connection to Ableton lost") from e + except json.JSONDecodeError as e: + logger.exception("Invalid JSON response from Ableton") if response_data: logger.error(f"Raw response (first 200 bytes): {response_data[:200]}") self.sock = None - raise Exception(f"Invalid response from Ableton: {e}") - except Exception as e: - logger.error(f"Error communicating with Ableton: {e}") - self.sock = None - raise Exception(f"Communication error with Ableton: {e}") + raise AbletonCommunicationError("Invalid response from Ableton") from e + except Exception as e: + logger.exception("Unexpected error communicating with Ableton") + self.sock = None + raise AbletonCommunicationError("Communication error with Ableton") from eThis keeps logs rich, preserves traceback, and lets callers specifically catch communication failures.
206-213: Connection “ping” withsendall(b'')is effectively a no‑opThe health‑check using
sock.sendall(b'')is unlikely to reliably detect a dead TCP connection, because sending an empty payload is effectively a no‑op and may not trigger an error even if the peer is gone.Given you already validate the connection once with
get_session_infoon creation, you could either:
- Drop this ping entirely and let the first real command surface the failure (which you already handle in
send_command), or- Replace it with a very lightweight real command (e.g. a dedicated
"ping"or"get_session_info"again) that the Remote Script understands.This is a minor robustness tweak; current behavior will still eventually recover via the error path in
send_command.
584-594: Clarify and/or validatechain_index/rack_device_indexinterplayThe docstrings for
get_device_parametersandset_device_parameterstate thatrack_device_indexis “required whenchain_index >= 0”, but the function signatures make both optional with a default of-1, and there’s no local validation.To avoid confusing callers:
- Either add a small validation that raises a clear error if
chain_index >= 0andrack_device_index < 0, or- Relax the docstring wording to “must be provided when
chain_index >= 0” and explicitly state that omitting it will result in an error from the Remote Script.Example validation:
def get_device_parameters(..., chain_index: int = -1, rack_device_index: int = -1) -> str: @@ - ableton = get_ableton_connection() + ableton = get_ableton_connection() + if chain_index >= 0 and rack_device_index < 0: + return "Error: rack_device_index must be provided when querying a device inside a rack chain." @@ def set_device_parameter(..., chain_index: int = -1, rack_device_index: int = -1) -> str: @@ - ableton = get_ableton_connection() + ableton = get_ableton_connection() + if chain_index >= 0 and rack_device_index < 0: + return "Error: rack_device_index must be provided when setting a parameter on a device inside a rack chain."Also applies to: 609-621
755-817: Browser tree error handling is user‑friendly; logging may be a bit noisyThe mapping from Remote Script errors to human‑readable guidance in
get_browser_treeis solid, especially the tailored messages for “browser not available” vs “cannot access Live application”.You are using
logger.exceptioneven for these expected conditions, which will emit full stack traces. If that’s too noisy in practice, consider downgrading these specific branches tologger.warningorlogger.errorand reservelogger.exceptionfor unexpected failures in the finalelse.
819-858: Granular error mapping for browser paths is nice; same note on logging noise
get_browser_items_at_pathdoes a good job turning Remote Script error strings into actionable messages (unknown category, missing path part, etc.), which will be very helpful for tools/agents.Similar to
get_browser_tree, every known error case useslogger.exception, which may clutter logs with stack traces for routine user errors (typos in paths, etc.). Consider:
- Using
logger.warningfor path/category issues, and- Keeping
logger.exceptiononly in the final “unexpected error” branch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCP_Server/server.py(25 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
MCP_Server/server.py (1)
AbletonMCP_Remote_Script/__init__.py (2)
get_browser_tree(1367-1481)get_browser_items_at_path(1483-1606)
🪛 Ruff (0.14.7)
MCP_Server/server.py
32-32: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
42-42: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
75-75: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
78-78: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
157-157: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
159-159: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
159-159: Create your own exception
(TRY002)
159-159: Avoid specifying long messages outside the exception class
(TRY003)
161-161: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
163-163: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
165-165: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
165-165: Create your own exception
(TRY002)
165-165: Avoid specifying long messages outside the exception class
(TRY003)
166-166: Do not catch blind exception: Exception
(BLE001)
167-167: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
169-169: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
169-169: Create your own exception
(TRY002)
169-169: Avoid specifying long messages outside the exception class
(TRY003)
240-240: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
246-246: Do not catch blind exception: Exception
(BLE001)
247-247: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
275-275: Redundant exception object included in logging.exception call
(TRY401)
291-291: Redundant exception object included in logging.exception call
(TRY401)
307-307: Redundant exception object included in logging.exception call
(TRY401)
325-325: Redundant exception object included in logging.exception call
(TRY401)
345-345: Consider moving this statement to an else block
(TRY300)
347-347: Redundant exception object included in logging.exception call
(TRY401)
373-373: Redundant exception object included in logging.exception call
(TRY401)
393-393: Consider moving this statement to an else block
(TRY300)
395-395: Redundant exception object included in logging.exception call
(TRY401)
422-422: Consider moving this statement to an else block
(TRY300)
424-424: Redundant exception object included in logging.exception call
(TRY401)
442-442: Consider moving this statement to an else block
(TRY300)
444-444: Redundant exception object included in logging.exception call
(TRY401)
467-467: Consider moving this statement to an else block
(TRY300)
469-469: Redundant exception object included in logging.exception call
(TRY401)
483-483: Consider moving this statement to an else block
(TRY300)
485-485: Redundant exception object included in logging.exception call
(TRY401)
500-500: Consider moving this statement to an else block
(TRY300)
502-502: Redundant exception object included in logging.exception call
(TRY401)
516-516: Consider moving this statement to an else block
(TRY300)
518-518: Redundant exception object included in logging.exception call
(TRY401)
535-535: Redundant exception object included in logging.exception call
(TRY401)
554-554: Consider moving this statement to an else block
(TRY300)
556-556: Redundant exception object included in logging.exception call
(TRY401)
578-578: Consider moving this statement to an else block
(TRY300)
580-580: Redundant exception object included in logging.exception call
(TRY401)
605-605: Redundant exception object included in logging.exception call
(TRY401)
632-632: Consider moving this statement to an else block
(TRY300)
634-634: Redundant exception object included in logging.exception call
(TRY401)
657-657: Consider moving this statement to an else block
(TRY300)
659-659: Redundant exception object included in logging.exception call
(TRY401)
690-690: Redundant exception object included in logging.exception call
(TRY401)
708-708: Consider moving this statement to an else block
(TRY300)
710-710: Redundant exception object included in logging.exception call
(TRY401)
728-728: Consider moving this statement to an else block
(TRY300)
730-730: Redundant exception object included in logging.exception call
(TRY401)
739-739: Consider moving this statement to an else block
(TRY300)
741-741: Redundant exception object included in logging.exception call
(TRY401)
750-750: Consider moving this statement to an else block
(TRY300)
752-752: Redundant exception object included in logging.exception call
(TRY401)
810-810: f-string without any placeholders
Remove extraneous f prefix
(F541)
813-813: f-string without any placeholders
Remove extraneous f prefix
(F541)
845-845: f-string without any placeholders
Remove extraneous f prefix
(F541)
848-848: f-string without any placeholders
Remove extraneous f prefix
(F541)
905-905: Redundant exception object included in logging.exception call
(TRY401)
🔇 Additional comments (3)
MCP_Server/server.py (3)
638-657: Rack chain loading looks consistent and well‑typed
load_effect_to_chaincleanly forwardstrack_index,rack_device_index,chain_index, andeffect_urito the Remote Script and returns a concise human‑readable summary. The parameter names and defaults line up with the new rack/chain tooling elsewhere in this file, so this entry point looks good.
521-557: New rack creation and master‑effect APIs look coherent
load_effect_on_mainandcreate_audio_effect_rackboth have clear docstrings, well‑typed parameters, and straightforward forwarding to their respective Remote Script commands. The usage ofdevice_indexwith a documented “-1 = append to end” convention fits well with the rest of the API surface.
560-579: Rack chain creation tool is well‑shaped and returns useful metadata
create_rack_chaincleanly exposestrack_index,device_index, and optionalchain_name, and it surfaces the returnedchain_indexin the human‑readable string. This aligns nicely with the rack/chain model and should be easy for tools to compose withload_effect_to_chain.
| # Check if this is a state-modifying command | ||
| is_modifying_command = command_type in [ | ||
| "create_midi_track", "create_audio_track", "set_track_name", | ||
| "create_clip", "add_notes_to_clip", "set_clip_name", | ||
| "create_clip", "add_notes_to_clip", "set_clip_name", "duplicate_clip", "remove_clip", "move_clip", | ||
| "set_tempo", "fire_clip", "stop_clip", "set_device_parameter", | ||
| "start_playback", "stop_playback", "load_instrument_or_effect" | ||
| "start_playback", "stop_playback", "load_instrument_or_effect", "load_effect_on_main", | ||
| "set_track_volume", "set_master_volume", | ||
| "create_audio_effect_rack", "create_rack_chain", "set_chain_name", | ||
| "load_effect_to_chain", "get_device_parameters", "set_device_param" | ||
| ] |
There was a problem hiding this comment.
Fix is_modifying_command entries for load operations
is_modifying_command lists "load_instrument_or_effect" and "load_effect_on_main", but the actual command_type values used are "load_browser_item" (in load_instrument_or_effect) and "load_effect_on_master" (in load_effect_on_main). As a result, these load operations are currently treated as non‑modifying (no extra delay, shorter timeout) despite changing Live state.
Update the list to include the actual command strings (you can keep the old ones if you plan to use them elsewhere):
is_modifying_command = command_type in [
"create_midi_track", "create_audio_track", "set_track_name",
- "create_clip", "add_notes_to_clip", "set_clip_name", "duplicate_clip", "remove_clip", "move_clip",
- "set_tempo", "fire_clip", "stop_clip", "set_device_parameter",
- "start_playback", "stop_playback", "load_instrument_or_effect", "load_effect_on_main",
+ "create_clip", "add_notes_to_clip", "set_clip_name", "duplicate_clip", "remove_clip", "move_clip",
+ "set_tempo", "fire_clip", "stop_clip", "set_device_parameter",
+ "start_playback", "stop_playback", "load_browser_item", "load_effect_on_master",
"set_track_volume", "set_master_volume",
"create_audio_effect_rack", "create_rack_chain", "set_chain_name",
"load_effect_to_chain", "get_device_parameters", "set_device_param"
]🤖 Prompt for AI Agents
In MCP_Server/server.py around lines 103 to 112, the is_modifying_command list
uses incorrect command strings for load operations; replace or augment the
entries so the actual command_type values "load_browser_item" and
"load_effect_on_master" are included (you may keep the existing
"load_instrument_or_effect" and "load_effect_on_main" if needed), ensuring those
load operations are treated as state‑modifying (so they receive the longer
delay/timeout logic).
1 participant
Feature: Rack Chain Tools & Master Track Effects
Summary
This PR introduces comprehensive tools for managing Audio Effect Racks and their chains, as well as new capabilities for the Master track. It also includes enhancements to browser navigation and device parameter control within racks.
New Tools
create_audio_effect_rack(track_index, device_index): Creates an empty Audio Effect Rack on a track.create_rack_chain(track_index, device_index, chain_name): Adds a new chain to a rack.load_effect_to_chain(track_index, rack_device_index, chain_index, effect_uri): Loads an effect directly into a specific chain.load_effect_on_main(uri): Loads an audio effect onto the Master track.load_drum_kit(track_index, rack_uri, kit_path): Loads a drum rack and a specific drum kit into it.get_browser_tree(category_type): Retrieves the hierarchical structure of browser categories.get_browser_items_at_path(path): Retrieves browser items at a specific path.Updated Tools
get_device_parameters: Updated to support accessing devices within rack chains (usingchain_indexandrack_device_index).set_device_parameter: Updated to support controlling parameters of devices within rack chains.Improvements
load_instrument_or_effectto use a shared_load_browser_itemmethod.TODO.mdfor tracking future tasks.Testing
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.