Close two defeatable ReleaseProof guards and fix the static-lane skip's dead comparison

ReleaseProof{} and copy-reuse both compiled under this project's C++17;
user-provided ctor, deleted copy ctor and friend close them. The skip now
compares stored values, not norms, so it actually fires. Abort downgraded
to a debug assert.

src/core/instrument/engine/live_params.h

@@ -70,11 +70,13 @@ struct LiveValues {
 // The seqlock copies the block as raw bytes, which is only defensible for a plain value type.
 static_assert(std::is_trivially_copyable_v<LiveValues>,
               "the live block is copied under a seqlock — it must stay a plain value");
-// Guards operator== against silent staleness: a member added to the struct above changes this
+// A SIZE-CHANGING edit only: padding can absorb a member added beside an existing one (a bool
-// size, so the assert fails at the new member's own commit instead of leaving a live control
+// beside splineActive, a fifth FilterSettings float) without moving this literal at all, so this
-// that never reaches a sounding voice with no compiler or test signal. Confirmed 352 bytes,
+// assert is NOT the guard against a forgotten operator== field —
-// MSVC 19.44 x64, Release (`SizeProbe<sizeof(LiveValues)>`, an incomplete-template size probe
+// testEveryFieldOfLiveValuesIsCompared (test_live_params.cpp) is that guard, poisoning one leaf
-// whose error message reports the value). Bump the literal AND operator== together.
+// at a time. This assert only catches an edit that changes sizeof(LiveValues) itself. Confirmed
+// 352 bytes, MSVC 19.44 x64, Release (`SizeProbe<sizeof(LiveValues)>`, an incomplete-template
+// size probe whose error message reports the value). Bump the literal AND operator== together.
 static_assert(sizeof(LiveValues) == 352,
               "a member was added or removed — extend operator== in live_params.cpp to match");
 

src/shell/instrument/CLAUDE.md

@@ -132,7 +132,7 @@ the single authority.** Everything else that holds these values is a cache or a
 
 Every writer above except the last two writes the model directly, so for those "authority ends"
 is just "the write happened". Reload seed is not itself a model write — `reloadInstrument` never
-touches `params_`; the only write in the tree is `setInstrumentParams`'s, `processor_state.cpp:192`
+touches `params_`; the only write in the tree is `setInstrumentParams`'s, `processor_state.cpp:193`
 — which is why its row states no authority window of its own. The automation lane is the only one
 that cannot write directly: the SDK delivers it on the audio thread, where the model path
 allocates (`resolvePlay` copies velocity curves and spline contours). So it patches the

src/shell/instrument/automation_channel.h

@@ -2,47 +2,74 @@
 // AUTHORITY LIFETIME is mechanised: a point outranks the model from the block it lands in until
 // the UI thread has folded it back into the model AND republished. This directory's CLAUDE.md
 // states the model; `core/instrument/param/param_merge` is the pure decision this feeds.
-// The release itself is observed the NEXT BLOCK, not the instant it happens — refreshReleases()
+// The release itself is observed the NEXT BLOCK, not the instant it happens — see
-// only runs inside process()'s merge branch (reasampler_processor.cpp), and publishLiveParams
+// refreshReleases().
-// always bumps the generation that branch checks, so the next block is guaranteed to take it.
 
 #pragma once
 
 #include <atomic>
+#include <cassert>
 #include <cstddef>
 #include <cstdint>
-#include <cstdlib>
 
 #include "core/instrument/param/param_merge.h"
 
 namespace reasampler::vst {
 
+class ReaSamplerProcessor;  // the one class that legitimately mints a ReleaseProof
+
 // The DeckParam ordinal space — what the automation slots and the notification diff both index.
 inline constexpr std::size_t kDeckParamSlots = instrument::param::kDeckParamSlots;
 
 class AutomationChannel {
 public:
     // Evidence release() below requires: the model has actually caught up with the
-    // point being released. The private constructor means the only way to obtain one
+    // point being released. The two factories are named for the case they cover —
-    // is through a factory here, and both are named for the case they cover —
     // `fromPublish` for an observed model republish, `noRepublishNeeded` for the two
     // cases drainAutomationToModel finds nothing to publish (the fold left the model
-    // unchanged, or no engine exists yet to read the live block). A caller that has
+    // unchanged, or no engine exists yet to read the live block). This is NOT a full
-    // not published cannot spell the argument release() needs, which is what turns
+    // compile-time proof of ordering: `noRepublishNeeded` still lets `ReaSamplerProcessor`
-    // reordering the release ahead of the publish into a compile error.
+    // claim the exemption with no publish at all. What it enforces structurally is that a
+    // caller cannot spell release()'s argument without naming, by which factory it called,
+    // WHICH exemption it is claiming — greppable by a reviewer, not silently inline — and
+    // both factories are restricted to the one class that legitimately needs either.
+    // The private, user-provided default constructor plus the deleted copy constructor
+    // close the two ways `{}` and copy-reuse would otherwise fabricate one for free; see
+    // the two definitions below for which C++17 rule each closes.
     class ReleaseProof {
-    public:
+    private:
+        friend class ReaSamplerProcessor;
+
         static ReleaseProof fromPublish(std::uint32_t before, std::uint32_t after) {
-            // publish() is guaranteed to advance the generation; landing here means
+            // publish() advances the generation BY CONSTRUCTION (gen + 2, skipping 0 on wrap —
-            // that guarantee broke, worth crashing on unconditionally rather than
+            // live_params.h), so `after == before` is unreachable from this, its one call site,
-            // tolerating silently.
+            // short of ~2^31 publishes wrapping exactly onto `before`. It also cannot see the
-            if (after == before) std::abort();
+            // actually-reachable failure mode, a caller falsely claiming noRepublishNeeded() —
+            // that path never runs this function. DEBUG-ONLY on purpose: an unconditional abort
+            // here would take down a musician's whole REAPER session, unsaved work on every other
+            // track and plugin included, for a condition this call site cannot produce; a debug
+            // build (where the test suite runs) is where a future regression in publish()'s
+            // advance guarantee should be caught.
+            assert(after != before && "publish() must advance the generation");
             return ReleaseProof{};
         }
         static ReleaseProof noRepublishNeeded() { return ReleaseProof{}; }
 
-    private:
+        // User-PROVIDED (a body, not `= default`): under C++17 a class with no data
-        ReleaseProof() = default;
+        // members and only a user-DECLARED (not user-provided) default constructor is
+        // still an aggregate, because C++17's aggregate rule excludes private data
+        // members only, not private constructors — so `ReleaseProof{}` would perform
+        // aggregate init and never call this. A user-provided constructor defeats that.
+        // (C++20's P1008 closes the same hole at the language level; this project is
+        // pinned to C++17 — CMakeLists.txt:28 — so the class must close it itself.)
+        ReleaseProof() {}
+        // Closes the other free-mint path: the implicit copy constructor is public by
+        // default, so one legitimately-minted proof cached in a member could be replayed
+        // by any later caller with no new publish behind it. A user-declared copy
+        // constructor (deleted or not) also suppresses the implicit move constructor, so
+        // no move-based replay path opens in its place — `release()` below takes this by
+        // const reference for exactly that reason, rather than needing one back.
+        ReleaseProof(const ReleaseProof&) = delete;
     };
 
     // --- AUDIO THREAD -------------------------------------------------------------------
@@ -72,12 +99,16 @@ public:
     }
 
     // Refreshes each held slot's release answer. Runs only inside process()'s merge branch, so a
-    // release lands the NEXT BLOCK after the UI thread makes it, never the same instant — benign,
+    // release lands the NEXT BLOCK after the UI thread makes it, never the same instant — benign
-    // because publishLiveParams always bumps the generation that branch checks, so the next block
+    // on the `moved` path because publishLiveParams always bumps the generation that branch
-    // is guaranteed to run this. Must run BEFORE the model block is read: the acquire here
+    // checks, so the next block is guaranteed to run this. The `noRepublishNeeded()` path has no
-    // synchronizes with the UI thread's release store, which it makes only AFTER republishing the
+    // publish and no generation bump to guarantee that — land() drops a static lane's repeats, so
-    // model — so a slot seen released is one whose value any block read after this point is
+    // `moved` is false there too — but it is equally benign: the model already carries the value
-    // guaranteed to already carry.
+    // (that is why nothing published), so the release is simply observed whenever the generation
+    // next moves under ANY writer, not specifically this one. Must run BEFORE the model block is
+    // read: the acquire here synchronizes with the UI thread's release store, which it makes only
+    // AFTER republishing the model — so a slot seen released is one whose value any block read
+    // after this point is guaranteed to already carry.
     void refreshReleases() {
         for (std::size_t i = 0; i < kDeckParamSlots; ++i) {
             if (!slots_[i].held) continue;
@@ -102,8 +133,10 @@ public:
 
     // Releases `slot`'s hold. The `ReleaseProof` argument is the enforcement: it can only be
     // constructed once the model carrying this point has been republished (or shown not to need
-    // it), so a caller earlier in that ordering has no value to pass.
+    // it), so a caller earlier in that ordering has no value to pass. By const reference, not
-    void release(std::size_t slot, std::uint32_t seq, ReleaseProof) {
+    // value: the copy constructor is deleted (see ReleaseProof), and the one caller releasing a
+    // whole fold's worth of slots passes the same proof through this repeatedly.
+    void release(std::size_t slot, std::uint32_t seq, const ReleaseProof&) {
         folded_[slot].store(seq, std::memory_order_release);
     }
 

src/shell/instrument/instrument_params.cpp

@@ -295,8 +295,27 @@ void ReaSamplerProcessor::drainAutomationToModel() {
         // A present-but-static lane resends the SAME point every tick; writing it back would
         // republish liveParams_ and move paramsGeneration_ — a full model copy, a controller
         // cache write and an editor repaint, every tick, forever, for a value that never moved.
-        // The release below still fires: the model already carries the point either way.
+        // Compared on the STORED side, never the normalized one: toNormalized(toPlain(n)) == n
-        if (value == modelParamNormalized(params, row.deck)) continue;
+        // does NOT hold in general (param_taper.h — "no log map satisfies it in double"), so
+        // comparing `value` to modelParamNormalized(params, ...) skipped almost nothing for the
+        // 33 of 43 exposed controls whose taper is log/curved rather than identity — the churn
+        // this comment describes ran unabated for those. hostStoredFromNorm is the exact map
+        // writeDeckParamToModel below applies, so comparing its answer against the field it would
+        // land in asks "would this write change anything", not "are two norms equal". The
+        // release below still fires either way: the model already carries the point regardless
+        // of whether this write actually runs.
+        const double newStored = param::hostStoredFromNorm(row.deck, value);
+        bool unchanged = false;
+        if (row.deck == DeckParam::kKeyTrack) {
+            unchanged = newStored == params.keyTrack;
+        } else if (float* f = instrument::ui::deckFloatField(row.deck, params.play)) {
+            // The filter's four store a float: compared at that width, since that is what a
+            // re-write would actually round to, not the double newStored computes before it.
+            unchanged = static_cast<float>(newStored) == *f;
+        } else if (double* d = instrument::ui::deckDoubleField(row.deck, params.play)) {
+            unchanged = newStored == *d;
+        }
+        if (unchanged) continue;
         writeDeckParamToModel(params, row.deck, value);
         moved = true;
     }
@@ -312,9 +331,15 @@ void ReaSamplerProcessor::drainAutomationToModel() {
     // CLAUDE.md, THE AUTHORITY MODEL) — `releaseProof` is that ordering enforced structurally:
     // `automation_.release` below cannot compile without one, and the only ways to obtain one are
     // `publishLiveParams`'s return (the `moved` branch) or `noRepublishNeeded` (nothing to
-    // publish because the fold left the model already matching the point).
+    // publish because the fold left the model already matching the point). Built via an
-    const AutomationChannel::ReleaseProof releaseProof =
+    // immediately-invoked lambda rather than `moved ? publishLiveParams() : noRepublishNeeded()`
-        moved ? publishLiveParams() : AutomationChannel::ReleaseProof::noRepublishNeeded();
+    // directly: ReleaseProof's copy constructor is deleted (automation_channel.h), and a ternary
+    // between two same-type prvalue arms needs it to merge them into one value — each `return`
+    // below is instead its OWN guaranteed-elided construction of the function's result object.
+    const AutomationChannel::ReleaseProof releaseProof = [&]() -> AutomationChannel::ReleaseProof {
+        if (moved) return publishLiveParams();
+        return AutomationChannel::ReleaseProof::noRepublishNeeded();
+    }();
     syncParamsFromModel();
     paramNotifySuppressed_ = wasSuppressed;
     for (std::size_t i = 0; i < foldedCount; ++i) {