diff --git a/AGENTS.md b/AGENTS.md index d9be61dd..9e1d82a2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -43,7 +43,9 @@ Four facts do most of the work: ## Ask the tool; do not infer -Every command takes `--json`. Use it. +Every command takes `--json`, and it works before or after the subcommand -- +`plainsong --json info song.song` and `plainsong info song.song --json` are the +same command. Use it. ```bash plainsong chord "G7alt" --explain # what a symbol means, degree by degree diff --git a/CHANGELOG.md b/CHANGELOG.md index 375a41d1..fff51b8d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,59 @@ Notable changes, newest first. Dates are ISO 8601. ## Unreleased +### `--help` now shows the shape of the notation + +Every small model observed driving this CLI cold ran `--help` first. The ones +that reached `plainsong new` were fine -- it writes a correct file and they read +the format off it, and one reached for it even when told not to. The ones that +could not, because they were adding a file to an existing project, guessed. They +guessed `Title:`, which is not the notation, and got a dropped title and a +phantom section for it. + +Six lines of `--help` remove the guess. Because those lines are now a claim +about the language, they are held to the same bar as a fenced block in the +prose: `tests/test_runtime.py::TestTheHelpTeachesRealNotation` compiles the +sample, requires it to sound, and requires it to produce no diagnostics. Putting +`Title:` in the help fails two of those three. + +### Global flags now work in either position + +`plainsong info song.song --json` was refused outright -- `unrecognized +arguments: --json` -- because argparse hangs global options off the top-level +parser only. It is what almost everyone types, and it is not a small failure: +the command does not run at all. + +This was documented rather than fixed, and documenting it did not work. +`tools/verify_release.py` in this repository was written against the +documentation and reported two false failures. To find out whether that +generalises, four Haiku-class agents were pointed at the CLI cold, with the +docs withheld. One hit the flag position three times in a single session -- on +`--json`, on `-v` and on `--quiet` -- and named it first when asked what it +wished the tool had said up front. + +So the trap is removed instead of explained. All 24 subcommands now accept +`--json`, `-v`/`--verbose` and `-q`/`--quiet` themselves, via +`argparse.SUPPRESS` so the global position is not silently clobbered by a +subparser default -- which would have traded a loud failure for a quiet one. +A subcommand defining its own flag keeps it: `info --verbose` is untouched. + +### `--verbose` now shows info-level diagnostics, which nothing showed before + +An entire severity was write-only. `_diagnostics` filtered to errors and +warnings, so info-level diagnostics were produced by the parser and displayed +by no command at all -- including `info --verbose`, which documents itself as +showing every diagnostic available. + +It matters because `Title: My Song` is the natural way to name a piece and is +not the notation. The parser says so at info level, so the title was dropped +silently, the stray row silently became a section, and the only trace was +`(untitled)` and a section count one too high. Another of the cold agents hit +exactly that and had to read the parser's source to work out why. + +The default output is unchanged; only `--verbose` grows. The corpus produces no +info-level diagnostics, so nothing in the songbook gets noisier. + + ### CI was red, and one of its checks could not go red at all Four jobs were failing on master and had been since 19 August. The cause of diff --git a/CLAUDE.md b/CLAUDE.md index d0b33bb4..fa595997 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -76,11 +76,15 @@ python3 -m plainsong setup # connect a model python3 -m plainsong build # tailor this install to the machine ``` -Every command takes `--json`, but it is a **global** flag and goes before the -subcommand: `plainsong --json info song.song`. Written after it, argparse -refuses the whole invocation with `unrecognized arguments: --json`. Use it when -parsing output, and note the totals live under `arrangement` -- `arrangement.notes` -is the note count, not a top-level key. +Every command takes `--json`, `-v` and `-q`, **in either position**: +`plainsong --json info song.song` and `plainsong info song.song --json` are the +same command. The second used to be refused outright with `unrecognized +arguments: --json` -- it is what almost everyone types, it is what +`tools/verify_release.py` was written against, and documenting the constraint +did not stop anyone falling into it, so the constraint was removed instead. + +Use it when parsing output, and note the totals live under `arrangement` -- +`arrangement.notes` is the note count, not a top-level key. Run the suite with `discover`, not by naming files. Several tests are about how modules behave when imported in a particular order, and a single-file run can diff --git a/docs/integration.md b/docs/integration.md index e636867c..7f8d1b3c 100644 --- a/docs/integration.md +++ b/docs/integration.md @@ -16,9 +16,11 @@ Every command takes `--json` and prints one JSON object on stdout. plainsong --json info song.song ``` -**The flag goes before the subcommand.** `plainsong --json info song.song` -works; `plainsong info --json song.song` does not. It is a global option, and -this catches nearly everyone once. +**The flag works in either position.** `plainsong --json info song.song` and +`plainsong info song.song --json` are the same command. The second used to be +refused, which caught nearly everyone once -- including this project's own +release checker -- so it is now accepted rather than explained. The same goes +for `-v` and `-q`. | Command | Top-level keys | |---|---| @@ -173,7 +175,8 @@ a client should install it from. Collected because every one of these cost somebody time: -1. **`--json` goes before the subcommand.** +1. **`--json` works before or after the subcommand.** It did not always; + if you are on 1.4.0 or older it must come first. 2. **JSON and HTTP APIs use `content`**; the CLI takes a file path positionally. 3. **MCP tool names differ from CLI command names** (`compile_score` vs `compile`). diff --git a/plainsong/interfaces/cli.py b/plainsong/interfaces/cli.py index fb235b81..bc45e255 100644 --- a/plainsong/interfaces/cli.py +++ b/plainsong/interfaces/cli.py @@ -35,6 +35,29 @@ """ +# Every agent observed driving this CLI cold ran `--help` first, and the ones +# that could not reach `plainsong new` -- writing a file into an existing +# project rather than scaffolding one -- had to guess the shape of the notation. +# They guessed `Title:`, which is not it, and got a dropped title and a phantom +# section for it. Six lines here cost nothing and remove the guess. +NOTATION_AT_A_GLANCE = """\ +The notation, in one section: + + **TRACK: Title** <- the title. `Title:` is not it. + [MetaData] + key: Am | tempo: 96 | time: 4/4 + + [V1] (Verse - 2 Bars) <- a section + Chords: | Am . . . | F . . . | + Melody: | A4 . C5 E5 | F4 . A4 C5 | + +A bar is one bar long and its tokens divide it, so three tokens are triplets and +you never write durations. Rows of different kinds sound together. + +Start with: plainsong new, then plainsong compile --play +Flags (--json, -v, -q) work before or after the subcommand.""" + + # -------------------------------------------------------------------------- # output helpers # -------------------------------------------------------------------------- @@ -91,14 +114,32 @@ def table(self, rows: list[tuple[str, ...]], gap: str = " ") -> None: self.say(" " + line.rstrip()) -def _diagnostics(out: Out, diagnostics: list, path: str = "", limit: int = 20) -> None: - shown = [diag for diag in diagnostics if diag.severity in ("error", "warning")] +def _diagnostics( + out: Out, diagnostics: list, path: str = "", limit: int = 20, verbose: bool = False +) -> None: + """Show the diagnostics. `verbose` adds the info-level ones. + + Info-level diagnostics used to be produced and then displayed by nothing at + all -- not by `compile`, not by `check`, and not by `info --verbose`, which + documents itself as showing every diagnostic available. An entire severity + was write-only. + + That is not a cosmetic gap. `Title: My Song` is the most natural way to name + a piece and is not the notation; the parser says so at info level, so the + title was silently dropped, the stray row silently became a section, and the + only visible trace was `(untitled)` and a section count one too high. A + small model driving this cold hit exactly that. + """ + levels = ("error", "warning", "info") if verbose else ("error", "warning") + shown = [diag for diag in diagnostics if diag.severity in levels] if not shown: return out.say() + painters = {"error": out.fail, "warning": out.warn} for diag in shown[:limit]: - painter = out.fail if diag.severity == "error" else out.warn - painter(diag.format(path)) + # Info keeps its own voice: painting it "warn" would say the file has a + # problem when what it has is a remark. + painters.get(diag.severity, out.dim)(diag.format(path)) if len(shown) > limit: out.dim(f" ... and {len(shown) - limit} more") @@ -147,7 +188,13 @@ def cmd_compile(args: argparse.Namespace, config: Config, out: Out) -> int: for message in result.messages: out.warn(message) if not args.quiet: - _diagnostics(out, result.diagnostics, str(source), limit=8 if not args.verbose else 100) + _diagnostics( + out, + result.diagnostics, + str(source), + limit=8 if not args.verbose else 100, + verbose=args.verbose, + ) if args.play and result.audio_path: from ..render.backends import play_audio @@ -234,7 +281,7 @@ def cmd_info(args: argparse.Namespace, config: Config, out: Out) -> int: ) for d in summary.get("diagnostics", []) ] - _diagnostics(out, reported, str(source), limit=100 if args.verbose else 10) + _diagnostics(out, reported, str(source), limit=100 if args.verbose else 10, verbose=args.verbose) return 0 @@ -1216,7 +1263,7 @@ def build_parser() -> argparse.ArgumentParser: parser = argparse.ArgumentParser( prog="plainsong", description="Plain-text music notation that compiles to MIDI and audio.", - epilog="Start with: plainsong new, then plainsong compile --play", + epilog=NOTATION_AT_A_GLANCE, formatter_class=argparse.RawDescriptionHelpFormatter, ) parser.add_argument("--version", action="version", version=f"plainsong {__version__}") @@ -1424,9 +1471,54 @@ def build_parser() -> argparse.ArgumentParser: bridge_parser.add_argument("--text", default="", help="the reply, for `answer`") bridge_parser.set_defaults(func=cmd_bridge) + _accept_global_flags_after_the_subcommand(subparsers) return parser +# `plainsong info song.song --json` is what almost everyone types, because that +# is where flags go in nearly every other CLI. argparse hangs global options off +# the top-level parser only, so that invocation was refused outright -- +# `unrecognized arguments: --json` -- and the refusal is total: the command does +# not run, it errors. This is not hypothetical. tools/verify_release.py in this +# repository was written against it and reported two false failures, and a small +# model driving this CLI cold hit it three times in one session, on --json, on +# -v and on --quiet, then named flag position as the single thing it most wished +# the tool had told it. +# +# Documenting the constraint was the old fix and it does not work: the sentence +# "Every command takes --json" was in AGENTS.md, and following it literally is +# what produced the bug above. So the trap is removed rather than described. +# +# argparse.SUPPRESS is the load-bearing detail. Without it the subparser's own +# default overwrites whatever the global flag set, so `plainsong --json info x` +# would parse and then silently lose its --json -- trading a loud failure for a +# quiet one, which is the worse trade. +GLOBAL_FLAGS_AFTER_SUBCOMMAND = ( + (("-q", "--quiet"), "quiet", "only print what was asked for"), + (("-v", "--verbose"), "verbose", "show more detail"), + (("--json",), "json_mode", "machine-readable output"), +) + + +def _accept_global_flags_after_the_subcommand(subparsers: argparse._SubParsersAction) -> None: + """Let every subcommand take the global flags in the natural position too.""" + for sub in subparsers.choices.values(): + for options, dest, help_text in GLOBAL_FLAGS_AFTER_SUBCOMMAND: + # A subcommand that already defines one of these keeps its own -- + # `info` has its own `--verbose`, and overriding it here would be a + # behaviour change rather than an addition. + free = [o for o in options if o not in sub._option_string_actions] + if not free: + continue + sub.add_argument( + *free, + dest=dest, + action="store_true", + default=argparse.SUPPRESS, + help=help_text, + ) + + def main(argv: list[str] | None = None) -> int: parser = build_parser() args = parser.parse_args(argv) diff --git a/tests/test_runtime.py b/tests/test_runtime.py index 53757c10..096f1827 100644 --- a/tests/test_runtime.py +++ b/tests/test_runtime.py @@ -5,6 +5,7 @@ import io import json import os +import re import tempfile import unittest from contextlib import redirect_stderr, redirect_stdout @@ -40,6 +41,125 @@ def run_cli(*argv: str) -> tuple[int, str, str]: return code, out.getvalue(), err.getvalue() +class TestTheHelpTeachesRealNotation(unittest.TestCase): + """`--help` now shows the shape of the notation, so it has to be true. + + Every small model observed driving this CLI cold ran `--help` first. The + ones that could not reach `plainsong new` guessed the format, guessed + `Title:`, and got a dropped title and a phantom section. The sample closes + that guess -- which makes it a promise, held to the same bar as a fenced + block in the prose: it must compile, and it must make a sound. + """ + + def _sample(self) -> str: + from plainsong.interfaces.cli import NOTATION_AT_A_GLANCE + + lines = [ + re.sub(r"\s{2,}<- .*$", "", line[2:]) + for line in NOTATION_AT_A_GLANCE.splitlines() + if line.startswith(" ") + ] + return "\n".join(lines) + "\n" + + def test_it_compiles_and_sounds(self) -> None: + from plainsong.notation import arrange, parse + + arrangement = arrange(parse(self._sample())) + self.assertGreater(arrangement.note_count, 0, "the help sample is silent") + + def test_it_produces_no_diagnostics(self) -> None: + """A sample that warns is teaching the warning.""" + from plainsong.notation import arrange, parse + + arrangement = arrange(parse(self._sample())) + self.assertEqual([d.format() for d in arrangement.diagnostics], []) + + def test_it_names_the_title_row_the_parser_actually_wants(self) -> None: + from plainsong.notation import parse + + self.assertEqual(parse(self._sample()).meta.title, "Title") + + +class TestGlobalFlagsWorkInBothPositions(unittest.TestCase): + """`plainsong info x --json` is what people type, so it has to work. + + argparse hangs global options off the top-level parser only, so the natural + position was refused outright -- and refused loudly enough to abort the + command. This repository's own tools/verify_release.py was written against + that and reported two false failures; a small model driving the CLI cold hit + it three times in one session, on --json, -v and --quiet. + """ + + SONG = "Key: Am\nTempo: 96\n\n[A]\nChords: | Am . . . |\n" + + def _song(self) -> str: + directory = tempfile.mkdtemp() + path = Path(directory) / "x.song" + path.write_text(self.SONG, encoding="utf-8") + return str(path) + + def test_json_after_the_subcommand_is_accepted(self) -> None: + code, out, _err = run_cli("info", self._song(), "--json") + self.assertEqual(code, 0) + json.loads(out) # it is real JSON, not the human summary + + def test_both_positions_agree(self) -> None: + song = self._song() + _c1, before, _e = run_cli("--json", "info", song) + _c2, after, _e = run_cli("info", song, "--json") + self.assertEqual(json.loads(before), json.loads(after)) + + def test_the_global_position_still_wins_when_the_flag_is_not_repeated(self) -> None: + """argparse.SUPPRESS is what stops the subparser default clobbering it.""" + code, out, _err = run_cli("--json", "info", self._song()) + self.assertEqual(code, 0) + self.assertEqual(json.loads(out)["key"], "Am") + + def test_every_subcommand_takes_them(self) -> None: + from plainsong.interfaces.cli import build_parser + + subparsers = [ + action + for action in build_parser()._actions + if isinstance(getattr(action, "choices", None), dict) + ][0] + for name, sub in subparsers.choices.items(): + with self.subTest(command=name): + options = sub._option_string_actions + self.assertIn("--json", options) + self.assertIn("--verbose", options) + self.assertIn("--quiet", options) + + +class TestVerboseShowsInfoDiagnostics(unittest.TestCase): + """Info-level diagnostics were produced and displayed by nothing at all. + + `Title: My Song` is the natural way to name a piece and is not the notation. + The parser says so at info level, so the title was dropped, the row became a + section, and the only visible trace was `(untitled)` and a section count one + too high. + """ + + TITLED = "Title: Midnight Echo\nKey: Dm\nTempo: 96\n\n[A]\nChords: | Dm . . . |\n" + + def _song(self) -> str: + directory = tempfile.mkdtemp() + path = Path(directory) / "titled.song" + path.write_text(self.TITLED, encoding="utf-8") + return str(path) + + def test_verbose_explains_why_the_title_vanished(self) -> None: + code, out, _err = run_cli("info", self._song(), "--verbose") + self.assertEqual(code, 0) + self.assertIn("Title", out) + self.assertIn("unrecognised row label", out) + + def test_the_default_stays_quiet(self) -> None: + """Only --verbose changes; a clean run must not grow new chatter.""" + _code, out, _err = run_cli("info", self._song()) + self.assertNotIn("unrecognised row label", out) + + class TestPaths(unittest.TestCase): def test_environment_overrides(self): with tempfile.TemporaryDirectory() as directory: