Review of #112: syncSelectedInputSource() treated an empty device name as
"nameless device — invalidate rather than guess" and called removeItem()
on the persisted selection. But the input dropdown's first option is
literally `<option value="">Default</option>`, so "" is the ordinary
"use the OS default" choice, not a nameless device.
init()'s auto-apply calls this on every startup, so a user sitting on
Default had their capability selection deleted at each launch — and that
selection is made in a DIFFERENT ui (the input_setup / tuner picker), so
this silently discarded a device they explicitly chose. Because
audioInputOpenHandler deliberately refuses to guess a device, a cleared
selection leaves plugins with no input at all: the same dead-guitar
symptom the PR set out to fix. Confirmed against the pre-fix build — a
sync with the Default value emits removeItem on the stored key.
"" now means "no opinion": leave the selection to the picker that owns it.
Also extract inputSourceNameKey() as the single place a named input's key
is built. Registration and selection were formatting the same template
independently and had already drifted on the nameless branch (registration
falls back to a positional key; sync emitted none). Tests pin the format,
the Default behaviour, and a round-trip through the open handler's own
parser regex.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When the GPU process can't launch or keeps crashing, Chromium's default is to
give up and FATAL-abort the whole browser process — the user sees the app vanish
("GPU process isn't usable. Goodbye."). We hit this repeatedly: a machine with a
flaky GPU stack took the app down mid-song, and it is the likely reason behind
the "gig always defaults to the classic 2D highway" reports — those machines are
one GPU hiccup away from a crash, not just a fallback.
--disable-gpu-process-crash-limit tells Chromium to keep the browser alive and
fall back to software rendering instead of aborting. Software rasterization
stays enabled (we never pass --disable-software-rasterizer), so there is a path
to land on. The 3D highway then degrades to 2D / runs slow under SwiftShader —
far better than the whole app dying. Cross-platform, set before whenReady.
Validated against a build that reliably FATAL-crashed within seconds on GPU
process launch failure: with the switch it stayed alive 40s+ and never hit the
fatal path — Chromium fell back instead of aborting.
Also adds child-process-gone / render-process-gone log handlers: now that the
app SURVIVES a dead GPU, that log is the only remaining signal it happened,
which is exactly what a "highway is 2D / app was crashing" report needs to be
diagnosable. Log-only.
typecheck + lint clean.
The same finding as the clearChain/remove/moveProcessor fix, in a path I
missed on the first pass. CodeRabbit pointed at this line for the wrong reason
(it claimed the mutation was unguarded — it is guarded); the real defect is
WHERE the guard is taken.
LoadVST's macOS branch took a blocking lock_guard on chainMutationMutex on the
Node/main thread. On macOS that thread is also JUCE's message thread, and it
has no pump — its queue is drained by a libuv timer that only runs when the
thread is idle.
Meanwhile a LoadPresetWorker holding that mutex on a libuv thread calls JUCE's
*synchronous* createPluginInstance, which — when called off the message thread,
which is exactly where a worker calls it — posts an AsyncCreateMessage to the
message thread and blocks until it runs (juce_AudioPluginFormat.cpp,
createInstanceFromDescription). So: main thread blocks on the mutex → the
message queue stops draining → the worker's load never completes → the mutex is
never released → the app hangs permanently. Adding a VST while a preset load is
in flight was enough to trigger it.
The in-code comment asserted this was deadlock-safe because "loadVstSandboxAware's
JUCE_MAC branch is a synchronous load on the worker itself" — but JUCE's sync
load is not synchronous off the message thread, which is what makes the cycle.
The plugin INSTANTIATION has to stay on the main thread (JUCE requires it for
VST/AU on macOS), so only the mutation moves: it now goes through
queueChainSlotMutation(), a slot-id-returning sibling of queueChainMutation().
While the worker waits for the mutex the main thread stays free to drain the
queue, so the in-flight load completes and releases it.
The branch is portable C++, so it was compile-checked on Linux by forcing the
#if; the macOS addon lane is the real gate.
CodeRabbit caught a real bug in the previous commit's fix. The latch mirrors
the NATIVE refcount, but it was flipped before the invoke resolved: a rejected
release left it reading "released" while the engine still held the
suppression, so every later release short-circuited and monitor mute stayed
suppressed for good — the same stuck-suppression bug the latch exists to
prevent, just one level up.
The latch now only stays flipped if the call actually landed, and rolls back
otherwise (guarded so a newer call can't be clobbered by a stale rejection). A
downlevel addon with no arbiter leaves the latch untouched instead of
recording a hold it never acquired.
Pins the whole contract with a vm-extracted unit test on the real screen.js
function: unpaired acquires hold at most one native suppression, cycles stay
balanced across 25 song loads, a rejected release retries, and both the
downlevel and sync-throw paths are clean. Fails 3/5 against the original
branch (the refcount leak) and 2/5 against the pre-rollback version.
Seven fixes on top of the audio-engine TLC branch, each with the gate that
catches its regression.
Blocking:
- Monitor-mute suppression leaked its refcount. setMonitorMuteSuppressed()
became a refcounted acquire/release, but screen.js's callers are
deliberately unpaired: resolveChainRebuildGuard() leaves the suppression on
when a rebuild yields an empty chain, and returns early without releasing
while a provider route is still resolving. Harmless against the old latched
bool, a permanent +1 each against a refcount — after a failed tone rebuild
the count never returned to zero and monitor mute was silently dead for the
rest of the session. The renderer now holds at most one suppression.
- Slot ids are monotonic HANDLES (nextSlotId, never reset by clear()), not
bounded indices, so argSlotId's 4096 ceiling meant that once a session
created its 4096th processor EVERY guarded binding — setBypass,
setParameter, remove/moveProcessor, open/closePluginEditor — silently
no-opped for the rest of the run. Ceiling removed (same for
SetMultiBypass's hardcoded 4096); unknown ids are still rejected by
SignalChain::findSlotIndex.
- clearChain / removeProcessor / moveProcessor took chainMutationMutex with a
blocking lock_guard on the N-API thread — Electron's main thread, and on
macOS also the JUCE message thread. LoadPreset/LoadVST hold that mutex
across an unbounded plugin init (done->wait() has no timeout by design), so
a slow plugin froze the whole main process, every IPC channel with it. They
now queue on a libuv worker via queueChainMutation() and resolve a promise;
the bridge awaits them so callers still observe the mutation applied.
Also:
- getChainState() dereferenced raw ProcessorSlot* returned by getAllSlots()
after the lock was dropped — a concurrent clear() frees them under the
reader. Replaced with SignalChain::getSlotSummaries(), which copies under
the lock. getAllSlots() is gone (it had one caller).
- The device-settings migration removed the localStorage copy even when the
file-store save failed or was unavailable, losing the user's settings.
- SetSlotState and GetParameters kept the raw Int32Value() path: IsNumber()
is true for NaN, so setSlotState(NaN) wrote onto slot 0 — the same
coercion class the rest of the branch fixed.
- RendererBus flushed to the LIVE writeIndex, so a disable→re-enable with no
pull in between discarded the freshly pushed audio along with the stale
tail. It now snapshots the flush target at disable time.
- LoadPreset's rebuild barrier is now released by a scope guard, so a throw
between arming it and Queue() can't block editor opens forever.
Gates: new renderer-bus case (fails on the old flush), new slot-id-handle
case (fails on the old ceiling). ctest 9/9, npm test 79 pass / 0 fail,
chain-mutation storm green, addon export contract unchanged.
Adds `nightly` as a selectable auto-update channel (Windows + macOS).
Client:
- update-manager.ts: add 'nightly' to UpdateChannel (veloChannel already
yields win-x64-nightly / osx-arm64-nightly, so no logic change)
- main.ts: allow 'nightly' in the runtime IPC channel guard
- preload.ts: add 'nightly' to the preload-local UpdateChannel union
- screen.js / settings.html: add the Nightly option + helper text
CI (nightly.yml):
- derive <pkg>-nightly.<UTC date> version in the setup job
- setup-dotnet (pinned from .build-config.json) so the vpk CLI has net8,
matching build.yml
- vpk pack win-x64-nightly / osx-arm64-nightly (mac signed + notarized,
mirroring build.yml's signed/unsigned fallback)
- publish a rolling `nightly` GitHub Release (prerelease=false, latest=false)
that the in-app updater reads for the nightly channel
- concurrency guard so an overlapping dispatch can't race the rolling release
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Python server only seeds bundled starter content (and scans) when
DLC_DIR.is_dir() is true, and it can't bootstrap the folder itself — the
seed's mkdir runs only after _get_dlc_dir() already resolves a directory. On a
fresh install the default library path didn't exist, so the scan bailed with
"DLC folder not configured" and starter content never seeded.
Create the resolved DLC dir in startPython() before spawning the server so the
first scan seeds the bundled songs. Also modernize the default library path to
~/.local/share/feedback/library, keeping the legacy slopsmith paths as
fallbacks so existing installs that relied on the default keep their populated
library.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copy core's content/starter/*.feedpak into the bundle so
server._seed_builtin_starter_content() can seed it into the library on first
run of packaged builds. Also fix a rename skew: the builtin diagnostic copy
looked for docs/diagnostics/slopsmith-diagnostic-basic-guitar.sloppak, but core
renamed it to feedBack-diagnostic-basic-guitar.sloppak — so the diagnostic
seeding was silently skipped in packaged builds ("not found" warning). Point at
the feedBack-* name to match server.py's _BUILTIN_DIAGNOSTIC_SOURCES.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The nupkg/installer artifact names were still 'Slopsmith-*' (packId), mismatching the app + the feedback-*.deb/.AppImage. Rebrand to 'feedback' now — safe before any installed base exists (Velopack matches the installed app's packId on update; changing it after users install would orphan their auto-updates). The client resolves updates by channel manifest (releases.<ch>.json), not packId, so no client change is needed.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Testers on 0.3.0-alpha.1 reported the signal chain duplicating (every
VST/NAM/IR exactly twice) with blown-out gain after leaving the Audio
menu, plus VST edit windows closing and the Edit button going dead.
Root cause: the native JUCE chain lives in the Electron main process and
survives renderer reloads and screen.js re-evaluations (host re-hydration
after a backend restart), but init() unconditionally restored the
localStorage-saved chain by APPENDING — aeRestoreSavedChain never clears.
The #50 review added a clear-before-restore in the amp-sims toggle
handler only; the identical hazard at init() remained. Since the saved
chain mirrors the live chain, every init re-run produced an exact 2x
duplicate (two amp stages in series = the blown-out gain).
Fixes:
- init(): probe getChainState() first and skip ALL auto-load (default
preset and saved-chain restore) when the engine already has a live
chain. Also covers splitscreen pop-out windows re-running init.
- saveChainStateFromChain(): never persist a Rig-Builder-owned chain
(identified by its _rb_unit_impulse / RB Final Leveler plumbing
stages). Rig Builder reloads its default tone off-screen on its own
schedule, so it is routinely the ambient live chain; snapshotting it
made the saved chain resurrect Rig Builder's tone on restore — the
exact processor set in the tester screenshot.
- aeRestoreSavedChain(): drop Rig Builder plumbing stages from legacy
polluted saves and rewrite the cleaned list (self-healing).
- _aeOpenEditor(): a false return means the baked-in slot id went stale
(chain rebuilt while the list was on screen); refresh the chain list
instead of silently doing nothing.
- Install-once guard (hookState) on the arrangement:changed/song:ready
reapply listeners — they stacked one pair per re-evaluation, running N
racing clear+load sequences per song load after a re-eval.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
electron-builder strips the brackets from productName 'fee[dB]ack' → the win launcher is 'feedback.exe', so deriving --mainExe from raw productName ('fee[dB]ack.exe') didn't match. Glob release/win-unpacked/*.exe instead (single root exe), mirroring the mac .app glob. Confirmed via the fail-loud guard on the v0.3.0-alpha.1 build.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
release.yml and build.yml were added in the same commit and are byte-near-duplicates: both build the 3-OS matrix, Velopack-pack, create the GitHub release, and notify core on v* tags. release.yml has been untouched since creation and still carried the Slopsmith rename skew (so it failed on mac and double-built every tag against build.yml). build.yml is the maintained twin (skew fixed) and fully covers the release path. No status check depends on release.yml.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The mac + win Velopack pack steps hardcoded 'Slopsmith' (the pre-rebrand name), so vpk looked for Slopsmith.app / Slopsmith.exe and failed after the app was renamed to productName 'fee[dB]ack'. Now: mac globs release/mac-arm64/*.app and derives --mainExe from its basename; win derives the launcher from package.json build.productName with a fail-loud guard; the non-tag mac tester-zip globs *.app too. packId stays 'Slopsmith' (the installed-client update contract). Linux unaffected (electron-builder names artifacts feedback-*, release globs by extension).
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both derive to dirnames (drum_highway_3d, keys_highway_3d) that core already ships as committed plugins/ dirs. The clone loop runs after core is in place, so these two always fail-to-clone-and-skip (dir exists) — dead entries. Core remains the authoritative source for both; no functional change to the shipped app.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
feedBack-plugin-find-more is a deliberately-withheld repo (kept private); it must not be bundled into or shipped with the desktop app. Drop it from the plugin clone/bundle list.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The feedBack-plugin-update-manager repo is private and not being shipped, so drop it from the plugin clone/bundle list in build-common.sh. The desktop app's own Velopack auto-updater (src/main/update-manager.ts) is unrelated and untouched.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Camera Director plugin (got-feedback/cameradirector_feedback, our fork of
nimuart's) only appeared in locally-built AppImages because the entry adding it
to build-common.sh's plugin clone list was never committed — it existed solely
as a working-tree edit. Fresh-clone builds (CI, Windows, tester AppImages) clone
core plus this fixed plugin list and ignore the resources/ copies, so the plugin
showed for the maintainer but not for testers.
Its only dependency, the highway_3d `window.__h3dCamCtl` freecam bridge, is
already on core main, and there is no per-plugin build step, so adding the repo
to the clone list is enough to bundle it for everyone.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(audio): close in-process plugin editor windows before freeing their processors (#56)
A `PluginEditorWindow` owns an `AudioProcessorEditor` bound to its slot's
processor, but nothing tore those windows down when the chain was freed. On
pause the renderer clears/reloads the chain (`clearChain` / `loadPreset`), which
destroys every slot processor — leaving any open editor pointing at freed
memory. Its next timer/paint callback then jumps through a dangling pointer:
the reported ACCESS_VIOLATION / DEP-execute at an unmapped address, seconds
after pausing (thread stack thick with `RB Final Leveler.vst3` editor-window
frames calling back into slopsmith_audio.node).
Fix: destroy the in-process editor windows BEFORE the processors they reference,
in all three teardown paths:
- ClearChain (JS thread) — close editors, then clear().
- LoadPresetWorker::Execute (libuv worker) — close editors, then clear() before
rebuilding the chain.
- doShutdown — destroy editors first inside the existing message-thread lambda,
before engine.reset().
editorWindows holds JUCE GUI objects, so teardown must happen on the message
thread. `closeAllPluginEditorWindows()` marshals via `dispatchOnMessageThread`
(post-and-wait) so the caller blocks until every editor is gone — guaranteeing
editors die before their processors. Callers already on the message thread
(doShutdown) use the inline `destroyAllPluginEditorWindowsOnMessageThread()` to
avoid a post-and-wait-on-self deadlock. On Linux/Windows the JUCE message thread
is a dedicated std::thread, so ClearChain (Node) and the worker never deadlock;
on macOS dispatch runs inline and in-process editors don't exist (sandboxed).
Native addon builds clean (Release).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* review: fix macOS worker-thread editor teardown; assert precondition; report post/timeout
Codex [P2]: closeAllPluginEditorWindows() delegated to dispatchOnMessageThread(),
which runs inline under JUCE_MAC — so LoadPresetWorker::Execute() (libuv worker)
could destroy JUCE DocumentWindow/AudioProcessorEditor objects off the message
thread on macOS. Now branch on the caller's actual thread: run inline only when
already on the message thread (else deadlock), otherwise post via
MessageManager::callAsync (drained by the JUCE thread on Linux/Windows and the
Node-main libuv timer on macOS) and wait. Correct on all platforms.
Copilot: report a refused post / 15s wait timeout via stderr instead of silently
assuming teardown completed (the previous "guarantee" wording overstated it);
add JUCE_ASSERT_MESSAGE_THREAD to the inline variant as a debug tripwire.
Native addon builds clean (Release).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* review(codex): don't callAsync+wait on an unpumped macOS MessageManager
Codex round 2 [P2]: my round-1 fix routed the off-message-thread teardown
through MessageManager::callAsync + WaitableEvent::wait on ALL platforms. On
macOS there is no separate message-thread pump (startJuceMessageThread's
JUCE_MAC branch only creates the manager; there is no dispatch loop), so a
callAsync+wait from LoadPresetWorker's libuv worker would stall the full 15s
timeout and then proceed with the editor still alive — the very UAF this targets.
Platform-split the off-thread path, matching loadVstSandboxAware()'s existing
JUCE_MAC handling:
- Already on the message thread → inline (doShutdown; ClearChain on macOS).
- Linux/Windows off-thread → post to the dedicated JUCE message thread + wait
(with refused-post / timeout reporting).
- macOS off-thread → clear inline (the pre-existing macOS worker-thread
limitation). editorWindows is empty on macOS in practice (in-process editors
route to the sandbox child), and the editor/processor UAF this targets is
Windows-specific, so no message-thread hop is needed there.
Native addon builds clean (Release).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* review(codex): tear down editors from LoadPreset (main thread), not the worker
Codex flagged a genuine dilemma in the previous approach: closing editor windows
from LoadPresetWorker::Execute (a libuv worker) is unsafe either way on macOS —
callAsync+wait stalls (no message-thread pump; the "libuv timer" the comment
promises was never implemented) AND clearing inline destroys JUCE GUI objects
off the message thread.
Resolve it by not tearing down from the worker at all: LoadPreset() (the N-API
entry, on the Node/main thread) now closes editors before queuing the
AsyncWorker. That is safe on every platform — macOS: main thread IS the message
thread (inline); Linux/Windows: post to the dedicated JUCE message thread and
wait — and still guarantees editors die before Execute() frees the chain's
processors. closeAllPluginEditorWindows() is consequently never called off a
worker thread, so its macOS special-case is gone and it reduces to the uniform
on-message-thread / post-and-wait form.
Native addon builds clean (Release).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
On hybrid-GPU Windows laptops (Intel iGPU + NVIDIA/AMD dGPU), Chromium's
GPU-process adapter selection is non-deterministic across launches. When
it binds the iGPU, the 3D Highway's per-frame WebGL cost blows the draw
budget and the load-adaptive resolution scaler (feedBack#654) silently
drops the canvas to as low as quarter-res — so the highway renders
pixelated even with Quality pinned at HD, varying launch to launch. The
renderer's `powerPreference: 'high-performance'` WebGL hint is only
advisory and doesn't reliably override the OS/Chromium adapter choice.
Append the Chromium `force_high_performance_gpu` switch on win32 (before
app.whenReady, so it's read during Chromium init) so the discrete adapter
is selected consistently and the scaler rarely engages. Single-GPU
machines are unaffected; on dual-GPU desktops it likewise picks discrete.
Fixes#52
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(audio-input): stable name-based input identity + fail-loud open + bound read-back
Replace the positional-index logicalSourceKey with a name-encoded one so a
named device survives reorder/hotplug; resolve by name and fail loud instead of
silently opening the default mic; read back and return the actually-bound device.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* perf(audio): gate the ML note-detection pipeline behind a master enable
The Basic-Pitch ONNX detector is the most expensive thing in the engine
(~30 ms inference every hop) and on the default desktop path nothing reads
it: note detection is scored by the harmonic-comb NoteVerifier, and the
always-on home tuner runs its own YIN over raw frames. Yet the pipeline ran
unconditionally from construction, pinning a core on an idle home screen.
Add a master gate so ML only runs when a consumer actually needs it:
- MlNoteDetector: std::atomic<bool> enabled{false}. pushSamples() early-
returns on the audio thread (lock-free relaxed load, no feed) and
runInferenceIfDue() early-returns on the inference thread (no Run()), so
the whole pipeline is dormant until armed. setEnabled(false) clears the
rolling window + published snapshot (clearAudioState resets hasPublished),
so a re-arm starts cold and serves the YIN fallback until the first fresh
inference. The inference thread stays alive but idle — toggling needs no
thread restart. isEnabled() for symmetry; no-op stubs in the ONNX-off build.
- AudioEngine::setMlNoteDetectionEnabled(bool) fans to every source's detector
(whole pool, so a later-activated source inherits the arm state).
- NodeAddon setNoteDetectionEnabled + audio-bridge ipc + preload, all typeof/
try-guarded so a downlevel addon ignores it (fail-safe to current behaviour).
The renderer (note_detect) arms this true only while it will read ML notes
(native-frame detection / non-verifier fallback) and false otherwise — a
follow-up renderer change. Default OFF means the shipped verifier path and
the home tuner pay nothing for ML.
Verified: native addon builds clean (ONNX path); the standalone mlnd_test
detects the full C-major triad when armed (3/3); ml-note-detection +
multi-source JS suites pass (16/16). mlnotedetector/test.cpp arms the
detector after prepare() to match the new default.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(audio): make the ML gate reset race-free (thread-owned cold start)
The first cut cleared the rolling window/FIFO from setEnabled() on the N-API
thread while the inference thread was still alive — a data race on the buffers.
Move the reset onto the thread that owns them, and fix two follow-on issues
Codex flagged:
- fifo.reset() TOCTOU: resetting the FIFO on the inference thread can still race
an in-flight pushSamples() that passed the resetPending gate just before it was
set (the >=8 ms callback gap is not a guarantee). Fix: the thread-side cold
start DRAINS the FIFO (fifo.finishedRead(getNumReady()) — advances only the
consumer's read index, safe SPSC) instead of fifo.reset(). clearAudioState()
(with the real reset) is kept for the prepare()/stop() paths where the thread
is already joined. resetPending stays set through the drain so pushSamples()
is gated off the FIFO the whole time, then is released.
- stale readiness on re-arm: setEnabled(true) exposed enabled=true immediately
while hasPublished stayed true from the previous arm, so isReady() briefly
served the old snapshot. Fix: drop hasPublished synchronously BEFORE storing
enabled=true (release/acquire ordering: isReady() loads enabled before
hasPublished, so seeing enabled=true guarantees seeing hasPublished=false).
Other gate mechanics: the enabled-gate is at the top of the inference callback
(disabled ⇒ no ingest, no inference), pushSamples() no-ops when !enabled or
resetPending, and isReady() gates on enabled so a suspended detector serves the
YIN fallback rather than a stale snapshot.
mlnotedetector/test.cpp asserts both directions: fed the chord region while
DISABLED, the detector publishes nothing and never becomes ready; armed, it
still detects the full C-major triad (3/3). Addon rebuilds clean; tsc clean;
ml-note-detection + multi-source JS suites pass (16/16).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(audio): own-rig opt-in — gate saved tone-chain restore on use_amp_sims
Second half of #46 (desktop side), paired with the core onboarding PR.
The amp-sim/tone chain auto-restores from localStorage on every launch, so
once a user has loaded a tone they get a processed monitor forever — an
idle high-gain amp is a constant distorted buzz, and the dry-only monitor
mute can't kill it (the full monitor kill from #47 can, but only on demand).
This makes monitoring "own-rig first": at app init, read the core
`use_amp_sims` preference (set during onboarding / the new toggle) and only
auto-restore the saved signal chain when the user opted IN. Default OFF — a
missing key or any read failure is treated as opt-out, so a flaky/late
backend can never resurrect the buzz. With no chain loaded, the existing
default-on dry mute keeps the monitor silent.
- screen.js: aeUseAmpSims() reads /api/settings; init gates loadDefaultPreset
+ saved-chain restore behind it. Extracted the restore loop into
aeRestoreSavedChain() (shared by init and the live opt-in toggle).
- screen.html/js: new "Use in-app amp sims" checkbox in Audio settings,
persisted to /api/settings (shared with onboarding). Reflects the saved
value on load; turning it ON loads the saved chain immediately (no restart).
Stacked on #47 (monitor kill). node --check clean. NOT built/run here — the
renderer change needs a desktop build + a tester check: with a saved tone and
amp sims OFF, launch is silent (no buzz); toggling ON loads the tone live;
the onboarding choice carries through.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(audio): make the amp-sim toggle apply live (Codex review on #50)
Addresses Codex findings on the "Use in-app amp sims" checkbox:
- P2: turning it OFF now clears the live engine chain so monitoring
actually goes silent this session (with no processors, the default-on
dry mute silences the bus) — previously OFF persisted the pref but left
the amp running, so the checkbox lied and the buzz persisted until
restart. The saved chain in localStorage is left intact (we don't call
saveChainState) so re-enabling restores the same tone.
- P2: ON no longer stacks a duplicate chain — when there's no default
preset (so loadDefaultPreset returns without clearing) we clearChain()
before aeRestoreSavedChain() instead of appending onto the current chain.
- P3: the /api/settings POST now warns on a non-ok HTTP status.
node --check clean. Still needs a desktop build + tester pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(audio): render empty chain directly on amp-sim OFF (avoid getChainState-after-clearChain JUCE crash)
Codex re-review P2: the OFF path called refreshChain() right after
clearChain(), which getChainStates the native engine — a sequence the
codebase documents can crash some JUCE bridges (clearChainForNewSong).
Render the empty-chain placeholder directly instead, mirroring that safe
pattern. localStorage is still preserved so re-enabling restores the tone.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Eliminates the fragile "delete the config folder before upgrading" tester
instruction, which was wrong-by-OS because the userData folder name was
derived inconsistently per platform (fee[dB]ack on macOS, slopsmith-desktop
on Linux/Windows).
A. Deterministic paths + migration framework
- Pin the userData name on every OS via app.setName('feedback-desktop') +
build.extraMetadata.name; brand (productName 'fee[dB]ack') unchanged.
- One-time userData migration copies a legacy folder into the new one so
upgraded users don't start fresh (atomic copy-then-rename, fail-soft).
Runs before the single-instance lock / crashReporter, which would otherwise
create userData and defeat the "new dir doesn't exist" gate.
- config-migrations.ts: versioned, ordered, idempotent, fail-soft migration
runner stamped in CONFIG_DIR/config_version.json; logs the active CONFIG_DIR
at startup (closes the Linux ~/.local/share/slopsmith shared-config gap).
B. In-app "Reset / repair configuration" (Settings panel)
- Granular options: reset app settings & caches, clear plugin state & cached
Python deps, and full reset with default-OFF opt-ins for installed plugins /
song library / ML caches.
- config-paths.ts is the single source of truth for per-OS path enumeration;
the song library, installed plugins and ML caches are structurally confined
to optInExtras and never wiped by the safe/full categories.
- Reset stops the backend, deletes immediate paths, includes SQLite WAL/SHM
sidecars + the migration stamp on full reset, and defers Chromium/Crashpad
state to next launch (consumed before any window reopens it). ML caches honor
TORCH_HOME/HF_HOME. Empty selection is a no-op (backend left running).
- SECURITY: destructive resets require a native main-process confirmation
dialog — the renderer bridge is reachable by plugin scripts, so a
renderer-only confirm is not a sufficient gate.
Tests: node:test suites for path enumeration (per-OS + library/plugins
preserved), migration idempotency/fail-soft, reset delete pipeline guarantees,
userData migration, and deferred-deletion schedule/consume. `npm test` green
(adds a test script). codex review --base origin/main clean.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(sandbox): native last-chance crash attribution for in-process VST3 faults (#35)
The vst-crash-guard sentinel only covers the windows it arms around an
in-process load or editor-open. A plugin that creates a top-level window keeps
it for its whole loaded lifetime, and the OS can dispatch to its WndProc at any
time (e.g. WM_ACTIVATEAPP on an alt-tab). A fault there arrives via USER32→
WndProc with no host frame on the stack — outside every armed sentinel window
and uncatchable by the SignalChain guard — so it's never attributed and the app
crash-loops (diagnosed from dmp a06f48e1 / McRocklin Suite; see #35).
Add a process-wide last-chance attributor (Windows): a SetUnhandledException
filter, chained to the previously installed filter (Crashpad), that on a fatal
fault whose faulting instruction lies inside a loaded .vst3 module stamps the
existing crash sentinel with { plugin, op: "native-crash" } and then defers to
the prior filter so the dump is still produced and the process dies normally.
initVstCrashGuard() already promotes a leftover sentinel into the persistent
blocklist, so the next launch routes the offender to the out-of-process sandbox.
This makes the dead-man's-pedal cover ANY fatal in-process VST3 fault, not just
the armed load/editor windows — generalizing beyond the per-vendor pre-seed.
- src/audio/Sandbox/CrashAttribution.{h,cpp}: install/uninstall + the filter.
SetUnhandledExceptionFilter (last-chance only) avoids first-chance false
positives and per-exception I/O; the write is allocation-free (stack buffers +
raw Win32). No-op on non-Windows (POSIX SignalChain guard covers the armed
path; sandbox is Windows-only today).
- NodeAddon: setVstCrashSentinelPath(path) binding arms it; uninstall on
shutdown (the addon/filter code may be unloaded).
- vst-crash-guard.ts: export getSentinelPath(); audio-bridge wires it after
initVstCrashGuard().
Addon builds clean; tsc --noEmit clean; sandbox tests + e2e unaffected. The
Windows filter path needs hands-on validation (confirm the sentinel is written
and Crashpad still dumps under the target Electron/Crashpad version).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* review: fix bundle-path attribution + one-shot gate in crash attributor
Local review of #36 found two correctness bugs:
- Inert on bundle VST3s: GetModuleFileNameW returns the INNER DLL of a Windows
VST3 bundle (Foo.vst3\Contents\x86_64-win\Foo.vst3), but the blocklist keys on
the bundle dir (desc.fileOrIdentifier = …\Foo.vst3). The two never matched, so
a native-written sentinel never routed the offender to the sandbox — defeating
the fix for bundle plugins. Add truncateToVst3Bundle(): resolve the module
path to its enclosing .vst3 component in place before writing (single-file
.vst3 is unchanged). Replaces endsWithVst3IgnoreCase.
- One-shot latch burned by the wrong exception: the g_writing.exchange gate
wrapped the whole filter evaluation, so the FIRST unhandled exception to reach
the filter — even a non-VST3 or concurrent benign one — permanently disabled
attribution for the real plugin fault. Move the latch to gate only the write,
after a CONFIRMED .vst3 fatal fault; it still serialises concurrent plugin
faults and guards write re-entrancy.
Also: stop zeroing g_sentinelPathW in uninstall (the g_installed acquire-gate
already disarms the write path; zeroing was the only non-atomic mutation that
could race a faulting thread during teardown), and note the address-based
attribution is a heuristic.
Addon builds clean; tsc clean. Windows filter path still needs hands-on
validation (sentinel written for a bundle + single-file VST3; Crashpad still
dumps).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(sandbox): force PolyChrome DSP plugins out-of-process (in-process WndProc DEP crash)
A tester crash dump (feedback.exe 0.3.0, Windows) showed an execute-DEP access
violation (0xC0000005) killing the app while McRocklin Suite.vst3 (PolyChrome
DSP) was loaded IN-PROCESS:
Rax == Rip == McRocklin Suite.vst3 + 0x1D57050 (non-executable module data)
caller [Rsp] = USER32.dll+0xEF5C
WndProc(hwnd=0x51CCA, msg=0x1C WM_ACTIVATEAPP, wParam=1, lParam=0x1838)
crash thread = the addon's background JUCE MessageManager thread (unnamed;
start frame slopsmith_audio.node), NOT Electron's CrBrowserMain.
PolyChrome creates a top-level window during in-process init on JUCE's
*background* message thread. Its WndProc lands in non-executable memory there, so
when Windows broadcasts WM_ACTIVATEAPP the OS message pump executes it → DEP AV.
The plugin assumes a real host main UI thread (STA/main); the sandbox child
provides exactly that, so routing it out-of-process both isolates the fault and
gives the plugin the environment it needs.
Crucially this crash cannot be caught by the SignalChain in-process fault guard:
it arrives asynchronously via USER32→WndProc with NO host frame on the stack, so
guarding prepareToPlay/processBlock (or even instantiation) never sees it. Under
the current in-process-by-default policy (#24) the only fix is to not host these
plugins in-process. Graphene (same vendor) was already pre-seeded; this extends
the pre-seed to the whole PolyChrome vendor via a path-fragment match so McRocklin
Suite and any other PolyChrome product route to the sandbox too.
- Add kDefaultNeedsSandboxPathFragments (vendor/path match) + the loop in
shouldSandbox; seed it with "PolyChrome".
- Refresh the stale kDefaultNeedsSandboxFilenames comment (it still claimed
sandbox-by-default; #24 made the list authoritative again).
- e2e_test: add testShouldSandboxRouting() — pure shouldSandbox assertions
(PolyChrome→sandbox, clean VST3→in-process, non-VST3→in-process).
Verified: audio addon builds clean; sandbox_e2e_test green (16/16, routing
assertions included).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* review: tighten PolyChrome match + dedupe path + harden routing test
Local high-effort review of the pre-seed fix surfaced four items; addressed:
- False-positive risk (no in-process fallback exists here — loadVstSandboxAware
hard-fails a force-sandboxed load that can't spawn the child): narrow the
fragment from the bare brand word "PolyChrome" to the vendor install folder
"PolyChrome DSP", so an unrelated path (e.g. a username "polychrome") no
longer forces the sandbox. Still matches McRocklin Suite + Graphene, which
ship under Common Files/VST3/PolyChrome DSP/.
- Dedupe: getFullPathName() was computed twice (blocklist `canonical` +
vendor `fullPath`); hoist one `fullPath` above the mutex block and reuse it.
- Test isolation: assert McRocklin Suite (NOT in the filename pre-seed) on both
Windows- and POSIX-style paths so the case can only pass via the new vendor
match; drop the redundant Graphene-in-folder line (Graphene already routes via
the filename list).
- Test specificity + exit-code masking: add a negative proving a bare
"polychrome" path is NOT sandboxed (guards the tightening), and surface
routing CHECK failures on the no-args path (return 1, not the usage code 2)
so a regression isn't masked on a manual/argless run.
Addon builds clean; sandbox_e2e_test 17/17 green (routing included).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codex review of #32: the audio-session sanitizer (_safeInputLabel) returns
`labelPseudonym` UN-redacted (it's assumed already safe), so putting a raw OS
device name there leaks PII (e.g. "Byron's AirPods") to diagnostics/consumers
regardless of labelSafe. Put the real name in `label` instead — the field the
sanitizer can redact for suspicious/PII-looking names — and mark labelSafe only
for the generic "Desktop input N" fallback.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
registerAudioSessionInputSources() discarded the real device name (the
forEach arg was `_deviceName`) and labelled every audio-input source with a
generic `Desktop input ${index+1}` pseudonym — so the audio-input picker
showed "Desktop input 1/2/…" instead of the actual device names. The real
names are already available: AudioEngine reads JUCE getDeviceNames(true),
NodeAddon exposes them as typeInfo.inputs, and the renderer already uses
them for setDevice(). Use that name as the source label, falling back to the
generic form only when it's empty.
(Re-applies a fix that previously lived on the dead byrongamatos/slopsmith-
desktop branch and never reached got-feedback/main.)
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A local macOS tester reported the DMG as "damaged and can't be opened."
Root cause: the bundled tree contained a dangling symlink — rig_builder
ships a build-time link `vst/src/racks/DPF` -> DISTRHO framework that isn't
present at runtime. A broken symlink is harmless on Linux/squashfs (the
AppImage was fine), but on macOS it:
- breaks codesign — `spctl` reports "a sealed resource is missing or
invalid", which Gatekeeper surfaces as "damaged"; and
- breaks `xattr -dr com.apple.quarantine` — it aborts on the dangling
link, so even the quarantine-removal workaround can't complete.
Strip dangling symlinks from the cloned core+plugins tree after bundling
(safe on every platform — they're broken/unused). Verified on-device:
after removal + ad-hoc re-sign the app verifies clean and launches once
quarantine is cleared.
Also fix build-macos.sh's artifact check, which still globbed the
pre-rebrand `Slopsmith.app` and falsely reported "No artifacts found" for
the now `feedback.app` bundle.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add got-feedback/rig_builder to the default bundled-plugin manifest so the
Rig Builder (NAM tone builder) ships with every build. The v3 sidebar now
promotes Rig Builder to a dedicated nav entry, so the plugin should be present.
Note: rig_builder is large (~633 MB of NAM models/assets), so this increases
release artifact size accordingly.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three independent gaps currently break a from-scratch Linux build (and CI):
1. .build-config.json is missing `versions.dotnet`, yet both the devcontainer
Dockerfile (`dotnet-install.sh --channel ${DOTNET_VERSION}`) and the CI
workflows (`require('./.build-config.json').versions.dotnet` → setup-dotnet
`${dotnet}.x`) read it. Empty → the image build / setup-dotnet step fails.
Add `"dotnet": "8.0"` (Velopack's vpk targets .NET 8; "8.0" satisfies both
`--channel 8.0` and setup-dotnet's `8.0.x`). Keeps .NET available for
`vpk pack` on tagged release builds.
2. build-linux-docker.sh never forwards GH_CLONE_TOKEN or SLOPSMITH_REF into
the container, so a local `bash scripts/build-linux-docker.sh` can't clone
the private got-feedback core/plugins or select a core ref. Pass both
through (defaulting SLOPSMITH_REF to main).
3. The ffmpeg libvorbis guard pipes `ffmpeg | grep -wq`, which is racy under
`set -o pipefail`: grep -q closes the pipe on first match, ffmpeg takes
SIGPIPE (141), pipefail returns 141, and the guard false-fails even though
libvorbis is present (reproducible on Docker Desktop). Capture the encoder
list into a variable first, then grep.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The file was an HTML page (a failed download) committed in the very
first commit — never a valid impulse response, and not referenced by
name. Shipping it would feed garbage to the convolver if selected.
The 5 other bundled cab IRs are valid and unaffected.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add the re-homed feedback-plugin-loosefolder (dir override :loose_folder
since the plugin id is loose_folder) and feedback-plugin-strum-fighter
to the bundled set (now 36 plugins).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
feedback-desktop is private, so its release assets aren't fetchable by
the unauthenticated build curl or by end users at runtime. Move the
GeneralUser-GS + FluidR3_GM downloads to the public got-feedback/
feedback-soundfonts repo (freely-redistributable GM soundfonts, no
third-party content). Checksums unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop the stale slopsmith-era name now that everything lives under the
got-feedback org. Same secret, used for both the authenticated private
clones (GH_CLONE_TOKEN) and the VERSION-sync dispatch.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The slopsmith org was deleted in the DMCA relaunch; core + plugins now
live under the private got-feedback org. Repoint the whole build:
- core clone (nightly.yml, build-common.sh) -> got-feedback/feedback,
authenticated via GH_CLONE_TOKEN (a PAT with read on the private org;
threaded through build.yml/nightly.yml/release.yml). Local builds
without it fall back to the git credential helper.
- bundled-plugin list -> got-feedback/feedback-plugin-*, pruned to the
set that exists in the org: drops the removed extraction plugins
(profileimport, tones, sloppak-converter) and not-yet-rehomed ones
(nam-rig-builder, tabimport); 34 plugins bundled.
- dirname derivation strips the new feedback-plugin- prefix.
- ghcr image -> ghcr.io/got-feedback/feedback; VERSION-sync dispatch
-> got-feedback/feedback; soundfont + update FEED_URL + docs ->
got-feedback/feedback-desktop.
Two prerequisites remain (owner-only): set the GH_CLONE_TOKEN secret,
and re-upload the soundfonts-v1 release assets to feedback-desktop.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The terminology rename swapped '.psarc' for '.archive' in the
log/telemetry filename-redaction regex. '.archive' is not a real
extension; real '.psarc' filenames on users' disks would stop being
scrubbed and could leak into logs/telemetry. This is a redaction
allow-list, not brand text.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
