feat(cli): complement subcommand — tab-seeded complementary part (S13) - #60
Conversation
A golden test that runs `griff complement <fixture> <out.mid>` and snapshots its summary line. Fails until the subcommand exists: clap rejects the unknown subcommand and no snapshots are blessed yet.
|
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 (9)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughA new ChangesComplement CLI subcommand
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
![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: 163a11c66b
ℹ️ 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".
| } | ||
|
|
||
| #[test] | ||
| fn complement_golden() { |
There was a problem hiding this comment.
AGENTS.md's mandatory TDD workflow for /workspace/griff requires non-trivial changes to commit failing tests before implementation and explicitly says reviewers must judge the commit sequence, not just the flattened diff. This commit introduces complement_golden and the CLI implementation/snapshots together, with no preceding red test commit for the complement subcommand on this branch, so the sequence is non-compliant; split the golden test commit from the implementation commit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is already satisfied on the branch — the complement subcommand was committed red→green:
512c26dtest(cli): pin a complement subcommand via golden test (red) — touches onlycli/tests/cli.rs(+26/−4). The newcomplement_goldentest fails at this commit: clap rejects the unknowncomplementsubcommand and the snapshot files don't exist yet.a0930e1feat(cli): complement subcommand … (green) — addscli/src/main.rsplus the snapshots that make the pinned test pass.
The flattened base→head diff collapses the two into one change, but the commit sequence has the failing-test-first split AGENTS.md asks reviewers to judge.
Generated by Claude Code
| #[arg(long, default_value_t = 0)] | ||
| seed: u64, | ||
| /// Semitone shift of B's register relative to A (e.g. -12 = octave down). | ||
| #[arg(long, default_value_t = 0)] |
There was a problem hiding this comment.
Because --offset defaults to 0 for every relation, two advertised modes are unusable with the invocation style the help suggests: octave_double rejects zero because it must be a non-zero octave shift, and register_contrast rejects zero because B's shifted band still overlaps A. A user running griff complement --mode octave_double in.mid out.mid gets InvalidSpec instead of an octave; either apply mode-specific defaults or require/validate --offset for those modes before calling the arranger.
Useful? React with 👍 / 👎.
| register_offset: offset, | ||
| }; | ||
| let candidate = | ||
| complement::arrange_complement(&score, 0, spec, generate::GenerationSeed(seed))?; |
There was a problem hiding this comment.
This hard-codes part A to score track 0, but Guitar Pro import preserves rest-only tracks, so a tab whose first GP track is empty while later tracks contain notes will fail with PartHasNoNotes even though it has material to complement. Choose the first primary-voice note-bearing track, matching the existing curation convention, or expose a --track option so multi-track/silent-leading tabs can be arranged.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cli/tests/cli.rs (1)
150-170: ⚡ Quick winAdd golden coverage for non-default complement args and invalid mode.
Current golden only validates defaults;
--mode,--seed,--offset, and the invalid-mode failure path remain unpinned.🧪 Suggested additions
+#[test] +fn complement_modes_and_options_golden() { + let src = fixture_path("simple_4_4"); + let dst = env::temp_dir().join("griff_s0_complement_opts.mid"); + fs::remove_file(&dst).ok(); + let out = griff( + &[ + "complement", + src.to_str().unwrap(), + dst.to_str().unwrap(), + "--mode", + "counter_melody", + "--seed", + "42", + "--offset", + "-12", + ], + dst.to_str(), + ); + let out = out.replace(src.to_str().unwrap(), "<SRC>"); + assert_golden("complement__opts_counter_melody_seed42_offset-12", &out); + fs::remove_file(&dst).ok(); +} + +#[test] +fn complement_invalid_mode_golden() { + let src = fixture_path("simple_4_4"); + let dst = env::temp_dir().join("griff_s0_complement_invalid.mid"); + let out = griff( + &["complement", src.to_str().unwrap(), dst.to_str().unwrap(), "--mode", "bad_mode"], + dst.to_str(), + ); + let out = out.replace(src.to_str().unwrap(), "<SRC>"); + assert_golden("error__complement_invalid_mode", &out); +}🤖 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 `@cli/tests/cli.rs` around lines 150 - 170, The complement_golden test function currently only validates the default behavior of the complement command without testing various argument combinations or error cases. Extend the test coverage by adding additional test cases that invoke the griff function with non-default arguments for --mode, --seed, and --offset parameters, and also add a test case that passes an invalid mode value to verify the error handling path is correct. Each test case variant should have corresponding golden file assertions via assert_golden calls to pin the expected output behavior.
🤖 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 690-693: The error returned for an unknown complement mode is
using CliError::Ensemble, which incorrectly labels the error as an ensemble
error instead of a complement-specific error. Replace CliError::Ensemble with a
complement-specific error type (such as CliError::Complement if it exists, or
the appropriate variant that corresponds to complement mode validation) in the
error return statement at the location where the "unknown complement mode"
message is constructed, so users see the correct error category prefix.
---
Nitpick comments:
In `@cli/tests/cli.rs`:
- Around line 150-170: The complement_golden test function currently only
validates the default behavior of the complement command without testing various
argument combinations or error cases. Extend the test coverage by adding
additional test cases that invoke the griff function with non-default arguments
for --mode, --seed, and --offset parameters, and also add a test case that
passes an invalid mode value to verify the error handling path is correct. Each
test case variant should have corresponding golden file assertions via
assert_golden calls to pin the expected output 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: 469d15a9-6796-4320-a737-f9a80e19de07
📒 Files selected for processing (7)
cli/src/main.rscli/tests/cli.rscli/tests/snapshots/complement__multi_track.txtcli/tests/snapshots/complement__seven_eight.txtcli/tests/snapshots/complement__simple_4_4.txtcli/tests/snapshots/complement__tempo_change.txtcli/tests/snapshots/complement__two_phrases.txt
163a11c to
a0930e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/tests/cli.rs`:
- Around line 201-220: The test function complement_invalid_mode_golden() has a
comment on line 201 stating the test should produce no output file, but the test
never validates this behavior. After the assert_golden() call, add an explicit
assertion to verify that the destination file (dst) was not created by checking
that !dst.exists() is true, thereby enforcing the documented contract about the
invalid-mode failure path.
🪄 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: 2bbfd39f-60f0-438f-a81c-bc4a5e580ffe
📒 Files selected for processing (9)
cli/src/main.rscli/tests/cli.rscli/tests/snapshots/complement__multi_track.txtcli/tests/snapshots/complement__opts_counter_melody.txtcli/tests/snapshots/complement__seven_eight.txtcli/tests/snapshots/complement__simple_4_4.txtcli/tests/snapshots/complement__tempo_change.txtcli/tests/snapshots/complement__two_phrases.txtcli/tests/snapshots/error__complement_invalid_mode.txt
✅ Files skipped from review due to trivial changes (4)
- cli/tests/snapshots/complement__multi_track.txt
- cli/tests/snapshots/complement__simple_4_4.txt
- cli/tests/snapshots/complement__two_phrases.txt
- cli/tests/snapshots/complement__seven_eight.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- cli/tests/snapshots/complement__tempo_change.txt
- cli/src/main.rs
Surface the S13 ComplementArranger (complement::arrange_complement, ADR-0015) on the .gpx/.mid front door. `griff complement <input> <out.mid> [--mode M] [--seed N] [--offset N]` derives a complementary part B from the source's primary track — one of six relations (rhythm_lock, register_contrast, call_response, support_layer, octave_double, counter_melody) — and writes A plus B to MIDI. Deterministic under a fixed seed. Greens the complement_golden test by blessing its summary snapshots. On a real GP6 tab: a rhythm-locked second part appended as a new track alongside the two guitars.
a0930e1 to
f041fe7
Compare
2 participants
What
griff complement <input> <out.mid> [--mode M] [--seed N] [--offset N]— the S13 ComplementArranger surfaced on the.gpx/.midfront door. The engine writes a second part — a complementary guitar/bass derived from your tab's primary track — and saves the tab plus that new part as one MIDI.Six relation modes (
complement::RelationMode):rhythm_lock(default) — B locks to A's onset grid, pitches from A's harmonyregister_contrast— B in a register band disjoint from Acall_response— B answers A in its gapssupport_layer— a sparser low layer under Aoctave_double— A's contour an octave awaycounter_melody— an independent line (delegates to the S6 generator)--offsetshifts B's register relative to A (e.g.-12= an octave down);--seedmakes it deterministic.How
Thin CLI wiring over the already-tested core
complement::arrange_complement; the returnedComplementCandidate.scoreis A with part B appended as a new track, exported viamidi::export_score(same path asexport/generate). No core changes. Adds aCliError::Complementvariant.Verified
On the real GP6 tab (Dance Gavin Dance):
→ the MIDI re-imports as 3 tracks (the two guitars + the generated part B on channel 1, 696 notes). All six fixtures complement cleanly under
rhythm_lock.Golden-snapshot tested (
complement_golden, mirroringexport/generate's path-scrubbing). TDD red → green. 21 cli tests green;clippy --all-targets -D warningsclean;fmtclean.Scope
CLI surface only — the arrangement logic and its tests already live in
core::complement. This completes the trio the user asked for: S4 (phrases) ✓, S14 (structure) ✓, S6 (generate) ✓, S13 (complement) ✓.https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
Generated by Claude Code
Summary by CodeRabbit
griff complementCLI subcommand that generates “part B” and appends it to the output MIDI alongside the original “part A”.--modeto control the complement relationship, deterministic--seed, and optional semitone--offsetfor register shifting.--modevalues now fail with a clear CLI argument error.