fix(core): merge user config over defaults so an absent key keeps its C++ default

ConfigMgr2::init() read vnotex.json raw and handed it straight to
MainConfig::fromJson(). The IConfig read helpers are not presence-aware, so any
key the file did not carry - one added by a newer VNote, or lost to a truncated
file - was read back as false/0/"" and then persisted. The docs claimed the
opposite.

The user's document is now applied as an RFC 7386 merge patch on top of the
default-constructed MainConfig's toJson(), from a single read, so every key is
present by the time fromJson() runs. This retires the bug class instead of
patching one key: the presence-aware readBool overload added for
autoFoldPreviewedBlocks is reverted.

Three deliberate exceptions:
- widget.newNoteDefaultTemplates is owned wholesale by the user (a present but
  empty object means "none"), so it is restored from the raw snapshot after the
  merge.
- A config that exists but cannot be read - unparseable, or valid JSON that is
  not an object - now suppresses every main-config write for the session instead
  of yielding {} and letting defaults overwrite it.
- session.json stays unmerged: SessionConfig uses isUndefinedKey() to tell
  absent from present-and-false.

Gate: a generic test drops each leaf key of the defaults document in turn,
reloads through a real ConfigMgr2::init(), and asserts the default survives.

Author: tamlok

src/core/AGENTS.md

@@ -114,7 +114,12 @@ QString templatePath = configMgr->getFileFromConfigFolder("web/markdown-viewer-t
 #### Why ConfigMgr2 Exists
 
 `ConfigMgr2` wraps `ConfigCoreService` and adds:
-1. **Default merging** — `ConfigMgr2::init()` loads the `vnotex.json` config file via `ConfigCoreService::getConfigByName()`, then calls `MainConfig::fromJson()`. The `MainConfig` constructor first calls `initDefaults()` on all child configs, then `fromJson()` overlays only the keys present in the JSON. Missing keys keep their C++ defaults.
+1. **Default merging** — `ConfigMgr2::init()` reads `vnotex.json` ONCE (`ConfigCoreService::getConfigByName`, with the `bool *p_ok` out-param) and applies it as an RFC 7386 merge patch on top of the default-constructed `MainConfig`'s `toJson()` (objects merge per key, a JSON `null` deletes the key, arrays and scalars replace wholesale). The JSON handed to `MainConfig::fromJson()` therefore always carries **every** key, which is what makes a key that a user's older `vnotex.json` never had — or one lost to a truncated file — keep its C++ default. The read helpers in `IConfig` (`readBool`, `readInt`, `readReal`, `readString`) are NOT presence-aware: absent → `false`/`0`/`""`. Their correctness depends entirely on this merge, which is why a throwaway `MainConfig` built from raw `getConfigByName()` JSON (the anti-pattern above) wipes defaults. Three deliberate exceptions:
+   - **Objects the user owns wholesale** (`kUserOwnedObjects` in `configmgr2.cpp`, currently `widget.newNoteDefaultTemplates`) are restored from the raw document after the merge: for them a present-but-empty object means "none", and per-key merging would resurrect the bundled entries.
+   - **A config that exists but cannot be read** (unparseable, or valid JSON that is not an object) sets `m_mainConfigReadFailed`, which suppresses every main-config write for the session so defaults can never overwrite it. An absent or empty file is NOT a failure.
+   - **`session.json` is not merged**: `SessionConfig` uses `isUndefinedKey()` to distinguish "absent" from "present and false".
+
+   Regression gates: `testAbsentKeyKeepsTheCppDefaultForEveryField`, `testAnExplicitlyEmptiedUserOwnedMapIsNotResurrected`, `testAnUnreadableConfigIsNeverOverwritten` in `tests/core/test_configmgr2.cpp`.
 2. **Debounced persistence** — Changes to config objects auto-save via 500ms debounced timers.
 3. **Path resolution** — `getFileFromConfigFolder()` resolves relative paths against the app data directory.
 4. **Version upgrade** — `upgradeMainConfigOnVersionChange()` runs the config migration (`doVersionSpecificOverride` then stamping `c_version`) when the persisted version differs.

src/core/configmgr2.cpp

@@ -72,26 +72,96 @@ ConfigMgr2::~ConfigMgr2() {
   }
 }
 
