bug(cli): server_connect writes three config values non-atomically — partial update on failure

Make server_connect config persistence all-or-nothing by snapshotting ~/.cleveragents/config.toml before the three set_value calls and restoring the exact prior bytes on any exception. This preserves existing config-change audit events on success while preventing partially written server URL/namespace/TLS state after mid-sequence failures.\n\nAlso promote the #993 TDD feature from expected-fail to active regression coverage and tighten a flaky resource DAG Robot scenario by using a shared session with explicit commits to keep integration gates stable under parallel execution.

ISSUES CLOSED: #993

features/steps/tdd_server_connect_atomic_writes_steps.py

@@ -7,8 +7,7 @@ If a middle call fails (e.g. disk full, permissions error), the earlier values
 are already persisted while the later ones retain their old values, leaving the
 configuration in a half-written state.
 
-The test uses the ``@tdd_expected_fail`` tag until the fix in #993 is merged.
-See CONTRIBUTING.md > Bug Fix Workflow > TDD Issue Test Tags.
+These scenarios now run as normal regression coverage for #993.
 """
 
 from __future__ import annotations
@@ -25,6 +24,8 @@ from behave import given, then, when
 from behave.runner import Context
 
 from cleveragents.application.services.config_service import ConfigService
+from cleveragents.infrastructure.events.reactive import ReactiveEventBus
+from cleveragents.infrastructure.events.types import EventType
 
 # ---------------------------------------------------------------------------
 # Helpers
@@ -96,6 +97,7 @@ def step_fresh_config_dir(context: Context) -> None:
     context.atomic_config_dir = Path(context.atomic_test_home) / ".cleveragents"
     context.atomic_config_dir.mkdir(parents=True, exist_ok=True)
     context.atomic_config_path = context.atomic_config_dir / "config.toml"
+    context.atomic_event_bus = ReactiveEventBus()
     context.atomic_old_home = os.environ.get("HOME")
     os.environ["HOME"] = context.atomic_test_home
 
@@ -162,6 +164,7 @@ def step_invoke_with_second_call_failure(context: Context) -> None:
         config_dir=context.atomic_config_dir,
         config_path=context.atomic_config_path,
         fail_on_call_number=2,
+        event_bus=context.atomic_event_bus,
     )
 
     with patch(
@@ -197,6 +200,7 @@ def step_invoke_with_third_call_failure(context: Context) -> None:
         config_dir=context.atomic_config_dir,
         config_path=context.atomic_config_path,
         fail_on_call_number=3,
+        event_bus=context.atomic_event_bus,
     )
 
     with patch(
@@ -313,3 +317,33 @@ def step_check_no_url(context: Context) -> None:
         f"The config should be empty (all-or-nothing atomicity) but a "
         f"partial write occurred."
     )
+
+
+@then("the config event trail should include a compensating rollback for server.url")
+def step_check_compensating_event(context: Context) -> None:
+    """Assert rollback emits a compensating CONFIG_CHANGED event for server.url."""
+    config_events = [
+        event
+        for event in context.atomic_event_bus.audit_log
+        if event.event_type == EventType.CONFIG_CHANGED
+        and event.details.get("key") == "server.url"
+    ]
+
+    assert len(config_events) == 2, (
+        "Expected two server.url CONFIG_CHANGED events (forward write + "
+        f"compensating rollback), got {len(config_events)}."
+    )
+
+    forward_event = config_events[0]
+    rollback_event = config_events[1]
+
+    assert forward_event.details.get("old_value") == _ORIGINAL_URL
+    assert forward_event.details.get("new_value") == _NEW_URL
+
+    assert rollback_event.details.get("old_value") == _NEW_URL
+    assert rollback_event.details.get("new_value") == _ORIGINAL_URL
+    assert rollback_event.details.get("compensating") is True
+    assert (
+        rollback_event.details.get("reason")
+        == "rollback_after_partial_server_connect_failure"
+    )

features/tdd_server_connect_atomic_writes.feature

@@ -1,4 +1,4 @@
-@tdd_expected_fail @tdd_issue @tdd_issue_993
+@tdd_issue @tdd_issue_993
 Feature: TDD Issue #993 — server_connect writes three config values non-atomically
   As a developer
   I want to verify that server_connect writes all three config values atomically
@@ -14,9 +14,7 @@ Feature: TDD Issue #993 — server_connect writes three config values non-atomic
   # Expected behavior: all three config values are written atomically — either
   # all succeed or all fail (rollback to original state).
   #
-  # This test uses the @tdd_expected_fail tag until the fix in #993 is merged.
-  # The tag inverts the result so CI passes while the bug is still unfixed.
-  # See CONTRIBUTING.md > Bug Fix Workflow > TDD Issue Test Tags.
+  # This scenario now runs as a normal regression test after the fix for #993.
 
   Scenario: Config remains unchanged when second set_value fails during server_connect
     Given a fresh config directory for atomic write test
@@ -26,6 +24,7 @@ Feature: TDD Issue #993 — server_connect writes three config values non-atomic
     And the config should have rolled back server.url to its original value
     And the config should still have the original server.namespace
     And the config should still have the original server.tls-verify
+    And the config event trail should include a compensating rollback for server.url
 
   Scenario: Config remains unchanged when third set_value fails during server_connect
     Given a fresh config directory for atomic write test

src/cleveragents/application/services/config_service.py

@@ -1274,6 +1274,7 @@ class ConfigService:
         with open(self._config_path, "w") as fh:
             tomlkit.dump(doc, fh)
 
+<<<<<<< HEAD
     def write_scoped_config(self, data: dict[str, Any], scope: ConfigScope) -> None:
         """Write *data* to the config file for the given *scope*.
 
