feat(service): merge-request Layer 2 - lifecycle, derived status, conflict resolution (DMD-1899)

[martinsifra]

Layer 2 of the merge-request stack (DMD-1899): services/merge_request_service.py over the Layer 3 namespace shipped in #556. No Layer 1 commands yet — those are DMD-1900, so none of the convention-#17 doc surfaces or E2E tests apply to this PR; unit tests only (tests/test_merge_request_service.py, 94 tests).

The design record — the L2 RFC plus the wire-truth notes every decision below is written down in — lives on branch ms/merge-requests-rfcs (RFCs deliberately do not merge to main). A full self-review pass (2026-08-27) was applied in the final commit; see "Review fixes" below.

What's in

• Status-derivation polyfill (pure module-level functions): derived_state (the UI list badge's decision table incl. the rejected / closed-by-creator reviewer overrides), merge_blockers + mergeable (a list, so concurrent blockers don't mask each other), allowed_actions (state-machine table), viewer (is_creator/has_approved, anchored on verify_token's new admin_id). Every function reads the future server-serialized field first (DMD-1988) and falls back to the local table — the fallbacks get deleted when Connection serializes.
• Reads: list_merge_requests (client-side state filter over the closed derived + raw vocabulary, typos refused), find_merge_request_for_branch (the resolver behind L1's optional --mr-id; a branch has at most one MR ever), get_merge_request (detail with full derived status; conflicts fetched only for open MRs).
• Writes: create_merge_request (targets the default branch itself; refuses the default as source with a readable error instead of the backend's 404), update_merge_request, request_review / approve / request_changes — all behind the branches-merge-requests pre-flight raising FEATURE_NOT_ENABLED (a missing feature is otherwise a 403 byte-identical to a role denial; SOX-fence assumption in the comment).
• merge: 409 remapped onto its two wire shapes — MR_NOT_READY_TO_MERGE (retryable; carries storage.mergeRequests.notReadyToMerge) vs MR_MERGE_CONFLICT (not retryable, names merge-request conflicts as the next step); backend counterpart asking for codes on every MR error is DMD-1984. Post-merge cleanup mirrors BranchService.delete_branch (conditional active_branch_id reset, sync-mapping unlink, best-effort with warnings); output says the source branch "is being deleted", never that it's gone.
• Conflict resolution: list_conflicts, get_config_diff flattened to a per-path changed_by: ours|theirs|both classification (+ agreed: true when both sides made the identical change; ours_deleted/theirs_deleted flags for tombstoned sides), and resolve_conflict where every mode goes through the rebase endpoint: take=ours|theirs composes the full replace body from the side's diff envelope with version=theirs.version, a deleted side collapses to the delete resolution (both directions), a custom body must spell out name/rows/configuration (rebase REPLACES — a defaulted key would be silent data loss). The branch is derived from the MR itself, never caller-supplied. The UI's reset-to-default alternative for take=theirs is tracked as DMD-1987.
• Infra: http_base now surfaces a Keboola user error's machine string code as KeboolaApiError.details["api_error_code"] (additive, all four raise sites); TokenVerifyResponse gains admin_id/admin_name; FEATURE_BRANCHES_MERGE_REQUESTS renamed to BRANCHES_MERGE_REQUESTS_FEATURE; the isDefault scan hoisted to services.base.find_default_branch_id and the config/sync/workspace copies migrated; json_utils.compute_diff split into structured compute_diff_entries + a byte-identical formatter.

Review fixes (final commit, tasks/pr-703-review.md)

The self-review found one critical wire-shape error: the diff sides were assumed flat, but the verified shape (connection ConfigurationVersionResponse + ConfigurationDiffData) nests all content under a diff envelope with version/isDeleted as side metadata — the original take/classify code would have been dead on arrival against the live API, invisibly, because the test fixtures encoded the same wrong assumption. Now recorded in docs/merge-requests-notes.md's wire-truth table. Also fixed: resolve_conflict could rebase into an unrelated branch (branch now derived from the MR), deleted-side asymmetry, _same_id on branch ids, FEATURE_NOT_ENABLED, spec'd mocks at the L3 seam, and regression tests for every finding.

Second review: wire truth vs. Connection (2026-08-27)

An adversarial Opus review verified every wire assumption against the Connection source (report: tasks/pr-703-opus-wire-review.md): 7 CONFIRMED, 3 MISMATCH, all fixed in the final commit:

• The conflict 409 is not code-less — ExceptionConverter serializes storage.mergeRequests.validation top-level plus the conflicting configs in params.errors. The remap now matches both codes explicitly (unknown 409 codes pass through unmapped), and http_base surfaces params as details.api_error_params — the conflict list travels with the error.
• approve exists only in in_review (from approved the backend answers 422 — the UI button offering it there is wrong); allowed_actions corrected, update added to in_merge. With the non-SOX default of 0 required approvals, approve is 422 everywhere and in_review is unreachable.
• derive_state's rejected/self-closed rows are best-effort by wire design: reviewers[].status needs a review_requested anchor that skip_review never writes, and explicit reviewers shadow non-reviewer decisions — so on a default non-SOX project those states are underivable from reviewers[]. The UI badge has the identical blind spot (our table is its port). Documented, not re-derived client-side — the reliable fix is server-side and is now recorded on DMD-1988 (derive from the activity log).

Layer 1 RFC findings applied (2026-08-28, commit 07daa50)

Writing the Layer 1 command RFC (DMD-1900, PR #708) surfaced seven Layer 2 findings (tasks/dmd-1899-findings-from-layer1.md); all verified against Connection and applied:

• get_config_diff derives the branch from the MR (signature now alias, merge_request_id, component_id, config_id) — the same make-the-wrong-call-unrepresentable reasoning resolve_conflict already had; the resolved branch_id is echoed, a published/canceled MR is refused readably. The old tests passed shifted positional args straight into MagicMock — rewritten to pin the wiring.
• Every enriched return carries allowed_actions (list rows, find, create, update, transitions, merge's post-merge state) — a --json consumer answers "what can I do next" without a second call.
• Safety fact recorded: autoMergeStrategy=immediately makes a background backend tick merge any approved MR through the same MergeProcessor under a system token (AutoMergeCandidateRepository.php:38-47, AutoMergeTickHandler.php:86) — create + request-review can end in a production merge with merge() never called. New notes section + both docstrings.
• Docs: full MergeRequestResponse wire shape in the notes table (merge{} is nested, createdAt top-level, autoMerge* are response fields); MergeRequestVoter cited (scoped token → 403 on detail/conflicts, a different axis than role whitelisting) + 403 added to the two read rows in the layer3 doc.
• Empty list now carries feature_enabled — 200 + [] on a project without the feature stops being indistinguishable from a genuinely empty project (the extra verify_token GET is spent only on the empty case).
• STATE_FILTER_VOCABULARY and TAKE_MODES are public so Layer 1 enumerates them in help text and pre-validates to exit 2 (precedent: notification_service.KNOWN_EVENTS).

Known CI caveats

• Rebased onto current main (0.92.0) — make changelog-check is green again. No version bump here: this lands in the DMD-1899/1900 stack and the bump PR owns the changelog entry (the feat(token): add token list, stop retrying non-idempotent writes (#599) #616 precedent). Note for the release PR: the entry must also cover details.api_error_params (+ api_error_params_truncated) — a cross-cutting, bounded addition to the --json error envelope on every 4xx/5xx from all clients — not just the merge-request command group.
• tests/test_changelog_render.py fails (2 tests) whenever FORCE_COLOR is set in the environment (Warp exports FORCE_COLOR=3): Rich then emits ANSI inside the asserted substrings (New: renders bold, splitting "New: alpha thing."). Pre-existing main-branch test fragility, fully reproducible and unrelated to this PR; worth a tiny follow-up fix on main.

Review pointers

• The derivation tables are 1:1 with the RFC section "Derived status" and DMD-1988 — that's the contract; if a table looks wrong here, it's wrong there first.
• resolve_conflict's conflict-set check, the MR-derived branch, and onto_version = theirs.version are the service-level guards Layer 3 deliberately does not have.

🤖 Generated with Claude Code

[linear-code]

DMD-1899

[Copilot]

Pull request overview

Implements Layer 2 (services) support for Keboola “merge requests” (Branches 2.0), including client-side derived-status polyfills, merge lifecycle orchestration, and conflict resolution built on the existing Layer 3 client namespace.

Changes:

• Adds MergeRequestService with list/get/create/update/review transitions, merge error remapping, and conflict diff/resolve workflows.
• Introduces derived-status polyfill helpers (derived_state, merge_blockers, allowed_actions, viewer) with “server-first, fallback” behavior.
• Extends shared infrastructure: structured diff entries, richer API error details (code/params), verify-token viewer anchoring (admin_id), and a shared default-branch resolver.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/test_merge_request_service.py	New unit tests covering MR service behavior, derived status, merge 409 remapping, and conflict resolution flows.
tests/test_json_utils.py	Adds coverage for new compute_diff_entries() structured diff walker and its formatting parity with compute_diff().
tests/test_http_base.py	Adds tests ensuring API error body code and params are surfaced into KeboolaApiError.details.
src/keboola_agent_cli/services/workspace_service.py	Switches default-branch detection to shared find_default_branch_id().
src/keboola_agent_cli/services/sync_service.py	Uses shared find_default_branch_id() for sync init default branch resolution.
src/keboola_agent_cli/services/merge_request_service.py	New Layer 2 service: lifecycle ops, derived status, merge cleanup, conflict diff/resolve.
src/keboola_agent_cli/services/config_service.py	Replaces duplicated default-branch scan with find_default_branch_id().
src/keboola_agent_cli/services/base.py	Adds shared helper find_default_branch_id().
src/keboola_agent_cli/models.py	Extends TokenVerifyResponse with admin_id/admin_name used for viewer-relative derivation.
src/keboola_agent_cli/json_utils.py	Adds DiffEntry + compute_diff_entries(); refactors compute_diff() into a formatter over entries.
src/keboola_agent_cli/http_base.py	Surfaces API error code/params into KeboolaApiError.details for higher-layer branching.
src/keboola_agent_cli/errors.py	Adds FEATURE_NOT_ENABLED, MR-specific error codes, and FeatureNotEnabledError.
src/keboola_agent_cli/constants.py	Renames merge-request feature flag constant to BRANCHES_MERGE_REQUESTS_FEATURE.
src/keboola_agent_cli/client/tokens.py	Parses verify-token admin block into TokenVerifyResponse.admin_id/admin_name.
src/keboola_agent_cli/client/merge_requests.py	Updates docs/comments to new constant name and clarifies merge 409 conflict shape.
docs/merge-requests-notes.md	Adds verified backend “wire truth” notes, including 409 shapes and diff/rebase envelopes.
docs/merge-requests-layer3.md	Documents shipped Layer 3 surface and contracts; updated with verified conflict 409 code/params.
docs/merge-requests-layer2.md	Design/decision record for Layer 2 service behavior, derivations, preflight, merge cleanup, and conflict resolution.
docs/merge-requests-layer1.md	Working notes for future CLI commands and UX wording (Layer 1).
docs/error-codes.md	Documents newly introduced error codes for feature gating and MR merge conflict/not-ready cases.

---

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

[martinsifra]

Valid catch — the skip predicate was looser than derive_viewer's server-field predicate, so a bare viewer: {} (or one with foreign keys) would skip verify_token and yield {None, None} instead of the local derivation. Fixed in cbdf1d1: both sites now share a single _server_viewer() predicate (dict AND has isCreator/hasApproved), so they cannot disagree; regression test added (test_empty_server_viewer_falls_back_to_local_derivation).

[zajca]

Changes requested. See the published review findings.

[zajca]

Selected findings from the automated read-only review.

[zajca]

Reviewed by Opus.

The guard exists precisely because rebase REPLACES rather than patches ("a missing key would silently wipe data, so it is refused instead of defaulted"), and the code already recognises that a present-but-null value defeats a bare presence check -- that is exactly why the name special case on the next line was added.

configuration has the same hole and a worse outcome. missing only tests key not in body, so a body carrying "configuration": None passes, and rebase_config(..., configuration=body["configuration"]) posts {"configuration": null}. Per this PR's own wire-truth table (docs/merge-requests-notes.md: RebaseRequest::mapValidatedData uses ?? new stdClass()), PHP's null-coalescing operator fires on null as well as on an absent key, so the backend substitutes {} -- the configuration body is replaced with an empty object and the rebase returns 200. No error, no warning.

Failure scenario: a caller invokes resolve_conflict(alias, mr_id, component_id, config_id, resolved={"name": "My config", "rows": [], "configuration": None}). missing is empty (all three keys present), the non-empty check applies only to name, so rebase_config POSTs {"version": N, "diff": {"name": "My config", "rows": [], "configuration": null}}. The backend's ?? new stdClass() fires on the null and REPLACES the dev-branch configuration body with {}. The call returns 200 with resolution: "custom" -- the entire configuration payload is destroyed with no error surfaced.

Reachable two ways:

• The caller-authored path is fully under caller control. resolve_conflict(resolved={...}) is the public service entry point that Layer 1 (DMD-1900) will feed from a user-supplied JSON/YAML resolution file; a file with a null configuration is refused nowhere.
• The take=ours|theirs path composes body from the server's diff envelope. The comment above acknowledges that envelope declares name nullable despite being required; if configuration is nullable on the same schema, a null there is indistinguishable from a legitimate one here.

Suggested fix: extend the non-null check beyond name -- treat a None value for configuration (and rows, whose []-deletes-all-rows semantics make a null equally ambiguous) as missing, so both take the existing refusal path rather than reaching rebase_config.

The existing regression test test_null_name_on_a_take_side_is_a_contract_violation covers only name; no test pins the null-configuration case.

[martinsifra]

Confirmed and fixed in 1e239b4 — the guard now refuses present-but-null for every required key (key not in body or body[key] is None), so a configuration: null (which PHP's ?? new stdClass() would turn into a silent full-body wipe) and a null rows both take the existing refusal path. A genuine rows: [] (delete-all) stays legal — pinned by test_empty_rows_list_stays_a_legal_resolution; new regressions cover null configuration on the resolved path and null rows on a take side.

[zajca]

Reviewed by Opus.

This PR added feature_enabled to list_merge_requests for exactly this ambiguity, with the rationale spelled out in the comment at line 330: "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."

find_merge_request_for_branch reads the same ungated client.merge_requests.list() and did not get the same treatment. On a project without branches-merge-requests the loop sees an empty list and raises NOT_FOUND whose message asserts a fact it cannot know ("Branch N has no merge request") and prescribes a next step that is guaranteed to fail -- create_merge_request runs _require_merge_requests_feature first and refuses with FEATURE_NOT_ENABLED.

Failure scenario: on a project where branches-merge-requests is not enabled, find_merge_request_for_branch(alias, 123) gets 200 + [] from the ungated list endpoint, matches nothing, and raises NOT_FOUND with "Branch 123 has no merge request in project 'prod'. Create one with kbagent merge-request create." The user runs that command and gets FEATURE_NOT_ENABLED ("Merge requests are not enabled on this project") -- the first error's diagnosis and its prescribed next step were both wrong, and the actual cause was knowable from the already-loaded features cache.

This matters more than for list: the docstring identifies this method as "the resolver behind L1's optional --mr-id", so it is on the path of every merge-request command invoked without an explicit id. The user is sent in a loop -- told to create an MR, then told merge requests are not enabled.

Suggested fix: mirror the list_merge_requests treatment -- on the no-match path only (so the cost is not paid on the happy path), call client.has_feature(BRANCHES_MERGE_REQUESTS_FEATURE) before raising, and either raise FeatureNotEnabledError or qualify the message when the feature is absent. The client.close() in the finally currently runs before the raise, so the check needs to move inside the try.

[martinsifra]

Confirmed and fixed in 1e239b4 — find_merge_request_for_branch now runs _require_merge_requests_feature on the no-match path (inside the try, so the client is still open), mirroring the list_merge_requests treatment: a featureless project gets FEATURE_NOT_ENABLED (with the SOX/enable message split) instead of a NOT_FOUND prescribing a create that cannot succeed. The check is spent only when nothing matched — test_find_happy_path_does_not_spend_the_feature_call pins that the happy path never pays the extra GET.

[zajca]

Reviewed by Opus.

api_error_params copies the response body's params object verbatim into KeboolaApiError.details, with no size bound, at all four raise sites (401/403/404 and the generic path) -- so it now rides along on every 4xx/5xx from every BaseHttpClient subclass (KeboolaClient, AiServiceClient, DataScienceClient, AuthClient via its _raise_api_error override), not just the merge 409 this was added for. Numerous command-layer sites re-propagate it with details=exc.details (commands/data_app.py, commands/_data_app_runtime.py, commands/_data_app_git.py, commands/_semantic_layer_helpers.py), and output.py emits details into the --json error envelope whenever non-empty.

The asymmetry is the concern: api_message twenty lines below is deliberately capped at MAX_API_ERROR_LENGTH, and the comment at line 495 establishes bounding server-supplied text as the rule for this function. params gets no equivalent. The merge-conflict case is the motivating example and also the worst one -- params.errors carries one entry per conflicting configuration, so a branch with hundreds of conflicts produces a proportionally large blob. In a CLI whose --json output is consumed by agents, that lands unbounded in a context window.

Failure scenario: a merge over a branch with several hundred conflicting configurations answers 409 with params.errors listing every one. _raise_api_error copies the whole object into details["api_error_params"], MergeRequestService._remap_merge_conflict forwards details=exc.details into the MR_MERGE_CONFLICT error, and the --json error envelope emits the entire list verbatim -- while the human error message beside it was truncated to MAX_API_ERROR_LENGTH.

Suggested fix: bound what is copied (cap the serialized size, or cap list lengths inside params) so the details payload has a ceiling like the message does. Alternatively, narrow the passthrough to the specific keys a caller is documented to branch on rather than copying the whole object.

[martinsifra]

Confirmed and fixed in 1e239b4 — api_error_params is now bounded like the message beside it: over MAX_API_ERROR_PARAMS_LENGTH (8192 serialized) every top-level list is truncated to MAX_API_ERROR_PARAMS_LIST_ITEMS (20) entries; if still over, params is dropped entirely. Either way details.api_error_params_truncated: true marks it, and an unserializable payload is dropped rather than crashing the error path. Three regression tests (small passes verbatim / oversized list truncated with the kept prefix verbatim / untruncatable dropped with marker).

[zajca]

Reviewed by Sonnet.

The Layer 1 RFC says the state-filter vocabulary "lives in the service (_STATE_FILTER_VOCABULARY)", implying an underscore-prefixed private name. The constant this same PR actually defines in src/keboola_agent_cli/services/merge_request_service.py:80 is the public STATE_FILTER_VOCABULARY (no leading underscore) — its own comment says "Public: Layer 1 enumerates it in --state help text". A future implementer of DMD-1900 following this doc verbatim would try to import a name that doesn't exist. Low impact since it's a design doc for not-yet-built Layer 1, but worth a one-word fix to avoid confusion later.

[martinsifra]

Already resolved — the Layer 1 RFC rewrite (980b28d, now pushed as part of this branch) references the public STATE_FILTER_VOCABULARY and TAKE_MODES names (docs/merge-requests-layer1.md:86: "importing the now-public STATE_FILTER_VOCABULARY and TAKE_MODES from the service"); the underscore-prefixed mention this comment anchored to no longer exists in the current revision.

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.

[martinsifra]

Valid — and independently raised as the blocking finding of the human review a day later; fixed in 01fb0fa. The guard now requires all five replaced-body keys: name/rows/configuration/isDisabled refuse absent-or-null (a null isDisabled would still reach bool() as False — the same silent re-enable), description refuses absence only — an explicit description: null is a legitimate clear-the-description decision, exactly as this comment suggests. is_disabled forwards verbatim via body["isDisabled"]. Pinned by test_missing_is_disabled_is_refused_not_defaulted and test_missing_description_is_refused_but_explicit_null_is_legal.

[zajca]

Reviewed the full diff on the branch, ran the complete test suite and every quality gate locally.

Gate status (on the PR branch): pytest tests/ 6527 passed / 183 skipped, ruff check + ruff format --check clean, ty check clean on the new modules, make loc-check / check-sentinel-guards / changelog-check all OK. I also reproduced the FORCE_COLOR caveat — FORCE_COLOR=3 pytest tests/test_changelog_render.py does fail 2 tests, pre-existing on main, unrelated to this PR.

The design work here is genuinely good — see "What works well" at the bottom. One finding blocks.

---

BLOCKING

1. resolve_conflict guards 3 of the 5 replace keys — isDisabled / description silently lose data

src/keboola_agent_cli/services/merge_request_service.py:1023-1024

is_disabled=bool(body.get("isDisabled", False)),
description=body.get("description"),

Layer 3 warns about exactly this in rebase_config's own docstring (client/configs.py):

is_disabled: Required: on a missing key the backend substitutes false, re-enabling a config that was disabled.
description: … It has no default precisely because that default would silently drop an existing description.

Layer 2 reintroduces both defaults. The guard at :987-991 covers only name / rows / configuration, and its stated reasoning — "rebase REPLACES; a missing key would silently wipe data" — applies verbatim to the other two:

• a custom resolved body without isDisabled silently re-enables a disabled configuration in the dev branch, and the merge then pushes that into production;
• a custom body without description wipes the description;
• the take= path has the same hole whenever the envelope omits a key — and the guard on the other three exists precisely because the envelope is not trusted.

tests/test_merge_request_service.py:892 (test_custom_body_rebases_verbatim) pins the permissive behaviour as expected: a body of {name, rows, configuration} passes and sends is_disabled=False.

Suggested fix — check for presence, not non-None (description: None is a legitimate resolved value, and the _side fixture sends exactly that):

missing = [k for k in ("name", "rows", "configuration") if k not in body or body[k] is None]
missing += [k for k in ("isDisabled", "description") if k not in body]

plus is_disabled=bool(body["isDisabled"]).

---

NON-BLOCKING

2. allowed_actions points at a write that cannot succeed

_enrich_row (merge_request_service.py:244) is state-only. On a project without the feature, list_merge_requests returns feature_enabled: false and rows carrying allowed_actions: ["request_review", "merge", "update", "resolve_conflicts"]. A --json/agent consumer — the stated reason the field exists — is steered straight into the guaranteed FEATURE_NOT_ENABLED loop that find_merge_request_for_branch:371 deliberately closes. The features cache is already warm at that point on the empty-list path, so correcting it is free.

3. api_error_params is a cross-cutting output-contract change with no changelog line

http_base.py:485-497 adds up to 8 KB of server-supplied params to details on every 4xx/5xx from all seven clients, and existing commands already forward details=exc.details into --json (commands/data_app.py, commands/job.py:442, _semantic_layer_helpers.py, …). Two consequences:

• consumers treating details as a presence flag (it was empty on 401/403/404 until now) start seeing content;
• params reaches the output with no masking at all, whereas the message at least goes through truncation. Low risk, but it deserves to be a stated decision rather than a side effect.

Deferring the changelog to the bump PR follows the #616 precedent, but that entry needs to mention this too — not just "merge-request Layer 2".

4. _classify_three_way: a null side with a non-empty base emits contradictory signals

merge_request_service.py:846-852 — when one side is null while base exists, every base key comes out as "that side removed it" (changed_by: "theirs", theirs: None), while theirs_deleted reports None, meaning "the side does not exist at all". A Layer 1 renderer will make nonsense of that pair. Either skip changes for a null side or flag it separately.

5. change_description is silently dropped for take="delete"

merge_request_service.py:973-975 — rebase_config_delete has no such parameter. Layer 1 will expose --change-description, the user will pass it, and nothing reports that it was ignored. A warning in the result (or plumbing it through L3) would close it.

---

NIT

• Comments narrating history/process, against the repo's own "Code Comments — never narrate the change" rule: constants.py:539 ("Renamed from FEATURE_BRANCHES_MERGE_REQUESTS"), services/base.py:124 ("previously copy-pasted"), merge_request_service.py:47,113 ("Opus wire review 2026-08-27"), :371,978 ("Zajca's PR review"). "Verified against MergeRequestLifecycleStateMachine" is a fact about the code and belongs there; dated review references and reviewer names belong in the PR description.
• services/search_service.py:205 still uses the raw string "FEATURE_NOT_ENABLED". Now that the enum exists, this is a one-line cleanup in the same spirit as the find_default_branch_id hoist.
• merge() — the except Exception wraps set_project_branch too, so a failed config write also skips the mapping cleanup. Two separate try blocks would keep the two cleanups independent.
• merge() :660 — int(raw_branch_from) raises ValueError on a non-numeric wire string, i.e. an unhandled exception instead of a KeboolaApiError. _same_id exists precisely because the payload mixes id types.
• find_default_branch_id — int(branch["id"]) raises KeyError for an isDefault: true entry with no id; the rest of the function is defensive (branch.get("isDefault")).
• cleanup_branch_id_from_mapping(branch_from_id) matches on the numeric id alone with no project scoping — a sync workspace of a different project in the CWD with the same branch id gets unlinked. Inherited from BranchService.delete_branch, so not a regression, but merge extends it to another path.

---

What works well

• The tests are above average: a regression test per review finding, MagicMock(spec=KeboolaClient) + spec=MergeRequests at the L3 seam (a renamed client method fails the tests instead of leaving them falsely green), and the _DEFAULT_SIDE sentinel distinguishing "keep the fixture default" from "the side does not exist".
• _server_viewer as the single predicate for "did DMD-1988 land" — both derive_viewer and the verify_token skip read it, so they cannot drift apart. Right call.
• resolve_conflict / get_config_diff deriving the branch from the MR instead of taking it as a parameter is honest make-the-wrong-call-unrepresentable design.
• The 409 remap letting an unknown code pass through unmapped rather than guessing "conflict" is exactly right.
• The find_default_branch_id hoist and the compute_diff → compute_diff_entries split are clean refactors with tests; I verified the formatted output stays byte-identical.
• The layer1/2/3 + notes doc set carrying the wire-truth table is what made the nested-diff-envelope bug visible before production.

Verdict: NEEDS-WORK — finding #1 (silent isDisabled / description loss on rebase). Everything else is non-blocking.

[martinsifra]

Thanks — applied in 01fb0fa. Per finding:

BLOCKING #1 (isDisabled/description) — confirmed and fixed. All five replaced-body keys are now required; one refinement over the suggested fix: isDisabled refuses absent-or-null (a null would still reach bool() as False — the exact silent re-enable), while description refuses only absence (an explicit null is a decision on a genuinely nullable field, an omission is not). is_disabled forwards verbatim. Regression tests: missing isDisabled, missing vs. explicit-null description, take-side isDisabled: true forwarded.

#2 (allowed_actions on a featureless project) — respectfully declined, with the reasoning now documented on _enrich_row. The stated scenario cannot occur: feature_enabled: false is emitted only on the empty-list path, where there are no rows to carry allowed_actions. The real residual is a project that lost the feature after MRs existed — closing that costs a verify_token GET on every non-empty list (the cache is warm only on the empty path), for a rare configuration where the pre-flight already answers any attempted write with the precise FEATURE_NOT_ENABLED. The proper fix is DMD-1988's server-side allowedActions, which can honor features.

#3 (api_error_params output contract) — agreed. The unmasked pass-through is now a stated decision in the code (server-authored error context, same trust as the message text), and the PR body's bump-PR note now says the release changelog entry must cover details.api_error_params (+_truncated) as a cross-cutting output-contract addition, not just "merge-request Layer 2".

#4 (null side vs. base) — confirmed and fixed: _classify_three_way emits no per-path rows when either side is null; the side-level fact stays on ours_deleted/theirs_deleted. Docstring + test updated.

#5 (change_description on take=delete) — confirmed; fixed with a warnings[] entry in the result. Plumbing through L3 is not possible: changeDescription lives inside the diff envelope and the delete tombstone is exactly {"version": N, "diff": {}} — the wire has nowhere to carry it (the server records its default message). Layer 1 will additionally reject the combination at exit 2 per its RFC.

Nits — all applied except the last: history-narrating comments removed (facts like "verified against MergeRequestLifecycleStateMachine" stayed, names/dates went); search_service now uses ErrorCode.FEATURE_NOT_ENABLED; merge()'s two cleanups run in independent try blocks (pinned by a test that a failed config write no longer skips the mapping unlink); wire branch ids coerce via a _coerce_branch_id helper (non-numeric degrades instead of raising); find_default_branch_id tolerates an isDefault entry without an id. The unscoped cleanup_branch_id_from_mapping is real but inherited from BranchService.delete_branch — fixing it means fixing both call sites together, so it's noted as a follow-up rather than diverging the two here.

Full triage record: tasks/pr-703-zajca-review-2.md (local). Gates on the branch: 107 service tests, ruff/format/ty clean.