docs(adr): ADR-0031 — canonical curator-assigned song identity (schema v10)

[PhysShell]

What

ADR-0031 proposes a canonical song identity for the corpus — the Phase-1 prerequisite the Generator Reachability Lab Phase 0 audit (#154) named: without it, song-level holdout is not implementable fail-closed (two transcriptions of one song have different sha256, so a "song holdout" silently keeps one and excludes the other), which also weakens the human-similarity benchmark's "split by source identity" law (#153).

Docs-only; Status: Proposed — binds nothing until accepted (docs/adr/README.md). Implementation is a separate red→green slice once accepted.

The decision

Add an optional, curator-assigned SourceRef.song_id: Option<SongId> (schema v10) at the Work level:

• griff already has the Manifestation level (SourceRef.sha256, v9) and the span level (EnsembleRef, v4) — this adds the missing Work (composition) level above them.
• Follows the established optional-field, forward-compatible pattern (like rights v7, EnsembleRef v4, sha256 v9): pre-v10 records load as None and round-trip byte-identically.
• Curator-assigned, never content-derived (like rights v7). Groups files that are the same composition for leakage-safe holdout / source-identity splits only — explicitly no cover-detection, arrangement-equivalence, or production-scoring claim.
• Fail-closed: HoldoutTargetSong requires song_id; a record without it can't be song-held-out (typed refusal / migrate), never a basename/title fallback.

Prior art

Surveyed in decisions.log.md (prior-art-first rule): FRBR (Work → Expression → Manifestation → Item) and MusicBrainz (Work ↔ Recording ↔ Release, ISWC work code) — both separate the abstract work from its manifestations. SongId is the Work level; no external id scheme (ISWC/MBID) is adopted as the stored value.

Also

• docs/adr/README.md — index row 0031.
• docs/decisions.log.md — prior-art Y-statement.
• …-metric-inventory.md §3 — back-reference to the ADR from the Phase-1-prerequisite note.

Docs-only → CI (Rust-only) green. Not merging without an explicit human GO.

🤖 Generated with Claude Code

https://claude.ai/code/session_012T7SRMiXmZe5v1imtrKMPi

Summary by CodeRabbit

• Documentation
  • Added a proposed architecture decision for curator-assigned canonical song identities at the composition level.
  • Documented schema v10 requirements for song-level holdouts, including fail-closed coverage checks and manifest validation.
  • Updated the architecture decision index, audit documentation, and decision log with the new canonical identity model.
  • Clarified that identities are curator assertions and are not derived from content, metrics, or external identifier systems.

[coderabbitai]

Warning

Review limit reached

@PhysShell, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 51 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 54e245d0-5118-4cd1-a620-e8b52b2520e0

📥 Commits

Reviewing files that changed from the base of the PR and between 8965184 and 532c797.

📒 Files selected for processing (2)

• docs/adr/0031-canonical-song-identity.md
• docs/audit/2026-07-generator-reachability-metric-inventory.md

📝 Walkthrough

Walkthrough

Changes

Canonical song identity

Layer / File(s)	Summary
Canonical identity and holdout decision docs/adr/0031-canonical-song-identity.md, docs/decisions.log.md	Defines a curator-assigned Work-level SongId, schema v10 SourceRef support, fail-closed song-level holdout semantics, and rejected identity alternatives.
Repository decision references docs/adr/README.md, docs/audit/2026-07-generator-reachability-metric-inventory.md	Indexes ADR-0031 and links the schema v10 proposal from the HoldoutTargetSong feasibility discussion.

Estimated code review effort: 1 (Trivial) | ~4 minutes

Suggested reviewers: claude

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: ADR-0031 proposing a curator-assigned canonical song identity in schema v10.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/adr-canonical-song-identity

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7837533019

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Enforce corpus-wide song-ID coverage for song holdout

When the target has a song_id but an alternate transcription of that composition is a pre-v10 or uncurated record with song_id = None, the specified equality filter retains that record and leaks the held-out song into the corpus. Refusing only a target that lacks an ID therefore does not make the experiment fail-closed; song-holdout mode must also reject, migrate, or exclude every participating source with unknown identity (and validate the corpus-wide mapping) before material construction.

Useful? React with 👍 / 👎.

[PhysShell]

@codex — accepted, sharp catch. Fixed in 8965184.

You're right: guarding only the target isn't fail-closed. An alternate transcription of the target composition that is a pre-v10 / uncurated record (song_id = None) would be silently retained by an equality filter (exclude where song_id == target), re-leaking the held-out song.

Strengthened the HoldoutTargetSong contract in the ADR: it now requires song_id coverage over the corpus, and additionally excludes (or refuses the run on) every source with song_id = None before material construction — an unidentified record can't be proven not to be an alternate of the target, so it must not remain. The manifest songs cross-check validates coverage. The Consequences note is aligned (a HoldoutTargetSong run excludes pre-v10 None records until curated).

Still a Proposed, docs-only ADR (binds nothing); the schema-v10 implementation will carry this contract into its red→green tests. Not merging without an explicit human GO.

[chatgpt-codex-connector]

Review Outcome

The requested P1 feedback is already fully addressed in commit 8965184:

• HoldoutTargetSong now requires corpus-wide song_id coverage rather than checking only the target. docs/adr/0031-canonical-song-identity.mdL80-L89
• Every source with song_id = None must be excluded—or the run refused—before material construction, preventing unidentified alternate transcriptions from leaking into the corpus. docs/adr/0031-canonical-song-identity.mdL90-L95
• The manifest songs cross-check validates coverage while retaining the per-source field as authoritative. docs/adr/0031-canonical-song-identity.mdL93-L100
• The ADR remains Proposed and explicitly defers implementation to a separate red→green slice after acceptance. docs/adr/0031-canonical-song-identity.mdL102-L105

No further code or documentation changes were necessary, so I did not create another commit or pull request.

Testing

• ✅ git status --porcelain=v1 — working tree is clean.
• ✅ git show --check 8965184 — commit has no whitespace errors.
• ✅ git show --stat --oneline 8965184 — confirmed the requested correction is contained in the current PR head commit.

View task →

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@docs/adr/0031-canonical-song-identity.md`:
- Around line 68-78: Clarify the cover-version rule in the ADR section
describing song_id scope: either remove the curator override and require all
composition-sharing covers to use the same song_id, or define the exact
conditions for assigning a different ID and the required rationale field or
schema location. Ensure holdout grouping remains deterministic and does not
depend on an undocumented musical-similarity judgment.

In `@docs/audit/2026-07-generator-reachability-metric-inventory.md`:
- Around line 208-210: Align the audit’s holdout eligibility contract with
ADR-0031 by replacing canonical_song_id with the ADR-defined SourceRef.song_id
name, unless an intentional projection rename is required; in that case,
explicitly document the mapping and identify the authoritative source of truth.

🪄 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: 8f3a7c34-c255-495f-af1c-404218772bcb

📥 Commits

Reviewing files that changed from the base of the PR and between 8fff781 and 8965184.

📒 Files selected for processing (4)

• docs/adr/0031-canonical-song-identity.md
• docs/adr/README.md
• docs/audit/2026-07-generator-reachability-metric-inventory.md
• docs/decisions.log.md

[PhysShell]

All four accepted and fixed in edcc309:

1. Cover override removed. You're right — allowing a "musically distinct" cover onto a different song_id turned the field from Work identity into a similarity/arrangement grouping and re-opened the holdout boundary. Now: every manifestation / arrangement / edition / cover of one composition MUST share one song_id; musical or arrangement distinctness never changes Work identity. Splitting expressions/arrangements, if ever wanted, is a separate arrangement_id / expression_id (a lower FRBR level) or an explicit split policy — one field doesn't play both FRBR Work and a taste judgement.
2. Unified naming. The audit's eligibility DTO canonical_song_id → song_id everywhere, stated as copied verbatim from the authoritative SourceRef.song_id (ADR-0031). One name, no alias for the future archaeologist.
3. Strict-refusal coverage. The initial contract is now unambiguous: HoldoutTargetSong preflight MUST refuse the run when any participating unique source lacks song_id, before material construction. Conservative exclusion of all None sources is also leakage-safe but silently shrinks the corpus / changes the benchmark population, so it's a separate, explicitly versioned policy, not the default.
4. SHA→SongId invariants added (checked in the same preflight): (1) all chunks sharing a sha256 carry the same song_id — else one file gets split song_ids across chunk records and a song holdout excludes only part of the file, re-introducing chunk-level leakage through the very field meant to stop it; (2) no sha256 maps to two SongIds; (3) if the songs manifest is present, manifest ↔ SourceRef.song_id agree both ways. The manifest is a convenience/typo check, not the coverage proof — coverage is proven by the preflight scan of every participating source.

Still a Proposed docs-only ADR (binds nothing); the schema-v10 implementation will carry these contracts into its red→green tests. Not merging without an explicit human GO.

[PhysShell]

Fixed in 532c797 — the last documentation cockroach evicted.

The Consequences "out of scope" bullet still said a HoldoutTargetSong run "excludes every such record", contradicting the Decision's strict-refusal default. Rewrote it to match:

pre-v10 records remain song_id = None. Under the default HoldoutTargetSong policy the preflight refuses the entire run until every participating unique source is curated; conservative exclusion of unknown sources is permitted only under a separate, explicitly versioned policy.

So Decision and Consequences now give one normative answer: default = refuse; exclusion = separate versioned policy. Verified the ADR has no other stale "exclude the None records and continue" wording (the remaining excludes are the correct target-chunk holdout and the invariant explanation).

Docs-only; head is now 532c797. Not merging without an explicit human GO.