Skip to content

feat(core): the explicit rhythm scheduler — silent bars survive the rotation (S16 Phase 2, slice 2) - #115

Merged
PhysShell merged 2 commits into
mainfrom
claude/s16-phase2-rhythm-override
Jul 14, 2026
Merged

feat(core): the explicit rhythm scheduler — silent bars survive the rotation (S16 Phase 2, slice 2)#115
PhysShell merged 2 commits into
mainfrom
claude/s16-phase2-rhythm-override

Conversation

@PhysShell

Copy link
Copy Markdown
Owner

Summary

The rhythm override in the shared generation compiler (ADR-0029 §7), built as the #114 review demanded: a separate explicit scheduler, not another if in front of effective_grids — otherwise a silent bar is sacrificed to the old seam's convenience and a four-bar cycle quietly becomes a three-bar one.

Commit sequence (TDD)

  1. Red (f28031b) — the four laws as tests (core/tests/explicit_rhythm.rs), explicit paths unimplemented!(), plus the compile plumbing with every construction site saying explicit_rhythms: None. The two characterization tests (laws that pin existing behavior) are green in red, per AGENTS' exemption.
  2. Green (0163b46) — explicit_grids (one grid per template, palette order and count, clamp but never filter, no quarter fallback), explicit_rhythm_diagnostics (uncompressed), and active-palette rhythm-copy checks.

The four laws (#114 review), as landed

  1. Explicit palettes keep silent positions: a [sounding, silent] palette over four bars generates 2 0 2 0 notes, onsets exactly 0 960 3840 4800; an all-silent palette stays silent — no quarter fallback.
  2. Corpus/source keep today's filtering: characterization tests watch the automatic path fall back to quarters byte-for-byte.
  3. The explicit scheduler rotates the original vector, silent bars included — it is a distinct function with its own doc explaining why.
  4. Diagnostics never compress the palette: explicit_rhythm_diagnostics reports loaded == effective == fingerprints.len(), contrasted in the same test against rhythm_diagnostics compressing the same palette to one grid.

Surface changes

  • RuleGenerationRequest / SetRequest gain explicit_rhythms: Option<Vec<RhythmTemplate>> (wins over source_rhythms outright);
  • ranked_candidates gains a fourth parameter rhythm_override: Option<&[RhythmTemplate]> — precedence explicit > corpus > source first bar, novelty references and gesture stay corpus-based;
  • RankedSet gains rhythm_explicit: bool and its source_rhythms doc now promises verbatim provenance;
  • all three frontend call sites (CLI, ui-core→cockpit) pass None — behavior unchanged until the CLI slice arrives.

Local gate: clippy -D warnings clean; workspace tests green except the pre-existing Windows-locale missing_file_golden.

Next slice: the namespaced --rhythm-* CLI (kernel literal, mandatory unit, tail policy, independent seed, SWG diagnostic mapping incl. the honest zero-bar error) and the versioned expansion artifact.

Refs #108, ADR-0029, S16 Phase 2.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NkqJUU6d1sW1RAfvyHrqVM

PhysShell and others added 2 commits July 15, 2026 00:02
…16 Phase 2)

The four laws from the #114 review, as tests, with the explicit paths
unimplemented!(): an explicit palette (ADR-0029 §7) is honored by its
own scheduler, not by effective_grids with an if — otherwise a silent
bar is sacrificed to the old seam's convenience and a four-bar cycle
quietly becomes a three-bar one.

- law 1+3 (red): a [sounding, silent] palette generates
  2, 0, 2, 0 notes across four bars, onsets exactly on the template's
  offsets — the silent template is a bar of the cycle, not a dropped
  entry; an all-silent palette stays silent instead of falling back to
  quarters;
- law 2 (green, characterization): the automatic path keeps its
  empty-filtering and quarter fallback byte-for-byte;
- precedence (red): ranked_candidates gains a rhythm_override that
  beats corpus rhythms while novelty references and gesture stay
  corpus-based, and RankedSet reports the palette verbatim plus a
  rhythm_explicit provenance flag;
- law 4 (red): explicit_rhythm_diagnostics reports loaded == effective
  == fingerprints.len() — contrasted in the same test against the
  automatic diagnostics compressing the same palette to one grid.

Plumbing that lets the tests compile: RuleGenerationRequest and
SetRequest carry explicit_rhythms: Option<Vec<RhythmTemplate>> (all
construction sites say None), generate() routes Some(palette) to the
unimplemented explicit_grids, and the three frontend call sites pass
None — baseline behavior is untouched, which the green suites confirm.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NkqJUU6d1sW1RAfvyHrqVM
…hase 2)

