[DMD-1833] Merge requests — Part 1, Layer 3 (client)

[martinsifra]

What

Part 1 of merge-request support in kbagent — Layer 3 only (HTTP client). The design
decisions live in the module and method docstrings; this PR is code-only.

Linear: DMD-1701 — milestone "Branches 2.0".

• client/merge_requests.py (new): the nine MR endpoints as a namespace —
  client.merge_requests.{list,get,conflicts,create,update,request_review,approve,request_changes,merge}.
  The namespace depends on a StorageRequester Protocol (not on the client) via a temporary
  _ClientRequester adapter — the seam the client-split work (RFC: Split KeboolaClient into a transport and resource namespaces #595) later builds under.
  merge() awaits the Storage job implicitly, like every job-backed method in client/,
  with a dedicated 600 s MERGE_JOB_MAX_WAIT budget. _optional_mr_fields (shared by
  create/update so their camelCase mappings cannot drift) is keyword-only: four of its five
  parameters are str | None, so a positional transposition would type-check cleanly and
  surface only as a backend 422.
• client/configs.py: get_config_diff + rebase_config / rebase_config_delete.
  branch_id is required (no production fallback — the endpoints 400 on the default
  branch). Keep vs delete rebase are two methods so no illegal combination is expressible;
  the diff envelope stays private to them. The keep rebase requires the full replaced
  body (name, rows, configuration, is_disabled, description): /rebase replaces
  rather than patches, so an omitted key takes the server-side default — a caller sending
  only name+rows would wipe the configuration body and re-enable a disabled config,
  then merge that into production.
• constants.py: FEATURE_BRANCHES_MERGE_REQUESTS for Part 2's pre-flight feature
  check (Layer 3 deliberately does no feature check; a missing feature is a 403 identical
  to a role denial, so only a Layer 2 pre-flight can word the error).
• tests/test_merge_request_client.py: pins the wire contract — bare vs
  branch-prefixed paths, JSON bodies with real types (the surrounding configs.py teaches
  form encoding — the opposite), presence detection, the diff envelope, the {} delete
  resolution, merge-job waiting and its 600 s budget, the replaced-body requirement,
  include=activityLog, and the stub-requester seam.

What this deliberately does NOT do

• No CLI command, no service — that is Part 2, so no E2E test (convention Close must-have gaps in explorer command #16 binds
  commands), no changelog entry (no version bump), and no plugin/docs surfaces change
  (convention v0.6.0: Branch lifecycle management + security hardening #17 binds commands).
• No SESSION_UNSUPPORTED_FEATURES entry — verified in Connection code: bearer-session
  auth on Storage routes is route-agnostic, the session resolves to the user's real admin
  Storage token before any MR action runs.

Review history (now squashed into the single feat commit)

An adversarial self-review, a Copilot round and follow-up PRs (#606, #608) shaped the
branch before it was squashed; the notable outcomes, in the code above:

• MERGE_JOB_MAX_WAIT (600 s) + max_wait on the StorageRequester Protocol, shaped
  before the Protocol has external consumers.
• The full-replaced-body requirement on rebase_config and the keyword-only
  _optional_mr_fields ([DMD-1833] Require the replaced body fields on rebase; keyword-only MR optional fields #606).
• The self-review also surfaced a poller blind spot: _wait_for_storage_job returned an
  already-terminal error body instead of raising, so a fast synchronous merge failure came
  back as success. This PR initially carried a local guard in merge() for it; the blind
  spot was a house-wide bug across every call site of the helper, so it was fixed centrally
  in fix(client): raise on an already-failed Storage job instead of returning it #603 (DMD-1898, merged first) and the guard was dropped here. What remains is the
  contract statement on StorageRequester.wait_for_storage_job: a Protocol implementation
  must raise on a failed job, initial-body or polled — merge() no longer re-checks.

Known limitation (deliberately not fixed here): _do_request retries POST/PUT on
timeout/5xx house-wide with no opt-out, so a committed-but-lost MR write can replay into a
misleading terminal error (create → 404 "already has an MR", merge → 409 notReadyToMerge,
rebase → 400 "version not newer") masking an operation that actually succeeded. This is a
cross-cutting http_base concern affecting every non-idempotent write in the codebase —
follow-up issue, not a Layer 3 patch.

Testing

• make check green locally.
• Everything Layer 3 is verifiable offline (pytest_httpx); live E2E lands with Part 2's
  commands.

🤖 Generated with Claude Code

[linear-code]

DMD-1833

DMD-1898

[Copilot]

Pull request overview

Adds Layer 3 (HTTP client) support for Storage API merge requests as a new, test-pinned client namespace, plus the related config diff/rebase endpoints needed for conflict resolution. This fits the repo’s 3-layer architecture by expanding the client/ surface area (Layer 3) without introducing any CLI/service behavior yet (Part 2).

Changes:

• Introduces client.merge_requests namespace (9 endpoints) via a StorageRequester Protocol + temporary client adapter seam for the upcoming client-split work.
• Adds branch-scoped config conflict helpers to client/configs.py: get_config_diff, rebase_config, and rebase_config_delete (JSON body + diff-envelope).
• Adds a dedicated merge-job wait budget constant and comprehensive offline contract tests for paths/bodies/envelopes/job-wait behavior.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/keboola_agent_cli/client/merge_requests.py	New merge-requests namespace + requester Protocol seam + cached client property; implements MR endpoints and merge job waiting.
src/keboola_agent_cli/client/configs.py	Adds config diff and rebase endpoints (branch-only, JSON bodies, diff envelope).
src/keboola_agent_cli/client/_client.py	Composes the new _MergeRequestsMixin into KeboolaClient.
src/keboola_agent_cli/constants.py	Adds MERGE_JOB_MAX_WAIT and FEATURE_BRANCHES_MERGE_REQUESTS.
tests/test_merge_request_client.py	New pytest-httpx suite pinning the wire contract and the requester seam behavior.
docs/merge-requests-layer3-rfc.md	RFC documenting backend contract + Layer 3 design decisions and test strategy.
docs/merge-requests-layer2-notes.md	Companion notes capturing verified backend semantics for Part 2 (service/commands).

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[martinsifra]

Fixed in a93225e — reworded to "the Part 2 service layer must call has_feature() with this constant before writes".

[martinsifra]

Fixed in a93225e — the new methods now use config_id, matching the rest of the mixin (RFC method inventory synced too).

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

[martinsifra]

Note for the rebase: the merge() terminal-error guard becomes dead code

The known-limitation guard in merge() is being fixed centrally in a separate PR
(#603, branch ms/wait-for-storage-job), which lands before this one.

Root cause, for the record: _wait_for_storage_job (client/_core.py:208-210) returns an
already-terminal initial job body as-is, including status == "error" — only the
polled body raises (:220-227). That is a duplicated terminal-state check that diverged,
and it affects all 20 call sites of the helper on this branch -- 19 on main
(storage_tables.py 16x, branches.py 2x, workspaces.py 1x) plus merge() here. The sibling pollers wait_for_queue_job
(queue.py:245) and wait_for_query_job (query.py:137) already have the correct shape —
one terminal check inside the loop, no initial-body special case — so the fix converges the
storage poller on that shape rather than patching the divergent branch.

Action item when rebasing this PR onto the fixed main — the local guard is then
redundant and should be dropped:

• client/merge_requests.py:293-302 — the if job.get("status") == "error": guard in merge()
• client/merge_requests.py:36 — from ..errors import ErrorCode, KeboolaApiError (no other use in the file)
• merge() docstring — the "also when the 202 body itself already carries a terminal error…" clause
• tests/test_merge_request_client.py — test_merge_raises_when_202_body_is_already_terminal_error

test_merge_failed_job_raises stays: it covers the polled-error path, which is unaffected.

Unrelated and still open (also noted in the PR description): _do_request retries POST/PUT
with no opt-out, so a committed-but-lost MR write can replay into a misleading terminal
error. Separate http_base concern.

[padak]

Code review — Layer 3 merge-request client

Reviewed the full diff against a local checkout of ms/dmd-1833. Verification I ran in an isolated worktree:

• pytest tests/ → 5770 passed, 166 skipped
• ruff check clean; ty reports only the 3 pre-existing warnings unrelated to this diff
• loc-check, changelog-check, check-sentinel-guards, command-sync-check, skill-check → all OK

Conventions #16 (E2E) and #17 (plugin/docs surfaces) are both command-bound, so a Layer-3-only PR correctly triggers neither. check-sentinel-guards passes because _MergeRequestsMixin descends from _CoreClient, which the script already settles as bearer-capable — no new entry needed.

Overall this is well above the bar: the StorageRequester seam, the "do not copy the neighbouring form-encoded idiom" warnings traced to Connection validators, and tests that pin {} vs null vs [] by dict equality rather than membership are all the right instincts. Three findings below; only the first one I would want resolved before merge.

---

1. 🔴 rebase_config: omitting configuration / is_disabled destroys state rather than preserving it

src/keboola_agent_cli/client/configs.py:688,691

rebase_config applies the house "None means omit the key" idiom, but /rebase is a full replacement, not a partial update. The RFC in this same PR (docs/merge-requests-layer3-rfc.md:109-110, citing RebaseRequest::validateDiff) documents that the backend defaults diff.configuration to {} and diff.isDisabled to false. On a replacement endpoint, "omit" therefore means "reset", not "leave unchanged".

Concrete failure: Part 2 resolves a conflict on a disabled extractor and calls

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

which mirrors the signature's own signal that name and rows are the required content fields. The wire body becomes {"version": 7, "diff": {"name": "X", "rows": [...]}}; the backend fills in configuration={} and isDisabled=false. The rebased dev-branch configuration loses its entire parameters block and is silently re-enabled. Merging the MR then promotes an empty, running configuration into production.

name and rows are required precisely to make that class of loss unrepresentable (RFC, D6) — these two parameters leave the same hole open. D1 does acknowledge "omitting it defaults to false server-side", but the docstring states the consequence as a neutral fact (configuration: Resolved configuration body (backend default: {})), which reads as harmless.

Suggested fix — either is fine:

• make configuration and is_disabled required for a keep rebase (consistent with D6's "no illegal combination expressible"), or
• keep the signature and reword both docstring lines to say omitting resets the field server-side, so no Layer 2 caller has to infer it.

2. 🟡 The already-terminal-job guard is on the wrong layer

src/keboola_agent_cli/client/merge_requests.py:293

The guard added in the second commit is correct, but placing it in merge() leaves the blind spot it fixes unpatched everywhere else. _CoreClient._wait_for_storage_job (client/_core.py:209) early-returns the initial job dict whenever status in ("success", "error") — it only raises on a polled error. Two other call sites pass that return value straight through:

• client/storage_tables.py:658 — import-async, i.e. storage upload-table / storage load-file
• client/storage_tables.py:1132 — the export path

So a Storage job that comes back already failed in the POST response is reported to the user as a successful import/export. Moving the check into _wait_for_storage_job fixes merge and both of those at once, and means the next job-backed method does not have to remember to re-add it.

3. 🟢 _optional_mr_fields is called positionally

src/keboola_agent_cli/client/merge_requests.py:196 and :224

The helper exists explicitly so the create/update bodies "cannot drift" — but both call sites pass its five arguments positionally, and four of them are str | None. The one drift mode the helper cannot catch is exactly the type-correct one: transposing auto_merge_at and auto_merge_strategy in a future edit to one call site passes ruff and ty, and the existing coverage asserts autoMergeStrategy == "scheduled" only for create — so update would ship sending a timestamp as the strategy and fail as a backend 422 at runtime. Passing the five by keyword removes the failure mode entirely.

---

Note on the disclosed limitation

The PR body says the _do_request write-retry hazard (a committed-but-lost POST/PUT replaying into a misleading terminal error — create → 404 "already has an MR", merge → 409 notReadyToMerge, rebase → 400 "version not newer") is a follow-up issue rather than a Layer 3 patch. Agreed on the scoping — the diagnosis is right and it is genuinely cross-cutting. I could not find the follow-up issue in this repo, though; worth filing before it drops off the radar, since MR writes are the first place where a masked-success replay has user-visible consequences.

Things worth calling out as good

• The StorageRequester Protocol + _ClientRequester adapter: the namespace never sees the client, which is what makes test_namespace_works_against_a_stub_requester possible with no HTTP client at all, and makes the #595 transport swap a one-line change.
• MERGE_JOB_MAX_WAIT follows the existing IMPORT_JOB_MAX_WAIT / EXPORT_JOB_MAX_WAIT precedent rather than inventing a new knob.
• Percent-encoding uses safe='', the stricter of the two forms already present in configs.py.
• The docstrings document the version wire-name trap (it is the target branch's version from theirs.version, not the dev branch config's) — that one would have cost someone an afternoon.

[padak]

Withdrawing finding 2 — I missed your note above before posting, sorry. The terminal-error guard placement is already handled centrally in #603, with the same root-cause analysis (initial-body special case in _wait_for_storage_job, converging on the wait_for_queue_job / wait_for_query_job shape) and a broader blast radius than I traced — you found 19 call sites on main, I only named the two in storage_tables.py. Your rebase checklist for dropping the local guard covers it. Nothing to do on this PR.

Findings 1 (rebase_config omitted configuration / is_disabled resetting server-side) and 3 (positional _optional_mr_fields) stand as written.

[padak]

Findings 1 and 3 are implemented in #606, which targets ms/dmd-1833 rather than main — merge it into your branch, cherry-pick it, or close it and do it your way. I did not push into your branch directly, since your rebase checklist above suggests you have work in flight.

make check is green on it (5774 passed, ty clean). One question I deliberately did not answer myself is in the PR body: whether an absent diff.description preserves the previous description or nulls it — if it nulls it, description belongs in the required set for the same reason. You have the Connection source in view.

[padak]

Update on #606: the open question is no longer open. I checked the Connection source instead of leaving it to you.

RebaseRequest::mapValidatedData maps an absent diff.description to null, and ConfigurationRebaseService documents $name / $description / $configuration / $isDisabled as "the complete 3-way diff result" that "fully replaces" the resolved version's body. So omitting the description writes null — the same silent-loss mode as configuration / isDisabled, and description is required in #606 as well (still str | None, so "no description" stays expressible).

Those two sources also draw the line cleanly: change_description is absent from that tuple and documented as null → default rebase message, so it stays optional. The RFC's D1 now cites both rather than just asserting the split.

make check green on the updated branch (5774 passed, ty clean).

[padak]

Approving — re-reviewed at 0b88991b

Both findings from my earlier review are resolved, and finding 2 was yours in #603 all along.

Re-verified on the merged branch rather than assuming the merge carried things across:

• rebase_config now takes configuration: dict[str, Any], is_disabled: bool and description: str | None as required parameters — the whole replaced body. change_description correctly stays optional.
• _optional_mr_fields is keyword-only in the signature.
• The squash carried no Co-Authored-By trailer.
• make check green on 0b88991b: 5774 passed, 12 skipped, ty clean apart from the 3 pre-existing warnings, and every drift gate OK (SKILL.md, command-sync across all 260 commands, changelog, error-codes, sentinel-guards, file-size).

The RFC's D1 now carries the replace-vs-patch reasoning with both Connection sources cited, so the split between required and optional reads as a decision rather than an inconsistency.

One 🟢 nit, and it is mine

docs/merge-requests-layer3-rfc.md:379-380 still says:

rebase_config sends is_disabled=False but omits is_disabled=None

That is stale as of #606 — is_disabled is bool now, so None is not expressible at all. I updated D1 and the signature table and missed the Testing section. Suggested replacement for that bullet:

- **Presence detection** — `create` / `update` omit unset optionals; `rebase_config` always
  sends the replaced body (`name` / `rows` / `configuration` / `isDisabled`) and omits only
  `description=None` and an unset `change_description`; `rows=[]` is sent, not treated as absent.

Not blocking — it is a doc line in an internal RFC, and the code and tests are right. Happy to push it as a one-line PR if you would rather not touch it.

Not in scope here

• The _do_request write-retry hazard stays open, as agreed — a cross-cutting http_base concern.
• No CI ran on #606 (ci.yml is scoped to pull_request: branches: [main]), so this PR's run against main is the first real CI on those two commits.

[padak]

The 🟢 nit from my approval is now #608 against ms/dmd-1833 — docs only, the single stale bullet in the RFC's Testing section. Merging it does not affect the approval here (dismiss_stale_reviews_on_push is off on both rulesets), so #556 stays approved either way.

[padak]

Approval re-confirmed at f994f3b7

The branch was rebased onto main (now carrying #603 and #604) and gained two commits after I approved at 0b88991b. Since dismiss_stale_reviews_on_push is off on both rulesets, that approval stayed green on code it had not seen — so I re-reviewed the delta rather than let it stand on its own.

25a80a5 — dropping merge()'s local guard. Correct, and I checked the premise rather than the commit message: _wait_for_storage_job on the rebased base now evaluates the terminal state in one place inside the loop, so an already-terminal error initial body raises instead of being returned. The removed test is not lost coverage either — test_already_failed_body_raises and test_already_successful_body_returns_without_polling in test_client.py pin exactly that, one layer down where it now belongs. Removing the local guard, the unused errors import, the docstring clause and the dedicated test is the whole checklist, executed.

f994f3b7 — my own doc fix (#608).

I also re-checked the RFC for anything the guard removal left stale: D3 and the "Merge waiting" testing bullet both describe the behaviour via the shared helper, which is now more accurate than before, not less. Nothing to change.

make check green on the rebased branch: 5782 passed, 12 skipped, ty clean, all drift gates OK.

One thing to decide at merge time

Three commits on this branch carry a Co-Authored-By: Claude Fable 5 trailer (4954b33, f302d87, 25a80a5). GitHub's squash default concatenates the commit messages, so those trailers follow onto main unless the merge is given an explicit subject and body. Flagging it because this repo's convention is not to carry them — entirely your call, and nothing to change on the branch itself.