Consolidate command definitions into a single descriptor table (phases 0/1/2/4) - #117
Merged
Conversation
Phase 0 of the command-definition consolidation refactor: build the safety net before moving any production code. No production code changes here. The existing public contract snapshot captures what each command *does* (args/result/errors/side_effect) but not how it is spelled on the command line. A refactor that renamed --track-index to --track_index, swapped two positional arguments, or changed an option default would leave it green. Two new nets close that gap: - tests/snapshots/command_surface_snapshot.json records, for all 250 commands and groups in the real app, every parameter's option strings, type, default, arity and help, plus the rendered --help. Rich's width, color system and terminal detection are pinned during capture so the output does not depend on COLUMNS or TERM. - tests/test_command_set_hash_stability.py pins the command-set hash to a literal. That value is shared with Remote Scripts already installed in users' Ableton Live; changing it forces every user to reinstall and restart Live, so it must not move under a pure refactor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Phase 1 of the command-definition consolidation refactor. public_command_names() collected command names by regex-scanning commands/**/*.py for command_name="..." literals. That scan could only ever fail quietly: a command spelled in a way the pattern did not match simply vanished from the set, and with it from the side-effect exhaustiveness check and the contract registry. The comment above _SIDE_EFFECTS admitted as much and named per-command descriptors as the intended replacement. src/ableton_cli/command_registry.py is that replacement: one row per command carrying its name, its remote command, and its side effect. The rows were transcribed mechanically from the current output of _remote_command_name() and _side_effect_spec(), not by hand, so every regex branch and exception-table entry is now spelled out literally. Deleted in exchange: the two regex patterns and the directory scan, _LOCAL_ONLY_COMMANDS, _REMOTE_COMMAND_EXCEPTIONS, _SIDE_EFFECTS, and the synth/effect/master-effect name-matching branches. command_specs.py goes from 658 lines to 152 and no longer imports re or pathlib. tests/test_command_registry_matches_cli.py is what makes the table safe: it walks the real Typer app and asserts the command set matches the table in both directions, reporting the exact difference. Missing a row is now a test failure rather than silence. test_layering.py extends its no-import-of-commands check to the new module, which inherits the constraint. The command set is unchanged: 250 commands, 161 remote commands, and the same command_set_hash, so installed Remote Scripts keep working. Both snapshots and the generated skill docs are byte-identical. Findings noticed but deliberately not fixed here are in docs/refactor-findings.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Phase 2 of the command-definition consolidation refactor.
123 of the client's 159 methods were a single `return self._call("name",
{literal dict})`. They carried no logic — only the opportunity to typo a
remote command name or a payload key, neither of which any test could catch
until the call reached Live.
CLIENT_METHOD_SPECS in command_registry.py now describes them, and
tools/generate_client_methods.py renders src/ableton_cli/client/
_client_generated.py from it. The generated file is checked in and readable:
ordinary annotated methods an IDE can complete and a debugger can step
through, not runtime dispatch. CI regenerates it and fails on any diff,
the same contract generate_skill_docs.py already uses.
The specs were transcribed from the existing methods by AST, and the
classification is mechanical: a method is generated only if its entire body
is one self._call with a literal command name and a payload dict whose
values are all bare parameter names. The 36 methods that assemble
arguments, branch, or post-process — the clip-note builders, the browser
loaders, the parameter-set-safe family — are untouched and stay
hand-written.
_client_song_transport.py is deleted rather than left as an empty class:
all 20 of its methods were pass-throughs.
tests/test_ableton_client.py is unchanged and passes, which is the point:
every generated signature is identical to the one it replaced. Three new
tests guard the arrangement — the generated file matches the generator,
every spec names a declared remote command, and no mixin still defines a
generated method where the MRO would silently shadow it.
The command set and command_set_hash are unchanged, and both snapshots and
the generated skill docs are byte-identical.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Phase 4 of the command-definition consolidation refactor. Tests only; no Remote Script or CLI production code changes. A CLI command pointing at a handler that does not exist currently ships green: the only check is `doctor`/`ping` comparing command_set_hash against the Remote Script already installed on a user's machine. The failure lands on the user, after a reinstall, as a protocol mismatch. tests/test_command_registry_remote_parity.py moves that to the build, and reports the difference in both directions rather than a bare set inequality, so the message says which side to edit. It also covers the CommandBackend Protocol: every backend method a handler calls must be declared, and every declared method must be called by something. Both hold exactly today (159 methods, all reachable). The Remote Script cannot import src/, so its eight handler tables are necessarily a separate copy. Merging them is not the fix; asserting parity is. Follows the CLI/Remote split already used by test_note_field_specs.py. test_remote_handler_registry_matches_command_specs is removed as superseded by the first of the new tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both new tests passed on macOS and failed on Windows CI. Neither failure was a real defect in the refactor; both were defects in the tests I added. rich substitutes a square panel box for the rounded one whenever it thinks it is on a legacy Windows console, so the captured --help text used ╭─ on macOS and ┌─ on Windows and the snapshot could never match on both. Typer builds its Console without passing legacy_windows, so pinning the detector during capture is the only injection point -- the same treatment MAX_WIDTH, FORCE_TERMINAL and COLOR_SYSTEM already get for the same reason. The generated-client check piped source to `ruff format -` with text=True, which encodes stdin using the locale codec. That is cp1252 on Windows CI, and the generated module's docstring contains an em dash, so ruff rejected the stream as invalid UTF-8. Piping explicit UTF-8 bytes removes the locale from the path entirely. Verified locally by reproducing both: the snapshot is now byte-identical with rich's legacy-windows detection forced on, and encoding the module as cp1252 reproduces the exact CI error while the utf-8 byte path succeeds. The checked-in snapshot is unchanged, since macOS already rendered the rounded box. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Last Windows CI failure. write_text translates to os.linesep, so running the updater on Windows rewrote all 37,500 line endings to CRLF, and the test that reruns the tool and compares bytes caught it. .gitattributes pins the repo to eol=lf, so LF is what belongs on disk everywhere; newline="\n" makes the tool produce that regardless of host. The same latent inconsistency exists in the three pre-existing generator tools. It is harmless there because git normalises it away before `git diff --exit-code` sees it, and fixing it is out of scope for this refactor, so it is recorded in docs/refactor-findings.md instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Consolidates command definitions behind a single descriptor table. Phases 0, 1, 2 and 4 of the refactor plan, as four independently revertable commits.
This is a pure internal restructuring. The public surface does not move by a single byte — both snapshots and the generated skill docs are byte-identical, and
command_set_hashis unchanged, so Remote Scripts already installed in users' Ableton Live keep working.Why
Adding one command meant editing 6–7 places. Worse,
public_command_names()collected command names by regex-scanningcommands/**/*.pyforcommand_name="..."literals. That scan could only ever fail quietly: a command spelled in a way the pattern did not match simply vanished from the set, and with it from the side-effect exhaustiveness check and the contract registry. The comment above_SIDE_EFFECTSadmitted as much and named per-command descriptors as the intended replacement.This follows the pattern
src/ableton_cli/track_facets.pyalready established, rather than inventing a new one.What changed
Phase 0 — characterization tests (no production code).
The existing public contract snapshot captures what each command does, but not how it is spelled on the command line. A refactor renaming
--track-indexto--track_indexor reordering two positional arguments would leave it green.tests/snapshots/command_surface_snapshot.jsonrecords every parameter's option strings, type, default, arity and help for all 250 commands, plus rendered--help. Rich's width, color system and terminal detection are pinned during capture so output does not depend onCOLUMNSorTERM.command_set_hashis pinned to a literal with a comment explaining that moving it forces every user to reinstall and restart Live.Phase 1 — the descriptor table.
src/ableton_cli/command_registry.pyholds one row per command. Rows were transcribed mechanically, by script, from the current output of_remote_command_name()and_side_effect_spec()— not by hand — so every regex branch and exception-table entry is now spelled out literally. Deleted: the two regex patterns, the directory scan,_LOCAL_ONLY_COMMANDS,_REMOTE_COMMAND_EXCEPTIONS,_SIDE_EFFECTS, and the synth/effect name-matching branches.command_specs.pygoes 658 → 152 lines and no longer importsreorpathlib.tests/test_command_registry_matches_cli.pyis what makes the table safe: it walks the real Typer app and asserts the command set matches in both directions, reporting the exact difference. Missing a row is now a test failure rather than silence.Phase 2 — generated client methods.
123 of 159 client methods were a single
return self._call("name", {literal dict}), carrying no logic — only the opportunity to typo a remote command name or payload key, neither catchable by any unit test.CLIENT_METHOD_SPECSdescribes them andtools/generate_client_methods.pyrendersclient/_client_generated.py. The generated file is checked in and readable: ordinary annotated methods an IDE can complete, not runtime dispatch.Classification is mechanical: generated only if the entire body is one
self._callwith a literal command name and a payload dict whose values are all bare parameter names. The 36 methods that assemble arguments, branch, or post-process are untouched._client_song_transport.pyis deleted rather than left as an empty class — all 20 of its methods were pass-throughs.tests/test_ableton_client.pyis unchanged and passes, which is the point: every generated signature is identical to the one it replaced.Phase 4 — build-time CLI/Remote parity.
A CLI command pointing at a nonexistent handler currently ships green; the only check is
doctor/pingagainst the Remote Script already installed on a user's machine.tests/test_command_registry_remote_parity.pymoves that to the build and reports the difference in both directions. It also covers theCommandBackendProtocol: every backend method a handler calls must be declared, and every declared method must be called. Both hold exactly today (159 methods).The Remote Script cannot import
src/, so its eight handler tables are necessarily a separate copy. Merging them is not the fix; asserting parity is.Verification
Unit: 1075 passed, ruff clean. All four generated artifacts regenerate byte-identical.
Verified against real Ableton Live 12 after reinstalling the Remote Script:
doctor8/8 PASS;command_set_hashreported by the live Remote Script matches the constant pinned in Phase 0, 161/161 commands, zero drift either direction.All 123 generated methods driven against Live with a scaffold (MIDI track + Drift + clip with notes). Classifying failures by whether the payload reached the handler:
Zero WIRE failures — no "unknown command", "missing argument" or "unexpected key" anywhere. Every DOMAIN failure is the Remote Script parsing the payload and rejecting the value (MIDI clip not audio, Drift is not a Rack, no arrangement clips, cue routing unsupported by this Live version), which confirms names and payload keys are correct. SKIP are global/destructive commands deliberately excluded from an unattended sweep.
Reviewer notes
docs/refactor-findings.mdrather than fixed, since any fix would move the public surface. Notably, the quality-harness baseline is already stale onmain(1 failure, 419 warnings vs a recorded 0/395) — verified identical with and without these changes.CommandDescriptor, because CLI-command → client-method is many-to-one (all fourmaster effect <type> keyscommands call onemaster_effect_keys). Hanging it off the descriptor would duplicate the parameter list per sharing command and let the copies drift.command_surface_snapshot.jsonis large (1.7MB); ~54% is rendered--help, which couples it to rich's output. Keeping it was a deliberate call.🤖 Generated with Claude Code