+// Sections/keys whose object value is owned WHOLESALE by the user, i.e. where a present but
+// empty object is a deliberate "none", not "unset". merge_patch merges objects per key, so
+// without this the bundled default entries would come back and the user could never turn them
+// all off (see WidgetConfig::fromJson for the newNoteDefaultTemplates contract).
+struct UserOwnedObject {
+  const char *m_section;
+  const char *m_key;
+};
+
+constexpr UserOwnedObject kUserOwnedObjects[] = {{"widget", "newNoteDefaultTemplates"}};
+
+void ConfigMgr2::restoreUserOwnedObjects(QJsonObject &p_merged, const QJsonObject &p_raw) {
+  for (const auto &entry : kUserOwnedObjects) {
+    const QString section = QLatin1String(entry.m_section);
+    const QString key = QLatin1String(entry.m_key);
+
+    const auto rawSection = p_raw.value(section).toObject();
+    if (!rawSection.contains(key)) {
+      continue;
+    }
+
+    auto mergedSection = p_merged.value(section).toObject();
+    mergedSection[key] = rawSection.value(key);
+    p_merged[section] = mergedSection;
+  }
+}
+
+// RFC 7386 JSON Merge Patch: @p_patch (the user's document) applied on top of @p_target (the
+// defaults document). Objects merge per key, a null DELETES the key, and everything else -
+// including arrays - replaces wholesale.
+//
+// Applied Qt-side, on the ONE raw document read below, rather than through
+// ConfigCoreService::getConfigByNameWithDefaults(): that variant re-reads the file, so the
+// merged result and the raw snapshot used by restoreUserOwnedObjects() could come from two
+// different versions of it.
+static QJsonValue applyMergePatch(const QJsonValue &p_target, const QJsonValue &p_patch) {
+  if (!p_patch.isObject()) {
+    return p_patch;
+  }
+
+  QJsonObject result = p_target.isObject() ? p_target.toObject() : QJsonObject();
+  const auto patchObj = p_patch.toObject();
+  for (auto it = patchObj.begin(); it != patchObj.end(); ++it) {
+    if (it.value().isNull()) {
+      result.remove(it.key());
+    } else {
+      result[it.key()] = applyMergePatch(result.value(it.key()), it.value());
+    }
+  }
+  return result;
+}
+
 void ConfigMgr2::init() {
   qCDebug(lcConfig) << "ConfigMgr2 initializing with paths:"
                     << "app=" << m_appDataPath << "user=" << m_localDataPath;
 
-  // Load and initialize main config
+  // Load and initialize main config.
+  //
+  // The user's document is MERGED on top of the defaults: m_mainConfig is default-constructed
+  // (every child config has already run its initDefaults()), so its toJson() is exactly the
+  // defaults document. A key the file does not carry - a key added by a newer VNote, or one
+  // lost to a truncated/hand-edited file - therefore keeps its C++ default instead of being
+  // read back as false/0/"". Session config deliberately does NOT go through this path; see
+  // below.
   {
-    auto mainConfigJson =
-        m_configService->getConfigByName(DataLocation::App, kMainConfigFileBaseName);
-    m_versionChanged = MainConfig::peekVersion(mainConfigJson) != c_version.toString();
-
-    if (mainConfigJson.isEmpty()) {
-      // Fresh start: no config file on disk. Keep the default-constructed config
-      // objects as-is (their C++ initDefaults() provide correct defaults).
-      qInfo() << "Fresh start detected, using default-constructed config";
+    // ONE read, used both as the merge patch and as the presence oracle for the user-owned
+    // objects restored afterwards.
+    bool readOk = true;
+    const auto rawJson =
+        m_configService->getConfigByName(DataLocation::App, kMainConfigFileBaseName, &readOk);
+
+    if (!readOk) {
+      // The file exists but could not be read or parsed. Keep the in-memory defaults and
+      // refuse to write, so a transient read failure never overwrites the user's settings
+      // with defaults.
+      m_mainConfigReadFailed = true;
+      qWarning() << "Failed to read main config; running on defaults and suppressing writes";
     } else {
+      auto mainConfigJson = applyMergePatch(m_mainConfig->toJson(), rawJson).toObject();
+      restoreUserOwnedObjects(mainConfigJson, rawJson);
+      m_versionChanged = MainConfig::peekVersion(mainConfigJson) != c_version.toString();
       m_mainConfig->fromJson(mainConfigJson);
     }
   }
 
-  // Load and initialize session config
+  // Load and initialize session config.
+  //
+  // NOT merged on purpose: SessionConfig branches on isUndefinedKey() for systemTitleBar and
+  // minimizeToSystemTray, i.e. it distinguishes "absent" from "present and false". Merging
+  // defaults in would make absent impossible and silently change that behavior.
   {
     auto sessionConfigJson =
         m_configService->getConfigByName(DataLocation::Local, kSessionFileBaseName);
@@ -278,6 +348,14 @@ void ConfigMgr2::doWriteMainConfig() {
     return;
   }
 
+  if (m_mainConfigReadFailed) {
+    // init() could not read the existing config, so what we hold is defaults plus whatever
+    // this session changed. Writing that out would destroy the user's settings.
+    qWarning() << "Refusing to write main config: it could not be read at startup";
+    m_pendingMainConfig = QJsonObject();
+    return;
+  }
+
   qDebug() << "Writing main config";
   Error err = m_configService->updateConfigByName(DataLocation::App, kMainConfigFileBaseName,
                                                   m_pendingMainConfig);

src/core/configmgr2.h

@@ -158,6 +158,12 @@ private slots:
   void scheduleMainConfigWrite();
   void scheduleSessionConfigWrite();
 
+  // Put back the objects the user owns wholesale (e.g. widget.newNoteDefaultTemplates) after
+  // the defaults merge: merge_patch merges objects per key, which would resurrect the bundled
+  // entries of a map the user deliberately emptied. @p_raw is the unmerged document the merge
+  // was built from - the only place that still knows whether the key was present on disk.
+  static void restoreUserOwnedObjects(QJsonObject &p_merged, const QJsonObject &p_raw);
+
   // Perform version upgrade of the config itself (version-gated forced
   // overrides + version stamping). The bundled extra-data dump is NOT part of
   // this; it is owned by ensureExtraData(), which runs on every launch.
@@ -185,6 +191,11 @@ private slots:
   // Whether version changed since last run
   bool m_versionChanged = false;
 
+  // Set when init() could not READ an existing main config (as opposed to there being none).
+  // Suppresses every main-config write for the rest of the session so defaults cannot
+  // overwrite a config we merely failed to read.
+  bool m_mainConfigReadFailed = false;
+
   // Folders whose extra-data install failed during the last ensureExtraData().
   QVector<ExtraDataFailure> m_extraDataFailures;
 

src/core/iconfig.h

@@ -92,28 +92,15 @@ class IConfig {
     writeByteArray(p_obj, p_key, bytes);
   }
 
+  // NOTE: an absent key maps to false here, and that is safe ONLY because ConfigMgr2::init()
+  // applies the user's vnotex.json as an RFC 7386 merge patch on top of the
+  // default-constructed config's toJson(), so every key is present by the time fromJson()
+  // runs. Do not feed a raw, unmerged document to a config's fromJson(), and do not construct
+  // a throwaway MainConfig from ConfigCoreService::getConfigByName() JSON.
   static bool readBool(const QJsonObject &p_obj, const QString &p_key) {
     return p_obj.value(p_key).toBool();
   }
 
-  // Presence-aware overload, for a key whose default is NOT false.
-  //
-  // The plain readBool() above maps an absent key to false, which is correct only for a
-  // false-default setting. ConfigMgr2::init() skips fromJson() only when the config file is
-  // entirely absent, so on every existing installation a newly introduced key IS read - and a
-  // true-default one would silently flip to false on upgrade. Use this overload for any
-  // true-default key you ADD.
-  //
-  // The pre-existing true-default keys (markdowneditorconfig, widgetconfig, texteditorconfig,
-  // coreconfig) still use the two-argument form. They are not exposed to the upgrade path
-  // above, because any config file written by a version that had them also contains them;
-  // only a hand-edited or truncated file can hit it. Converting them is a separate audit with
-  // its own behavior change, deliberately not folded into the commit that added this overload.
-  static bool readBool(const QJsonObject &p_obj, const QString &p_key, bool p_defaultValue) {
-    const auto value = p_obj.value(p_key);
-    return value.isBool() ? value.toBool() : p_defaultValue;
-  }
-
   static int readInt(const QJsonObject &p_obj, const QString &p_key) {
     return p_obj.value(p_key).toInt();
   }

src/core/mainconfig.cpp

@@ -23,7 +23,10 @@ MainConfig::MainConfig(IConfigMgr *p_mgr) : IConfig(p_mgr, nullptr) {
 MainConfig::~MainConfig() {}
 
 void MainConfig::fromJson(const QJsonObject &p_jobj) {
-  // p_jobj is already merged (defaults + user overrides)
+  // p_jobj MUST already be merged (defaults + user overrides). ConfigMgr2::init() guarantees
+  // this by applying the user's document as an RFC 7386 merge patch on top of the defaults;
+  // the child fromJson()s below map an absent key to false/0/"", so a raw, unmerged document
+  // would wipe every non-zero default.
   loadMetadata(p_jobj);
 
   for (auto &childConfig : m_childConfigs) {

src/core/markdowneditorconfig.cpp

@@ -58,10 +58,7 @@ void MarkdownEditorConfig::fromJson(const QJsonObject &p_jobj) {
   m_smartTableEnabled = READBOOL(QStringLiteral("smartTable"));
   m_smartTableInterval = READINT(QStringLiteral("smartTableInterval"));
   m_alignTableSourceEnabled = READBOOL(QStringLiteral("alignTableSource"));
-  // Defaults to true, so it must not go through READBOOL: an absent key would turn
-  // auto-folding off for every existing installation on upgrade.
-  m_autoFoldPreviewedBlocksEnabled =
-      readBool(p_jobj, QStringLiteral("autoFoldPreviewedBlocks"), true);
+  m_autoFoldPreviewedBlocksEnabled = READBOOL(QStringLiteral("autoFoldPreviewedBlocks"));
 
   m_spellCheckEnabled = READBOOL(QStringLiteral("spellCheck"));
 

src/core/services/configcoreservice.cpp

@@ -102,8 +102,12 @@ QJsonObject ConfigCoreService::getSessionConfig() const {
   return parseJsonObjectFromCStr(json);
 }
 
-QJsonObject ConfigCoreService::getConfigByName(DataLocation p_location,
-                                               const QString &p_baseName) const {
+QJsonObject ConfigCoreService::getConfigByName(DataLocation p_location, const QString &p_baseName,
+                                               bool *p_ok) const {
+  if (p_ok) {
+    *p_ok = false;
+  }
+
   if (!checkContext()) {
     return QJsonObject();
   }
@@ -113,14 +117,41 @@ QJsonObject ConfigCoreService::getConfigByName(DataLocation p_location,
       vxcore_context_get_config_by_name(m_context, static_cast<VxCoreDataLocation>(p_location),
                                         p_baseName.toUtf8().constData(), &json);
   if (err != VXCORE_OK) {
+    // An absent config is not a read failure: there is simply nothing there yet.
+    if (p_ok) {
+      *p_ok = (err == VXCORE_ERR_NOT_FOUND);
+    }
     return QJsonObject();
   }
-  return parseJsonObjectFromCStr(json);
+
+  // A payload that does not parse, or that is not a JSON OBJECT, is a read failure rather
+  // than an empty config: silently returning {} would let a caller overwrite it with
+  // defaults. An EMPTY payload is the exception - an empty file carries no settings to lose.
+  const QByteArray payload = QByteArray(json).trimmed();
+  vxcore_string_free(json);
+  if (payload.isEmpty()) {
+    if (p_ok) {
+      *p_ok = true;
+    }
+    return QJsonObject();
+  }
+
+  QJsonParseError parseError;
+  const auto doc = QJsonDocument::fromJson(payload, &parseError);
+  if (p_ok) {
+    *p_ok = (parseError.error == QJsonParseError::NoError && doc.isObject());
+  }
+  return doc.object();
 }
 
 QJsonObject ConfigCoreService::getConfigByNameWithDefaults(DataLocation p_location,
                                                            const QString &p_baseName,
-                                                           const QJsonObject &p_defaults) const {
+                                                           const QJsonObject &p_defaults,
+                                                           bool *p_ok) const {
+  if (p_ok) {
+    *p_ok = false;
+  }
+
   if (!checkContext()) {
     return p_defaults;
   }
@@ -130,7 +161,15 @@ QJsonObject ConfigCoreService::getConfigByNameWithDefaults(DataLocation p_locati
       m_context, static_cast<VxCoreDataLocation>(p_location), p_baseName.toUtf8().constData(),
       QJsonDocument(p_defaults).toJson(QJsonDocument::Compact).constData(), &json);
   if (err != VXCORE_OK) {
-    return QJsonObject();
+    // vxcore already folded "absent" into VXCORE_OK + defaults, so reaching here means the
+    // config exists but could not be read or parsed. Hand back the defaults (there is nothing
+    // better to run on) but report the failure, so the caller can refuse to persist over it.
+    qWarning() << "Failed to read config" << p_baseName << "with defaults: error=" << err;
+    return p_defaults;
+  }
+
+  if (p_ok) {
+    *p_ok = true;
   }
   return parseJsonObjectFromCStr(json);
 }

src/core/services/configcoreservice.h

@@ -48,11 +48,24 @@ class ConfigCoreService : public QObject, private Noncopyable {
   QJsonObject getSessionConfig() const;
 
   // Get configuration by name for specified location.
-  QJsonObject getConfigByName(DataLocation p_location, const QString &p_baseName) const;
-
-  // Get configuration by name with default values fallback.
+  //
+  // @p_ok, when given, reports whether the config could be READ. An absent file is not a
+  // failure (it yields an empty object with *p_ok == true); a present-but-unreadable or
+  // unparseable one sets *p_ok to false.
+  QJsonObject getConfigByName(DataLocation p_location, const QString &p_baseName,
+                              bool *p_ok = nullptr) const;
+
+  // Get configuration by name, deep-merged (RFC 7386) on top of @p_defaults, so a key the
+  // user's file does not carry keeps its default value.
+  //
+  // @p_ok, when given, reports whether the config could actually be READ. An absent file is
+  // NOT a failure (it yields @p_defaults with *p_ok == true). A present-but-unreadable file
+  // sets *p_ok to false and also yields @p_defaults - vxcore deliberately distinguishes the
+  // two, and a caller that persists what it got back MUST check this flag or it will
+  // overwrite a config it merely failed to read.
   QJsonObject getConfigByNameWithDefaults(DataLocation p_location, const QString &p_baseName,
-                                          const QJsonObject &p_defaults) const;
+                                          const QJsonObject &p_defaults,
+                                          bool *p_ok = nullptr) const;
 
   // Update configuration by name. Returns Error for failure cases.
   Error updateConfigByName(DataLocation p_location, const QString &p_baseName,