From 89b4087cba23d13d70aa8d39d204763ebea7b9b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C5=A0ifra?= Date: Wed, 5 Aug 2026 08:33:24 +0200 Subject: [PATCH] TEMP RFC --- docs/merge-requests-rfc.md | 561 +++++++++++++++++++++++++++++++++++++ 1 file changed, 561 insertions(+) create mode 100644 docs/merge-requests-rfc.md diff --git a/docs/merge-requests-rfc.md b/docs/merge-requests-rfc.md new file mode 100644 index 00000000..f5c40687 --- /dev/null +++ b/docs/merge-requests-rfc.md @@ -0,0 +1,561 @@ +# RFC: Merge requests in kbagent — Part 1, Layer 3 (HTTP client) + +Linear: [DMD-1701](https://linear.app/keboola/issue/DMD-1701) — milestone +["Branches 2.0"](https://linear.app/keboola/project/finalize-dev-branches-327e7756d2fd/overview). + +Every backend claim in this document was verified directly against `keboola/connection` and is +cited to a file and line, so this RFC stands on its own. There is a parallel effort to add the +same capability to `keboola-mcp-server`; the only place it is referenced here is the parity +non-goal at the end, where the cross-repo dependency is real. + +The `/rebase` request body is specified per +[connection#8040](https://github.com/keboola/connection/pull/8040) (DMD-1890), which wrapped the +resolved content in a `diff` envelope; this RFC assumes that shape is deployed everywhere and +targets nothing older. + +## Problem + +Keboola's "Branches 2.0" promotes work from a development branch to production through a +**merge request** (MR): create → request review → (approve) → merge, with a per-configuration +rebase when production has moved on. The Connection backend +(`FEATURE_BRANCHES_MERGE_REQUESTS`) and the UI implement the full non-SOX flow. + +kbagent can create, use, reset and delete dev branches (`kbagent branch …`), but has no way to +promote one. Today `kbagent branch merge` is an escape hatch that calls no API at all: it +composes a URL to the Keboola UI, resets the active branch, and tells the user to finish by +hand (`services/branch_service.py:309`). Any agent or script driving kbagent therefore has to +stop at exactly the moment the work is ready to ship. + +This RFC covers **Layer 3 only** — the HTTP client methods over the Connection MR endpoints. +Layer 2 (service, status derivation) and Layer 1 (commands, output) are Part 2; this document +states just enough about them to justify the client's shape. Scope is the **non-SOX** flow +(`branches-merge-requests`); SOX (`protected-default-branch`) is out of scope. + +## Verified backend contract + +### Paths and status codes + +Project-level — `isAvailableInBranch: false, isAvailableWithoutBranch: true`, so **never** +branch-prefixed. Paths from `openapi/storage.json`; flags from each action's `#[StorageRoute]`. + +| Method / path | Body | Success | Notable failures | +|---|---|---|---| +| `GET /v2/storage/merge-request` | — | 200 | — | +| `POST /v2/storage/merge-request` | JSON | **201** | 404 invalid branch, 422 invalid reviewer, 403 | +| `GET /v2/storage/merge-request/{id}` | — | 200 | 404 | +| `PUT /v2/storage/merge-request/{id}` | JSON | 200 | 403, 404, 422 | +| `PUT /v2/storage/merge-request/{id}/request-review` | — | 200 | 403, 404, 422 | +| `PUT /v2/storage/merge-request/{id}/approve` | — | 200 | 403, 404, 422 | +| `PUT /v2/storage/merge-request/{id}/request-changes` | JSON `{reason?}` | 200 | 403, 404, 422 | +| `PUT /v2/storage/merge-request/{id}/merge` | — | **202** + a Job | **409** (four causes, below), 403, 404 | +| `GET /v2/storage/merge-request/{id}/conflicts` | — | 200 | 404 | + +Branch-scoped — `isAvailableInBranch: true, isAvailableWithoutBranch: false`, so **always** +branch-prefixed: + +| Method / path | Body | Success | Notable failures | +|---|---|---|---| +| `GET …/branch/{branch}/components/{c}/configs/{cfg}/diff` | — | 200 | 400 on default branch, 404 if absent in both branches | +| `POST …/branch/{branch}/components/{c}/configs/{cfg}/rebase` | JSON | **200 + the rebased configuration** | 400 default branch / target version not newer / missing-or-malformed `diff`, 403, 404 | + +`GET /merge-request` declares **no query parameters**, so a `state` filter is necessarily +client-side. `GET /merge-request/{id}` takes `include` (→ `include=activityLog`). + +Rebase returns the configuration (`ConfigurationRebaseAction.php:91-94`, +`toApiResponse(['configuration','state','rows'])`), **not** a job, so it needs no waiting. Only +`merge` is asynchronous. + +### Request bodies are JSON — form encoding does not work + +Four endpoints take a body: create (`MergeRequestCreateAction.php:79`), update +(`MergeRequestUpdateAction.php:89`), request-changes (`MergeRequestRejectAction.php:91`) and +rebase (`ConfigurationRebaseAction.php:67`). Approve, request-review and merge take none — their +`__invoke` signatures are `(Model_Row_AccessToken $token, int $id)`. + +Each body-taking action carries `#[MapRequestBody]`, whose own docblock reads *"Will map request +form data but in case Content-type is application/json header is send will map json body"* +(`Core/Symfony/RequestObject/MapRequestBody.php:13-14`). Form encoding is therefore *accepted* — +and would nonetheless fail: + +- `FormDataExtractor` returns `$request->request->all()` with **no type coercion** + (`PayloadExtractor/FormDataExtractor.php:22`), so form-encoded values stay strings. +- The validators require real integers: `branchFromId` / `branchIntoId` are `Assert\Type('int')` + (`MergeRequestCreateRequest.php:78-83`), rebase's `version` is `Assert\Type('integer')`, and + `reviewerIds` is `Assert\Type('array')` of positive integers. + +**So: always `json=`.** This is the opposite of what the neighbouring configuration endpoints in +`client/configs.py` do — `update_config` and `create_config_row` post form data with nested +values `json.dumps`'d and booleans as `"1"` / `"0"`. Copying that idiom into `rebase_config` +would fail validation on `version` alone. + +Nested values need no manual encoding either. The rebase action declares `realJsonMapProps: +[PARAM_DIFF]`, and `JsonExtractor` re-encodes exactly that prop to a JSON string server-side, +re-decoding the raw body in object mode so `{}` stays an object and `[]` stays an array +(`PayloadExtractor/JsonExtractor.php:33-52`). **We send real nested JSON; no `json.dumps` on our +side.** + +### State machine + +States and transitions are enums (`MergeRequestLifecycle/MergeRequestLifecycleState.php`, +`…Transition.php`): + +- States: `development`, `in_review`, `approved`, `in_merge`, `published`, `canceled`. +- Transitions: `request_review`, `skip_review`, `approve`, `finish_review`, `merge`, + `rollback_merge`, `request_changes`, `publish`, `cancel`. + +Note that `skip_review`, `finish_review`, `rollback_merge` and `publish` have **no endpoint** — +they are driven internally. + +### Merge behavior + +`MergeProcessor::process` (`Storage/MergeRequests/Merge/MergeProcessor.php:45-80`) does more than +enqueue: + +1. **If the MR is in `development` and already has enough approvals, it calls `skipReview` + itself.** On a non-SOX project with the default of 0 required approvals this means + `merge` works **directly from `development`** — no explicit `request-review` needed — and + `skipReview` populates the change log on the way through + (`MergeRequestService.php:130-137`). This materially shortens the CLI's happy path. +2. Acquires a **project-wide lock**; a held lock raises `BranchIsNotReadyToMerge`. +3. Checks the state machine can apply `merge`; otherwise `BranchIsNotReadyToMerge` with + `Cannot merge, branch is in "" state.` +4. Rejects if another MR in the project is already processing (`isOtherMrInProjectProcessing`). +5. Validates conflicts, then `setInMerge` and enqueues the job. + +**409 therefore has four distinct causes, in two different response shapes** +(`MergeAction.php:97-109`): the three `BranchIsNotReadyToMerge` cases carry the machine-readable +`storage.mergeRequests.notReadyToMerge`, while a **conflict** raises `MergeValidationException` +and is thrown *without* that string code. Part 2 can tell "not ready" from "conflicted" on that +basis alone. + +The merge itself is atomic: the job applies the configuration changes and transitions to +`published` in one transaction, rolling back to `approved` on failure (`MergeRequestService.php` +`publish:194` / `rollbackMerge:186`, both wrapped in `transactionManager->transactional`). There +is no publish endpoint. + +### Conflicts are computed live + +`DefaultConflictValidator::validateMergeRequest` +(`Storage/MergeRequests/Merge/DefaultConflictValidator.php:70-98`) compares each dev-branch +config's **version(1)** `versionIdentifier` against the default branch's current one. Not a +conflict when: the config exists only in the default branch; both sides are deleted; or the +identifiers match. Otherwise +`MergeValidationExceptionError::createConfigurationInDefaultBranchChanged(componentId, +configurationId, isDeleted, devVersionIdentifier, defaultVersionIdentifier)` — which is exactly +the shape `GET …/conflicts` returns. + +Two consequences: a conflict requires the configuration to exist on **both** sides, so the +`theirs` side of a conflicting config's diff is always populated; and because the check runs on +every merge attempt, rebasing every conflicting config is sufficient to make the MR mergeable — +there is no MR-level "re-validate" step. + +### A successful merge deletes the source branch + +After the merge transaction commits, `MergeDevBranchJob` enqueues a `DevBranchDelete` job for +`branchFromId` (`Storage/Jobs/MergeDevBranchJob.php:179-187`). This is the happy path, every +time — there is no keep-the-branch option. Three consequences: + +- **Only the merged configurations survive**, applied to the default branch. Everything else + scoped to the dev branch — its buckets, tables, files, workspaces — is dropped with it. +- **It is a second, separate async job.** `merge_merge_request(wait=True)` waits for the *merge* + job; when that returns `success` the MR is `published`, but the branch deletion has only just + been enqueued. Callers must not assume the branch is already gone — nor that it still exists. +- **Every local reference to the branch goes stale**: `active_branch_id` in `config.json`, a + sync `branch-mapping.json` entry, a workspace created on the branch. Cleaning these up is + Part 2's job, and the precedent already exists — today's `branch merge` (the UI-URL escape + hatch) resets the active branch and calls `cleanup_branch_id_from_mapping` + (`services/branch_service.py:348-355`) for exactly this reason. The real merge command must do + at least as much, and its output must say the branch was deleted. + +This also closes the loop on *No cancel endpoint* below: deletion is not just how an MR is +canceled, it is also how every published MR ends. A branch's MR lifecycle always terminates with +the branch ceasing to exist. + +### Approvals + +`RequiredApprovalsCountProvider` (`Storage/MergeRequests/RequiredApprovalsCountProvider.php`): + +- Non-SOX default is **0** (`DEFAULT_COUNT_BRANCHES_MERGE_REQUESTS = 0`); SOX default is 2. +- The non-SOX count is project metadata under + `KBC.branches-merge-requests.required-approvals-count`, provider **`user`**. +- `hasEnoughApprovals` = `count(approvals) >= required`. + +Approvals are deleted on `request_changes` and on `cancel` +(`MergeRequestService.php:139-152`, `:163-176`, both `approvalRepository->deleteAllForMergeRequest`) +and nowhere else — so a rebase does not cost you an approval. + +**Important limitation, and a correction to an earlier draft of this RFC.** That count is *not* +reachable with a Storage token. It is read via `$project->getMetadataByProviderAndKey(...)` — +**project** metadata, written through `projectsMetadataModel` and exposed only on the **Manage +API** (`Controller/Manage/Projects/ProjectListMetadataAction.php:24`, +`/projects/{projectId}/metadata`). Branch metadata is a different store entirely +(`ListBranchMetadataService` returns `$branch->getMetadata()`, i.e. `DevBranchesMetadata` rows), +so kbagent's existing `get_branch_metadata_value(key, branch_id="default")` — which is how +`project description-get` reads `KBC.projectDescription` — **cannot** read it. Nor is the count +in any MR response: `MergeRequestResponse` carries `approvals` (a list of +`{approverId, approverName, createdAt}`) and `reviewers` (`{id, name, email, status}` with +`status` ∈ `approved`/`rejected`/null), but no required-count field. See the Part 2 non-goal +below for what follows from this. + +### Roles (non-SOX) + +Every write carries `#[MergeRequestsAllowedRoles(roles: [ProjectRole::ROLE_ADMIN, +ProjectRole::ROLE_SHARE])]` — verified on all six: create (`:39`), update (`:44`), +request-review (`:43`), approve (`:43`), reject (`:49`), merge (`:40`). The `reviewer`, +`developer` and `production_manager` roles appear only in the sibling +`#[ProtectedBranchAllowedRoles]` attribute, which `StorageRouteGuard` selects for the **SOX** +feature — so those roles carry no MR privileges in a non-SOX project. Reads (list, detail, +conflicts) are `#[AsReadOnlyAction]` with no role whitelist. + +The dev branch is locked for editing only while the MR is `in_merge` +(`Core/Storage/RouteGuard/StorageRouteGuard.php:108`, `:125` — `$isBranchLocked = +$mr->isInMerge()`), so editing and rebasing are allowed in `development`, `in_review` and +`approved`. + +### Feature gating + +Which endpoints sit behind a project feature — from each action's `#[RequireFeature]` attribute +and `StorageRouteGuard::canAccessStorageScope` (`Core/Storage/RouteGuard/StorageRouteGuard.php:158-180`): + +- **All six MR writes** (create, update, request-review, approve, reject, merge) require + `RequireFeature([FEATURE_PROTECTED_DEFAULT_BRANCH, FEATURE_BRANCHES_MERGE_REQUESTS])` — the + guard loops with a `break` on the first hit, so **either** feature suffices. +- **The MR reads** (list, detail, conflicts) and **`/diff`** carry no `RequireFeature` at all — + they work on any project. On a project without the feature, list simply returns what exists + (nothing). +- **`/rebase`** requires `FEATURE_BRANCHES_MERGE_REQUESTS` alone — the only endpoint tied + specifically to the non-SOX feature. + +A failed feature check makes the guard return false, which `RouteGuardListener` turns into +`AccessDeniedException` (`RouteGuardListener.php:75`) — **HTTP 403, byte-for-byte +indistinguishable from a role denial**. The API therefore gives a caller no way to tell "this +project doesn't have merge requests" from "your token's role can't do this". That fact is what +makes D9's split genuinely load-bearing rather than cosmetic: only a client-side pre-flight +`has_feature()` check can produce the right error message. + +### Create-time guards + +`MergeRequestCreateProcessor` rejects a target branch that is not the default +(`:53-54`, `InvalidBranchException::createTargetBranchNotDefault`) and any existing MR for the +same source branch (`:57-62`, `createMergeRequestExists`). Note the existence check +(`MergeRequestsModel::fetchForBranchFrom`) filters by `branchFromId` **only — no state filter** — +so a branch has at most one MR *ever*, not merely one *open* MR. In practice the two readings +coincide, because both terminal states end with the branch being deleted (published per the +section above, canceled via branch deletion), but the code's rule is the stronger one. Both +guards surface as **404**, not 400 (`MergeRequestCreateAction.php:84-88`). + +`reviewerIds` duplicates are de-duplicated server-side (`MergeRequestCreateRequest.php:107-108`), +`reason` on request-changes is capped at 1000 characters, and `AutoMergeStrategy` is exactly +`immediately` | `scheduled` | `none` (`Storage/MergeRequests/AutoMergeStrategy.php`). + +There is **no cancel endpoint**. An MR is canceled only as a side effect of deleting its source +branch (`legacy-app/src/Storage/Job/DevBranch/DevBranchDelete.php:201` → +`mergeRequestService->cancel`). + +### The rebase payload — the `diff` envelope + +The resolved content is wrapped in a `diff` envelope (connection#8040), mirroring the shape +`/diff` returns each side in, so a resolved diff side can be posted back nearly 1:1. + +Keep rebase: + +```json +{"version": 42, "diff": {"name": "…", "rows": [], "description": "…", + "configuration": {}, "changeDescription": "…", "isDisabled": false}} +``` + +Delete rebase — an **empty envelope**: + +```json +{"version": 42, "diff": {}} +``` + +Rules, from `RebaseRequest::validateDiff` and `mapValidatedData` +(`Storage/ComponentConfigurations/Rebase/Request/RebaseRequest.php`): + +- **`diff` is required.** A missing key, an explicit `null`, or a non-string (post-extractor) + value is a 400 quoting `MESSAGE_DIFF_REQUIRED`. Invalid JSON → *"diff" must be a valid JSON + object.*; a decoded non-object → *"diff" must be an object.* +- **Delete is signalled by an empty `diff`** — either `{}` (a decoded `stdClass` with zero + properties) or an empty / whitespace-only **string**. Both are accepted; the official PHP + client sends `(object) []`. +- **`diff.name` is required for a keep rebase** and must be non-empty after trimming + (`validateDiffName`). +- **`diff.rows` is required for a keep rebase**; the key must be present and be an array, while + `[]` legitimately deletes all rows (`validateDiffRows`). +- `diff.description` and `diff.changeDescription` must be strings or null; `diff.isDisabled` + accepts bool / string / numeric and defaults to `false`; `diff.configuration` must be an object + (an array is coerced) and defaults to `{}`. +- Row objects are `{id?, name?, description?, isDisabled?, configuration?}`; a missing or null + `id` means a new row, duplicate ids are rejected, array order becomes the sort order, and + `configuration` must be an object. +- `version` stays at the **top level**, is always required — it is the default-branch version + being re-anchored onto — and a target version that is not newer is rejected with 400 + (`ConfigurationRebaseTargetVersionNotNewerException`). The wire name is a genuine trap: bare + "version" reads as the dev-branch config's version, which is precisely the wrong one. The + client keeps the wire name (`version`, house style is wire fidelity) but the docstring must + spell out that it is the **default-branch** version, taken from the diff's `theirs.version` — + and Part 2 should not surface a bare `--version` flag; something explicit like + `--onto-version` says what it does. + +Compared with the older flat shape (content fields at the top level), delete/keep is decided by +the emptiness of one key rather than by the absence of six, and there is no trap where a lone +`isDisabled: false` silently turns a delete into a keep rebase. + +### The change log + +`Model_Row_MergeRequest::updateChangeLog` (`legacy-app/src/Model/Row/MergeRequest.php:324-331`) +writes `$changeLog['configurations'] = $changes`, and is called from `requestReview` +(`MergeRequestService.php:120`) and `skipReview` (`:134`) — **not** at merge. So the change list +is legitimately empty while the MR sits in `development`, and appears the moment it is sent for +review (or skipped past review by a merge from `development`, per *Merge behavior*). + +## Design decisions + +**D1 — Explicit typed parameters, not payload dicts.** The obvious alternative is to take a +prepared `payload: dict` per endpoint. The house style is explicit named parameters with presence +detection inside the method — see `update_config` (`client/configs.py:352`), whose docstring +already reads *"Only provided (non-None) fields are sent in the request"*. `is_disabled: bool | +None`, where `None` means "not sent", is exactly what `update_config` and `update_config_row` +already do. + +Honest note on that last point: under the pre-#8040 flat body the tri-state was a *correctness* +requirement, because a lone `isDisabled: false` flipped a delete into a keep rebase. With the +`diff` envelope it is no longer load-bearing — inside a non-empty `diff` the field is already +accompanied by `name` and `rows`, and omitting it defaults to `false` server-side either way. It +stays tri-state for consistency with the two neighbouring methods, not because the API forces it. + +**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 +docstring line saying so, because the surrounding file teaches the opposite. + +**D3 — The client does not implicitly wait for the merge job.** `merge_merge_request` takes +`wait: bool = False` and returns the raw job dict when `wait=False`. Waiting reuses the existing +`_wait_for_storage_job` (`client/_core.py:160`, which already raises `STORAGE_JOB_FAILED` / +`STORAGE_JOB_TIMEOUT`), keeping that protected helper inside the client where it belongs while +letting Layer 1 expose the usual `--wait` / `--timeout` idiom (`job run`, `data-app deploy`). + +This intentionally differs from its neighbour `create_dev_branch` (`client/branches.py:15`), +which always waits — and correctly so, because the created branch's id only exists in +`job.results`, making a non-waited return useless. A merge returns nothing the caller needs +beyond the job id, and it is the long operation in this feature, so the choice belongs to the +caller. Rebase needs none of this: it returns the configuration synchronously. + +**D4 — `diff` / `rebase` live in `client/configs.py`, not in the new module.** They are +configuration endpoints (`/components/{id}/configs/{id}/…`) and sit next to `get_config_detail` +and `update_config`. `client/` was split by URL family in #520; splitting by feature instead +would put two config endpoints in a file about merge requests. + +**D5 — Their `branch_id` is required, breaking the house idiom on purpose.** Every other config +method takes `branch_id: int | None = None` and falls back to production +(`prefix = f"/v2/storage/branch/{branch_id}" if branch_id else "/v2/storage"`). Both endpoints +carry `isAvailableWithoutBranch: false` and answer 400 on the default branch, so both take +`branch_id: int` with no default — production becomes unrepresentable rather than a runtime +error. The docstrings state why. + +**D6 — Keep and delete rebases are two methods, not one method with a `delete` flag.** A single +`rebase_config(…, delete: bool = False)` would have to validate, client-side, that the envelope is +empty when deleting and complete when keeping. Splitting the method makes Python do that work: +`rebase_config` takes `name` and `rows` as **required** parameters (matching +`validateDiffName` / `validateDiffRows` exactly) and always sends a populated `diff`, while +`rebase_config_delete` takes only the four addressing arguments and sends `{"version": N, +"diff": {}}`. No runtime guard, no `ValueError`, no illegal combination expressible. The cost is +two methods for one endpoint — a deliberate, documented exception to one-method-per-endpoint. + +The `diff` envelope makes this split cheap — the two bodies differ by one key's content rather +than by which of six fields are present — and the two methods are the only place in kbagent that +knows the envelope exists. + +**D7 — Payload *semantics* stay in Layer 2.** The client sends what it is given. Whether a +resolved configuration is a sensible three-way merge, whether `version` came from +`theirs.version`, and whether the config is actually in the MR's conflict set are all service +concerns. D6 removes the only validation the client could usefully do. + +**D8 — `merge_request_update` and `external_id` are included.** Both are easy to omit as +low-value for an interactive user, but kbagent's audience includes CI pipelines and the +importable SDK facade (`lib.py`), where "set auto-merge on an existing MR" is a reasonable +one-liner and correlating an MR with an external ticket id is the obvious use for a +255-character external reference. *Conditional:* `update` ships only if Part 2 also ships a +`merge-request update` command — an unused client method is dead code, not an SDK feature. + +**D9 — No feature-flag plumbing at Layer 3; the pre-flight check is Layer 2's job.** The mixin +methods send the request and translate errors generically, nothing more. Two verified facts +decide where the feature concern lands (see *Feature gating*): the reads aren't gated at all, and +a missing feature on a write answers a 403 identical to a role denial — so no amount of Layer 3 +error mapping can produce the right message, and a client-side pre-flight is the only source of +"this project doesn't have merge requests enabled". kbagent already has the machinery: +`has_feature(feature: str)` (`client/tokens.py:200`) takes a plain string and its cache is +populated on every `verify_token`. Part 1 contributes only the flag name as a constant in +`constants.py` (`branches-merge-requests`); Part 2's service calls `has_feature()` before writes +and words the error. + +The same pre-flight doubles as the **SOX fence**. Server-side, every MR write accepts *either* +feature (see *Feature gating*), so a SOX project (`protected-default-branch`) would sail through +the API gate — into a flow whose approvals semantics this RFC explicitly does not cover. +Checking specifically for `branches-merge-requests` makes kbagent refuse SOX projects with the +same "not enabled" message, which is exactly the declared scope. + +## Method inventory + +Returns are raw parsed JSON, as everywhere in `client/`. + +### Merge requests — new file `client/merge_requests.py` + +| Method | Endpoint | +|---|---| +| `list_merge_requests() -> list[dict]` | `GET /v2/storage/merge-request` | +| `get_merge_request(merge_request_id, include_activity_log=False) -> dict` | `GET …/merge-request/{id}[?include=activityLog]` | +| `get_merge_request_conflicts(merge_request_id) -> list[dict]` | `GET …/merge-request/{id}/conflicts` | +| `create_merge_request(branch_from_id, branch_into_id, title, description=None, reviewer_ids=None, auto_merge_strategy=None, auto_merge_at=None, external_id=None) -> dict` | `POST /v2/storage/merge-request` | +| `update_merge_request(…)` — **conditional, see D8** — `(merge_request_id, title=None, description=None, reviewer_ids=None, auto_merge_strategy=None, auto_merge_at=None, external_id=None) -> dict` | `PUT …/merge-request/{id}` | +| `request_merge_request_review(merge_request_id) -> dict` | `PUT …/merge-request/{id}/request-review` | +| `approve_merge_request(merge_request_id) -> dict` | `PUT …/merge-request/{id}/approve` | +| `request_merge_request_changes(merge_request_id, reason=None) -> dict` | `PUT …/merge-request/{id}/request-changes` | +| `merge_merge_request(merge_request_id, wait=False, max_wait=STORAGE_JOB_MAX_WAIT) -> dict` | `PUT …/merge-request/{id}/merge` | + +`list_merge_requests` takes no arguments — the endpoint declares no query parameters, so +`--state` filtering happens in the service. + +`merge_merge_request` reads awkwardly, but the house style is verb-first (`list_dev_branches`, +`create_dev_branch`, `update_config`) and the verb here really is "merge". A noun-first +`merge_request_merge` would be the only noun-first name in `client/`. + +The request-changes method is deliberately **not** named `reject_merge_request`. "Reject" reads +terminal; the backend transition is `request_changes` and it sends the MR back to `development` +to be revised and resubmitted — the terminal negative outcome is branch deletion, not this call. +`request_merge_request_changes` says what happens and is symmetric with +`request_merge_request_review`. (Part 2 should carry the same care into the command name.) + +The mixin `_MergeRequestsMixin(_CoreClient)` is composed into `KeboolaClient` +(`client/_client.py:28`) — one import and one base class, exactly like `_BranchesMixin`. + +### Conflict resolution — added to `client/configs.py` + +Branch-scoped; `branch_id` required (D5). + +| Method | Endpoint | +|---|---| +| `get_config_diff(component_id, configuration_id, branch_id) -> dict` | `GET …/branch/{branch_id}/components/{c}/configs/{cfg}/diff` | +| `rebase_config(component_id, configuration_id, branch_id, version, name, rows, configuration=None, description=None, change_description=None, is_disabled=None) -> dict` | `POST …/rebase` (keep) | +| `rebase_config_delete(component_id, configuration_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 from the envelope, +`is_disabled=False` is sent. `rebase_config_delete` sends exactly `{"version": N, "diff": {}}`. +Component and configuration ids are `quote()`d, as everywhere in `configs.py`. + +## Testing + +New `tests/test_merge_request_client.py`, using `pytest_httpx` like `tests/test_ai_client.py` +and `tests/test_manage_client.py`. + +- **Path construction** — the invariant most likely to be broken by someone copying the + `branch_id or production` idiom: every MR method hits a bare `/v2/storage/merge-request…` path + even when the project has an active branch, and `diff` / `rebase` always hit + `/v2/storage/branch/{id}/…`. +- **JSON encoding** — the second-most-likely mistake, since the surrounding file does the + opposite: assert `Content-Type: application/json`, that `configuration` and `rows` arrive as + real nested JSON rather than strings, that `version` / `branchFromId` / `branchIntoId` are + JSON numbers rather than strings, and that booleans are `true` / `false` rather than + `"1"` / `"0"`. +- **Presence detection** — `create` / `update` omit unset optionals; inside the envelope + `rebase_config` sends `is_disabled=False` but omits `is_disabled=None`, and `rows=[]` is sent + rather than treated as absent. +- **The `diff` envelope** — `rebase_config` puts `version` at the top level and every content + field inside `diff`, with nothing content-like leaking to the top level. Pin this explicitly: + a regression to the flat pre-envelope body would not fail loudly in a round-trip test, it + would just build the wrong request. +- **Delete resolution** — `rebase_config_delete` sends exactly `{"version": N, "diff": {}}`, with + `diff` serialised as an empty JSON **object** and not as `null`, `""` or an empty array. (Per + D6 there is no mixed-mode case to test; the split makes it unrepresentable.) +- **Merge waiting** — `wait=False` returns the job dict after a single request; `wait=True` + polls to a terminal state, and a failing job surfaces `STORAGE_JOB_FAILED` from the existing + helper, with no new error handling of our own. +- **`include=activityLog`** is present only when asked for. + +An E2E test against a real project is mandatory for the commands (convention #16) and lands in +Part 2; Layer 3 alone has no command to exercise. Everything above is verifiable offline, so no +`branches-merge-requests` project is needed to write or review this part. + +## Non-goals for Part 1 + +- Any command, service, or output formatting (Part 2). + +- **The status object, and the approvals problem it inherits.** Part 2 wants a derived status + ("mergeable / blocked because conflicts / blocked because approvals") so callers branch on data + rather than on parsed prose. Its inputs are the verified facts above: the six states, the + live conflicts list, and the approvals count. But per *Approvals*, **`required` is not + readable with a Storage token** — it is project metadata behind the Manage API, and kbagent's + manage-token policy is default-deny with an interactive prompt (convention #12). Requiring a + manage token to render a status line is not acceptable, so Part 2 should derive what it needs + from behaviour instead: `approvals` in the payload gives *given*, the state transition after + `request-review` reveals whether the requirement was already met (straight to `approved` means + it was), and the backend remains the authority via the merge 409. Part 2 must therefore + specify its own decision table rather than assume a readable `required` — and should not + report a required count it cannot see. + +- New `ErrorCode` members. Note what the existing mapping actually does + (`http_base.py:306-336`): 401 / 403 / 404 get bespoke codes (`INVALID_TOKEN`, + `ACCESS_DENIED`, `NOT_FOUND`), while **409 and 422 fall through to the generic `API_ERROR` + catch-all**, indistinguishable from any other unclassified status (neither is in + `RETRYABLE_STATUS_CODES`, `constants.py:53`). That is tolerable for Part 1, which has no + user-facing surface, but Part 2 almost certainly wants dedicated codes for the merge 409 — + and, given it has *two* shapes (`storage.mergeRequests.notReadyToMerge` versus a bare conflict + validation error), quite possibly two. Any new member must also be documented in + `docs/error-codes.md`, which `scripts/check_error_codes.py` enforces in CI. + +- `mcp_parity.py` entries. A parallel effort adds merge-request tools to + `keboola-mcp-server`, and `mcp_parity.py`'s docstring is explicit that "an unmapped tool is a + parity BUG by definition". The map currently holds 39 entries and none mention merge requests. + The nightly `mcp-parity-canary` (`make parity-check`, *not* part of `make check`) diffs the + live server catalogue against that map, so if the server ships its tools before kbagent ships + Part 2's commands, the canary goes red through no fault of any kbagent change and stays red + for the length of that window. Part 2 — or an interim commit, if the server lands first — must + add the entries. Flagged here so a red canary in the interim is recognised as expected + sequencing rather than a kbagent regression. + +- Whether `merge-request create` takes the source branch from `--branch` or from + `active_branch_id`. Layer 3 only ever receives it explicitly. + +- Post-merge cleanup UX. Per *A successful merge deletes the source branch*, after a successful + merge Part 2 must reset `active_branch_id`, clean the sync branch mapping (reusing + `cleanup_branch_id_from_mapping`), and say in the output that the branch was deleted. + +- Part 2 UX facts established here so they are not re-derived: + - **Requesting review can email the whole project.** The `ReviewRequested` notification falls + back to *all project members* when the MR has no selected reviewers and the project has no + designated reviewers (`MergeRequestNotificationRecipientResolver.php:86-95`, + `reviewRequestedPool`). The submit command's docs should say so; on a non-SOX project with 0 + required approvals, merging straight from `development` avoids the blast entirely. + - **Branch names are not resolvable for finished MRs.** `branches.branchFromId` is nullable in + `MergeRequestResponse` — the branch is deleted for both `published` and `canceled` MRs — so + any "IDs → names" rendering must tolerate a missing branch. + - **Reviewer ids are obtainable in kbagent** via `project member-list`, so `--reviewer-id` is + usable here (unlike in a chat context with no user-listing surface). + - **A resolve-shortcut is worth considering**: a `--take ours|theirs` mode that composes the + rebase payload from the diff server-side data instead of requiring the caller to author the + full resolved configuration. Layer 3 needs nothing for it — it is a Part 2 convenience over + `get_config_diff` + `rebase_config`. + +- The fate of `kbagent branch merge` (the UI-URL escape hatch). It becomes redundant once the + real commands exist; deprecate-with-pointer is the likely answer, decided in Part 2. + +- SOX flow, branch creation/deletion changes, auto-merge scheduling UX beyond passing the fields + through. + +## Checks + +`client/` has a file-size budget of 1500 soft / 2000 hard **code** lines +(`scripts/check_file_size.py:71-73`); `client/configs.py` is at 292 today, so both the new module +and the two added config methods are far inside it. No new `BaseHttpClient` subclass is +introduced — only a mixin on `KeboolaClient` — so `make check-sentinel-guards` is unaffected. MR +endpoints are Storage paths, which the programmatic-session (`kbc-session://`) v1 scope covers, +so no `SESSION_UNSUPPORTED_FEATURES` entry is expected; confirm once against a session-token +project rather than assuming. + +`make check` must be green before the PR.