From 862ba99802a52e102d2c92ee46760b76db28b1f9 Mon Sep 17 00:00:00 2001
From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com>
Date: Mon, 23 Feb 2026 12:15:44 +0000
Subject: [PATCH] fix(settings): rollback restores old_value instead of
 new_value and supports deletion fallback

Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com>
---
 app/api/settings.py               |  9 +++++----
 app/utils/settings_service.py     | 17 +++++++++--------
 frontend/templates/audit_log.html |  2 +-
 tests/test_settings_audit_log.py  | 25 ++++++++++++++++++++-----
 4 files changed, 35 insertions(+), 18 deletions(-)

diff --git a/app/api/settings.py b/app/api/settings.py
index 97688f5e..555f8a82 100644
--- a/app/api/settings.py
+++ b/app/api/settings.py
@@ -434,12 +434,13 @@ async def rollback_setting_to_history(
     admin: AdminUser,
 ):
     """
-    Revert a setting to the value it held at a specific point in the audit log.
+    Revert a setting to the value it had *before* a specific audit log change.
 
     The ``history_id`` is the ID of the :class:`~app.models.SettingsAuditLog`
-    entry whose ``new_value`` should be reinstated.  If that entry recorded a
-    deletion (``new_value`` is ``None``), the setting is removed from the
-    database and reverts to its ENV/default value.
+    entry whose ``old_value`` should be reinstated, effectively undoing that
+    change.  If ``old_value`` is ``None`` (the setting did not exist before
+    that change), the setting is removed from the database and reverts to its
+    ENV/default value.
 
     A new audit log entry is written to record the rollback.
     Admin only.
diff --git a/app/utils/settings_service.py b/app/utils/settings_service.py
index b7344118..fb813bbb 100644
--- a/app/utils/settings_service.py
+++ b/app/utils/settings_service.py
@@ -1201,19 +1201,20 @@ def get_setting_history(db: Session, key: str) -> List[Dict[str, Any]]:
 
 def rollback_setting(db: Session, key: str, history_id: int, changed_by: str = "system") -> bool:
     """
-    Revert a setting to the value recorded in a specific audit log entry.
+    Revert a setting to the value it had *before* a specific audit log entry.
 
-    The value stored in the chosen history entry's ``new_value`` field is
-    re-applied as the current database value.  If that value is ``None``
-    (i.e. the entry recorded a deletion) the setting is removed from the
-    database entirely, reverting to ENV/defaults.
+    The value stored in the chosen history entry's ``old_value`` field is
+    re-applied as the current database value, effectively undoing that change.
+    If ``old_value`` is ``None`` (i.e. the setting did not exist before that
+    change) the setting is removed from the database entirely, reverting to
+    ENV/defaults.
 
     A new audit log entry is written to record the rollback operation.
 
     Args:
         db: Database session
         key: Setting key to roll back
-        history_id: ID of the SettingsAuditLog entry whose ``new_value``
+        history_id: ID of the SettingsAuditLog entry whose ``old_value``
                     should become the restored value
         changed_by: Username performing the rollback (for audit log)
 
@@ -1229,10 +1230,10 @@ def rollback_setting(db: Session, key: str, history_id: int, changed_by: str = "
             logger.warning(f"Rollback failed: audit log entry {history_id} not found for key '{key}'")
             return False
 
-        target_value = history_entry.new_value
+        target_value = history_entry.old_value
 
         if target_value is None:
-            # The history entry recorded a deletion – reinstate that by deleting the current db value
+            # The old value was empty – remove the current db value to revert to ENV/default
             return delete_setting_from_db(db, key, changed_by=changed_by)
         else:
             return save_setting_to_db(db, key, target_value, changed_by=changed_by)
diff --git a/frontend/templates/audit_log.html b/frontend/templates/audit_log.html
index fa16bac6..067cc0b7 100644
--- a/frontend/templates/audit_log.html
+++ b/frontend/templates/audit_log.html
@@ -73,7 +73,7 @@
           <td class="px-4 py-3 text-sm">
             <button
               type="button"
-              @click="rollback('{{ entry.key }}', {{ entry.id }}, '{{ entry.new_value or '' }}')"
+              @click="rollback('{{ entry.key }}', {{ entry.id }}, '{{ entry.old_value or '' }}')"
               :disabled="rollingBack === {{ entry.id }}"
               class="inline-flex items-center px-2 py-1 text-xs font-medium rounded border border-gray-300 text-gray-700 bg-white hover:bg-yellow-50 hover:border-yellow-400 focus:outline-none focus:ring-2 focus:ring-offset-1 focus:ring-yellow-400 disabled:opacity-50 disabled:cursor-not-allowed"
               title="Revert '{{ entry.key }}' to the value in this log entry"
diff --git a/tests/test_settings_audit_log.py b/tests/test_settings_audit_log.py
index 79021423..e0ff9ed9 100644
--- a/tests/test_settings_audit_log.py
+++ b/tests/test_settings_audit_log.py
@@ -203,19 +203,34 @@ class TestRollbackSetting:
     """rollback_setting reinstates the value from a given audit log entry."""
 
     def test_rollback_to_previous_value(self, db_session):
+        """Rolling back an entry restores the old_value (the value *before* that change)."""
         from app.utils.settings_service import get_setting_from_db, rollback_setting, save_setting_to_db
 
-        save_setting_to_db(db_session, "workdir", "/v1", changed_by="admin")  # entry id 1
-        save_setting_to_db(db_session, "workdir", "/v2", changed_by="admin")  # entry id 2
+        save_setting_to_db(db_session, "workdir", "/v1", changed_by="admin")  # entry 1: old=None, new=/v1
+        save_setting_to_db(db_session, "workdir", "/v2", changed_by="admin")  # entry 2: old=/v1, new=/v2
 
-        first_entry = db_session.query(SettingsAuditLog).filter_by(key="workdir").first()
-        # first entry has new_value="/v1"
-        success = rollback_setting(db_session, "workdir", first_entry.id, changed_by="rollbacker")
+        # Rolling back entry 2 should undo the /v1→/v2 change and restore /v1
+        second_entry = db_session.query(SettingsAuditLog).filter_by(key="workdir").order_by(SettingsAuditLog.id.desc()).first()
+        success = rollback_setting(db_session, "workdir", second_entry.id, changed_by="rollbacker")
 
         assert success is True
         current = get_setting_from_db(db_session, "workdir")
         assert current == "/v1"
 
+    def test_rollback_deletes_setting_when_old_value_is_none(self, db_session):
+        """Rolling back the first-ever entry (old_value=None) deletes the setting from DB."""
+        from app.utils.settings_service import get_setting_from_db, rollback_setting, save_setting_to_db
+
+        save_setting_to_db(db_session, "workdir", "/v1", changed_by="admin")  # old=None, new=/v1
+
+        first_entry = db_session.query(SettingsAuditLog).filter_by(key="workdir").first()
+        success = rollback_setting(db_session, "workdir", first_entry.id, changed_by="rollbacker")
+
+        assert success is True
+        # Setting should be removed from DB (fallback to ENV/default)
+        current = get_setting_from_db(db_session, "workdir")
+        assert current is None
+
     def test_rollback_creates_new_audit_entry(self, db_session):
         from app.utils.settings_service import rollback_setting, save_setting_to_db