From a1ce78e197ddd744dcc081253aeb6121c34b9c0f 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 01/15] Merge requests notes: general + per-layer (L3 as-built, L2/L1 working) [DMD-1899] L2 RFC decisions from the 2026-08-26 brainstorm: service shape (house pattern, verb_noun methods), derived status (4 derivates - derived_state, merge_blockers, allowed_actions, viewer; polyfill until DMD-1988), create source-branch via resolve_branch, merge-409 error codes mapped in the service (backend counterpart DMD-1984), conflict resolution uniformly via rebase (take ours/theirs; UI reset-to-default discrepancy filed as DMD-1987), per-path 3-way diff presentation, post-merge cleanup mirroring delete_branch, feature-constant rename; mcp_parity section marked obsolete (map removed in 0.85.0). Co-Authored-By: Claude Fable 5 --- docs/merge-requests-layer1.md | 61 +++++++++ docs/merge-requests-layer2.md | 232 ++++++++++++++++++++++++++++++++++ docs/merge-requests-layer3.md | 155 +++++++++++++++++++++++ docs/merge-requests-notes.md | 215 +++++++++++++++++++++++++++++++ 4 files changed, 663 insertions(+) create mode 100644 docs/merge-requests-layer1.md create mode 100644 docs/merge-requests-layer2.md create mode 100644 docs/merge-requests-layer3.md create mode 100644 docs/merge-requests-notes.md diff --git a/docs/merge-requests-layer1.md b/docs/merge-requests-layer1.md new file mode 100644 index 00000000..bb2abae4 --- /dev/null +++ b/docs/merge-requests-layer1.md @@ -0,0 +1,61 @@ +# Merge requests — Layer 1 (commands), working notes + +Linear: [DMD-1900](https://linear.app/keboola/issue/DMD-1900). UX facts for the future +`merge-request` command group, all verified against the backend +(citations in [`merge-requests-notes.md`](merge-requests-notes.md)). Service-layer material is +in [`merge-requests-layer2.md`](merge-requests-layer2.md). + +## Naming + +- **`request-changes`, not `reject`.** The transition sends the MR back to `development` to be + revised and resubmitted; the terminal negative outcome is branch deletion. Layer 3's method + name already carries this care — the command name must too. +- **Rebase's `version` needs an explicit flag name.** It is the *default-branch* version being + re-anchored onto (from the diff's `theirs.version`) — expose something like + `--onto-version`, never a bare `--version`. +- The `update` command pairs with Layer 3's `merge_requests.update` (shipped per decision + 2026-08-18, so the method is not dead code) — auto-merge strategy, `external_id` (255 chars, + correlates an MR with a ticket). + +## Output and wording + +- **Merge output must say the source branch was deleted** (it always is — no keep-the-branch + option), without asserting it is already gone (second async job; see notes doc). +- **Branch names are not resolvable for finished MRs.** `branches.branchFromId` is nullable — + the branch is deleted for both `published` and `canceled` MRs — so any "IDs → names" + rendering must tolerate a missing branch. +- **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. +- The happy path on non-SOX defaults is short: `merge` works **directly from `development`** + (the backend auto-skips review when approvals suffice) — the command flow should not force + an unnecessary `request-review` step. + +## Flags and conveniences + +- **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). Duplicates are + de-duplicated server-side. +- **A resolve shortcut is worth considering**: `--take ours|theirs` composing the rebase + payload from the diff's server-side data instead of requiring the caller to author the full + resolved configuration. Layer 3 needs nothing for it — it is a convenience over + `get_config_diff` + `rebase_config`. +- `reason` on request-changes: cap 1000 characters. `AutoMergeStrategy`: + `immediately` | `scheduled` | `none`. +- `--state` filtering on list is client-side (no query params on the endpoint). +- **`--wait` on merge has no Layer 3 fire-and-forget to map to**: `merge()` always awaits the + Storage job (`MERGE_JOB_MAX_WAIT` 600 s). A `--timeout` can map to `max_wait`; a `--no-wait` + mode would need its own polling via `merge_requests.get()` — decide deliberately, don't copy + the `job run --wait` idiom blindly. + +## Bookkeeping when the commands land + +- E2E test per command (convention #16), needs a `branches-merge-requests` project. +- All convention-#17 silent-drift surfaces: `commands/context.py`, CLAUDE.md command list, + `keboola-expert.md`, `SKILL.md`, `commands-reference.md`, `gotchas.md` + (`(since vX.Y.Z)` tags), a workflow reference doc. +- `mcp_parity.py` entries (see layer2 doc, canary sequencing). +- Dedicated error codes for the merge 409 must be documented in `docs/error-codes.md`. +- Decide the fate of `kbagent branch merge` in the same release (deprecate-with-pointer). diff --git a/docs/merge-requests-layer2.md b/docs/merge-requests-layer2.md new file mode 100644 index 00000000..ddf93add --- /dev/null +++ b/docs/merge-requests-layer2.md @@ -0,0 +1,232 @@ +# Merge requests — Layer 2 (service), working notes + +Linear: [DMD-1899](https://linear.app/keboola/issue/DMD-1899). Layer 3 shipped in #556 (see +[`merge-requests-layer3.md`](merge-requests-layer3.md)); backend behavior facts with citations +live in [`merge-requests-notes.md`](merge-requests-notes.md). Commands/UX material is in +[`merge-requests-layer1.md`](merge-requests-layer1.md). Scope stays **non-SOX**. + +## Service shape + +Decided 2026-08-26: the house pattern, no deviations. + +- One `MergeRequestService` class in `services/merge_request_service.py` (DI: `ConfigStore` + + `client_factory`, like every service). +- Method names are full `verb_noun` (`list_merge_requests`, `get_merge_request`, + `create_merge_request`, …) — the convention of all ~30 services. The L3 namespace de-dup + (`client.merge_requests.list()`) does not transfer: at L2 call sites the instance lives in a + generic `service` variable, so the noun must be in the method name or it is nowhere. + Methods whose name carries the noun another way stay short (`list_conflicts`, + `resolve_conflict`, `get_config_diff`). +- The class holds orchestration and I/O only. Pure logic with no dependencies — state + derivation, diff flattening, rebase-payload composition — goes into module-level functions + (testable without mocks). +- Single file until `make loc-check` says otherwise; the natural split line is lifecycle + (create/list/get/transitions/merge) vs. conflict resolution (conflicts/diff/rebase). + Precedent for a second file: the non-1:1 services (`member_service`, `variables_service`, …). + +## Derived status (decided 2026-08-26) + +Callers branch on data, not parsed prose. Long-term the derivation belongs to the backend — +one evaluation point, every client (UI, CLI, MCP) consumes it evaluated, the way GitHub +serializes `mergeable_state` / `reviewDecision` / `viewer*` instead of letting every client +re-derive them. Connection has no capacity now, so the CLI ships a **polyfill**: one pure +module-level function, server-first (`mr.get(...)` prefers the future serialized field), +local fallback implementing the tables below. The fallback carries a comment pointing at the +Connection issue — [DMD-1988](https://linear.app/keboola/issue/DMD-1988) — and is deleted +when the backend serializes. Precedent for the defensive +read: `changeLog`, and the required-approvals count +([DMD-1969](https://linear.app/keboola/issue/DMD-1969)). + +Evidence the derivation must not live in clients: the UI already disagrees with itself — the +list badge (`MergeRequestRow.tsx`: `published`→"Merged", `canceled`→"Closed", `rejected`/ +closed-by-creator derived from `reviewers[]`, no `in_merge` badge) vs. the detail panel +(`MergeRequestInfoPanel.tsx:12-19`: "Published", "Canceled", "Merging", no derivations at +all). The same MR shows "Rejected" in the list and "Development" in the panel. + +Four derivates; all `--json` fields are additive, raw `state` is always emitted alongside +(derivation never replaces wire truth): + +**1. `derived_state`** (list + detail) — the UI list badge's decision table, evaluated in +order; canonical vocabulary for all clients: + +| value | derivation | GitHub analog | +|---|---|---| +| `rejected` | `development` + a non-creator reviewer with `status=rejected` | open + CHANGES_REQUESTED | +| `closed` | `canceled`, or `development` + creator self-rejection (the UI "cancel" trick) | closed | +| `in_development` | `development` otherwise | open | +| `in_review` | `in_review` | open + REVIEW_REQUIRED | +| `approved` | `approved` | open + APPROVED | +| `in_merge` | `in_merge` (the one state the UI badge omits — we name it) | — | +| `merged` | `published` | merged | + +**2. `merge_blockers`** (detail only; list omits it — conflicts are not fetched per row) — a +*list*, not a single enum, so concurrent blockers don't mask each other; plus sugar +`mergeable: bool` (= empty list). Purely mechanical, **not a guard** — the merge 409 stays +the authority: + +| blocker | derivation | +|---|---| +| `conflicts` | live conflicts list non-empty (count + list emitted alongside) | +| `approvals` | `state == in_review` (the state collapses the requirement; quantitative "1 of 2" only when DMD-1969 lands — read defensively) | +| `state` | `in_merge` / `published` / `canceled` — merge not applicable | + +Note the honest consequence of the backend facts: a `rejected` MR has **no** blocker — it +sits in `development` and a non-SOX merge from there succeeds (auto-`skipReview`). The story +is told by `derived_state`, not by a fake blocker. + +**3. `allowed_actions`** (detail) — subset of `{request_review, approve, request_changes, +merge, update, resolve_conflicts}`, mechanically from the state machine (what the UI buttons +gate on: approve/reject only in `in_review|approved`, send-for-review only in `development`, +…). The polyfill does *not* mix roles/features in (the pre-flight owns those); the backend +adds them when it takes over. + +**4. `viewer`** (detail) — `{is_creator, has_approved}`, relative to the caller's identity +(admin id from `verify_token`, compared against `creator.id` and `approvals[].approverId`). +What the UI's approve button derives today (`ApproveMergeRequestButton.tsx:161`), and what an +MCP/agent response needs to phrase the next step: blocker `approvals` + `has_approved=true` +→ "wait for the other reviewers", + `is_creator=true` → "you cannot approve your own MR". + +`approvals[]` gives *who* approved; `reviewers[].status` (`approved`/`rejected`/null) gives +*who is still pending* — both stay available raw in the detail output. + +## Pre-flight feature check + +`GET /merge-request` list/detail/conflicts are ungated; a write without the feature is a 403 +byte-for-byte identical to a role denial. So the service calls +`has_feature(FEATURE_BRANCHES_MERGE_REQUESTS)` (`client/tokens.py:200`, cache populated on +every `verify_token`) before writes and words the "not enabled" error itself. + +Caveats to carry into the implementation: + +- **SOX fence assumption:** server-side, the six MR writes accept *either* feature; only + `/rebase` requires `branches-merge-requests` specifically. The pre-flight fences off SOX + projects **only if** a SOX project never also has `branches-merge-requests` — state that + assumption explicitly in the code comment. +- The pre-flight is **stricter than the server** for a project with only + `protected-default-branch`: kbagent refuses what the API would allow. Deliberate (the SOX + approvals semantics are out of scope), but the error message should mention it. +- Constant name decided 2026-08-26: rename to `BRANCHES_MERGE_REQUESTS_FEATURE` when wiring + the pre-flight — the file's dominant convention is the `…_FEATURE` suffix + (`STORAGE_BRANCHES_FEATURE`, `GLOBAL_SEARCH_FEATURE`, `PAYG_FEATURE`); the prefix form is + the lone outlier. Two touch points: `constants.py:439` + the docstring mention in + `client/merge_requests.py:132`. + +## Client-side `state` filtering + +The list endpoint has no query parameters — a `--state` filter is the service's job. + +## Merge: 409 handling and error codes + +The merge 409 has four causes in two shapes (`MergeAction.php:97-109`): three "not ready" +cases carry the machine-readable `storage.mergeRequests.notReadyToMerge`; a conflict raises +`MergeValidationException` **without** that code. Today both fall through `http_base.py`'s +generic `API_ERROR` catch-all (`http_base.py:306-336`; neither 409 nor 422 is mapped, neither +retryable). + +Decided 2026-08-26: **two new `ErrorCode` members, mapped in the service** — only the service +knows the 409 came from the merge endpoint, and the conflict shape carries no string code, so +the generic `http_base` layer cannot tell it apart: + +- `MR_NOT_READY_TO_MERGE` — the 409 carrying `storage.mergeRequests.notReadyToMerge` (three + causes: merge lock / wrong state / another MR processing; distinguishable only by message + text, hence one code). Transient states → `retryable=True`. +- `MR_MERGE_CONFLICT` — a 409 from the merge endpoint *without* the string code (the body + lists the conflicting configurations). `retryable=False`, details carry a "run + merge-request conflicts" next-step hint. + +Names may be polished to the enum's convention at implementation time. Both must be +documented in `docs/error-codes.md` (`scripts/check_error_codes.py` enforces in CI). + +## Post-merge cleanup + +A successful merge always deletes the source branch (second async job, no handle — the await +covers the merge only; see notes doc). + +Decided 2026-08-26 — mirror `delete_branch` (`services/branch_service.py:255-307`), which +already performs exactly this cleanup today. After a successful merge: + +- reset `active_branch_id` **only if** it points at the merged source branch (the + `was_active` logic of `delete_branch:286-288`; do *not* copy `get_merge_url:349`'s + unconditional reset — that is the worse of the two precedents), +- read `branches.branchFromId` from the MR payload **before** calling merge — it is + nullable once the MR is published, +- clean the sync branch mapping via `cleanup_branch_id_from_mapping`. The helper swallows + read errors (returns `None`) but its final `save_branch_mapping` can raise on IO — so + `merge()` wraps the whole cleanup block: a cleanup failure degrades to a warning in the + result, never changes the success exit code, +- a failed merge does no cleanup (the branch is still alive), +- workspaces on the branch need nothing: the server drops them with the branch (notes doc); + leftovers are `workspace list --orphaned` / `workspace gc` territory, +- output says the source branch "is being deleted" — never "is deleted" — and the result is + structured like `delete_branch`'s (`was_active`, `mapping_cleanup`, `message`). + +## Rebase / conflict resolution semantics + +Layer 3 deliberately does no payload validation (the signature covers structure). Service +concerns: + +- `version` for a rebase comes from the diff's `theirs.version` (it is the default-branch + version being re-anchored onto). +- Whether the config is in the MR's conflict set, and whether the resolved body is a sensible + three-way merge, are service checks. +- A conflict requires the config to exist on both sides, so `theirs` of a conflicting config's + diff is always populated; rebasing every conflicting config makes the MR mergeable (no + re-validate step). +- Flattening the nested `base`/`ours`/`theirs` diff for presentation is Layer 2's job (each + side may be null). + +Decided 2026-08-26: `resolve_conflict` offers four modes, **all via the rebase endpoint** +(one uniform mechanism, no new Layer 3 method): + +- `take=theirs` — the diff's theirs side (production content) rebased onto `theirs.version`; +- `take=ours` — the ours side (dev content) rebased onto `theirs.version`; +- `delete` — `rebase_config_delete` (the `{}` tombstone); +- a caller-supplied resolved body (JSON/@file) — pass-through with the conflict-set check, + the escape hatch for a genuine manual three-way merge. + +Edge case: an ours side with `isDeleted` turns `take=ours` into the delete resolution. + +Known deviation from the UI: its "Keep production version" button calls +`POST …/reset-to-default` (the config drops out of the MR entirely — not in the changeLog, +untouched by the merge), while our `take=theirs` via rebase keeps the config in the changeset +(changeLog entry + a content-no-op write at merge). Which behavior is intended is DMD-1987; +if reset wins, Layer 3 gains a `reset_config_to_default` method and `take=theirs` switches. +A bulk "resolve all one way" is deliberately out of v1 — it is a trivial Layer 1 loop over +`list_conflicts` + `resolve_conflict`. + +Presenting the three-way diff (decided 2026-08-26): no three panes — a **per-path change +classification**. A pure Layer 2 function computes two pairwise diffs (`base→ours`, +`base→theirs`) and tags every touched path `changed_by: ours | theirs | both`; only `both` +paths are the actual conflict. Tooling to steal: `json_utils.compute_diff` already has the +recursive walk but returns formatted strings — refactor it into a structured per-path +variant (entries as data) and keep the string output as a formatter over it +(`config_service` uses it today). The human rendering (Layer 1, DMD-1900 material) is a +table in three sections — *Both changed / Only you changed / Only production changed* — +with long values elided behind a `--format full`. `--json` carries the entries plus all +three raw sides. Manual merge stays marker-free: `diff --output resolved.json` writes an +ours-prefilled candidate, the caller edits it and submits via `resolve --file` — the +git-mergetool loop with a file as the third pane. + +## ~~`mcp_parity.py` and the canary~~ (obsolete since 0.85.0) + +This section predates 0.85.0 and no longer applies: the parity map (`mcp_parity.py`), +`scripts/check_mcp_parity.py` and the weekly `mcp-parity-canary` were all deleted with the +MCP passthrough removal (#609, 2026-08-19). No parity entries are needed anywhere for the +MR commands; the historical tool-to-command map lives in `docs/mcp-migration.md`. The +parallel `keboola-mcp-server` MR-tools effort continues independently, untracked by kbagent +CI. + +## Open decisions + +- ~~Whether `merge-request create` takes the source branch from `--branch` or from + `active_branch_id`~~ — **decided 2026-08-26: the standard `resolve_branch()` idiom**, like + every other branch-taking command. Explicit `--branch` wins, else `active_branch_id`; with + neither, a readable error ("pass --branch or run branch use") — no further fallback. The + create output must state which branch the MR was created from. +- The fate of `kbagent branch merge` (the UI-URL escape hatch, `branch_service.py:309`): + deprecate-with-pointer, already committed as DMD-1900 scope (pattern: the #390 tool-group + deprecation). +- SOX flow, branch creation/deletion changes, auto-merge scheduling UX beyond passing the + fields through — all out of scope for now. +- E2E tests are mandatory with the commands (convention #16) — they need a + `branches-merge-requests` project. diff --git a/docs/merge-requests-layer3.md b/docs/merge-requests-layer3.md new file mode 100644 index 00000000..b8e5cc35 --- /dev/null +++ b/docs/merge-requests-layer3.md @@ -0,0 +1,155 @@ +# Merge requests — Layer 3 (HTTP client), as built + +**Status: shipped.** [DMD-1701](https://linear.app/keboola/issue/DMD-1701), PR +[#556](https://github.com/keboola/cli/pull/556), squash-merged to main as `b7b66af` +(2026-08-19), released in 0.86.0 (changelog completed by #619). This document is the as-built +record distilled from the original RFC and the review cycle; behavioral backend facts live in +[`merge-requests-notes.md`](merge-requests-notes.md). + +Code: `client/merge_requests.py` (namespace + Protocol + adapter + mixin), +`client/configs.py` (diff/rebase), `constants.py` (`FEATURE_BRANCHES_MERGE_REQUESTS`, +`MERGE_JOB_MAX_WAIT`), `tests/test_merge_request_client.py`. + +## Backend contract (what shapes the client) + +Project-level — `isAvailableInBranch: false`, so **never** branch-prefixed: + +| 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 …/{id}/request-review` | — | 200 | 403, 404, 422 | +| `PUT …/{id}/approve` | — | 200 | 403, 404, 422 | +| `PUT …/{id}/request-changes` | JSON `{reason?}` | 200 | 403, 404, 422 | +| `PUT …/{id}/merge` | — | **202** + a Job | **409**, 403, 404 | +| `GET …/{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 / bad `diff`, 403, 404 | + +`GET /merge-request` declares **no query parameters** — a `state` filter is necessarily +client-side. `GET /merge-request/{id}` takes `include=activityLog`. Only `merge` is +asynchronous. + +**Bodies are JSON with real types** — `#[MapRequestBody]` accepts form data but +`FormDataExtractor` does no type coercion, and validators require real types (`branchFromId` +`Assert\Type('int')`, rebase `version` `Assert\Type('integer')`). Form-encoded values stay +strings and fail validation. No client-side `json.dumps` for nesting either — the rebase +action's `realJsonMapProps: ['diff']` re-encodes server-side, preserving `{}` vs `[]`. + +**The rebase `diff` envelope** (connection#8040): keep = +`{"version": N, "diff": {"name", "rows", "configuration", "isDisabled", "description"?, +"changeDescription"?}}`; delete = `{"version": N, "diff": {}}`. `version` stays top-level and +is the **default-branch** version being re-anchored onto (from the diff's `theirs.version`) — +the wire name is a trap, kept for wire fidelity, spelled out in the docstring. + +## Shipped surface + +`client.merge_requests.*` (namespace, raw parsed JSON returns): + +| Method | Endpoint | +|---|---| +| `list()` | `GET /v2/storage/merge-request` | +| `get(id, include_activity_log=False)` | `GET …/{id}[?include=activityLog]` | +| `conflicts(id)` | `GET …/{id}/conflicts` | +| `create(branch_from_id, branch_into_id, title, …)` | `POST /v2/storage/merge-request` | +| `update(id, …)` | `PUT …/{id}` | +| `request_review(id)` | `PUT …/{id}/request-review` | +| `approve(id)` | `PUT …/{id}/approve` | +| `request_changes(id, reason=None)` | `PUT …/{id}/request-changes` | +| `merge(id)` | `PUT …/{id}/merge` + awaits the Storage job | + +On `_ConfigsMixin` (flat, config endpoints): `get_config_diff(component_id, config_id, +branch_id)`, `rebase_config(…, version, name, rows, configuration, is_disabled, description, +change_description=None)`, `rebase_config_delete(…, version)`. + +## Design decisions, as built + +- **Namespace over flat methods** (`client.merge_requests.*`). Flat naming collides + (`request_merge_request_review`, `merge_merge_request`); the namespace keeps verbs + wire-faithful. **Normative for new endpoint families only** — existing flat families stay + flat (policy stated in the module docstring; consider promoting to CONTRIBUTING.md with + Part 2). +- **The namespace depends on a `StorageRequester` Protocol, not on the client.** + `_ClientRequester` is a marked temporary adapter delegating to `_CoreClient`'s protected + methods; the client-split RFC (#595, branch `martinsifra/requestor`) builds the real + transport under this seam later — swap is one line, the namespace and its tests stay + byte-identical. Keep the Protocol minimal. (Honest caveat from review: `request()` returning + `httpx.Response` ties the future transport to httpx — today it is more a rename of + `_request` than an abstraction.) +- **JSON bodies throughout**, deviating from `configs.py`'s form idiom; every method's + docstring says so. (Post-rebase note: #598 already added JSON-body methods to `configs.py`, + so "unlike this file's idiom" phrasing was softened.) +- **`merge()` awaits implicitly** like every Storage-job method in `client/` — no `wait` flag. + Budget `MERGE_JOB_MAX_WAIT` = 600 s (precedent `IMPORT/EXPORT_JOB_MAX_WAIT`): a many-config + merge plausibly outlives the default 60 s, and a mid-merge `STORAGE_JOB_TIMEOUT` with + `retryable=True` would be actively misleading. Returns the completed job dict whose + `results` carry the MR incl. change log. **The await covers the merge outcome only** — the + source-branch deletion is a second job with no handle (docstring states it). +- **Contract dependency on the poller (#603):** `wait_for_storage_job` must raise on a failed + job whether the failure arrives in the initial body or polled — `merge()` does **not** + re-check. History: the poller originally returned an already-terminal error body silently + (a house-wide bug across all 19 call sites); `merge()` carried a local guard until the + central check-then-fetch fix merged as #603 (`2f0544d`, v0.84.3) and the guard was dropped + on rebase. The requirement is stated on the Protocol docstring and pinned by #603's + `TestWaitForStorageJob`, not by a merge-level test. +- **diff/rebase live in `client/configs.py`** — `client/` is split by URL family (#520), not + by feature. Their `branch_id: int` is **required with no production fallback** (both answer + 400 on the default branch) — production is unrepresentable rather than a runtime error, + deliberately breaking the house `branch_id: int | None = None` idiom. +- **Keep and delete rebases are two methods.** `rebase_config` requires `name` and `rows` + (matching `validateDiffName`/`validateDiffRows`); `rebase_config_delete` sends exactly + `{"version": N, "diff": {}}`. The signature does the validation; no illegal combination is + expressible. +- **Rebase REPLACES, so every replaced-body field is required** (`name`, `rows`, + `configuration`, `is_disabled`, `description` — the last required-but-nullable). Optional + params with defaults would make silent data loss the signature's default (wiped config body, + re-enabled config). Landed via padak's #606 during review. `change_description` is the one + genuine optional (not part of the replaced body; null selects a default message). + Presence-detection (`is not None`, omit unset) stays correct for `create`/`update`, which + genuinely patch; `_optional_mr_fields` is keyword-only (four of five params are + `str | None`). +- **No feature-flag plumbing at Layer 3.** A missing feature is a 403 identical to a role + denial — only a Layer 2 pre-flight can word the error. Part 1 contributed only the constant + `FEATURE_BRANCHES_MERGE_REQUESTS`. +- Tried and reverted: keyword-only `rebase_config` (bare `*`). The "ids are positional + house-wide" premise was false (Layer 2 call sites are mixed), so no placement had a + consistency case; signature stays shaped like its `configs.py` siblings. If keyword-only is + ever wanted, make it a house-wide CONTRIBUTING.md rule, not a one-method exception. +- Python gotcha hit: `MergeRequests.list` shadows the builtin in class-scope annotations → + module-level aliases `_DictList`/`_IntList`. + +## What the tests pin (`tests/test_merge_request_client.py`, 27 tests) + +- Path construction: MR paths never branch-prefixed even with an active branch; diff/rebase + always are. +- JSON encoding: content-type, real nested JSON, JSON ints for `version`/`branchFromId`, + real booleans. +- The `diff` envelope: `version` top-level, content inside `diff`; delete sends exactly + `{"version": N, "diff": {}}` (object, not null/`""`/`[]`); `rows=[]` is sent, not omitted. +- Required replaced-body params: a TypeError loop over each omitted field + a wire test that + `configuration`/`isDisabled` always reach the body. +- `merge()` waits with `MERGE_JOB_MAX_WAIT`, not the default — pinned via a recording stub + requester (httpx mocks can't see the kwarg). +- The seam: one test constructs `MergeRequests` against a stub `StorageRequester`, no HTTP. + +## Deferred / follow-ups recorded during review + +- `FEATURE_BRANCHES_MERGE_REQUESTS` naming is off the file's dominant convention (suffix: + `STORAGE_BRANCHES_FEATURE`, `GLOBAL_SEARCH_FEATURE`, `PAYG_FEATURE`) and the constant is + unused until Layer 2 calls it — decide rename vs. keep when wiring the pre-flight. +- SOX-fence caveat on the constant's comment: the fence holds only if a SOX project never also + has `branches-merge-requests` — see the layer2 doc for the pre-flight consequences. +- Docstring nit: `request_review`/`approve` PUT an empty body while `request_changes` without + a reason PUTs `{}` — both correct, difference undocumented. +- Retry policy: since #616 POST is no longer retried; PUT/DELETE are, and the four PUT + transitions verifiably cannot double-apply (#617; see the notes doc, *PUT transitions cannot + double-apply*). The residual lost-response-replay caveat (attempt 2's misleading 422/409) is + house-wide, not MR-specific. diff --git a/docs/merge-requests-notes.md b/docs/merge-requests-notes.md new file mode 100644 index 00000000..cf199161 --- /dev/null +++ b/docs/merge-requests-notes.md @@ -0,0 +1,215 @@ +# Merge requests — verified backend facts (all layers) + +Everything here was verified directly against `keboola/connection` and is cited to a file and +line. Scope is the **non-SOX** flow (`branches-merge-requests`); SOX +(`protected-default-branch`) is out of scope. Layer-specific material lives in the siblings: +[`merge-requests-layer3.md`](merge-requests-layer3.md) (the shipped HTTP client), +[`merge-requests-layer2.md`](merge-requests-layer2.md) (service, DMD-1899), +[`merge-requests-layer1.md`](merge-requests-layer1.md) (commands UX). + +## 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`. + +`skip_review`, `finish_review`, `rollback_merge` and `publish` have **no endpoint** — they are +driven internally. The lifecycle is a Symfony Workflow `state_machine`, which matters for +retries (see *PUT transitions cannot double-apply* below). + +## 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. "Not ready" can be told 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. 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 with no job handle returned.** `merge_requests.merge()` + awaits 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** — cleaning that up is Layer 2's job (see + the layer2 doc, *Post-merge cleanup*). + +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`). Deletion is thus how every MR lifecycle ends — published or +canceled, the branch ceases to exist. Related create-time nuance: 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 readings +coincide because both terminal states end with branch deletion, but the code's rule is the +stronger one. + +## Approvals + +- **The state machine collapses the approvals requirement into the state.** An MR sits in + `in_review` only while approvals are insufficient; the moment the requirement is met the + backend auto-transitions to `approved` (internal `finish_review`). With the non-SOX default + of **0** required approvals, `request-review` lands straight in `approved` and the approve + step never runs. The `state` field is therefore the authoritative answer to "are approvals + satisfied?" — no count needed. +- **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. +- **The required count itself is unreadable with a Storage token** — a documented trap. It is + **project** metadata (`KBC.branches-merge-requests.required-approvals-count`), exposed only + on the Manage API; branch metadata is a different store entirely, so + `get_branch_metadata_value(key, branch_id="default")` would not fail, it would quietly + report the key as absent. Nor is the count in any MR response: `MergeRequestResponse` + carries `approvals` (`{approverId, approverName, createdAt}`) and `reviewers` + (`{id, name, email, status}` with `status` ∈ `approved`/`rejected`/null), but no + required-count field. +- The count's mechanics: `RequiredApprovalsCountProvider` computes + `hasEnoughApprovals = given >= required`, defaults 0 (non-SOX) / 2 (SOX), reading project + metadata (provider `user`) via `Controller/Manage/Projects/ProjectListMetadataAction.php:24`. + The Keboola UI *can* show "1 of 2 approvals" because it runs as an admin session and reads + that Manage endpoint as a side channel. kbagent deliberately does not chase that parity — + its manage-token policy is default-deny (convention #12), and requiring a manage token to + render a status line would invert it for cosmetics. **Connection is expected to add the + count to the Storage API** ([DMD-1969](https://linear.app/keboola/issue/DMD-1969)) — the + recommended shape is a field + serialized into `MergeRequestResponse` (the provider already exists server-side), which + flows through Layer 3's raw dicts with zero client change. Read it defensively. +- Server-side quirk (filed as keboola/connection#8209, surfaced by the #616 audit): + `bi_rMergeRequestsApprovals` has no unique constraint on `(mergeRequestId, idAdmin)` and + `hasEnoughApprovals()` counts rows rather than distinct admins. + +## Roles and feature gating (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. + +All six MR writes accept **either** `protected-default-branch` **or** +`branches-merge-requests`; the reads and `/diff` are ungated; `/rebase` alone requires +`branches-merge-requests` (`StorageRouteGuard::canAccessStorageScope`, +`Core/Storage/RouteGuard/StorageRouteGuard.php:158-180`). A failed feature check makes the +route guard return false, which `RouteGuardListener` turns into `AccessDeniedException` +(`RouteGuardListener.php:75`) — **HTTP 403, byte-for-byte indistinguishable from a role +denial**. Only a client-side pre-flight can produce the right "not enabled" message — hence +Layer 2's `has_feature` check (layer2 doc). + +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`. + +## 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. Shape: +`{configurations: [{componentId, configurationId, lastVersionIdentifier, isDeleted}]}`. 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*). +Read it defensively. + +## PUT transitions cannot double-apply + +kbagent's retry policy (`RETRY_SAFE_METHODS`, since #616) treats PUT as retry-safe, and the MR +client uses PUT for four action-style transitions (`/request-review`, `/approve`, +`/request-changes`, `/merge`). Verified against Connection (recorded in #617): a retried +transition **cannot fire twice**. Three of the four are refused structurally on a second call +(a Symfony state machine enables a transition only from its declared `from` place); `/approve` +is the one self-loop and carries `AddApprovalGuard` instead. Notifications ride +`workflow.merge_request_lifecycle.completed` from inside `apply()`, inside +`MergeRequestService`'s `transactional()` — no transition, no notification. + +Caveat that survives: a retried PUT that succeeded but lost its response reports **attempt 2's +error** (a 422/409 on an operation that actually applied). That applies to every retried +PUT/DELETE in kbagent, not just merge requests. + +## Misc limits + +- `reason` on request-changes is capped at 1000 characters + (`MergeRequestRejectRequest::REASON_MAX_LENGTH`); the body is `required: false`. +- `reviewerIds` duplicates are de-duplicated server-side (`array_unique` in + `mapValidatedData`). +- `AutoMergeStrategy` is exactly `immediately` | `scheduled` | `none`. +- `externalId` max 255 (`Assert\Length(max: 255)`, create and update DTOs). +- update semantics: null = leave unchanged, absent ≡ null, no clear-to-null — but an **empty + string** clears `description`/`externalId` (`?? null` mapping + `!== null` guards in + `MergeRequestService::updateMergeRequest`). `PUT {}` is a no-op returning the MR. + +## Wire-truth verification table + +Re-verified against Martin's local `keboola/connection` checkout on 2026-08-19, during the +final Layer 3 review: + +| Claim | Backend evidence | +|---|---| +| MR endpoints project-level, never branch-prefixed | `isAvailableInBranch: false` on every `Controller/Storage/MergeRequest/*Action.php` route | +| `branchFromId`/`branchIntoId` must be JSON ints | `Assert\Type('int')` in `MergeRequestCreateRequest::getConstraint()` | +| Non-default target & existing-MR-per-source-branch → **404** (not 400) | both throw `InvalidBranchException` in `MergeRequestCreateProcessor`, caught → `HTTP_NOT_FOUND` | +| merge answers 202 + Storage job | `JsonResponse($job->toApiResponse(...), 202)` in `MergeAction` | +| 409: three "not ready" causes carry `storage.mergeRequests.notReadyToMerge`, a conflict does not | exactly 3 `throw new BranchIsNotReadyToMerge` sites in `MergeProcessor` → `createConflictException(stringCode: ...)`; `MergeValidationException` → plain 409 | +| Failed merge rolls back the MR | `rollbackMerge` in `MergeDevBranchJob`'s catch | +| Source branch deleted as a second job, no handle returned | `createAndEnqueueJobFromJob(..., DevBranchDelete::OPERATION_NAME, ...)` after commit in `MergeDevBranchJob` | +| diff/rebase 400 on default branch | `ConfigurationRebaseNotAvailableOnDefaultBranchException` / diff OA doc → `createBadRequestException` | +| Diff shape `base`/`ours`/`theirs`, each nullable | `ConfigurationDiffResponse` | +| Rebase replaces; missing `configuration` → `{}`, `isDisabled` → `false`, `description` → null | `RebaseRequest::mapValidatedData` (`?? new stdClass()`, `?? false`, `isset` → null); "complete 3-way diff result … fully replaces" verbatim in `ConfigurationRebaseService` docblock | +| `rows` required for keep; `[]` deletes all rows; order = sort order | `validateDiffRows` + OA schema | +| Empty `diff` `{}` = delete resolution (tombstone) | `validateDiff` empty-stdClass branch → `isDelete: true` | +| `changeDescription` null → default rebase message | `ConfigurationRebaseService` line ~102 | +| Target version must be newer → 400 | `ConfigurationRebaseTargetVersionNotNewerException` → 400 (ULID comparison) | +| `protected-default-branch` passes the same feature gate | `StorageRouteGuard` loops `RequireFeature.features` with OR semantics; MR routes list both features, rebase lists only `branches-merge-requests` | + +Resolved subtlety worth remembering: `RebaseRequest::validateDiff` expects `diff` as a *string* +and `json_decode`s it — which at first glance contradicts the client sending a real nested +object. It doesn't: the rebase action maps the body with +`#[MapRequestBody(realJsonMapProps: ['diff'])]`, and `JsonExtractor` re-encodes a nested `diff` +object back into a JSON string before validation (preserving the `{}`-vs-`[]` distinction). So +the nested-object body is correct, and `{}` survives as the delete sentinel while `[]` is +rejected ("diff must be an object"). From ff27365ace236c553dcbf997aa582d6b1bdfc797 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C5=A0ifra?= Date: Wed, 26 Aug 2026 03:27:28 +0200 Subject: [PATCH 02/15] refactor(json): expose compute_diff's walk as structured DiffEntry data compute_diff returned pre-formatted strings, so a caller needing the changed paths as data (the MR three-way conflict classification, DMD-1899, intersects two pairwise diffs per path) had nothing to build on short of parsing the strings back. Split the recursive walk into compute_diff_entries() returning frozen DiffEntry dataclasses (path + old/new with an _ABSENT sentinel distinct from an explicit None), and keep compute_diff() as a formatter over it -- byte-identical output, pinned by a delegation test against the existing format tests. Co-Authored-By: Claude Fable 5 --- src/keboola_agent_cli/json_utils.py | 76 ++++++++++++++++++++++++----- tests/test_json_utils.py | 65 ++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 12 deletions(-) diff --git a/src/keboola_agent_cli/json_utils.py b/src/keboola_agent_cli/json_utils.py index 5829ee82..50dd0d89 100644 --- a/src/keboola_agent_cli/json_utils.py +++ b/src/keboola_agent_cli/json_utils.py @@ -8,8 +8,13 @@ from __future__ import annotations import copy +from dataclasses import dataclass from typing import Any +# Sentinel marking "the key does not exist on this side" in a DiffEntry -- +# distinct from an explicit ``None`` value, which is a legal JSON value. +_ABSENT: Any = object() + def deep_merge(target: dict[str, Any], source: dict[str, Any]) -> dict[str, Any]: """Recursively merge *source* into *target* (non-mutating). @@ -95,19 +100,46 @@ def set_nested_value(obj: dict[str, Any], path: str, value: Any) -> dict[str, An return result -def compute_diff( +@dataclass(frozen=True) +class DiffEntry: + """One changed dot-separated path between two nested dicts. + + ``old`` / ``new`` hold the value on each side, or the module-private + ``_ABSENT`` sentinel when the key does not exist there -- distinct from an + explicit ``None``, which is a legal JSON value. Callers read the + ``old_present`` / ``new_present`` properties instead of comparing against + the sentinel. + """ + + path: str + old: Any + new: Any + + @property + def old_present(self) -> bool: + return self.old is not _ABSENT + + @property + def new_present(self) -> bool: + return self.new is not _ABSENT + + +def compute_diff_entries( old: dict[str, Any], new: dict[str, Any], path: str = "", -) -> list[str]: - """Produce a human-readable list of changes between two dicts. +) -> list[DiffEntry]: + """Compute changed paths between two dicts as structured entries. - Each entry looks like: - ``"parameters.tables.count: 5 -> 10"`` - ``"parameters.newKey: (absent) -> 'hello'"`` - ``"parameters.removed: 42 -> (absent)"`` + The recursive walk behind :func:`compute_diff` (which formats these + entries for humans), exposed as data so callers can post-process paths -- + e.g. intersect two pairwise diffs into a three-way ``ours``/``theirs``/ + ``both`` classification (merge-request conflict presentation, DMD-1899). + + Nested dicts recurse; any other type mismatch or value change yields one + entry for the whole path. Keys are visited in sorted order. """ - changes: list[str] = [] + entries: list[DiffEntry] = [] all_keys = sorted(set(list(old.keys()) + list(new.keys()))) for key in all_keys: @@ -119,14 +151,34 @@ def compute_diff( old_val = old[key] new_val = new[key] if isinstance(old_val, dict) and isinstance(new_val, dict): - changes.extend(compute_diff(old_val, new_val, full_path)) + entries.extend(compute_diff_entries(old_val, new_val, full_path)) elif old_val != new_val: - changes.append(f"{full_path}: {_fmt(old_val)} -> {_fmt(new_val)}") + entries.append(DiffEntry(path=full_path, old=old_val, new=new_val)) elif in_old and not in_new: - changes.append(f"{full_path}: {_fmt(old[key])} -> (absent)") + entries.append(DiffEntry(path=full_path, old=old[key], new=_ABSENT)) else: - changes.append(f"{full_path}: (absent) -> {_fmt(new[key])}") + entries.append(DiffEntry(path=full_path, old=_ABSENT, new=new[key])) + + return entries + +def compute_diff( + old: dict[str, Any], + new: dict[str, Any], + path: str = "", +) -> list[str]: + """Produce a human-readable list of changes between two dicts. + + A formatter over :func:`compute_diff_entries`. Each entry looks like: + ``"parameters.tables.count: 5 -> 10"`` + ``"parameters.newKey: (absent) -> 'hello'"`` + ``"parameters.removed: 42 -> (absent)"`` + """ + changes: list[str] = [] + for entry in compute_diff_entries(old, new, path): + old_s = _fmt(entry.old) if entry.old_present else "(absent)" + new_s = _fmt(entry.new) if entry.new_present else "(absent)" + changes.append(f"{entry.path}: {old_s} -> {new_s}") return changes diff --git a/tests/test_json_utils.py b/tests/test_json_utils.py index 6dffa3c1..2df2b45d 100644 --- a/tests/test_json_utils.py +++ b/tests/test_json_utils.py @@ -4,6 +4,7 @@ from keboola_agent_cli.json_utils import ( compute_diff, + compute_diff_entries, deep_merge, get_nested_value, set_nested_value, @@ -177,3 +178,67 @@ def test_nested_changes(self) -> None: changes = compute_diff(old, new) assert len(changes) == 1 assert "db.host:" in changes[0] + + +class TestComputeDiffEntries: + """Tests for compute_diff_entries() -- the structured walk under compute_diff().""" + + def test_identical_dicts_yield_no_entries(self) -> None: + d = {"a": 1, "nested": {"b": 2}} + assert compute_diff_entries(d, d) == [] + + def test_value_change_carries_both_sides(self) -> None: + entries = compute_diff_entries({"host": "old"}, {"host": "new"}) + assert len(entries) == 1 + entry = entries[0] + assert entry.path == "host" + assert entry.old == "old" + assert entry.new == "new" + assert entry.old_present and entry.new_present + + def test_added_key_is_absent_on_old_side(self) -> None: + entries = compute_diff_entries({"a": 1}, {"a": 1, "b": 2}) + assert len(entries) == 1 + entry = entries[0] + assert entry.path == "b" + assert not entry.old_present + assert entry.new_present + assert entry.new == 2 + + def test_removed_key_is_absent_on_new_side(self) -> None: + entries = compute_diff_entries({"a": 1, "b": 2}, {"a": 1}) + assert len(entries) == 1 + entry = entries[0] + assert entry.path == "b" + assert entry.old_present + assert not entry.new_present + + def test_explicit_none_is_present_not_absent(self) -> None: + # None is a legal JSON value -- it must not be conflated with absence. + entries = compute_diff_entries({"a": None}, {"a": 1}) + assert len(entries) == 1 + assert entries[0].old_present + assert entries[0].old is None + + def test_nested_paths_are_dotted(self) -> None: + entries = compute_diff_entries( + {"db": {"host": "old", "port": 5432}}, {"db": {"host": "new", "port": 5432}} + ) + assert [e.path for e in entries] == ["db.host"] + + def test_dict_vs_scalar_is_one_whole_path_entry(self) -> None: + entries = compute_diff_entries({"a": {"x": 1}}, {"a": 2}) + assert len(entries) == 1 + assert entries[0].path == "a" + assert entries[0].old == {"x": 1} + assert entries[0].new == 2 + + def test_compute_diff_formats_entries_identically(self) -> None: + # compute_diff is now a formatter over compute_diff_entries; its + # output format is pinned by TestComputeDiff above -- this pins the + # delegation (same paths, same order). + old = {"a": 1, "b": {"c": 2}, "gone": 3} + new = {"a": 9, "b": {"c": 2, "d": 4}} + strings = compute_diff(old, new) + entries = compute_diff_entries(old, new) + assert [s.split(":")[0] for s in strings] == [e.path for e in entries] From 40e9d8f1228b11d02c47f3f8452527b06a8d61f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C5=A0ifra?= Date: Wed, 26 Aug 2026 03:30:07 +0200 Subject: [PATCH 03/15] feat(errors): MR merge-409 codes + surface the API's string code in details [DMD-1899] Three pieces of error plumbing the merge-request service needs: - ErrorCode.MR_NOT_READY_TO_MERGE / MR_MERGE_CONFLICT (+ docs/error-codes.md, both categorized 'conflict'). The merge 409 has four causes in two wire shapes; the split follows the backend's own line (the three transient causes carry storage.mergeRequests.notReadyToMerge, a conflict carries no code). Mapping happens in the service -- only it knows the 409 came from the merge endpoint. - http_base: a Keboola user error's machine string `code` now survives into KeboolaApiError.details['api_error_code'] (all four raise sites). The message holds only the human `error` text, so without this no caller can branch on the code. Additive; no behavior change when absent. - constants: FEATURE_BRANCHES_MERGE_REQUESTS renamed to BRANCHES_MERGE_REQUESTS_FEATURE (the file's dominant suffix convention; the constant was unused until now). SOX-fence assumption spelled out in the comment per the L2 RFC. Backend counterpart asking for codes on every MR error: DMD-1984. Co-Authored-By: Claude Fable 5 --- docs/error-codes.md | 7 ++ .../client/merge_requests.py | 2 +- src/keboola_agent_cli/constants.py | 18 +++-- src/keboola_agent_cli/errors.py | 10 +++ src/keboola_agent_cli/http_base.py | 13 ++++ tests/test_http_base.py | 66 +++++++++++++++++++ 6 files changed, 109 insertions(+), 7 deletions(-) diff --git a/docs/error-codes.md b/docs/error-codes.md index a3aba6e7..e933cd95 100644 --- a/docs/error-codes.md +++ b/docs/error-codes.md @@ -185,3 +185,10 @@ of `ErrorCode` in `src/keboola_agent_cli/errors.py`. | Code | Description | |---|---| | `PAYG_NOT_AVAILABLE` | The project does not have the `pay-as-you-go` feature, so it has no credit balance; the billing host may not even resolve on this stack | + +### Merge requests + +| Code | Description | +|---|---| +| `MR_NOT_READY_TO_MERGE` | Merge answered 409 with `storage.mergeRequests.notReadyToMerge`: a project merge lock is held, the MR is in a state that cannot merge, or another MR in the project is already processing. Transient -- retryable | +| `MR_MERGE_CONFLICT` | Merge answered 409 without a machine code: configurations changed on both branches. Not retryable -- resolve the conflicts (`kbagent merge-request conflicts`) and merge again | diff --git a/src/keboola_agent_cli/client/merge_requests.py b/src/keboola_agent_cli/client/merge_requests.py index 4cf0fb02..db580aaa 100644 --- a/src/keboola_agent_cli/client/merge_requests.py +++ b/src/keboola_agent_cli/client/merge_requests.py @@ -129,7 +129,7 @@ class MergeRequests: branch-prefixed; all bodies are JSON (module docstring). Returns are raw parsed JSON, as everywhere in ``client/``. - The pre-flight feature check (``has_feature(FEATURE_BRANCHES_MERGE_REQUESTS)``) + The pre-flight feature check (``has_feature(BRANCHES_MERGE_REQUESTS_FEATURE)``) is deliberately NOT done here -- a missing feature surfaces as a 403 byte-for-byte identical to a role denial, so only a Layer 2 pre-flight can word the error. diff --git a/src/keboola_agent_cli/constants.py b/src/keboola_agent_cli/constants.py index ffc809af..4583523f 100644 --- a/src/keboola_agent_cli/constants.py +++ b/src/keboola_agent_cli/constants.py @@ -482,12 +482,18 @@ def _resolve_app_name() -> str: # --- Merge Requests (Branches 2.0) --- # Feature flag gating the non-SOX merge-request flow. Layer 3 # (client/merge_requests.py) does no feature check itself -- a missing -# feature is a 403 identical to a role denial -- so the Part 2 service layer -# must call has_feature() with this constant before writes and word the error. It -# also doubles as the SOX fence: server-side, `protected-default-branch` -# passes the same gate, so checking for this flag specifically keeps SOX -# projects out of a flow whose approvals semantics kbagent does not cover. -FEATURE_BRANCHES_MERGE_REQUESTS: str = "branches-merge-requests" +# feature is a 403 identical to a role denial -- so the service layer calls +# has_feature() with this constant before writes and words the error. It +# also doubles as the SOX fence: server-side, the six MR writes accept +# `protected-default-branch` OR `branches-merge-requests` (only /rebase +# requires this flag specifically), so checking for this flag keeps SOX +# projects out of a flow whose approvals semantics kbagent does not cover -- +# but the fence holds ONLY as long as a SOX project never also carries +# `branches-merge-requests`. The pre-flight is thus deliberately stricter +# than the server for a project with only `protected-default-branch`. +# (Renamed from FEATURE_BRANCHES_MERGE_REQUESTS to match this file's +# dominant `..._FEATURE` suffix convention; decided in DMD-1899.) +BRANCHES_MERGE_REQUESTS_FEATURE: str = "branches-merge-requests" # --- Global Search --- # Feature flag that gates the Storage API ``GET /v2/storage/global-search`` diff --git a/src/keboola_agent_cli/errors.py b/src/keboola_agent_cli/errors.py index 6e3d4993..018dd57c 100644 --- a/src/keboola_agent_cli/errors.py +++ b/src/keboola_agent_cli/errors.py @@ -142,6 +142,14 @@ class ErrorCode(StrEnum): # Billing / Pay-As-You-Go (since #594) PAYG_NOT_AVAILABLE = "PAYG_NOT_AVAILABLE" + # Merge requests (DMD-1899). The merge 409 has four causes in two wire + # shapes; these split exactly where the backend does (see + # docs/merge-requests-notes.md). Mapped in MergeRequestService.merge() -- + # only the service knows the 409 came from the merge endpoint, and the + # conflict shape carries no machine string code for http_base to key on. + MR_NOT_READY_TO_MERGE = "MR_NOT_READY_TO_MERGE" + MR_MERGE_CONFLICT = "MR_MERGE_CONFLICT" + def mask_token(token: str) -> str: """Mask a Keboola Storage API token for safe display. @@ -334,6 +342,8 @@ def __init__(self, feature: str, *, remedy: str = "") -> None: # A refused-by-us safety guard, not an upstream fault: nothing was sent to # the API, and the caller fixes it by re-issuing the request with --force. ErrorCode.WORKSPACE_LOAD_COPY_TOO_LARGE: "validation", + ErrorCode.MR_NOT_READY_TO_MERGE: "conflict", + ErrorCode.MR_MERGE_CONFLICT: "conflict", } diff --git a/src/keboola_agent_cli/http_base.py b/src/keboola_agent_cli/http_base.py index 6bf97432..6634634d 100644 --- a/src/keboola_agent_cli/http_base.py +++ b/src/keboola_agent_cli/http_base.py @@ -409,6 +409,7 @@ def _raise_api_error( url_label = base_url or self._base_url exception_id = "" + details: dict = {} try: body = response.json() # Keboola answers a 5xx with a generic `error` ("Application @@ -417,6 +418,14 @@ def _raise_api_error( # left the operator with nothing to escalate (issue #599). if isinstance(body, dict): exception_id = self._safe_exception_id(body.get("exceptionId")) + # Keboola user errors also carry a machine-readable string + # `code` (e.g. `storage.mergeRequests.notReadyToMerge`). + # Surface it in details so a service can branch on it -- the + # message alone holds only the human `error` text (DMD-1899; + # the merge 409's two shapes differ exactly by this field). + api_error_code = body.get("code") + if isinstance(api_error_code, str) and api_error_code: + details["api_error_code"] = api_error_code # Real Keboola APIs answer with one of these keys in priority # order. Two caveats: # 1. Keboola Metastore puts the HTTP status code into `error` @@ -454,6 +463,7 @@ def _raise_api_error( status_code=status, error_code=ErrorCode.INVALID_TOKEN, retryable=False, + details=details, ) if status == 403: @@ -462,6 +472,7 @@ def _raise_api_error( status_code=status, error_code=ErrorCode.ACCESS_DENIED, retryable=False, + details=details, ) if status == 404: @@ -470,6 +481,7 @@ def _raise_api_error( status_code=status, error_code=ErrorCode.NOT_FOUND, retryable=False, + details=details, ) # Appended AFTER the truncation above so they always survive into the @@ -489,4 +501,5 @@ def _raise_api_error( status_code=status, error_code=ErrorCode.API_ERROR, retryable=status in RETRYABLE_STATUS_CODES if retryable is None else retryable, + details=details, ) diff --git a/tests/test_http_base.py b/tests/test_http_base.py index 7cf97bff..2c6b9cd1 100644 --- a/tests/test_http_base.py +++ b/tests/test_http_base.py @@ -1056,3 +1056,69 @@ def test_context_manager(self, httpx_mock) -> None: ) as client: response = client._do_request("GET", "/test-path") assert response.status_code == 200 + + +class TestApiErrorCodeDetails: + """The body's machine string `code` must survive into KeboolaApiError.details.""" + + def _client(self) -> BaseHttpClient: + return BaseHttpClient( + base_url=STACK_URL, + token=TOKEN, + headers={"X-StorageApi-Token": TOKEN}, + ) + + def test_body_code_lands_in_details(self, httpx_mock) -> None: + # The merge 409's "not ready" shape -- the message carries only the + # human `error` text, so `code` in details is the ONLY machine handle + # (MergeRequestService.merge() branches on it, DMD-1899). + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", + status_code=409, + json={ + "error": "Cannot merge, another merge request is processing.", + "code": "storage.mergeRequests.notReadyToMerge", + }, + ) + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("GET", "/test-path") + assert ( + exc_info.value.details["api_error_code"] + == "storage.mergeRequests.notReadyToMerge" + ) + finally: + client.close() + + def test_no_code_means_no_details_key(self, httpx_mock) -> None: + # The merge 409's conflict shape carries no string code -- details + # must not invent one. + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", + status_code=409, + json={"error": "Configuration was changed in the default branch."}, + ) + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("GET", "/test-path") + assert "api_error_code" not in exc_info.value.details + finally: + client.close() + + def test_non_string_code_ignored(self, httpx_mock) -> None: + # Keboola Metastore puts an int HTTP status into `error`; guard the + # same way against a non-string `code`. + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", + status_code=404, + json={"error": "not found", "code": 404}, + ) + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("GET", "/test-path") + assert "api_error_code" not in exc_info.value.details + finally: + client.close() From 4a2e6831832e870aaffe1c0c3b1bbe3f958058a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C5=A0ifra?= Date: Wed, 26 Aug 2026 03:32:31 +0200 Subject: [PATCH 04/15] feat(service): MR status-derivation polyfill + admin identity on verify [DMD-1899] The four derivates from the L2 RFC ('Derived status'), as pure module-level functions in the new services/merge_request_service.py: derive_state (the UI list badge's decision table incl. the rejected / closed-by-creator reviewer overrides), derive_merge_blockers (a list, so concurrent blockers don't mask each other; conflicts=None means not-fetched, not conflict-free), derive_allowed_actions (state-only; roles/features stay with the pre-flight), derive_viewer (is_creator / has_approved relative to the caller). All four are a POLYFILL: they read the future server-serialized field first (derivedState / mergeBlockers / allowedActions / viewer -- Connection issue DMD-1988) and fall back to the local tables; the fallbacks get deleted when DMD-1988 lands. Same defensive-read pattern as changeLog and the DMD-1969 approvals count. verify_token now also parses the response's top-level admin block into TokenVerifyResponse.admin_id/admin_name (additive; absent for scoped tokens) -- the anchor derive_viewer compares creator.id and approverId against. approverId is a string on the wire, ids are ints; comparisons normalize via str() like the UI does. Co-Authored-By: Claude Fable 5 --- src/keboola_agent_cli/client/tokens.py | 11 ++ src/keboola_agent_cli/models.py | 12 ++ .../services/merge_request_service.py | 179 +++++++++++++++++ tests/test_merge_request_service.py | 181 ++++++++++++++++++ 4 files changed, 383 insertions(+) create mode 100644 src/keboola_agent_cli/services/merge_request_service.py create mode 100644 tests/test_merge_request_service.py diff --git a/src/keboola_agent_cli/client/tokens.py b/src/keboola_agent_cli/client/tokens.py index 9c354857..6128ed4f 100644 --- a/src/keboola_agent_cli/client/tokens.py +++ b/src/keboola_agent_cli/client/tokens.py @@ -41,6 +41,15 @@ def verify_token(self) -> TokenVerifyResponse: org_id = int(org_id_raw) if org_id_raw is not None else None except (TypeError, ValueError): org_id = None + # Top-level `admin` block: present for admin tokens only (a scoped + # token has no admin identity behind it). Its id is the caller's + # user id -- the anchor for viewer-relative MR derivations. + admin = data.get("admin") or {} + admin_id_raw = admin.get("id") + try: + admin_id = int(admin_id_raw) if admin_id_raw is not None else None + except (TypeError, ValueError): + admin_id = None response = TokenVerifyResponse( token_id=str(data.get("id", "")), token_description=data.get("description", ""), @@ -55,6 +64,8 @@ def verify_token(self) -> TokenVerifyResponse: # the id (e.g. "#73") as a fallback until `org setup` fills # in the human-readable name. org_name=None, + admin_id=admin_id, + admin_name=admin.get("name"), ) # Refresh the features cache on every successful verify so explicit # callers stay consistent with the cached view used by has_feature(). diff --git a/src/keboola_agent_cli/models.py b/src/keboola_agent_cli/models.py index d48f91b8..57d489db 100644 --- a/src/keboola_agent_cli/models.py +++ b/src/keboola_agent_cli/models.py @@ -279,6 +279,18 @@ class TokenVerifyResponse(BaseModel): default=None, description="Organization name parsed from owner.organization (when present)", ) + admin_id: int | None = Field( + default=None, + description=( + "Admin (user) ID behind the token, from the verify response's top-level " + "`admin` block. Present only for admin tokens; scoped tokens have no admin " + "identity. Used for viewer-relative merge-request derivations (DMD-1899)." + ), + ) + admin_name: str | None = Field( + default=None, + description="Admin (user) display name, when the `admin` block is present", + ) class ComponentDetail(BaseModel): diff --git a/src/keboola_agent_cli/services/merge_request_service.py b/src/keboola_agent_cli/services/merge_request_service.py new file mode 100644 index 00000000..cc3a38f7 --- /dev/null +++ b/src/keboola_agent_cli/services/merge_request_service.py @@ -0,0 +1,179 @@ +"""Merge-request service -- MR lifecycle, status derivation, error mapping (DMD-1899). + +Business logic for the future ``kbagent merge-request`` command group, over the +Layer 3 namespace ``client.merge_requests`` (shipped in #556) and the config +diff/rebase endpoints in ``client/configs.py``. Scope is the **non-SOX** flow +(``branches-merge-requests``); design record: ``docs/merge-requests-layer2.md``. + +The module-level functions below are the **status-derivation polyfill**: the +derived vocabulary (``derived_state`` / ``merge_blockers`` / ``allowed_actions`` +/ ``viewer``) belongs on the backend so the UI, this CLI, and the MCP tools +consume one evaluation instead of re-deriving it three times (the way GitHub +serializes ``mergeable_state`` / ``reviewDecision`` / ``viewer*``). Connection +tracks that as DMD-1988; until it lands, every function here reads the future +serialized field FIRST and only falls back to the local decision table -- +delete the fallbacks when DMD-1988 ships. The canonical decision tables live +in the L2 RFC and in DMD-1988; the local logic is a port of the UI list badge +(``kbc-ui .../merge-requests/components/MergeRequestRow.tsx`` + ``helpers.ts``). +""" + +from __future__ import annotations + +import logging +from typing import Any + +logger = logging.getLogger(__name__) + +# Raw lifecycle state -> derived_state, before the reviewer-based overrides. +# `published`/`canceled` get client-facing names matching the UI list badge +# ("Merged"/"Closed"); `in_merge` is named even though the UI badge omits it. +_DERIVED_STATE_BY_RAW: dict[str, str] = { + "development": "in_development", + "in_review": "in_review", + "approved": "approved", + "in_merge": "in_merge", + "published": "merged", + "canceled": "closed", +} + +# Mechanical action availability per raw state -- what the UI buttons gate on +# (approve/request-changes only in in_review|approved, send-for-review only in +# development; editing/rebasing is allowed in development, in_review and +# approved -- the dev branch is locked only while in_merge). `merge` appears +# for `development` because a non-SOX merge from there succeeds when approvals +# suffice (the backend auto-applies skip_review). Deliberately state-only: +# roles and features are the pre-flight's job client-side, and the backend can +# honor them once it serializes `allowedActions` (DMD-1988). +_ALLOWED_ACTIONS_BY_STATE: dict[str, tuple[str, ...]] = { + "development": ("request_review", "merge", "update", "resolve_conflicts"), + "in_review": ("approve", "request_changes", "update", "resolve_conflicts"), + "approved": ("approve", "request_changes", "merge", "update", "resolve_conflicts"), + "in_merge": (), + "published": (), + "canceled": (), +} + + +def _same_id(a: Any, b: Any) -> bool: + """Compare two ids that may arrive as int or str (approverId is a string + on the wire while creator.id and reviewer.id are numbers).""" + return a is not None and b is not None and str(a) == str(b) + + +def _creator_id(mr: dict[str, Any]) -> Any: + return (mr.get("creator") or {}).get("id") + + +def derive_state(mr: dict[str, Any]) -> str: + """Derive the client-facing lifecycle state of a merge request. + + Server-first: prefers a serialized ``derivedState`` when Connection ships + it (DMD-1988); the local fallback is the UI list badge's decision table, + evaluated in order: + + - ``rejected``: ``development`` + a non-creator reviewer with + ``status=rejected``. + - ``closed``: ``canceled``, or ``development`` + the creator's + self-rejection -- the UI "cancel" action reuses request-changes under + the hood, so a self-closed MR keeps ``state=development``. + - otherwise the raw state mapped through ``_DERIVED_STATE_BY_RAW`` + (an unknown raw state passes through unchanged, defensively). + """ + server = mr.get("derivedState") + if isinstance(server, str) and server: + return server + + state = mr.get("state") or "" + creator_id = _creator_id(mr) + non_creator_rejected = False + creator_self_rejected = False + for reviewer in mr.get("reviewers") or []: + if reviewer.get("status") != "rejected": + continue + if _same_id(reviewer.get("id"), creator_id): + creator_self_rejected = True + else: + non_creator_rejected = True + + if state == "development" and non_creator_rejected: + return "rejected" + if state == "canceled" or (state == "development" and creator_self_rejected): + return "closed" + return _DERIVED_STATE_BY_RAW.get(state, state) + + +def derive_merge_blockers(mr: dict[str, Any], conflicts: list[dict[str, Any]] | None) -> list[str]: + """Derive what currently blocks ``merge`` -- a list, so concurrent + blockers don't mask each other (unlike GitHub's single-valued + ``mergeable_state``). + + Server-first (``mergeBlockers``, DMD-1988). Local fallback, in + deterministic order: + + - ``conflicts``: the live conflicts list is non-empty. Pass ``None`` when + conflicts were not fetched (list rows) -- absence of data is not + absence of conflicts, so ``None`` simply skips the check. + - ``approvals``: ``state == in_review`` -- the state machine collapses + the requirement (insufficient approvals is the only way to sit there). + No count is reported until Connection serializes it (DMD-1969). + - ``state``: ``in_merge`` / ``published`` / ``canceled`` -- merge is not + applicable. + + Purely informational, NOT a guard: the backend stays the authority via + the merge 409 (conflicts are validated live on every attempt). Note a + ``rejected`` MR has no blocker -- it sits in ``development`` and a + non-SOX merge from there succeeds (auto skip_review); the story is told + by ``derived_state``. + """ + server = mr.get("mergeBlockers") + if isinstance(server, list): + return [str(blocker) for blocker in server] + + state = mr.get("state") or "" + blockers: list[str] = [] + if conflicts: + blockers.append("conflicts") + if state == "in_review": + blockers.append("approvals") + if state in ("in_merge", "published", "canceled"): + blockers.append("state") + return blockers + + +def derive_allowed_actions(mr: dict[str, Any]) -> list[str]: + """Derive which MR actions the current state mechanically allows. + + Server-first (``allowedActions``, DMD-1988). The local fallback is + state-only (see ``_ALLOWED_ACTIONS_BY_STATE``); an unknown state yields + an empty list rather than guessing. + """ + server = mr.get("allowedActions") + if isinstance(server, list): + return [str(action) for action in server] + return list(_ALLOWED_ACTIONS_BY_STATE.get(mr.get("state") or "", ())) + + +def derive_viewer(mr: dict[str, Any], admin_id: int | None) -> dict[str, bool | None]: + """Derive the caller-relative flags: am I the creator, did I approve. + + ``admin_id`` is the caller's user id from ``verify_token`` (the response's + ``admin`` block); a scoped token has none, in which case both flags are + ``None`` -- honest "unknown", not ``False``. Server-first (``viewer`` + with ``isCreator``/``hasApproved``, DMD-1988). These flags are what turns + a blocker into a next step: ``approvals`` + ``has_approved=True`` means + "wait for the other reviewers", not "approve it". + """ + server = mr.get("viewer") + if isinstance(server, dict) and ("isCreator" in server or "hasApproved" in server): + return { + "is_creator": server.get("isCreator"), + "has_approved": server.get("hasApproved"), + } + + if admin_id is None: + return {"is_creator": None, "has_approved": None} + is_creator = _same_id(_creator_id(mr), admin_id) + has_approved = any( + _same_id(approval.get("approverId"), admin_id) for approval in mr.get("approvals") or [] + ) + return {"is_creator": is_creator, "has_approved": has_approved} diff --git a/tests/test_merge_request_service.py b/tests/test_merge_request_service.py new file mode 100644 index 00000000..812124e9 --- /dev/null +++ b/tests/test_merge_request_service.py @@ -0,0 +1,181 @@ +"""Tests for MergeRequestService and the status-derivation polyfill (DMD-1899). + +The derivation tables are the canonical spec from the L2 RFC +(docs/merge-requests-layer2.md, "Derived status") and DMD-1988 -- a port of +the UI list badge. When Connection serializes the fields, the server-first +tests keep passing and the fallback tests get deleted with the fallbacks. +""" + +from __future__ import annotations + +from typing import Any + +from keboola_agent_cli.services.merge_request_service import ( + derive_allowed_actions, + derive_merge_blockers, + derive_state, + derive_viewer, +) + +CREATOR = {"id": 42, "name": "Martin"} + + +def _mr( + state: str = "development", + reviewers: list[dict[str, Any]] | None = None, + approvals: list[dict[str, Any]] | None = None, + **extra: Any, +) -> dict[str, Any]: + return { + "id": 7, + "state": state, + "creator": CREATOR, + "reviewers": reviewers or [], + "approvals": approvals or [], + **extra, + } + + +class TestDeriveState: + def test_plain_states_map_to_client_vocabulary(self) -> None: + assert derive_state(_mr("development")) == "in_development" + assert derive_state(_mr("in_review")) == "in_review" + assert derive_state(_mr("approved")) == "approved" + assert derive_state(_mr("in_merge")) == "in_merge" + assert derive_state(_mr("published")) == "merged" + assert derive_state(_mr("canceled")) == "closed" + + def test_rejected_needs_a_non_creator_rejection_in_development(self) -> None: + mr = _mr("development", reviewers=[{"id": 99, "name": "R", "status": "rejected"}]) + assert derive_state(mr) == "rejected" + + def test_creator_self_rejection_is_closed_not_rejected(self) -> None: + # The UI "cancel" action reuses request-changes: the creator rejects + # their own MR and it stays in development. + mr = _mr("development", reviewers=[{"id": 42, "name": "Martin", "status": "rejected"}]) + assert derive_state(mr) == "closed" + + def test_rejection_outside_development_does_not_override(self) -> None: + # A stale rejected reviewer entry must not relabel a re-submitted MR. + mr = _mr("in_review", reviewers=[{"id": 99, "status": "rejected"}]) + assert derive_state(mr) == "in_review" + + def test_non_creator_rejection_wins_over_self_rejection(self) -> None: + # RFC order: rejected is evaluated before closed. + mr = _mr( + "development", + reviewers=[ + {"id": 42, "status": "rejected"}, + {"id": 99, "status": "rejected"}, + ], + ) + assert derive_state(mr) == "rejected" + + def test_unknown_raw_state_passes_through(self) -> None: + assert derive_state(_mr("some_future_state")) == "some_future_state" + + def test_server_field_wins_over_local_derivation(self) -> None: + # Polyfill contract: once DMD-1988 serializes derivedState, the local + # table must never contradict it. + mr = _mr("development", derivedState="rejected") + assert derive_state(mr) == "rejected" + + def test_reviewer_without_status_is_pending_not_rejection(self) -> None: + mr = _mr("development", reviewers=[{"id": 99, "name": "R", "status": None}]) + assert derive_state(mr) == "in_development" + + +class TestDeriveMergeBlockers: + def test_open_mr_without_conflicts_is_mergeable(self) -> None: + assert derive_merge_blockers(_mr("development"), conflicts=[]) == [] + assert derive_merge_blockers(_mr("approved"), conflicts=[]) == [] + + def test_conflicts_block(self) -> None: + conflicts = [{"componentId": "keboola.snowflake-transformation", "configurationId": "1"}] + assert derive_merge_blockers(_mr("development"), conflicts) == ["conflicts"] + + def test_in_review_blocks_on_approvals(self) -> None: + assert derive_merge_blockers(_mr("in_review"), conflicts=[]) == ["approvals"] + + def test_concurrent_blockers_do_not_mask_each_other(self) -> None: + conflicts = [{"componentId": "c", "configurationId": "1"}] + assert derive_merge_blockers(_mr("in_review"), conflicts) == ["conflicts", "approvals"] + + def test_terminal_and_transient_states_block_on_state(self) -> None: + for state in ("in_merge", "published", "canceled"): + assert derive_merge_blockers(_mr(state), conflicts=[]) == ["state"] + + def test_none_conflicts_means_not_fetched_not_conflict_free(self) -> None: + # List rows don't fetch conflicts; absence of data must not assert + # "no conflicts". + assert derive_merge_blockers(_mr("development"), conflicts=None) == [] + + def test_rejected_mr_has_no_blocker(self) -> None: + # It sits in development; a non-SOX merge from there succeeds + # (auto skip_review). derived_state tells the story instead. + mr = _mr("development", reviewers=[{"id": 99, "status": "rejected"}]) + assert derive_merge_blockers(mr, conflicts=[]) == [] + + def test_server_field_wins(self) -> None: + mr = _mr("development", mergeBlockers=["approvals"]) + assert derive_merge_blockers(mr, conflicts=[]) == ["approvals"] + + +class TestDeriveAllowedActions: + def test_development_offers_submit_and_direct_merge(self) -> None: + assert derive_allowed_actions(_mr("development")) == [ + "request_review", + "merge", + "update", + "resolve_conflicts", + ] + + def test_review_states_gate_approve_and_request_changes(self) -> None: + # Mirrors the UI buttons: approve/request-changes only in + # in_review|approved. + assert "approve" in derive_allowed_actions(_mr("in_review")) + assert "approve" in derive_allowed_actions(_mr("approved")) + assert "approve" not in derive_allowed_actions(_mr("development")) + + def test_merge_not_offered_from_in_review(self) -> None: + # in_review means approvals are insufficient by definition; the + # moment they suffice the backend auto-transitions to approved. + assert "merge" not in derive_allowed_actions(_mr("in_review")) + + def test_locked_and_terminal_states_offer_nothing(self) -> None: + for state in ("in_merge", "published", "canceled"): + assert derive_allowed_actions(_mr(state)) == [] + + def test_unknown_state_offers_nothing(self) -> None: + assert derive_allowed_actions(_mr("some_future_state")) == [] + + def test_server_field_wins(self) -> None: + mr = _mr("published", allowedActions=["merge"]) + assert derive_allowed_actions(mr) == ["merge"] + + +class TestDeriveViewer: + def test_creator_is_flagged(self) -> None: + viewer = derive_viewer(_mr(), admin_id=42) + assert viewer == {"is_creator": True, "has_approved": False} + + def test_approver_is_flagged(self) -> None: + # approverId is a string on the wire while admin ids are ints -- the + # comparison must not care (the UI does String(id) for the same + # reason). + mr = _mr("in_review", approvals=[{"approverId": "77", "approverName": "R"}]) + viewer = derive_viewer(mr, admin_id=77) + assert viewer == {"is_creator": False, "has_approved": True} + + def test_no_admin_identity_means_unknown_not_false(self) -> None: + # A scoped token has no admin block; None is honest, False would lie. + viewer = derive_viewer(_mr(), admin_id=None) + assert viewer == {"is_creator": None, "has_approved": None} + + def test_approval_without_approver_id_is_ignored(self) -> None: + mr = _mr("in_review", approvals=[{"approverId": None, "approverName": "gone"}]) + assert derive_viewer(mr, admin_id=42)["has_approved"] is False + + def test_server_field_wins(self) -> None: + mr = _mr(viewer={"isCreator": False, "hasApproved": True}) + assert derive_viewer(mr, admin_id=42) == {"is_creator": False, "has_approved": True} From bb515f5668a2d371bdbbeb4d75f05139a5ef07d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C5=A0ifra?= Date: Wed, 26 Aug 2026 03:34:25 +0200 Subject: [PATCH 05/15] feat(service): MergeRequestService reads -- list, find-by-branch, detail [DMD-1899] The service class (BaseService DI: ConfigStore + client_factory) and its three read methods: - list_merge_requests: rows enriched with derived_state; --state filters client-side (the endpoint has no query params) and matches the derived vocabulary (rejected/merged/closed/...) as well as raw states. - find_merge_request_for_branch: the branch->MR resolver behind L1's optional --mr-id (a branch has at most one MR ever, so the match is unambiguous); no MR -> NOT_FOUND naming merge-request create as the next step. - get_merge_request: detail with the full derived status -- merge_blockers/ mergeable/allowed_actions/viewer + the live conflicts list, fetched only for open MRs (a published/canceled MR's source branch is deleted, the conflicts endpoint is moot). viewer anchors on verify_token's admin id; a scoped token yields honest None flags. Derivations stay informational: the merge 409 remains the authority. Also the write pre-flight helper (_require_merge_requests_feature) with the SOX-fence assumption spelled out, used by the write methods that follow. Co-Authored-By: Claude Fable 5 --- .../services/merge_request_service.py | 162 +++++++++++++++ tests/test_merge_request_service.py | 186 ++++++++++++++++++ 2 files changed, 348 insertions(+) diff --git a/src/keboola_agent_cli/services/merge_request_service.py b/src/keboola_agent_cli/services/merge_request_service.py index cc3a38f7..a875dffd 100644 --- a/src/keboola_agent_cli/services/merge_request_service.py +++ b/src/keboola_agent_cli/services/merge_request_service.py @@ -22,6 +22,13 @@ import logging from typing import Any +from ..client import KeboolaClient +from ..config_store import ConfigError +from ..constants import BRANCHES_MERGE_REQUESTS_FEATURE +from ..errors import ErrorCode, KeboolaApiError +from ..models import ProjectConfig +from .base import BaseService + logger = logging.getLogger(__name__) # Raw lifecycle state -> derived_state, before the reviewer-based overrides. @@ -177,3 +184,158 @@ def derive_viewer(mr: dict[str, Any], admin_id: int | None) -> dict[str, bool | _same_id(approval.get("approverId"), admin_id) for approval in mr.get("approvals") or [] ) return {"is_creator": is_creator, "has_approved": has_approved} + + +def _enrich_row(mr: dict[str, Any]) -> dict[str, Any]: + """List-row enrichment: raw MR + derived_state (conflicts are not fetched + per row, so no blockers here -- detail-level enrichment does that).""" + return {**mr, "derived_state": derive_state(mr)} + + +class MergeRequestService(BaseService): + """Business logic for the non-SOX merge-request lifecycle. + + Single-project operations over ``client.merge_requests`` (Layer 3, #556). + Reads pass through with derived-status enrichment; writes run the + ``branches-merge-requests`` pre-flight first, because a missing feature + surfaces as a 403 byte-for-byte identical to a role denial and only a + client-side check can word the real error. + + Uses dependency injection for config_store and client_factory. + """ + + # States whose source branch still exists and whose conflicts endpoint is + # meaningful; for published/canceled MRs the branch is deleted and + # branchFromId is null. + _OPEN_STATES = ("development", "in_review", "approved") + + def _project(self, alias: str) -> ProjectConfig: + return self.resolve_projects([alias])[alias] + + def _require_merge_requests_feature(self, client: KeboolaClient) -> None: + """Refuse a write early when the project lacks the non-SOX MR feature. + + Server-side, the six MR writes accept `protected-default-branch` OR + `branches-merge-requests` (only /rebase requires the latter), so this + pre-flight fences off SOX projects ONLY as long as a SOX project + never also carries `branches-merge-requests` -- and it is deliberately + stricter than the server for a project with only + `protected-default-branch` (SOX approvals semantics are out of + kbagent's scope). See docs/merge-requests-layer2.md. + """ + if client.has_feature(BRANCHES_MERGE_REQUESTS_FEATURE): + return + raise ConfigError( + "Merge requests are not enabled on this project: the " + f"'{BRANCHES_MERGE_REQUESTS_FEATURE}' feature is missing. Without this " + "pre-flight the API would answer an unexplained 403 (identical to a role " + "denial). Note: kbagent deliberately refuses SOX projects that carry only " + "'protected-default-branch', although the server would accept some writes -- " + "the SOX approvals flow is not supported by this CLI." + ) + + # -- Reads ---------------------------------------------------------------- + + def list_merge_requests(self, alias: str, state: str | None = None) -> dict[str, Any]: + """List the project's merge requests, each with ``derived_state``. + + ``state`` filters client-side (the endpoint declares no query + parameters): a value matching either the derived vocabulary + (``rejected``, ``merged``, ``closed``, ...) or a raw lifecycle state + (``published``, ...) keeps the row; matching is case-insensitive. + """ + project = self._project(alias) + client = self._client_factory(project.stack_url, project.token) + try: + rows = [_enrich_row(mr) for mr in client.merge_requests.list()] + finally: + client.close() + + if state is not None: + wanted = state.lower() + rows = [ + mr + for mr in rows + if wanted in ((mr.get("state") or "").lower(), mr["derived_state"].lower()) + ] + + result: dict[str, Any] = { + "alias": alias, + "count": len(rows), + "merge_requests": rows, + } + if state is not None: + result["state_filter"] = state + return result + + def find_merge_request_for_branch(self, alias: str, branch_id: int) -> dict[str, Any]: + """Resolve a dev branch to its merge request (a branch has at most + one MR, ever -- the backend's existence check has no state filter). + + The list endpoint cannot filter server-side, so this lists and + matches ``branches.branchFromId`` client-side. Raises ``NOT_FOUND`` + when the branch has no MR, with the create command as the next step. + """ + project = self._project(alias) + client = self._client_factory(project.stack_url, project.token) + try: + for mr in client.merge_requests.list(): + if (mr.get("branches") or {}).get("branchFromId") == branch_id: + return {"alias": alias, **_enrich_row(mr)} + finally: + client.close() + raise KeboolaApiError( + message=( + f"Branch {branch_id} has no merge request in project '{alias}'. " + "Create one with `kbagent merge-request create`." + ), + status_code=404, + error_code=ErrorCode.NOT_FOUND, + retryable=False, + ) + + def get_merge_request( + self, + alias: str, + merge_request_id: int, + include_activity_log: bool = False, + ) -> dict[str, Any]: + """Get an MR's detail with the full derived status. + + On top of the raw payload: ``derived_state``, ``merge_blockers`` + + ``mergeable``, ``allowed_actions``, ``viewer`` and -- for open MRs -- + the live ``conflicts`` list (skipped for published/canceled/in_merge, + where the source branch is gone or locked and the readiness question + is moot). The derivations are informational; the merge 409 stays the + authority. + """ + project = self._project(alias) + client = self._client_factory(project.stack_url, project.token) + try: + mr = client.merge_requests.get( + merge_request_id, include_activity_log=include_activity_log + ) + conflicts: list[dict[str, Any]] | None = None + if (mr.get("state") or "") in self._OPEN_STATES: + conflicts = client.merge_requests.conflicts(merge_request_id) + # verify_token is cheap (and refreshes the features cache); its + # admin block anchors the viewer-relative flags. A scoped token + # has no admin identity -> viewer flags are None, not False. + admin_id = client.verify_token().admin_id + finally: + client.close() + + blockers = derive_merge_blockers(mr, conflicts) + detail: dict[str, Any] = { + "alias": alias, + **mr, + "derived_state": derive_state(mr), + "merge_blockers": blockers, + "mergeable": not blockers and conflicts is not None, + "allowed_actions": derive_allowed_actions(mr), + "viewer": derive_viewer(mr, admin_id), + } + if conflicts is not None: + detail["conflicts"] = conflicts + detail["conflicts_count"] = len(conflicts) + return detail diff --git a/tests/test_merge_request_service.py b/tests/test_merge_request_service.py index 812124e9..bc104dd5 100644 --- a/tests/test_merge_request_service.py +++ b/tests/test_merge_request_service.py @@ -179,3 +179,189 @@ def test_approval_without_approver_id_is_ignored(self) -> None: def test_server_field_wins(self) -> None: mr = _mr(viewer={"isCreator": False, "hasApproved": True}) assert derive_viewer(mr, admin_id=42) == {"is_creator": False, "has_approved": True} + + +# -- Service-level tests ------------------------------------------------------ + +from pathlib import Path # noqa: E402 +from unittest.mock import MagicMock # noqa: E402 + +import pytest # noqa: E402 + +from keboola_agent_cli.config_store import ConfigStore # noqa: E402 +from keboola_agent_cli.errors import ErrorCode, KeboolaApiError # noqa: E402 +from keboola_agent_cli.models import ProjectConfig, TokenVerifyResponse # noqa: E402 +from keboola_agent_cli.services.merge_request_service import ( # noqa: E402 + MergeRequestService, +) + +STACK_URL = "https://connection.keboola.com" +TOKEN = "901-55555-fakeTestTokenDoNotUseXXXXXXXX" +ALIAS = "prod" + + +def _verify_response(admin_id: int | None = 42) -> TokenVerifyResponse: + return TokenVerifyResponse( + token_id="901", + token_description="test", + project_id=10, + project_name="Prod", + owner_name="Prod", + admin_id=admin_id, + ) + + +@pytest.fixture +def store(tmp_config_dir: Path) -> ConfigStore: + s = ConfigStore(config_dir=tmp_config_dir) + s.add_project( + ALIAS, + ProjectConfig(stack_url=STACK_URL, token=TOKEN, project_name="Prod", project_id=10), + ) + return s + + +@pytest.fixture +def client_factory() -> tuple[MagicMock, MagicMock]: + mock = MagicMock() + mock.verify_token.return_value = _verify_response() + mock.has_feature.return_value = True + factory = MagicMock(return_value=mock) + return factory, mock + + +def _svc(store: ConfigStore, factory: MagicMock) -> MergeRequestService: + return MergeRequestService(store, client_factory=factory) + + +def _wire_mr( + mr_id: int = 7, + state: str = "development", + branch_from: int | None = 123, + **extra: Any, +) -> dict[str, Any]: + return { + "id": mr_id, + "state": state, + "title": "My MR", + "creator": CREATOR, + "reviewers": [], + "approvals": [], + "branches": {"branchFromId": branch_from, "branchIntoId": 1}, + **extra, + } + + +class TestListMergeRequests: + def test_rows_carry_derived_state(self, store, client_factory) -> None: + factory, mock = client_factory + mock.merge_requests.list.return_value = [ + _wire_mr(1, "published"), + _wire_mr(2, "development"), + ] + result = _svc(store, factory).list_merge_requests(ALIAS) + assert result["count"] == 2 + assert [mr["derived_state"] for mr in result["merge_requests"]] == [ + "merged", + "in_development", + ] + mock.close.assert_called_once() + + def test_state_filter_matches_derived_vocabulary(self, store, client_factory) -> None: + factory, mock = client_factory + mock.merge_requests.list.return_value = [ + _wire_mr(1, "published"), + _wire_mr(2, "development"), + ] + result = _svc(store, factory).list_merge_requests(ALIAS, state="merged") + assert result["count"] == 1 + assert result["merge_requests"][0]["id"] == 1 + assert result["state_filter"] == "merged" + + def test_state_filter_matches_raw_state_too(self, store, client_factory) -> None: + factory, mock = client_factory + mock.merge_requests.list.return_value = [_wire_mr(1, "published")] + result = _svc(store, factory).list_merge_requests(ALIAS, state="published") + assert result["count"] == 1 + + def test_rejected_filter_uses_reviewer_derivation(self, store, client_factory) -> None: + factory, mock = client_factory + rejected = _wire_mr(1, "development", reviewers=[{"id": 99, "status": "rejected"}]) + mock.merge_requests.list.return_value = [rejected, _wire_mr(2, "development")] + result = _svc(store, factory).list_merge_requests(ALIAS, state="rejected") + assert [mr["id"] for mr in result["merge_requests"]] == [1] + + +class TestFindMergeRequestForBranch: + def test_finds_by_branch_from_id(self, store, client_factory) -> None: + factory, mock = client_factory + mock.merge_requests.list.return_value = [ + _wire_mr(1, branch_from=111), + _wire_mr(2, branch_from=222), + ] + result = _svc(store, factory).find_merge_request_for_branch(ALIAS, 222) + assert result["id"] == 2 + assert result["alias"] == ALIAS + assert result["derived_state"] == "in_development" + + def test_branch_without_mr_raises_not_found_with_next_step(self, store, client_factory) -> None: + factory, mock = client_factory + mock.merge_requests.list.return_value = [_wire_mr(1, branch_from=111)] + with pytest.raises(KeboolaApiError) as exc_info: + _svc(store, factory).find_merge_request_for_branch(ALIAS, 999) + assert exc_info.value.error_code == ErrorCode.NOT_FOUND + assert "merge-request create" in exc_info.value.message + + +class TestGetMergeRequest: + def test_open_mr_fetches_conflicts_and_derives_everything(self, store, client_factory) -> None: + factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "in_review") + mock.merge_requests.conflicts.return_value = [{"componentId": "c", "configurationId": "1"}] + detail = _svc(store, factory).get_merge_request(ALIAS, 7) + assert detail["derived_state"] == "in_review" + assert detail["merge_blockers"] == ["conflicts", "approvals"] + assert detail["mergeable"] is False + assert detail["allowed_actions"] == [ + "approve", + "request_changes", + "update", + "resolve_conflicts", + ] + assert detail["viewer"] == {"is_creator": True, "has_approved": False} + assert detail["conflicts_count"] == 1 + mock.merge_requests.conflicts.assert_called_once_with(7) + + def test_mergeable_open_mr(self, store, client_factory) -> None: + factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "approved") + mock.merge_requests.conflicts.return_value = [] + detail = _svc(store, factory).get_merge_request(ALIAS, 7) + assert detail["merge_blockers"] == [] + assert detail["mergeable"] is True + assert detail["conflicts"] == [] + + def test_closed_mr_skips_conflicts_and_is_not_mergeable(self, store, client_factory) -> None: + # The source branch of a published/canceled MR is deleted; the + # conflicts endpoint is moot there and must not be called. + factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "published", branch_from=None) + detail = _svc(store, factory).get_merge_request(ALIAS, 7) + mock.merge_requests.conflicts.assert_not_called() + assert detail["merge_blockers"] == ["state"] + assert detail["mergeable"] is False + assert "conflicts" not in detail + + def test_activity_log_flag_passes_through(self, store, client_factory) -> None: + factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "published", branch_from=None) + _svc(store, factory).get_merge_request(ALIAS, 7, include_activity_log=True) + mock.merge_requests.get.assert_called_once_with(7, include_activity_log=True) + + def test_scoped_token_yields_unknown_viewer(self, store, client_factory) -> None: + factory, mock = client_factory + mock.verify_token.return_value = _verify_response(admin_id=None) + mock.merge_requests.get.return_value = _wire_mr(7, "development") + mock.merge_requests.conflicts.return_value = [] + detail = _svc(store, factory).get_merge_request(ALIAS, 7) + assert detail["viewer"] == {"is_creator": None, "has_approved": None} From fd5a0412ccdf5346d4d583c9f168d2b77acff79a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C5=A0ifra?= Date: Wed, 26 Aug 2026 03:35:36 +0200 Subject: [PATCH 06/15] feat(service): MR create, update and review transitions [DMD-1899] - create_merge_request: resolves the target itself (always the default branch -- the backend rejects any other) and refuses the default branch as source with a readable error instead of the backend's confusing 404. The source branch arrives explicit; L1 resolves it via the house resolve_branch() idiom per the RFC decision. - update_merge_request: None = leave unchanged (the API cannot clear to null; empty string clears description/externalId server-side). - request_review / approve / request_changes: thin transitions with the feature pre-flight; request_changes doubles as the close mechanism (no cancel endpoint exists -- the UI's cancel is this call by the creator, and derived_state renders it as closed). Every write runs _require_merge_requests_feature first; returns are the raw MR enriched with derived_state. Co-Authored-By: Claude Fable 5 --- .../services/merge_request_service.py | 144 ++++++++++++++++++ tests/test_merge_request_service.py | 94 +++++++++++- 2 files changed, 237 insertions(+), 1 deletion(-) diff --git a/src/keboola_agent_cli/services/merge_request_service.py b/src/keboola_agent_cli/services/merge_request_service.py index a875dffd..8043f9c5 100644 --- a/src/keboola_agent_cli/services/merge_request_service.py +++ b/src/keboola_agent_cli/services/merge_request_service.py @@ -339,3 +339,147 @@ def get_merge_request( detail["conflicts"] = conflicts detail["conflicts_count"] = len(conflicts) return detail + + # -- Writes: create / update / review transitions -------------------------- + + def create_merge_request( + self, + alias: str, + branch_from_id: int, + title: str, + description: str | None = None, + reviewer_ids: list[int] | None = None, + auto_merge_strategy: str | None = None, + auto_merge_at: str | None = None, + external_id: str | None = None, + ) -> dict[str, Any]: + """Create a merge request from ``branch_from_id`` into the default branch. + + The target is always the default branch (the backend rejects any + other), so the service resolves its id itself. The source branch is + an explicit parameter -- Layer 1 resolves it via the house + ``resolve_branch()`` idiom (``--branch`` -> ``active_branch_id`` -> + readable error) and the output must state which branch the MR was + created from. A source branch can have at most one MR, ever; a + second create answers 404 server-side. + """ + project = self._project(alias) + client = self._client_factory(project.stack_url, project.token) + try: + self._require_merge_requests_feature(client) + default_branch_id = self._default_branch_id(client, alias) + if branch_from_id == default_branch_id: + raise ConfigError( + f"Branch {branch_from_id} is the default (production) branch of " + f"project '{alias}'. A merge request merges a development branch " + "into it -- create one with `kbagent branch create`, or pass " + "--branch with a dev branch id." + ) + mr = client.merge_requests.create( + branch_from_id=branch_from_id, + branch_into_id=default_branch_id, + title=title, + description=description, + reviewer_ids=reviewer_ids, + auto_merge_strategy=auto_merge_strategy, + auto_merge_at=auto_merge_at, + external_id=external_id, + ) + finally: + client.close() + return { + "alias": alias, + "branch_from_id": branch_from_id, + "branch_into_id": default_branch_id, + **_enrich_row(mr), + } + + def update_merge_request( + self, + alias: str, + merge_request_id: int, + title: str | None = None, + description: str | None = None, + reviewer_ids: list[int] | None = None, + auto_merge_strategy: str | None = None, + auto_merge_at: str | None = None, + external_id: str | None = None, + ) -> dict[str, Any]: + """Update an MR's metadata. ``None`` = leave unchanged (the API cannot + clear a field to null; an empty string clears description/externalId + server-side).""" + project = self._project(alias) + client = self._client_factory(project.stack_url, project.token) + try: + self._require_merge_requests_feature(client) + mr = client.merge_requests.update( + merge_request_id, + title=title, + description=description, + reviewer_ids=reviewer_ids, + auto_merge_strategy=auto_merge_strategy, + auto_merge_at=auto_merge_at, + external_id=external_id, + ) + finally: + client.close() + return {"alias": alias, **_enrich_row(mr)} + + def request_review(self, alias: str, merge_request_id: int) -> dict[str, Any]: + """Move the MR from ``development`` to review. + + With the non-SOX default of 0 required approvals the MR lands + straight in ``approved`` (the backend auto-applies finish_review) -- + and a merge from ``development`` skips this step entirely, so the + happy path never needs it. + """ + project = self._project(alias) + client = self._client_factory(project.stack_url, project.token) + try: + self._require_merge_requests_feature(client) + mr = client.merge_requests.request_review(merge_request_id) + finally: + client.close() + return {"alias": alias, **_enrich_row(mr)} + + def approve(self, alias: str, merge_request_id: int) -> dict[str, Any]: + """Add the caller's approval.""" + project = self._project(alias) + client = self._client_factory(project.stack_url, project.token) + try: + self._require_merge_requests_feature(client) + mr = client.merge_requests.approve(merge_request_id) + finally: + client.close() + return {"alias": alias, **_enrich_row(mr)} + + def request_changes( + self, alias: str, merge_request_id: int, reason: str | None = None + ) -> dict[str, Any]: + """Send the MR back to ``development`` (approvals are deleted). + + Also the closest thing to closing an MR: the backend has no cancel + endpoint, and the UI's "cancel" is exactly this call made by the + creator on their own MR (rendered as Closed; derived_state mirrors + that). ``reason`` is capped at 1000 characters server-side. + """ + project = self._project(alias) + client = self._client_factory(project.stack_url, project.token) + try: + self._require_merge_requests_feature(client) + mr = client.merge_requests.request_changes(merge_request_id, reason=reason) + finally: + client.close() + return {"alias": alias, **_enrich_row(mr)} + + def _default_branch_id(self, client: KeboolaClient, alias: str) -> int: + """Resolve the project's default branch id (the only legal MR target).""" + for branch in client.list_dev_branches(): + if branch.get("isDefault"): + return int(branch["id"]) + raise KeboolaApiError( + message=f"Project '{alias}' reports no default branch -- cannot target a merge request.", + status_code=0, + error_code=ErrorCode.API_ERROR, + retryable=False, + ) diff --git a/tests/test_merge_request_service.py b/tests/test_merge_request_service.py index bc104dd5..89ecd72e 100644 --- a/tests/test_merge_request_service.py +++ b/tests/test_merge_request_service.py @@ -188,7 +188,7 @@ def test_server_field_wins(self) -> None: import pytest # noqa: E402 -from keboola_agent_cli.config_store import ConfigStore # noqa: E402 +from keboola_agent_cli.config_store import ConfigError, ConfigStore # noqa: E402 from keboola_agent_cli.errors import ErrorCode, KeboolaApiError # noqa: E402 from keboola_agent_cli.models import ProjectConfig, TokenVerifyResponse # noqa: E402 from keboola_agent_cli.services.merge_request_service import ( # noqa: E402 @@ -365,3 +365,95 @@ def test_scoped_token_yields_unknown_viewer(self, store, client_factory) -> None mock.merge_requests.conflicts.return_value = [] detail = _svc(store, factory).get_merge_request(ALIAS, 7) assert detail["viewer"] == {"is_creator": None, "has_approved": None} + + +DEFAULT_BRANCH = {"id": 1, "name": "Main", "isDefault": True} +DEV_BRANCH = {"id": 123, "name": "feature", "isDefault": False} + + +class TestCreateMergeRequest: + def test_targets_the_default_branch_automatically(self, store, client_factory) -> None: + factory, mock = client_factory + mock.list_dev_branches.return_value = [DEFAULT_BRANCH, DEV_BRANCH] + mock.merge_requests.create.return_value = _wire_mr(9, "development", branch_from=123) + result = _svc(store, factory).create_merge_request(ALIAS, 123, "My MR") + kwargs = mock.merge_requests.create.call_args.kwargs + assert kwargs["branch_from_id"] == 123 + assert kwargs["branch_into_id"] == 1 + assert result["branch_from_id"] == 123 + assert result["derived_state"] == "in_development" + + def test_refuses_the_default_branch_as_source(self, store, client_factory) -> None: + factory, mock = client_factory + mock.list_dev_branches.return_value = [DEFAULT_BRANCH, DEV_BRANCH] + with pytest.raises(ConfigError) as exc_info: + _svc(store, factory).create_merge_request(ALIAS, 1, "My MR") + assert "default" in str(exc_info.value) + mock.merge_requests.create.assert_not_called() + + def test_preflight_blocks_without_the_feature(self, store, client_factory) -> None: + factory, mock = client_factory + mock.has_feature.return_value = False + with pytest.raises(ConfigError) as exc_info: + _svc(store, factory).create_merge_request(ALIAS, 123, "My MR") + assert "branches-merge-requests" in str(exc_info.value) + mock.merge_requests.create.assert_not_called() + + def test_optional_fields_pass_through(self, store, client_factory) -> None: + factory, mock = client_factory + mock.list_dev_branches.return_value = [DEFAULT_BRANCH] + mock.merge_requests.create.return_value = _wire_mr(9) + _svc(store, factory).create_merge_request( + ALIAS, + 123, + "My MR", + description="d", + reviewer_ids=[5, 6], + external_id="TICKET-1", + ) + kwargs = mock.merge_requests.create.call_args.kwargs + assert kwargs["reviewer_ids"] == [5, 6] + assert kwargs["external_id"] == "TICKET-1" + + +class TestUpdateAndTransitions: + def test_update_passes_fields_and_enriches(self, store, client_factory) -> None: + factory, mock = client_factory + mock.merge_requests.update.return_value = _wire_mr(7, "in_review") + result = _svc(store, factory).update_merge_request(ALIAS, 7, title="New") + assert mock.merge_requests.update.call_args.kwargs["title"] == "New" + assert result["derived_state"] == "in_review" + + def test_request_review(self, store, client_factory) -> None: + factory, mock = client_factory + mock.merge_requests.request_review.return_value = _wire_mr(7, "approved") + result = _svc(store, factory).request_review(ALIAS, 7) + # Non-SOX default of 0 required approvals: lands straight in approved. + assert result["derived_state"] == "approved" + + def test_approve(self, store, client_factory) -> None: + factory, mock = client_factory + mock.merge_requests.approve.return_value = _wire_mr(7, "approved") + result = _svc(store, factory).approve(ALIAS, 7) + assert result["derived_state"] == "approved" + + def test_request_changes_carries_reason(self, store, client_factory) -> None: + factory, mock = client_factory + mock.merge_requests.request_changes.return_value = _wire_mr(7, "development") + _svc(store, factory).request_changes(ALIAS, 7, reason="fix the mapping") + assert ( + mock.merge_requests.request_changes.call_args.kwargs["reason"] == "fix the mapping" + ) + + def test_every_write_runs_the_preflight(self, store, client_factory) -> None: + factory, mock = client_factory + mock.has_feature.return_value = False + svc = _svc(store, factory) + for call in ( + lambda: svc.update_merge_request(ALIAS, 7, title="x"), + lambda: svc.request_review(ALIAS, 7), + lambda: svc.approve(ALIAS, 7), + lambda: svc.request_changes(ALIAS, 7), + ): + with pytest.raises(ConfigError): + call() From 8dc3533cd24d69da578ed078615270001fd9e988 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C5=A0ifra?= Date: Wed, 26 Aug 2026 03:37:24 +0200 Subject: [PATCH 07/15] feat(service): merge -- 409 mapping + post-merge local cleanup [DMD-1899] merge() waits for the Storage job (Layer 3, MERGE_JOB_MAX_WAIT) and adds the two service-level concerns from the RFC: 409 mapping (_remap_merge_conflict): the shape carrying storage.mergeRequests.notReadyToMerge (read from details.api_error_code, plumbed by the earlier http_base commit) becomes MR_NOT_READY_TO_MERGE, retryable -- all three causes are transient; a 409 without the code is the conflict validation and becomes MR_MERGE_CONFLICT, not retryable, with 'kbagent merge-request conflicts' named as the next step. Mapped here and not in http_base because only this call site knows the 409 came from the merge endpoint. Non-409 errors (STORAGE_JOB_FAILED rollback included) pass through unmapped. Post-merge cleanup mirrors BranchService.delete_branch: branchFromId is captured from the PRE-merge payload (nullable once published), active_branch_id resets only if it pointed at the merged branch (the was_active logic -- deliberately not get_merge_url's unconditional reset), and the sync branch-mapping entry is unlinked. The whole cleanup block is best-effort: the merge already happened, so a failure degrades to cleanup_warnings in the result instead of a failed command. The message says the source branch 'is being deleted' -- never that it is gone (second async job, no handle). Co-Authored-By: Claude Fable 5 --- .../services/merge_request_service.py | 127 ++++++++++++++++ tests/test_merge_request_service.py | 139 +++++++++++++++++- 2 files changed, 263 insertions(+), 3 deletions(-) diff --git a/src/keboola_agent_cli/services/merge_request_service.py b/src/keboola_agent_cli/services/merge_request_service.py index 8043f9c5..f4091f67 100644 --- a/src/keboola_agent_cli/services/merge_request_service.py +++ b/src/keboola_agent_cli/services/merge_request_service.py @@ -483,3 +483,130 @@ def _default_branch_id(self, client: KeboolaClient, alias: str) -> int: error_code=ErrorCode.API_ERROR, retryable=False, ) + + # -- Merge ------------------------------------------------------------------ + + _NOT_READY_CODE = "storage.mergeRequests.notReadyToMerge" + + def merge(self, alias: str, merge_request_id: int) -> dict[str, Any]: + """Merge the MR into the default branch and clean local references. + + Waits for the merge Storage job (Layer 3 awaits with + MERGE_JOB_MAX_WAIT; a failed merge raises STORAGE_JOB_FAILED and the + MR rolls back to ``approved``). Works straight from ``development`` + when approvals are satisfied -- the backend auto-applies skip_review. + + The merge 409 is remapped onto its two wire shapes (the RFC's + decision; docs/error-codes.md): + + - ``storage.mergeRequests.notReadyToMerge`` (project merge lock / + wrong state / another MR processing) -> ``MR_NOT_READY_TO_MERGE``, + retryable -- all three causes are transient. + - a 409 without the string code is the conflict validation -> + ``MR_MERGE_CONFLICT``, not retryable, next step in the message. + + A successful merge always also deletes the source branch -- as a + second async job with no handle, so the result says the branch "is + being deleted", never that it is gone. Local cleanup mirrors + ``BranchService.delete_branch``: reset ``active_branch_id`` only if + it pointed at the merged branch, and unlink any sync + ``branch-mapping.json`` entry. Cleanup is best-effort -- the merge + already happened, so a cleanup failure degrades to a warning in the + result instead of failing the command. + """ + from ..sync.branch_mapping import cleanup_branch_id_from_mapping + + project = self._project(alias) + client = self._client_factory(project.stack_url, project.token) + try: + self._require_merge_requests_feature(client) + # branchFromId is nullable once the MR is published -- capture it + # from the pre-merge payload, not the post-merge one. + branch_from_id = ( + client.merge_requests.get(merge_request_id).get("branches") or {} + ).get("branchFromId") + try: + job = client.merge_requests.merge(merge_request_id) + except KeboolaApiError as exc: + self._remap_merge_conflict(exc) + raise + finally: + client.close() + + was_active = branch_from_id is not None and project.active_branch_id == branch_from_id + mapping_cleanup: dict[str, Any] | None = None + cleanup_warnings: list[str] = [] + try: + if was_active: + self._config_store.set_project_branch(alias, None) + if branch_from_id is not None: + mapping_cleanup = cleanup_branch_id_from_mapping(branch_from_id) + except Exception as exc: + # a failed local cleanup must not turn it into a failed command. + logger.warning("Post-merge cleanup failed: %s", exc) + cleanup_warnings.append(f"Post-merge cleanup failed: {exc}") + + results = job.get("results") + mr_after: dict[str, Any] = results if isinstance(results, dict) else {} + + message_parts = [f"Merge request {merge_request_id} merged into production."] + if branch_from_id is not None: + message_parts.append( + f"Source branch {branch_from_id} is being deleted (a separate async " + "job -- it may still briefly exist)." + ) + if was_active: + message_parts.append("Active branch reset to main.") + if mapping_cleanup: + unlinked = ", ".join(mapping_cleanup["git_branches_unlinked"]) + message_parts.append(f"Unlinked git branch(es): {unlinked}.") + + result: dict[str, Any] = { + "alias": alias, + "merge_request_id": merge_request_id, + "branch_from_id": branch_from_id, + "was_active": was_active, + "job": job, + "message": " ".join(message_parts), + } + if mr_after.get("state"): + result["state"] = mr_after["state"] + result["derived_state"] = derive_state(mr_after) + if mapping_cleanup: + result["mapping_cleanup"] = mapping_cleanup + if cleanup_warnings: + result["cleanup_warnings"] = cleanup_warnings + return result + + def _remap_merge_conflict(self, exc: KeboolaApiError) -> None: + """Raise the RFC's dedicated error for a merge 409; return for others. + + Only this call site knows the 409 came from the merge endpoint, and + the conflict shape carries no machine string code -- which is why the + mapping cannot live in http_base (see docs/merge-requests-layer2.md). + """ + if exc.status_code != 409: + return + if exc.details.get("api_error_code") == self._NOT_READY_CODE: + raise KeboolaApiError( + message=( + f"{exc.message} The merge lock, MR state or a concurrently " + "processing merge request blocks the merge -- these are " + "transient; retry once it clears." + ), + status_code=409, + error_code=ErrorCode.MR_NOT_READY_TO_MERGE, + retryable=True, + details=exc.details, + ) from exc + raise KeboolaApiError( + message=( + f"{exc.message} Configurations changed on both branches. Inspect them " + "with `kbagent merge-request conflicts`, resolve each one, then merge " + "again (conflicts are re-validated live on every attempt)." + ), + status_code=409, + error_code=ErrorCode.MR_MERGE_CONFLICT, + retryable=False, + details=exc.details, + ) from exc diff --git a/tests/test_merge_request_service.py b/tests/test_merge_request_service.py index 89ecd72e..11eec838 100644 --- a/tests/test_merge_request_service.py +++ b/tests/test_merge_request_service.py @@ -441,9 +441,7 @@ def test_request_changes_carries_reason(self, store, client_factory) -> None: factory, mock = client_factory mock.merge_requests.request_changes.return_value = _wire_mr(7, "development") _svc(store, factory).request_changes(ALIAS, 7, reason="fix the mapping") - assert ( - mock.merge_requests.request_changes.call_args.kwargs["reason"] == "fix the mapping" - ) + assert mock.merge_requests.request_changes.call_args.kwargs["reason"] == "fix the mapping" def test_every_write_runs_the_preflight(self, store, client_factory) -> None: factory, mock = client_factory @@ -457,3 +455,138 @@ def test_every_write_runs_the_preflight(self, store, client_factory) -> None: ): with pytest.raises(ConfigError): call() + + +class TestMerge: + def _arm_merge(self, mock: MagicMock, branch_from: int | None = 123) -> None: + mock.merge_requests.get.return_value = _wire_mr(7, "approved", branch_from=branch_from) + mock.merge_requests.merge.return_value = { + "id": 555, + "status": "success", + "results": {**_wire_mr(7, "published", branch_from=None)}, + } + + def test_happy_path_says_being_deleted_never_deleted( + self, store, client_factory, monkeypatch + ) -> None: + factory, mock = client_factory + self._arm_merge(mock) + monkeypatch.setattr( + "keboola_agent_cli.sync.branch_mapping.cleanup_branch_id_from_mapping", + lambda branch_id: None, + ) + result = _svc(store, factory).merge(ALIAS, 7) + assert "is being deleted" in result["message"] + assert "is deleted" not in result["message"].replace("is being deleted", "") + assert result["branch_from_id"] == 123 + assert result["state"] == "published" + assert result["derived_state"] == "merged" + + def test_active_branch_reset_only_when_it_was_the_merged_one( + self, store, client_factory, monkeypatch + ) -> None: + factory, mock = client_factory + self._arm_merge(mock) + monkeypatch.setattr( + "keboola_agent_cli.sync.branch_mapping.cleanup_branch_id_from_mapping", + lambda branch_id: None, + ) + store.set_project_branch(ALIAS, 999) # a DIFFERENT branch is active + result = _svc(store, factory).merge(ALIAS, 7) + assert result["was_active"] is False + assert store.get_project(ALIAS).active_branch_id == 999 # untouched + + store.set_project_branch(ALIAS, 123) # the merged branch is active + result = _svc(store, factory).merge(ALIAS, 7) + assert result["was_active"] is True + assert store.get_project(ALIAS).active_branch_id is None + + def test_sync_mapping_cleanup_is_reported(self, store, client_factory, monkeypatch) -> None: + factory, mock = client_factory + self._arm_merge(mock) + monkeypatch.setattr( + "keboola_agent_cli.sync.branch_mapping.cleanup_branch_id_from_mapping", + lambda branch_id: {"project_root": "/x", "git_branches_unlinked": ["feat/a"]}, + ) + result = _svc(store, factory).merge(ALIAS, 7) + assert result["mapping_cleanup"]["git_branches_unlinked"] == ["feat/a"] + assert "feat/a" in result["message"] + + def test_cleanup_failure_degrades_to_warning_not_error( + self, store, client_factory, monkeypatch + ) -> None: + factory, mock = client_factory + self._arm_merge(mock) + + def boom(branch_id: int) -> None: + raise OSError("disk full") + + monkeypatch.setattr( + "keboola_agent_cli.sync.branch_mapping.cleanup_branch_id_from_mapping", boom + ) + result = _svc(store, factory).merge(ALIAS, 7) # must NOT raise + assert any("disk full" in w for w in result["cleanup_warnings"]) + + def test_409_with_code_maps_to_not_ready_retryable(self, store, client_factory) -> None: + factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "approved") + mock.merge_requests.merge.side_effect = KeboolaApiError( + message="API error 409: Cannot merge, another merge request is processing.", + status_code=409, + error_code=ErrorCode.API_ERROR, + details={"api_error_code": "storage.mergeRequests.notReadyToMerge"}, + ) + with pytest.raises(KeboolaApiError) as exc_info: + _svc(store, factory).merge(ALIAS, 7) + assert exc_info.value.error_code == ErrorCode.MR_NOT_READY_TO_MERGE + assert exc_info.value.retryable is True + + def test_409_without_code_maps_to_conflict_with_next_step(self, store, client_factory) -> None: + factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "approved") + mock.merge_requests.merge.side_effect = KeboolaApiError( + message="API error 409: Configuration was changed in the default branch.", + status_code=409, + error_code=ErrorCode.API_ERROR, + ) + with pytest.raises(KeboolaApiError) as exc_info: + _svc(store, factory).merge(ALIAS, 7) + assert exc_info.value.error_code == ErrorCode.MR_MERGE_CONFLICT + assert exc_info.value.retryable is False + assert "merge-request conflicts" in exc_info.value.message + + def test_non_409_errors_pass_through_unmapped(self, store, client_factory) -> None: + factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "approved") + mock.merge_requests.merge.side_effect = KeboolaApiError( + message="job failed", + status_code=0, + error_code=ErrorCode.STORAGE_JOB_FAILED, + ) + with pytest.raises(KeboolaApiError) as exc_info: + _svc(store, factory).merge(ALIAS, 7) + assert exc_info.value.error_code == ErrorCode.STORAGE_JOB_FAILED + + def test_failed_merge_does_no_cleanup(self, store, client_factory, monkeypatch) -> None: + factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "approved") + mock.merge_requests.merge.side_effect = KeboolaApiError( + message="conflict", status_code=409, error_code=ErrorCode.API_ERROR + ) + called: list[int] = [] + monkeypatch.setattr( + "keboola_agent_cli.sync.branch_mapping.cleanup_branch_id_from_mapping", + lambda branch_id: called.append(branch_id), + ) + store.set_project_branch(ALIAS, 123) + with pytest.raises(KeboolaApiError): + _svc(store, factory).merge(ALIAS, 7) + assert called == [] + assert store.get_project(ALIAS).active_branch_id == 123 + + def test_preflight_blocks_without_the_feature(self, store, client_factory) -> None: + factory, mock = client_factory + mock.has_feature.return_value = False + with pytest.raises(ConfigError): + _svc(store, factory).merge(ALIAS, 7) + mock.merge_requests.merge.assert_not_called() From e4a00c1dc08c8472bab46d004d76a61c234498dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C5=A0ifra?= Date: Wed, 26 Aug 2026 03:40:26 +0200 Subject: [PATCH 08/15] feat(service): conflicts, per-path 3-way diff, resolve via rebase [DMD-1899] The conflict-resolution half of the service: - list_conflicts: pass-through with count (conflicts are computed live by the backend on every call/merge attempt -- rebasing each listed config is sufficient, no re-validate step exists). - get_config_diff: flattens the nested base/ours/theirs (each nullable) into a per-path classification -- changed_by: ours | theirs | both, built by intersecting two compute_diff_entries() pairwise diffs. A side that removed a key shows None (never the base value); an untouched side shows the base. rows compare wholesale. onto_version carries theirs.version -- the rebase version trap spelled out once, in data. - resolve_conflict: every mode goes through the rebase endpoint (RFC decision; the UI's reset-to-default for take=theirs is DMD-1987). take=ours|theirs composes the full replace body from the diff side with version=theirs.version; take=ours of a deleted side becomes the delete resolution; take=delete sends the tombstone; a caller-authored resolved body must spell out name/rows/configuration explicitly (rebase REPLACES -- a defaulted key would be silent data loss). The config must be in the MR's live conflict set, and rebase runs the feature pre-flight (the one endpoint gated on branches-merge-requests specifically). Co-Authored-By: Claude Fable 5 --- .../services/merge_request_service.py | 228 ++++++++++++++++++ tests/test_http_base.py | 3 +- tests/test_merge_request_service.py | 190 +++++++++++++++ 3 files changed, 419 insertions(+), 2 deletions(-) diff --git a/src/keboola_agent_cli/services/merge_request_service.py b/src/keboola_agent_cli/services/merge_request_service.py index f4091f67..b2cc2f0e 100644 --- a/src/keboola_agent_cli/services/merge_request_service.py +++ b/src/keboola_agent_cli/services/merge_request_service.py @@ -26,6 +26,7 @@ from ..config_store import ConfigError from ..constants import BRANCHES_MERGE_REQUESTS_FEATURE from ..errors import ErrorCode, KeboolaApiError +from ..json_utils import DiffEntry, compute_diff_entries from ..models import ProjectConfig from .base import BaseService @@ -610,3 +611,230 @@ def _remap_merge_conflict(self, exc: KeboolaApiError) -> None: retryable=False, details=exc.details, ) from exc + + # -- Conflicts / diff / resolution -------------------------------------------- + + # Content-bearing keys of a diff side -- what a three-way comparison is + # about. Version/identifier fields differ by construction on a conflict + # and would only add noise to the per-path classification. + _DIFF_CONTENT_KEYS = ("name", "description", "configuration", "isDisabled", "rows") + + def list_conflicts(self, alias: str, merge_request_id: int) -> dict[str, Any]: + """List the configurations conflicting between the MR's branches. + + Conflicts are computed live by the backend on every call (and on + every merge attempt), so rebasing each listed config is sufficient -- + there is no MR-level re-validate step. + """ + project = self._project(alias) + client = self._client_factory(project.stack_url, project.token) + try: + conflicts = client.merge_requests.conflicts(merge_request_id) + finally: + client.close() + return { + "alias": alias, + "merge_request_id": merge_request_id, + "count": len(conflicts), + "conflicts": conflicts, + } + + def get_config_diff( + self, alias: str, component_id: str, config_id: str, branch_id: int + ) -> dict[str, Any]: + """Three-way diff of one config, flattened to a per-path classification. + + No three panes: each touched path is tagged ``changed_by`` -- + ``ours`` (only the dev branch changed it), ``theirs`` (only + production), or ``both`` (the actual conflict hotspots). ``rows`` + compares wholesale (row-level three-way diffing is not attempted). + ``onto_version`` is the default-branch version a rebase re-anchors + onto -- the ``theirs.version`` trap spelled out once, here. + """ + project = self._project(alias) + client = self._client_factory(project.stack_url, project.token) + try: + diff = client.get_config_diff(component_id, config_id, branch_id) + finally: + client.close() + theirs = diff.get("theirs") or {} + return { + "alias": alias, + "component_id": component_id, + "config_id": config_id, + "branch_id": branch_id, + "onto_version": theirs.get("version"), + "changes": self._classify_three_way(diff), + "diff": diff, + } + + def _classify_three_way(self, diff: dict[str, Any]) -> list[dict[str, Any]]: + """Intersect the two pairwise diffs (base->ours, base->theirs) per path.""" + + def content(side: dict[str, Any] | None) -> dict[str, Any]: + side = side or {} + return {key: side[key] for key in self._DIFF_CONTENT_KEYS if key in side} + + base = content(diff.get("base")) + ours_entries = {e.path: e for e in compute_diff_entries(base, content(diff.get("ours")))} + theirs_entries = { + e.path: e for e in compute_diff_entries(base, content(diff.get("theirs"))) + } + + changes: list[dict[str, Any]] = [] + for path in sorted(set(ours_entries) | set(theirs_entries)): + ours_entry = ours_entries.get(path) + theirs_entry = theirs_entries.get(path) + reference = ours_entry or theirs_entry + assert reference is not None # path came from one of the two maps + changed_by = ( + "both" if ours_entry and theirs_entry else ("ours" if ours_entry else "theirs") + ) + base_value = reference.old if reference.old_present else None + + def side_value(entry: DiffEntry | None, base_val: Any) -> Any: + # A side that changed the path shows its own value (None when + # it REMOVED the key -- never the base value); a side with no + # entry did not touch the path and still holds the base. + if entry is not None: + return entry.new if entry.new_present else None + return base_val + + changes.append( + { + "path": path, + "changed_by": changed_by, + "base": base_value, + "ours": side_value(ours_entry, base_value), + "theirs": side_value(theirs_entry, base_value), + } + ) + return changes + + def resolve_conflict( + self, + alias: str, + merge_request_id: int, + component_id: str, + config_id: str, + branch_id: int, + take: str | None = None, + resolved: dict[str, Any] | None = None, + change_description: str | None = None, + ) -> dict[str, Any]: + """Resolve one conflicting config by rebasing it (uniformly -- every + mode goes through the rebase endpoint, per the RFC decision; the UI's + reset-to-default alternative for take=theirs is DMD-1987). + + Modes (exactly one of ``take`` / ``resolved``): + + - ``take="ours"``: keep the dev-branch content, re-anchored onto the + production version. An ours side that is deleted turns this into + the delete resolution. + - ``take="theirs"``: adopt the production content (the config stays + in the MR's changeset; the merge then writes a content-no-op). + - ``take="delete"``: the ``{"version": N, "diff": {}}`` tombstone. + - ``resolved={...}``: a caller-authored three-way merge -- must carry + ``name``, ``rows`` and ``configuration`` explicitly (rebase + REPLACES; a missing key would silently wipe data, so it is refused + instead of defaulted). + + The config must be in the MR's live conflict set; ``version`` is + taken from the diff's ``theirs.version`` (the default-branch version + being re-anchored onto). Rebasing every conflicting config makes the + MR mergeable -- no re-validate step exists or is needed. + """ + if (take is None) == (resolved is None): + raise ConfigError("Pass exactly one of take=ours|theirs|delete or a resolved body.") + if take is not None and take not in ("ours", "theirs", "delete"): + raise ConfigError(f"Unknown take mode {take!r}: use ours, theirs or delete.") + + project = self._project(alias) + client = self._client_factory(project.stack_url, project.token) + try: + self._require_merge_requests_feature(client) + self._require_in_conflict_set(client, merge_request_id, component_id, config_id) + diff = client.get_config_diff(component_id, config_id, branch_id) + theirs = diff.get("theirs") or {} + onto_version = theirs.get("version") + if onto_version is None: + raise KeboolaApiError( + message=( + f"The diff of {component_id}/{config_id} has no theirs side -- " + "cannot determine the default-branch version to rebase onto." + ), + status_code=0, + error_code=ErrorCode.VALIDATION_ERROR, + retryable=False, + ) + + side = resolved + resolution = "custom" + if take == "delete": + side, resolution = None, "delete" + elif take == "ours": + ours = diff.get("ours") + if ours is None or ours.get("isDeleted"): + # Taking a deleted ours side IS the delete resolution. + side, resolution = None, "delete" + else: + side, resolution = ours, "ours" + elif take == "theirs": + side, resolution = theirs, "theirs" + + if side is None: + configuration = client.rebase_config_delete( + component_id, config_id, branch_id, version=onto_version + ) + else: + missing = [key for key in ("name", "rows", "configuration") if key not in side] + if missing: + raise ConfigError( + "A resolved body must spell out the full replaced content " + f"(rebase REPLACES): missing {', '.join(missing)}." + ) + configuration = client.rebase_config( + component_id, + config_id, + branch_id, + version=onto_version, + name=side["name"], + rows=side["rows"], + configuration=side["configuration"], + is_disabled=bool(side.get("isDisabled", False)), + description=side.get("description"), + change_description=change_description, + ) + finally: + client.close() + + return { + "alias": alias, + "merge_request_id": merge_request_id, + "component_id": component_id, + "config_id": config_id, + "branch_id": branch_id, + "resolution": resolution, + "onto_version": onto_version, + "configuration": configuration, + } + + def _require_in_conflict_set( + self, client: KeboolaClient, merge_request_id: int, component_id: str, config_id: str + ) -> None: + """Refuse to rebase a config the MR does not list as conflicting.""" + for conflict in client.merge_requests.conflicts(merge_request_id): + if conflict.get("componentId") == component_id and str( + conflict.get("configurationId") + ) == str(config_id): + return + raise KeboolaApiError( + message=( + f"{component_id}/{config_id} is not in merge request " + f"{merge_request_id}'s conflict set -- nothing to resolve. " + "See `kbagent merge-request conflicts` for the current set." + ), + status_code=0, + error_code=ErrorCode.VALIDATION_ERROR, + retryable=False, + ) diff --git a/tests/test_http_base.py b/tests/test_http_base.py index 2c6b9cd1..eec5b3a4 100644 --- a/tests/test_http_base.py +++ b/tests/test_http_base.py @@ -1085,8 +1085,7 @@ def test_body_code_lands_in_details(self, httpx_mock) -> None: with pytest.raises(KeboolaApiError) as exc_info: client._do_request("GET", "/test-path") assert ( - exc_info.value.details["api_error_code"] - == "storage.mergeRequests.notReadyToMerge" + exc_info.value.details["api_error_code"] == "storage.mergeRequests.notReadyToMerge" ) finally: client.close() diff --git a/tests/test_merge_request_service.py b/tests/test_merge_request_service.py index 11eec838..d146725f 100644 --- a/tests/test_merge_request_service.py +++ b/tests/test_merge_request_service.py @@ -590,3 +590,193 @@ def test_preflight_blocks_without_the_feature(self, store, client_factory) -> No with pytest.raises(ConfigError): _svc(store, factory).merge(ALIAS, 7) mock.merge_requests.merge.assert_not_called() + + +def _diff( + base: dict[str, Any] | None, + ours: dict[str, Any] | None, + theirs: dict[str, Any] | None, +) -> dict[str, Any]: + return {"base": base, "ours": ours, "theirs": theirs} + + +def _side(configuration: dict[str, Any], version: int = 5, **extra: Any) -> dict[str, Any]: + return { + "version": version, + "name": "My config", + "description": None, + "isDisabled": False, + "configuration": configuration, + "rows": [], + **extra, + } + + +CONFLICT_ENTRY = {"componentId": "keboola.ex-db", "configurationId": "111"} + + +class TestGetConfigDiff: + def test_classifies_ours_theirs_both(self, store, client_factory) -> None: + factory, mock = client_factory + mock.get_config_diff.return_value = _diff( + base=_side({"limit": 100, "timeout": 30, "flag": True}, version=3), + ours=_side({"limit": 500, "timeout": 30, "flag": False}, version=4), + theirs=_side({"limit": 250, "timeout": 60, "flag": True}, version=7), + ) + result = _svc(store, factory).get_config_diff(ALIAS, "keboola.ex-db", "111", 123) + by_path = {c["path"]: c for c in result["changes"]} + assert by_path["configuration.limit"]["changed_by"] == "both" + assert by_path["configuration.limit"] == { + "path": "configuration.limit", + "changed_by": "both", + "base": 100, + "ours": 500, + "theirs": 250, + } + assert by_path["configuration.timeout"]["changed_by"] == "theirs" + # The side that did NOT touch the path still holds the base value. + assert by_path["configuration.timeout"]["ours"] == 30 + assert by_path["configuration.flag"]["changed_by"] == "ours" + assert result["onto_version"] == 7 + + def test_removed_key_shows_none_not_base(self, store, client_factory) -> None: + factory, mock = client_factory + mock.get_config_diff.return_value = _diff( + base=_side({"secret": "old"}, version=3), + ours=_side({}, version=4), # ours REMOVED the key + theirs=_side({"secret": "new"}, version=7), + ) + result = _svc(store, factory).get_config_diff(ALIAS, "keboola.ex-db", "111", 123) + (change,) = result["changes"] + assert change["changed_by"] == "both" + assert change["ours"] is None # removed, never the base value + assert change["theirs"] == "new" + + def test_null_base_means_added_on_both_sides(self, store, client_factory) -> None: + factory, mock = client_factory + mock.get_config_diff.return_value = _diff( + base=None, + ours=_side({"a": 1}, version=4), + theirs=_side({"a": 2}, version=7), + ) + result = _svc(store, factory).get_config_diff(ALIAS, "keboola.ex-db", "111", 123) + by_path = {c["path"]: c for c in result["changes"]} + # A key absent from base diffs wholesale (compute_diff semantics): + # the whole `configuration` dict was added on both sides. + assert by_path["configuration"]["base"] is None + assert by_path["configuration"]["changed_by"] == "both" + assert by_path["configuration"]["ours"] == {"a": 1} + assert by_path["configuration"]["theirs"] == {"a": 2} + + def test_rows_compare_wholesale(self, store, client_factory) -> None: + factory, mock = client_factory + mock.get_config_diff.return_value = _diff( + base=_side({}, version=3, rows=[{"id": "r1"}]), + ours=_side({}, version=4, rows=[{"id": "r1"}, {"id": "r2"}]), + theirs=_side({}, version=7, rows=[{"id": "r1"}]), + ) + result = _svc(store, factory).get_config_diff(ALIAS, "keboola.ex-db", "111", 123) + (change,) = result["changes"] + assert change["path"] == "rows" + assert change["changed_by"] == "ours" + + +class TestResolveConflict: + def _arm(self, mock: MagicMock, ours: dict[str, Any] | None = None) -> None: + mock.merge_requests.conflicts.return_value = [CONFLICT_ENTRY] + mock.get_config_diff.return_value = _diff( + base=_side({"limit": 100}, version=3), + ours=ours if ours is not None else _side({"limit": 500}, version=4), + theirs=_side({"limit": 250}, version=7), + ) + mock.rebase_config.return_value = {"id": "111", "version": 8} + mock.rebase_config_delete.return_value = {"id": "111", "version": 8, "isDeleted": True} + + def test_take_ours_rebases_dev_content_onto_theirs_version(self, store, client_factory) -> None: + factory, mock = client_factory + self._arm(mock) + result = _svc(store, factory).resolve_conflict( + ALIAS, 7, "keboola.ex-db", "111", 123, take="ours" + ) + kwargs = mock.rebase_config.call_args.kwargs + assert kwargs["version"] == 7 # theirs.version, never ours' + assert kwargs["configuration"] == {"limit": 500} + assert result["resolution"] == "ours" + assert result["onto_version"] == 7 + + def test_take_theirs_rebases_production_content(self, store, client_factory) -> None: + factory, mock = client_factory + self._arm(mock) + _svc(store, factory).resolve_conflict(ALIAS, 7, "keboola.ex-db", "111", 123, take="theirs") + kwargs = mock.rebase_config.call_args.kwargs + assert kwargs["configuration"] == {"limit": 250} + assert kwargs["version"] == 7 + + def test_take_delete_sends_the_tombstone(self, store, client_factory) -> None: + factory, mock = client_factory + self._arm(mock) + result = _svc(store, factory).resolve_conflict( + ALIAS, 7, "keboola.ex-db", "111", 123, take="delete" + ) + mock.rebase_config_delete.assert_called_once_with("keboola.ex-db", "111", 123, version=7) + assert result["resolution"] == "delete" + + def test_take_ours_of_a_deleted_side_becomes_delete(self, store, client_factory) -> None: + factory, mock = client_factory + self._arm(mock, ours=_side({}, version=4, isDeleted=True)) + result = _svc(store, factory).resolve_conflict( + ALIAS, 7, "keboola.ex-db", "111", 123, take="ours" + ) + assert result["resolution"] == "delete" + mock.rebase_config.assert_not_called() + + def test_custom_body_must_spell_out_replaced_content(self, store, client_factory) -> None: + factory, mock = client_factory + self._arm(mock) + with pytest.raises(ConfigError) as exc_info: + _svc(store, factory).resolve_conflict( + ALIAS, 7, "keboola.ex-db", "111", 123, resolved={"name": "n"} + ) + assert "rows" in str(exc_info.value) and "configuration" in str(exc_info.value) + mock.rebase_config.assert_not_called() + + def test_custom_body_rebases_verbatim(self, store, client_factory) -> None: + factory, mock = client_factory + self._arm(mock) + body = {"name": "merged", "rows": [], "configuration": {"limit": 300}} + result = _svc(store, factory).resolve_conflict( + ALIAS, 7, "keboola.ex-db", "111", 123, resolved=body, change_description="3-way" + ) + kwargs = mock.rebase_config.call_args.kwargs + assert kwargs["configuration"] == {"limit": 300} + assert kwargs["change_description"] == "3-way" + assert result["resolution"] == "custom" + + def test_exactly_one_mode_required(self, store, client_factory) -> None: + factory, _ = client_factory + svc = _svc(store, factory) + with pytest.raises(ConfigError): + svc.resolve_conflict(ALIAS, 7, "c", "1", 123) + with pytest.raises(ConfigError): + svc.resolve_conflict(ALIAS, 7, "c", "1", 123, take="ours", resolved={}) + with pytest.raises(ConfigError): + svc.resolve_conflict(ALIAS, 7, "c", "1", 123, take="mine") + + def test_config_outside_conflict_set_is_refused(self, store, client_factory) -> None: + factory, mock = client_factory + mock.merge_requests.conflicts.return_value = [CONFLICT_ENTRY] + with pytest.raises(KeboolaApiError) as exc_info: + _svc(store, factory).resolve_conflict( + ALIAS, 7, "keboola.other", "999", 123, take="ours" + ) + assert exc_info.value.error_code == ErrorCode.VALIDATION_ERROR + mock.rebase_config.assert_not_called() + + +class TestListConflicts: + def test_returns_count_and_raw_entries(self, store, client_factory) -> None: + factory, mock = client_factory + mock.merge_requests.conflicts.return_value = [CONFLICT_ENTRY] + result = _svc(store, factory).list_conflicts(ALIAS, 7) + assert result["count"] == 1 + assert result["conflicts"] == [CONFLICT_ENTRY] From 51eaa2c1c0f9a2ff9af7f5da1be7c01f045869c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C5=A0ifra?= Date: Thu, 27 Aug 2026 01:40:16 +0200 Subject: [PATCH 09/15] fix(service): apply the PR #703 self-review findings [DMD-1899] The big one (#2, upgraded): the diff sides' wire shape was WRONG in the implementation and in the test fixtures that defined it. Verified against connection (ConfigurationVersionResponse + ConfigurationDiffData OA schemas, now recorded in the notes wire-truth table): each side is {version, isDeleted, diff: {name, description, changeDescription, isDisabled, configuration, rows}} -- content NESTED under diff, version/deletion as side metadata. The classification and both take modes now read the envelope; the flat-side code would have been dead on arrival against the live API. #1: resolve_conflict no longer takes branch_id -- it derives the branch from the MR itself (branches.branchFromId), so the conflict-set guard and the branch being written to can never disagree; a caller-supplied id could point the REPLACing rebase at an unrelated dev branch the guard never checked. A published/canceled MR (null branchFromId) is refused readably. #3: take=theirs of a deleted side collapses to the delete resolution, symmetric with ours ('production deleted it, dev changed it' is a live conflict shape). #4: deletion surfaces as top-level ours_deleted/theirs_deleted booleans on get_config_diff (None = side never existed) -- it is side metadata, not a content path. #5: a 'both' row where the sides agree on the identical value carries agreed: true -- agreement, not a conflict hotspot. #2 (message half): a take side missing required envelope keys is reported as a backend contract violation pointing at the resolved-body workaround, not as caller error. #6: wire ids compared via _same_id / int-coerced (find_merge_request_for_ branch, merge()'s was_active) -- a string-serialized branchFromId can no longer silently defeat the post-merge cleanup. #7: the feature pre-flight raises FeatureNotEnabledError carrying the new ErrorCode.FEATURE_NOT_ENABLED (value matches the string SearchService already emits; categorized 'configuration' like PAYG_NOT_AVAILABLE). #8: ConfigError imported from ..errors like every other service; the isDefault scan hoisted to services.base.find_default_branch_id and the copies in config/sync/workspace services migrated (lib.py keeps its own loop -- the SDK facade does not import the services layer); verify_token is skipped when the server already serialized viewer (the polyfill's cost dies with the polyfill); list --state validates against the closed vocabulary instead of returning a silent count: 0 on a typo. #9: test imports hoisted (no mid-file noqa), mocks spec'd at the L3 seam (KeboolaClient + MergeRequests -- a renamed L3 method now fails the tests), and regression tests added for every finding (82 tests total). Doc drift: layer2/layer3 references updated to BRANCHES_MERGE_REQUESTS_ FEATURE, the layer3 open nit closed, tokens.py line ref fixed. Review: tasks/pr-703-review.md (2026-08-27). Co-Authored-By: Claude Fable 5 --- docs/error-codes.md | 1 + docs/merge-requests-layer2.md | 2 +- docs/merge-requests-layer3.md | 9 +- docs/merge-requests-notes.md | 1 + src/keboola_agent_cli/errors.py | 23 ++ src/keboola_agent_cli/services/base.py | 15 + .../services/config_service.py | 13 +- .../services/merge_request_service.py | 207 ++++++++++---- .../services/sync_service.py | 8 +- .../services/workspace_service.py | 9 +- tests/test_merge_request_service.py | 268 +++++++++++++++--- 11 files changed, 430 insertions(+), 126 deletions(-) diff --git a/docs/error-codes.md b/docs/error-codes.md index e933cd95..962c107c 100644 --- a/docs/error-codes.md +++ b/docs/error-codes.md @@ -54,6 +54,7 @@ of `ErrorCode` in `src/keboola_agent_cli/errors.py`. | `CONFIG_ERROR` | kbagent config problem (e.g. unknown project alias) | | `NOT_INITIALIZED` | `.keboola/manifest.json` not found; run `sync init` first | | `INIT_ERROR` | Error during `sync init` auto-init path | +| `FEATURE_NOT_ENABLED` | A client-side pre-flight found the project lacks a required feature flag (e.g. `branches-merge-requests` for `merge-request` writes); the server would answer an opaque 403/404 | ### Jobs diff --git a/docs/merge-requests-layer2.md b/docs/merge-requests-layer2.md index ddf93add..30f2a068 100644 --- a/docs/merge-requests-layer2.md +++ b/docs/merge-requests-layer2.md @@ -93,7 +93,7 @@ MCP/agent response needs to phrase the next step: blocker `approvals` + `has_app `GET /merge-request` list/detail/conflicts are ungated; a write without the feature is a 403 byte-for-byte identical to a role denial. So the service calls -`has_feature(FEATURE_BRANCHES_MERGE_REQUESTS)` (`client/tokens.py:200`, cache populated on +`has_feature(BRANCHES_MERGE_REQUESTS_FEATURE)` (`client/tokens.py:302`, cache populated on every `verify_token`) before writes and words the "not enabled" error itself. Caveats to carry into the implementation: diff --git a/docs/merge-requests-layer3.md b/docs/merge-requests-layer3.md index b8e5cc35..adde1038 100644 --- a/docs/merge-requests-layer3.md +++ b/docs/merge-requests-layer3.md @@ -7,7 +7,8 @@ record distilled from the original RFC and the review cycle; behavioral backend [`merge-requests-notes.md`](merge-requests-notes.md). Code: `client/merge_requests.py` (namespace + Protocol + adapter + mixin), -`client/configs.py` (diff/rebase), `constants.py` (`FEATURE_BRANCHES_MERGE_REQUESTS`, +`client/configs.py` (diff/rebase), `constants.py` (`BRANCHES_MERGE_REQUESTS_FEATURE` -- +renamed from `FEATURE_BRANCHES_MERGE_REQUESTS` when Layer 2 wired the pre-flight, `MERGE_JOB_MAX_WAIT`), `tests/test_merge_request_client.py`. ## Backend contract (what shapes the client) @@ -118,7 +119,7 @@ change_description=None)`, `rebase_config_delete(…, version)`. `str | None`). - **No feature-flag plumbing at Layer 3.** A missing feature is a 403 identical to a role denial — only a Layer 2 pre-flight can word the error. Part 1 contributed only the constant - `FEATURE_BRANCHES_MERGE_REQUESTS`. + `BRANCHES_MERGE_REQUESTS_FEATURE`. - Tried and reverted: keyword-only `rebase_config` (bare `*`). The "ids are positional house-wide" premise was false (Layer 2 call sites are mixed), so no placement had a consistency case; signature stays shaped like its `configs.py` siblings. If keyword-only is @@ -142,7 +143,9 @@ change_description=None)`, `rebase_config_delete(…, version)`. ## Deferred / follow-ups recorded during review -- `FEATURE_BRANCHES_MERGE_REQUESTS` naming is off the file's dominant convention (suffix: +- ~~`FEATURE_BRANCHES_MERGE_REQUESTS` naming~~ -- resolved by Layer 2 (PR #703): renamed to + `BRANCHES_MERGE_REQUESTS_FEATURE` when the pre-flight was wired. Original note: off the + file's dominant convention (suffix: `STORAGE_BRANCHES_FEATURE`, `GLOBAL_SEARCH_FEATURE`, `PAYG_FEATURE`) and the constant is unused until Layer 2 calls it — decide rename vs. keep when wiring the pre-flight. - SOX-fence caveat on the constant's comment: the fence holds only if a SOX project never also diff --git a/docs/merge-requests-notes.md b/docs/merge-requests-notes.md index cf199161..23d106fa 100644 --- a/docs/merge-requests-notes.md +++ b/docs/merge-requests-notes.md @@ -199,6 +199,7 @@ final Layer 3 review: | Source branch deleted as a second job, no handle returned | `createAndEnqueueJobFromJob(..., DevBranchDelete::OPERATION_NAME, ...)` after commit in `MergeDevBranchJob` | | diff/rebase 400 on default branch | `ConfigurationRebaseNotAvailableOnDefaultBranchException` / diff OA doc → `createBadRequestException` | | Diff shape `base`/`ours`/`theirs`, each nullable | `ConfigurationDiffResponse` | +| Each diff side = `{version, isDeleted, diff: {name, description, changeDescription, isDisabled, configuration, rows}}` -- content NESTED under `diff`, version/deletion as side metadata; all six `diff` keys `required` | `ConfigurationVersionResponse` + `ConfigurationDiffData` OA schemas (re-verified 2026-08-27; a flat-side assumption breaks every take/classify consumer) | | Rebase replaces; missing `configuration` → `{}`, `isDisabled` → `false`, `description` → null | `RebaseRequest::mapValidatedData` (`?? new stdClass()`, `?? false`, `isset` → null); "complete 3-way diff result … fully replaces" verbatim in `ConfigurationRebaseService` docblock | | `rows` required for keep; `[]` deletes all rows; order = sort order | `validateDiffRows` + OA schema | | Empty `diff` `{}` = delete resolution (tombstone) | `validateDiff` empty-stdClass branch → `isDelete: true` | diff --git a/src/keboola_agent_cli/errors.py b/src/keboola_agent_cli/errors.py index 018dd57c..0eac7860 100644 --- a/src/keboola_agent_cli/errors.py +++ b/src/keboola_agent_cli/errors.py @@ -46,6 +46,11 @@ class ErrorCode(StrEnum): CONFIG_ERROR = "CONFIG_ERROR" NOT_INITIALIZED = "NOT_INITIALIZED" INIT_ERROR = "INIT_ERROR" + # A client-side pre-flight found the project lacks a required feature + # flag (raised via FeatureNotEnabledError). The value matches the string + # SearchService already emits in its per-project error envelopes, so the + # two surfaces agree. + FEATURE_NOT_ENABLED = "FEATURE_NOT_ENABLED" # Jobs QUEUE_JOB_FAILED = "QUEUE_JOB_FAILED" @@ -210,6 +215,23 @@ def __init__(self, message: str) -> None: self.message = message +class FeatureNotEnabledError(ConfigError): + """Raised by a client-side pre-flight when the project lacks a feature flag. + + A missing feature often surfaces server-side as an opaque 403 (or 404) + indistinguishable from a role denial -- only a pre-flight can word the + real error. Carries ``error_code`` so a ``--json`` consumer can tell + "feature not enabled" from every other :class:`ConfigError` shape + (precedent: ``PAYG_NOT_AVAILABLE`` -- a missing project feature is a + configuration problem, and ``SessionAuthUnsupportedError`` for the + ConfigError-with-a-code pattern). + """ + + def __init__(self, message: str) -> None: + super().__init__(message) + self.error_code = ErrorCode.FEATURE_NOT_ENABLED + + class SyncConflictError(Exception): """Raised when ``sync pull --force`` would overwrite locally-modified configs whose remote **also** changed since the last pull -- a true 3-way @@ -339,6 +361,7 @@ def __init__(self, feature: str, *, remedy: str = "") -> None: # it inherits the right category instead of silently taking the "api" # default -- a missing project feature is a configuration problem. ErrorCode.PAYG_NOT_AVAILABLE: "configuration", + ErrorCode.FEATURE_NOT_ENABLED: "configuration", # A refused-by-us safety guard, not an upstream fault: nothing was sent to # the API, and the caller fixes it by re-issuing the request with --force. ErrorCode.WORKSPACE_LOAD_COPY_TOO_LARGE: "validation", diff --git a/src/keboola_agent_cli/services/base.py b/src/keboola_agent_cli/services/base.py index b8e4e5af..33727dee 100644 --- a/src/keboola_agent_cli/services/base.py +++ b/src/keboola_agent_cli/services/base.py @@ -114,6 +114,21 @@ def project_error_entry( } +def find_default_branch_id(branches: list[dict[str, Any]]) -> int | None: + """The id of the ``isDefault`` branch in a ``list_dev_branches()`` result. + + One home for the ``isDefault`` scan previously copy-pasted across + services (config, sync, workspace, merge-request). Returns ``None`` when + no branch is flagged -- what that means (error vs. fallback) stays the + caller's decision. ``lib.py`` keeps its own loop deliberately: the SDK + facade does not import the services layer. + """ + for branch in branches: + if branch.get("isDefault"): + return int(branch["id"]) + return None + + def default_client_factory(stack_url: str, token: str) -> KeboolaClient: """Create a KeboolaClient with the given stack URL and token. diff --git a/src/keboola_agent_cli/services/config_service.py b/src/keboola_agent_cli/services/config_service.py index 7f441672..5b49764c 100644 --- a/src/keboola_agent_cli/services/config_service.py +++ b/src/keboola_agent_cli/services/config_service.py @@ -29,7 +29,7 @@ from ._config_clone import clone_config_method from ._config_set_guard import validate_set_paths from ._encryption import collect_secrets, encrypt_secrets_in_config, find_plaintext_secret_keys -from .base import BaseService, ClientFactory, sanitize_unexpected_error +from .base import BaseService, ClientFactory, find_default_branch_id, sanitize_unexpected_error from .workspace_service import find_storage_workspace_for_sandbox_config AiClientFactory = Callable[[str, str], AiServiceClient] @@ -168,10 +168,7 @@ def _fetch_project_configs( folder_branch_id = effective_branch_id if not folder_branch_id: # Fetch default branch ID from dev-branches endpoint - branches = client.list_dev_branches() - default = next((b for b in branches if b.get("isDefault")), None) - if default: - folder_branch_id = default["id"] + folder_branch_id = find_default_branch_id(client.list_dev_branches()) if folder_branch_id: result = client.list_config_folder_metadata(branch_id=folder_branch_id) folder_map = result if isinstance(result, dict) else {} @@ -1487,9 +1484,9 @@ def _resolve_metadata_branch_id( f"Unexpected error listing branches for metadata route: {exc}. " "Pass --branch explicitly." ) from exc - default = next((b for b in branches if b.get("isDefault")), None) - if default: - return int(default["id"]) + default_branch_id = find_default_branch_id(branches) + if default_branch_id is not None: + return default_branch_id raise ConfigError( "Could not determine a branch for config metadata. " "Set an active branch with 'kbagent branch use' or pass --branch." diff --git a/src/keboola_agent_cli/services/merge_request_service.py b/src/keboola_agent_cli/services/merge_request_service.py index b2cc2f0e..835f84f7 100644 --- a/src/keboola_agent_cli/services/merge_request_service.py +++ b/src/keboola_agent_cli/services/merge_request_service.py @@ -23,12 +23,11 @@ from typing import Any from ..client import KeboolaClient -from ..config_store import ConfigError from ..constants import BRANCHES_MERGE_REQUESTS_FEATURE -from ..errors import ErrorCode, KeboolaApiError +from ..errors import ConfigError, ErrorCode, FeatureNotEnabledError, KeboolaApiError from ..json_utils import DiffEntry, compute_diff_entries from ..models import ProjectConfig -from .base import BaseService +from .base import BaseService, find_default_branch_id logger = logging.getLogger(__name__) @@ -62,6 +61,13 @@ } +# Everything `list_merge_requests`' --state filter accepts: the derived +# vocabulary plus the raw lifecycle states (the two overlap on purpose). +_STATE_FILTER_VOCABULARY: frozenset[str] = ( + frozenset(_DERIVED_STATE_BY_RAW) | frozenset(_DERIVED_STATE_BY_RAW.values()) | {"rejected"} +) + + def _same_id(a: Any, b: Any) -> bool: """Compare two ids that may arrive as int or str (approverId is a string on the wire while creator.id and reviewer.id are numbers).""" @@ -226,7 +232,7 @@ def _require_merge_requests_feature(self, client: KeboolaClient) -> None: """ if client.has_feature(BRANCHES_MERGE_REQUESTS_FEATURE): return - raise ConfigError( + raise FeatureNotEnabledError( "Merge requests are not enabled on this project: the " f"'{BRANCHES_MERGE_REQUESTS_FEATURE}' feature is missing. Without this " "pre-flight the API would answer an unexplained 403 (identical to a role " @@ -243,8 +249,16 @@ def list_merge_requests(self, alias: str, state: str | None = None) -> dict[str, ``state`` filters client-side (the endpoint declares no query parameters): a value matching either the derived vocabulary (``rejected``, ``merged``, ``closed``, ...) or a raw lifecycle state - (``published``, ...) keeps the row; matching is case-insensitive. + (``published``, ...) keeps the row; matching is case-insensitive. An + unknown value is refused with the accepted list -- the vocabulary is + closed and known here, and a typo returning a silent ``count: 0`` + would read as "no MRs". """ + if state is not None and state.lower() not in _STATE_FILTER_VOCABULARY: + raise ConfigError( + f"Unknown --state value {state!r}. Accepted values: " + f"{', '.join(sorted(_STATE_FILTER_VOCABULARY))}." + ) project = self._project(alias) client = self._client_factory(project.stack_url, project.token) try: @@ -281,7 +295,7 @@ def find_merge_request_for_branch(self, alias: str, branch_id: int) -> dict[str, client = self._client_factory(project.stack_url, project.token) try: for mr in client.merge_requests.list(): - if (mr.get("branches") or {}).get("branchFromId") == branch_id: + if _same_id((mr.get("branches") or {}).get("branchFromId"), branch_id): return {"alias": alias, **_enrich_row(mr)} finally: client.close() @@ -319,10 +333,15 @@ def get_merge_request( conflicts: list[dict[str, Any]] | None = None if (mr.get("state") or "") in self._OPEN_STATES: conflicts = client.merge_requests.conflicts(merge_request_id) - # verify_token is cheap (and refreshes the features cache); its - # admin block anchors the viewer-relative flags. A scoped token - # has no admin identity -> viewer flags are None, not False. - admin_id = client.verify_token().admin_id + # The verify_token call exists only to anchor the viewer + # polyfill (its admin block is the caller's identity); once the + # server serializes `viewer` (DMD-1988), derive_viewer never + # reads admin_id -- so skip the call and its cost with it. A + # scoped token has no admin identity -> flags are None, not + # False. + admin_id: int | None = None + if not isinstance(mr.get("viewer"), dict): + admin_id = client.verify_token().admin_id finally: client.close() @@ -475,9 +494,9 @@ def request_changes( def _default_branch_id(self, client: KeboolaClient, alias: str) -> int: """Resolve the project's default branch id (the only legal MR target).""" - for branch in client.list_dev_branches(): - if branch.get("isDefault"): - return int(branch["id"]) + branch_id = find_default_branch_id(client.list_dev_branches()) + if branch_id is not None: + return branch_id raise KeboolaApiError( message=f"Project '{alias}' reports no default branch -- cannot target a merge request.", status_code=0, @@ -522,10 +541,14 @@ def merge(self, alias: str, merge_request_id: int) -> dict[str, Any]: try: self._require_merge_requests_feature(client) # branchFromId is nullable once the MR is published -- capture it - # from the pre-merge payload, not the post-merge one. - branch_from_id = ( + # from the pre-merge payload, not the post-merge one. Coerced to + # int so the was_active comparison and the mapping cleanup below + # cannot be defeated by a string-serialized wire id (the payload + # mixes int and str ids -- the reason _same_id exists). + raw_branch_from = ( client.merge_requests.get(merge_request_id).get("branches") or {} ).get("branchFromId") + branch_from_id = int(raw_branch_from) if raw_branch_from is not None else None try: job = client.merge_requests.merge(merge_request_id) except KeboolaApiError as exc: @@ -614,9 +637,14 @@ def _remap_merge_conflict(self, exc: KeboolaApiError) -> None: # -- Conflicts / diff / resolution -------------------------------------------- - # Content-bearing keys of a diff side -- what a three-way comparison is - # about. Version/identifier fields differ by construction on a conflict - # and would only add noise to the per-path classification. + # Content-bearing keys of a diff side's ``diff`` envelope -- what a + # three-way comparison is about. Wire truth (ConfigurationDiffData, + # verified against connection): each side serializes as + # ``{version, isDeleted, diff: {name, description, changeDescription, + # isDisabled, configuration, rows}}`` -- content nested under ``diff``, + # version/deletion as side metadata. ``changeDescription`` is excluded: + # it is a per-version commit message, not content to resolve (the rebase + # takes its own ``change_description``). _DIFF_CONTENT_KEYS = ("name", "description", "configuration", "isDisabled", "rows") def list_conflicts(self, alias: str, merge_request_id: int) -> dict[str, Any]: @@ -646,10 +674,17 @@ def get_config_diff( No three panes: each touched path is tagged ``changed_by`` -- ``ours`` (only the dev branch changed it), ``theirs`` (only - production), or ``both`` (the actual conflict hotspots). ``rows`` - compares wholesale (row-level three-way diffing is not attempted). - ``onto_version`` is the default-branch version a rebase re-anchors - onto -- the ``theirs.version`` trap spelled out once, here. + production), or ``both``. A ``both`` row where the two sides agree on + the identical value additionally carries ``agreed: true`` -- both + sides moved, but there is nothing to decide; the actual conflict + hotspots are the ``both`` rows without it. ``rows`` compares + wholesale (row-level three-way diffing is not attempted). + + Deletions do not show up as paths: a soft-deleted side is flagged by + the top-level ``ours_deleted`` / ``theirs_deleted`` booleans instead + (``None`` = the side does not exist at all). ``onto_version`` is the + default-branch version a rebase re-anchors onto -- the + ``theirs.version`` trap spelled out once, here. """ project = self._project(alias) client = self._client_factory(project.stack_url, project.token) @@ -658,12 +693,17 @@ def get_config_diff( finally: client.close() theirs = diff.get("theirs") or {} + ours = diff.get("ours") return { "alias": alias, "component_id": component_id, "config_id": config_id, "branch_id": branch_id, "onto_version": theirs.get("version"), + "ours_deleted": bool(ours.get("isDeleted")) if ours is not None else None, + "theirs_deleted": ( + bool(theirs.get("isDeleted")) if diff.get("theirs") is not None else None + ), "changes": self._classify_three_way(diff), "diff": diff, } @@ -672,8 +712,10 @@ def _classify_three_way(self, diff: dict[str, Any]) -> list[dict[str, Any]]: """Intersect the two pairwise diffs (base->ours, base->theirs) per path.""" def content(side: dict[str, Any] | None) -> dict[str, Any]: - side = side or {} - return {key: side[key] for key in self._DIFF_CONTENT_KEYS if key in side} + # The side's content lives in its nested ``diff`` envelope (wire + # truth above); a null side contributes nothing. + envelope = (side or {}).get("diff") or {} + return {key: envelope[key] for key in self._DIFF_CONTENT_KEYS if key in envelope} base = content(diff.get("base")) ours_entries = {e.path: e for e in compute_diff_entries(base, content(diff.get("ours")))} @@ -700,15 +742,22 @@ def side_value(entry: DiffEntry | None, base_val: Any) -> Any: return entry.new if entry.new_present else None return base_val - changes.append( - { - "path": path, - "changed_by": changed_by, - "base": base_value, - "ours": side_value(ours_entry, base_value), - "theirs": side_value(theirs_entry, base_value), - } - ) + change: dict[str, Any] = { + "path": path, + "changed_by": changed_by, + "base": base_value, + "ours": side_value(ours_entry, base_value), + "theirs": side_value(theirs_entry, base_value), + } + if changed_by == "both": + # Identical independent changes are agreement, not a + # conflict hotspot -- flag them so renderers can demote them. + assert ours_entry is not None and theirs_entry is not None + change["agreed"] = ( + ours_entry.new_present == theirs_entry.new_present + and side_value(ours_entry, base_value) == side_value(theirs_entry, base_value) + ) + changes.append(change) return changes def resolve_conflict( @@ -717,7 +766,6 @@ def resolve_conflict( merge_request_id: int, component_id: str, config_id: str, - branch_id: int, take: str | None = None, resolved: dict[str, Any] | None = None, change_description: str | None = None, @@ -726,18 +774,27 @@ def resolve_conflict( mode goes through the rebase endpoint, per the RFC decision; the UI's reset-to-default alternative for take=theirs is DMD-1987). + The branch is NOT a parameter: it is derived from the merge request + itself (``branches.branchFromId``), so the conflict-set guard and the + branch being written to can never disagree -- a caller-supplied + branch id could point the rebase at an unrelated dev branch that the + guard never checked (rebase REPLACES; that would be silent data + loss). + Modes (exactly one of ``take`` / ``resolved``): - ``take="ours"``: keep the dev-branch content, re-anchored onto the - production version. An ours side that is deleted turns this into - the delete resolution. + production version. - ``take="theirs"``: adopt the production content (the config stays in the MR's changeset; the merge then writes a content-no-op). - ``take="delete"``: the ``{"version": N, "diff": {}}`` tombstone. - - ``resolved={...}``: a caller-authored three-way merge -- must carry - ``name``, ``rows`` and ``configuration`` explicitly (rebase - REPLACES; a missing key would silently wipe data, so it is refused - instead of defaulted). + A take of a side whose ``isDeleted`` is true collapses to this + resolution too -- "production deleted it, dev changed it" and its + mirror are live conflict shapes. + - ``resolved={...}``: a caller-authored three-way merge -- a FLAT + body that must carry ``name``, ``rows`` and ``configuration`` + explicitly (rebase REPLACES; a missing key would silently wipe + data, so it is refused instead of defaulted). The config must be in the MR's live conflict set; ``version`` is taken from the diff's ``theirs.version`` (the default-branch version @@ -753,6 +810,7 @@ def resolve_conflict( client = self._client_factory(project.stack_url, project.token) try: self._require_merge_requests_feature(client) + branch_id = self._branch_from_id_of(client, merge_request_id) self._require_in_conflict_set(client, merge_request_id, component_id, config_id) diff = client.get_config_diff(component_id, config_id, branch_id) theirs = diff.get("theirs") or {} @@ -768,27 +826,45 @@ def resolve_conflict( retryable=False, ) - side = resolved + # Normalize the three sources of a replace body onto one flat + # shape: a take side contributes its nested ``diff`` envelope, a + # caller-authored body is already flat. + body: dict[str, Any] | None = resolved resolution = "custom" if take == "delete": - side, resolution = None, "delete" - elif take == "ours": - ours = diff.get("ours") - if ours is None or ours.get("isDeleted"): - # Taking a deleted ours side IS the delete resolution. - side, resolution = None, "delete" + body, resolution = None, "delete" + elif take is not None: + side = diff.get("ours") if take == "ours" else diff.get("theirs") + if side is None or side.get("isDeleted"): + # Taking a deleted (or never-existing) side IS the delete + # resolution -- symmetric for ours and theirs. + body, resolution = None, "delete" else: - side, resolution = ours, "ours" - elif take == "theirs": - side, resolution = theirs, "theirs" + body, resolution = side.get("diff") or {}, take - if side is None: + if body is None: configuration = client.rebase_config_delete( component_id, config_id, branch_id, version=onto_version ) else: - missing = [key for key in ("name", "rows", "configuration") if key not in side] + missing = [key for key in ("name", "rows", "configuration") if key not in body] if missing: + if take is not None: + # The diff side's envelope is server-produced and its + # schema marks all content keys required -- a hole + # here is a backend contract violation, not caller + # error. Point at the manual path as the workaround. + raise KeboolaApiError( + message=( + f"The diff's {take} side carries no " + f"{', '.join(missing)} -- cannot compose a replace " + "body from it. Author the resolution manually and " + "pass it as a resolved body instead." + ), + status_code=0, + error_code=ErrorCode.VALIDATION_ERROR, + retryable=False, + ) raise ConfigError( "A resolved body must spell out the full replaced content " f"(rebase REPLACES): missing {', '.join(missing)}." @@ -798,11 +874,11 @@ def resolve_conflict( config_id, branch_id, version=onto_version, - name=side["name"], - rows=side["rows"], - configuration=side["configuration"], - is_disabled=bool(side.get("isDisabled", False)), - description=side.get("description"), + name=body["name"], + rows=body["rows"], + configuration=body["configuration"], + is_disabled=bool(body.get("isDisabled", False)), + description=body.get("description"), change_description=change_description, ) finally: @@ -819,6 +895,23 @@ def resolve_conflict( "configuration": configuration, } + def _branch_from_id_of(self, client: KeboolaClient, merge_request_id: int) -> int: + """The MR's source branch id -- the only branch a resolution may write to.""" + mr = client.merge_requests.get(merge_request_id) + branch_from_id = (mr.get("branches") or {}).get("branchFromId") + if branch_from_id is None: + raise KeboolaApiError( + message=( + f"Merge request {merge_request_id} has no source branch (state: " + f"{mr.get('state', 'unknown')}) -- a published or canceled MR " + "cannot be resolved." + ), + status_code=0, + error_code=ErrorCode.VALIDATION_ERROR, + retryable=False, + ) + return int(branch_from_id) + def _require_in_conflict_set( self, client: KeboolaClient, merge_request_id: int, component_id: str, config_id: str ) -> None: diff --git a/src/keboola_agent_cli/services/sync_service.py b/src/keboola_agent_cli/services/sync_service.py index 746a3ecc..40543d72 100644 --- a/src/keboola_agent_cli/services/sync_service.py +++ b/src/keboola_agent_cli/services/sync_service.py @@ -110,7 +110,7 @@ stamp_created_config, stamp_updated_config, ) -from .base import BaseService +from .base import BaseService, find_default_branch_id logger = logging.getLogger(__name__) @@ -308,11 +308,7 @@ def init_sync( if project_id is None: raise ConfigError("Token verification returned no project ID; cannot build manifest.") api_host = project.stack_url.replace("https://", "").rstrip("/") - default_branch_info = next( - (b for b in branches if b.get("isDefault")), - None, - ) - default_branch_id = default_branch_info["id"] if default_branch_info else None + default_branch_id = find_default_branch_id(branches) default_branch_name = "main" # Git branching setup diff --git a/src/keboola_agent_cli/services/workspace_service.py b/src/keboola_agent_cli/services/workspace_service.py index f41cd48f..0212020e 100644 --- a/src/keboola_agent_cli/services/workspace_service.py +++ b/src/keboola_agent_cli/services/workspace_service.py @@ -38,7 +38,7 @@ coerce_data_size_bytes, plan_auto_load_type, ) -from .base import BaseService +from .base import BaseService, find_default_branch_id logger = logging.getLogger(__name__) @@ -232,10 +232,9 @@ def _resolve_branch_id(self, alias: str, project: ProjectConfig) -> int: client = self._client_factory(project.stack_url, project.token) try: - branches = client.list_dev_branches() - for branch in branches: - if branch.get("isDefault", False): - return int(branch["id"]) + default_branch_id = find_default_branch_id(client.list_dev_branches()) + if default_branch_id is not None: + return default_branch_id raise ConfigError( f"No default branch found for project '{alias}'. " "Set an active branch with 'kbagent branch use'." diff --git a/tests/test_merge_request_service.py b/tests/test_merge_request_service.py index d146725f..0c813597 100644 --- a/tests/test_merge_request_service.py +++ b/tests/test_merge_request_service.py @@ -8,9 +8,19 @@ from __future__ import annotations +from pathlib import Path from typing import Any +from unittest.mock import MagicMock +import pytest + +from keboola_agent_cli.client import KeboolaClient +from keboola_agent_cli.client.merge_requests import MergeRequests +from keboola_agent_cli.config_store import ConfigStore +from keboola_agent_cli.errors import ConfigError, ErrorCode, KeboolaApiError +from keboola_agent_cli.models import ProjectConfig, TokenVerifyResponse from keboola_agent_cli.services.merge_request_service import ( + MergeRequestService, derive_allowed_actions, derive_merge_blockers, derive_state, @@ -183,18 +193,6 @@ def test_server_field_wins(self) -> None: # -- Service-level tests ------------------------------------------------------ -from pathlib import Path # noqa: E402 -from unittest.mock import MagicMock # noqa: E402 - -import pytest # noqa: E402 - -from keboola_agent_cli.config_store import ConfigError, ConfigStore # noqa: E402 -from keboola_agent_cli.errors import ErrorCode, KeboolaApiError # noqa: E402 -from keboola_agent_cli.models import ProjectConfig, TokenVerifyResponse # noqa: E402 -from keboola_agent_cli.services.merge_request_service import ( # noqa: E402 - MergeRequestService, -) - STACK_URL = "https://connection.keboola.com" TOKEN = "901-55555-fakeTestTokenDoNotUseXXXXXXXX" ALIAS = "prod" @@ -223,7 +221,11 @@ def store(tmp_config_dir: Path) -> ConfigStore: @pytest.fixture def client_factory() -> tuple[MagicMock, MagicMock]: - mock = MagicMock() + # spec'd at the L3 seam this PR builds on (#556): a renamed or removed + # client/namespace method fails these tests instead of silently keeping + # them green against an interface that no longer exists. + mock = MagicMock(spec=KeboolaClient) + mock.merge_requests = MagicMock(spec=MergeRequests) mock.verify_token.return_value = _verify_response() mock.has_feature.return_value = True factory = MagicMock(return_value=mock) @@ -600,15 +602,27 @@ def _diff( return {"base": base, "ours": ours, "theirs": theirs} -def _side(configuration: dict[str, Any], version: int = 5, **extra: Any) -> dict[str, Any]: +def _side( + configuration: dict[str, Any], + version: int = 5, + is_deleted: bool = False, + rows: list[dict[str, Any]] | None = None, + **diff_extra: Any, +) -> dict[str, Any]: + """One diff side in the verified wire shape (ConfigurationVersionResponse): + version/isDeleted as side metadata, content nested under ``diff``.""" return { "version": version, - "name": "My config", - "description": None, - "isDisabled": False, - "configuration": configuration, - "rows": [], - **extra, + "isDeleted": is_deleted, + "diff": { + "name": "My config", + "description": None, + "changeDescription": "edited", + "isDisabled": False, + "configuration": configuration, + "rows": rows if rows is not None else [], + **diff_extra, + }, } @@ -625,10 +639,10 @@ def test_classifies_ours_theirs_both(self, store, client_factory) -> None: ) result = _svc(store, factory).get_config_diff(ALIAS, "keboola.ex-db", "111", 123) by_path = {c["path"]: c for c in result["changes"]} - assert by_path["configuration.limit"]["changed_by"] == "both" assert by_path["configuration.limit"] == { "path": "configuration.limit", "changed_by": "both", + "agreed": False, "base": 100, "ours": 500, "theirs": 250, @@ -637,7 +651,22 @@ def test_classifies_ours_theirs_both(self, store, client_factory) -> None: # The side that did NOT touch the path still holds the base value. assert by_path["configuration.timeout"]["ours"] == 30 assert by_path["configuration.flag"]["changed_by"] == "ours" + assert "agreed" not in by_path["configuration.flag"] # only on `both` rows assert result["onto_version"] == 7 + assert result["ours_deleted"] is False + assert result["theirs_deleted"] is False + + def test_identical_change_on_both_sides_is_agreed(self, store, client_factory) -> None: + factory, mock = client_factory + mock.get_config_diff.return_value = _diff( + base=_side({"limit": 100}, version=3), + ours=_side({"limit": 500}, version=4), + theirs=_side({"limit": 500}, version=7), + ) + result = _svc(store, factory).get_config_diff(ALIAS, "keboola.ex-db", "111", 123) + (change,) = result["changes"] + assert change["changed_by"] == "both" + assert change["agreed"] is True def test_removed_key_shows_none_not_base(self, store, client_factory) -> None: factory, mock = client_factory @@ -649,24 +678,49 @@ def test_removed_key_shows_none_not_base(self, store, client_factory) -> None: result = _svc(store, factory).get_config_diff(ALIAS, "keboola.ex-db", "111", 123) (change,) = result["changes"] assert change["changed_by"] == "both" + assert change["agreed"] is False assert change["ours"] is None # removed, never the base value assert change["theirs"] == "new" - def test_null_base_means_added_on_both_sides(self, store, client_factory) -> None: + def test_deleted_side_is_flagged_not_a_path(self, store, client_factory) -> None: + # Deletion is side metadata (isDeleted, top-level on the wire), not a + # content path -- it must surface as a boolean, or the "Only you / + # Only production" rendering would hide the most consequential + # difference. + factory, mock = client_factory + mock.get_config_diff.return_value = _diff( + base=_side({"limit": 100}, version=3), + ours=_side({"limit": 100}, version=4), + theirs=_side({"limit": 100}, version=7, is_deleted=True), + ) + result = _svc(store, factory).get_config_diff(ALIAS, "keboola.ex-db", "111", 123) + assert result["theirs_deleted"] is True + assert result["ours_deleted"] is False + assert result["changes"] == [] # identical content, no paths + + def test_null_side_reports_none_deleted_flag(self, store, client_factory) -> None: factory, mock = client_factory mock.get_config_diff.return_value = _diff( base=None, - ours=_side({"a": 1}, version=4), + ours=None, # never existed on this side theirs=_side({"a": 2}, version=7), ) result = _svc(store, factory).get_config_diff(ALIAS, "keboola.ex-db", "111", 123) + assert result["ours_deleted"] is None by_path = {c["path"]: c for c in result["changes"]} - # A key absent from base diffs wholesale (compute_diff semantics): - # the whole `configuration` dict was added on both sides. - assert by_path["configuration"]["base"] is None - assert by_path["configuration"]["changed_by"] == "both" - assert by_path["configuration"]["ours"] == {"a": 1} - assert by_path["configuration"]["theirs"] == {"a": 2} + assert by_path["configuration"]["changed_by"] == "theirs" + + def test_change_description_is_not_content(self, store, client_factory) -> None: + # changeDescription is a per-version commit message; two sides always + # differ there and it is not something a resolution decides. + factory, mock = client_factory + mock.get_config_diff.return_value = _diff( + base=_side({"a": 1}, version=3, changeDescription="base msg"), + ours=_side({"a": 1}, version=4, changeDescription="ours msg"), + theirs=_side({"a": 1}, version=7, changeDescription="theirs msg"), + ) + result = _svc(store, factory).get_config_diff(ALIAS, "keboola.ex-db", "111", 123) + assert result["changes"] == [] def test_rows_compare_wholesale(self, store, client_factory) -> None: factory, mock = client_factory @@ -682,32 +736,61 @@ def test_rows_compare_wholesale(self, store, client_factory) -> None: class TestResolveConflict: - def _arm(self, mock: MagicMock, ours: dict[str, Any] | None = None) -> None: + def _arm( + self, + mock: MagicMock, + ours: dict[str, Any] | None = ..., + theirs: dict[str, Any] | None = ..., + ) -> None: # type: ignore[assignment] + mock.merge_requests.get.return_value = _wire_mr(7, "development", branch_from=123) mock.merge_requests.conflicts.return_value = [CONFLICT_ENTRY] mock.get_config_diff.return_value = _diff( base=_side({"limit": 100}, version=3), - ours=ours if ours is not None else _side({"limit": 500}, version=4), - theirs=_side({"limit": 250}, version=7), + ours=_side({"limit": 500}, version=4) if ours is ... else ours, + theirs=_side({"limit": 250}, version=7) if theirs is ... else theirs, ) mock.rebase_config.return_value = {"id": "111", "version": 8} mock.rebase_config_delete.return_value = {"id": "111", "version": 8, "isDeleted": True} + def test_branch_is_derived_from_the_mr_never_supplied(self, store, client_factory) -> None: + # Finding #1 of the PR review: a caller-supplied branch could point + # the rebase at a branch the conflict-set guard never checked. + factory, mock = client_factory + self._arm(mock) + result = _svc(store, factory).resolve_conflict( + ALIAS, 7, "keboola.ex-db", "111", take="ours" + ) + mock.get_config_diff.assert_called_once_with("keboola.ex-db", "111", 123) + args = mock.rebase_config.call_args.args + assert args == ("keboola.ex-db", "111", 123) + assert result["branch_id"] == 123 + + def test_closed_mr_cannot_be_resolved(self, store, client_factory) -> None: + factory, mock = client_factory + self._arm(mock) + mock.merge_requests.get.return_value = _wire_mr(7, "published", branch_from=None) + with pytest.raises(KeboolaApiError) as exc_info: + _svc(store, factory).resolve_conflict(ALIAS, 7, "keboola.ex-db", "111", take="ours") + assert exc_info.value.error_code == ErrorCode.VALIDATION_ERROR + mock.rebase_config.assert_not_called() + def test_take_ours_rebases_dev_content_onto_theirs_version(self, store, client_factory) -> None: factory, mock = client_factory self._arm(mock) result = _svc(store, factory).resolve_conflict( - ALIAS, 7, "keboola.ex-db", "111", 123, take="ours" + ALIAS, 7, "keboola.ex-db", "111", take="ours" ) kwargs = mock.rebase_config.call_args.kwargs assert kwargs["version"] == 7 # theirs.version, never ours' assert kwargs["configuration"] == {"limit": 500} + assert kwargs["name"] == "My config" # from the side's diff envelope assert result["resolution"] == "ours" assert result["onto_version"] == 7 def test_take_theirs_rebases_production_content(self, store, client_factory) -> None: factory, mock = client_factory self._arm(mock) - _svc(store, factory).resolve_conflict(ALIAS, 7, "keboola.ex-db", "111", 123, take="theirs") + _svc(store, factory).resolve_conflict(ALIAS, 7, "keboola.ex-db", "111", take="theirs") kwargs = mock.rebase_config.call_args.kwargs assert kwargs["configuration"] == {"limit": 250} assert kwargs["version"] == 7 @@ -716,26 +799,60 @@ def test_take_delete_sends_the_tombstone(self, store, client_factory) -> None: factory, mock = client_factory self._arm(mock) result = _svc(store, factory).resolve_conflict( - ALIAS, 7, "keboola.ex-db", "111", 123, take="delete" + ALIAS, 7, "keboola.ex-db", "111", take="delete" ) mock.rebase_config_delete.assert_called_once_with("keboola.ex-db", "111", 123, version=7) assert result["resolution"] == "delete" def test_take_ours_of_a_deleted_side_becomes_delete(self, store, client_factory) -> None: factory, mock = client_factory - self._arm(mock, ours=_side({}, version=4, isDeleted=True)) + self._arm(mock, ours=_side({}, version=4, is_deleted=True)) result = _svc(store, factory).resolve_conflict( - ALIAS, 7, "keboola.ex-db", "111", 123, take="ours" + ALIAS, 7, "keboola.ex-db", "111", take="ours" ) assert result["resolution"] == "delete" mock.rebase_config.assert_not_called() + def test_take_theirs_of_a_deleted_side_becomes_delete(self, store, client_factory) -> None: + # "Production deleted it, dev changed it" is a live conflict shape -- + # symmetric with the ours mirror (review finding #3). + factory, mock = client_factory + self._arm(mock, theirs=_side({}, version=7, is_deleted=True)) + result = _svc(store, factory).resolve_conflict( + ALIAS, 7, "keboola.ex-db", "111", take="theirs" + ) + assert result["resolution"] == "delete" + mock.rebase_config_delete.assert_called_once_with("keboola.ex-db", "111", 123, version=7) + + def test_take_side_without_content_keys_blames_the_server_not_the_caller( + self, store, client_factory + ) -> None: + # The side's diff envelope is server-produced with all content keys + # required -- a hole is a backend contract violation and the message + # must point at the manual path, not lecture the caller about a body + # they never supplied (review finding #2). + factory, mock = client_factory + self._arm(mock, theirs={"version": 7, "isDeleted": False, "diff": {"name": "x"}}) + with pytest.raises(KeboolaApiError) as exc_info: + _svc(store, factory).resolve_conflict(ALIAS, 7, "keboola.ex-db", "111", take="theirs") + assert exc_info.value.error_code == ErrorCode.VALIDATION_ERROR + assert "theirs side carries no" in exc_info.value.message + assert "resolved body" in exc_info.value.message # names the workaround + + def test_missing_theirs_version_is_refused(self, store, client_factory) -> None: + factory, mock = client_factory + self._arm(mock, theirs=None) + with pytest.raises(KeboolaApiError) as exc_info: + _svc(store, factory).resolve_conflict(ALIAS, 7, "keboola.ex-db", "111", take="ours") + assert exc_info.value.error_code == ErrorCode.VALIDATION_ERROR + assert "theirs side" in exc_info.value.message + def test_custom_body_must_spell_out_replaced_content(self, store, client_factory) -> None: factory, mock = client_factory self._arm(mock) with pytest.raises(ConfigError) as exc_info: _svc(store, factory).resolve_conflict( - ALIAS, 7, "keboola.ex-db", "111", 123, resolved={"name": "n"} + ALIAS, 7, "keboola.ex-db", "111", resolved={"name": "n"} ) assert "rows" in str(exc_info.value) and "configuration" in str(exc_info.value) mock.rebase_config.assert_not_called() @@ -745,7 +862,7 @@ def test_custom_body_rebases_verbatim(self, store, client_factory) -> None: self._arm(mock) body = {"name": "merged", "rows": [], "configuration": {"limit": 300}} result = _svc(store, factory).resolve_conflict( - ALIAS, 7, "keboola.ex-db", "111", 123, resolved=body, change_description="3-way" + ALIAS, 7, "keboola.ex-db", "111", resolved=body, change_description="3-way" ) kwargs = mock.rebase_config.call_args.kwargs assert kwargs["configuration"] == {"limit": 300} @@ -756,19 +873,17 @@ def test_exactly_one_mode_required(self, store, client_factory) -> None: factory, _ = client_factory svc = _svc(store, factory) with pytest.raises(ConfigError): - svc.resolve_conflict(ALIAS, 7, "c", "1", 123) + svc.resolve_conflict(ALIAS, 7, "c", "1") with pytest.raises(ConfigError): - svc.resolve_conflict(ALIAS, 7, "c", "1", 123, take="ours", resolved={}) + svc.resolve_conflict(ALIAS, 7, "c", "1", take="ours", resolved={}) with pytest.raises(ConfigError): - svc.resolve_conflict(ALIAS, 7, "c", "1", 123, take="mine") + svc.resolve_conflict(ALIAS, 7, "c", "1", take="mine") def test_config_outside_conflict_set_is_refused(self, store, client_factory) -> None: factory, mock = client_factory - mock.merge_requests.conflicts.return_value = [CONFLICT_ENTRY] + self._arm(mock) with pytest.raises(KeboolaApiError) as exc_info: - _svc(store, factory).resolve_conflict( - ALIAS, 7, "keboola.other", "999", 123, take="ours" - ) + _svc(store, factory).resolve_conflict(ALIAS, 7, "keboola.other", "999", take="ours") assert exc_info.value.error_code == ErrorCode.VALIDATION_ERROR mock.rebase_config.assert_not_called() @@ -780,3 +895,64 @@ def test_returns_count_and_raw_entries(self, store, client_factory) -> None: result = _svc(store, factory).list_conflicts(ALIAS, 7) assert result["count"] == 1 assert result["conflicts"] == [CONFLICT_ENTRY] + + +class TestReviewFollowUps: + """Regression tests for the PR #703 review findings.""" + + def test_string_wire_branch_id_still_matches_find(self, store, client_factory) -> None: + # Finding #6: MR payload ids mix int and str; a string-serialized + # branchFromId must not defeat the branch->MR resolver. + factory, mock = client_factory + mr = _wire_mr(1) + mr["branches"]["branchFromId"] = "123" + mock.merge_requests.list.return_value = [mr] + result = _svc(store, factory).find_merge_request_for_branch(ALIAS, 123) + assert result["id"] == 1 + + def test_string_wire_branch_id_still_resets_active_branch( + self, store, client_factory, monkeypatch + ) -> None: + factory, mock = client_factory + mr = _wire_mr(7, "approved") + mr["branches"]["branchFromId"] = "123" + mock.merge_requests.get.return_value = mr + mock.merge_requests.merge.return_value = {"id": 5, "status": "success", "results": {}} + monkeypatch.setattr( + "keboola_agent_cli.sync.branch_mapping.cleanup_branch_id_from_mapping", + lambda branch_id: None, + ) + store.set_project_branch(ALIAS, 123) + result = _svc(store, factory).merge(ALIAS, 7) + assert result["was_active"] is True + assert result["branch_from_id"] == 123 # coerced to int + assert store.get_project(ALIAS).active_branch_id is None + + def test_unknown_state_filter_is_refused_with_the_vocabulary( + self, store, client_factory + ) -> None: + factory, mock = client_factory + with pytest.raises(ConfigError) as exc_info: + _svc(store, factory).list_merge_requests(ALIAS, state="mereged") + assert "merged" in str(exc_info.value) # names the accepted values + mock.merge_requests.list.assert_not_called() + + def test_no_default_branch_is_a_readable_error(self, store, client_factory) -> None: + factory, mock = client_factory + mock.list_dev_branches.return_value = [DEV_BRANCH] # no isDefault anywhere + with pytest.raises(KeboolaApiError) as exc_info: + _svc(store, factory).create_merge_request(ALIAS, 123, "My MR") + assert "no default branch" in exc_info.value.message + mock.merge_requests.create.assert_not_called() + + def test_server_viewer_skips_the_verify_token_call(self, store, client_factory) -> None: + # Once DMD-1988 serializes `viewer`, the polyfill's cost (one + # verify_token round-trip per detail) must disappear with it. + factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr( + 7, "development", viewer={"isCreator": True, "hasApproved": False} + ) + mock.merge_requests.conflicts.return_value = [] + detail = _svc(store, factory).get_merge_request(ALIAS, 7) + mock.verify_token.assert_not_called() + assert detail["viewer"] == {"is_creator": True, "has_approved": False} From a0160a48740f29de887160cb86ec01128f6b663c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C5=A0ifra?= Date: Thu, 27 Aug 2026 02:02:38 +0200 Subject: [PATCH 10/15] fix(service): apply the Opus wire-truth review -- 409 codes, allowed_actions, derive_state honesty [DMD-1899] Adversarial review of every wire assumption against Connection source (tasks/pr-703-opus-wire-review.md): 7 CONFIRMED, 3 MISMATCH. The fixes: 409 mapping (finding #4): the conflict 409 is NOT code-less -- an earlier reading missed ExceptionConverter, which serializes MergeValidationException's own string code storage.mergeRequests.validation top-level as `code`, plus the conflicting configurations in `params.errors`. _remap_merge_conflict now matches BOTH codes explicitly (code-less 409 falls back to conflict for older stacks; a 409 with any OTHER code passes through unmapped instead of being confidently mislabeled a conflict), and http_base surfaces `params` as details.api_error_params so the conflict list travels with the error instead of demanding a second round trip. Stale 'carries no code' claims corrected in errors.py, the L3 docstring, error-codes.md, the notes wire-truth table and the RFC. allowed_actions (finding #7): `approve` removed from the `approved` tuple -- the transition's sole `from` place is in_review; from approved the backend answers 422 (the UI button offering it there is wrong). `update` added to in_merge (the server blocks update only in terminal states). Docstring records the AddApprovalGuard gating and that with the non-SOX default of 0 required approvals, approve is 422 in every state and in_review itself is unreachable. derive_state honesty (finding #10, the significant one): reviewers[].status is populated only within a review round anchored by a review_requested activity event -- which skip_review never writes -- and explicit reviewers shadow every non-reviewer's decision (the creator can never BE a reviewer). So in a default non-SOX project the rejected / self-closed derivations never fire; the UI badge has the identical blind spot, since our table is its port. Documented in the docstring and the RFC rather than re-derived from the activity log client-side: the reliable fix is server-side (commented on DMD-1988 -- derive from the activity log, not reviewers[]). Sharp edges from confirmed items: a take side with a null/empty `name` (nullable in the diff envelope, required non-empty by the rebase validator) is refused as a contract violation instead of sailing into a server 400; _branch_from_id_of documents that its null check is racy (branchFromId is nulled by the FK when the async branch delete lands, not by the state change); the viewer docstring notes detail/conflicts require an admin token anyway (MergeRequestVoter). Regression tests for each fix; 87 service tests total. Co-Authored-By: Claude Fable 5 --- docs/error-codes.md | 2 +- docs/merge-requests-layer2.md | 38 ++++-- docs/merge-requests-notes.md | 13 +- .../client/merge_requests.py | 4 +- src/keboola_agent_cli/errors.py | 7 +- src/keboola_agent_cli/http_base.py | 8 ++ .../services/merge_request_service.py | 125 +++++++++++++----- tests/test_http_base.py | 22 +++ tests/test_merge_request_service.py | 83 +++++++++++- 9 files changed, 247 insertions(+), 55 deletions(-) diff --git a/docs/error-codes.md b/docs/error-codes.md index 962c107c..a3fccf37 100644 --- a/docs/error-codes.md +++ b/docs/error-codes.md @@ -192,4 +192,4 @@ of `ErrorCode` in `src/keboola_agent_cli/errors.py`. | Code | Description | |---|---| | `MR_NOT_READY_TO_MERGE` | Merge answered 409 with `storage.mergeRequests.notReadyToMerge`: a project merge lock is held, the MR is in a state that cannot merge, or another MR in the project is already processing. Transient -- retryable | -| `MR_MERGE_CONFLICT` | Merge answered 409 without a machine code: configurations changed on both branches. Not retryable -- resolve the conflicts (`kbagent merge-request conflicts`) and merge again | +| `MR_MERGE_CONFLICT` | Merge answered 409 with `storage.mergeRequests.validation` (or, on older stacks, no code): configurations changed on both branches; `details.api_error_params.errors` lists them. Not retryable -- resolve the conflicts and merge again | diff --git a/docs/merge-requests-layer2.md b/docs/merge-requests-layer2.md index 30f2a068..9f287ffa 100644 --- a/docs/merge-requests-layer2.md +++ b/docs/merge-requests-layer2.md @@ -59,6 +59,19 @@ order; canonical vocabulary for all clients: | `in_merge` | `in_merge` (the one state the UI badge omits — we name it) | — | | `merged` | `published` | merged | +Reliability caveat (Opus wire review 2026-08-27, verified against Connection): the +`rejected` / self-`closed` rows depend on `reviewers[].status`, which the backend populates +only within a review round anchored by a real `request_review` activity event — and +`skip_review` writes none. With the non-SOX default of 0 required approvals, every +`request-review` takes the skip path, so `status` is always `null` and those two overrides +never fire; additionally, explicit reviewers shadow every non-reviewer's decision and the +creator can never *be* a reviewer (422). **The UI badge has the identical blind spot** — +this table is its port. The reliable source is the MR's **activity log** +(`changes_requested` events, un-anchored and un-shadowed), which is what DMD-1988 asks +Connection to derive `derivedState` from server-side. The CLI polyfill stays a best-effort +port of the UI on purpose: matching the UI's (flawed) behavior until the backend serializes +the truth beats maintaining a third, differently-wrong derivation. + **2. `merge_blockers`** (detail only; list omits it — conflicts are not fetched per row) — a *list*, not a single enum, so concurrent blockers don't mask each other; plus sugar `mergeable: bool` (= empty list). Purely mechanical, **not a guard** — the merge 409 stays @@ -75,10 +88,16 @@ sits in `development` and a non-SOX merge from there succeeds (auto-`skipReview` is told by `derived_state`, not by a fake blocker. **3. `allowed_actions`** (detail) — subset of `{request_review, approve, request_changes, -merge, update, resolve_conflicts}`, mechanically from the state machine (what the UI buttons -gate on: approve/reject only in `in_review|approved`, send-for-review only in `development`, -…). The polyfill does *not* mix roles/features in (the pre-flight owns those); the backend -adds them when it takes over. +merge, update, resolve_conflicts}`, mechanically from the state machine. Corrected against +the real workflow (Opus wire review 2026-08-27): `approve` exists **only in `in_review`** +(its sole `from` place — from `approved` the backend answers 422; the UI button showing it +there is wrong), and even in `in_review` it is further gated by `AddApprovalGuard` (not the +creator, not already approved, required count not reached) — with the non-SOX default of 0 +required approvals, `approve` is 422 in every state and `in_review` itself is unreachable. +`update` is blocked only in terminal states (an `in_merge` MR is still updatable); +`request_changes` from `in_review|approved`; send-for-review only in `development`. The +polyfill does *not* mix roles/features in (the pre-flight owns those); the backend adds them +when it takes over. **4. `viewer`** (detail) — `{is_creator, has_approved}`, relative to the caller's identity (admin id from `verify_token`, compared against `creator.id` and `approvals[].approverId`). @@ -119,7 +138,9 @@ The list endpoint has no query parameters — a `--state` filter is the service' The merge 409 has four causes in two shapes (`MergeAction.php:97-109`): three "not ready" cases carry the machine-readable `storage.mergeRequests.notReadyToMerge`; a conflict raises -`MergeValidationException` **without** that code. Today both fall through `http_base.py`'s +`MergeValidationException` with its own code `storage.mergeRequests.validation` (plus the +conflicting configurations in `params.errors` -- see the notes doc, corrected 2026-08-27). +Today both fall through `http_base.py`'s generic `API_ERROR` catch-all (`http_base.py:306-336`; neither 409 nor 422 is mapped, neither retryable). @@ -130,9 +151,10 @@ the generic `http_base` layer cannot tell it apart: - `MR_NOT_READY_TO_MERGE` — the 409 carrying `storage.mergeRequests.notReadyToMerge` (three causes: merge lock / wrong state / another MR processing; distinguishable only by message text, hence one code). Transient states → `retryable=True`. -- `MR_MERGE_CONFLICT` — a 409 from the merge endpoint *without* the string code (the body - lists the conflicting configurations). `retryable=False`, details carry a "run - merge-request conflicts" next-step hint. +- `MR_MERGE_CONFLICT` — the 409 carrying `storage.mergeRequests.validation` (matched by + code; a code-less 409 falls back here for older stacks, any *other* code passes through + unmapped). The body's `params.errors` lists the conflicting configurations and is passed + through in details. `retryable=False`, message names the conflicts command as next step. Names may be polished to the enum's convention at implementation time. Both must be documented in `docs/error-codes.md` (`scripts/check_error_codes.py` enforces in CI). diff --git a/docs/merge-requests-notes.md b/docs/merge-requests-notes.md index 23d106fa..f2b30cab 100644 --- a/docs/merge-requests-notes.md +++ b/docs/merge-requests-notes.md @@ -38,9 +38,14 @@ than enqueue: **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. "Not ready" can be told from "conflicted" on that -basis alone. +`storage.mergeRequests.notReadyToMerge`, while a **conflict** raises `MergeValidationException`, +whose own string code is **`storage.mergeRequests.validation`** (`getStringCode`, +`MergeValidationException.php:174-177`) -- serialized top-level as `code` by +`ExceptionConverter` (`legacy-app/.../ExceptionConverter.php:99-125`), alongside the human +message in `error` and **the conflicting configurations in `params.errors`** (the +HttpException context). "Not ready" vs "conflicted" is a code-vs-code match, not +code-vs-absence -- an earlier reading of `MergeAction` missed the converter and recorded the +conflict 409 as code-less (corrected by the Opus wire review, 2026-08-27). 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` @@ -194,7 +199,7 @@ final Layer 3 review: | `branchFromId`/`branchIntoId` must be JSON ints | `Assert\Type('int')` in `MergeRequestCreateRequest::getConstraint()` | | Non-default target & existing-MR-per-source-branch → **404** (not 400) | both throw `InvalidBranchException` in `MergeRequestCreateProcessor`, caught → `HTTP_NOT_FOUND` | | merge answers 202 + Storage job | `JsonResponse($job->toApiResponse(...), 202)` in `MergeAction` | -| 409: three "not ready" causes carry `storage.mergeRequests.notReadyToMerge`, a conflict does not | exactly 3 `throw new BranchIsNotReadyToMerge` sites in `MergeProcessor` → `createConflictException(stringCode: ...)`; `MergeValidationException` → plain 409 | +| 409: "not ready" carries `storage.mergeRequests.notReadyToMerge`, a conflict carries `storage.mergeRequests.validation` + `params.errors` with the conflicting configs | 3 `BranchIsNotReadyToMerge` sites in `MergeProcessor`; `MergeValidationException::getStringCode` + `ExceptionConverter.php:99-125` (re-verified 2026-08-27; previously mis-recorded as code-less) | | Failed merge rolls back the MR | `rollbackMerge` in `MergeDevBranchJob`'s catch | | Source branch deleted as a second job, no handle returned | `createAndEnqueueJobFromJob(..., DevBranchDelete::OPERATION_NAME, ...)` after commit in `MergeDevBranchJob` | | diff/rebase 400 on default branch | `ConfigurationRebaseNotAvailableOnDefaultBranchException` / diff OA doc → `createBadRequestException` | diff --git a/src/keboola_agent_cli/client/merge_requests.py b/src/keboola_agent_cli/client/merge_requests.py index db580aaa..dad159c3 100644 --- a/src/keboola_agent_cli/client/merge_requests.py +++ b/src/keboola_agent_cli/client/merge_requests.py @@ -313,7 +313,9 @@ def merge(self, merge_request_id: int) -> dict[str, Any]: The merge 409 has four causes in two response shapes (three "not ready" cases carry ``storage.mergeRequests.notReadyToMerge``, a - conflict does not); mapping them is Layer 2's concern. + conflict carries ``storage.mergeRequests.validation`` plus the + conflicting configurations in ``params.errors``); mapping them is + Layer 2's concern. """ response = self._requester.request("PUT", f"{_BASE}/{merge_request_id}/merge") return self._requester.wait_for_storage_job(response.json(), max_wait=MERGE_JOB_MAX_WAIT) diff --git a/src/keboola_agent_cli/errors.py b/src/keboola_agent_cli/errors.py index 0eac7860..f955260c 100644 --- a/src/keboola_agent_cli/errors.py +++ b/src/keboola_agent_cli/errors.py @@ -149,9 +149,10 @@ class ErrorCode(StrEnum): # Merge requests (DMD-1899). The merge 409 has four causes in two wire # shapes; these split exactly where the backend does (see - # docs/merge-requests-notes.md). Mapped in MergeRequestService.merge() -- - # only the service knows the 409 came from the merge endpoint, and the - # conflict shape carries no machine string code for http_base to key on. + # docs/merge-requests-notes.md): storage.mergeRequests.notReadyToMerge + # vs storage.mergeRequests.validation, both in the body's top-level + # `code`. Mapped in MergeRequestService.merge() -- only the service + # knows the 409 came from the merge endpoint. MR_NOT_READY_TO_MERGE = "MR_NOT_READY_TO_MERGE" MR_MERGE_CONFLICT = "MR_MERGE_CONFLICT" diff --git a/src/keboola_agent_cli/http_base.py b/src/keboola_agent_cli/http_base.py index 6634634d..e4075362 100644 --- a/src/keboola_agent_cli/http_base.py +++ b/src/keboola_agent_cli/http_base.py @@ -426,6 +426,14 @@ def _raise_api_error( api_error_code = body.get("code") if isinstance(api_error_code, str) and api_error_code: details["api_error_code"] = api_error_code + # A Package HttpException additionally serializes its context + # as `params` (ExceptionConverter) -- e.g. the merge-conflict + # 409 carries the conflicting configurations in + # `params.errors`. Surface it so a caller does not have to + # re-fetch data the error already delivered. + api_error_params = body.get("params") + if isinstance(api_error_params, dict) and api_error_params: + details["api_error_params"] = api_error_params # Real Keboola APIs answer with one of these keys in priority # order. Two caveats: # 1. Keboola Metastore puts the HTTP status code into `error` diff --git a/src/keboola_agent_cli/services/merge_request_service.py b/src/keboola_agent_cli/services/merge_request_service.py index 835f84f7..141e69d9 100644 --- a/src/keboola_agent_cli/services/merge_request_service.py +++ b/src/keboola_agent_cli/services/merge_request_service.py @@ -43,19 +43,31 @@ "canceled": "closed", } -# Mechanical action availability per raw state -- what the UI buttons gate on -# (approve/request-changes only in in_review|approved, send-for-review only in -# development; editing/rebasing is allowed in development, in_review and -# approved -- the dev branch is locked only while in_merge). `merge` appears -# for `development` because a non-SOX merge from there succeeds when approvals -# suffice (the backend auto-applies skip_review). Deliberately state-only: -# roles and features are the pre-flight's job client-side, and the backend can -# honor them once it serializes `allowedActions` (DMD-1988). +# Mechanical action availability per raw state, verified against the Symfony +# workflow (MergeRequestLifecycleStateMachine + guards; Opus wire review +# 2026-08-27): +# - `approve` exists ONLY in in_review (the transition's sole `from` place); +# from `approved` the backend answers 422. Even in in_review it is further +# gated by AddApprovalGuard (not the creator, not already approved, +# required count not yet reached) -- which no state-only table can express. +# With the non-SOX default of 0 required approvals, approve is 422 in every +# state and in_review itself is unreachable (request-review jumps straight +# to approved via skip_review). +# - `merge` appears for `development` because a non-SOX merge from there +# succeeds when approvals suffice (the backend auto-applies skip_review). +# - `update` is blocked server-side only in the terminal states +# (published/canceled) -- an in_merge MR is still updatable. +# - `resolve_conflicts` (diff+rebase) is allowed while the MR is open; the +# in_merge branch lock is enforced on the SOX path only, but rebasing +# mid-merge is pointless, so the table deliberately omits it there. +# Deliberately state-only: roles and features are the pre-flight's job +# client-side, and the backend can honor them once it serializes +# `allowedActions` (DMD-1988). _ALLOWED_ACTIONS_BY_STATE: dict[str, tuple[str, ...]] = { "development": ("request_review", "merge", "update", "resolve_conflicts"), "in_review": ("approve", "request_changes", "update", "resolve_conflicts"), - "approved": ("approve", "request_changes", "merge", "update", "resolve_conflicts"), - "in_merge": (), + "approved": ("request_changes", "merge", "update", "resolve_conflicts"), + "in_merge": ("update",), "published": (), "canceled": (), } @@ -92,6 +104,19 @@ def derive_state(mr: dict[str, Any]) -> str: the hood, so a self-closed MR keeps ``state=development``. - otherwise the raw state mapped through ``_DERIVED_STATE_BY_RAW`` (an unknown raw state passes through unchanged, defensively). + + Reliability caveat (verified against Connection, Opus wire review + 2026-08-27): ``reviewers[].status`` is populated only within a review + round anchored by an actual ``request_review`` event, and a non-reviewer's + decision (the creator's included -- the creator can never BE a reviewer) + is dropped whenever explicit reviewers exist. ``skip_review`` writes no + activity event, so in a project with the non-SOX default of 0 required + approvals every status is ``null`` and the ``rejected`` / self-``closed`` + overrides never fire -- the same blind spot the UI badge has, since this + table is its port. The truth lives in the MR's activity log + (``changes_requested`` events, un-shadowed and un-anchored); serializing a + reliable ``derivedState`` from it is exactly what DMD-1988 asks Connection + to do. The fallback here stays best-effort by design. """ server = mr.get("derivedState") if isinstance(server, str) and server: @@ -334,7 +359,11 @@ def get_merge_request( if (mr.get("state") or "") in self._OPEN_STATES: conflicts = client.merge_requests.conflicts(merge_request_id) # The verify_token call exists only to anchor the viewer - # polyfill (its admin block is the caller's identity); once the + # polyfill (its admin block is the caller's identity). Note the + # detail and conflicts endpoints themselves require an admin + # token (MergeRequestVoter denies a token with no admin) -- a + # scoped token fails above before viewer is ever derived; the + # None-flags path stays as defense in depth. Once the # server serializes `viewer` (DMD-1988), derive_viewer never # reads admin_id -- so skip the call and its cost with it. A # scoped token has no admin identity -> flags are None, not @@ -507,6 +536,9 @@ def _default_branch_id(self, client: KeboolaClient, alias: str) -> int: # -- Merge ------------------------------------------------------------------ _NOT_READY_CODE = "storage.mergeRequests.notReadyToMerge" + # MergeValidationException's string code (ExceptionConverter serializes + # it top-level as `code`, with the conflicting configs in `params.errors`). + _CONFLICT_CODE = "storage.mergeRequests.validation" def merge(self, alias: str, merge_request_id: int) -> dict[str, Any]: """Merge the MR into the default branch and clean local references. @@ -517,13 +549,18 @@ def merge(self, alias: str, merge_request_id: int) -> dict[str, Any]: when approvals are satisfied -- the backend auto-applies skip_review. The merge 409 is remapped onto its two wire shapes (the RFC's - decision; docs/error-codes.md): + decision; docs/error-codes.md). Both carry a machine string code + (ExceptionConverter serializes it top-level as ``code``): - ``storage.mergeRequests.notReadyToMerge`` (project merge lock / wrong state / another MR processing) -> ``MR_NOT_READY_TO_MERGE``, retryable -- all three causes are transient. - - a 409 without the string code is the conflict validation -> - ``MR_MERGE_CONFLICT``, not retryable, next step in the message. + - ``storage.mergeRequests.validation`` is the conflict validation -> + ``MR_MERGE_CONFLICT``, not retryable; the conflicting + configurations arrive in the 409's own ``params.errors`` and are + passed through in details. A code-less 409 is treated as a + conflict too (older stacks), but a 409 with any OTHER code passes + through unmapped -- never mislabeled as a conflict. A successful merge always also deletes the source branch -- as a second async job with no handle, so the result says the branch "is @@ -603,15 +640,20 @@ def merge(self, alias: str, merge_request_id: int) -> dict[str, Any]: return result def _remap_merge_conflict(self, exc: KeboolaApiError) -> None: - """Raise the RFC's dedicated error for a merge 409; return for others. - - Only this call site knows the 409 came from the merge endpoint, and - the conflict shape carries no machine string code -- which is why the - mapping cannot live in http_base (see docs/merge-requests-layer2.md). + """Raise the RFC's dedicated error for a known merge 409; return for others. + + Only this call site knows the 409 came from the merge endpoint -- + which is why the mapping cannot live in http_base (see + docs/merge-requests-layer2.md). Both shapes match on the top-level + ``code`` of the error body (surfaced as ``details.api_error_code``); + a code-less 409 falls back to the conflict interpretation for older + stacks, but a 409 carrying any *other* code passes through unmapped + rather than being confidently mislabeled a conflict. """ if exc.status_code != 409: return - if exc.details.get("api_error_code") == self._NOT_READY_CODE: + code = exc.details.get("api_error_code") + if code == self._NOT_READY_CODE: raise KeboolaApiError( message=( f"{exc.message} The merge lock, MR state or a concurrently " @@ -623,17 +665,23 @@ def _remap_merge_conflict(self, exc: KeboolaApiError) -> None: retryable=True, details=exc.details, ) from exc - raise KeboolaApiError( - message=( - f"{exc.message} Configurations changed on both branches. Inspect them " - "with `kbagent merge-request conflicts`, resolve each one, then merge " - "again (conflicts are re-validated live on every attempt)." - ), - status_code=409, - error_code=ErrorCode.MR_MERGE_CONFLICT, - retryable=False, - details=exc.details, - ) from exc + if code == self._CONFLICT_CODE or code is None: + # The 409 already carries the conflicting configurations in + # params.errors (surfaced as details.api_error_params) -- keep + # them so the caller does not need a second round trip; the + # conflicts command remains the way to re-inspect later. + raise KeboolaApiError( + message=( + f"{exc.message} Configurations changed on both branches. Inspect " + "them with `kbagent merge-request conflicts`, resolve each one, " + "then merge again (conflicts are re-validated live on every " + "attempt)." + ), + status_code=409, + error_code=ErrorCode.MR_MERGE_CONFLICT, + retryable=False, + details=exc.details, + ) from exc # -- Conflicts / diff / resolution -------------------------------------------- @@ -847,7 +895,13 @@ def resolve_conflict( component_id, config_id, branch_id, version=onto_version ) else: + # `name` must also be non-empty: the diff envelope declares + # it nullable, but the rebase validator requires a non-empty + # trimmed string -- a null would sail through a bare presence + # check straight into a server 400. missing = [key for key in ("name", "rows", "configuration") if key not in body] + if "name" not in missing and not str(body.get("name") or "").strip(): + missing.insert(0, "name") if missing: if take is not None: # The diff side's envelope is server-produced and its @@ -896,7 +950,14 @@ def resolve_conflict( } def _branch_from_id_of(self, client: KeboolaClient, merge_request_id: int) -> int: - """The MR's source branch id -- the only branch a resolution may write to.""" + """The MR's source branch id -- the only branch a resolution may write to. + + The null check is best-effort, not airtight: `branchFromId` is nulled + by the FK's ON DELETE SET NULL when the source branch row is deleted, + and that deletion is a separate async job -- a freshly published MR + can still carry the id for a while. Harmless: the rebase then fails + server-side (the MR is no longer open), it just fails later. + """ mr = client.merge_requests.get(merge_request_id) branch_from_id = (mr.get("branches") or {}).get("branchFromId") if branch_from_id is None: diff --git a/tests/test_http_base.py b/tests/test_http_base.py index eec5b3a4..0abbe961 100644 --- a/tests/test_http_base.py +++ b/tests/test_http_base.py @@ -1121,3 +1121,25 @@ def test_non_string_code_ignored(self, httpx_mock) -> None: assert "api_error_code" not in exc_info.value.details finally: client.close() + + def test_params_context_lands_in_details(self, httpx_mock) -> None: + # The merge-conflict 409 delivers the conflicting configurations in + # params.errors (ExceptionConverter serializes HttpException context) + # -- surface it so callers don't re-fetch data the error carried. + params = {"errors": [{"componentId": "c", "configurationId": "1"}]} + httpx_mock.add_response( + url=f"{STACK_URL}/test-path", + status_code=409, + json={ + "error": "Merge request 7 cannot be merged.", + "code": "storage.mergeRequests.validation", + "params": params, + }, + ) + client = self._client() + try: + with pytest.raises(KeboolaApiError) as exc_info: + client._do_request("GET", "/test-path") + assert exc_info.value.details["api_error_params"] == params + finally: + client.close() diff --git a/tests/test_merge_request_service.py b/tests/test_merge_request_service.py index 0c813597..5bdfe3fd 100644 --- a/tests/test_merge_request_service.py +++ b/tests/test_merge_request_service.py @@ -140,20 +140,29 @@ def test_development_offers_submit_and_direct_merge(self) -> None: "resolve_conflicts", ] - def test_review_states_gate_approve_and_request_changes(self) -> None: - # Mirrors the UI buttons: approve/request-changes only in - # in_review|approved. + def test_approve_exists_only_in_in_review(self) -> None: + # The approve transition's sole `from` place is in_review; from + # `approved` the backend answers 422 (Opus wire review 2026-08-27). assert "approve" in derive_allowed_actions(_mr("in_review")) - assert "approve" in derive_allowed_actions(_mr("approved")) + assert "approve" not in derive_allowed_actions(_mr("approved")) assert "approve" not in derive_allowed_actions(_mr("development")) + def test_request_changes_in_review_and_approved(self) -> None: + assert "request_changes" in derive_allowed_actions(_mr("in_review")) + assert "request_changes" in derive_allowed_actions(_mr("approved")) + def test_merge_not_offered_from_in_review(self) -> None: # in_review means approvals are insufficient by definition; the # moment they suffice the backend auto-transitions to approved. assert "merge" not in derive_allowed_actions(_mr("in_review")) - def test_locked_and_terminal_states_offer_nothing(self) -> None: - for state in ("in_merge", "published", "canceled"): + def test_in_merge_offers_only_update(self) -> None: + # The server blocks update only in the terminal states; an in_merge + # MR is still updatable (metadata), nothing else is sensible. + assert derive_allowed_actions(_mr("in_merge")) == ["update"] + + def test_terminal_states_offer_nothing(self) -> None: + for state in ("published", "canceled"): assert derive_allowed_actions(_mr(state)) == [] def test_unknown_state_offers_nothing(self) -> None: @@ -956,3 +965,65 @@ def test_server_viewer_skips_the_verify_token_call(self, store, client_factory) detail = _svc(store, factory).get_merge_request(ALIAS, 7) mock.verify_token.assert_not_called() assert detail["viewer"] == {"is_creator": True, "has_approved": False} + + +class TestOpusWireReviewFollowUps: + """Regression tests for the Opus wire-truth review (2026-08-27).""" + + def test_conflict_409_matches_the_validation_code(self, store, client_factory) -> None: + # The conflict 409 DOES carry a machine code + # (storage.mergeRequests.validation) plus the conflicting configs in + # params.errors -- both must survive into the remapped error. + factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "approved") + conflict_params = { + "errors": [{"componentId": "c", "configurationId": "1", "isDeleted": False}] + } + mock.merge_requests.merge.side_effect = KeboolaApiError( + message="Merge request 7 cannot be merged.", + status_code=409, + error_code=ErrorCode.API_ERROR, + details={ + "api_error_code": "storage.mergeRequests.validation", + "api_error_params": conflict_params, + }, + ) + with pytest.raises(KeboolaApiError) as exc_info: + _svc(store, factory).merge(ALIAS, 7) + assert exc_info.value.error_code == ErrorCode.MR_MERGE_CONFLICT + assert exc_info.value.details["api_error_params"] == conflict_params + + def test_unknown_409_code_passes_through_unmapped(self, store, client_factory) -> None: + # A future backend 409 with a different code must NOT be confidently + # mislabeled as a merge conflict. + factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "approved") + mock.merge_requests.merge.side_effect = KeboolaApiError( + message="something else entirely", + status_code=409, + error_code=ErrorCode.API_ERROR, + details={"api_error_code": "storage.somethingElse.entirely"}, + ) + with pytest.raises(KeboolaApiError) as exc_info: + _svc(store, factory).merge(ALIAS, 7) + assert exc_info.value.error_code == ErrorCode.API_ERROR + + def test_null_name_on_a_take_side_is_a_contract_violation(self, store, client_factory) -> None: + # The diff envelope declares name nullable, but the rebase validator + # requires a non-empty string -- a null must not sail through the + # presence check into a server 400. + factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "development", branch_from=123) + mock.merge_requests.conflicts.return_value = [CONFLICT_ENTRY] + side = _side({"limit": 250}, version=7) + side["diff"]["name"] = None + mock.get_config_diff.return_value = _diff( + base=_side({"limit": 100}, version=3), + ours=_side({"limit": 500}, version=4), + theirs=side, + ) + with pytest.raises(KeboolaApiError) as exc_info: + _svc(store, factory).resolve_conflict(ALIAS, 7, "keboola.ex-db", "111", take="theirs") + assert exc_info.value.error_code == ErrorCode.VALIDATION_ERROR + assert "name" in exc_info.value.message + mock.rebase_config.assert_not_called() From a55b22d41815bbf977508cd3195a63a8e597acf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C5=A0ifra?= Date: Thu, 27 Aug 2026 02:19:24 +0200 Subject: [PATCH 11/15] style(service): hoist side_value out of the loop, typed test sentinel [DMD-1899] Two quality nits from the pre-review self-audit: the side_value helper was re-defined on every classification loop iteration (it captures nothing loop-scoped -- hoisted above the loop); the test _arm fixture abused Ellipsis as a keep-default sentinel with a type: ignore -- replaced with a typed module object sentinel, which also unbreaks `make typecheck` over tests (ty rejects an EllipsisType default on a dict|None parameter). Also pinned down the test_changelog_render 'environmental' failures for good: they reproduce exactly when FORCE_COLOR is set (Warp exports FORCE_COLOR=3), which makes Rich emit ANSI inside the asserted substrings ('New: ' renders bold, splitting 'New: alpha thing.'). A main-branch test robustness issue, unrelated to this PR. Co-Authored-By: Claude Fable 5 --- .../services/merge_request_service.py | 17 ++++++++--------- tests/test_merge_request_service.py | 14 +++++++++----- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/keboola_agent_cli/services/merge_request_service.py b/src/keboola_agent_cli/services/merge_request_service.py index 141e69d9..ecb736b2 100644 --- a/src/keboola_agent_cli/services/merge_request_service.py +++ b/src/keboola_agent_cli/services/merge_request_service.py @@ -771,6 +771,14 @@ def content(side: dict[str, Any] | None) -> dict[str, Any]: e.path: e for e in compute_diff_entries(base, content(diff.get("theirs"))) } + def side_value(entry: DiffEntry | None, base_val: Any) -> Any: + # A side that changed the path shows its own value (None when + # it REMOVED the key -- never the base value); a side with no + # entry did not touch the path and still holds the base. + if entry is not None: + return entry.new if entry.new_present else None + return base_val + changes: list[dict[str, Any]] = [] for path in sorted(set(ours_entries) | set(theirs_entries)): ours_entry = ours_entries.get(path) @@ -781,15 +789,6 @@ def content(side: dict[str, Any] | None) -> dict[str, Any]: "both" if ours_entry and theirs_entry else ("ours" if ours_entry else "theirs") ) base_value = reference.old if reference.old_present else None - - def side_value(entry: DiffEntry | None, base_val: Any) -> Any: - # A side that changed the path shows its own value (None when - # it REMOVED the key -- never the base value); a side with no - # entry did not touch the path and still holds the base. - if entry is not None: - return entry.new if entry.new_present else None - return base_val - change: dict[str, Any] = { "path": path, "changed_by": changed_by, diff --git a/tests/test_merge_request_service.py b/tests/test_merge_request_service.py index 5bdfe3fd..f72cf0be 100644 --- a/tests/test_merge_request_service.py +++ b/tests/test_merge_request_service.py @@ -745,18 +745,22 @@ def test_rows_compare_wholesale(self, store, client_factory) -> None: class TestResolveConflict: + # Sentinel: "keep the fixture's default side" -- distinct from an + # explicit None, which means "the side does not exist". + _DEFAULT_SIDE: Any = object() + def _arm( self, mock: MagicMock, - ours: dict[str, Any] | None = ..., - theirs: dict[str, Any] | None = ..., - ) -> None: # type: ignore[assignment] + ours: Any = _DEFAULT_SIDE, + theirs: Any = _DEFAULT_SIDE, + ) -> None: mock.merge_requests.get.return_value = _wire_mr(7, "development", branch_from=123) mock.merge_requests.conflicts.return_value = [CONFLICT_ENTRY] mock.get_config_diff.return_value = _diff( base=_side({"limit": 100}, version=3), - ours=_side({"limit": 500}, version=4) if ours is ... else ours, - theirs=_side({"limit": 250}, version=7) if theirs is ... else theirs, + ours=_side({"limit": 500}, version=4) if ours is self._DEFAULT_SIDE else ours, + theirs=(_side({"limit": 250}, version=7) if theirs is self._DEFAULT_SIDE else theirs), ) mock.rebase_config.return_value = {"id": "111", "version": 8} mock.rebase_config_delete.return_value = {"id": "111", "version": 8, "isDeleted": True} From cbdf1d13b98c6689e36f72b491bd5f4c4f37e1fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C5=A0ifra?= Date: Thu, 27 Aug 2026 02:44:45 +0200 Subject: [PATCH 12/15] fix(service): one predicate for the server viewer block (Copilot review) [DMD-1899] get_merge_request's verify_token skip accepted ANY dict as a server viewer, while derive_viewer required isCreator/hasApproved keys -- a bare `viewer: {}` (or one with foreign keys) would skip the call and yield {None, None} instead of the local derivation. Both sites now share _server_viewer(), the single predicate for 'did DMD-1988 land', so they cannot disagree. Regression test included. Co-Authored-By: Claude Fable 5 --- .../services/merge_request_service.py | 21 ++++++++++++++++--- tests/test_merge_request_service.py | 14 +++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/src/keboola_agent_cli/services/merge_request_service.py b/src/keboola_agent_cli/services/merge_request_service.py index ecb736b2..07c312e7 100644 --- a/src/keboola_agent_cli/services/merge_request_service.py +++ b/src/keboola_agent_cli/services/merge_request_service.py @@ -192,6 +192,21 @@ def derive_allowed_actions(mr: dict[str, Any]) -> list[str]: return list(_ALLOWED_ACTIONS_BY_STATE.get(mr.get("state") or "", ())) +def _server_viewer(mr: dict[str, Any]) -> dict[str, Any] | None: + """The server-serialized ``viewer`` block, or None when absent/unusable. + + THE single predicate for "did DMD-1988 land": both ``derive_viewer`` and + ``get_merge_request``'s verify_token skip use it, so they can never + disagree on what counts as a usable server field (a bare ``viewer: {}`` + or one with foreign keys must fall back to the local derivation, not + silently yield None flags). + """ + server = mr.get("viewer") + if isinstance(server, dict) and ("isCreator" in server or "hasApproved" in server): + return server + return None + + def derive_viewer(mr: dict[str, Any], admin_id: int | None) -> dict[str, bool | None]: """Derive the caller-relative flags: am I the creator, did I approve. @@ -202,8 +217,8 @@ def derive_viewer(mr: dict[str, Any], admin_id: int | None) -> dict[str, bool | a blocker into a next step: ``approvals`` + ``has_approved=True`` means "wait for the other reviewers", not "approve it". """ - server = mr.get("viewer") - if isinstance(server, dict) and ("isCreator" in server or "hasApproved" in server): + server = _server_viewer(mr) + if server is not None: return { "is_creator": server.get("isCreator"), "has_approved": server.get("hasApproved"), @@ -369,7 +384,7 @@ def get_merge_request( # scoped token has no admin identity -> flags are None, not # False. admin_id: int | None = None - if not isinstance(mr.get("viewer"), dict): + if _server_viewer(mr) is None: admin_id = client.verify_token().admin_id finally: client.close() diff --git a/tests/test_merge_request_service.py b/tests/test_merge_request_service.py index f72cf0be..bd8eb4e2 100644 --- a/tests/test_merge_request_service.py +++ b/tests/test_merge_request_service.py @@ -1031,3 +1031,17 @@ def test_null_name_on_a_take_side_is_a_contract_violation(self, store, client_fa assert exc_info.value.error_code == ErrorCode.VALIDATION_ERROR assert "name" in exc_info.value.message mock.rebase_config.assert_not_called() + + def test_empty_server_viewer_falls_back_to_local_derivation( + self, store, client_factory + ) -> None: + # Copilot review: a `viewer: {}` (or one with foreign keys) must NOT + # skip verify_token -- the skip predicate and derive_viewer's + # server-field predicate are one function, so they cannot disagree. + factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "development", viewer={}) + mock.merge_requests.conflicts.return_value = [] + detail = _svc(store, factory).get_merge_request(ALIAS, 7) + mock.verify_token.assert_called_once() + # admin_id=42 == creator -> locally derived, not None/None + assert detail["viewer"] == {"is_creator": True, "has_approved": False} From 116bb6b66dca42b72a57d1041eacb63d4c85ecf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C5=A0ifra?= Date: Thu, 27 Aug 2026 02:56:37 +0200 Subject: [PATCH 13/15] feat(service): tell a SOX refusal apart from a missing feature [DMD-1899] The pre-flight refusal said the same thing to two very different projects: a SOX project (protected-default-branch -- refused as deliberate CLI policy, nothing to enable) and a plain project that simply does not have merge requests turned on (something to ask support for). The features cache is already loaded by the first has_feature call, so the distinction is free: a new PROTECTED_DEFAULT_BRANCH_FEATURE constant + a dedicated SOX message pointing at the Keboola UI, vs. the enable-the-feature hint. Both paths stay FeatureNotEnabledError / FEATURE_NOT_ENABLED -- the split is in the wording, not the contract. Co-Authored-By: Claude Fable 5 --- src/keboola_agent_cli/constants.py | 7 +++++ .../services/merge_request_service.py | 19 ++++++++---- tests/test_merge_request_service.py | 29 ++++++++++++++++++- 3 files changed, 49 insertions(+), 6 deletions(-) diff --git a/src/keboola_agent_cli/constants.py b/src/keboola_agent_cli/constants.py index 4583523f..b2df5922 100644 --- a/src/keboola_agent_cli/constants.py +++ b/src/keboola_agent_cli/constants.py @@ -495,6 +495,13 @@ def _resolve_app_name() -> str: # dominant `..._FEATURE` suffix convention; decided in DMD-1899.) BRANCHES_MERGE_REQUESTS_FEATURE: str = "branches-merge-requests" +# The SOX flavour of protected branches. kbagent does NOT support its +# approvals flow; the constant exists so the merge-request pre-flight can +# tell "this is a SOX project (unsupported)" from "merge requests are simply +# not enabled" when wording its refusal -- the features cache is already +# loaded at that point, so the distinction is free. +PROTECTED_DEFAULT_BRANCH_FEATURE: str = "protected-default-branch" + # --- Global Search --- # Feature flag that gates the Storage API ``GET /v2/storage/global-search`` # endpoint used by ``kbagent search`` (textual mode). Projects without this diff --git a/src/keboola_agent_cli/services/merge_request_service.py b/src/keboola_agent_cli/services/merge_request_service.py index 07c312e7..06b0faa1 100644 --- a/src/keboola_agent_cli/services/merge_request_service.py +++ b/src/keboola_agent_cli/services/merge_request_service.py @@ -23,7 +23,7 @@ from typing import Any from ..client import KeboolaClient -from ..constants import BRANCHES_MERGE_REQUESTS_FEATURE +from ..constants import BRANCHES_MERGE_REQUESTS_FEATURE, PROTECTED_DEFAULT_BRANCH_FEATURE from ..errors import ConfigError, ErrorCode, FeatureNotEnabledError, KeboolaApiError from ..json_utils import DiffEntry, compute_diff_entries from ..models import ProjectConfig @@ -272,13 +272,22 @@ def _require_merge_requests_feature(self, client: KeboolaClient) -> None: """ if client.has_feature(BRANCHES_MERGE_REQUESTS_FEATURE): return + # The features cache is already loaded by the has_feature call above, + # so telling a SOX project apart from a plain "not enabled" is free -- + # and the two need different advice: a SOX refusal is deliberate CLI + # policy, a missing feature is something to enable. + if client.has_feature(PROTECTED_DEFAULT_BRANCH_FEATURE): + raise FeatureNotEnabledError( + "This is a SOX project ('protected-default-branch'): kbagent " + "deliberately does not support its merge-request approvals flow, " + "although the server would accept some writes. Use the Keboola UI " + "for SOX merge requests." + ) raise FeatureNotEnabledError( "Merge requests are not enabled on this project: the " f"'{BRANCHES_MERGE_REQUESTS_FEATURE}' feature is missing. Without this " - "pre-flight the API would answer an unexplained 403 (identical to a role " - "denial). Note: kbagent deliberately refuses SOX projects that carry only " - "'protected-default-branch', although the server would accept some writes -- " - "the SOX approvals flow is not supported by this CLI." + "pre-flight the API would answer an unexplained 403 (identical to a " + "role denial). Ask Keboola support to enable the feature." ) # -- Reads ---------------------------------------------------------------- diff --git a/tests/test_merge_request_service.py b/tests/test_merge_request_service.py index bd8eb4e2..aa9708d9 100644 --- a/tests/test_merge_request_service.py +++ b/tests/test_merge_request_service.py @@ -17,7 +17,12 @@ from keboola_agent_cli.client import KeboolaClient from keboola_agent_cli.client.merge_requests import MergeRequests from keboola_agent_cli.config_store import ConfigStore -from keboola_agent_cli.errors import ConfigError, ErrorCode, KeboolaApiError +from keboola_agent_cli.errors import ( + ConfigError, + ErrorCode, + FeatureNotEnabledError, + KeboolaApiError, +) from keboola_agent_cli.models import ProjectConfig, TokenVerifyResponse from keboola_agent_cli.services.merge_request_service import ( MergeRequestService, @@ -1045,3 +1050,25 @@ def test_empty_server_viewer_falls_back_to_local_derivation( mock.verify_token.assert_called_once() # admin_id=42 == creator -> locally derived, not None/None assert detail["viewer"] == {"is_creator": True, "has_approved": False} + + def test_sox_project_gets_the_sox_refusal_not_a_generic_one( + self, store, client_factory + ) -> None: + # A SOX project (protected-default-branch, no branches-merge-requests) + # is refused as deliberate CLI policy -- the message must say so, not + # suggest enabling a feature the project deliberately does not have. + factory, mock = client_factory + mock.has_feature.side_effect = lambda f: f == "protected-default-branch" + with pytest.raises(FeatureNotEnabledError) as exc_info: + _svc(store, factory).create_merge_request(ALIAS, 123, "My MR") + assert "SOX" in str(exc_info.value) + assert exc_info.value.error_code == ErrorCode.FEATURE_NOT_ENABLED + mock.merge_requests.create.assert_not_called() + + def test_plain_project_gets_the_enable_hint(self, store, client_factory) -> None: + factory, mock = client_factory + mock.has_feature.return_value = False + with pytest.raises(ConfigError) as exc_info: + _svc(store, factory).create_merge_request(ALIAS, 123, "My MR") + assert "not enabled" in str(exc_info.value) + assert "SOX" not in str(exc_info.value) From 85a3454b602b24732ee89c66f4e926015e034bf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C5=A0ifra?= Date: Thu, 27 Aug 2026 03:26:54 +0200 Subject: [PATCH 14/15] docs(rfc): Layer 1 command RFC -- the merge-request group [DMD-1900] Turns the working notes into an implementation-ready RFC for the `kbagent merge-request` group over MergeRequestService (DMD-1899, #703). Decided in this pass: - `--mr-id` is optional everywhere, resolved `--mr-id` -> `resolve_branch()` -> `find_merge_request_for_branch()`; `merge` is not exempt. - `merge` is classified `destructive`, so `--deny-destructive` lets an agent run the whole flow and hands only the last step to a human. - No `--wait`/`--timeout` on merge in v1 (L3 always awaits, 600 s) and no `resolve --all` (rebase replaces; conflicts are meant to be walked). - A full `server/routers/merge_requests.py` ships with the commands, plus a serve-only `by-branch` route; routers are not gated by CI, so a skip would reach users as an HTTP 404 with nothing red. Facts the analysis surfaced that shape the commands: - The MR serializer emits no timestamps, so no date column is possible and the renderer must preserve the server's `createdAt DESC` order. - `FeatureNotEnabledError` carries `FEATURE_NOT_ENABLED`; the common `except ConfigError` idiom would flatten it to `CONFIG_ERROR`. - An empty `--reviewer-id` list is sent as `reviewerIds: []` and clears the reviewer set -- it must be normalised to None. - `detail`/`conflicts` 403 on a scoped token while `list` works. - `approve` answers 422 in every state on a 0-approval project, and `request-review` lands directly in `approved` -- neither has a happy path to assert, and there is no `close` command for the same reason. E2E is deliberately left open: no project carries the feature, kbagent cannot provision one, and the happy path necessarily merges into production. The RFC records the proposed path and marks it unsettled. Co-Authored-By: Claude Opus 5 (1M context) --- docs/merge-requests-layer1.md | 403 +++++++++++++++++++++++++++++----- 1 file changed, 342 insertions(+), 61 deletions(-) diff --git a/docs/merge-requests-layer1.md b/docs/merge-requests-layer1.md index bb2abae4..78191099 100644 --- a/docs/merge-requests-layer1.md +++ b/docs/merge-requests-layer1.md @@ -1,61 +1,342 @@ -# Merge requests — Layer 1 (commands), working notes - -Linear: [DMD-1900](https://linear.app/keboola/issue/DMD-1900). UX facts for the future -`merge-request` command group, all verified against the backend -(citations in [`merge-requests-notes.md`](merge-requests-notes.md)). Service-layer material is -in [`merge-requests-layer2.md`](merge-requests-layer2.md). - -## Naming - -- **`request-changes`, not `reject`.** The transition sends the MR back to `development` to be - revised and resubmitted; the terminal negative outcome is branch deletion. Layer 3's method - name already carries this care — the command name must too. -- **Rebase's `version` needs an explicit flag name.** It is the *default-branch* version being - re-anchored onto (from the diff's `theirs.version`) — expose something like - `--onto-version`, never a bare `--version`. -- The `update` command pairs with Layer 3's `merge_requests.update` (shipped per decision - 2026-08-18, so the method is not dead code) — auto-merge strategy, `external_id` (255 chars, - correlates an MR with a ticket). - -## Output and wording - -- **Merge output must say the source branch was deleted** (it always is — no keep-the-branch - option), without asserting it is already gone (second async job; see notes doc). -- **Branch names are not resolvable for finished MRs.** `branches.branchFromId` is nullable — - the branch is deleted for both `published` and `canceled` MRs — so any "IDs → names" - rendering must tolerate a missing branch. -- **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. -- The happy path on non-SOX defaults is short: `merge` works **directly from `development`** - (the backend auto-skips review when approvals suffice) — the command flow should not force - an unnecessary `request-review` step. - -## Flags and conveniences - -- **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). Duplicates are - de-duplicated server-side. -- **A resolve shortcut is worth considering**: `--take ours|theirs` composing the rebase - payload from the diff's server-side data instead of requiring the caller to author the full - resolved configuration. Layer 3 needs nothing for it — it is a convenience over - `get_config_diff` + `rebase_config`. -- `reason` on request-changes: cap 1000 characters. `AutoMergeStrategy`: - `immediately` | `scheduled` | `none`. -- `--state` filtering on list is client-side (no query params on the endpoint). -- **`--wait` on merge has no Layer 3 fire-and-forget to map to**: `merge()` always awaits the - Storage job (`MERGE_JOB_MAX_WAIT` 600 s). A `--timeout` can map to `max_wait`; a `--no-wait` - mode would need its own polling via `merge_requests.get()` — decide deliberately, don't copy - the `job run --wait` idiom blindly. - -## Bookkeeping when the commands land - -- E2E test per command (convention #16), needs a `branches-merge-requests` project. -- All convention-#17 silent-drift surfaces: `commands/context.py`, CLAUDE.md command list, - `keboola-expert.md`, `SKILL.md`, `commands-reference.md`, `gotchas.md` - (`(since vX.Y.Z)` tags), a workflow reference doc. -- `mcp_parity.py` entries (see layer2 doc, canary sequencing). -- Dedicated error codes for the merge 409 must be documented in `docs/error-codes.md`. -- Decide the fate of `kbagent branch merge` in the same release (deprecate-with-pointer). +# Merge requests — Layer 1 (commands), RFC + +Linear: [DMD-1900](https://linear.app/keboola/issue/DMD-1900). The `kbagent merge-request` +command group over `MergeRequestService` (Layer 2, DMD-1899). Backend facts with citations +live in [`merge-requests-notes.md`](merge-requests-notes.md); the service contract this RFC +consumes is [`merge-requests-layer2.md`](merge-requests-layer2.md); the HTTP client is +[`merge-requests-layer3.md`](merge-requests-layer3.md). Scope stays **non-SOX**. + +Layer 2 settles almost the whole surface: eleven service methods map to eleven commands with +no invention. What this RFC decides is the parts Layer 2 deliberately left to the caller — +target resolution, rendering, error presentation, risk classification — plus the surfaces +convention #17 requires. + +## Shape + +- Group name **`merge-request`**, mounted in `cli.py` under the `_DEV` help panel immediately + after `branch`; hidden alias **`mr`** (precedent: `sl` for `semantic-layer`). +- Two modules: `commands/merge_request.py` (Typer commands) and + `commands/_merge_request_render.py` (Rich renderers). **The split is decided up front, not + deferred.** Eleven commands at this repo's ~50-code-lines-per-command average already sit + near the `commands/*.py` soft ceiling of 800, and four renderers are non-trivial (list + table, detail panel, conflicts table, three-way diff). `output.py` is not an option: it is + at 1013 lines against a 1000 soft ceiling, so a fifth `format_*` family there would push it + over. Precedent for the private sibling: `_storage_describe.py`, `_auth_picker.py`. +- Group callback `check_cli_permission(ctx, "merge-request")`, as every group has. +- Service registered in `cli.py` as `ctx.obj["merge_request_service"]`. + +## Command surface + +Eleven commands, one per service method. `--project` follows the house rule everywhere +(`resolve_project_alias` on writes). + +| Command | Service method | Notes | +|---|---|---| +| `list --project A [--state V]` | `list_merge_requests` | `--state` filters client-side | +| `detail [--mr-id N] [--branch B] [--activity-log]` | `get_merge_request` | adds live conflicts for open MRs | +| `create --title T [--branch B] [--description D] [--reviewer-id ID ...] [--auto-merge-strategy S] [--auto-merge-at TS] [--external-id X]` | `create_merge_request` | source branch via `resolve_branch()` | +| `update [--mr-id N] [--title] [--description] [--reviewer-id ...] [--auto-merge-strategy] [--auto-merge-at] [--external-id]` | `update_merge_request` | | +| `request-review [--mr-id N]` | `request_review` | lands in `approved` directly (see below) | +| `approve [--mr-id N]` | `approve` | 422 on a 0-approval project (see below) | +| `request-changes [--mr-id N] [--reason TEXT]` | `request_changes` | `reason` capped at 1000 chars server-side | +| `merge [--mr-id N] [--yes]` | `merge` | confirmation unless `--yes` / `--json` | +| `conflicts [--mr-id N]` | `list_conflicts` | | +| `diff --component-id C --config-id I [--mr-id N] [--format short\|full] [--output PATH]` | `get_config_diff` | branch derived from the MR | +| `resolve --component-id C --config-id I (--take ours\|theirs\|delete \| --resolved JSON\|@file\|-) [--mr-id N] [--change-description TEXT]` | `resolve_conflict` | exactly one of `--take` / `--resolved` | + +`find_merge_request_for_branch` gets **no command of its own** — it is the resolver behind +`--mr-id` (below), and its result is visible in every command's output line. + +### `request-review` and `approve` on a default project + +Both commands exist because the state machine has them, but on a non-SOX project with the +default **0 required approvals** neither has a happy path: `request_review` is auto-finished by +the backend (`skip_review`), so the MR jumps straight to `approved` and `in_review` is never +reached — and `approve`, whose only `from` place is `in_review`, therefore answers **422 in +every state**. `merge` works directly from `development` anyway. The commands ship (a project +that raises the required count needs them), but their `--help` states the default-project +reality plainly rather than letting the user discover it as an unexplained 422. + +### Flag details that are not free choices + +- **`--reviewer-id` must normalise empty to `None`.** `_optional_mr_fields` + (`client/merge_requests.py:113`) includes `reviewerIds` whenever it is not `None`, so an + empty list is sent as `reviewerIds: []` and the server **replaces the reviewer set with + nothing**. A Typer repeatable option that yields `()` instead of `None` would therefore + silently clear all reviewers on any `update` that did not mention them. Normalise with an + explicit `or None` at the call site. +- **`update` semantics:** `None` = leave unchanged; an **empty string clears** + `description` / `externalId` (server-side `?? null` mapping). There is no clear-to-null for + anything else. Passing `--reviewer-id` at all **replaces** the set — it never appends. +- `--auto-merge-strategy` accepts exactly `immediately` | `scheduled` | `none`. +- `--external-id` is capped at 255 characters server-side. +- **`--state` stays a plain `str`, not a Typer enum.** The vocabulary lives in the service + (`_STATE_FILTER_VOCABULARY`) and an unknown value already fails with the accepted list. A + second copy as an `Enum` in Layer 1 is exactly the drift convention #17 exists to prevent. + The help string enumerates the values for discoverability. + +## Which merge request am I working on (decided 2026-08-27) + +`--mr-id` is **optional on every command that takes one**, resolved in three steps: + +1. `--mr-id` given → use it. +2. Otherwise `resolve_branch()` — the standard idiom of 11 command modules: explicit + `--branch`, else `active_branch_id` from `config.json`. +3. On that branch, `find_merge_request_for_branch()` → the MR id. + +With neither an id nor a branch, the error is the house wording: *pass `--branch` or run +`branch use`*. The chain is unambiguous because a branch has **at most one MR, ever** (the +backend's existence check has no state filter), so step 3 can only find one or none. + +This is why `find_merge_request_for_branch` was added to Layer 2 at all. It calls +`GET /v2/storage/merge-request` **on every invocation** — nothing is cached. `active_branch_id` +is persisted because it is a *user decision* that cannot be derived; branch→MR is *derivable +server state*, and persisting it would be a cache with no invalidation (someone merges from +the UI, and the CLI operates on a stale id). Persist decisions, derive facts. + +`merge` is **not** exempt from the fallback. It is the same principle as everywhere else, and +the risk is covered by the confirmation prompt plus the resolution line in the output. + +**Every command that resolved its target implicitly says so**, on stderr in human mode: + +``` +Info: Using active branch (ID: 123) for project 'acme' +Info: Resolved merge request #7 from branch 123 +``` + +## What the wire actually carries + +Verified against `MergeRequestResponseProvider.php` (Opus wire review, 2026-08-27). Two facts +change what the renderers can show: + +- **List and detail have a byte-identical item shape.** Detail adds `changeLog`, and + `activityLog` only with `?include=activityLog`. So anything the detail panel renders, the + list could too — the limit is width, not data. +- **There are no timestamps.** The serializer emits `id`, `state`, `title`, `description`, + `externalId`, `mergerName`, `creator{id,name}`, `approvals[]{approverId,approverName}`, + `reviewers[]{id,name,status}`, `branches{branchFromId,branchIntoId}` — and nothing else. + A `Created` / `Updated` column is **impossible**, not merely omitted. The list arrives + server-side `createdAt DESC`, so **the renderer must preserve the server's order** (newest + first) — it is the only chronological signal that survives. + +Type traps already handled in Layer 2, restated so renderers do not re-introduce them: +`creator.id` and `reviewers[].id` are ints, `approvals[].approverId` is a **string**; +`branches.branchFromId` is nullable once the source branch row is deleted. + +**Access:** every `/merge-request/{id}` endpoint — detail and conflicts included — goes through +`MergeRequestVoter`, which denies a token with no admin. **A scoped Storage token gets 403 on +`detail`, `conflicts`, `diff` and `resolve`, but `list` works** (the list action has no voter). +Layer 1 does not special-case this in v1; it surfaces as `ACCESS_DENIED` and is documented in +`gotchas.md`. See *Known gaps* below. + +## Human rendering + +**`list`** — a Rich table in server order: + +| ID | Status | Title | Author | Branch | Reviewers | + +`Status` is `derived_state`, not raw `state` — the whole point of the derivation is that the +CLI shows what the web shows. `Branch` renders `branches.branchFromId`, or `—` when null +(published/canceled MRs have no branch). `Reviewers` shows names with their status; an MR with +none renders empty rather than a placeholder. `External ID` and `Merged by` are shown only +when at least one row carries a value, so the common table stays narrow. + +**`detail`** — a panel, then sections: + +- header `#7 ` + `derived_state` +- **Mergeable / blocked** — `mergeable` plus `merge_blockers` spelled out (`conflicts (3)`, + `approvals`, `state`). Worded as information, never as a guarantee: the merge 409 is the + authority. +- **You** — from `viewer`: *you created this MR* / *you have approved*. `None` flags render as + nothing, never as "no". +- **Next** — `allowed_actions` rendered as the commands that produce them. +- branches from → into, reviewers with status, approvals +- **Change log** — the configurations the merge will apply. **Legitimately empty while the MR + sits in `development`**: the backend writes it at `request_review` / `skip_review`, not at + create. The renderer says so instead of showing a bare empty table. +- **Conflicts** — count + list, for open MRs only +- **Activity log** — only with `--activity-log` + +**Hint-next.** In human mode every command ends with a one-line next step (`create` → *when +you are done, `merge-request merge`*; a merge conflict → *run `merge-request conflicts`*). This +is Rich-only: `--json` already carries `allowed_actions`, and inventing a `next_steps` payload +in Layer 1 would mean the command layer manufacturing data the service never produced. No +`--no-hint-next` flag in v1 (one line is not noise). + +## Conflicts, diff, resolve + +**`conflicts`** — a table of `componentId` / `configurationId` / `isDeleted` / `message`. Note +the entry's `isDeleted` is the **dev-branch** side's flag, not production's. + +**`diff`** — three sections, from the service's per-path `changed_by` classification: + +- **Both changed** — the actual conflict hotspots. Rows carrying `agreed: true` are demoted + to the bottom of this section and marked as agreement (both sides moved, nothing to decide). +- **Only you changed** +- **Only production changed** + +Long values are elided by default; `--format full` prints them untruncated. Deletions never +appear as paths — they are the top-level `ours_deleted` / `theirs_deleted` flags, rendered as a +line above the table. + +`--output PATH` writes the **ours-prefilled resolution candidate**: the flat body +`--resolved` expects (`name`, `rows`, `configuration`, and optionally `description`, +`isDisabled`). This is the git-mergetool loop with a file as the third pane — edit, then +`resolve --resolved @file`. When the ours side is absent or deleted there is nothing to +prefill: refuse with a pointer to `--take delete` rather than writing a misleading skeleton. + +**`resolve` has no `--all` (decided 2026-08-27).** Rebase *replaces*; a bulk +`--all --take theirs` is a bulk irreversible overwrite of the dev branch behind one keystroke. +Conflicts are meant to be walked, not waved away. Layer 2 excluded it from v1 for the same +reason. A caller who genuinely wants the loop can write it in three lines of shell over +`conflicts --json`. + +## Merge + +- **No `--wait` / `--timeout` in v1 (decided 2026-08-27).** Layer 3 always awaits the Storage + job with `MERGE_JOB_MAX_WAIT` (600 s) and exposes no parameter; a `--timeout` would have to + be threaded through both already-reviewed layers, and `--no-wait` would need polling that + does not exist. The help says the command can block for up to 10 minutes. A timeout already + reports `STORAGE_JOB_TIMEOUT` → exit 4, which scripts can tell apart from a failure. +- **Confirmation** unless `--yes` or `--json`, the house pattern (`notification delete`, + `branch metadata-delete`). The prompt names the MR and the source branch that will be + deleted. +- **Wording:** the source branch **"is being deleted"**, never "is deleted" — the deletion is + a second async job with no handle. The service already words this; the renderer must not + rephrase it into a completed fact. +- The 409 arrives pre-mapped as `MR_MERGE_CONFLICT` or `MR_NOT_READY_TO_MERGE`, both already + carrying the next step in the message. Layer 1 prints them as-is. + +## Errors and exit codes + +- **`FeatureNotEnabledError` must not be flattened.** It is a `ConfigError` subclass carrying + `error_code = FEATURE_NOT_ENABLED`. The common `except ConfigError` idiom — and especially + the shared `_handle_config_service_error` (`commands/config.py:585`) — hardcodes + `ErrorCode.CONFIG_ERROR` and would throw that code away, leaving a `--json` consumer unable + to tell "merge requests are not enabled on this project" from a typo in the alias. Use + `getattr(exc, "error_code", ErrorCode.CONFIG_ERROR)`, the way `server/app.py:760` does. + Exit 5 either way. +- `KeboolaApiError` → `map_error_to_exit_code` unchanged. **`MR_NOT_READY_TO_MERGE` is + deliberately not added to the exit-4 set**: 4 means network/retryable-transport, and + conflating a backend "another MR is processing" with a connection failure would make exit 4 + useless. The error envelope already carries `retryable: true` and the code — that is what a + script branches on. +- No new error codes. `MR_MERGE_CONFLICT` / `MR_NOT_READY_TO_MERGE` shipped with Layer 2 and + are documented in `docs/error-codes.md`. + +## Permissions + +`merge` is **destructive** (decided 2026-08-27): it irreversibly deletes the source branch and +rewrites production — the same class as `branch.delete`. The useful consequence is the split +it creates: an agent under `--deny-destructive` can run the entire flow (create, review, +inspect conflicts, resolve them) and **must hand the last step to a human**. + +`resolve` stays **write** despite replacing configuration content: Keboola keeps configuration +versions, so a rebase adds a version rather than destroying the previous one. + +``` +merge-request.list read +merge-request.detail read +merge-request.conflicts read +merge-request.diff read +merge-request.create write +merge-request.update write +merge-request.request-review write +merge-request.approve write +merge-request.request-changes write +merge-request.resolve write +merge-request.merge destructive +merge-request.by-branch read # serve-only, see below +``` + +No `FLAG_ESCALATIONS` entries — no flag on any of these crosses into a higher class. + +## `kbagent serve` + +A full `server/routers/merge_requests.py` ships **in this PR** (decided 2026-08-27), prefix +`/merge-requests`, paths `/{project}/…` per the `branches.py` / `notifications.py` convention. +CONTRIBUTING requires the 1:1 mirror, the service returns plain dicts so it is cheap, and +`scripts/check_command_sync.py` deliberately does **not** gate routers — a missing route would +therefore reach users as an HTTP 404 with nothing red in CI. + +- Every command gets a route except **`diff --output PATH`**, which writes to the host's disk + and has no meaning over HTTP; `GET …/diff` returns the same payload and the caller writes its + own file. Document the skip in the PR description, as CONTRIBUTING asks. +- One route with **no CLI leaf command**: `GET /merge-requests/{project}/by-branch/{branch_id}`, + exposing `find_merge_request_for_branch` — over HTTP there is no "active branch" idiom to + hide it behind. Register `merge-request.by-branch` in `OPERATION_REGISTRY` and add it to + `SERVE_ONLY_OPERATIONS` so the dead-key check passes (precedent: `auth.projects`). +- Run `make endpoints-gen` (gated by `make endpoints-check`). + +## `kbagent branch merge` + +Deprecate-with-pointer, in the same release. **It is not a 1:1 replacement**: today's command +only builds a UI URL and works on **any** project, including one without the +`branches-merge-requests` feature. So the deprecation notice must be conditional — *if this +project has merge requests enabled, use `kbagent merge-request create` + `merge`* — and the +command keeps working unchanged. Removal is a later release's decision. + +## Bookkeeping + +### E2E (convention #16) + +**Still open — the E2E story is not settled by this RFC.** What follows is the analysis and +the proposed path, not a decision: how the group actually gets live coverage is a separate +conversation before implementation lands. + +No E2E project carries `branches-merge-requests` today, and **kbagent cannot provision one** — +`ManageClient` has `get_project` / `list_organization_projects` but no project create, and +Connection's own E2E suite creates its projects itself. A new project is not needed, though: +the feature is additive and kbagent already ships the command to turn it on, so the plan is a +one-time enablement of the existing E2E project with a super-admin manage token: + +``` +kbagent feature project-add --project kbagent-e2e --feature branches-merge-requests +``` + +Until it is enabled, the tests gate the way `conditional_flows` does — `pytest.skip` on a +`FEATURE_NOT_ENABLED` pre-flight — so the suite stays green and starts covering the group the +moment the flag lands. Two properties of the scenario are not obvious: + +- **The happy path merges into production.** There is no dry-run merge, so the test necessarily + creates a throwaway config in a dev branch, merges it, and then deletes it from production. + That is inside the blast radius the flow/config E2E tests already have, but it must be + written as an explicit teardown, not left to the next run. +- **`merge` takes a project-wide lock** and refuses while another MR in the project is + processing, so two concurrent E2E runs against the same project will collide with + `MR_NOT_READY_TO_MERGE`. Serialise the MR test or accept it as a known flake source. + +**`approve` and `request-review` have no happy path to assert.** On a non-SOX project with the +default 0 required approvals, `request_review` is auto-finished by the backend and the MR lands +directly in `approved` (`in_review` is unreachable), and `approve` — whose only `from` place is +`in_review` — answers **422 in every state**. The E2E for `approve` therefore asserts the +refusal, not a success, and the command's `--help` says so. This is not a kbagent limitation: +it is what a 0-approval project means. +- Convention #17 silent-drift surfaces, all mandatory: `commands/context.py` `AGENT_CONTEXT`, + the CLAUDE.md `## All CLI Commands` section, `keboola-expert.md` (tool-selection matrix + + version gate), `SKILL.md` triggers, `commands-reference.md`, `gotchas.md`, and a new + `merge-request-workflow.md` reference doc. +- `gotchas.md` entries, each tagged `(since vNEXT)`: the no-timestamps list shape; the + scoped-token 403 on everything but `list`; the empty change log in `development`; + `--reviewer-id` replacing rather than appending; the empty-string-clears rule; the source + branch being deleted asynchronously. +- `docs/web-server-endpoints.md` via `make endpoints-gen`. +- No `mcp_parity.py` work — that map was deleted with the MCP passthrough in 0.85.0. + +## Known gaps, documented rather than handled + +- **A scoped Storage token gets 403 on everything but `list`.** Wording it properly is Layer 2 + work (only the service knows which endpoint answered), and Layer 2 is already reviewed in + PR #703. v1 surfaces the raw `ACCESS_DENIED` and documents it; fix it the next time Layer 2 + is opened. +- **"What will this MR merge" is unavailable while the MR is in `development`.** The change log + is written at review time; the UI computes the preview client-side with no endpoint behind + it. Out of scope, as agreed for Layer 2. +- **`derived_state` never reports `rejected` / `closed` on a default non-SOX project.** Those + overrides read `reviewers[].status`, which is only populated inside a review round anchored + by a real `request_review` event — and with 0 required approvals every request-review takes + the `skip_review` path, which writes none. **The UI badge has the identical blind spot**, so + the CLI and the web still agree; the fix is server-side (DMD-1988). The practical consequence + for Layer 1: **there is no `close` command.** Creator-request-changes *is* the UI's cancel, + but it leaves `state=development`, so a `close` command would look like a no-op. It is + documented in `request-changes --help` and in `gotchas.md` instead. From 07daa5055e9e90f85e981a78c69c97db54c0038c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C5=A0ifra?= <me@martinsifra.cz> Date: Thu, 27 Aug 2026 03:51:47 +0200 Subject: [PATCH 15/15] fix(service): apply the Layer 1 RFC findings [DMD-1899] Seven findings surfaced while writing the Layer 1 command RFC (DMD-1900), all verified against Connection (tasks/dmd-1899-findings-from-layer1.md): 1. get_config_diff now derives the branch from the merge request (signature: alias, merge_request_id, component_id, config_id) -- the same make-the-wrong-call-unrepresentable reasoning resolve_conflict already carries; a caller-supplied branch id had no relation to any MR. The resolved branch_id is echoed in the result; a published/canceled MR is refused readably. The old tests passed the shifted positional args straight into MagicMock without failing -- rewritten to pin the wiring (the L3 diff call must receive branchFromId). 2. _enrich_row now adds allowed_actions alongside derived_state, so every return (list rows, find, create, update, transitions, merge's post-merge state) answers 'what can I do next' without a second call. 3. SAFETY FACT recorded: autoMergeStrategy=immediately makes a background backend tick merge any approved MR through the same MergeProcessor under a system token (AutoMergeCandidateRepository.php:38-47, AutoMergeTickHandler.php:86) -- create+request_review can end in a production merge with merge() never called. New notes section + one sentence in both create/update docstrings. 4. Notes wire-truth table gains the full MergeRequestResponse shape: merge{mergedAt,mergerId,mergerName} is NESTED, createdAt is top-level, autoMergeStrategy/autoMergeAt are response fields too (MergeRequestResponseProvider.php:86-117; list==detail item shape). 5. MergeRequestVoter recorded in the notes (admin-identity axis vs. the role axis -- not a contradiction), 403 added to the detail/conflicts rows in the layer3 doc, derive_viewer docstring softened (the None-flags branch is defense in depth, not a reachable path via get_merge_request). 6. An empty list result now carries feature_enabled from has_feature, so 'no merge requests' and 'feature not enabled' stop being the same 200 + []; the extra verify_token GET is spent only on the empty case. 7. STATE_FILTER_VOCABULARY and TAKE_MODES are public for Layer 1 to enumerate in help text and pre-validate to exit 2 (precedent: notification_service's KNOWN_EVENTS). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --- docs/merge-requests-layer3.md | 4 +- docs/merge-requests-notes.md | 25 ++++++ .../services/merge_request_service.py | 75 ++++++++++++++---- tests/test_merge_request_service.py | 79 +++++++++++++++++-- 4 files changed, 160 insertions(+), 23 deletions(-) diff --git a/docs/merge-requests-layer3.md b/docs/merge-requests-layer3.md index adde1038..d50f7f8d 100644 --- a/docs/merge-requests-layer3.md +++ b/docs/merge-requests-layer3.md @@ -19,13 +19,13 @@ Project-level — `isAvailableInBranch: false`, so **never** branch-prefixed: |---|---|---|---| | `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 | +| `GET /v2/storage/merge-request/{id}` | — | 200 | 404, 403 (scoped token -- `MergeRequestVoter` requires an admin identity) | | `PUT /v2/storage/merge-request/{id}` | JSON | 200 | 403, 404, 422 | | `PUT …/{id}/request-review` | — | 200 | 403, 404, 422 | | `PUT …/{id}/approve` | — | 200 | 403, 404, 422 | | `PUT …/{id}/request-changes` | JSON `{reason?}` | 200 | 403, 404, 422 | | `PUT …/{id}/merge` | — | **202** + a Job | **409**, 403, 404 | -| `GET …/{id}/conflicts` | — | 200 | 404 | +| `GET …/{id}/conflicts` | — | 200 | 404, 403 (scoped token -- same voter) | Branch-scoped — `isAvailableInBranch: true, isAvailableWithoutBranch: false`, so **always** branch-prefixed: diff --git a/docs/merge-requests-notes.md b/docs/merge-requests-notes.md index f2b30cab..40ac7e5a 100644 --- a/docs/merge-requests-notes.md +++ b/docs/merge-requests-notes.md @@ -127,6 +127,23 @@ stronger one. `bi_rMergeRequestsApprovals` has no unique constraint on `(mergeRequestId, idAdmin)` and `hasEnoughApprovals()` counts rows rather than distinct admins. +## Auto-merge: `immediately` merges WITHOUT anyone calling merge + +`autoMergeStrategy` is not metadata. A background tick selects every MR in `approved` whose +strategy is `immediately` (or `scheduled` with `autoMergeAt <= now`) -- +`AutoMerge/AutoMergeCandidateRepository.php:38-47` (`findCandidates`: `WHERE mr.state = +:approved AND (mr.autoMergeStrategy = :immediately OR (:scheduled AND autoMergeAt <= :now))`) +-- and drives it through the **same `MergeProcessor`** the merge endpoint uses, under a +system token (`AutoMerge/AutoMergeTickHandler.php:86`: +`$this->mergeProcessor->process($legacyRow, new SystemToken(...))`). A conflict blocks the +scheduled merge and the tick retries until it clears (`:88-94`). + +Consequence: on a non-SOX project with the default 0 required approvals, +`create(auto_merge_strategy="immediately")` + `request_review()` ends in a production merge +and the source branch's deletion, with `merge()` never called; an +`update(auto_merge_strategy="immediately")` on an already-approved MR is enough on its own. +Both service docstrings say so; Layer 1 escalates the flag's permission class accordingly. + ## Roles and feature gating (non-SOX) Every write carries `#[MergeRequestsAllowedRoles(roles: [ProjectRole::ROLE_ADMIN, @@ -137,6 +154,13 @@ request-review (`:43`), approve (`:43`), reject (`:49`), merge (`:40`). The `rev feature — so those roles carry no MR privileges in a non-SOX project. Reads (list, detail, conflicts) are `#[AsReadOnlyAction]` with no role whitelist. +Role whitelisting is not the only access axis, though: every `/merge-request/{id}` route -- +the read-only detail and conflicts actions included -- runs `MergeRequestVoter`, which denies +a token with **no admin identity** (`Voters/MergeRequestVoter.php:49-56`, via +`MergeRequestService::requireMergeRequest`). A scoped Storage token therefore gets 403 on +detail/conflicts while the un-votered `GET /merge-request` list still works. Different axis +(admin identity vs. role), not a contradiction of the sentence above. + All six MR writes accept **either** `protected-default-branch` **or** `branches-merge-requests`; the reads and `/diff` are ungated; `/rebase` alone requires `branches-merge-requests` (`StorageRouteGuard::canAccessStorageScope`, @@ -205,6 +229,7 @@ final Layer 3 review: | diff/rebase 400 on default branch | `ConfigurationRebaseNotAvailableOnDefaultBranchException` / diff OA doc → `createBadRequestException` | | Diff shape `base`/`ours`/`theirs`, each nullable | `ConfigurationDiffResponse` | | Each diff side = `{version, isDeleted, diff: {name, description, changeDescription, isDisabled, configuration, rows}}` -- content NESTED under `diff`, version/deletion as side metadata; all six `diff` keys `required` | `ConfigurationVersionResponse` + `ConfigurationDiffData` OA schemas (re-verified 2026-08-27; a flat-side assumption breaks every take/classify consumer) | +| Full `MergeRequestResponse` item: `id, creator{id,name}, title, description, state, branches{branchFromId,branchIntoId}, merge{mergedAt,mergerId,mergerName}, createdAt, externalId, autoMergeStrategy, autoMergeAt, approvals[], reviewers[]` -- `merge{}` is NESTED (no flat mergerName), `createdAt` is top-level; list and detail share this item shape byte-for-byte (detail adds `changeLog`, `?include=activityLog` adds `activityLog`) | `MergeRequestResponseProvider.php:86-117` (`getCreateMRResponseArray`), `:132-139` (list maps the same builder) | | Rebase replaces; missing `configuration` → `{}`, `isDisabled` → `false`, `description` → null | `RebaseRequest::mapValidatedData` (`?? new stdClass()`, `?? false`, `isset` → null); "complete 3-way diff result … fully replaces" verbatim in `ConfigurationRebaseService` docblock | | `rows` required for keep; `[]` deletes all rows; order = sort order | `validateDiffRows` + OA schema | | Empty `diff` `{}` = delete resolution (tombstone) | `validateDiff` empty-stdClass branch → `isDelete: true` | diff --git a/src/keboola_agent_cli/services/merge_request_service.py b/src/keboola_agent_cli/services/merge_request_service.py index 06b0faa1..96fa7cec 100644 --- a/src/keboola_agent_cli/services/merge_request_service.py +++ b/src/keboola_agent_cli/services/merge_request_service.py @@ -75,10 +75,15 @@ # Everything `list_merge_requests`' --state filter accepts: the derived # vocabulary plus the raw lifecycle states (the two overlap on purpose). -_STATE_FILTER_VOCABULARY: frozenset[str] = ( +# Public: Layer 1 enumerates it in --state help text and pre-validates to +# exit 2 (house precedent: notification_service's KNOWN_EVENTS). +STATE_FILTER_VOCABULARY: frozenset[str] = ( frozenset(_DERIVED_STATE_BY_RAW) | frozenset(_DERIVED_STATE_BY_RAW.values()) | {"rejected"} ) +# The resolve_conflict take modes. Public for the same reason. +TAKE_MODES: tuple[str, ...] = ("ours", "theirs", "delete") + def _same_id(a: Any, b: Any) -> bool: """Compare two ids that may arrive as int or str (approverId is a string @@ -211,10 +216,13 @@ def derive_viewer(mr: dict[str, Any], admin_id: int | None) -> dict[str, bool | """Derive the caller-relative flags: am I the creator, did I approve. ``admin_id`` is the caller's user id from ``verify_token`` (the response's - ``admin`` block); a scoped token has none, in which case both flags are - ``None`` -- honest "unknown", not ``False``. Server-first (``viewer`` - with ``isCreator``/``hasApproved``, DMD-1988). These flags are what turns - a blocker into a next step: ``approvals`` + ``has_approved=True`` means + ``admin`` block); when it is ``None`` both flags are ``None`` -- honest + "unknown", not ``False``. In practice that branch is defense in depth + rather than a reachable path: a scoped token (no admin identity) is + denied by ``MergeRequestVoter`` on the detail/conflicts endpoints before + this function ever runs. Server-first (``viewer`` with + ``isCreator``/``hasApproved``, DMD-1988). These flags are what turns a + blocker into a next step: ``approvals`` + ``has_approved=True`` means "wait for the other reviewers", not "approve it". """ server = _server_viewer(mr) @@ -234,9 +242,16 @@ def derive_viewer(mr: dict[str, Any], admin_id: int | None) -> dict[str, bool | def _enrich_row(mr: dict[str, Any]) -> dict[str, Any]: - """List-row enrichment: raw MR + derived_state (conflicts are not fetched - per row, so no blockers here -- detail-level enrichment does that).""" - return {**mr, "derived_state": derive_state(mr)} + """Row-level enrichment applied to every MR the service returns: raw MR + + derived_state + allowed_actions (both are free -- state-only). Conflicts + are not fetched here, so no blockers -- detail-level enrichment does that. + A --json consumer of create/update/transitions can answer "what can I do + next" without a second call (findings doc, DMD-1900).""" + return { + **mr, + "derived_state": derive_state(mr), + "allowed_actions": derive_allowed_actions(mr), + } class MergeRequestService(BaseService): @@ -303,15 +318,23 @@ def list_merge_requests(self, alias: str, state: str | None = None) -> dict[str, closed and known here, and a typo returning a silent ``count: 0`` would read as "no MRs". """ - if state is not None and state.lower() not in _STATE_FILTER_VOCABULARY: + if state is not None and state.lower() not in STATE_FILTER_VOCABULARY: raise ConfigError( f"Unknown --state value {state!r}. Accepted values: " - f"{', '.join(sorted(_STATE_FILTER_VOCABULARY))}." + f"{', '.join(sorted(STATE_FILTER_VOCABULARY))}." ) project = self._project(alias) client = self._client_factory(project.stack_url, project.token) try: rows = [_enrich_row(mr) for mr in client.merge_requests.list()] + # The list endpoint is ungated, so a project without the feature + # answers 200 + [] -- indistinguishable from a genuinely empty + # project, while every subsequent write would fail. Disambiguate + # only for the empty result (the has_feature call costs one + # verify_token GET, so it is not spent on non-empty lists). + feature_enabled = ( + client.has_feature(BRANCHES_MERGE_REQUESTS_FEATURE) if not rows else None + ) finally: client.close() @@ -328,6 +351,8 @@ def list_merge_requests(self, alias: str, state: str | None = None) -> dict[str, "count": len(rows), "merge_requests": rows, } + if feature_enabled is not None: + result["feature_enabled"] = feature_enabled if state is not None: result["state_filter"] = state return result @@ -435,6 +460,13 @@ def create_merge_request( readable error) and the output must state which branch the MR was created from. A source branch can have at most one MR, ever; a second create answers 404 server-side. + + ``auto_merge_strategy="immediately"`` is NOT metadata: a background + backend tick merges any *approved* MR carrying it (same + MergeProcessor as the merge endpoint, under a system token) -- and on + non-SOX defaults ``request_review`` lands straight in approved, so + create+submit alone can end in a production merge and the source + branch's deletion. See the notes doc, *Auto-merge*. """ project = self._project(alias) client = self._client_factory(project.stack_url, project.token) @@ -480,7 +512,12 @@ def update_merge_request( ) -> dict[str, Any]: """Update an MR's metadata. ``None`` = leave unchanged (the API cannot clear a field to null; an empty string clears description/externalId - server-side).""" + server-side). + + ``auto_merge_strategy="immediately"`` on an already-approved MR is + enough for the backend's auto-merge tick to merge it -- no ``merge()`` + call involved (see the notes doc, *Auto-merge*). Not just metadata. + """ project = self._project(alias) client = self._client_factory(project.stack_url, project.token) try: @@ -657,6 +694,7 @@ def merge(self, alias: str, merge_request_id: int) -> dict[str, Any]: if mr_after.get("state"): result["state"] = mr_after["state"] result["derived_state"] = derive_state(mr_after) + result["allowed_actions"] = derive_allowed_actions(mr_after) if mapping_cleanup: result["mapping_cleanup"] = mapping_cleanup if cleanup_warnings: @@ -740,10 +778,17 @@ def list_conflicts(self, alias: str, merge_request_id: int) -> dict[str, Any]: } def get_config_diff( - self, alias: str, component_id: str, config_id: str, branch_id: int + self, alias: str, merge_request_id: int, component_id: str, config_id: str ) -> dict[str, Any]: """Three-way diff of one config, flattened to a per-path classification. + The branch is derived from the merge request (``branches.branchFromId``) + for the same reason ``resolve_conflict`` derives it: a caller-supplied + branch id has no relation to the MR, so the diff could silently show an + unrelated branch (findings doc, DMD-1899/DMD-1900). One GET buys the + symmetry; the resolved ``branch_id`` is echoed in the result so Layer 1 + never re-derives the branch->MR relation itself. + No three panes: each touched path is tagged ``changed_by`` -- ``ours`` (only the dev branch changed it), ``theirs`` (only production), or ``both``. A ``both`` row where the two sides agree on @@ -761,6 +806,7 @@ def get_config_diff( project = self._project(alias) client = self._client_factory(project.stack_url, project.token) try: + branch_id = self._branch_from_id_of(client, merge_request_id) diff = client.get_config_diff(component_id, config_id, branch_id) finally: client.close() @@ -768,6 +814,7 @@ def get_config_diff( ours = diff.get("ours") return { "alias": alias, + "merge_request_id": merge_request_id, "component_id": component_id, "config_id": config_id, "branch_id": branch_id, @@ -874,8 +921,8 @@ def resolve_conflict( """ if (take is None) == (resolved is None): raise ConfigError("Pass exactly one of take=ours|theirs|delete or a resolved body.") - if take is not None and take not in ("ours", "theirs", "delete"): - raise ConfigError(f"Unknown take mode {take!r}: use ours, theirs or delete.") + if take is not None and take not in TAKE_MODES: + raise ConfigError(f"Unknown take mode {take!r}: use {', '.join(TAKE_MODES)}.") project = self._project(alias) client = self._client_factory(project.stack_url, project.token) diff --git a/tests/test_merge_request_service.py b/tests/test_merge_request_service.py index aa9708d9..637b7eef 100644 --- a/tests/test_merge_request_service.py +++ b/tests/test_merge_request_service.py @@ -646,12 +646,18 @@ def _side( class TestGetConfigDiff: def test_classifies_ours_theirs_both(self, store, client_factory) -> None: factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "development", branch_from=123) mock.get_config_diff.return_value = _diff( base=_side({"limit": 100, "timeout": 30, "flag": True}, version=3), ours=_side({"limit": 500, "timeout": 30, "flag": False}, version=4), theirs=_side({"limit": 250, "timeout": 60, "flag": True}, version=7), ) - result = _svc(store, factory).get_config_diff(ALIAS, "keboola.ex-db", "111", 123) + result = _svc(store, factory).get_config_diff(ALIAS, 7, "keboola.ex-db", "111") + # The branch is derived from the MR, never caller-supplied -- the L3 + # diff call must receive branchFromId (123), and the result echoes it. + mock.get_config_diff.assert_called_once_with("keboola.ex-db", "111", 123) + assert result["branch_id"] == 123 + assert result["merge_request_id"] == 7 by_path = {c["path"]: c for c in result["changes"]} assert by_path["configuration.limit"] == { "path": "configuration.limit", @@ -672,24 +678,26 @@ def test_classifies_ours_theirs_both(self, store, client_factory) -> None: def test_identical_change_on_both_sides_is_agreed(self, store, client_factory) -> None: factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "development", branch_from=123) mock.get_config_diff.return_value = _diff( base=_side({"limit": 100}, version=3), ours=_side({"limit": 500}, version=4), theirs=_side({"limit": 500}, version=7), ) - result = _svc(store, factory).get_config_diff(ALIAS, "keboola.ex-db", "111", 123) + result = _svc(store, factory).get_config_diff(ALIAS, 7, "keboola.ex-db", "111") (change,) = result["changes"] assert change["changed_by"] == "both" assert change["agreed"] is True def test_removed_key_shows_none_not_base(self, store, client_factory) -> None: factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "development", branch_from=123) mock.get_config_diff.return_value = _diff( base=_side({"secret": "old"}, version=3), ours=_side({}, version=4), # ours REMOVED the key theirs=_side({"secret": "new"}, version=7), ) - result = _svc(store, factory).get_config_diff(ALIAS, "keboola.ex-db", "111", 123) + result = _svc(store, factory).get_config_diff(ALIAS, 7, "keboola.ex-db", "111") (change,) = result["changes"] assert change["changed_by"] == "both" assert change["agreed"] is False @@ -702,24 +710,26 @@ def test_deleted_side_is_flagged_not_a_path(self, store, client_factory) -> None # Only production" rendering would hide the most consequential # difference. factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "development", branch_from=123) mock.get_config_diff.return_value = _diff( base=_side({"limit": 100}, version=3), ours=_side({"limit": 100}, version=4), theirs=_side({"limit": 100}, version=7, is_deleted=True), ) - result = _svc(store, factory).get_config_diff(ALIAS, "keboola.ex-db", "111", 123) + result = _svc(store, factory).get_config_diff(ALIAS, 7, "keboola.ex-db", "111") assert result["theirs_deleted"] is True assert result["ours_deleted"] is False assert result["changes"] == [] # identical content, no paths def test_null_side_reports_none_deleted_flag(self, store, client_factory) -> None: factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "development", branch_from=123) mock.get_config_diff.return_value = _diff( base=None, ours=None, # never existed on this side theirs=_side({"a": 2}, version=7), ) - result = _svc(store, factory).get_config_diff(ALIAS, "keboola.ex-db", "111", 123) + result = _svc(store, factory).get_config_diff(ALIAS, 7, "keboola.ex-db", "111") assert result["ours_deleted"] is None by_path = {c["path"]: c for c in result["changes"]} assert by_path["configuration"]["changed_by"] == "theirs" @@ -728,22 +738,24 @@ def test_change_description_is_not_content(self, store, client_factory) -> None: # changeDescription is a per-version commit message; two sides always # differ there and it is not something a resolution decides. factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "development", branch_from=123) mock.get_config_diff.return_value = _diff( base=_side({"a": 1}, version=3, changeDescription="base msg"), ours=_side({"a": 1}, version=4, changeDescription="ours msg"), theirs=_side({"a": 1}, version=7, changeDescription="theirs msg"), ) - result = _svc(store, factory).get_config_diff(ALIAS, "keboola.ex-db", "111", 123) + result = _svc(store, factory).get_config_diff(ALIAS, 7, "keboola.ex-db", "111") assert result["changes"] == [] def test_rows_compare_wholesale(self, store, client_factory) -> None: factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "development", branch_from=123) mock.get_config_diff.return_value = _diff( base=_side({}, version=3, rows=[{"id": "r1"}]), ours=_side({}, version=4, rows=[{"id": "r1"}, {"id": "r2"}]), theirs=_side({}, version=7, rows=[{"id": "r1"}]), ) - result = _svc(store, factory).get_config_diff(ALIAS, "keboola.ex-db", "111", 123) + result = _svc(store, factory).get_config_diff(ALIAS, 7, "keboola.ex-db", "111") (change,) = result["changes"] assert change["path"] == "rows" assert change["changed_by"] == "ours" @@ -1072,3 +1084,56 @@ def test_plain_project_gets_the_enable_hint(self, store, client_factory) -> None _svc(store, factory).create_merge_request(ALIAS, 123, "My MR") assert "not enabled" in str(exc_info.value) assert "SOX" not in str(exc_info.value) + + +class TestLayer1FindingsFollowUps: + """Regression tests for tasks/dmd-1899-findings-from-layer1.md.""" + + def test_every_enriched_return_carries_allowed_actions(self, store, client_factory) -> None: + # Finding #2: a --json consumer of create/transitions must be able to + # answer "what can I do next" without a second call. + factory, mock = client_factory + mock.list_dev_branches.return_value = [{"id": 1, "isDefault": True}] + mock.merge_requests.create.return_value = _wire_mr(9, "development") + created = _svc(store, factory).create_merge_request(ALIAS, 123, "My MR") + assert created["allowed_actions"] == [ + "request_review", + "merge", + "update", + "resolve_conflicts", + ] + + mock.merge_requests.request_review.return_value = _wire_mr(9, "approved") + submitted = _svc(store, factory).request_review(ALIAS, 9) + assert "merge" in submitted["allowed_actions"] + + mock.merge_requests.list.return_value = [_wire_mr(9, "published")] + rows = _svc(store, factory).list_merge_requests(ALIAS)["merge_requests"] + assert rows[0]["allowed_actions"] == [] + + def test_empty_list_reports_feature_enabled_flag(self, store, client_factory) -> None: + # Finding #6: 200 + [] on a project without the feature must be + # tellable from a genuinely empty project. + factory, mock = client_factory + mock.merge_requests.list.return_value = [] + mock.has_feature.return_value = False + result = _svc(store, factory).list_merge_requests(ALIAS) + assert result["count"] == 0 + assert result["feature_enabled"] is False + + def test_non_empty_list_does_not_spend_the_feature_call(self, store, client_factory) -> None: + factory, mock = client_factory + mock.merge_requests.list.return_value = [_wire_mr(1)] + result = _svc(store, factory).list_merge_requests(ALIAS) + assert "feature_enabled" not in result + mock.has_feature.assert_not_called() + + def test_diff_on_a_closed_mr_is_refused(self, store, client_factory) -> None: + # Finding #1: the branch comes from the MR; a published/canceled MR + # has none (FK nulled it), so the diff is refused readably. + factory, mock = client_factory + mock.merge_requests.get.return_value = _wire_mr(7, "published", branch_from=None) + with pytest.raises(KeboolaApiError) as exc_info: + _svc(store, factory).get_config_diff(ALIAS, 7, "keboola.ex-db", "111") + assert exc_info.value.error_code == ErrorCode.VALIDATION_ERROR + mock.get_config_diff.assert_not_called()