[DMD-1833] Require the replaced body fields on rebase; keyword-only MR optional fields

docs/merge-requests-layer3-rfc.md

@@ -135,9 +135,31 @@ per source branch, ever; both guards are **404**, not 400 (`MergeRequestCreateAc
 
 **D1 — Explicit typed parameters, not payload dicts.** House style is named parameters with
 presence detection inside the method — see `update_config` (`client/configs.py:352`, *"Only
-provided (non-None) fields are sent"*). `is_disabled: bool | None` stays tri-state for
-consistency with `update_config` / `update_config_row`, not because the API forces it (inside a
-non-empty `diff` envelope, omitting it defaults to `false` server-side).
+provided (non-None) fields are sent"*).
+
+Presence detection is the right idiom for `update_config`, which **patches**, and the wrong one
+for `rebase_config`, which **replaces**: there, an omitted key is not "leave unchanged" but
+"take the server-side default". `diff.configuration` defaults to `{}` and `diff.isDisabled` to
+`false`, and an absent `diff.description` to null (`RebaseRequest::mapValidatedData`). So a
+tri-state `is_disabled: bool | None` and optional `configuration` / `description` would make
+silent data loss the signature's default — a caller resolving a conflict on a disabled config
+and passing only `name` / `rows` would wipe the configuration body, drop the description and
+re-enable the config, then merge that into production.
+
+`ConfigurationRebaseService` settles which fields that covers: `$name` / `$description` /
+`$configuration` / `$isDisabled` are *"the complete 3-way diff result"* and *"fully replace"* the
+resolved version's body. All four are therefore **required** parameters, alongside `rows` (which
+the backend rejects the request without). `description` is required-but-nullable — `None` is a
+legitimate resolved value meaning "no description", and it omits the key rather than sending an
+explicit null, which costs no expressiveness because the two are indistinguishable server-side.
+
+`change_description` is the one genuine optional: it is not part of the replaced body, and null
+selects a default rebase message rather than clearing anything.
+
+The same reasoning applies to `_optional_mr_fields` in `client/merge_requests.py`, where
+presence detection *is* correct (create and update genuinely patch): the helper is keyword-only,
+because four of its five parameters are `str | None` and a positional transposition would
+type-check cleanly and surface only as a backend 422.
 
 **D2 — JSON bodies throughout, deviating from `configs.py`.** Per *Request bodies are JSON*:
 `json=`, real nested objects, no `json.dumps`, no `"1"` / `"0"` booleans. Every method gets a
@@ -329,16 +351,18 @@ Branch-scoped; `branch_id` required (D5).
 | Method | Endpoint |
 |---|---|
 | `get_config_diff(component_id, config_id, branch_id) -> dict` | `GET …/branch/{branch_id}/components/{c}/configs/{cfg}/diff` |
-| `rebase_config(component_id, config_id, branch_id, version, name, rows, configuration=None, description=None, change_description=None, is_disabled=None) -> dict` | `POST …/rebase` (keep) |
+| `rebase_config(component_id, config_id, branch_id, version, name, rows, configuration, is_disabled, description, change_description=None) -> dict` | `POST …/rebase` (keep) |
 | `rebase_config_delete(component_id, config_id, branch_id, version) -> dict` | `POST …/rebase` (delete) |
 
 `get_config_diff` returns the three-way diff (`base` = dev branch v1, `ours` = dev head,
 `theirs` = default head); each side may be null when the config does not exist there.
 Flattening the nested `diff` payload is Layer 2's job.
 
 The Python signatures are flat; only the body construction knows about the envelope.
-`rebase_config` sends `version` at the top level and puts `name` and `rows` (always) plus any
-non-`None` optional inside `diff`; `is_disabled=None` is omitted, `is_disabled=False` is sent.
+`rebase_config` sends `version` at the top level and puts the five required content fields
+(`name`, `rows`, `configuration`, `is_disabled`, `description`) plus `change_description` when
+set inside `diff`. `description=None` is required-but-nullable: it omits the key, which is how
+"the resolved config has no description" is expressed.
 `rebase_config_delete` sends exactly `{"version": N, "diff": {}}`. Component and configuration
 ids are `quote()`d, as everywhere in `configs.py`.
 

src/keboola_agent_cli/client/configs.py

@@ -685,10 +685,10 @@ def rebase_config(
         version: int,
         name: str,
         rows: list[dict[str, Any]],
-        configuration: dict[str, Any] | None = None,
-        description: str | None = None,
+        configuration: dict[str, Any],
+        is_disabled: bool,
+        description: str | None,
         change_description: str | None = None,
-        is_disabled: bool | None = None,
     ) -> dict[str, Any]:
         """Rebase a dev-branch configuration onto a newer default-branch version.
 
@@ -698,11 +698,21 @@ def rebase_config(
 
         The resolved content travels in a ``diff`` envelope mirroring the
         shape ``get_config_diff`` returns each side in, so a resolved diff
-        side posts back nearly 1:1. ``name`` and ``rows`` are required by the
-        backend for a keep rebase (``rows=[]`` legitimately deletes all
-        rows); to resolve a conflict by DELETING the config, use
-        ``rebase_config_delete`` -- the two rebase kinds are separate methods
-        on purpose, so no illegal combination is expressible (RFC, D6).
+        side posts back nearly 1:1. Rebase REPLACES the configuration rather
+        than patching it: server-side, ``name`` / ``description`` /
+        ``configuration`` / ``isDisabled`` are "the complete 3-way diff
+        result" and "fully replace" the resolved version's body
+        (``ConfigurationRebaseService``). So all four are required here, not
+        optional -- an omitted key is not "leave unchanged" but "take the
+        server-side default", which for ``configuration`` is ``{}``, for
+        ``isDisabled`` is ``false`` and for ``description`` is null. ``rows``
+        is required too, because the backend rejects a keep rebase without
+        it (``rows=[]`` legitimately deletes all rows). Only
+        ``change_description`` is genuinely optional: it is not part of the
+        replaced body, and null selects a default rebase message. To resolve
+        a conflict by DELETING the config, use ``rebase_config_delete`` --
+        the two rebase kinds are separate methods on purpose, so no illegal
+        combination is expressible (RFC, D6).
 
         ``branch_id`` is required with no production fallback (see
         ``get_config_diff``); the endpoint also requires the
@@ -722,28 +732,35 @@ def rebase_config(
                 (``{id?, name?, description?, isDisabled?, configuration?}``);
                 missing/null ``id`` means a new row, duplicates are rejected,
                 array order becomes sort order.
-            configuration: Resolved configuration body (backend default: {}).
-            description: Resolved description. ``None`` omits the key --
-                which loses nothing: server-side an explicit JSON null and
-                an absent key are indistinguishable (``isset`` mapping).
+            configuration: Resolved configuration body. Required: on a
+                missing key the backend substitutes ``{}``, wiping it.
+            is_disabled: Resolved disabled flag. Required: on a missing key
+                the backend substitutes ``false``, re-enabling a config that
+                was disabled. Not the tri-state of ``update_config``
+                (RFC, D1) -- that method patches, this one replaces.
+            description: Resolved description. Required, but ``None`` is a
+                legitimate resolved value -- it means the rebased config
+                ends up with no description. ``None`` omits the key rather
+                than sending an explicit null, which costs no expressiveness
+                because server-side the two are indistinguishable (``isset``
+                mapping). It has no default precisely because that default
+                would silently drop an existing description.
             change_description: Change log message; when ``None``/omitted
                 the backend uses a default rebase message.
-            is_disabled: When None, omitted (backend defaults to False);
-                False is sent explicitly -- tri-state for consistency with
-                ``update_config`` (RFC, D1).
 
         Returns:
             The rebased configuration dict.
         """
-        diff: dict[str, Any] = {"name": name, "rows": rows}
-        if configuration is not None:
-            diff["configuration"] = configuration
+        diff: dict[str, Any] = {
+            "name": name,
+            "rows": rows,
+            "configuration": configuration,
+            "isDisabled": is_disabled,
+        }
         if description is not None:
             diff["description"] = description
         if change_description is not None:
             diff["changeDescription"] = change_description
-        if is_disabled is not None:
-            diff["isDisabled"] = is_disabled
         return self._rebase_request(component_id, config_id, branch_id, version, diff)
 
     def rebase_config_delete(

src/keboola_agent_cli/client/merge_requests.py

@@ -79,6 +79,7 @@ def wait_for_storage_job(
 
 
 def _optional_mr_fields(
+    *,
     description: str | None,
     reviewer_ids: _IntList | None,
     auto_merge_strategy: str | None,
@@ -88,7 +89,10 @@ def _optional_mr_fields(
     """Build the optional-field part of a create/update body.
 
     Shared so the two bodies cannot drift: only provided (non-None) fields
-    are included, under their wire (camelCase) names.
+    are included, under their wire (camelCase) names. Keyword-only by
+    signature: four of the five parameters are ``str | None``, so a
+    positional transposition would type-check cleanly and only surface as a
+    backend 422 -- the one drift mode this helper otherwise cannot catch.
     """
     body: dict[str, Any] = {}
     if description is not None:
@@ -194,7 +198,11 @@ def create(
         }
         body.update(
             _optional_mr_fields(
-                description, reviewer_ids, auto_merge_strategy, auto_merge_at, external_id
+                description=description,
+                reviewer_ids=reviewer_ids,
+                auto_merge_strategy=auto_merge_strategy,
+                auto_merge_at=auto_merge_at,
+                external_id=external_id,
             )
         )
         return self._requester.request("POST", _BASE, json=body).json()
@@ -222,7 +230,11 @@ def update(
         PUTs ``{}``, which the backend treats as a no-op returning the MR.
         """
         body = _optional_mr_fields(
-            description, reviewer_ids, auto_merge_strategy, auto_merge_at, external_id
+            description=description,
+            reviewer_ids=reviewer_ids,
+            auto_merge_strategy=auto_merge_strategy,
+            auto_merge_at=auto_merge_at,
+            external_id=external_id,
         )
         if title is not None:
             body["title"] = title

tests/test_merge_request_client.py

@@ -8,14 +8,15 @@
 the namespace-never-touches-the-client seam.
 """
 
+import inspect
 import json
 from typing import Any
 
 import httpx
 import pytest
 
 from keboola_agent_cli.client import KeboolaClient
-from keboola_agent_cli.client.merge_requests import MergeRequests
+from keboola_agent_cli.client.merge_requests import MergeRequests, _optional_mr_fields
 from keboola_agent_cli.errors import ErrorCode, KeboolaApiError
 
 STACK_URL = "https://connection.keboola.com"
@@ -325,9 +326,9 @@ def test_keep_rebase_builds_the_envelope(self, client, httpx_mock) -> None:
             name="My config",
             rows=[{"id": "r1", "name": "Row 1"}],
             configuration={"parameters": {"baseUrl": "https://example.com"}},
+            is_disabled=False,
             description="resolved",
             change_description="rebase onto v7",
-            is_disabled=False,
         )
 
         body = _sent_json(httpx_mock.get_requests()[0])
@@ -346,15 +347,78 @@ def test_keep_rebase_builds_the_envelope(self, client, httpx_mock) -> None:
     def test_keep_rebase_omits_unset_optionals_and_sends_empty_rows(
         self, client, httpx_mock
     ) -> None:
-        """is_disabled=None is omitted; rows=[] is sent (it deletes all rows)."""
+        """description=None and an unset change_description are omitted; rows=[] is sent."""
         httpx_mock.add_response(url=self.REBASE_URL, json={})
 
         client.rebase_config(
-            "keboola.ex-http", "cfg-1", branch_id=123, version=7, name="My config", rows=[]
+            "keboola.ex-http",
+            "cfg-1",
+            branch_id=123,
+            version=7,
+            name="My config",
+            rows=[],
+            configuration={},
+            is_disabled=False,
+            description=None,
         )
 
         body = _sent_json(httpx_mock.get_requests()[0])
-        assert body["diff"] == {"name": "My config", "rows": []}
+        assert body["diff"] == {
+            "name": "My config",
+            "rows": [],
+            "configuration": {},
+            "isDisabled": False,
+        }
+
+    def test_keep_rebase_always_sends_configuration_and_is_disabled(
+        self, client, httpx_mock
+    ) -> None:
+        """Rebase REPLACES, so no body field may be left to a server-side default.
+
+        A missing ``diff.configuration`` is substituted with ``{}`` and a missing
+        ``diff.isDisabled`` with ``false``, so omitting either would wipe the
+        configuration body and re-enable a disabled config. Both are required
+        parameters; this pins that they always reach the wire.
+        """
+        httpx_mock.add_response(url=self.REBASE_URL, json={})
+
+        client.rebase_config(
+            "keboola.ex-http",
+            "cfg-1",
+            branch_id=123,
+            version=7,
+            name="My config",
+            rows=[],
+            configuration={"parameters": {"keep": "me"}},
+            is_disabled=True,
+            description="kept",
+        )
+
+        diff = _sent_json(httpx_mock.get_requests()[0])["diff"]
+        assert diff["configuration"] == {"parameters": {"keep": "me"}}
+        assert diff["isDisabled"] is True
+        assert diff["description"] == "kept"
+
+    def test_keep_rebase_requires_every_replaced_body_field(self, client) -> None:
+        """Omitting any replaced body field is a TypeError, not silent loss on the wire.
+
+        ``name`` / ``description`` / ``configuration`` / ``isDisabled`` are the
+        tuple the backend calls "the complete 3-way diff result" and fully
+        replaces the resolved version's body with, so none of them may carry a
+        default here. ``change_description`` is not part of that tuple and stays
+        optional -- omitting it selects a default rebase message.
+        """
+        required = {
+            "name": "N",
+            "rows": [],
+            "configuration": {},
+            "is_disabled": False,
+            "description": None,
+        }
+        for omitted in required:
+            kwargs = {k: v for k, v in required.items() if k != omitted}
+            with pytest.raises(TypeError):
+                client.rebase_config("keboola.ex-http", "cfg-1", branch_id=123, version=7, **kwargs)
 
     def test_delete_rebase_sends_empty_diff_object(self, client, httpx_mock) -> None:
         """Delete resolution is exactly {"version": N, "diff": {}} -- diff a JSON object."""
@@ -389,6 +453,40 @@ def wait_for_storage_job(
         assert namespace.list() == [SAMPLE_MR]
         assert calls == [("GET", "/v2/storage/merge-request")]
 
+
+class TestOptionalFieldHelper:
+    """_optional_mr_fields is keyword-only, so create/update cannot transpose."""
+
+    def test_every_field_is_keyword_only(self) -> None:
+        """No parameter may be positional -- a transposition would type-check cleanly.
+
+        Asserted on the signature rather than by making a deliberately wrong
+        call: a positional call is a static error too (which is the point), so
+        writing one would just mean fighting ``ty`` to prove ``ty`` is right.
+        """
+        kinds = {
+            name: param.kind
+            for name, param in inspect.signature(_optional_mr_fields).parameters.items()
+        }
+        assert set(kinds) == {
+            "description",
+            "reviewer_ids",
+            "auto_merge_strategy",
+            "auto_merge_at",
+            "external_id",
+        }
+        assert all(kind is inspect.Parameter.KEYWORD_ONLY for kind in kinds.values()), kinds
+
+    def test_helper_omits_unset_fields(self) -> None:
+        """Only non-None fields are included, under their camelCase wire names."""
+        assert _optional_mr_fields(
+            description=None,
+            reviewer_ids=[7],
+            auto_merge_strategy=None,
+            auto_merge_at=None,
+            external_id="DMD-1701",
+        ) == {"reviewerIds": [7], "externalId": "DMD-1701"}
+
     def test_namespace_is_cached_on_the_client(self) -> None:
         """client.merge_requests returns the same namespace instance every time."""
         client = KeboolaClient(stack_url=STACK_URL, token=TOKEN)