explicit_grids maps the palette one grid per template, in the palette's
own order and count: notes still clamp to the bar, an empty template
stays an empty grid — a silent bar in the rotation — and there is no
quarter fallback. This is deliberately a separate scheduler, not
effective_grids with an if: the two answer to different laws, and the
automatic corpus/source path keeps its filtering and fallback
byte-for-byte (the characterization tests watched).

explicit_rhythm_diagnostics reports the palette uncompressed
(loaded == effective == fingerprints.len(), the silent template
fingerprinting as itself). The rhythm-copy presence checks — in
generate() and in the set builder's skip logic — consult whichever
palette is active, so an explicit palette with one sounding template
keeps RhythmCopyPitchSubstitute in the set.

All six explicit_rhythm tests green; onsets of a [sounding, silent]
2-cycle over four bars land exactly at 0, 960, 3840, 4800.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NkqJUU6d1sW1RAfvyHrqVM
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Warning

Review limit reached

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

Next review available in: 20 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: eae35bda-2c10-49f8-8287-751db0b36bab

📥 Commits

Reviewing files that changed from the base of the PR and between 38efeaa and 0163b46.

📒 Files selected for processing (17)
  • cli/src/main.rs
  • core/src/complement.rs
  • core/src/generate.rs
  • core/src/generation_input.rs
  • core/src/rerank.rs
  • core/src/structure.rs
  • core/tests/characterization.rs
  • core/tests/explicit_rhythm.rs
  • core/tests/gesture_control.rs
  • core/tests/pitch_ladder.rs
  • core/tests/rerank.rs
  • core/tests/rhythm_grid.rs
  • core/tests/rule_generator.rs
  • core/tests/shuffle_window.rs
  • core/tests/structure_control.rs
  • core/tests/wrap_free_traversal.rs
  • ui-core/src/generate.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/s16-phase2-rhythm-override

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

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

Copy link
Copy Markdown
Owner Author

Architecture/code review — Phase 2 slice 2 accepted

The implementation satisfies the four override laws and keeps the automatic path isolated:

  • explicit_grids preserves palette order and cardinality, clamps notes but never filters templates, and has no quarter fallback;
  • generate chooses the active scheduler by source, while RhythmCopyPitchSubstitute checks the active palette rather than the automatic one;
  • ranked_candidates implements explicit > corpus > source, but corpus references and gesture remain corpus-owned;
  • RankedSet keeps the explicit palette verbatim plus a provenance bit;
  • explicit_rhythm_diagnostics reuses the canonical fingerprint implementation without compressing silent positions;
  • all existing call sites pass None, and the existing deterministic characterization remains green.

No merge blocker found.

Carry into the final CLI/artifact slice

  1. All-silent explicit input needs an explicit frontend decision. The direct generator correctly produces silence with no quarter fallback, but the current reranker drops note-free candidates (closure_axes cannot score them), and gestured generation cannot measure an empty line. Before accepting an all-. kernel in CLI, choose and test one contract: either a typed diagnostic for “no active onsets”, or a deliberate non-ranked silent-output path. Do not let it decay into “no candidates” with an unrelated message.

  2. Scope the rotation promise. Four per-bar-independent strategies rotate the explicit palette. RepeatVariation intentionally keeps grids[0] under the pre-existing S6 contract. CLI help/artifact prose should preserve that exception rather than claiming every strategy rotates.

  3. The final diagnostic path should select explicit_rhythm_diagnostics when rhythm_explicit == true; using the automatic diagnostic would immediately re-compress the palette we just worked rather hard not to compress.

CI, cockpit-web-test, and CodeRabbit are green on 0163b460c94fe3a284059fc641bf5852616adf01; no unresolved review threads. Verdict: merge-ready.

@PhysShell
PhysShell merged commit 7751728 into main Jul 14, 2026
4 checks passed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0163b460c9

ℹ️ 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".

Comment thread core/src/generate.rs
Comment on lines +127 to +130
pub fn explicit_rhythm_diagnostics(
palette: &[RhythmTemplate],
bar_duration: Ticks,
) -> RhythmDiagnostics {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Split the scheduler implementation from its tests

AGENTS.md's TDD workflow requires a failing-test commit before implementation and says never to commit a new pub fn implementation in the same commit as the tests that cover it. This commit adds core/tests/explicit_rhythm.rs alongside this new public scheduler API, so the required red/green split is missing; please split the history into a red test commit and a later implementation commit.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant