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

[padak]

What

Addresses finding 1 and finding 3 from my review of #556. Targets ms/dmd-1833, not main — merge this into your branch (or cherry-pick, or close it and do it your way; the analysis is the point, not the patch).

Only touches things my review flagged. No new endpoints, no behavior change beyond the two below.

1. rebase_config: the whole replaced body becomes required

/rebase replaces a configuration rather than patching it, so an omitted key is not "leave unchanged" but "take the server-side default". Your own RFC documents those defaults (lines 109-110, from RebaseRequest::validateDiff): diff.configuration → {}, diff.isDisabled → false.

With both optional, this call is the natural one to write — it passes exactly the fields the signature marks as required:

client.rebase_config(component_id, config_id, branch_id=123, version=7, name="X", rows=[...])

and it sends {"version": 7, "diff": {"name": "X", "rows": [...]}}. The backend fills in the rest, so the rebased config loses its whole parameters block and a disabled config comes back enabled. Merge the MR and that reaches production.

name and rows are required precisely to make that class of loss unrepresentable (D6). This extends the same reasoning to the two fields where the backend substitutes content instead of rejecting the request.

The same applies to description, which the first commit left optional and the second makes required after checking the source (see below). It stays str | None — None is a legitimate resolved value meaning "the config ends up with no description", and it still omits the key rather than sending an explicit null, which costs no expressiveness because the two are indistinguishable server-side. What it no longer has is a default, because that default was the silent-loss path.

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.

D1 in the RFC is updated to say why presence detection is right for update_config (which patches) and wrong here (which replaces), rather than reading as an unexplained inconsistency later.

2. _optional_mr_fields becomes keyword-only

The helper exists so the create and update bodies "cannot drift" — but both call sites passed five arguments positionally and four are str | None. A transposition of auto_merge_strategy and auto_merge_at type-checks cleanly, passes ruff and ty, and surfaces only as a backend 422; the existing coverage asserts autoMergeStrategy for create only. * in the signature closes it the way _billing_get hardcodes its verb — a guarantee no source-scanning test has to catch.

Not included

• Finding 2 (the merge() terminal-error guard) — yours in fix(client): raise on an already-failed Storage job instead of returning it #603, with a wider blast radius than I traced. Nothing here.
• The _do_request write-retry hazard — agreed it is a separate http_base concern.

The open question, now resolved (second commit)

The first commit left description optional and flagged that I could not tell from the RFC whether an absent diff.description preserves the previous description or nulls it. Devin's review flagged the same gap. Rather than leave it to you, I checked the Connection source:

• RebaseRequest::mapValidatedData — description: isset($diff->description) ? (string) $diff->description : null, so a missing key becomes null.
• ConfigurationRebaseService — "The resolved config body ($name/$description/$configuration/$isDisabled) is the complete 3-way diff result and fully replaces version 2's body."

So an omitted description is written as null: the same silent-loss mode, and description belongs in the required set. That is the second commit. The same two sources also confirm the first commit's premise rather than just the RFC's summary of it, and they draw the line for change_description, which is absent from that four-field tuple and documented as null → default rebase message.

Testing

• make check green: 5774 passed, 12 skipped, ty clean (only the 3 pre-existing warnings), all repo gates OK.
• New tests: the replaced body fields always reach the wire; omitting any of them is a TypeError at the call site rather than silent loss on the wire (the test iterates the whole set, so an optional-with-default reintroduced on any one of them fails); every _optional_mr_fields parameter is KEYWORD_ONLY.
• The keyword-only test asserts on inspect.signature rather than making a deliberately-wrong call — a positional call is a static error too, so writing one would mean fighting ty to prove ty is right.
• test_keep_rebase_omits_unset_optionals_and_sends_empty_rows was updated: it can no longer be about is_disabled, so it now covers description / change_description.

[linear-code]

DMD-1833

[devin-ai-integration]

Devin Review found 1 potential issue.

[padak]

Heads-up so you are not waiting on a green tick that will never arrive: no CI runs on this PR. ci.yml is scoped to pull_request: branches: [main], and this one targets ms/dmd-1833, so no workflow is triggered at all — the empty check list is the filter, not a failure.

Verification here is therefore local only: make check green on 5920cc6 (5774 passed, 12 skipped, ty clean apart from the 3 pre-existing warnings, all drift gates OK). Once these commits land on ms/dmd-1833, #556 gets the real CI run against main.