Chord.Parse / ParseChord accept a phrase string like "Ctrl+K, Ctrl+C" and return one chord with a made-up key "K, CTRL" instead of throwing

[matt-edmondson]

What's wrong

KeyStringTokenizer.SplitChord (Keybinding/Models/KeyStringTokenizer.cs:24) splits only on +. A , that follows a key is appended to the current token as an ordinary character.

The resulting token is passed to Note, which accepts any non-blank string (Keybinding/Models/MusicalTypes.cs:31-46). The documented NoteName pattern ^[A-Z0-9_]+$ is never checked.

Two public entry points take this path:

• Chord.Parse (MusicalTypes.cs:268-292)
• KeybindingService.ParseChord (Services/KeybindingService.cs:208-226)

Failure scenario

Chord.Parse("Ctrl+K, Ctrl+C") and service.ParseChord("Ctrl+K, Ctrl+C") both return a 3-note chord without throwing. The notes are C, CTRL and "K, CTRL".

• ToString() on that chord gives "Ctrl+C+K, CTRL".
• Phrase.Parse reads that same string back as two chords, so a bound chord's display string means something different when parsed again.

I reproduced this with a scratch MSTest against the current main.

The README says "Invalid key combinations are rejected during parsing." In practice, a user who types a VS-style sequence into a chord field gets a binding that can never fire, and sees no error. Keys with inner whitespace, such as "Page Up", get through the same way.

Suggested fix / acceptance criteria

• SplitChord throws ArgumentException when a , follows a key.
  • A , on its own in the key position must stay valid, because Ctrl+, depends on it ("Ctrl++" and "Ctrl+," silently parse as plain "Ctrl", so the + and , keys cannot be bound #108).
• Note rejects keys that contain whitespace or ,, except for the single-character keys "," and "+".
  • Don't enforce ^[A-Z0-9_]+$ literally: it would break + and , (see the note in A Command built from a CommandId with surrounding whitespace registers but can never be found, bound or unregistered #123).
• Add tests:
  • Chord.Parse("Ctrl+K, Ctrl+C") throws.
  • Chord.Parse("Ctrl+,") still parses.
  • Chord.Parse(chord.ToString()) round-trips for valid chords.

[matt-edmondson]

Triage

• Category: Bug
• Priority: Medium. Invalid input is accepted silently and produces a binding that can never fire, which contradicts the README's "rejected during parsing" promise. There's no crash or data loss.
• Area / suggested assignee: parsing and validation (KeyStringTokenizer.SplitChord, Note constructor). Owner: @matt-edmondson
• Duplicates / in progress: no duplicate or open PR. Related to Multi-chord phrases (e.g. "Ctrl+K, Ctrl+C") can be parsed but never bound, looked up or executed #135 (phrases can't be bound), and fixing this first makes that gap visible. Keep the Ctrl+, behaviour from "Ctrl++" and "Ctrl+," silently parse as plain "Ctrl", so the + and , keys cannot be bound #108 and follow the note in A Command built from a CommandId with surrounding whitespace registers but can never be found, bound or unregistered #123 about not enforcing ^[A-Z0-9_]+$ literally.
• Next step: throw when a , follows a key, reject keys that contain whitespace or a comma (except the single-character , and + keys), and add the round-trip tests.

---

Generated by Claude Code