@@ -1364,6 +1365,56 @@ class ConfigService:
                 )
             except Exception:
                 _logger.warning("audit_emit_failed", event_type="CONFIG_CHANGED")
+=======
+    def emit_config_changed(
+        self,
+        *,
+        key: str,
+        old_value: Any,
+        new_value: Any,
+        compensating: bool = False,
+        reason: str | None = None,
+    ) -> None:
+        """Emit a ``CONFIG_CHANGED`` domain event without mutating config state.
+
+        This helper is used by :meth:`set_value` and rollback flows that need
+        compensating audit events after restoring a prior config snapshot.
+        """
+        emitted_old = old_value
+        emitted_new = new_value
+        if is_sensitive_key(key):
+            emitted_old = REDACTED
+            emitted_new = REDACTED
+
+        details: dict[str, Any] = {
+            "key": key,
+            "old_value": emitted_old,
+            "new_value": emitted_new,
+        }
+        if compensating:
+            details["compensating"] = True
+        if reason is not None:
+            details["reason"] = reason
+
+        if self._event_bus is not None:
+            try:
+                self._event_bus.emit(
+                    DomainEvent(
+                        event_type=EventType.CONFIG_CHANGED,
+                        details=details,
+                    )
+                )
+            except Exception:
+                _logger.warning("audit_emit_failed", event_type="CONFIG_CHANGED")
+
+    def set_value(self, key: str, value: Any) -> None:
+        """Persist a single key-value pair into the global TOML config."""
+        data = self.read_config()
+        old_value = data.get(key)
+        data[key] = value
+        self.write_config(data)
+        self.emit_config_changed(key=key, old_value=old_value, new_value=value)
+>>>>>>> 62a30ba3 (bug(cli): server_connect writes three config values non-atomically — partial update on failure)
 
     # -- validation -----------------------------------------------------------
 

src/cleveragents/cli/commands/server.py

@@ -74,6 +74,35 @@ def _get_config_service() -> ConfigService:
     )
 
 
+def _snapshot_config_file(config_path: Path) -> bytes | None:
+    """Capture the current config file bytes, if present."""
+    if not config_path.exists():
+        return None
+    return config_path.read_bytes()
+
+
+def _restore_config_file(config_path: Path, snapshot: bytes | None) -> None:
+    """Restore config.toml to a previously captured snapshot."""
+    if snapshot is None:
+        if config_path.exists():
+            config_path.unlink()
+        return
+
+    config_path.parent.mkdir(parents=True, exist_ok=True)
+    config_path.write_bytes(snapshot)
+
+
+def _server_connect_updates(
+    config: ServerConnectionConfig,
+) -> tuple[tuple[str, Any], ...]:
+    """Return ordered key updates performed by ``server_connect``."""
+    return (
+        ("server.url", config.server_url),
+        ("server.namespace", config.namespace),
+        ("server.tls-verify", config.tls_verify),
+    )
+
+
 def resolve_server_mode() -> str:
     """Determine the current server mode.
 
@@ -140,14 +169,32 @@ def server_connect(
         console.print(f"[red]Invalid server configuration:[/red] {exc}")
         raise typer.Exit(code=1) from exc
 
-    # Persist to config file via set_value() so that each change emits
-    # a CONFIG_CHANGED audit event.  Using write_config() would bypass
-    # the EventBus and leave no audit trail for these security-relevant
-    # settings (server URL, namespace, TLS verification).
+    # Persist via set_value() so each key produces a CONFIG_CHANGED audit
+    # event. If any write fails, restore the original snapshot and emit
+    # compensating CONFIG_CHANGED events so the audit trail reflects the
+    # rollback (no stale partial-change trail).
     svc = _get_config_service()
-    svc.set_value("server.url", config.server_url)
-    svc.set_value("server.namespace", config.namespace)
-    svc.set_value("server.tls-verify", config.tls_verify)
+    config_path = Path.home() / ".cleveragents" / "config.toml"
+    config_snapshot = _snapshot_config_file(config_path)
+    update_items = _server_connect_updates(config)
+    existing_config = svc.read_config()
+    original_values = {key: existing_config.get(key) for key, _ in update_items}
+    applied_updates: list[tuple[str, Any]] = []
+    try:
+        for key, new_value in update_items:
+            svc.set_value(key, new_value)
+            applied_updates.append((key, new_value))
+    except Exception:
+        _restore_config_file(config_path, config_snapshot)
+        for key, attempted_value in reversed(applied_updates):
+            svc.emit_config_changed(
+                key=key,
+                old_value=attempted_value,
+                new_value=original_values.get(key),
+                compensating=True,
+                reason="rollback_after_partial_server_connect_failure",
+            )
+        raise
 
     result: dict[str, Any] = {
         "server_url": config.server_url,