From e82c1689b31ae4941472068ebcd37b12bd4cecf0 Mon Sep 17 00:00:00 2001 From: aiongg <67543387+aiongg@users.noreply.github.com> Date: Tue, 27 Oct 2026 21:09:43 -0400 Subject: [PATCH] refactor(cli): parse `soundcraft-cli` arguments with clap, following RFC-0012 (#1136) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(cli): pin today's argument handling Run the real binary and record how the hand-written parser behaves for every subcommand: help and version spellings, no arguments, unknown subcommands and flags, value flags in both spellings, missing and repeated values, positional order, exit codes and which stream carries each message. No behaviour change. Later parser changes update these assertions on purpose. * refactor(cli): split main.rs into modules Move only: args, error, output, convert, info, session, catalog, app and mcp each get their functions and unit tests; main.rs keeps the usage doc, help_text and the dispatch. Moved items become pub(crate) where other modules call them. No behaviour change. * refactor(cli): typed errors and run(args, out, err) soundcraft-cli is now a library plus a thin binary. `soundcraft_cli::run(args, out, err) -> i32` is the whole program; main.rs only passes the real streams and shuts the plugin hosts down afterwards (process teardown, not part of run, which tests call repeatedly in one process). Subcommands write to the given writers, return `Result<(), CliError>`, and `fail` is gone. CliError wraps engine, audio-io, io (with the path), serde_json and the control channel errors; UsageError, ConvertError and RootFlagError cover the subcommands' own failures. No Result<_, String> is left except where a dependency reports a string (control-channel call, AutomationRoots::new), wrapped in a variant. The tests that need no files, working directory, environment or stdin now call `run` in-process (tests/it/run.rs); the rest still run the binary. Behaviour changes: - Error messages start with `error: ` instead of `soundcraft-cli: `. - A script line that stops the run now prints `error: FILE:N: ID: ...` (it had no prefix). With --keep-going the line stays bare. - A convert encode or write failure now names the output path. - An empty or help-less call still prints the one-line usage to stderr without a prefix and exits 1; `files confined:` keeps its `soundcraft-cli:` prefix (it is a note, not an error). * refactor(cli): parse arguments with clap Replace the hand parser (arg_value, check_options, ENGINE_OPTIONS, the lib.rs dispatch, the per-subcommand positional handling and the root flags' consume) with clap derive types in args.rs: a Parser, a Subcommand enum and one Args struct per subcommand, with the run/script engine flags flattened. main passes args_os(); run takes OsString-like arguments and renders clap's help, version and usage errors (help and version on stdout, exit 0; usage errors on stderr, exit 2). Add command() returning the clap definition. The usage block in the module doc and help_text are gone: the doc comments are the help. Behaviour changes: - Usage errors exit 2 with clap's message and the --help hint on stderr, instead of exit 1 with hand-written text: unknown subcommand, unknown flag, missing value, missing or extra positional, a value on a boolean flag (--demo=1, --normalize=1). - No arguments prints the help to stderr, exit 2 (was a one-line usage, exit 1). - --help, -h and help print clap's generated help, and every subcommand has its own (SUB --help, SUB -h, help SUB). Before, info and script read --help as a file name, describe as an id, commands as a filter, run rejected --help and ignored -h, convert and app printed their usage error, mcp, parity and plugins ignored it, and app --port P -h called the method -h. - --help or -h before -- shows the subcommand's help even after a mistake on the same line (run --bounce --help). - --version, -V and version print "soundcraft-cli X.Y.Z" (was "SoundCraft CLI X.Y.Z"). - Unknown flags on info, commands, describe, app, mcp, parity and plugins are usage errors; they were ignored or read as a positional. mcp --automation-bogus exits 2 (was exit 1). - Extra positionals are usage errors on every subcommand (were ignored). - A single-dash word (-x) is a flag, so an unknown one is a usage error; a value starting with - is written --flag=-value. - Flags may come before or between the positionals: convert IN --rate 44100 OUT, convert --rate 44100 IN OUT and script --keep-going FILE parse (the flag word was taken as a file name). - A repeated single-value flag keeps its last value (was the first): --rate, --bit-depth, --save, --port and the others. - parity --write without a value is a usage error (printed the report). - mcp --connect without a value, or followed by a flag, is a usage error (was exit 1, or took the flag as the port). - mcp --demo=1 is a usage error (was ignored, without the demo). - A root flag without a folder, with an empty one, or followed by a flag is a usage error with clap's message (was exit 1). - A non-Unicode argument is a usage error where text is expected and accepted as a root folder (std::env::args panicked). - The help lists --sample as --demo's alias. Unchanged: --dither with --no-dither, bad --rate and --bit-depth values, the roots with --connect and the SOUNDCRAFT_CONTROL_PORT fallback are still checked after parsing (exit 1). * refactor(cli): reject repeated flags, clap --version (photocraft#7071 decisions) Behaviour changes: - A flag given twice is a usage error (exit 2), in either spelling, including an alias pair such as --demo and --sample. Before, the last one won. Only run's --cmd and mcp's --automation-read-root / --automation-write-root repeat; they accumulate in order. - --version and -V print the long version, "soundcraft-cli 0.13.1 (linux x86_64)": the version and the platform the binary was built for. - The hidden `version` subcommand is gone; `soundcraft-cli version` is an unrecognized-subcommand usage error. - --start and --end accept a negative number (`--start -1`, as the engine counts positions before 0); any other value starting with `-` needs `=` and otherwise is an unexpected-argument usage error. No flag has allow_hyphen_values. - `help ` is unchanged (tested). * refactor(cli): typed values and flag relations Handlers receive typed values: convert --bit-depth is a ValueEnum, --rate a NonZeroU32, file arguments PathBuf, --start/--end a TimeArg, --cmd a parsed (id, JSON) spec, app's JSON a serde_json::Value, app --port and mcp --connect a validated port or host:port. Relations (--dither/--no-dither, --in/--demo, --start/--end needing --bounce) are declared on the flags. describe ID, commands FILTER and app METHOD refuse an empty string. parse_rate, parse_bit_depth, parse_dither, parse_cmd_spec, time_arg and the Rate, BitDepth, DitherConflict and CmdJson errors are gone. Behaviour changes: - --dither with --no-dither is a usage error (exit 2) instead of exit 1. - --rate 0 / non-numeric and an unknown --bit-depth are usage errors (exit 2) with clap's wording, naming the flag. --bit-depth help lists the values. - --in with --demo (or --sample) is a usage error; --in used to win silently. - --start/--end without --bounce are a usage error; they used to be ignored. - Malformed JSON in --cmd ID=JSON and in app's JSON argument is a usage error (exit 2) at parse time. Before, run opened the engine and executed every earlier --cmd (printing their results) before the bad one failed with exit 1. - An empty --start/--end, an empty command id (--cmd =1), and an empty describe ID, commands FILTER or app METHOD are usage errors. An empty commands FILTER used to list everything. - app --port and mcp --connect accept only a port (0-65535) or host:port; junk used to be accepted and failed at connect time (exit 1). - Non-Unicode file names are accepted wherever the callee takes a Path (info audio files, convert, script, parity --write). Session and bounce/save paths go to the engine as text, so a non-Unicode one fails with exit 1, "the path is not valid Unicode", after any earlier --cmd ran. * refactor(cli): shared launch flags and SOUNDCRAFT_CONTROL_PORT as clap env soundcraft_automation::args holds the launch flags as clap Args structs: ControlClientArgs (`app --port`, env SOUNDCRAFT_CONTROL_PORT, default 7979), ControlServerArgs (the desktop `--control`, same variable, wired in a later step; its ControlPort value is a port, also from a loopback host:port, or another host's host:port, which the app decides on, so one export serves both), AutomationArgs (the two root flags, repeatable, flags only), the ControlTarget value parser, and the blank_env_is_unset, name_env_vars and conflict helpers. The module and its clap/thiserror dependencies are left out of the wasm build. `app` and `mcp` flatten them; the hand fallback to the variable in `app` is gone. Behaviour changes: - An empty or blank SOUNDCRAFT_CONTROL_PORT counts as unset, so `app` uses 7979; before, it tried to connect to `127.0.0.1:` (exit 1). - A bad SOUNDCRAFT_CONTROL_PORT is a usage error naming the variable (exit 2); before, the connection failed (exit 1). - Spaces around the `app --port` or `mcp --connect` value, or the variable's, are ignored; before, the connection failed (exit 1). - An IPv6 host in `app --port`, `mcp --connect` or the variable goes in brackets (`[::1]:7979`); `::1:7979` is a usage error (exit 2), as the host and port can't be told apart; before, the connection failed (exit 1). - `app --help` shows `[env: SOUNDCRAFT_CONTROL_PORT=]` and `[default: 7979]` instead of the hand-written default note. - `mcp` with a root flag and --connect is clap's conflict error (exit 2, "the argument '...' cannot be used with '...'"), reported before any root folder is opened; before, a custom message with exit 1, after the roots were opened. * refactor(app): move launch flag parsing into launch_args The parsing moves out of main unchanged, into a LaunchArgs struct and a parse function that takes SOUNDCRAFT_CONTROL_PORT as a parameter. Tests pin the flags, the file list, ignored unknown flags and the fallback of a bad --control to the variable. No behaviour change. * refactor(app): parse launch flags with clap soundcraft --help now comes from the LaunchArgs doc comments and ends with the environment variables the app reads, and the shared ControlServerArgs supplies --control and SOUNDCRAFT_CONTROL_PORT. Process tests cover the variable (bad value, blank value, loopback host:port, another host). Behaviour changes: - An unknown flag (--bogus, -x) is a usage error on stderr, exit 2. It was ignored, and a single-dash word was opened as a file; after `--` it is a file. - A bad --control (out of range, not a number, missing value) is a usage error, exit 2. It was ignored and the variable used instead. - A bad SOUNDCRAFT_CONTROL_PORT is a usage error naming the variable, exit 2. It was ignored. An empty or blank variable still counts as unset. - A loopback host:port (127.0.0.1, localhost or [::1]) in --control or SOUNDCRAFT_CONTROL_PORT gives its port, so the value `soundcraft-cli app` takes from the variable works for both. The control server listens on 127.0.0.1 only, so another host in --control is a usage error. In the variable, which may point `soundcraft-cli app` at another machine, it is ignored with a log warning and the app opens without a control server. Both were ignored before. - A repeated flag is a usage error (--demo twice, --control twice). - --control=PORT and a port with surrounding spaces are accepted; a value that starts with `-` is not taken as the value, except a negative number, which is a bad port (`--control -1`). - --help ends with an Environment section listing SOUNDCRAFT_CONTROL_PORT, SOUNDCRAFT_CONFIG_DIR, SOUNDCRAFT_NO_PREFS, SOUNDCRAFT_NO_SYSTEM_FONTS, SOUNDCRAFT_WAYLAND (Linux) and RUST_LOG. - --help and -h print the help, exit 0. They were ignored. - --version and -V print `soundcraft 0.13.1 (linux x86_64)`, as soundcraft-cli does. It printed `SoundCraft 0.13.1`. - -V and --version are read in order with the other arguments: an earlier error comes first. - macOS -psn_… process serial numbers are dropped, as before they were opened as files. - A file whose name is not valid Unicode is skipped with a log warning. It used to panic in std::env::args. * feat(cli): shell completions and man pages An example, `cargo run -p soundcraft-cli --example man-and-completions -- `, writes bash, zsh, fish, elvish and PowerShell completions and soundcraft-cli.1 plus a page per subcommand, using clap_complete and clap_mangen (dev-dependencies of soundcraft-cli only, not in the binary). The Linux package (deb, rpm, tar.gz, AppImage) installs the pages in /usr/share/man/man1 and the bash, zsh and fish completions in their standard directories; the deb also links the zsh completion into Debian's vendor-completions. A cross build runs the example through $EMULATOR and leaves the files out without one. The FreeBSD tarball carries them in the ports layout; a cross build builds the example for the host as well. No change to the CLI's behaviour. A test pins the files the example writes. * ci(cli): check the command-line code with ast-grep rules Adds lint/ast-grep (rules for clap errors built from strings, String as an error type, and std::env::args) and a workflow that runs the rule tests and a scan over the CLI code through the shared craft-common-actions workflow. No behaviour change; the scan has no hits. * docs(cli): document soundcraft-cli and RFC-0012 Add docs/cli.md: the subcommands, exit codes, environment variables, the desktop app's launch flags, man pages and completions, and the RFC-0012 rules not followed yet (convert can overwrite its input; no --compact/--json). Link it from the README and the MCP and control-protocol guides. No code change. * feat(cli): refuse an output that replaces an input An output that is one of the subcommand's inputs is a usage error (exit 2, `error: …` on stderr, clap's usage line), reported while parsing, before the engine loads or any --cmd runs. Paths are compared by file identity (device and inode, through same-file), or, when neither exists yet, by where they would be created, so `a.wav`, `./a.wav`, an absolute path and a symbolic or hard link all count as the same file. Inputs and outputs per subcommand: - convert: input IN, output OUT. --in-place lets OUT replace IN. - run: input --in, outputs --bounce and --save. --in-place lets --save replace the --in session; it requires --in. A --bounce naming the --in session is always refused. - script: inputs FILE and --in, outputs --bounce and --save. --in-place lets --save replace the --in session; a --bounce naming it is always refused, and neither may ever replace FILE. - run and script: --bounce and --save naming one file is refused with or without --in-place (the save would replace the bounce). - info, commands, describe, app, mcp, plugins write no file; parity --write has no input. Paths inside --cmd parameters are not checked. With --in-place, an output that is a symbolic link to the input is written to the file the link names, so convert and --save both replace the input and leave the link in place (a save renames a new file over its path, which would otherwise replace the link and leave the input unchanged). Behaviour changes: - `convert a.wav a.wav`, `run --in s.scraft --save s.scraft`, `run --in s.scraft --bounce s.scraft` and the other cases above exit 2 instead of replacing the input. - New flag --in-place on convert, run and script. - An existing file that is not an input is still replaced without asking. * feat(cli): --compact for JSON output RFC-0012 rule 1.3: JSON is pretty-printed by default and --compact prints one line. Every pretty-printed JSON output now goes through output::print_json. Changed outputs: - info