Claude/instrument recognition v2 xy1mkj - #1890
Merged
Merged
Conversation
Cross-verified audit of the capability-recognition subsystem across three axes (ingestion, storage/validation, consumption/enforcement). Key findings: file-playback enforcement is solid (range fold, discrete snap, polyphony gate, timing, sync_delay, string CC), but recognition is never auto-triggered on connect, several declared capabilities are not enforced (supported_ccs, octave_mode/scale_root, live route-through, strings, secondary voices), the capability validator is never called on write paths, and two pre-existing identity bugs (saveSysExIdentity field mapping; DeviceSettingsModal display nesting). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BZ7sHZ9pTnJMt6h9Cus5mn
Address the P1 findings from the capability-recognition audit: - saveSysExIdentity read v1-only field names (deviceFamily, deviceFamilyMember, softwareRevision) that no parser emits, so sysex_family/model/version were always persisted null. Add a pure exported normalizeSysexIdentity() mapping all three parser shapes (Universal reply, GMB v1, GMB v2 handshake) and use it in both branches. Covered by tests/sysex-identity-mapping.test.js. - DeviceSettingsModal read data.name/.firmware/.protocol at the top level, but the device_identity broadcast nests them under data.identity — the panel always showed "Inconnu / - / -". Read the nested identity. - updateInstrumentCapabilities INSERT branch omitted hands_config, bagpipe_config, accordion_config, harmonica_config (the UPDATE branch persists them), silently dropping configs on the first write for a device+channel. Add them to the INSERT. - Harden capability writes: validate note_selection_mode and capabilities_source enums up-front (clear error instead of a deep CHECK failure), and normalise supported_ccs/selected_notes through the shared strict MidiListParser so non-integer or malformed input can no longer reach the json_valid CHECK (matches the per-voice path). Full backend suite green (1114 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BZ7sHZ9pTnJMt6h9Cus5mn
Recognition previously never fired on connect for any transport — the only trigger was a frontend probe when the Loop Editor opened, and the device_connected event was subscribed but never emitted (audit P2-1). DeviceManager now, on each opened port (addInput/addOutput): - emits device_connected once per device (coalescing input+output ports), which the UI broadcast and the clock generator's cache already listen for; - arms a debounced identity probe (sendIdentityRequest) with a reply timeout and up to 3 attempts. Output-less devices (DIN-IN-only) are skipped without throwing, and the probe leaves itself armed for when an output for the same name appears. A parsed identity reply marks the device recognised and cancels pending retries; timers are cleared on close() and unref'd so they never hold the process open. Covered by tests/devicemanager-auto-identity.test.js (7 tests, fake timers). Full backend suite green (1121 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BZ7sHZ9pTnJMt6h9Cus5mn
octave_mode / scale_root had no backend consumer (audit P2-5): the playback pipeline only snapped to explicit selected_notes, so a range-mode instrument restricted to a diatonic/pentatonic scale played every chromatic note unless the settings modal happened to materialise the scale into selected_notes. Any non-UI writer (auto-assign editor, API, future descriptor) bypassed scale enforcement entirely. - New pure ScaleSnapper (src/midi/adaptation/ScaleSnapper.js) materialises the in-scale notes for a mode+root over a range, mirroring the frontend InstrumentSettingsModal.computePlayableNotes. - Expose octave_mode/scale_root through getInstrumentCapabilities → CapabilityResolver.getTimingConstraints → PlaybackSnapshot. - PlaybackScheduler._dispatchToDevice snaps range-mode notes to the scale after range folding, when no explicit selected_notes are set (those keep priority, so no double-snap). Chromatic / no-range instruments are untouched. Covered by tests/scale-snapper.test.js and tests/playback-scheduler-scale-snap.test.js. Full backend suite green (1135 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BZ7sHZ9pTnJMt6h9Cus5mn
The live route-through (MidiRouter) applied only user route filters and sync_delay — a source keyboard could send a mechanical instrument chords and pitches it physically cannot produce (audit P2-3). - Extract the stateless note clamp (range fold + discrete/scale snap) into a shared pure module NoteEnforcement.clampNote, and refactor PlaybackScheduler to delegate to it (_foldIntoRange / _snapToSelected / the dispatch clamp) so both paths clamp identically — guarded by the existing scheduler range-fold / discrete-notes / scale-snap suites. - MidiRouter._sendAndEmit now clamps routed notes to the destination instrument's capabilities via CapabilityResolver. Non-note messages, the GM drum channel, and the no-resolver case are pass-throughs. Stateful enforcement (polyphony / min-note timing) is intentionally NOT applied on the live path yet — it needs per-stream note state the router does not track (documented in NoteEnforcement + the audit). Covered by tests/note-enforcement.test.js and tests/midi-router-capability-clamp.test.js. Full backend suite green (1152 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BZ7sHZ9pTnJMt6h9Cus5mn
Add a follow-up table marking P1-1/P1-2/P1-4/P1-5, P2-1, P2-3 and P2-5 as resolved, and listing what remains (descriptor pipeline, supported_ccs filter, declarative instrument schemas, per-voice engine consumption, string audible clamp, comm_timeout wiring, P3 items). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BZ7sHZ9pTnJMt6h9Cus5mn
Implement the transport/DB-independent pieces the migration plan calls for (docs/SYSEX_IDENTITY.md §11.2), ready to wire into DeviceManager and the persistence layer (audit P2-2): - assembleChunks — reassemble a block 0x10 segmented transfer (§3) from out-of-order/duplicated chunks into the descriptor JSON string; rejects a stream whose chunks disagree on the total. - validateDescriptor — structural validation of a parsed descriptor (§5): version, revision, device, and 1-16 unique-channel instruments with optional notes (range/discrete). Unknown fields are ignored, honouring the protocol's extensibility rule. - diffOverrides — the §6 override arbitration: drop a user override only when the device itself moved that field's value, with the special cases (first descriptor, configured→false, declared→absent all keep). tuning is compared as a whole value. Pure and side-effect-free; covered by tests/descriptor-protocol.test.js (22 tests). Full backend suite green (1174 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BZ7sHZ9pTnJMt6h9Cus5mn
Record DescriptorProtocol (chunk reassembly / validation / override diff) as delivered, with the transport + DB wiring listed as remaining. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BZ7sHZ9pTnJMt6h9Cus5mn
A stored polyphony of 0 (or negative / non-integer) coerced through `|| null` to null, silently disabling the polyphony gate instead of being treated as "no limit" deterministically (audit P3). Read it as a positive integer or null in both read sites (CapabilityResolver.getTimingConstraints and InstrumentCapabilitiesDB.getInstrumentCapabilities). Add the missing unit test for getTimingConstraints — the per-note hot-path lookup the scheduler depends on — covering the null defaults, the polyphony robustness, the octave_mode/scale_root exposure added for scale enforcement, the discrete-only selectedNotes rule, and note-range passthrough. Full backend suite green (1181 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BZ7sHZ9pTnJMt6h9Cus5mn
Wire the descriptor pipeline's application brain on top of the pure core: - descriptorToCapabilities — map a §5 descriptor instrument onto the capability store fields (notes range/discrete, polyphony.max, expression.cc, gm_program). Absent fields are omitted (unknown). - descriptorLookaheadMs — max timing.prepare.max_ms across instruments (§9), the lookahead the scheduler needs to hide silent preparation. - DescriptorService.applyDescriptor — validate → map each declared instrument onto the repository → honour the per-instrument `configured` flag (unconfigured ⇒ manual, nothing written) → compute the §6 override purges → broadcast instruments_configured. Repository failures isolate to the offending instrument. capabilities_source stays 'auto' (valid under the current CHECK) until the 'descriptor' value is enabled by the deferred migration (§12). Transport-agnostic and unit-tested with a mock repository; the block 0x10 / HTTP fetch and the persisted instance_id→config + override state are the remaining integration step. Covered by tests/descriptor-mapping.test.js and tests/descriptor-service.test.js (14 tests). Full backend suite green (1195 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BZ7sHZ9pTnJMt6h9Cus5mn
Connect the descriptor transport to the application brain, completing the v2 pipeline (docs/SYSEX_IDENTITY.md §7): - A level-1 v2 handshake (descriptor_size > 0) now triggers a sequential block 0x10 fetch: parseDescriptorChunk decodes each response frame (14-bit total/index + ASCII payload), the state machine requests chunks one by one with a per-chunk timeout + bounded retries, assembleChunks reassembles them, and the parsed JSON is handed to DescriptorService.applyDescriptor. Incomplete transfers and invalid JSON fall back to level 0. - Register DescriptorService in the DI container; DeviceManager reaches it lazily (registered after DeviceManager). Fetch timers are unref'd and cleared on close(). The SysEx byte I/O itself can't be exercised without hardware, but the frame parser and the request/collect/timeout/retry/apply state machine are covered by tests/devicemanager-descriptor-fetch.test.js (fake timers, 7 tests). Full backend suite green (1202 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BZ7sHZ9pTnJMt6h9Cus5mn
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BZ7sHZ9pTnJMt6h9Cus5mn
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.
No description provided.