spec: allow idx: 0 for a pickup measure (1.19.0) - #65
Conversation
§7.6 defined a `pickup` (anacrusis) measure flag, but notation.schema.json required `idx >= 1`, so a pickup measure had no valid number: the conventional 0 was rejected, and 1 collided with the first full measure and shifted every later measure against the printed score. The flag was unusable as specified. - schemas/notation.schema.json: relax measures[].idx to `minimum: 0`, and add a one-directional conditional — idx 0 requires pickup:true, so 0 cannot become a general off-by-one, while a pickup MAY still carry the number its source gives it. - spec/feedpak-v1.md §7.6: state the numbering rule; define a pickup's `t` as its first sounded beat (it has no downbeat); state how the timeline represents an anacrusis (upbeats are `measure: -1`, the first downbeat is measure 1); record what integer `idx` deliberately cannot express (mid-score implicit/partial measures, "7a"-style numbering) so the reserved 0 is not overloaded later. - examples/extended.feedpak: the pack now opens with a keys pickup measure, exercised coherently across every sibling time stream — all event times shift with the grid, apply-until-next maps (tempos, time signatures, keys, harmony, sections, first anchors) keep their 0.0 anchor so the pickup region stays covered, and song_timeline shows the upbeat-then-downbeat shape. Closes the coverage gap that let this survive every release since 1.0.0: `pickup` appeared in no example and no test. - tests: idx 0 with pickup passes; idx 0 with the flag absent or false fails (location + mechanism pinned); pickup with a source-given idx 1 still passes; negative idx still fails on the minimum bound. - version bump to 1.19.0 across spec header (incl. date), §4.1, README table + citation, the extended example manifest, and CHANGELOG. Compatibility: purely a relaxation — every package valid under 1.18.0 stays valid. Not provably zero-impact for readers that assumed idx >= 1, but no conformant pack could ever have used idx 0, so no such corpus exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: gionnibgud <gionnibgud@gmail.com>
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughChangesPickup measure support
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@spec/feedpak-v1.md`:
- Around line 200-203: Update the compatibility policy for the new
measures[].idx value 0: in spec/feedpak-v1.md lines 200-203, explicitly document
it as an opt-in carve-out because older readers may reject it; in CHANGELOG.md
lines 13-18, qualify the “Additive (MINOR)” classification with this reader
limitation; and in CHANGELOG.md lines 41-48, replace the “no reader behaviour
changes” and “risk is nil” claims with the actual compatibility boundary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b10b7dd-65df-494b-92b4-4ea07426e1f6
📒 Files selected for processing (19)
CHANGELOG.mdREADME.mdexamples/extended.feedpak/arrangements/bass.jsonexamples/extended.feedpak/arrangements/lead.jsonexamples/extended.feedpak/arrangements/notation_keys.jsonexamples/extended.feedpak/drum_tab.jsonexamples/extended.feedpak/drum_tab_live.jsonexamples/extended.feedpak/harmony.jsonexamples/extended.feedpak/keys.jsonexamples/extended.feedpak/lyrics.jsonexamples/extended.feedpak/lyrics_ja.jsonexamples/extended.feedpak/lyrics_romaji.jsonexamples/extended.feedpak/manifest.yamlexamples/extended.feedpak/song_timeline.jsonexamples/extended.feedpak/vocal_pitch.jsonexamples/extended.feedpak/vocal_pitch_contour.jsonschemas/notation.schema.jsonspec/feedpak-v1.mdtests/test_validate.py
Review follow-up: the previous wording covered only the backward direction (existing packs stay valid). State the forward direction plainly — a pre-1.19 Reader that re-validates against its old bundled schema rejects an idx-0 pack rather than degrading — and name the Writer mitigation (a pickup MAY keep a source-given number until 1.19-aware Readers are common). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: gionnibgud <gionnibgud@gmail.com>
|
@coderabbitai Addressed in c286c48, with one part deliberately deferred: Taken — the Compatibility section now states the two directions separately instead of blending them: existing packages (pure relaxation, no corpus of Deferred — adding this to the §4.2 opt-in carve-out list. That list is currently defined for encoding relaxations ("a data file's text, or a stem's audio codec"), and widening a schema value domain is a different class; whether §4.2 should grow a third category is a spec-philosophy call for the maintainers, and the MINOR classification itself was already ruled in #64. Happy to add it if @topkoa wants it in that list. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline c286c48
git show --format=fuller --find-renames -- spec/feedpak-v1.md c286c48
printf '\n--- Compatibility / §4.2 references in current specification ---\n'
rg -n -i -C 8 'compatib|opt-in|encoding relax|schema value|idx:?\s*0|pre-1\.19|older reader' spec/feedpak-v1.mdLength of output: 29842
Deferring §4.2 is reasonable: its current carve-out is expressly limited to opt-in file-encoding relaxations, while ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
topkoa
left a comment
topkoa
left a comment
There was a problem hiding this comment.
Approving. Verified by execution, not just reading: on this branch locally, check_versions.py is consistent at 1.19.0, both example packs validate, all 63 tests pass, and the original #64 reproducer pack — which fails on main — passes unmodified.
The schema fix is exactly as accepted in #64, and the test matrix covers the case the issue didn't spell out (idx: 0 with pickup: false, not just absent) plus the one-directionality proof. I traced the example's +0.5s shift value-by-value across all 13 files: every event stream shifts exactly +0.5, the apply-until-next maps and first anchors correctly keep 0.0, duration matches, and the timeline renumbering matches the new §7.6 prose — using the already-established measure: -1 convention rather than inventing one. The spec additions beyond the issue's text (pickup t = first sounded beat, the timeline shape, the mid-score numbering out-of-scope MUST) settle exactly the questions an implementer would otherwise guess at, and the two-direction compatibility section is more honest than what was asked for. Both dates are right this time, too.
Noted and verified the release-on-merge warning against release.yml — merging this publishes v1.19.0, so it'll be merged when we intend to release. LGTM.
|
@gionnibgud On the deferred §4.2 question — decision: add it to the carve-out list, in this PR. The encoding-vs-value-domain distinction is real but immaterial to what the carve-out exists for: "a pack that opts in may be rejected by an older Reader with a clear error" describes Concretely:
My approval stands for the PR as-is plus this addition — no re-review needed for it, just re-run the local battery ( |
Per maintainer decision on PR #65: broaden the carve-out's framing from encoding-only relaxations to opt-in relaxations generally (encoding or schema value domain), add idx 0 as entry 3 with the Reader-MAY-reject boundary and the Writer mitigation, and cross-reference the carve-out from the §7.6 numbering paragraph and the CHANGELOG Compatibility section so the two statements of the same boundary point at each other. Consistency sweep for the rename (the carve-out is no longer file-format- only): the §4.2 Reader-rule exception and §8's .jsonc note now say "opt-in relaxation"; the §4.1 narrative for 1.19.0 references the carve-out the same way 1.9.0's does; the justification paragraph maps each of the three relaxations to the Reader it requires, states the scoping test as "narrowly scoped" (per-file or per-value), and names the pickup mitigation alongside the .jsonc one. Historical CHANGELOG entries keep the old name they shipped with. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: gionnibgud <gionnibgud@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
13-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify that v1.19.0 adds conventional
idx: 0, not numbering support for the first time.The changelog later states that
pickup: truewithidx: 1was already valid, so describing this as the first conformant way to number or encode an anacrusis is misleading. Please scope the claim specifically to the reservedidx: 0representation.Proposed wording
-Additive (MINOR) release: a **pickup measure can finally be numbered**. Defect `#64`. +Additive (MINOR) release: a **pickup measure can now use reserved index 0**. Defect `#64`. ... -The flag was unusable as specified; it shipped with no example and no +The conventional index-0 numbering was unavailable; it shipped with no example and no ... -Writers gain the first conformant way to encode an anacrusis. +Writers gain the first conformant way to encode an anacrusis using reserved index 0.Also applies to: 50-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` around lines 13 - 18, Revise the v1.19.0 changelog entry to state that the release adds support for the conventional reserved pickup-measure representation idx: 0. Avoid claiming this is the first way to number or encode anacrusis measures, and preserve the existing context about pickup: true with idx: 1 already being valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 13-18: Revise the v1.19.0 changelog entry to state that the
release adds support for the conventional reserved pickup-measure representation
idx: 0. Avoid claiming this is the first way to number or encode anacrusis
measures, and preserve the existing context about pickup: true with idx: 1
already being valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 423a9deb-7be7-4a53-ae92-0a04a288ed77
📒 Files selected for processing (2)
CHANGELOG.mdspec/feedpak-v1.md
Review follow-up: pickup:true with a source-given idx >= 1 was always schema-valid, so 'the flag was unusable' and 'first conformant way to encode an anacrusis' overclaimed. What 1.19.0 actually adds is the conventional idx 0 representation — encoding an anacrusis without renumbering it against the printed score. Also rewraps the one overlong line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: gionnibgud <gionnibgud@gmail.com>
|
@coderabbitai Taken in e387f8a — the headline and closing line now scope the claim to the conventional |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
topkoa
left a comment
topkoa
left a comment
There was a problem hiding this comment.
Re-approving (the earlier approval was auto-dismissed by the two follow-up pushes). Re-reviewed everything since c286c48:
7014c39 — §4.2 carve-out. Matches the decision comment point for point:
- Framing broadened from file-format relaxations to opt-in relaxations generally ("how a file is encoded … or of a schema value domain"), with the §4.2 Reader-rule wording and the §8
.jsoncnote updated to match — no dangling references to the old narrower phrase. - Entry 3 carries exactly the requested content: a pre-1.19.0 Reader validating notation against its older schema MAY reject a pack using
idx: 0, with a clear error; a Writer needing the broadest Reader range keeps a source-givenidx >= 1withpickup: true. - Cross-references land in both directions: §7.6's numbering paragraph points at the carve-out, and the CHANGELOG Compatibility section names it ("entry 3 there").
- The justification paragraph was reworked coherently rather than just appended to — "per-file for the first two, per-value for the third", and the future-relaxation rule now reads "opt-in and equally narrowly scoped", which correctly generalizes the old "per-file" constraint.
e387f8a — headline scoping. "Carry its conventional number" / "first way to encode an anacrusis without renumbering it against the printed score" — accurate now that pickup: true + source-given idx >= 1 is acknowledged as always-valid. CodeRabbit's confirmation matches my reading.
CI: all checks green on head, including docs (mkdocs --strict resolves the new §4.2 anchors) and links.
One optional nit, take or leave: the CHANGELOG line "the only schema-valid option, 1" is slightly loose — any idx >= 1 was schema-valid; 1 is just the only choice that doesn't invent a gap. Not worth a push on its own; fold in only if something else comes up.
As before: merging publishes v1.19.0 — merge when you intend to release.
2 participants
Fixes #64, implementing the accepted proposal with the three review asks from that thread.
The fix
schemas/notation.schema.json—measures[].idxrelaxed tominimum: 0, plus a one-directional conditional:idx: 0⇒pickup: true, so0can't become a general off-by-one, while a pickup MAY still carry the number its source gives it (as discussed in §7.6: apickupmeasure has no valididx— schema requiresidx >= 1, but an anacrusis is numbered 0 #64).spec/feedpak-v1.md§7.6 — states the numbering rule, and defines the semantics the flag needed around it: a pickup'stis its first sounded beat (it has no downbeat), and insong_timeline.jsonan anacrusis is represented as upbeats (measure: -1) withmeasure: 1on the first full measure's downbeat — the timeline numbers downbeats only, so it needs no0and no second schema change.The three asks from #64
examples/extended.feedpaknow opens with a keys pickup measure, and it's exercised coherently across the whole pack: every sibling time stream shifts with the grid (values-only diff), while apply-until-next maps (tempos, time signatures, keys, harmony, sections, first anchors) keep their0.0anchor so the pickup region stays covered. Timeline downbeats, the notation's measure grid, the 6/8 meter change on the barline, and the keys/lead/vocal melody doubling all stay aligned. Tests:idx: 0+pickuppasses;idx: 0with the flag absent or explicitly false fails (location + mechanism pinned);pickup: truewith a source-givenidx: 1still passes (proves one-directionality); negativeidxstill fails on the minimum bound.idxand0MUST NOT be reused for them — stated as a Writer rule the schema alone cannot check, like this section's other Writer MUSTs.idx >= 1; in practice nil because no conformant pack could ever have usedidx: 0) rather than asserting zero impact.Verification
Full CI battery run locally: schema check (11/11), examples in directory and zip form, 63 tests, ruff,
check_versions.py(consistent at 1.19.0),gen_docs.py+mkdocs build --strict. Also verified against the real-world MusicXML-imported pack from #64 — it validates unmodified with this branch.A heads-up rather than a request:
release.ymlcuts the tag + GitHub Release from the newest dated CHANGELOG heading on push tomain, andcheck_versions.pyrequires that dated heading in the PR — so merging this publishes v1.19.0 immediately. Merge when you intend to release; happy to restructure if you'd rather stage it differently.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
0, with requiredpickup: true.1.Documentation
Validation
Examples