From 4f06c004026d770da4e1d76b572c78d4d76a1fe9 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Fri, 3 Jul 2026 06:34:40 +0000 Subject: [PATCH 1/8] docs: fix stale/incorrect agent instructions; comment surprising code - AGENTS.md: quickstart, mx::api presence/absence conventions (std::optional for new fields, unspecified enumerators for enums incl. Bool), accurate 'make test' scope, soften anonymous-namespace rule to match reality, condense Docker/audit/corert sections, fix typos - gen/AGENTS.md: fix nonexistent 'make test-cpp' target, unify commit policy - gen/README.md: point at gen/AGENTS.md instead of duplicating commands - README.md: fix non-compiling createFromScore example (Result.value()), ARGS filter example, 'make dev' description - audit/data READMEs, Makefile, CI: fix skill-name references, add classify.py to layout table, fix garbled sentences, drop stale 829 pin count comment - code: add TODOs for 5 suspected bugs (arranger/publisher clobber lyricist, time-mod normal-type && condition, filename off-by-one, silent normal-type drop for tuplets without marks, ambiguous __ path flattening) and 9 clarifying comments at verified confusing spots --- .../resources/pr-template.md | 15 ---- .../SKILL.md | 0 .github/workflows/ci.yaml | 2 +- AGENTS.md | 74 ++++++++++--------- Makefile | 2 +- README.md | 14 ++-- audit/README.md | 3 +- audit/__init__.py | 2 +- audit/classify.py | 3 + audit/featuresfile.py | 2 +- data/README.md | 9 +-- docs/ai/design/mx-core-plan.md | 3 +- gen/AGENTS.md | 9 ++- gen/README.md | 13 +--- src/include/mx/api/ApiCommon.h | 2 + src/private/mx/api/DocumentManager.cpp | 2 + src/private/mx/impl/NoteFunctions.cpp | 5 ++ src/private/mx/impl/NoteReader.cpp | 8 ++ src/private/mx/impl/NoteWriter.cpp | 8 ++ src/private/mx/impl/ScoreReader.cpp | 8 ++ src/private/mx/impl/ScoreWriter.cpp | 3 + src/private/mxtest/file/MxFileRepositoy.cpp | 3 + 22 files changed, 109 insertions(+), 81 deletions(-) delete mode 100644 .claude/skills/mx-api-add-feature/resources/pr-template.md rename .claude/skills/{mx-api-classify-rountrip-failures => mx-api-classify-roundtrip-failures}/SKILL.md (100%) diff --git a/.claude/skills/mx-api-add-feature/resources/pr-template.md b/.claude/skills/mx-api-add-feature/resources/pr-template.md deleted file mode 100644 index 3eea91626..000000000 --- a/.claude/skills/mx-api-add-feature/resources/pr-template.md +++ /dev/null @@ -1,15 +0,0 @@ ---- -pr_title: "lowercase only, for example, feat: add segno to mx::api" ---- -## Summary - -Add support for `` elements to the `mx::api` layer. - -New types added: -- `Segno` -- `WhateverEnum` - -## Testing - -- Enabled `mx::api` round trip tests for `file-abc-1.xml` and `file-abc-2.xml` -- Added `unit_test_123` diff --git a/.claude/skills/mx-api-classify-rountrip-failures/SKILL.md b/.claude/skills/mx-api-classify-roundtrip-failures/SKILL.md similarity index 100% rename from .claude/skills/mx-api-classify-rountrip-failures/SKILL.md rename to .claude/skills/mx-api-classify-roundtrip-failures/SKILL.md diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index a099245d8..270ba4096 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -179,7 +179,7 @@ jobs: key: ccache-${{ runner.os }}-core-${{ github.sha }} restore-keys: ccache-${{ runner.os }}-core- - # corert is pinned at 829/0/0 by an in-suite counts case. + # corert file counts are pinned by an in-suite counts case (see CoreRoundtripTest.cpp). - name: Core roundtrip suite run: make test-core-dev diff --git a/AGENTS.md b/AGENTS.md index 50482e1cf..2873f97d1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -5,6 +5,9 @@ `mx` is a MusicXML C++ library. The product surface is `mx::api`, a simplified and narrowed interface of MusicXML backed by the strongly-typed `mx::core` model. +Quickstart: `make test-core-dev` (fast corert gate) - `make fmt` (format) - `make help` (all +targets). Everything runs in the `mx-sdk` Docker image unless `MX_RUNNING_IN_DOCKER=1`. + ## Repository layout ``` @@ -13,6 +16,7 @@ mx/ Makefile <- top-level build driver Dockerfile <- mx-sdk image with toolchains and dev tools CMakeLists.txt <- C++ project + Package.swift <- Swift Package Manager wrapper (built by the swift-local CI job) data/ <- MusicXML test corpus (large, see data/README.md) docs/ai/design/ <- design docs docs/musicxml-4.0-ed15c23.xsd <- the current musicxml.xsd schema @@ -37,6 +41,8 @@ mx/ mxtest/control/ <- compile-control flags (CompileControl.h: enables/disables test suite compilation) cpul/ <- vendored Catch2 test runner gen/ <- code generator system (see gen/README.md) + cpp/ <- C++ target (the product): config.toml + templates + schema/ <- JSON Schema target test/go/ <- A toy Go implementation of MusicXML XSD for gen validation test/c/ <- A toy C implementation of MusicXML XSD for gen validation audit/ <- MusicXML feature-audit tool (see audit/README.md); `make audit` @@ -44,46 +50,28 @@ mx/ ## Feature audit (`audit/`) -`python3 -m audit` (run via `make audit`) inventories which MusicXML features the corpus uses, so we -can compare against what `mx::api` exposes. It writes a `*.features.xml` sidecar next to each corpus -file and a `data/corpus.xml` aggregate (all checked in; the round-trip suites skip them). The -`api-feature-audit` skill uses these to find enum bugs and feature gaps in `mx::api`. See -`audit/README.md` and `data/README.md`. +`make audit` inventories corpus feature usage (`*.features.xml` sidecars + `data/corpus.xml`, +checked in; the test suites skip them). See `audit/README.md`; the `mx-api-feature-audit` skill +consumes the output. ## Build system -### Docker (mx-sdk) - -Docker is used for tool reproducibility. The `Makefile` has sections that are invoked on the host -and others that are invoked inside the container. The `MX_RUNNING_IN_DOCKER` env var drives this -distinction. - -In general, we should strive to make build processes reproducable on developer machines and in CI by -leveraging the `mx-sdk` image and extending it with new tools as they are needed. - -### Makefile targets - -The `Makefile` serves as the entrypoint of build processes. It calls `cmake` which produces deeper, -generated makefiles in the build directory. You should lean heavily on our top-level `Makefile` and -suggest improvements when it doesn't have what you need. - -Run `make help` for the target list. +The `Makefile` is the entrypoint for all build processes; it drives `cmake` and runs everything +inside the `mx-sdk` Docker image unless `MX_RUNNING_IN_DOCKER=1` (see the README's Build section). +Lean on it and suggest improvements when it doesn't have what you need. Run `make help` for the +target list. Extend the `mx-sdk` image when new tools are needed. ## The corert (core roundtrip) test -Runs with `make test-core-dev`. - -This test suite deserializes and reserializes the test corupus with `mx::core`. It can be compiled -without compiling the `mx::impl` and `mx::api` layes. This provides a mechanism for innovating on -the generated code and templates found in `gen/cpp` without fixing the `mx::impl` layer on every -change to `mx::core`. i.e. you can defer integrating `mx::core` changes with `mx::impl` (and by -extension `mx::api`) until you are ready. +Runs with `make test-core-dev`. Deserializes and reserializes the test corpus with `mx::core`. It +compiles without `mx::impl`/`mx::api`, so you can iterate on `gen/cpp` and the generated core and +defer fixing the `mx::impl` integration until you are ready. ### Flow (same in all three languages) -1. **Discover** eligible `.xml`/`.musicxml` files under `data/`, (excluding certain directories, and - marker files. See `data/README.md) - - unparseable files have a sibling file ending with `.invalid` and are skipped. +1. **Discover** eligible `.xml`/`.musicxml` files under `data/` (excluding certain directories and + marker files; see `data/README.md`). Files with a sibling `.invalid` marker are skipped as + unparseable. 2. For each file: Load the XML into a DOM, make certain expected alterations, parse it with `mx::core`, serialize it back to XML, normalize the output, and compare the two DOMs. 3. Report pass/fail per file. @@ -101,8 +89,8 @@ What you need to know right now is that `gen/cpp` is where our MusicXML types ar ## C++ coding rules -Do not use anonymous namespaces (`namespace { }`) anywhere in the codebase. All symbols must be in -a named namespace. Anonymous namespaces give internal linkage, which is correct for a normal +Do not add new anonymous namespaces (`namespace { }`); many existing files still have them and +they are being retired as touched. Anonymous namespaces give internal linkage, which is correct for a normal one-TU-per-file build but causes redefinition errors in unity builds (where multiple `.cpp` files are compiled as a single translation unit). Use a named helper function or per-type name instead: @@ -123,10 +111,28 @@ cmake --build build/unity --target mx `BATCH_SIZE=0` puts all files in a target into one translation unit, which is the strictest test. +## mx::api conventions (presence/absence) + +`mx::api` is the public product surface: plain-old-data structs that make MusicXML easier to use +than the strongly-typed `mx::core` model. No core type may appear in a public api header. + +- New fields that can be absent: use `std::optional` (values and sub-structs alike). + Precedent: `NoteData::tieLetRing`, `PartData::transposition`, `MeasureData::partSymbol`. +- Exception, enums: an absent-able enum gets an `unspecified` enumerator as its first value and + default — never `std::optional`. This includes the ternary + `Bool { unspecified, yes, no }` in `ApiCommon.h`; use it, never `std::optional`. +- Do not mass-migrate existing `bool is...Specified` / `has...` flags or `-1` sentinels; they are + legacy (issue #249). Migrations are separate, deliberate breaking changes. +- Every new field needs a default value and a matching `MXAPI_EQUALS_MEMBER(field)` line + (`MXAPI_DOUBLES_EQUALS_MEMBER` for doubles) in the type's equality block — a missed line + silently drops the field from equality and round-trip checks. + ## Quality gates Run `make fmt` to format. `make check` is the clang-format gate **only** — it builds and tests nothing. -Run `make test-core-dev` for corert (especially `mx/core` work); `make test` for everything (slow). +Run `make test-core-dev` for corert (especially `mx/core` work) and `make check-core-dev` (fmt-check ++ warning-free build) before pushing core work. `make test` runs the api/impl suite + examples; +corert, `make test-api-roundtrip`, and the gen/audit gates run separately (see `make help` / CI). Adding/removing a `data/` file: bump the pinned count in `CoreRoundtripTest.cpp`, run `make audit` (regenerates `corpus.xml` + `*.features.xml`), confirm round-trip via `make test-core-dev`. `ApiLoadSmokeTest` proves a file imports without crashing, not that the data is correct; the read→write→read gate (`make test-api-roundtrip` / `roundtrip-baseline.txt`) is the correctness check — pin a fixture there to defend a feature. diff --git a/Makefile b/Makefile index c21b2e117..02b7c618f 100644 --- a/Makefile +++ b/Makefile @@ -378,7 +378,7 @@ gen-schema: python3 -m gen gen/schema/config.toml # Feature audit: inventory which MusicXML features the corpus uses, for comparing -# against mx::api support. See audit/README.md and the api-feature-audit skill. +# against mx::api support. See audit/README.md and the mx-api-feature-audit skill. audit: python3 -m audit all diff --git a/README.md b/README.md index d62b13bc8..526df7702 100644 --- a/README.md +++ b/README.md @@ -46,7 +46,7 @@ and `MX_COVERAGE` (gcov instrumentation). The useful workflows are exposed as Ma | Target | What it does | |-----------------------------|--------------------------------------------------------------------| | `make lib` | build the `mx` static library (`mx::api` + `mx::impl`) | -| `make dev` | build `mx` + all test and example binaries | +| `make dev` | build `mx` + mxtest + example + api-roundtrip binaries | | `make test` | run api examples, then the `mxtest` suite (api/impl/file/control) | | `make test-api-roundtrip` | corpus api roundtrip in regression mode (CI gate) | | `make run-examples` | build and run all three api example programs | @@ -58,7 +58,7 @@ and `MX_COVERAGE` (gcov instrumentation). The useful workflows are exposed as Ma `make clean` removes the build tree. Knobs: `JOBS` (parallelism, auto-detected), `BUILD_TYPE` (default `Debug`), and `ARGS` (forwarded to test binaries, e.g. -`make test-core-dev ARGS='lysuite/*'`). +`make test-core-dev ARGS='[core-roundtrip] lysuite/*'`). ### Build Tenets @@ -148,9 +148,9 @@ int main () { part.measures.push_back(measure); score.parts.push_back(part); auto& mgr = DocumentManager::getInstance(); - const auto id = mgr.createFromScore(score); - mgr.writeToStream(id, std::cout); - mgr.destroyDocument(id); + const auto idResult = mgr.createFromScore(score); + mgr.writeToStream(idResult.value(), std::cout); + mgr.destroyDocument(idResult.value()); } EOF @@ -454,8 +454,8 @@ subset of MusicXML using only `mx::api`, without delving into `mx::core`. ```C++ using namespace mx::api; // an easier interface for reading and writing MusicXML using namespace mx::core; // a direct representation of a musicxml document in C++ classes -using namespace mx::impl // the logic that translates between mx::api and mx::core -using namespace mx::utility // a typical catch-all for generic stuff like logging macros +using namespace mx::impl; // the logic that translates between mx::api and mx::core +using namespace mx::utility; // a typical catch-all for generic stuff like logging macros ``` ##### `mx::api` diff --git a/audit/README.md b/audit/README.md index 7a4443773..df0e1e34e 100644 --- a/audit/README.md +++ b/audit/README.md @@ -17,7 +17,7 @@ A *feature* is an element plus the set of attribute names that appear on it. `data/synthetic/` files (which are built to cover the spec symbol by symbol, so they approximate "what is in the specification"). -The same `` document shape is what the `api-feature-audit` skill +The same `` document shape is what the `mx-api-feature-audit` skill emits for `mx::api` (as `data/api.features.xml`), so the three views -- spec (synthetic), corpus (wild), and api -- are directly comparable. @@ -85,4 +85,5 @@ and files with an `*.invalid` marker -- plus the tool's own outputs. | `extract.py` | parse one file -> element/attribute surface | | `featuresfile.py`| read/write the per-file `` format | | `summary.py` | aggregate into `data/corpus.xml` | +| `classify.py` | diff expected/actual round-trip dump pairs, rank the worklist | | `__main__.py` | CLI | diff --git a/audit/__init__.py b/audit/__init__.py index ddcece7c1..3dd7d250c 100644 --- a/audit/__init__.py +++ b/audit/__init__.py @@ -5,7 +5,7 @@ 1. the test corpus under ``data/`` (what real and synthetic files actually use), 2. the MusicXML specification (approximated by the synthetic corpus, which is built to exercise every element/attribute/enum symbol of 3.0/3.1/4.0), and - 3. the ``mx::api`` public layer (audited separately by the ``api-feature-audit`` + 3. the ``mx::api`` public layer (audited separately by the ``mx-api-feature-audit`` skill, which reads source and emits ``data/api.features.xml``). A "feature" is an element together with the set of attribute names that appear diff --git a/audit/classify.py b/audit/classify.py index 0efd7e839..f802b7be2 100644 --- a/audit/classify.py +++ b/audit/classify.py @@ -90,6 +90,9 @@ class DumpEntry: def _flat_to_rel(flat: str) -> str: """Reverse the dump harness's path flattening: ``a__b.xml`` -> ``a/b.xml``.""" + # TODO: "__" also occurs inside real corpus filenames (musetrainer/...__Chopin.xml), so + # this reversal mis-splits them; the flattener needs an unambiguous separator or an + # index file. Issue candidate. return flat.replace("__", "/") diff --git a/audit/featuresfile.py b/audit/featuresfile.py index 18a0d1141..b0ca5077a 100644 --- a/audit/featuresfile.py +++ b/audit/featuresfile.py @@ -1,7 +1,7 @@ """Read and write the ```` document format. This is the per-file sidecar shape (one ```` per corpus file) and -the same shape the ``api-feature-audit`` skill emits for ``mx::api``. Elements and +the same shape the ``mx-api-feature-audit`` skill emits for ``mx::api``. Elements and attributes are sorted so the output is stable and diff-friendly. diff --git a/data/README.md b/data/README.md index 87898fd90..af18fbfc0 100644 --- a/data/README.md +++ b/data/README.md @@ -15,19 +15,18 @@ corpus uses, to compare against `mx::api` support: Both are checked in and both have a `.xml` extension but are NOT MusicXML scores, so every corpus-walking test suite skips them (the `corert` C++/Go/C round-trip discoverers and the api -corpus walker; see `audit/README.md`). The `api-feature-audit` skill consumes them and writes its -own `api.features.xml` (also skipped). +corpus walker; see `audit/README.md`). The `mx-api-feature-audit` skill consumes them and writes +its own `api.features.xml` (also skipped). ## `.invalid` marker convention A file `foo.xml` (or `foo.musicxml`) that is **not** valid MusicXML must be accompanied by a sibling marker file `foo.xml.invalid` whose contents describe, in prose, why the file is invalid. The -purpose is to use the file is to mark for test suites which files are not expected to be parseable -by `mx`. +marker tells the test suites that the file is not expected to be parseable by `mx`. ## `.fixup.xml` sidecar convention -A file `foo.xml` that is schema-invalid parseable due to `mx` leniency will have a sidecar file, +A file `foo.xml` that is schema-invalid but parseable due to `mx` leniency will have a sidecar file, `foo.fixup.xml`, which describes how the file will be parsed. The core roundtrip suite (`corert`) loads it via `Fixer` (`src/private/mxtest/corert/Fixer.h`) and uses it to prepare the "expected" test by altering values it finds after loading the test file. diff --git a/docs/ai/design/mx-core-plan.md b/docs/ai/design/mx-core-plan.md index c0f2a2277..37e11216c 100644 --- a/docs/ai/design/mx-core-plan.md +++ b/docs/ai/design/mx-core-plan.md @@ -8,7 +8,8 @@ either proposal, **this document wins**. Where it is silent, the proposals remai material for rendered-shape sketches (the gpt-5.0 doc for API philosophy, the opus-4.7 doc for plates mechanics, file layout, and dispatch technique). -The reader is assumed to know `plates.md`, `generator-agnosticism.md`, and `gen/README.md`. +The reader is assumed to know `plates.md`, `generator-agnosticism.md` (both since folded into +`gen/DESIGN.md`), and `gen/README.md`. Nothing here amends the generator architecture: C++ is a target directory (`gen/cpp/config.toml` plus `gen/cpp/templates/`), and the cardinal rule holds — no language knowledge enters the generator's Python. One exception is explicitly scoped in §2.9: projecting the content tree and diff --git a/gen/AGENTS.md b/gen/AGENTS.md index f73b9211a..838b3e0bd 100644 --- a/gen/AGENTS.md +++ b/gen/AGENTS.md @@ -17,7 +17,7 @@ python3 -m gen # full emit: render t - `--resolve` prints the collapsed IR view (attribute groups flattened, group refs spliced). - `--config C` applies that target's companion patches before lowering. -- `--check` on `plates` validates renames and detects identifier collisions; exits non-zero -- a CI gate. +- `--check` on `plates` validates renames and detects identifier collisions; exits non-zero on failure -- a CI gate. - Full emit shortcuts: `make gen-cpp` (C++ only), `make gen` (all targets). ## Quality gates @@ -32,9 +32,10 @@ make gen-lint # pylint (config: .pylintrc) ``` After changing a target's `config.toml` or `templates/`, also run that target's corert suite -(`make test-cpp`, `make test-go`, `make test-c`). +(`make test-core-dev`, `make test-go`, `make test-c`). -If generated output changes, commit the new generated files alongside the Python change. +If generated output changes, commit the regenerated files in the same commit as the change +that produced them (`make test-gen` fails CI on drift). ## Cardinal rules @@ -60,7 +61,7 @@ collisions -- all exit non-zero with a clear `template:line` message. ## Workflows **Modifying IR or Plates** -- change `gen/ir/` or `gen/plates/`. Run `make test-gen` and -`make gen-check`. Commit updated output as a separate hunk or commit from the Python change. +`make gen-check`. **Modifying a target** -- edit only that target's `config.toml` and `templates/`. Regenerate, run the target's corert, commit the updated output. diff --git a/gen/README.md b/gen/README.md index 3a705f7c1..8b7110fd3 100644 --- a/gen/README.md +++ b/gen/README.md @@ -22,16 +22,9 @@ there. Adding a new language touches no generator Python. ## Running it -``` -python3 -m gen analyze [xsd] # structural analysis report -python3 -m gen ir [--type NAME] [--resolve] [--config C] # IR as JSON -python3 -m gen plates --config C [--type NAME] [--check] # Plates as JSON; --check gates CI -python3 -m gen render --config C --type NAME # render one type to stdout -python3 -m gen # emit the target (full run) -``` - -Full runs: `make gen-cpp` (C++ target), `make gen` (all targets). Generated output is -committed; `make test-gen` regenerates and asserts `git diff --exit-code`. +Commands, gates, and workflows: see `gen/AGENTS.md`. Full runs: `make gen-cpp` (C++ target), +`make gen` (all targets). Generated output is committed; `make test-gen` regenerates and +asserts `git diff --exit-code`. ## For agents diff --git a/src/include/mx/api/ApiCommon.h b/src/include/mx/api/ApiCommon.h index 8595b48b3..7c423a652 100644 --- a/src/include/mx/api/ApiCommon.h +++ b/src/include/mx/api/ApiCommon.h @@ -24,6 +24,8 @@ inline bool areSame(Double left, Double right) constexpr int DEFAULT_TICKS_PER_QUARTER = 3 * 4 * 5 * 7; constexpr int TICK_TIME_INFINITY = std::numeric_limits::max(); +// Intentional ternary: absent-able bools use Bool::unspecified, not std::optional. +// See "mx::api conventions" in AGENTS.md. enum class Bool { unspecified, diff --git a/src/private/mx/api/DocumentManager.cpp b/src/private/mx/api/DocumentManager.cpp index 11255e6c9..43c830cb0 100644 --- a/src/private/mx/api/DocumentManager.cpp +++ b/src/private/mx/api/DocumentManager.cpp @@ -117,6 +117,8 @@ class DocumentManager::Impl DocumentMap myMap; int myCurrentUniqueId; + // myCurrentUniqueId is seeded high so synthesized ids ("ID1000000"...) are unlikely to + // collide with ids already present in parsed documents (see PartWriter). Impl() : myMutex{}, myCurrentId{0}, myMap{}, myCurrentUniqueId{1000000} { } diff --git a/src/private/mx/impl/NoteFunctions.cpp b/src/private/mx/impl/NoteFunctions.cpp index 6dd6ceabb..57e74666f 100644 --- a/src/private/mx/impl/NoteFunctions.cpp +++ b/src/private/mx/impl/NoteFunctions.cpp @@ -116,6 +116,11 @@ api::NoteData NoteFunctions::parseNote() const const core::NoteTypeValue timeModType = reader.getTimeModificationNormalType(); const int timeModTypeDots = reader.getTimeModificationNormalTypeDots(); + // TODO: should this be ||? Either conjunct alone proves was present (the + // reader defaults normalType to the note's own type with 0 dots when absent), so && drops + // an explicit undotted , and a dotted one equal to the note's own type, as + // "unspecified". Also, the two assignments above are dead stores -- both branches below + // overwrite them. Issue candidate. bool isTimeModTypeSpecified = (timeModTypeDots > 0) && (timeModType != reader.getDurationType()); if (isTimeModTypeSpecified) diff --git a/src/private/mx/impl/NoteReader.cpp b/src/private/mx/impl/NoteReader.cpp index 1114d3a83..d100ba148 100644 --- a/src/private/mx/impl/NoteReader.cpp +++ b/src/private/mx/impl/NoteReader.cpp @@ -117,9 +117,13 @@ std::string getElisionDisplayText(const core::LyricSyllableGroup &inGroup) } } + // UTF-8 for U+203F (undertie), the conventional elision joiner, substituted when + // has no text of its own. return "\xE2\x80\xBF"; } +// Flattens elided syllables into one string: mx::api models a lyric as a single text, so +// structure is intentionally collapsed and cannot be round-tripped. std::string getLyricDisplayText(const core::LyricTextGroup &inGroup) { std::string result; @@ -368,6 +372,8 @@ void NoteReader::setTimeModification() { if (!myNote.timeModification().has_value()) { + // 1/1 is the api's "no time modification" sentinel; NoteWriter skips writing 1/1, so + // a literal 1/1 in a source file will not round-trip. myTimeModificationActualNotes = 1; myTimeModificationNormalNotes = 1; return; @@ -503,6 +509,8 @@ void NoteReader::setLyric() case core::LyricChoice::Kind::laughing: case core::LyricChoice::Kind::humming: { + // laughing/humming lyrics are not modeled in mx::api; the whole is + // intentionally dropped (not just the choice element). break; } } diff --git a/src/private/mx/impl/NoteWriter.cpp b/src/private/mx/impl/NoteWriter.cpp index 344a0f618..77d0dc5de 100644 --- a/src/private/mx/impl/NoteWriter.cpp +++ b/src/private/mx/impl/NoteWriter.cpp @@ -125,6 +125,8 @@ core::Note NoteWriter::getNote(bool isStartOfChord) const ++beamIndex; } + // 1/1 is NoteReader's "no time modification" sentinel -- writing it back would fabricate + // a on every plain note, so it is skipped here. if (myNoteData.durationData.timeModificationNormalNotes > 0 && myNoteData.durationData.timeModificationActualNotes > 0 && (myNoteData.durationData.timeModificationNormalNotes > 1 || @@ -152,6 +154,10 @@ core::Note NoteWriter::getNote(bool isStartOfChord) const } } + // TODO: is recomputed from sibling TupletStart/TupletStop geometry; + // DurationData.timeModificationNormalType is never consulted as a fallback. Tuplets + // expressed only via (no notations -- legal MusicXML) + // hit this no-op throw in release builds and silently lose . Issue candidate. if (!isTupletStartFound) { MX_DEBUG_THROW("tupletStart was not found"); @@ -521,6 +527,8 @@ void NoteWriter::setMiscData() const bool isFirst = true; for (auto s : myNoteData.miscData) { + // Comma is the item separator in the miscellaneous-field wire encoding, so commas + // inside user misc-data are irreversibly replaced with underscores. std::string::size_type position = 0; while ((position = s.find(comma, position)) != std::string::npos) { diff --git a/src/private/mx/impl/ScoreReader.cpp b/src/private/mx/impl/ScoreReader.cpp index b1756ceae..71925ce7f 100644 --- a/src/private/mx/impl/ScoreReader.cpp +++ b/src/private/mx/impl/ScoreReader.cpp @@ -199,6 +199,9 @@ api::ScoreData ScoreReader::getScoreData() const myOutScoreData.lyricist = i.value(); } + // TODO: arranger/publisher overwrite lyricist (ScoreData has no fields for them), + // and ScoreWriter re-emits the value as type="lyricist" -- lossy and mislabeled. + // Issue: add arranger/publisher fields to ScoreData. if (typeStr == "arranger") { myOutScoreData.lyricist = i.value(); @@ -349,6 +352,9 @@ void ScoreReader::stopPartGroup(int partIndex, const core::PartGroup &inPartGrou grpData = popMostRecentGroupFromStack(); } + // A part-group stop arrives before the *next* score-part in the part-list walk, so the + // group's last member is the previously indexed part; partIndex==0 guards a malformed + // stop-before-any-part. grpData.lastPartIndex = partIndex > 0 ? partIndex - 1 : partIndex; myOutScoreData.partGroups.emplace_back(std::move(grpData)); @@ -592,6 +598,8 @@ int ScoreReader::findMaxDivisionsPerQuarter() const } const auto tempDiv = attrs.divisions()->value().value(); + // is a decimal in MusicXML but api ticksPerQuarter is an int: + // round to nearest, halves down (ceil(x - 0.5)). const int tempDivInt = static_cast(std::ceil(tempDiv - 0.5)); if (tempDivInt > 0) { diff --git a/src/private/mx/impl/ScoreWriter.cpp b/src/private/mx/impl/ScoreWriter.cpp index 8183918a9..4e0bfac64 100644 --- a/src/private/mx/impl/ScoreWriter.cpp +++ b/src/private/mx/impl/ScoreWriter.cpp @@ -48,6 +48,9 @@ core::ScorePartwise ScoreWriter::getScorePartwise() const } break; + // ThreePointZero also represents parsed "4.0" documents (see ScoreReader). The "3.0" + // written here only reaches the getDocument escape hatch -- writeTo*() overrides the + // version to "4.0" via DocumentManager's withWriteVersion. case api::MusicXmlVersion::ThreePointZero: { myOutScorePartwise.setVersion(std::string{"3.0"}); } diff --git a/src/private/mxtest/file/MxFileRepositoy.cpp b/src/private/mxtest/file/MxFileRepositoy.cpp index 56187ca18..139052f4a 100644 --- a/src/private/mxtest/file/MxFileRepositoy.cpp +++ b/src/private/mxtest/file/MxFileRepositoy.cpp @@ -37,6 +37,9 @@ const std::string MxFileRepository::getFullPath(const std::string &fileName) const std::string MxFileRepository::getNameWithoutExtension(const std::string &fileName) { const auto lastDot = fileName.find_last_of('.'); + // TODO: off-by-one -- substr(0, lastDot) is the name without extension; this drops the + // final character ("freezing.xml" -> "freezin") and relies on npos-1 wraparound when + // there is no dot. Only affects test-output filenames today (FreezingRoundTrip.cpp). return fileName.substr(0, lastDot - 1); } From eb63cbca41e20d60ad3b0f98a8e377bbaec4095e Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Fri, 3 Jul 2026 06:36:34 +0000 Subject: [PATCH 2/8] skills: apply audit fixes to agent skills (in progress) Corrects stale claims and trims redundancy across .claude/skills per the instructions audit; a follow-up commit completes the remaining skill fixes. --- .claude/skills/arch/SKILL.md | 10 ++-- .claude/skills/mx-api-add-feature/SKILL.md | 14 ++---- .../steps/step-1-test-strategy.md | 46 +++++++++---------- .../steps/step-2-mx-api-data-model.md | 38 +++------------ .../steps/step-3-mx-impl.md | 31 ++++--------- .../SKILL.md | 35 ++++---------- .claude/skills/mx-api-feature-audit/SKILL.md | 17 ++----- .../resources/api-features-format.md | 6 +-- .../steps/step-1-refresh-corpus.md | 4 -- .../steps/step-2-audit-enums.md | 4 +- .../steps/step-3-audit-elements.md | 12 ++--- .../steps/step-4-recommend.md | 2 +- 12 files changed, 67 insertions(+), 152 deletions(-) diff --git a/.claude/skills/arch/SKILL.md b/.claude/skills/arch/SKILL.md index e652f6600..e001f2249 100644 --- a/.claude/skills/arch/SKILL.md +++ b/.claude/skills/arch/SKILL.md @@ -1,5 +1,5 @@ --- -name: architect +name: arch description: Prime the model with senior architect principles during design or coding. Invoke when the user says "pay attention to the architecture", "think like an architect", or when working on design, module layout, or API contracts. --- # Architect Mode @@ -17,13 +17,11 @@ should provide enough flexibility to generate just about any code from the Music ### C++ Goals -The `mx/core` layer should... - -Cardinal requirements: +The `mx/core` layer's cardinal requirements: - Spec correctness: it should be impossible to use the C++ code to construct a document that is not valid to the Music XML 4.0 spec. -- Modern, safe C++. Use C++20. No bare pointers, no bare free or malloc. Smart pointers. S Meyers - Effective Modern C+ +- Modern, safe C++. Use C++20. No bare pointers, no bare free or malloc. Smart pointers. Scott + Meyers, Effective Modern C++ C++ code optimization priorities 1. Compile time diff --git a/.claude/skills/mx-api-add-feature/SKILL.md b/.claude/skills/mx-api-add-feature/SKILL.md index 0f20ca7d3..21971216c 100644 --- a/.claude/skills/mx-api-add-feature/SKILL.md +++ b/.claude/skills/mx-api-add-feature/SKILL.md @@ -15,20 +15,14 @@ user-invocable: true The `mx::api` namespace is the public interface of the `mx` library: a deliberate *subset* of MusicXML expressed in a simple object model that is easier to work with than the MusicXML DOM. -Because it is a subset, a frequent user request is "add X". ## Architecture: three layers A feature flows through three layers. Know which one you are touching: -- `mx::core` (`src/private/mx/core/generated/`) -- the strongly-typed MusicXML XSD model, - *generated* from the schema by `gen/`. The element/attribute you are adding almost always ALREADY - EXISTS here, fully parseable. Do NOT regenerate core or edit `gen/` for a normal feature. -- `mx::impl` (`src/private/mx/impl/`) -- the translation layer. `*Reader` classes turn core into - api; `*Writer` classes turn api into core. See Step 3. -- `mx::api` (`src/include/mx/api/`) -- the public data model (`ScoreData`, `PartData`, - `MeasureData`, `NoteData`, `DirectionData`, ...). This is the surface you expose the feature on. - See Step 2. +- `mx::core` -- generated XSD model; almost always already has your element. Do NOT edit `gen/`. +- `mx::impl` -- translation layer: `*Reader` core -> api, `*Writer` api -> core. See Step 3. +- `mx::api` (`src/include/mx/api/`) -- the public data model you expose the feature on. See Step 2. So a typical feature is two-sided work in `mx::api` + `mx::impl` only; `mx::core` already has it. @@ -72,4 +66,4 @@ Run `make test` for the unit suites and `make test-api-roundtrip` for the corpus ## (optional) Step 5: Open a PR -If the user asks you to open a PR, see ./resources/pr-template.md +If the user asks for a PR, use the `open-mx-pr` skill. diff --git a/.claude/skills/mx-api-add-feature/steps/step-1-test-strategy.md b/.claude/skills/mx-api-add-feature/steps/step-1-test-strategy.md index f71283005..930210ba9 100644 --- a/.claude/skills/mx-api-add-feature/steps/step-1-test-strategy.md +++ b/.claude/skills/mx-api-add-feature/steps/step-1-test-strategy.md @@ -2,14 +2,13 @@ Decide how the new `mx::api` feature will be tested before you write data-model or impl code. The end state of this step is concrete red/green tests in the repo that exercise the feature through the -api. This file explains the testing machinery, then gives an ordered procedure. +api. ## Background -`mx::api` is a deliberate subset of MusicXML. Most real corpus files lose fidelity on an api -round-trip by design (the api drops or reorders `part-group`, `credit`, most of `defaults`, -`footnote`/`level`, etc.). Keep that reality in mind: it is why corpus-level testing rarely grows -and why unit tests carry most of the weight. +`mx::api` is a deliberate subset of MusicXML. Some features are still dropped by design (e.g. +`footnote`/`level`); `part-group`, `credit`, and `defaults` are now modeled. Keep that reality in +mind: it is why corpus-level testing rarely grows and why unit tests carry most of the weight. There are two kinds of api tests. @@ -69,32 +68,30 @@ side, and run a strict DOM compare (`mxtest/corert/Compare`). Zero tolerance on gate. Run with `make test-api-roundtrip`. CI runs it in `.github/workflows/ci.yaml` (the `linux-api` job step "Run corpus api roundtrip (regression mode)" and again in the docker job). - `discovery `: walks the whole corpus (excludes `expected/`, `testOutput/`, - `generalxml/`, `smufl/`, `*.fixup.xml`, and any file with a sibling `*.invalid` marker), prints - one tab-separated line per file + `generalxml/`, `smufl/`, `*.fixup.xml`, `*.features.xml`, `corpus.xml`, `api.features.xml`, and + any file with a sibling `*.invalid` marker), prints one tab-separated line per file `PASS|FAIL|SKIP|LOADFAIL|GETDATAFAIL|CREATEFAIL relpath detail`, and always exits 0. Run with `make discover-api-roundtrip`. Never a gate. -`roundtrip-baseline.txt` currently pins exactly one file (`ksuite/k016a_Miscellaneous_Fields.xml`). -Read its header comment: of 829 discovered files, only one survives the strict DOM compare, because -the api is a subset. The baseline grows only by deliberate manual commits, never automatically. +`roundtrip-baseline.txt` is a growing pinned list (~140 files across +ksuite/lysuite/musuite/mjbsuite/custom/synthetic; see the file's header). It grows only by +deliberate manual commits, never automatically. ### 3. Synthetic corpus `data/synthetic/` holds `~390` files (see `data/README.md`) named `..xml` (version = the schema the construct first appears in; root `version` stays `3.0`). Each isolates one -element/attribute set for symbol coverage. But they are dense: every file wraps the construct in -`part-group` + `footnote` + `level` scaffolding that the api drops, so a synthetic file almost -always fails the api round-trip by design and is not a drop-in baseline candidate. Treat synthetic -files as references for valid MusicXML shape, and as `fromXml(...)` *input* fixtures in a unit test -(load the file's XML, assert the api surfaced the field) -- not as new `roundtrip-baseline.txt` -entries unless the file genuinely survives the strict compare. +element/attribute set for symbol coverage. Many synthetic files still fail the strict compare due +to unrelated subset gaps; several (segno/coda/key-accidental) are already in the baseline; check +with discovery mode before assuming either way. Treat synthetic files as references for valid +MusicXML shape, and as `fromXml(...)` *input* fixtures in a unit test (load the file's XML, assert +the api surfaced the field) -- not as new `roundtrip-baseline.txt` entries unless the file +genuinely survives the strict compare. ## Build note -`make test-api-roundtrip` and `make discover-api-roundtrip` both depend on the `dev` target, which -builds `mxtest-api-roundtrip`. `make test` builds and runs `mxtest` (the unit tests) plus examples. -The procedure below runs these as it goes: discovery before you implement (step 2), then `make test` -and the quality gates afterward (steps 8-10). +`make test-api-roundtrip` and `make discover-api-roundtrip` depend on the `dev` target (builds +`mxtest-api-roundtrip`); `make test` builds and runs `mxtest` (the unit tests) plus examples. ## Procedure @@ -135,8 +132,8 @@ Do these in order. The discovery snapshot must be captured before you implement. 7. Implement the feature (skill steps 2-3: data model in `mx::api`, wiring in `mx::impl`). -8. Make the unit test pass (green): `make test` (runs `mxtest`). Iterate until your new `TEST(...)` - cases pass. Run `make fmt` and `make check` so CI gates stay green. +8. Make the unit test pass (green); iterate until your new `TEST(...)` cases pass (SKILL.md Step 4 + owns the test/fmt/check commands). 9. Re-run discovery and diff: `make discover-api-roundtrip > /tmp/api-discovery-after.txt`, then `diff /tmp/api-discovery-before.txt /tmp/api-discovery-after.txt`. Look for files that moved to @@ -146,10 +143,9 @@ Do these in order. The discovery snapshot must be captured before you implement. 10. Grow the baseline only if vetted: for each newly-PASSing, attributable file, add its `data/`-relative path to `src/private/mxtest/api/roundtrip-baseline.txt` with a short comment - explaining why it now passes. Then `make test-api-roundtrip` to confirm the full pinned list - (zero tolerance) still passes. Commit this deliberately. + explaining why it now passes. Commit this deliberately. 11. If discovery yielded nothing (common), do not force a baseline entry. The unit test(s) from steps 4-5 (plus any synthetic fixture) are the feature's regression coverage. Confirm the end state: at least one red-turned-green `TEST(...)` in `src/private/mxtest/api/` that exercises the - new field through the api, passing under `make test`. + new field through the api. diff --git a/.claude/skills/mx-api-add-feature/steps/step-2-mx-api-data-model.md b/.claude/skills/mx-api-add-feature/steps/step-2-mx-api-data-model.md index 92e1a0e52..e0a1ee9e8 100644 --- a/.claude/skills/mx-api-add-feature/steps/step-2-mx-api-data-model.md +++ b/.claude/skills/mx-api-add-feature/steps/step-2-mx-api-data-model.md @@ -28,17 +28,16 @@ is not a legal C++ identifier the value gets a trailing underscore or a spelled- Sentinel convention: most api enums carry a sentinel as the first value, usually `unspecified` (`Bool`, `Placement`, `MeasureNumbering`, `BarlineType`, `LineType`, `LineHook`, `DurationName`, -`MarkType`). `Accidental` and `Notehead` use `none` for "no glyph". `Step` is unusual: it uses -explicit integer values (`c = 0 ... b = 6`) plus `unspecified = -1`. `BarlineType` additionally has -`unsupported` for core values mx::api chooses not to model (e.g. `BarStyle::tick`). Pick the -sentinel that matches the sibling enums in the same area. +`MarkType`). `Accidental` uses `none` as its sentinel; `Notehead` has no sentinel (`none` is a real +notehead value). `Step` is unusual: it uses explicit integer values (`c = 0 ... b = 6`) plus +`unspecified = -1`. `BarlineType` additionally has `unsupported` for core values mx::api chooses +not to model. String/enum and core/api mapping does not live in the public header. The bridge tables live in `src/private/mx/impl/Converter.h` and `Converter.cpp` as `Converter::EnumMap`, which is a `std::vector>` walked by `findApiItem` / `findCoreItem`. For example `Converter::stepMap`, `Converter::durationMap`, `Converter::lineType`, -`Converter::barlineMap`. Wiring those tables up is step 3; just be aware that every new api enum -needs a parallel core enum and a bridge entry there. +`Converter::barlineMap`. ### 2. Classes are plain structs @@ -63,26 +62,7 @@ Optionality is expressed several ways; pick the one already used by neighbors: - `std::optional<...>` (via `OptionalDouble` in `ApiCommon.h`) - present but rare; prefer the flag/sentinel patterns the surrounding type already uses -Representative example (`BarlineData.h`): - -```cpp -class BarlineData -{ - public: - int tickTimePosition; - BarlineType barlineType; - EndingType endingType; - int endingNumber; - bool repeat; - HorizontalAlignment location; - - BarlineData() - : tickTimePosition{0}, barlineType{BarlineType::normal}, endingType{EndingType::none}, - endingNumber{0}, repeat{false}, location{HorizontalAlignment::unspecified} - { - } -}; -``` +See `src/include/mx/api/BarlineData.h` for a representative shape. Equality is generated by macros from `ApiEquality.h` (included via `ApiCommon.h`). The block is `MXAPI_EQUALS_BEGIN(TypeName)` then one `MXAPI_EQUALS_MEMBER(field)` per member (use @@ -110,7 +90,7 @@ than introducing a new top-level vector. Know these aggregates: - `DirectionData` (`DirectionData.h`) is the big one. A single direction holds parallel vectors for `tempos`, `marks`, `wedgeStarts`/`wedgeStops`, `ottavaStarts`/`ottavaStops`, `bracketStarts`/`bracketStops`, `dashesStarts`/`dashesStops`, `pedalStarts`/`pedalStops`, `words`, - `chords`, `segnos`, `codas`, `rehearsals`, plus an `orderedComponents` vector of + `chords`, `segnos`, `codas`, `rehearsals`, etc., plus an `orderedComponents` vector of `DirectionComponent{ DirectionComponentKind kind; int index; }` that records render order across those vectors. Adding a new direction-type element means adding a vector here, a `DirectionComponentKind` enumerator, and (usually) keeping `isDirectionDataEmpty` and the @@ -179,7 +159,3 @@ than introducing a new top-level vector. Know these aggregates: 10. Do not write any core/api conversion here. Defining the api enum/struct is the whole of step 2; the parallel `mx::core` enum, the `Converter::EnumMap` bridge entry in `Converter.h`/`.cpp`, and the reader/writer wiring are step 3. Just leave the api type ready for that step. - -11. Sanity-check by pattern-matching an existing analog before finishing: pick the closest existing - feature (segno/coda for a simple flagged element, wedge/ottava for a start/stop spanner, a - `MarkType` value for a note glyph) and confirm your additions look structurally identical. diff --git a/.claude/skills/mx-api-add-feature/steps/step-3-mx-impl.md b/.claude/skills/mx-api-add-feature/steps/step-3-mx-impl.md index 980c14aea..7de07aebe 100644 --- a/.claude/skills/mx-api-add-feature/steps/step-3-mx-impl.md +++ b/.claude/skills/mx-api-add-feature/steps/step-3-mx-impl.md @@ -28,10 +28,7 @@ Delegation chain on read: `ScoreReader::getScoreData` -> a `PartReader::getPartD `ScoreWriter::getScorePartwise` -> `PartWriter::writeMeasures` -> `MeasureWriter::getPartwiseMeasure`, emitting `core::MusicDataChoice` items and delegating to `NoteWriter` and `DirectionWriter`. -> The failure mode to avoid is adding only the read half. `` is read -> (`DirectionReader::parseSegno` fills `DirectionData.segnos`) but never written (`DirectionWriter` -> has no segno case), so a segno read from a file is silently dropped on write. The read half alone -> is not "done." +> Adding only the read half means data is silently dropped on write; always implement both. ## How a measure is walked @@ -41,7 +38,8 @@ Read: `MeasureReader::getMeasureData` iterates `musicData()`, a sequence of type `core::MusicDataChoice`. `parseMusicDataChoice` is an if/else on `mdc.isNote()` / `isBackup()` / `isForward()` / `isDirection()` / `isAttributes()` / `isHarmony()` / `isBarline()`, dispatching to `parseNote`, `parseBackup`, `parseForward`, `parseDirection`, `parseAttributes`, `parseBarline`, etc. -`parsePrint` / `parseSound` / `parseGrouping` / `parseLink` / `parseBookmark` are deliberate no-ops. +`parseGrouping` / `parseLink` / `parseBookmark` are deliberate no-ops; `parseSound` reads sound +data; print is handled at score level. The loop peeks one item ahead to detect whether the next note carries a `` tag, which the time logic needs. @@ -91,17 +89,6 @@ difference emits a synthesized `core::Backup`, a positive one a `core::Forward`. direction whose stored time differs from the cursor gets a `core::Offset` instead of a backup/forward (`DirectionWriter`). -## Worked example: a dynamic (mark on a note) - -Read: `NoteFunctions::parseNotations` switches on `core::NotationsChoice::Kind`; the `dynamics` case -constructs a `DynamicsReader` and calls `parseDynamics(...marks)`, which maps the core kind to an -`api::MarkType` via `Converter::convertDynamic`, sets `markData.tickTimePosition`, and appends to the -marks vector. - -Write: `NoteWriter::getNote` constructs a `NotationsWriter` that emits the marks back into a -`core::Notations`; the dynamic itself is rebuilt by `DynamicsWriter::getDynamics`, the symmetric -inverse. `Converter` supplies the enum mapping for both directions. - ## Instructions Assume step 2 already added the api field(s)/enum(s). For each step, find the existing element most @@ -121,7 +108,7 @@ like yours and copy its pattern - do not invent structure. item), `DirectionReader::parseSegno` (direction child), or the `dynamics` case in `NoteFunctions::parseNotations` (note mark). -3. Add the symmetric write - the step most often skipped (see the segno note above). Rebuild the core +3. Add the symmetric write - the step most often skipped. Rebuild the core element from the api field. Copy `MeasureWriter::writeBarlines`, or a loop in `DirectionWriter::getDirectionLikeThings` (e.g. the wedge loop) that does `core::DirectionType dt; dt.setChoice(...); addDirectionType(std::move(dt), direction);`. For a note @@ -161,10 +148,8 @@ like yours and copy its pattern - do not invent structure. `NotationsWriter`. Update `PartReader::calculateNumStaves` if your element carries a staff number that could imply more staves than otherwise detected. -10. Verify symmetry with a roundtrip. Once both halves compile, run the step-1 api roundtrip test and - `make test`. The roundtrip deserializes then reserializes; a missing write, wrong tick position, or - dropped attribute makes the output DOM differ from the input and the test fail. A passing roundtrip - is the definition of symmetric and done. +10. Verify symmetry with the step-1 roundtrip test (SKILL.md Step 4 owns the test/fmt/check runs); a + missing write, wrong tick position, or dropped attribute makes the output DOM differ and it fails. -11. Keep it lean and ASCII. Match the surrounding code style; run `make fmt` and `make check`. Add only - the api surface the feature actually needs - every field added here must be both read and written. +11. Keep it lean and ASCII; match the surrounding code style. Add only the api surface the feature + actually needs - every field added here must be both read and written. diff --git a/.claude/skills/mx-api-classify-roundtrip-failures/SKILL.md b/.claude/skills/mx-api-classify-roundtrip-failures/SKILL.md index 2042003ab..e9f924fa6 100644 --- a/.claude/skills/mx-api-classify-roundtrip-failures/SKILL.md +++ b/.claude/skills/mx-api-classify-roundtrip-failures/SKILL.md @@ -61,24 +61,16 @@ Then always run: make classify-api-roundtrip ``` -Read the stdout summary it prints — status counts, the distance histogram, and the ranked worklist. -That worklist *is* the headline answer to "what should we fix next." +Read the stdout summary it prints -- status counts, the distance histogram, the ranked worklist, +and the greedy batch plan. That worklist *is* the headline answer to "what should we fix next." ### Step 2 — mine `build/api/classified.json` These read-only analyses expand on the stdout summary. Adjust the path if `--out` was overridden. -The worklist — signatures ranked by candidate files unblocked (`sole_blocker` = files this fix flips -green on its own; `files_blocked` = candidate files that include it): - -``` -python3 - <<'PY' -import json -d = json.load(open("build/api/classified.json")) -for row in d["worklist"][:25]: - print(f"{row['sole_blocker']:>4} sole {row['files_blocked']:>5} total {row['signature']}") -PY -``` +The worklist (already in the stdout summary) ranks signatures by candidate files unblocked +(`sole_blocker` = files this fix flips green on its own; `files_blocked` = candidate files that +include it). Instant wins — candidate files one fix away from passing (distance 1): @@ -91,19 +83,8 @@ for f in d["near_misses"]["1"]: PY ``` -Small fix-sets — files that pass once a handful of features land (distance 2–3); the union of their -signatures is a high-yield batch: - -``` -python3 - <<'PY' -import json -from collections import Counter -d = json.load(open("build/api/classified.json")) -for dist in ("2", "3"): - sigs = Counter(s for f in d["near_misses"][dist] for s in f["signatures"]) - print(f"distance {dist}: {len(d['near_misses'][dist])} files; signatures {sigs.most_common(10)}") -PY -``` +Small fix-sets -- the greedy set-cover batch plan is already computed in `d["batch_plan"]` (rows +have `fix`/`added_files`/`cumulative_files`; also printed in the stdout summary). Crash cluster (highest severity — no output at all) — group by kind to find the common root: @@ -162,5 +143,5 @@ landscape, not a regression. ## Hand-off Fixes (if requested) -- To fix a dropped/under-supported element or a crash: use the `add-mx-api-feature` skill. +- To fix a dropped/under-supported element or a crash: use the `mx-api-add-feature` skill. - The findings belong under the tracking issue #208; file specifics with the `open-mx-issue` skill. diff --git a/.claude/skills/mx-api-feature-audit/SKILL.md b/.claude/skills/mx-api-feature-audit/SKILL.md index b51fba654..f69ac70ff 100644 --- a/.claude/skills/mx-api-feature-audit/SKILL.md +++ b/.claude/skills/mx-api-feature-audit/SKILL.md @@ -37,23 +37,16 @@ view 3 and the comparison. ## Two kinds of finding -Keep these strictly separate -- they have different severity: - - BUG (silent data loss): a value exists in a `mx::core` enum but the parallel `mx::api` enum lacks it, so the `mx::impl` reader maps it to a fallback (`unspecified`/`otherX`) and the original value - is lost on round-trip. This is almost always introduced when a newer MusicXML version adds an enum - member to `mx::core` (generated from the XSD) and the hand-written `mx::api` enum is not updated to - match. See Step 2. + is lost on round-trip. See Step 2. - GAP (unsupported feature): an element or attribute that `mx::api` simply does not model. This is by design for obscure features, but a gap in a *heavily-used wild* feature is a candidate to add. See Step 3. -A worked BUG example you can confirm today: `core::DynamicsChoice::Kind` -(`src/private/mx/core/generated/DynamicsChoice.h`) contains `n`, `pf`, and `sfzp` (added in MusicXML -3.1), but `api::MarkType` (`src/include/mx/api/MarkData.h`) stops at `fz`, and the `dynamicsMap` at -`src/private/mx/impl/Converter.cpp:170` has no entries for them -- so `findApiItem` returns -`api::MarkType::unspecified` and those dynamics are dropped. That is the exact failure mode Step 2 -hunts for. +A worked BUG example you can confirm today: `core::AccidentalValue::other` has no `api::Accidental` +member (recorded in `data/api.features.xml` as status="missing-members"). That is the exact failure +mode Step 2 hunts for. ## Procedure @@ -68,5 +61,5 @@ Run the steps in order. Each writes its output to a known location so the next c The `data/api.features.xml` output format (a superset of the per-file `` shape) is specified in `./resources/api-features-format.md`. -Read `AGENTS.md` (repo root) for the layer map, and the `add-mx-api-feature` skill for how to +Read `AGENTS.md` (repo root) for the layer map, and the `mx-api-add-feature` skill for how to actually implement anything this audit recommends. diff --git a/.claude/skills/mx-api-feature-audit/resources/api-features-format.md b/.claude/skills/mx-api-feature-audit/resources/api-features-format.md index e940707e3..29ca301bc 100644 --- a/.claude/skills/mx-api-feature-audit/resources/api-features-format.md +++ b/.claude/skills/mx-api-feature-audit/resources/api-features-format.md @@ -9,7 +9,7 @@ suites skip it automatically (same rule as the corpus sidecars). ```xml - + mx::api 4.0 @@ -21,7 +21,7 @@ suites skip it automatically (same rule as the corpus sidecars). - + n pf @@ -43,7 +43,7 @@ suites skip it automatically (same rule as the corpus sidecars). - `` -- `full`: element and its commonly-used attributes are exposed and round-trip; `partial`: element is exposed but some attributes/enum members are not; `none`: not modeled at all. -- `` -- whether that attribute is carried through `mx::api`. List the +- `` -- whether that attribute is carried through `mx::api`. List the attributes that appear in `data/corpus.xml` for the element (at least the ones with non-zero `wild-files`); you do not have to list spec attributes no corpus file uses. - `` -- the result of the Step 2 diff --git a/.claude/skills/mx-api-feature-audit/steps/step-1-refresh-corpus.md b/.claude/skills/mx-api-feature-audit/steps/step-1-refresh-corpus.md index 097d31cf5..7341579ab 100644 --- a/.claude/skills/mx-api-feature-audit/steps/step-1-refresh-corpus.md +++ b/.claude/skills/mx-api-feature-audit/steps/step-1-refresh-corpus.md @@ -43,9 +43,5 @@ Those are real but rare in practice -- low priority to add unless specifically r Useful greps over `data/corpus.xml` before you dive in: -- The most-used wild elements: they are simply the first `` entries in the file. - Elements with `wild-files="0"` (synthetic-only): spec features unused in the wild. - A specific element: search for `name=""` to read its attribute usage and example files. - -Carry the corpus picture into Step 2 and Step 3: a bug or gap in a high-`wild-files` feature matters -far more than one in a synthetic-only feature. diff --git a/.claude/skills/mx-api-feature-audit/steps/step-2-audit-enums.md b/.claude/skills/mx-api-feature-audit/steps/step-2-audit-enums.md index 325079b30..db7ef41c3 100644 --- a/.claude/skills/mx-api-feature-audit/steps/step-2-audit-enums.md +++ b/.claude/skills/mx-api-feature-audit/steps/step-2-audit-enums.md @@ -52,8 +52,8 @@ grep -n "const Converter::EnumMap" src/private/mx/impl/Converter.cpp ``` Each line names the `core::X` and `api::Y` pair. Work through all of them. The worked example from -the SKILL overview -- `dynamicsMap` (`Converter.cpp:170`) missing `n`/`pf`/`sfzp` from -`core::DynamicsChoice::Kind` -- is exactly this procedure applied to one table. +the SKILL overview -- `accidentalMap` missing `other` from `core::AccidentalValue` -- is exactly +this procedure applied to one table. ## Enums not behind an EnumMap diff --git a/.claude/skills/mx-api-feature-audit/steps/step-3-audit-elements.md b/.claude/skills/mx-api-feature-audit/steps/step-3-audit-elements.md index 490e6b4cf..921651d14 100644 --- a/.claude/skills/mx-api-feature-audit/steps/step-3-audit-elements.md +++ b/.claude/skills/mx-api-feature-audit/steps/step-3-audit-elements.md @@ -15,8 +15,8 @@ For each element you audit: 1. Is the element modeled in `mx::api`? The public data model is `src/include/mx/api/*Data.h` (`ScoreData`, `PartData`, `MeasureData`, `NoteData`, `DirectionData`, ...). Search there for a field or type that carries the element. Then confirm the translation exists in - `src/private/mx/impl/` (a `*Reader` populates it from core, a `*Writer` emits it). Element with a - field AND a reader/writer => exposed. + `src/private/mx/impl/`: read AND write must both exist (a `*Reader` populates it from core, a + `*Writer` emits it); one side only = `partial`. 2. Which of its attributes survive? For the element's `` entries in `data/corpus.xml` (focus on non-zero `wild-files`), check whether `mx::api` carries each one. Common position/font/ color attributes are often handled by shared helpers (`PositionData`, `FontData`, `ColorData`, @@ -29,12 +29,8 @@ Classify the element `support` as `full` / `partial` / `none` per the format voc ## Verify, do not guess -A field named like the element is not proof. Confirm both directions: - -- read: a `*Reader` in `mx::impl` sets the api field from the core value. -- write: a `*Writer` emits it back. If only one side exists, it is `partial` and worth a note. - -When unsure whether an attribute round-trips, find a wild file that uses it (via the element's +A field named like the element is not proof. When unsure whether an attribute round-trips, find a +wild file that uses it (via the element's `` pointers in `corpus.xml`, or the per-file sidecars) and trace it, or note the uncertainty rather than asserting support. diff --git a/.claude/skills/mx-api-feature-audit/steps/step-4-recommend.md b/.claude/skills/mx-api-feature-audit/steps/step-4-recommend.md index 1a632b0c9..3b695a98c 100644 --- a/.claude/skills/mx-api-feature-audit/steps/step-4-recommend.md +++ b/.claude/skills/mx-api-feature-audit/steps/step-4-recommend.md @@ -35,7 +35,7 @@ Recommend in this exact order -- it reflects severity, not convenience: ## Hand off to implementation -Do not implement here. The `add-mx-api-feature` skill is the implementation path -- enum bug fixes +Do not implement here. The `mx-api-add-feature` skill is the implementation path -- enum bug fixes and small attribute gaps both fit its `mx::api` + `mx::impl` two-sided flow (core almost always already has the value). Point the reader to it and, if asked, kick off the top-priority item there. From 24ea49f00957a69659ecad14dcfa0ff9f80e235c Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Fri, 3 Jul 2026 06:42:30 +0000 Subject: [PATCH 3/8] docs: finish skills audit; add mx::api design principles - skills: complete audit fixes (arch trimmed of irrelevant generic sections, stale examples replaced, optionality guidance in add-feature step-2 now matches the std::optional / unspecified convention) - docs/ai/design/api-design-principles.md: seven principles describing how mx::api re-shapes MusicXML (stateless time, containment over labels, one-fact-one-field, no neighbor-dependent meaning, denormalized state, quarantined edge cases, ignorable fidelity knobs), referenced from AGENTS.md and the mx-api-add-feature skill - NoteData.h: correct stale tie comment (writer emits both tie and tied) - DurationData.h: TODO on dead isTied field --- .claude/skills/arch/SKILL.md | 56 +++------------ .claude/skills/grill-me/SKILL.md | 5 +- .claude/skills/mx-api-add-feature/SKILL.md | 3 + .../steps/step-2-mx-api-data-model.md | 22 +++--- .claude/skills/open-mx-issue/SKILL.md | 9 +-- .claude/skills/open-mx-pr/SKILL.md | 10 ++- .claude/skills/project/SKILL.md | 34 ++++------ .claude/skills/questions/SKILL.md | 2 - AGENTS.md | 4 ++ docs/ai/design/api-design-principles.md | 68 +++++++++++++++++++ src/include/mx/api/DurationData.h | 2 + src/include/mx/api/NoteData.h | 5 +- 12 files changed, 119 insertions(+), 101 deletions(-) create mode 100644 docs/ai/design/api-design-principles.md diff --git a/.claude/skills/arch/SKILL.md b/.claude/skills/arch/SKILL.md index e001f2249..8e6cd49fd 100644 --- a/.claude/skills/arch/SKILL.md +++ b/.claude/skills/arch/SKILL.md @@ -33,53 +33,37 @@ C++ code optimization priorities ### 1. Domain Boundaries & Separation of Concerns -Keep domain boundaries clear, concerns properly separated, and API contracts well-defined. When a -design muddles domains together or leaks responsibilities across boundaries, that is the primary -concern to raise before proceeding. +Keep domain boundaries clear, concerns separated, and API contracts well-defined; raise muddled +domains before proceeding. ### 2. Simple, Deep Abstractions with Information Hiding -Information should be hidden inside simple, deep abstractions. Narrow interfaces with rich internals -are best. Flag leaking internals and wide, shallow interfaces. Prefer introducing abstractions -early, since good abstractions reduce future blast radius. +Hide information inside simple, deep abstractions: narrow interfaces, rich internals; flag the +reverse. ### 3. Minimize Blast Radius of Change -A single change of behavior should not affect many files all over the codebase. Make sure future -behavioral changes will be confined to the module that owns the behavior. Reject approaches that -spread change widely. Good modularity keeps most changes local. +Confine a behavior change to the module that owns it; reject approaches that spread change widely. ### 4. Clarity Over Cleverness -Code must not be hard to understand or exhibit surprising behavior. Naming must reduce cognitive -burden. It should be obvious what something does from its name. Follow local conventions. Push back -hard on anything confusing or unexpected. +Names must make behavior obvious; follow local conventions and push back on anything surprising. ## Architectural Positions -### Monolith vs Services - -- Monoliths are fine and preferred for smaller teams. -- Tipping point is `~20+`` engineers in one repository. - ### Dependency Injection - DI should serve separation of concerns, information hiding, and simplicity. -- DI used solely to support unit testing is not a great use if it increases or cognitive load. +- DI used solely to support unit testing is not a great use if it increases coupling or cognitive + load. - DI where it genuinely simplifies the design. ### Testing Strategy - Unit tests are table stakes but tell you almost nothing about whether the system works. -- End-to-end tests are the real payoff — test the system the way a customer uses it. +- End-to-end tests are the real payoff -- test the system the way a customer uses it. - Design for end-to-end testability as a first-class concern. -### Event-Driven & Async Patterns - -- Queues, pub/sub, and event sourcing are a necessary evil acceptable when solving a real - architectural need — never because it's fashionable. -- If a simpler procedural approach works, prefer it. - ### Error Handling - Prefer explicit error handling (Rust-style Result types) where the ecosystem supports it. @@ -95,35 +79,17 @@ hard on anything confusing or unexpected. - Configuration is behavioral surface area. Never expose it unless you must. - Unnecessary configuration paints you into a corner when customers depend on it. -- Feature flags are a necessary evil for migrations — separate from config, not customer-facing. +- Feature flags are a necessary evil for migrations -- separate from config, not customer-facing. ### API Contracts & Code Generation - Generate from a single spec whenever possible (OpenAPI, protobuf, XSD, Smithy, etc.). - A single source of truth for API surfaces is critical. -### Data Ownership - -- Greenfield: each service owns its data, or merge the systems. -- Legacy: be pragmatic about existing databases. - -### Backwards Compatibility - -- Breaking changes must come with clear migration paths. -- Customers must not be painfully impacted. - -### Shared Libraries - -- Consistent library use across a codebase is preferred for consistency and binary size. - ### Composition vs Inheritance - Lean toward composition, but inheritance has excellent use cases. -### Observability - -- Leave the door open if it doesn't harm the design. Don't compromise design quality for it. - ## When to Flag for Splitting a Module - Excessive size @@ -138,5 +104,5 @@ hard on anything confusing or unexpected. - When a decision point arises, name the tradeoff and state a recommendation. - Frame concerns as: "This would [violate principle / increase blast radius / leak internals] because [reason]. Consider [alternative]." -- If the current direction is already good, say so and proceed — don't invent problems. +- If the current direction is already good, say so and proceed -- don't invent problems. - Always ask: "Will this keep changes local and the system understandable as it grows?" diff --git a/.claude/skills/grill-me/SKILL.md b/.claude/skills/grill-me/SKILL.md index e3d9233bf..da0fd8e78 100644 --- a/.claude/skills/grill-me/SKILL.md +++ b/.claude/skills/grill-me/SKILL.md @@ -5,13 +5,12 @@ description: > shared understanding, resolving each branch of the decision tree. Use when user wants to stress-test a plan, get grilled on their design, or mentions "grill me". -argument-hint: "" +argument-hint: "" disable-model-invocation: false user-invocable: true --- -Never use the `AskUserQuestion` tool. Never render a numbered option picker. Ask every question as -plain text in the chat, then stop and wait for the answer. +Never use the `AskUserQuestion` tool; ask every question as plain text and wait for the answer. Interview me relentlessly about every aspect of this plan until we reach a shared understanding. Walk down each branch of the design diff --git a/.claude/skills/mx-api-add-feature/SKILL.md b/.claude/skills/mx-api-add-feature/SKILL.md index 21971216c..f351fa262 100644 --- a/.claude/skills/mx-api-add-feature/SKILL.md +++ b/.claude/skills/mx-api-add-feature/SKILL.md @@ -43,6 +43,9 @@ needs and nothing extra. Before writing any code: 3. Decide the minimal api surface and which existing aggregate the feature belongs in (for example, a direction-like marking joins the measure's directions; a note-attached marking joins `NoteData`). Step 2 covers these patterns. +4. Check the feature against `docs/ai/design/api-design-principles.md`: which MusicXML defect + (stateful / flat / duplicated / id-linked / order-dependent) does the element carry, and which + principle applies? Never mirror the element's raw shape. ## Step 1: Test Strategy diff --git a/.claude/skills/mx-api-add-feature/steps/step-2-mx-api-data-model.md b/.claude/skills/mx-api-add-feature/steps/step-2-mx-api-data-model.md index e0a1ee9e8..c994f5a52 100644 --- a/.claude/skills/mx-api-add-feature/steps/step-2-mx-api-data-model.md +++ b/.claude/skills/mx-api-add-feature/steps/step-2-mx-api-data-model.md @@ -51,16 +51,11 @@ hand-written default constructor initializer list (`MeasureData`, `DirectionData `LineData`, `MidiData`). Members are grouped loosely by topic with a short comment above non-obvious fields. -Optionality is expressed several ways; pick the one already used by neighbors: - -- a sentinel enum value: `placement{Placement::unspecified}`, `barlineType{BarlineType::normal}` -- a magic numeric value: `width{-1.0}` ("less than 0 means unspecified"), `voice{-1}`, - `staffLines = -1`, `MidiData` uses `-1` for each absent int -- a paired `bool ...IsSpecified` / `has...` flag: `isStaffValueSpecified`, `isDashLengthSpecified`, - `MarkData::hasMordentLong`, `SegnoData::isColorSpecified`, - `NoteData::isDisplayStepOctaveSpecified` -- `std::optional<...>` (via `OptionalDouble` in `ApiCommon.h`) - present but rare; prefer the - flag/sentinel patterns the surrounding type already uses +Optionality (see "mx::api conventions" in AGENTS.md): new absent-able fields use +`std::optional` (precedent: `NoteData::tieLetRing`, `PartData::transposition`); absent-able +enums get an `unspecified` first enumerator instead (incl. the ternary `Bool`). You will see +legacy patterns in existing types -- `-1` sentinels, paired `bool is...Specified` / `has...` +flags -- leave them alone (issue #249); do not use them for new fields. See `src/include/mx/api/BarlineData.h` for a representative shape. @@ -127,10 +122,9 @@ than introducing a new top-level vector. Know these aggregates: member initializer per field, OR a default constructor with a full member-initializer list. Every member must have a defined default. Follow the style of the file you are editing. -4. Choose the optionality convention used by neighboring fields: sentinel enum value, a magic - numeric sentinel (`-1`, `-1.0`), or a paired `bool is...Specified` / `has...` flag. Do not mix a - new convention into a type that already uses another. Reach for `std::optional` only if that type - already does. +4. For an absent-able field use `std::optional`; for an absent-able enum use an `unspecified` + first enumerator. Never add new `-1` sentinels or `is...Specified` flags, even in types that + already have them. 5. If a new positioned-in-a-measure type is being added (a sibling of `NoteData`, `DirectionData`, `BarlineData`, `MarkData`), give it `int tickTimePosition` defaulting to `0`. If it is a child of diff --git a/.claude/skills/open-mx-issue/SKILL.md b/.claude/skills/open-mx-issue/SKILL.md index a38c73dda..5a267cf54 100644 --- a/.claude/skills/open-mx-issue/SKILL.md +++ b/.claude/skills/open-mx-issue/SKILL.md @@ -25,13 +25,10 @@ documented by an api-feature-audit). Use `gh` for GitHub interactions. -Research the issue locally, make sure you understand it. - Search the open issues for one that it would duplicate. If it would fully or exactly duplicate an issue, then suggest editing or commenting on the existing issue instead of creating a new one. -Figure out which issues are related. Figure out if there is an issue that would serve as a good -parent for this new one to be a child of. +Identify related issues and a potential parent/tracking issue. When writing the issue, the title should be lowercase, e.g. fix missing flerbinator in `mx::impl`. @@ -48,6 +45,4 @@ to related issues and/or PRs (if any are related). If it is a child of another i tracking issue body to add it to the list of tracked issues and also make it a sub-issue of the tracker. -Do NOT attribute yourself in the issue. No "Written by Claud" or "Codex" or whatever. Nope. Do not -take credit for this. Instead you may add the `ai` label as a flag that a coding agent created the -issue. +No self-attribution in the issue body; add the `ai` label instead. diff --git a/.claude/skills/open-mx-pr/SKILL.md b/.claude/skills/open-mx-pr/SKILL.md index e74f8a596..3263e2790 100644 --- a/.claude/skills/open-mx-pr/SKILL.md +++ b/.claude/skills/open-mx-pr/SKILL.md @@ -36,8 +36,8 @@ When writing the PR, the title should be lowercase and start with one keyword, e Look at the labels on GitHub, choose the ones that match the pr best. -Write an pr body. Keep it tight and human-readable, but with enough information to understand what -needs to be done. +Write a PR body. Keep it tight and human-readable, but with enough information to understand what +was done and why. Determine from the original prompt whether the user wanted you to YOLO and open or whether the user wanted to see a draft first. @@ -45,11 +45,9 @@ wanted to see a draft first. When ready, create the pr and note the pr number. Make sure the PR body contains references to issues closed and issues/PRs related (if any). -Do NOT attribute yourself in the PR. No "Written by Claud" or "Codex" or whatever. Nope. Do not -take credit for this. Instead you may add the `ai` label as a flag that a coding agent created the -issue. +No self-attribution in the PR body; add the `ai` label instead. -Do not use excessive bold and italics in the issue body. Go easy on the keywords since they will add +Do not use excessive bold and italics in the PR body. Go easy on the keywords since they will add a lot of backtick formatting as well. Template/sample: diff --git a/.claude/skills/project/SKILL.md b/.claude/skills/project/SKILL.md index 61a495201..b66a23f7b 100644 --- a/.claude/skills/project/SKILL.md +++ b/.claude/skills/project/SKILL.md @@ -22,7 +22,7 @@ allowed-tools: > # /project A project is a longer-running piece of work that is larger than a single Agent context can -comprehend. Agents will be conduct a piece of work for the usable life of their context, then they +comprehend. Agents will conduct a piece of work for the usable life of their context, then they will be replaced with new Agents to continue the work. Each agent session should be designed to do a reasonably sized piece of work, then update the project context to prepare the next agent to continue the work. @@ -30,8 +30,8 @@ continue the work. ## Terminology A project has: -- sessions (or iterations): pieces of work that can be conducted within a single agent session. -- milestones: a piece of work that can be completed by many agent sessions (or iterations). +- sessions: pieces of work that can be conducted within a single agent session. +- milestones: a piece of work that can be completed by many agent sessions. - goal: the high-level goal of the project. - plan: the plan for reaching the goal through milestones. - current-state: what has been done immediately in the previous session, and what is expected to be @@ -40,23 +40,20 @@ A project has: ## Directory Structure -The project directory is at `docs/ai/projects`. A specific project is as +The project directory is at `docs/ai/projects`. A specific project is at `docs/ai/projects/{{name}}`. Your entrypoint into a project is `docs/ai/projects/{{name}}/index.md`. Always start there. The project directory is for context and coordination only. The actual work of the project may involve creating or modifying files anywhere in the repository or filesystem - the project directory -is not a sandbox. Keep tracking files, design docs, and other context materials here; keep work -products (code, configs, etc.) wherever they naturally belong. +is not a sandbox. Projects may add subdirectories and files beyond the standard set below. The `index.md` Index section should document any bespoke additions so the next agent can find them. Each new project starts with the following files inside `docs/ai/projects/{{name}}`: -- `.prompt`: a file for the user to craft AI prompts. You should not read this file, the user will - paste these prompts into sessions. The user may ask you to write the next prompt, in which case - you should insert the next prompt at the top followed by newlines and `---` to separate it from - what was in the `.prompt` file previously. +- `.prompt`: user-owned prompt scratch file. Never read it. If asked to write the next prompt, + prepend it followed by a `---` separator. - `index.md`: Entry point to the project directory. Treat it like an AGENTS.md file that appends your instruction set beyond this skills instructions. - `log.md`: append to this frequently to retain a record of what has been done, decisions made, @@ -88,19 +85,17 @@ accurate and useful landing place for the next agent. Keep it clean, crisp and a ### `log.md` -Dead simple, append-only log. When writing to the log, get the current date and time with +Dead simple, append-only log. - Starts with a heading: `# {{name}} Log` -- Each entry has a `## YYYY-MM-DD HH:MM` header (24-hour, machine timezone via - `date '+%Y-%m-%d %H:%M'`) +- Each entry has a `## YYYY-MM-DD HH:MM` header (24-hour, machine timezone; get the current date + and time with `date '+%Y-%m-%d %H:%M'`) - Entries are plain prose, no bold formatting, concise but complete, remove unnecessary words - Always append to the bottom (chronological order) - Record: what was done, what was decided (and why), what was discovered, what changed direction - Do not duplicate the agenda (what needs to be done) or the design (current state of the design), just the latest facts -You may read the log to gather context on how the project has progressed. - ### `state.md` This is your file to use so that the next agent knows exactly where we are in the project, what we @@ -109,15 +104,12 @@ information needed in the document: - which milestone or part of the project are we working on - what was done in the previous session - what the goal and instructions are for the next session -- gotchyas or memories from recent sessions that the next agent needs to watch out for +- gotchas or memories from recent sessions that the next agent needs to watch out for ## A Note on Design Docs A plan may have one or more design docs. A design doc is a static snapshot of the current state of -design. It must not record historical artifacts of the design process, such as "the frobulator no -longer frobulates flux capacitors". That is not a design statement, that is a historical note. A -design statement is only what is true about the design currently, not a record of what changed about -the design. Design docs should be kept in sync as the design changes through sessions. +design. Design docs should be kept in sync as the design changes through sessions. ## Invocation @@ -142,7 +134,7 @@ Continue an existing project. Follow the Session Flow below. 1. Read `index.md`, `plan.md`, then `state.md`. Gather additional context as needed. 2. If `state.md` is clear enough about what to do this session, proceed. Otherwise check with the user. -3. Do the work. Write to `log.md` as you go, especially when decisions are being made. +3. Do the work. 4. At session end, follow the Session End Checklist below. ## Session End Checklist diff --git a/.claude/skills/questions/SKILL.md b/.claude/skills/questions/SKILL.md index 31bd459eb..3130345b1 100644 --- a/.claude/skills/questions/SKILL.md +++ b/.claude/skills/questions/SKILL.md @@ -19,8 +19,6 @@ into one turn — not as a numbered list, not as "and also," not as a parentheti user answers one question at a time; batching forces them to scroll back and juggle context, and answers get lost. -This rule holds even when several questions feel related or obvious. One turn, one question. - **Wrong:** > A few things to clarify: diff --git a/AGENTS.md b/AGENTS.md index 2873f97d1..b6fccaa90 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -127,6 +127,10 @@ than the strongly-typed `mx::core` model. No core type may appear in a public ap (`MXAPI_DOUBLES_EQUALS_MEMBER` for doubles) in the type's equality block — a missed line silently drops the field from equality and round-trip checks. +Shaping a new feature's data model? Read `docs/ai/design/api-design-principles.md` first — +mx::api re-shapes MusicXML (stateless, hierarchical, one-fact-one-field); never mirror the +element's raw shape. + ## Quality gates Run `make fmt` to format. `make check` is the clang-format gate **only** — it builds and tests nothing. diff --git a/docs/ai/design/api-design-principles.md b/docs/ai/design/api-design-principles.md new file mode 100644 index 000000000..a24e6704f --- /dev/null +++ b/docs/ai/design/api-design-principles.md @@ -0,0 +1,68 @@ +# mx::api design principles + +mx::api is not "MusicXML minus rare features." It re-shapes MusicXML's information into a model +where invalid or ambiguous documents are hard or impossible to express. MusicXML's recurring +defects are: stateful encodings, flat streams with label fields, one fact stated in two places, +id-linked declarations, and order-dependent meaning. Each principle below counters one. When +adding a feature, do not mirror the element's shape; ask which defect it carries and apply the +matching move. The price of every simplification is paid in `mx::impl`: the writer regenerates +the spec-compliant encoding, the reader accepts every spec-legal variant. + +## 1. Store absolute values, not running state + +If interpreting an element requires remembering an earlier element, resolve it and store the +result on each item. Canonical: `ScoreData::ticksPerQuarter` + `NoteData::tickTimePosition` +replace ``/``/``; the writer synthesizes backup/forward from tick +deltas (`impl/MeasureWriter.cpp`, `writeForwardOrBackupIfNeeded`). Test: could a reader process +your feature's items in isolation, in any order? If not, you have hidden state. + +## 2. Express membership by containment, not by label fields + +When MusicXML tags each element with a number or id (voice, staff, part id), put the element +inside the thing it belongs to. Canonical: `MeasureData` -> staves -> voices map -> notes +(`StaffData.h`, `VoiceData.h`); `PartData` merges the `` declaration and the `` +body, killing the id join. Test: if two fields can disagree about where something belongs, or an +id can dangle, restructure into the tree. + +## 3. One fact, one field; the writer says it twice + +Where MusicXML states a fact in a sound place and a notation place, expose a single api field and +emit both encodings on write. Canonical: `isTieStart`/`isTieStop` produce both `` and +`` (`impl/NoteWriter.cpp`, `addTie`); part-name formatting is written only to the +non-deprecated `*-display` home (`PartData.h` header comment). Test: if a user can construct a +self-contradictory document by setting two api fields, merge them. + +## 4. No neighbor-dependent meaning + +A field on note N must not change meaning based on note N-1. Canonical: `NoteData::isChord` is +true for every chord member; the `` tag's first-note-omitted rule is computed by the +writer (`impl/NoteWriter.cpp`, `setNoteChoiceAndFullNoteGroup`). Test: copy one item out of a +vector -- does it still mean the same thing alone? + +## 5. Denormalize effective state onto the thing it governs + +Each measure/staff carries the attribute state in effect (`timeSignature` with `isImplicit`, +`keys`, `clefs`), even when the source did not restate it; the writer decides where +`` is emitted. Test: can a consumer answer "what is the time signature here?" +without scanning previous measures? + +## 6. Make the common case a plain value; quarantine the rare case + +Do not let an edge case complicate the mainline type -- and do not drop it. Canonical: +`PitchData::alter` is an int, microtones go in a separate `cents` field; the lone +`` is its own optional `TieLetRing` struct, not a third tie boolean +(`NoteData.h`). Test: does the 95% use read as one obvious field, while the 5% is still +expressible? + +## 7. Fidelity knobs must default to "automatic" and be ignorable + +When byte-level round-trip needs a source quirk recorded, add a field whose default applies the +sensible rule; only the reader sets it. Canonical: `ClefData::writeStaffNumber`, +`DurationData::isDurationNameSpecified`, `DirectionData::orderedComponents` ("Do NOT populate" +when authoring). Test: can an author produce correct MusicXML while never touching the knob? +Never let authoring correctness depend on one. Where fidelity and simplification truly conflict, +prefer documented normalization (part-name formatting; output is always MusicXML 4.0) over +complicating the model. + +Presence/absence conventions (`std::optional`, `unspecified` enumerators, the legacy `-1` / +`is...Specified` sentinels) are in AGENTS.md, "mx::api conventions". diff --git a/src/include/mx/api/DurationData.h b/src/include/mx/api/DurationData.h index 824d6ecfd..17f815a61 100644 --- a/src/include/mx/api/DurationData.h +++ b/src/include/mx/api/DurationData.h @@ -73,6 +73,8 @@ struct DurationData bool isDurationNameSpecified; int durationDots; // dots int durationTimeTicks; // length of the note denominated in ticksPerQuarter + // TODO: dead field -- nothing in mx::impl reads or writes it; the tie model lives on + // NoteData::isTieStart/isTieStop. Issue candidate: remove (breaking change). bool isTied; // affects sound only. is the note combined sound-wise with the following note of the same pitch int timeModificationActualNotes; // i.e. for a triplet this would be 3 int timeModificationNormalNotes; // i.e. for a triplet this would be 2 diff --git a/src/include/mx/api/NoteData.h b/src/include/mx/api/NoteData.h index 3a95a6ad5..3066d195f 100644 --- a/src/include/mx/api/NoteData.h +++ b/src/include/mx/api/NoteData.h @@ -118,9 +118,8 @@ class NoteData // tag, but subsequent chord notes do have the tag). bool isChord; - // This is separate from the tie curves themselves. This - // states that the note should be tied but noteAttachment.curve - // items are needed to draw the ties visibly + // One field, two encodings: on write these emit both (sound) and + // (notation), so the two can never contradict each other. bool isTieStart; bool isTieStop; From de532e7a7de472d318a75465af09697f985fa0a1 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Fri, 3 Jul 2026 06:43:36 +0000 Subject: [PATCH 4/8] ci: add path-scoped Copilot review instructions Six .github/instructions/*.instructions.md files gated by applyTo globs: public api headers (presence/absence conventions, equality macros), impl (read/write symmetry, silent-drop and sentinel contracts), generated core (never review or hand-edit generated code), gen (cardinal rules), repo-wide C++ (anonymous namespaces, formatter-owned layout), and corpus/tests (pinned counts, markers, baseline discipline). --- .../instructions/api-headers.instructions.md | 18 +++++++++++++++ .../corpus-and-tests.instructions.md | 19 ++++++++++++++++ .github/instructions/cpp.instructions.md | 15 +++++++++++++ .github/instructions/gen.instructions.md | 22 +++++++++++++++++++ .../generated-core.instructions.md | 12 ++++++++++ .github/instructions/impl.instructions.md | 19 ++++++++++++++++ 6 files changed, 105 insertions(+) create mode 100644 .github/instructions/api-headers.instructions.md create mode 100644 .github/instructions/corpus-and-tests.instructions.md create mode 100644 .github/instructions/cpp.instructions.md create mode 100644 .github/instructions/gen.instructions.md create mode 100644 .github/instructions/generated-core.instructions.md create mode 100644 .github/instructions/impl.instructions.md diff --git a/.github/instructions/api-headers.instructions.md b/.github/instructions/api-headers.instructions.md new file mode 100644 index 000000000..85da11e80 --- /dev/null +++ b/.github/instructions/api-headers.instructions.md @@ -0,0 +1,18 @@ +--- +applyTo: "src/include/mx/api/**" +--- + +These are the public `mx::api` headers: plain-old-data structs that re-shape MusicXML into a +simpler model (see `docs/ai/design/api-design-principles.md`). Review for: + +- No `mx::core` type may appear in a public header. +- New absent-able fields must be `std::optional`. New absent-able enums must instead have an + `unspecified` first enumerator (including uses of the ternary `Bool`). Reject new `-1` + sentinels and new `bool is...Specified` / `has...` flags; do not ask for migration of existing + ones (tracked in issue #249). +- Every new field needs a default value and a matching `MXAPI_EQUALS_MEMBER(field)` line + (`MXAPI_DOUBLES_EQUALS_MEMBER` for doubles) in the type's equality block. A missing line + silently exempts the field from equality and round-trip checks -- flag it as a bug. +- New data must not mirror MusicXML's raw shape when the element is stateful, flat, + duplicated, or id-linked; check the change against the principles doc. +- A new positioned-in-a-measure type needs `int tickTimePosition`; durations are in ticks. diff --git a/.github/instructions/corpus-and-tests.instructions.md b/.github/instructions/corpus-and-tests.instructions.md new file mode 100644 index 000000000..e95be953a --- /dev/null +++ b/.github/instructions/corpus-and-tests.instructions.md @@ -0,0 +1,19 @@ +--- +applyTo: "data/**,src/private/mxtest/**" +--- + +`data/` is the MusicXML test corpus; `src/private/mxtest/` holds the test suites +(see `data/README.md`). Review for: + +- Adding or removing a corpus file requires: the pinned count bump in + `src/private/mxtest/corert/CoreRoundtripTest.cpp`, regenerated audit artifacts + (`make audit`: the `*.features.xml` sidecar and `data/corpus.xml`), and a green + `make test-core-dev`. Flag a corpus change missing any of these. +- A deliberately unparseable file needs a sibling `.invalid` marker explaining why; a + schema-invalid-but-lenient file needs a `.fixup.xml` sidecar. New fixups must follow + the uniform leniency policy in `data/README.md`, not invent per-file rules. +- `roundtrip-baseline.txt` (the api round-trip pass list) only grows by deliberate commits; + flag removals or unexplained churn. +- `ApiLoadSmokeTest` only proves a file imports without crashing; a feature claim needs a + fixture pinned in the read->write->read gate (`make test-api-roundtrip`). +- Test code follows the same C++ rules as the library (no new anonymous namespaces). diff --git a/.github/instructions/cpp.instructions.md b/.github/instructions/cpp.instructions.md new file mode 100644 index 000000000..ce96d8884 --- /dev/null +++ b/.github/instructions/cpp.instructions.md @@ -0,0 +1,15 @@ +--- +applyTo: "src/**/*.cpp,src/**/*.h" +--- + +Repo-wide C++ rules (see AGENTS.md): + +- No new anonymous namespaces (`namespace { }`) -- they break unity builds. File-local helpers + get named functions (`tokenIsNameChar`); file-local constants get a per-type prefix + (`kYesNoWire`). Existing anonymous namespaces are being retired as touched; do not demand + drive-by fixes. +- Formatting is enforced by clang-format (`make fmt` / `make check`); do not comment on layout + the formatter controls. +- Keep source ASCII. +- Comments should state constraints and reasons the code cannot express; flag comments that + merely restate the code. diff --git a/.github/instructions/gen.instructions.md b/.github/instructions/gen.instructions.md new file mode 100644 index 000000000..f84567fb4 --- /dev/null +++ b/.github/instructions/gen.instructions.md @@ -0,0 +1,22 @@ +--- +applyTo: "gen/**" +--- + +`gen/` is a Python code generator (XSD -> IR -> Plates -> Mustache press) with four targets: +C++ (`gen/cpp`, the product), Go, C, JSON Schema. Review against its cardinal rules +(`gen/AGENTS.md`): + +- Language-agnosticism: no language knowledge in any `*.py` outside a target's own directory. + Reject language tables, per-language branches, or target-name checks in generator Python; + language facts belong in the target's `config.toml` and `templates/`. +- The IR is a pure function of the XSD: no configurable knobs in `gen/ir/` (the `sounds.xml` + fold is the one documented exception). +- Templates stay dumb: naming/structural decisions belong in Plates or press context, not in + template text. The Mustache engine has no expressions or filters -- reject attempts to add + logic to it. +- Fail loud: new error paths must exit non-zero with a `template:line` style message, never + emit best-effort output. +- Generated output is committed: a generator change without its regenerated output (or vice + versa) fails `make test-gen` drift checking. +- Template identifiers must be unity-build safe: use `{{ident}}`-suffixed names for + file-local symbols. diff --git a/.github/instructions/generated-core.instructions.md b/.github/instructions/generated-core.instructions.md new file mode 100644 index 000000000..63746bcbb --- /dev/null +++ b/.github/instructions/generated-core.instructions.md @@ -0,0 +1,12 @@ +--- +applyTo: "src/private/mx/core/generated/**" +--- + +Everything in this directory is GENERATED by `gen/` (`make gen-cpp`) from the MusicXML XSD. + +- Do not review style, naming, or repetition here; the generator owns those. +- A diff in these files must be accompanied by a change under `gen/` (templates, config, or + generator code) that produces it. Flag any hand edit -- it will be destroyed on the next + regeneration, and CI (`make test-gen`) fails on regeneration drift. +- Review the paired `gen/` change instead, and check the generated diff only for evidence of an + unintended blast radius (e.g. an edit meant for one type changing hundreds of files). diff --git a/.github/instructions/impl.instructions.md b/.github/instructions/impl.instructions.md new file mode 100644 index 000000000..6d7a4320f --- /dev/null +++ b/.github/instructions/impl.instructions.md @@ -0,0 +1,19 @@ +--- +applyTo: "src/private/mx/impl/**" +--- + +`mx::impl` translates between `mx::core` (generated MusicXML model) and `mx::api` (public +structs). Review for: + +- Read/write symmetry: a feature read in a `*Reader` must also be written by the matching + `*Writer`, and vice versa. A one-sided change silently drops data on round-trip -- flag it. +- Silent data loss: an empty `case`/`break`, a value read but never stored, or a field never + consulted on write is only acceptable with a comment saying the drop is intentional. +- Sentinel contracts must hold on both sides (e.g. time-modification 1/1 means "absent"; + `-1` means "unspecified"). A writer emitting a sentinel or a reader failing to produce one + breaks round-trip. +- `MX_DEBUG_THROW` is a no-op in release builds; it must not be the only guard on a path that + would otherwise write invalid or truncated MusicXML. +- Tick time: positions are absolute (`tickTimePosition` against `ScoreData::ticksPerQuarter`); + writers must synthesize ``/``/``, never expect callers to. +- No new anonymous namespaces (unity-build rule; use named helpers / `k`-prefixed constants). From 48c2d254ea8f11090fbab02393185952d8d4d22f Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Fri, 3 Jul 2026 06:44:54 +0000 Subject: [PATCH 5/8] docs: inline api design-principles digest into AGENTS.md Agents reliably read AGENTS.md (SessionStart hook) but often skip linked docs; carry the seven rules inline, keep the full doc for examples and per-rule tests. --- AGENTS.md | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b6fccaa90..688b36a2c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -127,9 +127,18 @@ than the strongly-typed `mx::core` model. No core type may appear in a public ap (`MXAPI_DOUBLES_EQUALS_MEMBER` for doubles) in the type's equality block — a missed line silently drops the field from equality and round-trip checks. -Shaping a new feature's data model? Read `docs/ai/design/api-design-principles.md` first — -mx::api re-shapes MusicXML (stateless, hierarchical, one-fact-one-field); never mirror the -element's raw shape. +### Design principles (digest) + +Never mirror a MusicXML element's raw shape; counter its defect. Full text with examples and +per-rule tests: `docs/ai/design/api-design-principles.md`. + +1. Store absolute values, not running state (ticks, not divisions/backup/forward). +2. Membership by containment, not label fields (measure -> staves -> voices -> notes). +3. One fact, one field; the writer emits both encodings (tie/tied). +4. No neighbor-dependent meaning (`isChord` is true on every chord member). +5. Denormalize effective state onto what it governs (each measure carries its time signature). +6. Common case a plain value; quarantine the rare case (`alter` int + `cents` double). +7. Fidelity knobs default to "automatic" and must be ignorable when authoring. ## Quality gates From 5b201d304c866d0109dda7fdd975145e5d545ed1 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Fri, 3 Jul 2026 08:38:46 +0000 Subject: [PATCH 6/8] chore: adopt mx- prefix for all repo skill names arch -> mx-architecture, grill-me -> mx-grill-me, open-mx-issue -> mx-open-issue, open-mx-pr -> mx-open-pr, project -> mx-project, questions -> mx-questions. Cross-references updated; open-issue/open-pr descriptions gain trigger phrases (report a bug, submit as PR). AGENTS.md layout notes the convention. --- .claude/skills/mx-api-add-feature/SKILL.md | 2 +- .../skills/mx-api-classify-roundtrip-failures/SKILL.md | 2 +- .claude/skills/{arch => mx-architecture}/SKILL.md | 2 +- .claude/skills/{grill-me => mx-grill-me}/SKILL.md | 4 ++-- .../skills/{open-mx-issue => mx-open-issue}/SKILL.md | 6 +++--- .claude/skills/{open-mx-pr => mx-open-pr}/SKILL.md | 6 +++--- .claude/skills/{project => mx-project}/SKILL.md | 2 +- .claude/skills/{questions => mx-questions}/SKILL.md | 10 +++++----- AGENTS.md | 1 + 9 files changed, 18 insertions(+), 17 deletions(-) rename .claude/skills/{arch => mx-architecture}/SKILL.md (99%) rename .claude/skills/{grill-me => mx-grill-me}/SKILL.md (89%) rename .claude/skills/{open-mx-issue => mx-open-issue}/SKILL.md (95%) rename .claude/skills/{open-mx-pr => mx-open-pr}/SKILL.md (96%) rename .claude/skills/{project => mx-project}/SKILL.md (99%) rename .claude/skills/{questions => mx-questions}/SKILL.md (84%) diff --git a/.claude/skills/mx-api-add-feature/SKILL.md b/.claude/skills/mx-api-add-feature/SKILL.md index f351fa262..01909994f 100644 --- a/.claude/skills/mx-api-add-feature/SKILL.md +++ b/.claude/skills/mx-api-add-feature/SKILL.md @@ -69,4 +69,4 @@ Run `make test` for the unit suites and `make test-api-roundtrip` for the corpus ## (optional) Step 5: Open a PR -If the user asks for a PR, use the `open-mx-pr` skill. +If the user asks for a PR, use the `mx-open-pr` skill. diff --git a/.claude/skills/mx-api-classify-roundtrip-failures/SKILL.md b/.claude/skills/mx-api-classify-roundtrip-failures/SKILL.md index e9f924fa6..3c589b322 100644 --- a/.claude/skills/mx-api-classify-roundtrip-failures/SKILL.md +++ b/.claude/skills/mx-api-classify-roundtrip-failures/SKILL.md @@ -144,4 +144,4 @@ landscape, not a regression. ## Hand-off Fixes (if requested) - To fix a dropped/under-supported element or a crash: use the `mx-api-add-feature` skill. -- The findings belong under the tracking issue #208; file specifics with the `open-mx-issue` skill. +- The findings belong under the tracking issue #208; file specifics with the `mx-open-issue` skill. diff --git a/.claude/skills/arch/SKILL.md b/.claude/skills/mx-architecture/SKILL.md similarity index 99% rename from .claude/skills/arch/SKILL.md rename to .claude/skills/mx-architecture/SKILL.md index 8e6cd49fd..2f15d30fb 100644 --- a/.claude/skills/arch/SKILL.md +++ b/.claude/skills/mx-architecture/SKILL.md @@ -1,5 +1,5 @@ --- -name: arch +name: mx-architecture description: Prime the model with senior architect principles during design or coding. Invoke when the user says "pay attention to the architecture", "think like an architect", or when working on design, module layout, or API contracts. --- # Architect Mode diff --git a/.claude/skills/grill-me/SKILL.md b/.claude/skills/mx-grill-me/SKILL.md similarity index 89% rename from .claude/skills/grill-me/SKILL.md rename to .claude/skills/mx-grill-me/SKILL.md index da0fd8e78..d275912d8 100644 --- a/.claude/skills/grill-me/SKILL.md +++ b/.claude/skills/mx-grill-me/SKILL.md @@ -1,5 +1,5 @@ --- -name: grill-me +name: mx-grill-me description: > Interview the user relentlessly about a plan or design until reaching shared understanding, resolving each branch of the decision tree. Use @@ -21,5 +21,5 @@ the codebase instead. For each question, provide your recommended answer. -Use the /questions skill to avoid sending more than one question at +Use the /mx-questions skill to avoid sending more than one question at once. diff --git a/.claude/skills/open-mx-issue/SKILL.md b/.claude/skills/mx-open-issue/SKILL.md similarity index 95% rename from .claude/skills/open-mx-issue/SKILL.md rename to .claude/skills/mx-open-issue/SKILL.md index 5a267cf54..1547e4763 100644 --- a/.claude/skills/open-mx-issue/SKILL.md +++ b/.claude/skills/mx-open-issue/SKILL.md @@ -1,7 +1,7 @@ --- -name: open-mx-issue +name: mx-open-issue description: > - Use this skill when the user says open an issue, create an issue, or file a new issue. + Use this skill when the user says open an issue, create an issue, file a new issue, or report a bug. argument-hint: "" disable-model-invocation: false user-invocable: true @@ -17,7 +17,7 @@ allowed-tools: > WebSearch, Write --- -# /open-mx-issue +# /mx-open-issue Use `` to understand what the issue is about. Check to see if there is documentation on the issue available in `docs/ai/api-feature-audit.md` (which might be the case if it was previously diff --git a/.claude/skills/open-mx-pr/SKILL.md b/.claude/skills/mx-open-pr/SKILL.md similarity index 96% rename from .claude/skills/open-mx-pr/SKILL.md rename to .claude/skills/mx-open-pr/SKILL.md index 3263e2790..d8df75bab 100644 --- a/.claude/skills/open-mx-pr/SKILL.md +++ b/.claude/skills/mx-open-pr/SKILL.md @@ -1,7 +1,7 @@ --- -name: open-mx-pr +name: mx-open-pr description: > - Use this skill when the user says open a PR, create a pull request, or make a PR. + Use this skill when the user says open a PR, create a pull request, make a PR, or submit this work as a PR. argument-hint: "" disable-model-invocation: false user-invocable: true @@ -17,7 +17,7 @@ allowed-tools: > WebSearch, Write --- -# /open-mx-pr +# /mx-open-pr Use `` and your context to understand what the PR is about. Check to see if there is documentation on the issue available in `docs/ai/api-feature-audit.md` (which might be the case if diff --git a/.claude/skills/project/SKILL.md b/.claude/skills/mx-project/SKILL.md similarity index 99% rename from .claude/skills/project/SKILL.md rename to .claude/skills/mx-project/SKILL.md index b66a23f7b..c4b112573 100644 --- a/.claude/skills/project/SKILL.md +++ b/.claude/skills/mx-project/SKILL.md @@ -1,5 +1,5 @@ --- -name: project +name: mx-project description: > Create or continue multi-session projects that persist state across agent context windows. Use when starting a new project, picking up where the last session left off, or when the user diff --git a/.claude/skills/questions/SKILL.md b/.claude/skills/mx-questions/SKILL.md similarity index 84% rename from .claude/skills/questions/SKILL.md rename to .claude/skills/mx-questions/SKILL.md index 3130345b1..d6d7ac097 100644 --- a/.claude/skills/questions/SKILL.md +++ b/.claude/skills/mx-questions/SKILL.md @@ -1,11 +1,11 @@ --- -name: questions +name: mx-questions description: > Ask the user clarifying questions one at a time to refine a plan or - task. Invoke with `/questions` or automatically when more information + task. Invoke with `/mx-questions` or automatically when more information is needed. --- -# /questions +# /mx-questions ## Non-negotiable: plain chat only @@ -37,8 +37,8 @@ answers get lost. ## Usage -- `/questions` -- `/questions ` — e.g., `/questions about the design of the flubber async module` +- `/mx-questions` +- `/mx-questions ` — e.g., `/mx-questions about the design of the flubber async module` ## Flow diff --git a/AGENTS.md b/AGENTS.md index 688b36a2c..0f601285b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -13,6 +13,7 @@ targets). Everything runs in the `mx-sdk` Docker image unless `MX_RUNNING_IN_DOC ``` mx/ AGENTS.md <- you are here + .claude/skills/ <- agent skills; every skill name starts with mx- Makefile <- top-level build driver Dockerfile <- mx-sdk image with toolchains and dev tools CMakeLists.txt <- C++ project From 926a413859549f06d3fdd57a81ae758fb119fa94 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Fri, 3 Jul 2026 12:31:24 +0000 Subject: [PATCH 7/8] ci: name jobs by what they run Job display names now state the suites executed (gen gates, api mxtest + round-trip, core corert/unit/xmllint/probes, Go+C corert targets, coverage, macOS core+api, swift build-only); the coverage and macos names previously claimed core-only while also running api suites. Job keys are unchanged. Comments note what each job deliberately skips. --- .github/workflows/ci.yaml | 68 +++++++++++++++++++++++---------------- 1 file changed, 41 insertions(+), 27 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 270ba4096..220c9ddde 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -20,9 +20,14 @@ concurrency: env: DOCKER_VOLUME: mx-build +# Every Linux job runs its make targets inside the pinned mx-sdk Docker +# toolchain; only the two macOS jobs build natively. jobs: + # Python-side gates for the gen/ generator and audit tool, plus the repo's + # only clang-format gate. Also runs plates --check for every target. No C++ + # is compiled or tested here. linux-gen: - name: Linux (generator gates) + name: "gen: unit/audit tests + regen drift + quality/lint + fmt-check (Linux)" runs-on: ubuntu-latest # Read-only token: this job builds untrusted fork PR code, so it never @@ -60,13 +65,13 @@ jobs: key: ccache-${{ runner.os }}-gen-${{ github.sha }} restore-keys: ccache-${{ runner.os }}-gen- - - name: fmt-check + - name: fmt-check (clang-format, all C++ under src/) run: make check - - name: Generator tests + - name: gen unit tests (parser/IR/plates/press) run: make test-gen - - name: Audit tests + - name: audit tool unit tests (incl. failure classifier) run: make test-audit - name: plates --check (all targets) @@ -122,8 +127,10 @@ jobs: name: pr-comment-gen path: pr-comment/ + # The mx::api/mx::impl product surface. Core-layer suites (corert, unit, + # xmllint, probes) run in linux-core, not here. linux-api: - name: Linux (mx::api/mx::impl) + name: "api: mxtest suite + examples + corpus round-trip (Linux)" runs-on: ubuntu-latest steps: @@ -149,14 +156,16 @@ jobs: - name: Build mx library run: make lib - - name: Run examples + mxtest suite (api/impl/file/control) + - name: mxtest suite (api/impl/file/control) + examples run: make test - - name: Run corpus api roundtrip (regression mode) + - name: api corpus round-trip (regression mode) run: make test-api-roundtrip + # The strongly-typed mx::core layer. Note: check-core-dev (warning-free + # build gate) is a local-only gate; CI does not run it. linux-core: - name: Linux (mx::core gates) + name: "core: corert round-trip + unit + xmllint + compile probes (Linux)" runs-on: ubuntu-latest steps: @@ -180,20 +189,21 @@ jobs: restore-keys: ccache-${{ runner.os }}-core- # corert file counts are pinned by an in-suite counts case (see CoreRoundtripTest.cpp). - - name: Core roundtrip suite + - name: corert round-trip suite run: make test-core-dev - - name: Core unit tests + - name: mx::core unit tests run: make test-cpp-unit - - name: validate-cpp (serialize + xmllint) + - name: validate-cpp (serialize corpus + xmllint vs XSD) run: make validate-cpp - name: probe-cpp (must-not-compile probes) run: make probe-cpp + # corert round-trip suites for the generator's emitted Go and C targets. linux-targets: - name: Linux (Go + C targets) + name: "corert: Go + C generated targets (Linux)" runs-on: ubuntu-latest steps: @@ -215,8 +225,11 @@ jobs: - name: C corert suite run: make test-c + # Re-runs the core (corert + unit) and api (mxtest + round-trip) suites in + # instrumented builds to produce gcovr reports. Coverage % is advisory (no + # floor), but a test failure still fails the job. coverage: - name: Linux (coverage-core-dev) + name: "coverage: core + api gcovr reports, advisory (Linux)" runs-on: ubuntu-latest # Read-only token: posting the coverage comment is delegated to the @@ -229,9 +242,7 @@ jobs: - uses: actions/checkout@v4 # Same container-driver + runtime-token setup as linux-gen so the - # coverage stage reuses the warm toolchain layer cache. Coverage runs in - # the pinned container via `make coverage-core-dev`; it is advisory (no - # gate) and runs in parallel with the other jobs. + # coverage stage reuses the warm toolchain layer cache. - uses: docker/setup-buildx-action@v3 - uses: crazy-max/ghaction-github-runtime@v3 @@ -251,10 +262,10 @@ jobs: key: ccache-${{ runner.os }}-coverage-${{ github.sha }} restore-keys: ccache-${{ runner.os }}-coverage- - - name: Build + run with coverage (core) + - name: coverage-core-dev (corert + unit, instrumented) run: make coverage-core-dev - - name: Build + run with coverage (api/impl/utility) + - name: coverage-api (mxtest + round-trip, instrumented) run: make coverage-api # Turn gcovr's 3-line summary (data/testOutput/coverage/summary.txt, e.g. @@ -332,14 +343,16 @@ jobs: name: pr-comment-coverage path: pr-comment/ + # Skips validate-cpp (xmllint) and probe-cpp; those run Linux-only in + # linux-core. No gen/audit gates here either. macos: - name: macOS (mx::core, AppleClang) + name: "core + api: corert + unit + mxtest + round-trip (macOS/AppleClang)" runs-on: macos-latest # No Docker on the macOS runners: MX_RUNNING_IN_DOCKER=1 flips the # Makefile to its direct-invocation branch so the build runs natively - # with AppleClang. This is the only job off the pinned toolchain, by - # design: it exists to find (and keep finding) AppleClang fallout. + # with AppleClang. This is the only C++ job off the pinned toolchain, + # by design: it exists to find (and keep finding) AppleClang fallout. env: MX_RUNNING_IN_DOCKER: 1 @@ -354,29 +367,30 @@ jobs: restore-keys: | ${{ runner.os }}-core-dev- - - name: Core roundtrip suite + - name: corert round-trip suite run: make test-core-dev - - name: Core unit tests + - name: mx::core unit tests run: make test-cpp-unit - name: Build mx library (api/impl) run: make lib - - name: Run examples + mxtest suite (api/impl) + - name: mxtest suite (api/impl/file/control) + examples run: make test - - name: Run corpus api roundtrip (regression mode) + - name: api corpus round-trip (regression mode) run: make test-api-roundtrip swift-local: - name: macOS (Swift package, local checkout) + name: "swift: Mx package build only (macOS/SPM)" runs-on: macos-latest # By default mx builds `Mx` as a source library straight from this checkout -- # the path komp takes when it depends on a sibling mx via `.package(path:)`. # Building it here proves mx compiles as a Swift package under AppleClang/SPM on - # every PR. The binary xcframework release arm (MX_BINARY_RELEASE) is follow-up work. + # every PR; it runs no test suites. The binary xcframework release arm + # (MX_BINARY_RELEASE) is follow-up work. steps: - uses: actions/checkout@v4 From 9be44517295e1f7c47ab5fb0b4b6bbcac2d1a706 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Sat, 4 Jul 2026 06:28:50 +0000 Subject: [PATCH 8/8] docs: apply review suggestions - mx-open-pr template gains a Human Summary section for the maintainer to fill in - api-headers review instructions: comments should serve humans and coding agents, crisp and focused on the surprising - cpp review instructions: comments state constraints and effects hard to ascertain from the code --- .claude/skills/mx-open-pr/SKILL.md | 4 ++++ .github/instructions/api-headers.instructions.md | 2 ++ .github/instructions/cpp.instructions.md | 3 +-- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/.claude/skills/mx-open-pr/SKILL.md b/.claude/skills/mx-open-pr/SKILL.md index d8df75bab..2072f7ca1 100644 --- a/.claude/skills/mx-open-pr/SKILL.md +++ b/.claude/skills/mx-open-pr/SKILL.md @@ -53,6 +53,10 @@ a lot of backtick formatting as well. Template/sample: ```markdown +## Human Summary + +TODO: human writes here + ## Summary `accidentalMap` in `Converter.cpp` was missing the entry for `sharpSharp`. Added a single item to the map so that the diff --git a/.github/instructions/api-headers.instructions.md b/.github/instructions/api-headers.instructions.md index 85da11e80..59a500f93 100644 --- a/.github/instructions/api-headers.instructions.md +++ b/.github/instructions/api-headers.instructions.md @@ -16,3 +16,5 @@ simpler model (see `docs/ai/design/api-design-principles.md`). Review for: - New data must not mirror MusicXML's raw shape when the element is stateful, flat, duplicated, or id-linked; check the change against the principles doc. - A new positioned-in-a-measure type needs `int tickTimePosition`; durations are in ticks. +- Comments should be helpful to humans and coding agents. Crisp, not verbose, explain how the + code works, especially anything surprising. diff --git a/.github/instructions/cpp.instructions.md b/.github/instructions/cpp.instructions.md index ce96d8884..785c2868f 100644 --- a/.github/instructions/cpp.instructions.md +++ b/.github/instructions/cpp.instructions.md @@ -11,5 +11,4 @@ Repo-wide C++ rules (see AGENTS.md): - Formatting is enforced by clang-format (`make fmt` / `make check`); do not comment on layout the formatter controls. - Keep source ASCII. -- Comments should state constraints and reasons the code cannot express; flag comments that - merely restate the code. +- Comments should state constraints and effects hard to ascertain from the code.