docs(adr): propose ADR-0032 — holdout filtering boundary over LoadedChunk - #166
Conversation
…hunk Docs-only. Fixes the executable boundary between corpus loading and material construction, plus offline-lab ownership, before any implementation — as the reachability Phase-0 audit recommends. Decisions (Proposed): - The offline Reachability Lab owns CorpusMode and holdout run artifacts; an isolated lab-style instrument (ADR-0010 precedent), no roadmap stage assigned (audit defers placement). - Holdout filtering occurs over Vec<LoadedChunk>, before corpus_material and before any rhythm/reference/gesture compilation — the only point where full provenance (sha256, song_id, bar_range, track_index) survives (core/src/generation_input.rs:65,137; cli/src/generation_input.rs:64,68). - HoldoutTargetSong: preflight the whole corpus (song_holdout_preflight), typed-refuse on any coverage/identity inconsistency, require a target song_id, exclude every LoadedChunk carrying it, then compile the rest. - TargetIdentity carries per-mode provenance (source_sha256, song_id, bar_range, track_index, projection, eligibility), not a vague source string. - Bound non-goals: no generation/scoring change, no curation or auto grouping, no post-hoc relabelling of a failed holdout as valid. - First slice = song mode only; file/fragment modes follow separately. No new roadmap stage number. Not appended to decisions.log.md (reserved for ADRs). Gates the holdout-wiring implementation PR once accepted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012T7SRMiXmZe5v1imtrKMPi
📝 WalkthroughWalkthroughAdds ADR-0032 defining a fail-closed, provenance-preserving holdout filtering boundary before ChangesHoldout filtering boundary
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 864977044c
ℹ️ 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".
![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 `@docs/adr/0032-holdout-filtering-boundary.md`:
- Around line 88-94: Update the ADR’s synthetic-fixture characterization and
boundary-function sections to describe refusal and end-to-end holdout execution
as expected future behavior after implementation, not current guarantees.
Preserve the distinctions among missing targets, uncurated or inconsistent
sources, unrelated songs, deterministic filtering, untouched metadata, NoCorpus,
and explicitly named leaky diagnostics.
🪄 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: 2158b5b9-4bb8-4098-bd8b-66c6c6b4f13f
📒 Files selected for processing (2)
docs/adr/0032-holdout-filtering-boundary.mddocs/adr/README.md
…tense
Address the review's three architectural/documentation holes:
1. Absence-capable result. prepare_corpus_for_mode now returns
Result<Option<CorpusMaterial>, HoldoutError>. NoCorpus -> Ok(None) (a
genuinely corpus-free run, matching the existing material: None), distinct
from a new LeakyDiagnostic -> Ok(Some(unfiltered)). NoCorpus is no longer
mislabelled a "no-holdout" mode; they are different experiments.
2. Single source of truth. The boundary consumes a bound LoadedCorpus
{ manifest, loaded, skipped } so it can pass the complete manifest (and
its optional songs map) to song_holdout_preflight. Manifest chunks and
loaded records must describe the same corpus (match by ChunkId; duplicate
/ missing / mismatched -> typed refusal), closing the stale-manifest leak
where preflight validates one dataset and filtering executes another.
3. Tense. Current-behavior claims rewritten as post-implementation
consequences — nothing calls the preflight in a generation path today.
Also states placement explicitly: CorpusMode / TargetIdentity / HoldoutError
and the orchestration live in the isolated lab; production CLI/cockpit gain
no holdout policy; core generation/rerank unchanged; a loader addition only
supplies the lab one authoritative LoadedCorpus.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012T7SRMiXmZe5v1imtrKMPi
REQUEST CHANGES addressed — ADR-0032 v2 (head
|
1 participant
What
Docs-only ADR-0032 (Proposed) fixing the executable holdout boundary between corpus loading and material construction, its authoritative input, and its ownership — the governance step the reachability Phase-0 audit requires before any implementation. The A→B→C→ADR-0031→D modernization arc stays frozen.
Diff: the new ADR + its
docs/adr/README.mdrow. Nodecisions.log.mdchange (reserved for ADRs).Why here
The generator loads
Vec<LoadedChunk>— each carrying fullChunkMeta(sha256,song_id,bar_range,track_index) — and immediately folds it intoCorpusMaterialviacorpus_material(core/src/generation_input.rs:137), discarding provenance into anonymous rhythms/references/gesture. The CLI loader does exactly this with no decision between (cli/src/generation_input.rs:64,68) and never reads aCorpusManifest. So the only place a holdout decision can happen is overVec<LoadedChunk>, beforecorpus_material. ADR-0031 gave ussong_holdout_preflight, but nothing calls it in a generation path yet.Decisions (Proposed)
CorpusMode,TargetIdentity,HoldoutError, and the boundary orchestration live in the offline Reachability Lab (isolated lab-style instrument, ADR-0010 precedent). Production CLI/cockpit gain no holdout policy; core generation/rerank unchanged; ordinary generation bypasses the seam. No roadmap stage assigned (audit defers placement).LoadedCorpus { manifest, loaded, skipped }, so the completeCorpusManifest(+ optionalsongsmap) reaches the preflight. Single-authority invariant:manifest.chunks↔LoadedChunk.metamatched byChunkId; duplicate / missing / provenance-mismatched records typed-refuse (or the lab buildsloadeddirectly frommanifest.chunks). Closes the stale-manifest leak.NoCorpus→Ok(None)— genuinely corpus-free (matches existingmaterial: None).LeakyDiagnostic→Ok(Some(corpus_material(all_loaded, skipped)))— a corpus deliberately supplied unfiltered, named so it can't be mistaken for a holdout.HoldoutTargetSong→ preflight → filter →Ok(Some(...)).HoldoutTargetSong— preflight the wholecorpus.manifest→ typed-refuse on any coverage/identity inconsistency → require a targetsong_id→ exclude everyLoadedChunkcarrying it → only then compile the rest.TargetIdentitycarries per-mode provenance (source_sha256,song_id,bar_range,track_index,projection,eligibility).bar_range == None= whole-source overlap) follow as separate slices.After acceptance
RED→GREEN: synthetic fixtures prove that, once the slice lands, an uncurated corpus typed-refuses before material construction and a curated fixture excludes every representation of the target song with zero leakage into any channel; then the smallest pure boundary over
LoadedCorpus.🤖 Generated with Claude Code
https://claude.ai/code/session_012T7SRMiXmZe5v1imtrKMPi