Skip to content

feat: modernize API, fix critical bugs, and add test suite#1

Merged
leandrodaf merged 4 commits into
mainfrom
modernize/api-refactor-and-tests
May 14, 2026
Merged

feat: modernize API, fix critical bugs, and add test suite#1
leandrodaf merged 4 commits into
mainfrom
modernize/api-refactor-and-tests

Conversation

@leandrodaf
Copy link
Copy Markdown
Owner

Summary

Complete modernization of the MIDI library: critical bug fixes, API redesign, and a new test suite (37 tests).


Bug Fixes

Darwin deadlock (critical)

StartCapture held mu.Lock() and called Stop(), which tried to re-acquire the same lock → deadlock. Fixed with stopLocked() pattern: Stop() acquires lock → calls stopLocked() → releases lock → calls wg.Wait() outside the lock.

sync.Once preventing restart

stopOnce sync.Once meant after the first Stop(), the client could never stop again after a restart. Replaced with a simple capturing bool guard.

Goroutine leak in CoreMIDI NewDestination

The processIncomingPacket goroutine was started before MIDIDestinationCreate. On failure, writeFd was never closed → goroutine blocked forever. Fixed: close writeFd on creation failure.

Windows syscall error handling

windows.NewLazySystemDLL proc.Call() always returns a non-nil err (it is Win32 GetLastError, not a Go error). Fixed: only use r1 != 0 as the error indicator.

fmt.Errorf throughout coremidi

All errors.New(fmt.Sprintf(...)) replaced with fmt.Errorf(...).


API Changes (breaking)

Field → plain struct

Field was a confusing interface that doubled as both a type and a builder. Now it's a plain struct { Key string; Value any } with standalone constructors:

contracts.IntField("deviceID", id)
contracts.ErrField("error", err)
contracts.StringField("name", name)
// etc.

StartCapture signature

// Before
StartCapture(eventChannel chan MIDI)

// After
StartCapture(ctx context.Context) (<-chan MIDI, error)

The client now creates and owns the channel. Cancelling the context triggers Stop() and closes the channel automatically.

New options

  • WithChannelBufferSize(n int) — configure event channel buffer (default: 100)
  • WithLogDestination(dest, filePath...) — route logs to console or file

LogLevel fix

InfoLevel = iota = 0 caused the default check to be ambiguous. Added logLevelSet bool + LogLevelIsSet(). LogLevel ordering corrected: Debug < Info < Warn < Error < Fatal.

contracts.IsCommandAllowed

Extracted from identical duplications in both darwin and windows clients. Handles nil filter internally.

dummyMIDIClient unexported

The cross-platform stub was exported unnecessarily.


Logger Rewrite

Replaced the uber-zap dependency with a lightweight stdLogger that writes structured lines with JSON fields. Same contracts.Logger interface — drop-in replacement. Added NewLoggerWithWriter(w io.Writer) for testability.


Test Suite (37 tests, all passing)

File Tests
sdk/contracts/filter_test.go IsCommandAllowed — nil filter, empty list, present, absent, multiple (5)
sdk/contracts/mock_test.go MockMIDIClient — configured funcs, zero defaults, call counters (3)
sdk/midi/options_setup_test.go applyDefaultOptions — defaults, overrides, LogLevelIsSet (8)
internal/logger/logger_wrapper_test.go Level filtering, field output, SetLevel, SetDestination (9)

Also added sdk/contracts/mock.go — a MockMIDIClient for library consumers to use in their own tests.


Files Changed

  • 31 files changed, 1821 insertions(+), 556 deletions(-)

## Bug fixes
- Fix Darwin deadlock: extract stopLocked() (no mutex), Stop() waits outside lock
- Fix sync.Once preventing restart: replaced with capturing bool guard
- Fix goroutine leak in coremidi NewDestination: close writeFd on creation failure
- Fix Windows syscall error handling: use r1 != 0 only, ignore spurious GetLastError
- Replace errors.New(fmt.Sprintf(...)) with fmt.Errorf throughout coremidi

## API modernization
- Field: interface → plain struct {Key string; Value any} with standalone constructors
  (contracts.IntField, contracts.StringField, contracts.ErrField, etc.)
- StartCapture: (chan MIDI) → (ctx context.Context) (<-chan MIDI, error)
  Client now creates and owns the channel; cancelled context triggers Stop()+close
- Add WithChannelBufferSize(n) option (default 100)
- Add WithLogDestination(dest, filePath...) option for console/file log routing
- Fix LogLevel default bug: add logLevelSet bool + LogLevelIsSet() method
- Fix LogLevel ordering: Debug < Info < Warn < Error < Fatal
- Extract IsCommandAllowed to contracts.IsCommandAllowed (removes duplication)
- Unexport DummyMIDIClient → dummyMIDIClient in darwin and windows stubs
- Add sentinel error variables in mididarwin and midiwindows

## Code quality
- Replace uber-zap with lightweight stdLogger using encoding/json fields
- Add NewLoggerWithWriter(w io.Writer) for testability
- NewLogger() / NewZapLogger() / NewStandardLogger() all available

## Tests (37 new test cases, all passing)
- sdk/contracts/filter_test.go — IsCommandAllowed (5 cases)
- sdk/contracts/mock.go + mock_test.go — MockMIDIClient for consumers (3 cases)
- sdk/midi/options_setup_test.go — applyDefaultOptions (8 cases)
- internal/logger/logger_wrapper_test.go — level filtering, fields, output (9 cases)

## Docs
- Add .github/copilot-instructions.md
- Add CLAUDE.md with build/test commands and architecture overview
- Update README with new API usage

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@leandrodaf leandrodaf self-assigned this May 14, 2026
leandrodaf and others added 3 commits May 14, 2026 09:10
- vet job runs go vet on ubuntu first
- test matrix: macos-latest (CGo=1, CoreMIDI), windows-latest (CGo=0, syscalls), ubuntu-latest (CGo=0, dummy stubs)
- uses go-version-file: go.mod to track version automatically
- fail-fast: false so all three platforms always run independently

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Windows: CGO_ENABLED=1 (gcc pre-installed on runners) so -race works
- Linux: CGO_ENABLED=0 (no C toolchain needed for dummy stubs), race='' to skip -race
- Add FORCE_JAVASCRIPT_ACTIONS_TO_NODE24=true env to silence deprecation warnings for actions/checkout and actions/setup-go

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Eliminates Node.js 20 deprecation warnings. Both v6 releases natively
target Node.js 24.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@leandrodaf leandrodaf merged commit 4585475 into main May 14, 2026
8 checks passed
@leandrodaf leandrodaf deleted the modernize/api-refactor-and-tests branch May 14, 2026 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant