fix(api): stop rejecting merge_args, and cover the round-trip

Seventh review pass:

- Delegating to the config validator caught merge_args in its `*_args`
  rule, which demands a string, so no value of merge_args could be
  saved. Only backend-descriptor options have a global-config
  counterpart, so only those are delegated now.
- Negative values are rejected for float options too, not just integers.
- DELETE resets the pin whether or not one was saved; a request-scoped
  /load can pin a process without writing anything.
- save_model_options rolls back a failed write like its sibling.
- RecipeOptions::inherit reads merge_args defensively; a hand-edited
  non-boolean took down every read of the model.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: jeremyfowers

src/cpp/include/lemon/recipe_options.h

@@ -31,6 +31,11 @@ class RecipeOptions {
     /// Option names this recipe accepts, in declaration order.
     static std::vector<std::string> keys_for_recipe(const std::string& recipe);
 
+    /// True when `key` comes from the recipe's backend descriptor rather than
+    /// the universal kit (ctx_size, merge_args, the eviction options, pinned).
+    /// Only the former have a matching key in the global config.
+    static bool is_backend_option(const std::string& recipe, const std::string& key);
+
     /// True for values the constructor drops as "not set": null, -1, "" and
     /// "auto". ctx_size is the exception — every non-number is dropped there,
     /// since -1 is the storable value meaning "size the context automatically".

src/cpp/server/model_manager.cpp

@@ -1585,15 +1585,34 @@ void ModelManager::save_model_options(const ModelInfo& info) {
     LOG(INFO, "ModelManager") << "Saving options for model: " << info.model_name << std::endl;
     // Persist under canonical ID (built-ins are keyed bare in cache but
     // recipe_options.json stores them as builtin.<name>).
+    const std::string id = cache_key_to_canonical_id(info.model_name);
     std::lock_guard<std::mutex> write_lock(recipe_options_write_mutex_);
+
     json snapshot;
+    json previous;
+    bool had_previous = false;
     {
         std::lock_guard<std::mutex> lock(models_cache_mutex_);
-        recipe_options_[cache_key_to_canonical_id(info.model_name)] = info.recipe_options.to_json();
+        had_previous = recipe_options_.contains(id);
+        if (had_previous) previous = recipe_options_[id];
+        recipe_options_[id] = info.recipe_options.to_json();
         snapshot = recipe_options_;
         update_model_options_in_cache_locked(info);
     }
-    save_user_json(get_recipe_options_file(), snapshot);
+
+    try {
+        save_user_json(get_recipe_options_file(), snapshot);
+    } catch (...) {
+        // Keep memory matching disk, as write_saved_model_options does.
+        std::lock_guard<std::mutex> lock(models_cache_mutex_);
+        if (had_previous) {
+            recipe_options_[id] = previous;
+        } else {
+            recipe_options_.erase(id);
+        }
+        cache_valid_ = false;
+        throw;
+    }
 }
 
 json ModelManager::get_saved_model_options(const std::string& model_name) {

src/cpp/server/recipe_options.cpp

@@ -109,6 +109,15 @@ std::vector<std::string> RecipeOptions::keys_for_recipe(const std::string& recip
     return get_keys_for_recipe(recipe);
 }
 
+bool RecipeOptions::is_backend_option(const std::string& recipe, const std::string& key) {
+    const auto* desc = lemon::backends::descriptor_for(recipe);
+    if (!desc) return false;
+    for (const auto& opt : desc->options) {
+        if (opt.name == key) return true;
+    }
+    return false;
+}
+
 bool RecipeOptions::is_default_sentinel(const std::string& key, const json& value) {
     return is_empty_option(key, value);
 }
@@ -202,7 +211,11 @@ std::string RecipeOptions::to_log_string(bool resolve_defaults) const {
 
 RecipeOptions RecipeOptions::inherit(const RecipeOptions& options) const {
     json merged = options_;
-    bool merge_args = options_.contains("merge_args") ? options_["merge_args"].get<bool>() : options.get_option("merge_args").get<bool>();
+    // Read defensively: a hand-edited recipe_options.json can hold any type
+    // here, and throwing would take down every read of the model.
+    const json own_merge_args = options_.contains("merge_args") ? options_["merge_args"]
+                                                                : options.get_option("merge_args");
+    bool merge_args = own_merge_args.is_boolean() ? own_merge_args.get<bool>() : true;
 
     for (auto it = options.options_.begin(); it != options.options_.end(); ++it) {
         if (merge_args && it.key().size() >= 5 && it.key().substr(it.key().size() - 5) == "_args") {

src/cpp/server/server.cpp

@@ -2809,18 +2809,20 @@ static std::string validate_option_value(const RuntimeConfig& config,
     // A fractional value for a whole-number option is silently ignored by the
     // consumers that read it, and a negative one is meaningless for all of
     // them, so refuse both.
-    if (expected.is_number_integer()) {
-        if (!value.is_number_integer()) {
-            return "'" + key + "' must be a whole number";
-        }
-        if (value.get<int64_t>() < 0) {
-            return "'" + key + "' cannot be negative";
-        }
+    if (expected.is_number_integer() && !value.is_number_integer()) {
+        return "'" + key + "' must be a whole number";
+    }
+    if (expected.is_number() && value.is_number() && value.get<double>() < 0) {
+        return "'" + key + "' cannot be negative";
     }
 
     // Defer to the validator the global config already uses for these same
-    // option names, so the two surfaces cannot disagree about a value. Keys it
-    // does not know about (the eviction options, *_args) fall through.
+    // option names, so the two surfaces cannot disagree about a value. Only
+    // descriptor options have a global-config counterpart; the universal kit
+    // must not be routed through it, since key names like merge_args collide
+    // with the config's own rules.
+    if (!RecipeOptions::is_backend_option(recipe, key)) return "";
+
     std::string config_key = key;
     const std::string recipe_prefix = recipe + "_";
     if (config_key.rfind(recipe_prefix, 0) == 0) {
@@ -2983,9 +2985,10 @@ void Server::handle_model_options_delete(const httplib::Request& req, httplib::R
     respond_with_model_options(req, res,
         [this](const std::string& model_key, const ModelInfo&, httplib::Response&,
                bool& touched_pinned) {
-            const nlohmann::json saved = model_manager_->get_saved_model_options(model_key);
-            touched_pinned = saved.contains("pinned");
-            if (!saved.empty()) {
+            // Resetting to defaults covers the pin whether or not one was saved:
+            // a request-scoped /load may have pinned the live process instead.
+            touched_pinned = true;
+            if (!model_manager_->get_saved_model_options(model_key).empty()) {
                 model_manager_->set_saved_model_options(model_key, nlohmann::json::object());
             }
             return true;

test/server_endpoints.py

@@ -1346,6 +1346,38 @@ def live_pinned():
 
         print("[OK] pinned changes are applied to the running model")
 
+    def test_012x_every_effective_option_can_be_saved_back(self):
+        """Posting the reported `effective` object back whole is accepted.
+
+        The response advertises `effective` as the set of names POST accepts, so
+        any key the endpoint reports but refuses is a contradiction. This caught
+        `merge_args`, whose name collides with the global config's `*_args` rule.
+        """
+        self.addCleanup(self._reset_options)
+        self._reset_options()
+
+        effective = requests.get(self._options_url(), timeout=TIMEOUT_DEFAULT).json()[
+            "effective"
+        ]
+        response = requests.post(
+            self._options_url(), json=effective, timeout=TIMEOUT_DEFAULT
+        )
+        self.assertEqual(
+            response.status_code,
+            200,
+            f"Every reported option must be settable, got {response.text}",
+        )
+
+        # merge_args specifically: a boolean, and both values have to round-trip.
+        for value in (True, False):
+            saved = requests.post(
+                self._options_url(), json={"merge_args": value}, timeout=TIMEOUT_DEFAULT
+            )
+            self.assertEqual(saved.status_code, 200, saved.text)
+            self.assertEqual(saved.json()["saved"]["merge_args"], value)
+
+        print("[OK] Every option reported in `effective` can be saved back")
+
     def test_012w_options_reject_values_the_global_config_rejects(self):
         """Per-model options are held to the same value rules as global config.