spec: retaining full after separation is a MUST — FEP #57 (version-scoped) - #58
Merged
Conversation
…, version-scoped) Separation is lossy and the mixdown is unrecoverable once dropped, and consumers now depend on the mixdown being present: a Reader that plays `full` at unity gain to avoid the separation loss (§5.3's own recommendation) silently degrades to a lossy stem recombination on any pack that used the old SHOULD's permission to discard it. Reader followed the spec, writer followed the spec, the user got worse audio than the format could guarantee. SHOULD is the wrong strength for "don't destroy the one irreplaceable copy of the audio". So it is a MUST — gated to the pack's own version (feedpak_version >= 1.16.0), which is what keeps this a MINOR rather than a MAJOR: a pack authored under an earlier version that separated without `full` is still judged by the SHOULD that was in force when it was written, so NOTHING becomes retroactively non-conformant and no pack needs regeneration. This mirrors how the spec already version-gates the 1.6.0 .jsonc and 1.9.0 audio-format relaxations. Scoped to the act of SEPARATING: a pack of real recorded multitrack stems, never separated from a single mix, is not forced to manufacture one. One normative verb, no new keys, no structural change. Not schema-enforceable (it's a cross-field conditional — "if separated, then full present"); it stays a Writer MUST a linter can check. Version line stays 1.15.0 (bumps to 1.16.0 at release, alongside the other unreleased change). check_versions passes, both examples validate, 57 tests pass. Signed-off-by: topkoa <topkoa@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR makes retaining the ChangesFull stem retention
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Updates the feedpak v1 specification to tighten the Writer obligation around stem separation by making retention of the full mixdown a version-scoped MUST (per FEP #57 Option B), and documents the compatibility story in the changelog.
Changes:
- In §5.3, changes the separation guidance from SHOULD to MUST and adds rationale + version scoping notes.
- Adds an Unreleased changelog entry describing the new requirement and its non-retroactive (version-gated) nature.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| spec/feedpak-v1.md | Tightens the normative Writer requirement to retain full after separation, with added version-scoping explanation. |
| CHANGELOG.md | Documents the new version-scoped MUST in Unreleased notes, including compatibility impact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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 404-418: Update the schema description for the separating Writer’s
retention of the full stem to match the version-scoped requirement: state SHOULD
for feedpak_version before 1.16.0 and MUST for feedpak_version 1.16.0 or newer.
Keep the existing structural validation unchanged and update only the normative
guidance in the relevant manifest schema description.
🪄 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: 2846bc76-e48b-4c9c-9fa1-600e151936d6
📒 Files selected for processing (2)
CHANGELOG.mdspec/feedpak-v1.md
Both review findings were the same gap from different angles: the version
scoping lived a paragraph away from the normative verb, so anything that
read just the rule — a human scanning §5.3, a validator grepping for
"MUST retain", or a schema consumer reading stemEntry.id's description —
saw an unconditional MUST and would misjudge pre-1.16.0 packs.
The MUST sentence now carries its own condition ("MUST — for packs
declaring feedpak_version 1.16.0 or newer; SHOULD for packs declaring an
earlier version"), the following paragraph is reworded as the expansion of
that condition rather than a surprise reveal, and the schema's stemEntry
description states the same gate.
Signed-off-by: topkoa <topkoa@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements FEP #57, Option B (the maintainer's pick): a version-scoped MUST, landing as a MINOR.
The change
§5.3, one normative verb — a Writer that separates a mixdown
SHOULDMUST retain thefullentry (default: false).Why SHOULD was wrong
fullpermanently lost the only exact copy of the song.fullat unity gain to skip the separation loss (the feedBack stem mixer does exactly this). On any pack that exercised the old SHOULD's permission to dropfull, that optimization silently degrades to a lossy recombination. Reader conformant, writer conformant, user gets worse audio than the format could guarantee.Why this is MINOR, not MAJOR
A blanket MUST would make previously-conformant packs (separated,
fulldropped) retroactively non-conformant — the §4.2 MAJOR trigger. Option B avoids that by gating the obligation to the pack's own version: it binds a Writer producing a pack atfeedpak_version≥ 1.16.0. A pack authored earlier is judged by the SHOULD in force when it was written, so nothing is retroactively invalidated and no pack needs regeneration. This mirrors how the spec already version-gates the 1.6.0.jsoncand 1.9.0 audio-format relaxations. Additive in effect → MINOR.The obligation is scoped to separating a mixdown, so a pack of real recorded multitrack stems (never separated) isn't forced to bounce one.
Not schema-enforceable
"IF this pack was separated THEN it contains
full" is a cross-field conditional (separation inferred fromstem_separation), which a JSON Schema can't express cleanly. It stays a normative Writer MUST that a validator/linter can check out-of-band — noted in the FEP.Verification
check_versions.pypasses (version line stays 1.15.0 — it bumps to 1.16.0 at release, with the other unreleased change).full, so it's conformant under the new rule.No schema or example change — this is a Writer-obligation tightening, not a structural addition.
Summary by CodeRabbit
fullmixdown alongside instrument stems.