test(core): red — candidate set + explainable rerank over S6 strategies - #102
Merged
Conversation
Failing tests for griff_core::rerank: generate_candidate_set fans a base request out over every S6 strategy × seed variants (deterministic derived seeds, template rotation across variants, optional gesture carving, rhythm-copy skipped without a template), and rerank_candidates scores each candidate on the four closure axes plus the two novelty axes under a caller WeightPolicy, returning Scored envelopes in rank order. This is the wiring the melodic-closure note left open (§7.2 rerank, §7.3 caller-side novelty cut; ADR-0017) and the S6 stage doc's promised Vec<GenerationCandidate>. The griff_core::rerank API does not exist yet, so the suite fails to compile until the green step. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VBNEW7XcGqRzyf8WfqRKVU
Match on the set error instead of comparing whole Results (Score carries no PartialEq), const-ify the constraints helper, name the fingerprint type, and import std::slice — clippy -D warnings + rustfmt conformance. Test-only fixups of the red suite; the contract is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VBNEW7XcGqRzyf8WfqRKVU
griff_core::rerank closes the wiring gap the melodic-closure note left open (§7.2 rerank into S6 candidate sets, §7.3 caller-side novelty measure): - generate_candidate_set fans one base request out over every S6 strategy × variants_per_strategy seed variants. Variant seeds come from a SplitMix64 mix of (base seed, strategy index, variant index) — deterministic (SPEC §6) and independent of the gesture ask, so plain and gestured runs pair up seed-for-seed. Rhythm-copy is skipped without a usable template; multiple templates rotate across variants; an optional GestureControl routes each candidate through generate_gestured (burst/rest carving) instead of wall-to-wall output. - rerank_candidates measures the four closure axes plus the two novelty axes (against caller-supplied references) per candidate and returns Scored envelopes in rank order via rank_indices. Weights stay data (ADR-0017 §3): generation_rerank v1 is the uniform untuned baseline; rejection thresholds remain the caller's policy. Green step for the rerank suite; all workspace tests pass, clippy clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VBNEW7XcGqRzyf8WfqRKVU
Failing tests for the CLI wiring of the rerank seam (research note §7.2/§7.3): - unit: bar_rhythms extracts one duration template per sounding bar of a chunk source (silent bars skipped, identical templates deduped); gesture_control_from_chunks averages per-chunk GestureControls and skips stats-less records; load_corpus_material reads *.chunk.json records next to their source tabs, slices bar_range provenance, and skips (by record name) chunks whose source cannot be read — group records are ignored. - integration: griff generate grows --corpus/--candidates/--no-gesture, reports what the corpus supplied, names the generation_rerank v1 policy in its ranking printout (ADR-0017 provenance), writes the top-ranked winner, stays byte-deterministic for a fixed seed + corpus, and rejects a missing corpus dir instead of silently falling back. The helpers and flags do not exist yet, so the suite fails to compile / run until the green step. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VBNEW7XcGqRzyf8WfqRKVU
griff generate now builds the full S6 candidate set (every strategy × --candidates seed variants, default 2) and reranks it on the closure + novelty axes under the generation_rerank v1 policy (ADR-0017), printing the ranked list with its policy provenance and writing the winner. Green step for the corpus-wiring suite. --corpus <DIR> feeds the pass from curated chunks instead of the input's first bar: per-bar rhythm templates from each record's bar_range-sliced source (silent bars skipped, duplicates deduped), the sliced scores as the novelty guard's reference set, and the mean of the per-chunk burst/rest GestureControls as the gesture ask (--no-gesture opts out). Records whose source is missing or unreadable are reported as skipped, never silently dropped; a missing corpus dir is an error, not a fallback. The generate golden snapshots are deliberately re-blessed (GRIFF_BLESS=1): the default path now emits the ranking printout and picks the top-ranked candidate rather than the single hardcoded rhythm-copy pass. Includes lint-conformance fixups of the red-phase tests (absolute paths, checked arithmetic, assert on content instead of is_ok). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VBNEW7XcGqRzyf8WfqRKVU
Y-statement for the rerank seam and the corpus-fed griff generate (--corpus / --candidates / --no-gesture), plus a progress note on the S6 stage doc naming what is still open (cadence-aware endings, anchor preservation, playability filter, density/syncopation gates). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VBNEW7XcGqRzyf8WfqRKVU
…t context Failing tests for two GP-adapter loss-report fixes surfaced by the 2026-07 corpus import scan (410 community tabs): - a per-string note entry whose type flag (0x20) is absent keeps the parser's default kind Rest and carries no fret — it encodes a silent string in an otherwise sounding beat, normal GP encoding, and must import without an ImportWarning (the scan mis-counted 138 of these as real losses, skewing the clean/lossy split); - an orphan tie's loss must name the string and start tick so corpus-side scans can bucket the 172 observed occurrences by cause instead of counting an opaque string. Plus a passing characterization guard: Unknown(_) note kinds stay reported losses, and the beat still occupies its time as a rest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VBNEW7XcGqRzyf8WfqRKVU
…tie loss carries context Green step for the GP loss-report suite: - NoteType::Rest per-string entries (type flag absent, no fret) are the normal GP encoding of a silent string in a sounding beat: skipped without an ImportWarning. Unknown(_) kinds stay reported losses, and an all-Rest-notes beat still lands as a gapless rest group. The 2026-07 corpus scan counted 138 of these as losses, skewing the clean/lossy split. - The orphan-tie loss now names the (one-indexed) string and the tie's start tick, so corpus-side scans can bucket the 172 observed occurrences by cause (cross-voice antecedent vs anacrusis vs skipped antecedent) instead of counting an opaque string. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VBNEW7XcGqRzyf8WfqRKVU
The corpus import scan (410 community tabs) hit 98 parse errors, dominated by the 0.3 parser's hard failures on cosmetic fields: an out-of-enum triplet feel byte aborted the whole file, and rse/lyrics/portamento type conversions did the same. Upstream 0.4.2 already fixes the biggest buckets: triplet feel reads leniently (unknown -> None), the strict conversion sites drop from 124 to 59, and the gpx importer is rewritten. Drop-in besides the model::legacy import-path rename (types are still re-exported at the crate root; only Duration/NoteEffect/BendEffect paths move). Verified: full workspace tests, clippy -D warnings --all-features, and the cockpit wasm32 build (zip stays at 8.6, so the wasm surface is unchanged; the only lockfile additions are guitarpro 0.4.2 + quick-xml 0.41). Fork/vendor stays deferred until the corpus re-scan shows what survives 0.4.2 (decisions.log 2026-07-11). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VBNEW7XcGqRzyf8WfqRKVU
…rests first-class Failing tests for the rhythm grid (2026-07-11 corpus playtest: the corpus is audible only when a rhythm-copy candidate wins — the other four strategies hardcode wall-to-wall quarters, so corpus templates and in-bar silence never reach their output): - RhythmTemplate replaces the bare duration list: one bar of onset-placed TemplateNotes, so gaps (rests, syncopation) survive extraction and generation; from_durations rebuilds the legacy back-to-back shape. - Every strategy lays its pitches onto the same per-bar grid: gapped templates place notes with silence between them in all five strategies; durations clamp to the bar end; offsets past the bar end drop; an unusable grid and an absent template both fall back to the quarter grid (today's no-input behaviour, characterized). - Determinism and pitch-range invariants hold on gapped grids. Existing suites (rule_generator, characterization, structure_control, rerank) are adapted to the new template type in the same commit — the contract change is theirs too. RhythmTemplate/TemplateNote do not exist yet, so the suites fail to compile until the green step. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VBNEW7XcGqRzyf8WfqRKVU
… first-class
Green step for the rhythm-grid suite (2026-07-11 corpus playtest: the corpus
was audible only when a rhythm-copy candidate won the rerank):
- RhythmTemplate { notes: [TemplateNote { offset, duration }] } replaces the
bare duration list, so in-bar silence (rests, syncopation) survives from
extraction through generation. from_durations rebuilds the legacy
back-to-back shape for wall-to-wall sources.
- Every strategy now writes one pitch per slot of a shared per-bar grid: the
first usable template clamped to the bar (offsets past the bar end drop,
durations clamp to the bar end, unusable grids and absent templates fall
back to the quarter grid — the no-input case is unchanged). Pitch logic
stays the strategy's own; GenNote carries its grid offset and
bars_to_score places notes at bar start + offset instead of packing them
back-to-back.
- The candidate-set builder feeds the rotated template to every strategy,
not just rhythm-copy — a multi-template corpus is audible across the set.
- complement keeps its historical quarter-grid behaviour (an explicitly
empty template list): re-rhythming part B onto A's bar durations is its
own increment with its own goldens.
Deliberate re-blesses: core generate__deterministic_7_8 (the 7/8 walk now
honours its eighth-note template) and the CLI generate goldens whose input
first bar is not plain quarters (multi_track, seven_eight). Fuzz targets
build templates via from_durations; the CLI corpus loader converts
mechanically (offset-aware extraction is the next increment).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VBNEW7XcGqRzyf8WfqRKVU
…over resting chunks Failing tests for the CLI half of the rhythm-grid increment (2026-07-11 playtest follow-ups): - bar_rhythms must keep in-bar offsets: a syncopated bar (off-beat eighths with a gap) extracts as placed TemplateNotes, not as flattened back-to-back durations — rests and syncopation are exactly what the corpus should teach the grid. - gesture_control_from_chunks aggregates only chunks that actually rest (a wall-to-wall riff's stats describe one giant burst and inflated the ask to burst 69 — nothing ever carved), takes the per-axis median so a long-burst outlier cannot drag the ask out of carving range, and returns None when no chunk rests instead of a degenerate ask. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VBNEW7XcGqRzyf8WfqRKVU
…esting chunks Green step for the CLI half of the rhythm grid: - bar_rhythms keeps each note's in-bar offset (TemplateNote), so corpus rests and syncopation reach the grid instead of being flattened into back-to-back durations. - gesture_control_from_chunks lets only chunks that actually rest vote (rest_count > 0), takes the per-axis median of the per-chunk controls, and returns None when no chunk rests. The 2026-07-11 playtest's ask of burst 69 / rest 6.6q — inflated by wall-to-wall chunks' one-giant-burst stats — never carved anything; a median over resting chunks stays in carving range. Verified end-to-end: with a corpus the winner now carves (12-note burst, then a full bar of silence over a 4-bar request); --no-gesture stays wall-to-wall. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VBNEW7XcGqRzyf8WfqRKVU
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VBNEW7XcGqRzyf8WfqRKVU
Formatting only — no behaviour or contract change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VBNEW7XcGqRzyf8WfqRKVU
…m variety Failing tests for the rhythm-monotony fix (2026-07 A/B: distinct_dur stuck at 1.0 — every bar reused the first template, and the rerank set fed one template per candidate): - multiple_templates_rotate_across_bars: given two rhythms (eighths vs one whole note), the four per-bar-independent strategies must alternate them across bars (8,1,8,1), so a corpus's rhythmic variety survives inside one phrase. - candidate_hears_every_template_across_its_bars: replaces the old per-variant rotation — one candidate now cycles the whole palette across its bars (1 + 8 = 9 notes over two bars). Guard tests (already green, pinning what must NOT change): RepeatVariation keeps one rhythm across bars (its call/response identity), and a single template still repeats every bar. Co-Authored-By: Claude <noreply@anthropic.com>
Green step for the rhythm-monotony fix (2026-07 A/B: distinct_dur stuck at 1.0 because every bar reused the first template and the rerank fed one template per candidate): - bar_grid -> bar_grids: returns one clamped grid per non-empty template (in input order), never empty (quarter-grid fallback). grid_for_bar cycles them by bar index, so the four per-bar-independent strategies alternate rhythms across bars. RepeatVariation stays on grid[0] — repetition is its identity. - generate_candidate_set now hands every candidate the whole template palette instead of one rotated template per variant; per-bar rotation inside generate makes a single candidate carry the corpus's rhythmic variety across its bars, and variants differ by seed (the pitch line). Verified end-to-end: a 6-bar generation over a 2-template corpus now alternates 4/8/4/8 notes per bar (was uniform before). Determinism and the single-template / no-template fallbacks are unchanged. Note: only the first bar_count templates of a large corpus are heard (index % len); diverse-template selection is a parked refinement. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Failing tests for a small deterministic diagnostic the CLI generation summary needs to make a corpus A/B interpretable (arbiter request): - separates loaded templates from effective grids (empty and clamped-away templates drop); - a stable FNV-1a fingerprint per effective grid — equal rhythms share one, a distinct rhythm gets a distinct one — so 'how many distinct bar rhythms did this run actually use' is answerable without eyeballing MIDI; - no usable template reads as zero effective (quarter fallback), no fingerprints. rhythm_diagnostics / RhythmDiagnostics do not exist yet, so the suite fails to compile until the green step. Co-Authored-By: Claude <noreply@anthropic.com>
Green step for the diagnostics seam plus the doc debt the rotation bump left: - rhythm_diagnostics(templates, bar_duration) -> RhythmDiagnostics reports loaded vs effective template counts and a stable FNV-1a fingerprint per effective grid (extracted effective_grids, shared with bar_grids). Pure and deterministic (SPEC §6); no runtime effect on generation. - Corrected docs that still described the pre-rotation contract: GenerationStrategy and RuleGenerationRequest.source_rhythms now state that the whole palette is used and the per-bar-independent strategies rotate the effective grids by bar index (RepeatVariation holds grid[0]), and the rerank module doc drops 'the rotated template' for 'the whole palette, rotated per bar; variants differ by seed'. Co-Authored-By: Claude <noreply@anthropic.com>
A one-line deterministic seam for corpus-A/B transparency: loaded vs effective templates, bar count, gesture on/off, and the fingerprints of the first min(bars, effective) grids the run rotates through. With no corpus the line reads '1 loaded / 1 effective' (the input's own first-bar rhythm), so the two A/B legs are directly comparable; effective 0 prints '(quarter fallback)'. distinct_bar_rhythms == bar_count is deliberately NOT asserted — it depends on unique effective templates, clamp, and gesture carving. The no-corpus generate goldens are re-blessed for the new first line. Co-Authored-By: Claude <noreply@anthropic.com>
…ions.log) Co-Authored-By: Claude <noreply@anthropic.com>
…iguous - decisions.log: rotation bump ACCEPTED. Records the A/B precisely — 10 unique rhythmic conditions (5 seeds x gesture on/off), not 30 independent confirmations (three inputs shared the corpus rhythm schedule); the effect held in all ten. distinct_bar_rhythms rose +5.60 (gesture on) / +6.00 (off) and matches per-seed between gesture on/off after rotation. States it as 'gesture does not re-collapse per-bar rhythm-signature diversity after rotation' — NOT orthogonal (it still moves distinct_dur 3.0/4.2 and npb_std 2.26/1.86). Determinism byte-identical; no empty pieces. - CLI: --candidates help and the ranking line now spell out that the flag is variants-per-strategy and the reranked set is variants x 5 strategies (e.g. --candidates 10 -> 50 total), so nobody reads it as 'reranker chose from 10'. Co-Authored-By: Claude <noreply@anthropic.com>
Failing tests for the register/pitch fix the rotation A/B exposed (narrow, low
register ~one octave, root E1): rhythm_copy/shuffle walked degrees only in
[0, scale_len) — one octave up from PitchMaterial.root (the input's minimum
pitch) — and motif_transpose shifted by semitones, escaping the pitch-class
palette.
Pins the contract for a single shared degree->pitch mapper
(griff_core::pitch::{PitchRange, PitchClassSet, ScaleLadder}):
- ScaleLadder spans the full [lo,hi] in-class, ascending, >1 octave;
PitchClassSet normalises mod 12; PitchRange normalises reversed/clamped
bounds; empty class set falls back to lo.
- Generation: every strategy stays in-bounds AND in the allowed pitch classes
over several seeds; the candidate SET reaches beyond the first octave
(reachability, not per-piece coverage); determinism holds; a narrow range
still works; reversed/equal bounds do not regress.
griff_core::pitch does not exist yet, so the suite fails to compile until the
green step.
Co-Authored-By: Claude <noreply@anthropic.com>
Green step for the register increment. New griff_core::pitch module holds the
single mapper the arbiter asked for:
- PitchRange { lo, hi } (normalises reversed/clamped bounds), PitchClassSet
{ classes } (folds mod 12, sorts, dedups), and ScaleLadder — the ascending
list of every in-range pitch whose class is in the palette, indexed by a
linear degree (clamped at the ends, never empty).
generate() builds one ScaleLadder over [pitch_lo, pitch_hi] from the
material's pitch classes and passes it to every strategy, replacing the
octave-walking PitchMaterial::pitch_at (removed) that pinned rhythm-copy and
shuffle to one octave above the anchor. motif_transpose now transposes along
the ladder in *degrees* (was semitones, which could leave the palette), so
every generated pitch is guaranteed in-class and in-bounds. complement's
private band_scale_ladder delegates to ScaleLadder — no second mapper.
PitchMaterial.root is re-documented as a pitch-class anchor, NOT a tonal
center; tonal-center inference stays a later increment. Contract (pitch_ladder
suite): pitches always in-bounds and in-class across all five strategies and
several seeds; the candidate SET reaches beyond the first octave (reachability,
not per-piece coverage); determinism holds; narrow and reversed/equal bounds
do not regress. Verified live: seeds now span up to ~3 octaves (was ~one).
Generate goldens (core + CLI) re-blessed for the new mapping.
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…biter) - The rotation A/B ran at the DEFAULT 2 variants-per-strategy (10 total ranked candidates); candidates=10 in the tooling JSON was the ranked-set size, not a variant count. An earlier draft wrongly read it as 10 x 5 = 50. The register baseline is the 10-variant / 50-per-condition config. - Register is NOT accepted yet: structural defect confirmed and the shared full-range ladder is implemented, but behavioral acceptance awaits the post-fix corpus/synthetic A/B. Records the checkable strategy risks (shuffle multi-octave leaps, motif/repeat top-clamp saturation, rhythm-copy high->low wrap, no register-coherence axis) and the parked LadderWindow / RegisterPlan direction. TonalCenter and cadence stay not-started. Co-Authored-By: Claude <noreply@anthropic.com>
…-palette fallback Failing tests for the empty-intersection contract fix (arbiter): the ladder must not fall back to range.lo when the palette is empty or no allowed class lands in range — that silently breaks the always-in-class guarantee. - ScaleLadder::build -> Result<_, PitchSelectionError>: empty class set -> EmptyPitchClassSet; a range with no in-class pitch -> NoAllowedPitchInRange; a range holding exactly one in-class pitch -> Ok(one-rung ladder). - The generator surfaces the error (a single-pitch range whose class is outside the palette makes generate() Err, not a silent floor). PitchSelectionError and the Result signature do not exist yet, so the suite fails to compile until the green step. Co-Authored-By: Claude <noreply@anthropic.com>
…te fallback Green step for the empty-intersection contract (arbiter): the ladder no longer falls back to range.lo when the palette selects nothing, which silently broke the always-in-class guarantee. - ScaleLadder::build -> Result<Self, PitchSelectionError>: EmptyPitchClassSet (no classes) / NoAllowedPitchInRange (non-empty palette, none in range). Ok is now genuinely always-in-class by construction; a range holding exactly one in-class pitch is a valid one-rung ladder. - generate() maps the error to GenerationError::PitchSelection (via From), so an impossible range surfaces explicitly rather than emitting an out-of-palette floor. - complement's band_scale_ladder keeps its pre-existing bare-lo floor on the error (its arranger contract is separate from the generator's strict guarantee) — now explicit, not silent. Co-Authored-By: Claude <noreply@anthropic.com>
The local register_scan harness re-implemented — and drifted from — the production corpus->generation compiler (duration-only rhythms, mean instead of median gesture aggregation, different gesture-record handling). Extract the one implementation into a griff_cli library so the binary and any experimental A/B harness share it (arbiter): - new cli/src/lib.rs (griff_cli) + cli/src/generation_input.rs hold generation_request_from_score, load_corpus_material, gesture_control_from_chunks, bar_rhythms, CorpusMaterial, and the shared primary_voice_note_count. The module is #[doc(hidden)] and documented as an experimental, stability-exempt tooling seam; file I/O stays in the CLI (not core), the pure transforms stay in griff-core. - generation_input carries its own GenerationInputError; main.rs maps it to CliError via From. Pure move — no behaviour change; the existing unit tests now exercise the public seam. No blanket lint allows added. Full workspace tests, clippy --workspace --all-features, fmt, and the cockpit wasm32 build all green. Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 50 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. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThe PR introduces onset-aware rhythm templates, shared pitch ladders, deterministic candidate generation and reranking, corpus-fed CLI generation, Guitar Pro import fixes, expanded tests, and updated generation documentation. ChangesGeneration pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 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: 2374b31735
ℹ️ 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".
| /// Fully deterministic: the same request always produces the same set, and | ||
| /// variant seeds do not depend on the gesture ask (so gestured and plain runs | ||
| /// pair up seed-for-seed). | ||
| pub fn generate_candidate_set(request: &SetRequest) -> Result<Vec<SetCandidate>, SetError> { |
There was a problem hiding this comment.
The repository’s AGENTS.md TDD workflow says not to commit new pub fn / pub struct implementation in the same commit as the tests that cover it. This commit adds the new public rerank API implementation here while also adding core/tests/rerank.rs and related test suites, so the required red/green commit sequence cannot be validated; please split the tests and implementation into separate commits.
Useful? React with 👍 / 👎.
| let templates: Vec<&RhythmTemplate> = request | ||
| .source_rhythms | ||
| .iter() | ||
| .filter(|t| !t.notes.is_empty()) |
There was a problem hiding this comment.
When a corpus supplies templates whose notes are non-empty but all clamp away for the target bar (for example every onset is at or past the bar end, or durations are zero), this filter treats them as usable, so RhythmCopyPitchSubstitute is included and generate falls through to the quarter-note fallback. That produces a misleading rhythm-copy candidate instead of skipping rhythm-copy when no usable rhythm template exists, which can skew the candidate set and rerank experiments for mismatched or malformed corpus rhythms.
Useful? React with 👍 / 👎.
…biter) Full ladder fixed reachability (synthetic in-range/in-class 1.0), but drawing every Shuffle note from the whole ladder regressed coherence: Shuffle wide-synthetic median span ~11->46, octave-leap share ~0->0.62; 10/50 post-ladder production winners crossed the incoherence threshold, all ShuffleMotifs. Corrects the wording: the reranker does NOT prefer wide register — it does not penalize register incoherence, so incoherent candidates score well on the closure/novelty axes and reach the output. Figures are local experimental evidence, not product thresholds. Register acceptance blocked pending a Shuffle-window A/B. Co-Authored-By: Claude <noreply@anthropic.com>
Failing tests for the register A/B blocker (arbiter): ShuffleMotifs drawing
every note from the whole ladder blew the register up (wide-synthetic median
span ~11->46, octave-leap share ~0->0.62). The fix is a deterministic
per-candidate contiguous ladder window (<= one octave span) for Shuffle only.
Pins:
- ScaleLadder::octave_window(selector) -> LadderWindow: span <= 12 semitones,
never empty, selectors cover the full ladder (no top/bottom bias), narrow
and single-rung ladders work.
- ShuffleMotifs candidates stay within one octave, in-bounds and in-class,
zero octave-leap share; variants together still reach below / middle /
above the first octave (reachability retained); deterministic; narrow and
single-rung ranges work; no empty line.
- RegisterStats::measure (diagnostic only, NOT wired into rerank).
pitch::{LadderWindow, RegisterStats} and octave_window do not exist yet, so
the suite fails to compile until the green step.
Co-Authored-By: Claude <noreply@anthropic.com>
…window Green step for the register A/B blocker (arbiter): ShuffleMotifs drawing every note from the whole ladder lost local coherence (wide-synthetic median span ~11->46, octave-leap share ~0->0.62). Fix, Shuffle-only: - ScaleLadder::octave_window(selector) -> LadderWindow: a contiguous slice spanning <= one octave (semitone span, not a fixed degree count, so chromatic vs pentatonic density both work). The low anchor is chosen from rungs that leave a full octave above them, so a top-edge selector does not shrink the window and selectors cover the whole ladder without top/bottom bias. Never empty; narrow and single-rung ladders degrade cleanly. - ShuffleMotifs derives one window per candidate from its seed and draws every note from it: candidates stay within an octave (locally coherent, zero octave-leap share) while variants together still reach the full register. The other four strategies are untouched. - RegisterStats::measure (mean/max abs interval, octave-leap share, pitch stddev) is a pure diagnostic for tests and the A/B harness — NOT wired into rerank_weights_v1 (repairing generation is separate from ranking; no rerank axis, no octave-leap rejection, no weight change). Verified live: Shuffle-winner outputs now span exactly one octave (was ~37+); non-Shuffle winners unchanged. Generate goldens re-blessed only where a Shuffle candidate's score/position moved — the other strategies' candidate lines are byte-identical. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…change) Diagnostic-only correction (arbiter, non-blocking). RegisterStats counted strictly > 12 but the doc/test called exact-12 intervals octave leaps, and the old test assertion (share < 1e-9 || share <= 1.0) was tautological. - Rename octave_leap_share -> over_octave_share (> 12, documented), and add exact_octave_share (== 12) and at_least_octave_share (>= 12) so the octave families are unambiguous. - register_stats_measure_interval_shape now asserts [40,52,64] concretely: mean 12, max 12, over-octave 0, exact-octave 1, at-least 1. - New mixed-interval test [50,55,67,80] (intervals 5, 12, 13) separates the three shares: over 1/3, exact 1/3, at-least 2/3. RegisterStats is a pure diagnostic, still NOT wired into rerank_weights_v1. No change to LadderWindow, the Shuffle algorithm, the reranker, weights, or any other strategy — generated output is byte-identical (no golden churn). Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@cli/src/generation_input.rs`:
- Around line 145-170: Update first_bar_rhythm to use the same primary-voice
extraction path as bar_rhythms instead of flattening notes from every track and
voice. Preserve the existing first-bar selection and quarter-note fallback,
while ensuring polyphonic material produces the same sequential rhythm as the
primary-voice corpus path and remains limited to the bar’s tick range.
In `@core/src/generate.rs`:
- Around line 324-328: Update the RhythmCopyPitchSubstitute validation in the
generation flow to detect templates whose rhythms become empty after clamping,
not just templates with no notes initially. Reuse the same
clamping/effective-grid logic used by bar_grids, and return
GenerationError::RhythmTemplateMissing when no effective grids remain; preserve
normal generation for templates retaining at least one grid.
🪄 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: 3580c64e-a2c3-4f34-a7c7-9548fd24f685
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
cli/Cargo.tomlcli/src/generation_input.rscli/src/lib.rscli/src/main.rscli/tests/generate_corpus.rscli/tests/snapshots/generate__multi_track.txtcli/tests/snapshots/generate__seven_eight.txtcli/tests/snapshots/generate__simple_4_4.txtcli/tests/snapshots/generate__tempo_change.txtcli/tests/snapshots/generate__two_phrases.txtcore/Cargo.tomlcore/src/complement.rscore/src/generate.rscore/src/gp.rscore/src/lib.rscore/src/pitch.rscore/src/rerank.rscore/src/structure.rscore/tests/characterization.rscore/tests/pitch_ladder.rscore/tests/rerank.rscore/tests/rhythm_grid.rscore/tests/rule_generator.rscore/tests/shuffle_window.rscore/tests/snapshots/generate__deterministic_7_8.txtcore/tests/structure_control.rsdocs/decisions.log.mddocs/stages/S6-rule-generator-v0.mdfuzz/fuzz_targets/generation_request.rsfuzz/fuzz_targets/gesture_request.rs
Comment on lines
324
to
328
| if request.strategy == GenerationStrategy::RhythmCopyPitchSubstitute | ||
| && request.source_rhythms.first().map_or(true, Vec::is_empty) | ||
| && !request.source_rhythms.iter().any(|t| !t.notes.is_empty()) | ||
| { | ||
| return Err(GenerationError::RhythmTemplateMissing); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm clamp_template can empty a non-empty template and that generate() relies only on the notes.is_empty() guard.
rg -nP -C3 'RhythmTemplateMissing|effective_grids|fn clamp_template' core/src/generate.rsRepository: PhysShell/griff
Length of output: 2368
🏁 Script executed:
#!/bin/bash
sed -n '90,140p' core/src/generate.rs
printf '\n---\n'
sed -n '180,210p' core/src/generate.rs
printf '\n---\n'
sed -n '320,360p' core/src/generate.rsRepository: PhysShell/griff
Length of output: 5027
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from pathlib import Path
text = Path('core/src/generate.rs').read_text()
for needle in ['effective_grids(', 'clamp_template(', 'RhythmCopyPitchSubstitute', 'quarter_grid(']:
print(f'### {needle}')
idx = text.find(needle)
if idx != -1:
start = max(0, text.rfind('\n', 0, idx-200))
end = min(len(text), idx+700)
print(text[start:end])
print()
PYRepository: PhysShell/griff
Length of output: 4007
RhythmCopyPitchSubstitute should reject templates that collapse after clamping.
A non-empty template can still reduce to no effective grids after clamping, and bar_grids then falls back to the quarter grid. This strategy should return RhythmTemplateMissing for that case instead of silently generating quarter-grid output.
🤖 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 `@core/src/generate.rs` around lines 324 - 328, Update the
RhythmCopyPitchSubstitute validation in the generation flow to detect templates
whose rhythms become empty after clamping, not just templates with no notes
initially. Reuse the same clamping/effective-grid logic used by bar_grids, and
return GenerationError::RhythmTemplateMissing when no effective grids remain;
preserve normal generation for templates retaining at least one grid.
Failing tests for the anchor-selection bias (arbiter): Shuffle drew the window via octave_window(prng.next_mod(ladder.len())), and octave_window re-mods by the anchor count internally — a double modulo that roughly doubles the lower anchors' probability when len is not divisible by the anchor count (chromatic 28..64: len 37, anchors 25, so anchors 0..11 occur twice). Pins an explicit anchor API: octave_window_count() (the full-octave anchor count) and octave_window(anchor_index). Tests: chromatic and pentatonic anchor counts; every anchor is a non-empty ≤12-semitone window; the first/last anchors reach the low/high ends; cycling 0..count visits each window once and wraps to 0 (the sole modulo). octave_window_count does not exist yet, so the suite fails to compile until the green step. Co-Authored-By: Claude <noreply@anthropic.com>
Shuffle drew its window via octave_window(prng.next_mod(ladder.len())), and octave_window re-modded by the anchor count internally — a double modulo that roughly doubled the lower anchors' probability when the ladder length is not divisible by the anchor count (chromatic 28..64: len 37, anchors 25, so anchors 0..11 occurred twice). - Add ScaleLadder::octave_window_count (the full-octave anchor count) and document octave_window's parameter as an anchor index, not a selector. - Shuffle now draws the anchor over the anchor count: octave_window(prng.next_mod(ladder.octave_window_count())) — one modulo, uniform anchors, variants still cover the full ladder. Shuffle-only: the four other strategies are byte-identical (their golden aggregates are unchanged); only ShuffleMotifs' aggregates shift, reordering the ranking and flipping the two_phrases winner to RhythmCopy. Generate CLI goldens re-blessed for that shuffle provenance. This is commit A of the wrap-free register work — the unbiased-window baseline for the RhythmCopy/ Repeat A/B delta. Co-Authored-By: Claude <noreply@anthropic.com>
Failing tests for the residual register defect (arbiter, commit B). After the Shuffle window fix, the production winners with a max interval > 12 were all RhythmCopyPitchSubstitute: its ascending degree walk wrapped modulo the whole ladder (top -> 0), a 44-57 semitone downward jump. RepeatVariation had a latent twin (its variation degree wrapped base + 2 modulo the ladder), caught here with a wide seed sweep. Contract: - RhythmCopy: consecutive notes move by one ladder rung (no high->low wrap), no interval > 12, no prolonged top clamp, union reaches low/middle/high, deterministic, in-class/in-bounds. - RepeatVariation: variation stays a local move (no full-ladder wrap, no interval > 12), narrow one/two-rung ladders work. The current wrapping code fails both, so the suite is red until the reflecting cursor / local displacement land. Co-Authored-By: Claude <noreply@anthropic.com>
…peat wrap Green step for commit B. The residual over-octave winners were all RhythmCopyPitchSubstitute, whose ascending degree walk wrapped modulo the ladder (top -> 0 -> a 44-57 semitone drop); RepeatVariation had the same latent wrap in its base + 2 variation degree. - New reflecting DegreeCursor: walks the full ladder one rung at a time and reverses at the ends (... 3 4 3 2 1 0 1 ...) instead of wrapping; a one-rung ladder stays put. RhythmCopy uses it, so consecutive notes are always adjacent rungs (no interval > 12, no high->low jump, no top-clamp dwell) while still traversing the whole register. advance_degree (modulo wrap) is removed. - RepeatVariation's variation degree is now a local two-rung displacement (base + 2, reflected to base - 2 near the top; one-rung fallback on short ladders), so it differs from base when possible without a full-ladder wrap. Touches RhythmCopy and RepeatVariation only: ShuffleMotifs, MotifTranspose, and ConstrainedRandomWalk are byte-identical (their golden aggregates and the core CRW determinism golden are unchanged). CLI generate goldens re-blessed for the RhythmCopy/Repeat provenance. This is commit B — the after-wrap point of the register A/B delta. Co-Authored-By: Claude <noreply@anthropic.com>
Whitespace-only wrap of an assert_eq! left unformatted by the red-A commit. Test contract unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
…rids
Failing tests for the proven dense-grid defect (arbiter): the variation degree
was local to base_degree, but replaces the LAST note of the ascending base bar.
On a dense grid the base bar climbs (and clamps) far from base_degree, so the
penultimate note is near the top while the variation sits near base — a large
intra-bar drop (chromatic len 37, base 0, 32-note grid: penultimate ~30,
var ~2, ~28-semitone jump). It can also fail to differ from the final note when
base is near the top (varied last clamps to the top too).
Matrix: grid sizes 4/6/8/16/32/64 x {wide chromatic, wide pentatonic, narrow
two-rung, single-rung}, seeds 0..256; asserts whole-line and penultimate->
varied-last intervals <= 12, in-class/in-bounds, determinism, and that the
variation differs from the base bar's final note when possible — plus an
explicit counterexample test. The current base-local code fails, so the suite
is red until the endpoint-local fix lands.
Co-Authored-By: Claude <noreply@anthropic.com>
…rids The proven dense-grid defect: the variation degree was local to base_degree but replaces the ascending base bar's LAST note. On a dense grid the bar climbs far from base_degree (chromatic len 37, base 0, 32-note grid: penultimate ~30), so a base-local variation (~degree 2) was a ~28-semitone intra-bar drop, and near the top it could also fail to differ from the final note. New variation_degree() computes the bar's endpoint trajectory (last/penultimate degrees) and: - keeps the historical base +/- 2 preference when it is already within two rungs of the actual penultimate AND differs from the final degree (the common short-grid case is byte-unchanged); - otherwise picks a rung around the penultimate (penult +/- 1/2), in bounds, not the final degree when an alternative exists, nearest the old preferred degree, deterministic tie-break. RepeatVariation only: RhythmCopy, Shuffle, MotifTranspose, and CRW are byte-identical (the non-Repeat golden strategy/seed/aggregate set is unchanged; only ranks renumber). The phrase-boundary reset (each bar restarts the ascending figure) is call/response identity and stays unbounded by design; the dense-grid tests bound the intra-bar and penultimate->varied-last intervals. CLI generate goldens re-blessed for the RepeatVariation provenance. Co-Authored-By: Claude <noreply@anthropic.com>
Records the wrap-free A/B result (RhythmCopy/Repeat/winner >12 all -> 0, non-target 500/500 identical, mean aggregate 0.8896 -> 0.8951) and accepts the register track: full-range reachability, unbiased Shuffle window, wrap-free RhythmCopy, endpoint-local RepeatVariation, no register rerank axis. Includes the arbiter's documentation corrections (scan = 10 variants/strategy; winner command used the default 2; 25.36->6.02 is the mean, 57->12 the max; bias CSV records output minima). TonalCenter, cadence, and reranker policy v2 stay frozen. Co-Authored-By: Claude <noreply@anthropic.com>
Design note (no behavior change) for the next increment after the accepted register track. Audits the current contract (generation seeds a pitch-class palette + range with PitchMaterial.root as an anchor, not a tonic) and names the tonal inference already in-tree: complement::estimate_harmony (Krumhansl-Schmuckler, duration-weighted, single winner, part-scoped, pub(crate)). Proposes a typed pure-core layer that GENERALISES that one estimator rather than adding a second: PitchEvidence (onset + duration pitch-class weights, range, counts, EvidenceScope) separated from a scored, uncertain TonalEstimate (candidate list + confidence_margin), with HarmonicContext becoming its projection. Heuristics-first (ADR-0008); weights are data (ADR-0017). Fixes an eight-case synthetic test plan (clean major/minor, pentatonic and chromatic ambiguity, conflicting-key tracks, melodic+noise, tonic pedal, modulating score) mapped to high / low / ambiguous / unsupported outcomes, and borrows only the key/root/degree/quality/confidence decomposition from AugmentedNet (no TensorFlow, no MusicXML runtime, no Roman-numeral model). Guardrails: RuleGenerationRequest, PitchMaterial, ScaleLadder, the strategies, the reranker, and cadence are untouched; production tonal inference waits on the local tonal_evidence scan. Co-Authored-By: Claude <noreply@anthropic.com>
Pins the `griff_core::tonal` contract generalising the private `complement::estimate_harmony`: - `PitchEvidence` measured for an explicit `EvidenceScope` (whole-score / track / voice) — raw onset counts, duration mass in ticks, observed `feature::PitchRange` — additive across scopes (whole score = Σ tracks = Σ voices); - `estimate_key` → a ranked 24-key `TonalEstimate` with an explicit `confidence_margin`; each `TonalCandidate` carries tonic, `KeyMode`, correlation and per-candidate `scale_fit`; - an exactly-flat histogram correlates to a finite zero everywhere, margin zero, C-major-first order (an ordering convention, not confidence); - KS v1 is duration-only: duration mass decides, raw onset counts are the fallback only when total duration mass is zero; - reproduces complement's minor-key winner and tie order. References `griff_core::tonal`, absent until the green step. Co-Authored-By: Claude <noreply@anthropic.com>
Generalise the private, single-winner `complement::estimate_harmony` into a pure-core `tonal` module split into evidence and inference: - `PitchEvidence::measure(score, scope)` projects an explicit `EvidenceScope` (whole-score / track / voice) into raw `onset_counts: [u32;12]`, `duration_mass: [u64;12]` (summed ticks), `note_count`, and the observed `feature::PitchRange` — additive across scopes (whole = Σ tracks = Σ voices); - `estimate_key` ranks all 24 keys best-first into a `TonalEstimate` with an explicit `confidence_margin`; each `TonalCandidate` carries tonic, `KeyMode`, Pearson correlation and its own `scale_fit`; - KS v1 stays duration-only: duration mass weights the histogram, raw onset counts are the fallback only when total duration mass is zero; no onset/duration blend, no metric-accent policy; - a stable descending sort reproduces `estimate_harmony`'s strict-greater winner and tie order, so an exactly flat histogram scores every key at a finite 0.0, margin 0.0, C-major-first (an order, not confidence). `KeyMode` (and its profiles/correlation) move to `tonal`; `complement` re-exports `KeyMode` and `estimate_harmony` now projects the winning candidate into `HarmonicContext`. No generation/reranker/cadence/ track-selection change; `PitchMaterial` untouched. Existing complement and structure suites stay green (no golden change). Co-Authored-By: Claude <noreply@anthropic.com>
Mark the Phase 0 note's Phase 1 bullet landed and add §7 reconciling what shipped with the §2 sketch: raw integer histograms (`onset_counts` `[u32;12]`, `duration_mass` `[u64;12]`) rather than f64 weights, a per-candidate `scale_fit`, `KeyMode` reused, and the scope kept on the evidence (not duplicated onto the estimate). §7 also states, per the arbiter, what the facts are and are not: a real-song key is a tonal hypothesis for a scope (not verified truth); onset counts are raw facts; duration mass is summed sounded ticks (not wall-clock time); every candidate carries scale_fit; confidence thresholds and automatic scope selection remain uncalibrated (the diatonic ≈ 0.076 / pentatonic ≈ 0.085 margins are diagnostics, not cutoffs). Append a decisions-log entry for the increment. Co-Authored-By: Claude <noreply@anthropic.com>
Record the acceptance of the shared tonal evidence/inference layer after green, independently-reviewed local equivalence validation. Cloud implementation: red 6f9114d, green 184b586, docs e2c9c7f. Local validation commit: bd2c7c8. Archival/housekeeping follow-up: 3993bb0 (adds phase1_evidence.jsonl, regenerates the smoke CSV with full 64-hex SHA-256, reconfirms 30/30 via cmp, documents the comparison method). Acceptance figures: HarmonicContext 16/16 (0 changed); structure consumer 7/7 byte-identical; core evidence mapping 39/39 (0 mismatches); histogram additivity PASS; 24 finite candidates/scope PASS; generation smoke 30/30 identical. No generation behavior changed. Confidence thresholds uncalibrated; automatic scope selection unapproved; generation integration, cadence, and Phase 2 all frozen. Co-Authored-By: Claude <noreply@anthropic.com>
This was referenced Jul 15, 2026
Merged
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
2 participants
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.
Failing tests for griff_core::rerank: generate_candidate_set fans a base
request out over every S6 strategy × seed variants (deterministic derived
seeds, template rotation across variants, optional gesture carving, rhythm-copy
skipped without a template), and rerank_candidates scores each candidate on
the four closure axes plus the two novelty axes under a caller WeightPolicy,
returning Scored envelopes in rank order. This is the wiring the
melodic-closure note left open (§7.2 rerank, §7.3 caller-side novelty cut;
ADR-0017) and the S6 stage doc's promised Vec.
The griff_core::rerank API does not exist yet, so the suite fails to compile
until the green step.
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01VBNEW7XcGqRzyf8WfqRKVU
Summary by CodeRabbit
New Features
--corpus,--candidates, and--no-gestureoptions togriff generate.Bug Fixes
Tests