diff --git a/docs/error-codes.md b/docs/error-codes.md index a3aba6e7..a3fccf37 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 @@ -185,3 +186,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 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-layer1.md b/docs/merge-requests-layer1.md new file mode 100644 index 00000000..78191099 --- /dev/null +++ b/docs/merge-requests-layer1.md @@ -0,0 +1,342 @@ +# 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. diff --git a/docs/merge-requests-layer2.md b/docs/merge-requests-layer2.md new file mode 100644 index 00000000..9f287ffa --- /dev/null +++ b/docs/merge-requests-layer2.md @@ -0,0 +1,254 @@ +# 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 | + +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 +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. 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`). +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(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: + +- **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` 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). + +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` — 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). + +## 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..d50f7f8d --- /dev/null +++ b/docs/merge-requests-layer3.md @@ -0,0 +1,158 @@ +# 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` (`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) + +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, 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, 403 (scoped token -- same voter) | + +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 + `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 + 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~~ -- 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 + 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..40ac7e5a --- /dev/null +++ b/docs/merge-requests-notes.md @@ -0,0 +1,246 @@ +# 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>" 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`, +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` +`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. + +## 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, +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. + +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`, +`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: "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 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` | +| `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"). diff --git a/src/keboola_agent_cli/client/merge_requests.py b/src/keboola_agent_cli/client/merge_requests.py index 4cf0fb02..dad159c3 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. @@ -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/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/constants.py b/src/keboola_agent_cli/constants.py index ffc809af..b2df5922 100644 --- a/src/keboola_agent_cli/constants.py +++ b/src/keboola_agent_cli/constants.py @@ -482,12 +482,25 @@ 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" + +# 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`` diff --git a/src/keboola_agent_cli/errors.py b/src/keboola_agent_cli/errors.py index 6e3d4993..f955260c 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" @@ -142,6 +147,15 @@ 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): 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" + def mask_token(token: str) -> str: """Mask a Keboola Storage API token for safe display. @@ -202,6 +216,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 @@ -331,9 +362,12 @@ 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", + 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..e4075362 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,22 @@ 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 + # 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` @@ -454,6 +471,7 @@ def _raise_api_error( status_code=status, error_code=ErrorCode.INVALID_TOKEN, retryable=False, + details=details, ) if status == 403: @@ -462,6 +480,7 @@ def _raise_api_error( status_code=status, error_code=ErrorCode.ACCESS_DENIED, retryable=False, + details=details, ) if status == 404: @@ -470,6 +489,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 +509,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/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/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/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 new file mode 100644 index 00000000..96fa7cec --- /dev/null +++ b/src/keboola_agent_cli/services/merge_request_service.py @@ -0,0 +1,1064 @@ +"""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 + +from ..client import KeboolaClient +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 +from .base import BaseService, find_default_branch_id + +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, 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": ("request_changes", "merge", "update", "resolve_conflicts"), + "in_merge": ("update",), + "published": (), + "canceled": (), +} + + +# Everything `list_merge_requests`' --state filter accepts: the derived +# vocabulary plus the raw lifecycle states (the two overlap on purpose). +# 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 + 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). + + 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: + 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 _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. + + ``admin_id`` is the caller's user id from ``verify_token`` (the response's + ``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) + if server is not None: + 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} + + +def _enrich_row(mr: dict[str, Any]) -> dict[str, Any]: + """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): + """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 + # 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). Ask Keboola support to enable the feature." + ) + + # -- 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. 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: + 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() + + 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 feature_enabled is not None: + result["feature_enabled"] = feature_enabled + 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 _same_id((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) + # The verify_token call exists only to anchor the viewer + # 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 + # False. + admin_id: int | None = None + if _server_viewer(mr) is None: + 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 + + # -- 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. + + ``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) + 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). + + ``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: + 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).""" + 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, + error_code=ErrorCode.API_ERROR, + retryable=False, + ) + + # -- 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. + + 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). 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. + - ``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 + 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. 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: + 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) + result["allowed_actions"] = derive_allowed_actions(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 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 + 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 " + "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 + 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 -------------------------------------------- + + # 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]: + """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, 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 + 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) + 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() + theirs = diff.get("theirs") or {} + 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, + "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, + } + + 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]: + # 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")))} + theirs_entries = { + 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) + 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 + 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( + self, + alias: str, + merge_request_id: int, + component_id: str, + config_id: str, + 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). + + 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. + - ``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. + 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 + 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 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) + 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 {} + 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, + ) + + # 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": + 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: + body, resolution = side.get("diff") or {}, take + + if body is None: + configuration = client.rebase_config_delete( + 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 + # 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)}." + ) + configuration = client.rebase_config( + component_id, + config_id, + branch_id, + version=onto_version, + 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: + 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 _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 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: + 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: + """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/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_http_base.py b/tests/test_http_base.py index 7cf97bff..0abbe961 100644 --- a/tests/test_http_base.py +++ b/tests/test_http_base.py @@ -1056,3 +1056,90 @@ 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() + + 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_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] diff --git a/tests/test_merge_request_service.py b/tests/test_merge_request_service.py new file mode 100644 index 00000000..637b7eef --- /dev/null +++ b/tests/test_merge_request_service.py @@ -0,0 +1,1139 @@ +"""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 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, + FeatureNotEnabledError, + 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, + 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_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" 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_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: + 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} + + +# -- Service-level tests ------------------------------------------------------ + +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]: + # 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) + 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} + + +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() + + +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() + + +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, + 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, + "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, + }, + } + + +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.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, 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", + "changed_by": "both", + "agreed": False, + "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 "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.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, 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, 7, "keboola.ex-db", "111") + (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_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.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, 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, 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" + + 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, 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, 7, "keboola.ex-db", "111") + (change,) = result["changes"] + assert change["path"] == "rows" + assert change["changed_by"] == "ours" + + +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: 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 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} + + 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", 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", 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", 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, is_deleted=True)) + result = _svc(store, factory).resolve_conflict( + 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", 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", 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") + with pytest.raises(ConfigError): + svc.resolve_conflict(ALIAS, 7, "c", "1", take="ours", resolved={}) + with pytest.raises(ConfigError): + 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 + self._arm(mock) + with pytest.raises(KeboolaApiError) as exc_info: + _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() + + +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] + + +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} + + +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() + + 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} + + 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) + + +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()