Add VariationControl: deterministic pitch-spread knob for grid-locked… - #64
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a ChangesPitch Variability Control for Complement Generation
Sequence Diagram(s)sequenceDiagram
participant Caller
participant arrange_complement_varied
participant inner_arranger
participant spread_window
participant grid_locked_groups
participant realized_band_spread
Caller->>arrange_complement_varied: score, spec, seed, VariationControl{pitch_spread}
arrange_complement_varied->>arrange_complement_varied: is_valid(control)
arrange_complement_varied->>inner_arranger: control (threaded)
inner_arranger->>spread_window: pitch_spread + ladder_len
spread_window-->>inner_arranger: window_size
inner_arranger->>grid_locked_groups: window_size for pitch_index modulo
grid_locked_groups-->>inner_arranger: ComplementCandidate
inner_arranger-->>arrange_complement_varied: ComplementCandidate
arrange_complement_varied->>realized_band_spread: B track + register band
realized_band_spread-->>arrange_complement_varied: f64 spread fraction
arrange_complement_varied-->>Caller: VariedComplement{complement, control, realized_spread}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 `@core/src/complement.rs`:
- Around line 89-134: The public API additions (VariationControl,
VariedComplement, VariationError structs/enums and arrange_complement_varied
function) were committed together with their test coverage, violating the
requirement to commit tests before implementation. Reorganize the commit history
by first creating a commit containing only the test functions for variation
control in core/tests/complement.rs (tests like
variation_full_spread_equals_plain_arrange,
variation_zero_spread_collapses_b_to_a_single_pitch, etc.), then create a second
separate commit with the public API implementation in core/src/complement.rs
containing the VariationControl struct, VariedComplement struct, VariationError
enum, and the arrange_complement_varied function.
In `@core/tests/complement.rs`:
- Around line 341-355: The test variation_rejects_out_of_range_control checks
for positive infinity (f64::INFINITY) in the invalid control rejection matrix
but does not check for negative infinity, which contradicts the contract
requirement to reject ±∞. Add f64::NEG_INFINITY to the array of bad values in
the for loop alongside the existing test cases (1.5_f64, -0.1, f64::NAN, and
f64::INFINITY).
🪄 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: f0a098cf-cfd2-40e3-a6f8-d88d4ce0b9d1
📒 Files selected for processing (4)
core/src/complement.rscore/tests/complement.rsdocs/adr/0023-variation-control-for-complement.mddocs/adr/README.md
Comment on lines
+89
to
+134
| #[derive(Debug, Clone, Copy, PartialEq)] | ||
| pub struct VariationControl { | ||
| /// Fraction of the band's scale ladder B may use, `0.0..=1.0`. `0.0` pins | ||
| /// every note to the band's anchor degree (a static line, still locked to | ||
| /// A's grid); `1.0` uses the whole band — the unconstrained default that | ||
| /// matches [`arrange_complement`]. | ||
| pub pitch_spread: f64, | ||
| } | ||
|
|
||
| impl VariationControl { | ||
| /// The identity control: the whole band, i.e. [`arrange_complement`]'s | ||
| /// behaviour. | ||
| pub const FULL: Self = Self { pitch_spread: 1.0 }; | ||
| /// A static line: B collapses onto the band's anchor degree. | ||
| pub const LOCKED: Self = Self { pitch_spread: 0.0 }; | ||
|
|
||
| /// Whether the control is in range: a finite `pitch_spread` within | ||
| /// `0.0..=1.0`. | ||
| #[must_use] | ||
| pub fn is_valid(self) -> bool { | ||
| self.pitch_spread.is_finite() && (0.0..=1.0).contains(&self.pitch_spread) | ||
| } | ||
| } | ||
|
|
||
| /// A varied complement: the arranged candidate plus provenance — the control | ||
| /// that asked for it (ask) and B's realised pitch spread (is). | ||
| #[derive(Debug, Clone)] | ||
| pub struct VariedComplement { | ||
| /// The arranged candidate (A's score with B appended). | ||
| pub complement: ComplementCandidate, | ||
| /// The control this candidate was arranged against. | ||
| pub control: VariationControl, | ||
| /// B's realised pitch ambitus as a fraction of the target band, | ||
| /// `0.0..=1.0` — what the spread actually *is*, not what was asked. | ||
| pub realized_spread: f64, | ||
| } | ||
|
|
||
| /// Errors the varied arrangement entry point can emit. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum VariationError { | ||
| /// The [`VariationControl`] is out of range (non-finite or outside | ||
| /// `0.0..=1.0`). | ||
| InvalidControl, | ||
| /// The underlying arrangement failed. | ||
| Arrange(ComplementError), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Commits touching implementation/tests =="
git log --reverse --format='%h %ad %s' --date=short -- core/src/complement.rs core/tests/complement.rs
echo
echo "== Commits that introduce new public variation API in core/src/complement.rs =="
for c in $(git rev-list --reverse HEAD -- core/src/complement.rs); do
if git show --pretty='' --unified=0 "$c" -- core/src/complement.rs | \
rg -n '^\+pub (struct VariationControl|struct VariedComplement|enum VariationError|fn arrange_complement_varied)'; then
echo "api_commit=$c"
fi
done
echo
echo "== Commits that add variation-control tests in core/tests/complement.rs =="
for c in $(git rev-list --reverse HEAD -- core/tests/complement.rs); do
if git show --pretty='' --unified=0 "$c" -- core/tests/complement.rs | \
rg -n '^\+fn variation_|VariationControl::(FULL|LOCKED)|arrange_complement_varied|VariationError::InvalidControl'; then
echo "test_commit=$c"
fi
doneRepository: PhysShell/griff
Length of output: 1843
Public API implementation and tests must be in separate commits.
The new VariationControl struct, VariedComplement struct, VariationError enum, and arrange_complement_varied function were introduced in the same commit (386f57d) as their test coverage. Per coding guidelines, public API implementation must never be committed together with tests that cover it. Tests should be committed first (TDD red phase), followed by implementation commits (green/refactor phases) in separate commits.
Please reorganize the commit history:
- Commit 1: Add test functions for variation control (
variation_full_spread_equals_plain_arrange,variation_zero_spread_collapses_b_to_a_single_pitch, etc.) in core/tests/complement.rs only - Commit 2: Implement the public API additions in core/src/complement.rs
🤖 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/complement.rs` around lines 89 - 134, The public API additions
(VariationControl, VariedComplement, VariationError structs/enums and
arrange_complement_varied function) were committed together with their test
coverage, violating the requirement to commit tests before implementation.
Reorganize the commit history by first creating a commit containing only the
test functions for variation control in core/tests/complement.rs (tests like
variation_full_spread_equals_plain_arrange,
variation_zero_spread_collapses_b_to_a_single_pitch, etc.), then create a second
separate commit with the public API implementation in core/src/complement.rs
containing the VariationControl struct, VariedComplement struct, VariationError
enum, and the arrange_complement_varied function.
Source: Coding guidelines
Pin the ADR-0023 ask — arrange_complement_varied, VariationControl, VariationError — before the implementation exists. The suite references symbols that aren't defined yet, so it fails to compile: the red step. https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
ADR-0023 records the decision: a pitch-variability *ask* orthogonal to RelationMode, the GestureControl/StructureControl duality (ADR-0012/0015), compiled over the arranger as a separate entry point — never a ComplementSpec change (35 construction sites) — and shaping pitch only, so a grid-locked mode stays grid-locked (rhythm_similarity stays 1.0). First axis `pitch_spread` in [0,1] windows the band's scale ladder: 0 pins B to the band's anchor degree (a static line on A's grid), 1 uses the whole band — the identity window, so `arrange_complement` stays byte-identical and the corpus / goldens / CLI snapshots are unaffected. `arrange_complement_varied` returns a `VariedComplement` carrying the control (ask) and B's realised ambitus fraction (is); out-of-range is the typed `VariationError::InvalidControl`. Deterministic (SPEC §6): the window only narrows the existing seeded pitch hash. Applies to the ladder-substitution modes (rhythm_lock, register_contrast, call_response). Tests (committed first, red) pin identity at full spread, single-pitch collapse at zero, determinism, the typed rejection, and ask-vs-is provenance. https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
PhysShell
force-pushed
the
claude/modest-maxwell-0zecwn
branch
from
June 16, 2026 16:16
386f57d to
69cfb33
Compare
`is_valid()` rejects ±∞ via `is_finite()`; the rejection test only exercised `+∞`. Add `f64::NEG_INFINITY` to the invalid-input matrix (CodeRabbit #64). https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/adr/0023-variation-control-for-complement.md (1)
68-68: ⚡ Quick winStandardize spelling: "realised" → "realized" to match the struct field identifier.
Line 68 uses "realised" (British variant), but the struct field identifier and surrounding text use "realized" (American). For consistency with code identifiers and document conventions, change the prose to match.
📝 Proposed fix
`VariedComplement { complement, control, - realized_spread }` carries the control that asked and B's realised pitch + realized_spread }` carries the control that asked and B's realized pitch ambitus as a fraction of the target band — the `GesturedCandidate` duality.🤖 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 `@docs/adr/0023-variation-control-for-complement.md` at line 68, Change the spelling of "realised" to "realized" in the prose where it appears alongside the struct field identifier. The struct field uses the American spelling variant "realized_spread", so update all instances of "realised" in the surrounding text to match this convention for consistency throughout the document.
🤖 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.
Nitpick comments:
In `@docs/adr/0023-variation-control-for-complement.md`:
- Line 68: Change the spelling of "realised" to "realized" in the prose where it
appears alongside the struct field identifier. The struct field uses the
American spelling variant "realized_spread", so update all instances of
"realised" in the surrounding text to match this convention for consistency
throughout the document.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 69a78fc8-3137-4bac-8af1-73677635cabd
📒 Files selected for processing (4)
core/src/complement.rscore/tests/complement.rsdocs/adr/0023-variation-control-for-complement.mddocs/adr/README.md
✅ Files skipped from review due to trivial changes (1)
- docs/adr/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- core/tests/complement.rs
- core/src/complement.rs
CodeRabbit nitpick #64: the prose used the British "realised" next to the American `realized_spread` identifier. https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
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.
… complements
ADR-0023 records the decision: a pitch-variability ask orthogonal to RelationMode, the GestureControl/StructureControl duality (ADR-0012/0015), compiled over the arranger as a separate entry point — never a ComplementSpec change (35 construction sites) — and shaping pitch only, so a grid-locked mode stays grid-locked (rhythm_similarity stays 1.0).
First axis
pitch_spreadin [0,1] windows the band's scale ladder: 0 pins B to the band's anchor degree (a static line on A's grid), 1 uses the whole band — the identity window, soarrange_complementstays byte-identical and the corpus / goldens / CLI snapshots are unaffected.arrange_complement_variedreturns aVariedComplementcarrying the control (ask) and B's realised ambitus fraction (is); out-of-range is the typedVariationError::InvalidControl. Deterministic (SPEC §6): the window only narrows the existing seeded pitch hash, no new RNG.Applies to the ladder-substitution modes (rhythm_lock, register_contrast, call_response). Red→green tests pin identity at full spread, single-pitch collapse at zero, determinism, the typed rejection, and ask-vs-is provenance.
https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
Summary by CodeRabbit
VariationControlto tune pitch spread for complementary-part generation in grid-locked ladder-substitution modes.realized_spread.pitch_spread, returning a typed error for invalid (out-of-range/NaN/∞) values.