feat(cli): --rhythm-* — the transport syntax reaches the generator (S16 Phase 2, slice 3)

[PhysShell]

Summary

The final Phase 2 slice: the namespaced --rhythm-* transport syntax on griff generate, the SWG diagnostic boundary, and the versioned expansion artifact. A fractal kernel now drives the unchanged S6 generator end to end from one command line.

griff generate seed.gp5 out.mid --bars 4 --seed 42 \
  --rhythm-kernel 'X.X/XX./.XX' --rhythm-fractal-depth 1 \
  --rhythm-density-bps 8000 --rhythm-seed 17 \
  --rhythm-traversal snake --rhythm-unit 1/16 \
  --rhythm-tail rest-pad --emit-rhythm-expansion expansion.json

Commit sequence (TDD)

1. Docs (de81bc3) — registry appends SWG0305 (zero/unrepresentable bar), SWG0306 (a silent expansion is a deliberate typed error, never an empty candidate set — the feat(core): the explicit rhythm scheduler — silent bars survive the rotation (S16 Phase 2, slice 2) #115 review's first obligation), SWG0307 (empty kernel literal), SWG0308 (density out of range); SWG0301 explicitly owns unit representability.
2. Red (fd92d20) — fifteen failing tests for cli::rhythm_pattern: every kernel-literal failure at its code and flag, unit parsing to whole ticks with the PPQN named, bar geometry from the master timeline (SWG0304/0305), the silent-expansion stop (SWG0306), budget mapping (SWG0201/0202), the spec kernel compiling to offsets 0 240 360 480 840 960, artifact byte-stability with geometry fields, fingerprints equal to explicit_rhythm_diagnostics (spec acceptance 16), and SWG0303 defense-in-depth behind clap's requires.
3. Green (f66aac1) — the module implementation, clap wiring (requires chains; max-cells default 4096 documented as a frontend default), CliError::Pattern, the explicit-palette summary line (uncompressed, per law 4), the artifact write before pitch generation, and two blessed integration goldens.

The goldens tell the story

The end-to-end golden ranks ten candidates over a six-template palette whose four silent bars survive verbatim (their fingerprint cbf29ce484222325 is the FNV offset basis — an empty grid, in the rotation, by law). The artifact is byte-stable across runs, --seed leaves it untouched, --rhythm-seed changes it. The ragged-kernel golden exits error[SWG0101] (--rhythm-kernel) before anything is written.

Scope notes

• The RepeatVariation contract (feat(core): the explicit rhythm scheduler — silent bars survive the rotation (S16 Phase 2, slice 2) #115 obligation 2) is unchanged: it holds grids[0] — with an explicit palette that is the palette's first template, silent or not.
• Local gate: clippy -D warnings clean, workspace green except the pre-existing Windows-locale missing_file_golden.

After this merges, Phase 2's remaining act is purely musical: run the pipeline against a Dance Gavin Dance riff from the corpus and listen.

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

🤖 Generated with Claude Code

https://claude.ai/code/session_01NkqJUU6d1sW1RAfvyHrqVM

Summary by CodeRabbit

• New Features

  • Added explicit rhythm-pattern controls to griff generate, including kernel, density, seed, traversal, unit, depth, and tail options.
  • Added optional rhythm expansion artifact output for reproducible pattern inspection.
  • Rhythm patterns now drive candidate generation and provide dedicated palette diagnostics.

• Bug Fixes

  • Added validation and clear errors for malformed kernels, invalid units, unsupported meters, empty patterns, and out-of-range density values.

• Documentation

  • Clarified rhythm mapping rules and diagnostic behavior in the rhythm specification.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 43 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: 86e8d6bf-79de-4466-a3e7-2b2e03c38905

📥 Commits

Reviewing files that changed from the base of the PR and between 4deea51 and fa9edc1.

📒 Files selected for processing (3)

• cli/src/main.rs
• cli/src/rhythm_pattern.rs
• cli/tests/cli.rs

📝 Walkthrough

Walkthrough

griff generate now accepts explicit rhythm kernels and related controls, compiles deterministic rhythm palettes through pattern and SWANG APIs, optionally emits expansion artifacts, integrates templates into ranking, and reports typed diagnostics. Unit, golden, snapshot, and specification updates cover the new behavior.

Changes

Explicit rhythm generation

Layer / File(s)	Summary
Rhythm CLI contracts and validation cli/Cargo.toml, cli/src/lib.rs, cli/src/main.rs, cli/src/rhythm_pattern.rs	Adds pattern dependencies, rhythm CLI options, public adapter types, transport literal parsing, and score-geometry validation.
Pattern compilation and artifact rendering cli/src/rhythm_pattern.rs	Expands and lowers kernels, applies pruning and tail policies, creates per-bar templates, validates the bars window, and renders deterministic expansion JSON.
Generate integration and diagnostics cli/src/main.rs	Compiles explicit rhythm plans before pitch generation, optionally writes artifacts, supplies explicit templates to ranking, and reports typed palette diagnostics.
CLI validation and diagnostic specification cli/src/rhythm_pattern.rs, cli/tests/*, docs/swang/spec.md	Adds parser and compiler tests, golden coverage, snapshots, and documentation for diagnostic locations, registry codes, and tick-compatible units.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant GriffCLI
  participant RhythmPattern
  participant GriffPattern
  participant GriffSwang
  participant CandidateRanking
  User->>GriffCLI: griff generate --rhythm-* options
  GriffCLI->>RhythmPattern: compile_pattern(args, score, bars)
  RhythmPattern->>GriffPattern: expand and linearize kernel
  RhythmPattern->>GriffSwang: lower rhythm expansion
  GriffSwang-->>RhythmPattern: rhythm templates
  RhythmPattern-->>GriffCLI: PatternPlan and artifact JSON
  GriffCLI->>CandidateRanking: rank candidates with explicit templates

Loading

Possibly related PRs

• PhysShell/griff#113: Introduces the pattern core used for kernel parsing, expansion, traversal, and validation.
• PhysShell/griff#114: Introduces the SWANG lowering API used for rhythm template generation.
• PhysShell/griff#102: Changes how rhythm templates are sourced and passed into ranked candidate generation.

Suggested labels: enhancement

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 matches the main change: adding namespaced --rhythm-* transport options to griff generate.
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/s16-phase2-cli-artifact

---

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.}

[PhysShell]

Architecture/code review — blockers before Phase 2 can close

The main path is well assembled: the CLI adapter is separate from the future grammar, unit/tail/kernel errors map to stable SWG codes, geometry comes from PPQN + master-bar meter, the artifact is produced before pitch generation, and fingerprints reuse explicit_rhythm_diagnostics. The end-to-end golden is meaningful.

I found one functional blocker and two contract/test blockers.

P1 — SWG0306 still permits the exact mysterious empty candidate set it was introduced to prevent

compile_pattern rejects only when the entire ActivitySequence has no onsets. It does not check whether the bars that will actually be generated contain an onset.

Concrete reproducer:

--bars 1
--rhythm-kernel '................X'
--rhythm-fractal-depth 0
--rhythm-traversal row-major
--rhythm-unit 1/16
--rhythm-tail rest-pad

This compiles to [silent bar, sounding tail bar]. The expansion is not globally silent, so SWG0306 does not fire. With --bars 1, every rotating strategy sees only the silent first grid, and RepeatVariation also holds that first grid. All candidates are note-free, reranking drops all of them, and the CLI exits through the unrelated Corpus("no candidate survived scoring") path. The artifact has already been written by then.

Add a red integration test for this case and reject it before artifact writing and before ranked_candidates with a typed pattern diagnostic. Either broaden SWG0306 while it is still unmerged to mean “the selected generated span has no onsets”, or append a distinct code for an onset-free output window. The check needs bars plus the explicit palette; global expansion activity is insufficient.

P1 — diagnostics location contract still contradicts the implementation

Spec §1.5 still says every pre-grammar diagnostic location is a NodePath, while PatternDiagnostic deliberately carries a CLI flag or INPUT. That is the boundary agreed in #114, but the normative text was not updated.

Rewrite §1.5 to distinguish locations explicitly, for example:

• structural-core diagnostics: NodePath;
• Phase-2 transport diagnostics: CLI flag / input;
• Phase-3+ language diagnostics: source span.

Do not leave the constitution saying one thing while the public error type says another. Software already has enough organized mythology.

P2 — claimed red coverage does not exist for two newly registered codes

The final rhythm_pattern.rs test module has no test for:

• SWG0305: zero/unrepresentable bar geometry;
• SWG0308: density outside 0..=10000.

Both codes are introduced by this PR and both paths should have red tests asserting code and location. Add at least a direct test for PPQN/meter producing zero/non-whole bar duration and a CLI/module test for density 10001.

Also note that rhythm_density_bps: Option<u16> means values above 65535 fail in clap with a generic integer parse error, not SWG0308. If the registry means every integer outside 0..=10000, parse into a wider/raw transport type and validate at the diagnostic boundary.

P2 — RepeatVariation exception is not reflected in user-facing prose

The PR body records the exception, but stdout currently says:

N templates rotated verbatim over B bars

unconditionally. That is false for RepeatVariation, which holds grids[0]. Change the summary to something source-oriented such as “N explicit templates preserved verbatim; per-bar strategies rotate them”, or otherwise state the exception. The artifact itself is fine; the human summary is overclaiming.

CI is still in progress on f66aac1c7ab5d85933cfa9ab483597f435b942a7, so the PR is not mergeable by review policy yet regardless. Verdict: changes required; do not merge this head.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f66aac1c7a

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

[PhysShell]

Review fixes landed (202ad74 red → head green) — point by point against comment 4974708085:

• Blocker, --bars window: check_bars_window runs after the lowering and before the artifact write. The review's exact reproduction is now the red test the_bars_window_guards_swg0306_too: ................X at 1/16 rest-pad → [silent, sounding]; with --bars 1 it exits SWG0306 naming the window ("the first 1 template(s) the --bars window rotates over are all silent — raise --bars past the silent prefix…"), with --bars 2 it compiles. No orphaned artifact, no post-rerank archaeology.
• §1.5 location kinds: the spec now names all three — structural NodePath (pattern core), offending flag/INPUT (Phase-2 transport), source span (Phase 3 on).
• Missing tests: an_empty_score_is_swg0305 and density_above_the_scale_is_swg0308_even_past_u16 added (18 module tests now).
• Density transport: --rhythm-density-bps is u32 end to end; 70000 meets error[SWG0308] (--rhythm-density-bps) instead of a bare clap range error.
• stdout fiction: the summary line now reads "per-bar strategies rotate them verbatim, RepeatVariation holds the first". Goldens re-blessed.

Gate: clippy -D warnings clean, workspace green except the pre-existing Windows-locale missing_file_golden.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NkqJUU6d1sW1RAfvyHrqVM

[coderabbitai]

Actionable comments posted: 4

🤖 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/main.rs`:
- Around line 676-677: Validate emit_rhythm_expansion in the surrounding
generation flow before the fs::write call, rejecting paths that alias input or
output, including equivalent existing symlink/canonical paths. Perform this
distinct-path check before any artifact or MIDI writes, and preserve normal
generation only when all paths are distinct.

In `@cli/src/rhythm_pattern.rs`:
- Around line 572-593: Update the duration calculation in seed_score_with_meters
so each master bar uses 480 ticks per quarter note: compute 4 × numerator /
denominator quarter-note units, yielding 1920 ticks for 4/4 and 1680 for 7/8.
Preserve the existing start accumulation and TickRange construction.
- Around line 498-539: The parse_unit calculation must avoid overflow when
multiplying ppqn, the beat constant, and numerator. Replace the unchecked
multiplication used to compute whole with checked arithmetic, and return the
existing malformed SWG0301 diagnostic if any multiplication overflows before the
representability check.

In `@cli/tests/cli.rs`:
- Around line 317-319: Clean up global temporary artifacts before each relevant
CLI test run: at cli/tests/cli.rs lines 317-319, remove dst with
fs::remove_file(&dst).ok() before invoking griff; at cli/tests/cli.rs lines
275-281, remove art inside the run closure before calling griff. Preserve the
existing assertions and artifact-reading behavior.

🪄 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: 2fe2b8e1-c6dc-4fa2-b3c6-9bdc6dcef42b

📥 Commits

Reviewing files that changed from the base of the PR and between 7751728 and 4deea51.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (8)

• cli/Cargo.toml
• cli/src/lib.rs
• cli/src/main.rs
• cli/src/rhythm_pattern.rs
• cli/tests/cli.rs
• cli/tests/snapshots/generate__rhythm_pattern.txt
• cli/tests/snapshots/generate__rhythm_pattern_ragged.txt
• docs/swang/spec.md

[PhysShell]

Re-review of 4deea51 — five requested fixes verified, one remaining blocker

The prior review items are closed:

• SWG0306 now checks the actual --bars window after lowering and before artifact rendering/writing;
• the silent-prefix reproduction is pinned, with --bars 1 failing and --bars 2 succeeding;
• §1.5 now defines structural NodePath, Phase-2 flag/INPUT, and Phase-3 source-span locations;
• SWG0305 and SWG0308 have direct tests;
• density transport is u32, so 70000 reaches error[SWG0308] (--rhythm-density-bps);
• stdout now states the Repeat

[PhysShell]

Re-review fixes landed — 52ef612 (red) → 97b7b12 (green) for the P2 blocker, fa9edc1 for the remaining threads:

• parse_unit overflow (comment 4975910403): red first — 9607679205057059/13 at PPQN 480 panicked in debug exactly as predicted; green gives the product u128 headroom (u16 × 4 × u64 ≤ 2^82), so a too-large unit falls out of the u32 fit check as error[SWG0301] naming the unit and the PPQN. The lint allow's reason now states the real invariant instead of claiming the user's numerator was validated.
• Critical, artifact aliasing: --emit-rhythm-expansion now rejects paths aliasing INPUT or OUTPUT before anything is written (canonical paths when they resolve, lexical otherwise). The new integration test proves the input tab survives byte-identical and no riff is written.
• Fixture bar-length formula: fixed — 1920-tick 4/4 bars, as the label claimed. Harmless in effect (geometry reads the meter, not the tick range), dishonest in principle.
• Temp-file hygiene: both golden tests clean their files before running; the artifact is removed before every run inside the loop so a stale write can never mask a failure.

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

🤖 Generated with Claude Code

https://claude.ai/code/session_01NkqJUU6d1sW1RAfvyHrqVM