docs: audit agent instructions and skills for accuracy and brevity

.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.
 
@@ -49,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
 
@@ -72,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 you to open a PR, see ./resources/pr-template.md
+If the user asks for a PR, use the `mx-open-pr` skill.

.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 <dataRoot>`: 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 <tab> relpath <tab> 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 `<element>.<musicxml-version>.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.

.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<CORE_TYPE, API_TYPE>`,
 which is a `std::vector<std::pair<CORE_TYPE, API_TYPE>>` 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
 
@@ -52,37 +51,13 @@ 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
-
-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}
-    {
-    }
-};
-```
+Optionality (see "mx::api conventions" in AGENTS.md): new absent-able fields use
+`std::optional<T>` (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.
 
 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 +85,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
@@ -147,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<T>`; 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
@@ -179,7 +153,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.

.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. `<segno>` 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 `<chord>` 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.