feat(cli): merge-request command group -- Layer 1 over MergeRequestService (DMD-1900)

[martinsifra]

The kbagent merge-request command group (Layer 1, DMD-1900) over MergeRequestService
(Layer 2, #703), plus its serve REST router. Implements
docs/merge-requests-layer1.md. Supersedes #735, which was merged into the L2 branch and then undone when L2 was rebased back to a pure Layer 2 (the L2 branch has since been squash-merged to main as 5281eef)..

Base

Layer 2 (#703) is in main; this PR is rebased onto that squash (5281eef) and targets main
directly. The four docs/merge-requests-*.md files ride along -- they lived on
ms/merge-requests-rfcs and main does not have them yet.

The Layer 2 documents in this PR: what describes #703, what this PR changes

docs/merge-requests-layer2.md and -layer2-followups.md land here because main never got
them (they lived on ms/merge-requests-rfcs). To keep "the CLI layer" review honest about the
service it also patches:

• Describes feat(service): merge-request Layer 2 - lifecycle, derived status, conflict resolution (DMD-1899) #703 as merged (no code change here): everything in layer2.md up to and
  including Rebase / conflict resolution semantics, minus the two items below; all of
  merge-requests-notes.md and -layer3.md.
• Changed by this PR (service code + doc, same commit set): the Additions made for Layer 1
  section of layer2.md (get_merge_request_row, resolution_candidate + diff warnings,
  merge() cleanup_warnings → warnings, feature_enabled on detail, cleanup_skipped +
  branch_from_id_raw on merge) and the warnings[] bullet under Post-merge cleanup; in
  the Merge: 409 handling section the rationale paragraph was corrected (both 409 shapes carry
  a string code -- the mapping lives in the service because the codes are endpoint-specific,
  not because one shape is code-less). -layer2-followups.md is the review-leftover list from
  feat(service): merge-request Layer 2 - lifecycle, derived status, conflict resolution (DMD-1899) #703's approval; each item's State line says whether this PR took it.
• The service diff itself (services/merge_request_service.py, +206/−26) is exactly the second
  bullet plus the field-cap validation and the row tier's lazy feature pre-flight from Zajca's
  review.

What ships

Eleven commands (hidden alias mr), one per service method, plus a permission-enforced
REST mirror:

merge-request list | detail | create | update | request-review | approve | request-changes
              | merge | conflicts | diff | resolve

Three small Layer 2 additions ride along (decided while walking #703's review findings into
the RFC): get_merge_request_row (the row tier by id — one GET, never the three-call
detail), get_config_diff → resolution_candidate (the ours envelope through the same
constant as the five-key replace guard, so diff --output can never write a file resolve
refuses), and merge() cleanup_warnings → warnings (one soft-failure key for the group).

The decisions a reviewer should push on

1. Destructive is a property of the command — never of a flag or of the MR's state.
Anything that moves a merge request toward or into production is destructive, always:
merge, request-review (on the non-SOX default of 0 approvals it lands the MR directly in
approved), approve (the last approval is what a merge waits for), resolve (removes the
blocker a merge waits on) and auto-merge. create, update, request-changes are writes.
So OPERATION_REGISTRY carries the class per command, FLAG_ESCALATIONS is back to its
single original entry, a policy is evaluated from the command name alone before any network
call, and --deny-destructive yields an agent that can observe and shape merge requests
but never move one.

This replaces the first draft's flag/state-derived escalations, which Zajca's review pointed at
twice (a permission depending on a GET; an exit 2 depending on state nobody typed). Deliberately
over-broad in one place: on a 2-approval project request-review only reaches in_review, yet
it is destructive here — the required-approvals count is unreadable with a Storage token
(DMD-1969), and a class that flips on a number the CLI cannot read would be the state condition
again by another name.

2. Auto-merge is its own command. merge-request auto-merge --strategy immediately|scheduled|none [--at TS] — destructive, prompts in human mode when arming. A backend
scheduler runs every approved MR whose strategy is set through the same MergeProcessor,
on its own, retrying every tick (AutoMergeCandidateRepository.php:44-47,
AutoMergeTickHandler.php:86): a delayed production merge with merge never called. Arming is
therefore a consciously separate step, not a flag on create/update (which no longer take
one). The disarm rides the same command, same class — a caller who could not arm never needs to
disarm. Under the hood it is update_merge_request(auto_merge_*); Layer 2 is untouched.

3. Under --json, every destructive command must name its target (--merge-request-id or
--branch), checked before anything is resolved. Every destructive kbagent command either
prompts or is told its target; --json has no prompt. Humans keep the active-branch fallback
(--merge-request-id/--id optional everywhere) and get a prompt on merge and arming.
Inverting --yes under --json was considered and rejected (zero precedent across 48 commands).

4. serve enforces the policy on every route (Depends(require_permission) — until now
only /auth/* did). Because the class is static, the route dependency is the whole check;
nothing is evaluated from the request body or a prior GET. PUT …/{id}/auto-merge is the
twelfth route.

5. One error handler, no per-command except. FeatureNotEnabledError (a ConfigError
with FEATURE_NOT_ENABLED) surfaces from the resolver behind every omitted id — reads included
— exactly where a copied branch.py idiom would flatten it. Pinned per command.

Renderers worth a glance

• diff checks the *_deleted flags before the table and recommends the --take: since
  feat(service): merge-request Layer 2 - lifecycle, derived status, conflict resolution (DMD-1899) #703 finding Phase 4: Agent context, doctor, CLAUDE.md, README #4 a null side yields zero per-path rows, so "production deleted it, dev
  changed it" would otherwise render as three empty sections and "No changes".
• MR_MERGE_CONFLICT renders the conflict list from details and appends list truncated —
  run merge-request conflicts when api_error_params_truncated is set (the list is bounded
  to 20 entries server-side of kbagent).
• Every wire-sourced string goes through escape(); the Phase-5 review caught a [/x] in a
  config id raising MarkupError after the rebase had landed — fixed everywhere,
  including the ad-hoc prints outside the renderer module.

Review already applied (Phase 5)

Three independent passes (Opus, Sonnet, /code-review); 28 findings, all in-scope ones
fixed with a pin — see the "apply the Phase-5 self-review round" commit for the full list. Notable: details were being dropped
from every merge-request error envelope; branch_from_id was null beside a payload saying
123; the auto-merge vocabulary was duplicated CLI vs router (now one service-owned validator); bool("false") on a
hand-edited isDisabled would have disabled a config on replace and returned 200; over
serve the MR 409s answered 502 with no details.

Deferred to #703 (Layer 2 design/refactor findings, too large for this PR's tail):
_classify_three_way missing both rows for nested-vs-parent edits; the or code is None
409 fallback; SOX-project reads reporting feature_enabled: false; the tuple return in
http_base._bound_error_params; post-merge cleanup being a third copy of BranchService's;
the try/finally client idiom vs the context manager.

Layer 2 follow-ups applied here

docs/merge-requests-layer2-followups.md (the non-blocking leftovers of #703, written after
Zajca's approval) routes its fixes through this PR. Done: F2 (classifier docstring + the
missing empty-envelope test; an empty envelope on either side now lands in warnings, so the diff
renderer says "no classification possible" instead of "the conflict cleared"), F3
(cleanup_skipped: true + branch_from_id_raw on a merge whose source branch id could not be
read; the renderer keys on the flag), F4 (the shared find_default_branch_id logs the skipped
non-numeric entry), F5 (detail carries feature_enabled -- free there -- and hint-next refuses
to recommend a write that cannot succeed; list stays feature-blind as L2 decided), F7
(truthiness sweep, positive reset assertion, long docstring line). Each item's State is recorded
in the doc. Not here: F6 (mapping cleanup project scope -- both call sites, standalone PR) and
F8 (a FORCE_COLOR test fragility on main).

Open decision for the reviewer/owner (F4): sync init exits 0 with an empty branches list
when no usable default branch is found -- pre-existing behaviour, left unchanged; the doc asks
whether that should be an error.

Still-open Layer 2 design findings (from the Phase-5 /code-review pass; not in the
follow-ups doc, too large for this PR): _classify_three_way produces no both row when one side
edits a nested key and the other replaces the parent; the or code is None 409 fallback treats a
code-less 409 as a conflict; SOX-project reads report feature_enabled: false; the tuple return in
http_base._bound_error_params; the post-merge cleanup being a third copy of BranchService's.
Worth a follow-up issue once this lands.

Zajca's review (2026-09-07), how each item landed

Must-fix 1 (_deleted_side_message None ordering — a missing side recommended --take ours, which resolves as DELETE): fixed, None checked first, no recommendation, two pins.
2 (hand-synced _REASON_MAX_LENGTH, unvalidated --external-id): MERGE_REQUEST_*_MAX_LENGTH
in constants.py, validated once in the service, both surfaces map INVALID_ARGUMENT.
3 (unescaped derived_state in the shared success renderer): escaped, pinned with a
server-supplied [/x]. 4 (get_merge_request_row without the feature pre-flight on the
explicit-id path): lazy pre-flight on a 403 only, happy path stays one GET, three pins.
5 (register() via a private Typer attribute, false precedent): app.command(name)(fn).

Design push-backs: exit 2 carrying a state-derived precondition and resolve escalating
too broadly are both resolved by decision 1 above — there is no state-derived condition left to
carry a code for, and resolve is destructive by command with the reasoning stated. Third copy
of the escalation rule: gone with the escalation. Scope (which parts of layer2.md describe
#703 vs this PR): the section above. --output OSError branch losing warnings: reordered.
Test classes named by provenance: regrouped by behaviour. Route-level escalation
coverage: every destructive route incl. the disarm now has a 403 test. The --timeout
question: yes, accepted — the merge job continues server-side and the project-wide merge lock
makes a premature retry answer MR_NOT_READY_TO_MERGE rather than start a second merge; now in
gotchas beside the no---timeout line.

Verification

• make check green: ruff, format, ty, skill-check, version sync, version gates
  (vNEXT tags on body lines), command-sync (279 commands), endpoints-check, error-codes,
  sentinel guards, loc-check (all six new/changed modules under 800 — merge_request.py was
  split at 829 into common/reads/writes), 6643 tests.
• New tests: ~100 CLI (test_merge_request_cli.py), ~25 router (test_server_router_calls.py),
  ~25 service; every destructive command/route has a real-engine denial test (exit 6 / HTTP 403). Escalations are tested against the real engine (--deny-destructive → exit 6 /
  HTTP 403), not a mocked check.
• E2E: written, cannot run yet. TestE2EMergeRequestLifecycle (branch → config on the
  branch → create → list/detail/conflicts via id and --branch → approve asserts the 422
  → bare --json merge exits 2 → merge → config in production → explicit teardown) is gated
  on the project feature: list answers feature_enabled: false and the suite skips with the
  one-time enable command in the reason. The E2E project does not carry
  branches-merge-requests today, and kbagent cannot provision one. Enable it once:
  kbagent feature project-add --project kbagent-e2e --feature branches-merge-requests.
  Not silent: the skip reason says exactly this.

Silent-drift surfaces (convention #17), all touched

CLAUDE.md command list · commands/context.py · commands-reference.md · gotchas.md
(since vNEXT) · keboola-expert.md matrix row · SKILL.md triggers + generated table ·
new merge-request-workflow.md · branch-workflow.md pointer · docs/web-server-endpoints.md
(generated). branch merge is deprecated with a conditional pointer — it works on
projects without the feature, so it is not a 1:1 replacement; behaviour unchanged,
deprecation in --json.

🤖 Generated with Claude Code

[linear-code]

DMD-1900

[Copilot]

🔵 Needs a closer look

It introduces a large cross-cutting feature (new CLI group + permission-sensitive REST surface + service behavior/shape changes) that warrants final human review despite strong test coverage.

Pull request overview

Adds the kbagent merge-request (alias mr) command group as a thin Layer 1 wrapper over MergeRequestService, plus a kbagent serve REST mirror that enforces permissions (including the “auto-merge arming/armed transitions are destructive” escalations). This extends the CLI’s Branches 2.0 (non‑SOX) lifecycle support end-to-end: listing/creating/updating MRs, review transitions, conflict inspection/diff, resolution, and merge.

Changes:

• Introduces the merge-request CLI group (with shared resolver/error-handling helpers) and deprecates branch merge with an explicit pointer to merge requests.
• Adds serve REST endpoints for merge requests, wires them into the app, and ensures MR merge-state 409s preserve details (conflict list) over HTTP.
• Applies small Layer 2 follow-ups (row-tier getter, unified warnings, diff resolution_candidate + warnings) and updates tests + documentation/agent surfaces accordingly.

File summaries

File	Description
tests/test_server_router_calls.py	Adds REST-router forwarding/validation/permission-escalation tests for /merge-requests/*.
tests/test_merge_request_service.py	Updates Layer 2 tests for warnings key and adds follow-up coverage (row getter, candidate/warnings, branch-id degradation logging).
tests/test_e2e.py	Adds feature-gated E2E lifecycle test for merge requests (create → inspect → merge → teardown).
src/keboola_agent_cli/services/merge_request_service.py	Adds auto-merge flag validation helpers, get_merge_request_row, unified warnings, diff resolution_candidate + diff warnings, and adjusted error-code blame for invalid branchFromId.
src/keboola_agent_cli/services/config_service.py	Fixes default-branch-id truthiness check (is not None) when listing config folder metadata.
src/keboola_agent_cli/services/base.py	Logs (non-silently) when an isDefault branch id is non-numeric and skipped.
src/keboola_agent_cli/server/routers/merge_requests.py	New serve router implementing REST mirror routes with permission enforcement + destructive escalations.
src/keboola_agent_cli/server/dependencies.py	Registers MergeRequestService in the server-side service registry.
src/keboola_agent_cli/server/app.py	Registers the router + OpenAPI tag and maps MR 409s to HTTP 409 while preserving error details.
src/keboola_agent_cli/server/_serve_command_map.py	Adds serve↔CLI telemetry mapping for merge-request endpoints (incl. serve-only by-branch resolver).
src/keboola_agent_cli/permissions.py	Adds merge-request operations + FLAG_ESCALATIONS and marks merge-request.by-branch as serve-only.
src/keboola_agent_cli/commands/transformation.py	Reuses centralized parse_json_arg helper instead of a local copy.
src/keboola_agent_cli/commands/merge_request.py	New Layer 1 read commands for the merge-request group (writes mounted from the split module).
src/keboola_agent_cli/commands/_merge_request_common.py	Shared resolver, destructive-under---json rule, armed escalation checks, and common output helpers for the merge-request group.
src/keboola_agent_cli/commands/_helpers.py	Adds parse_json_arg as a shared `JSON
src/keboola_agent_cli/commands/context.py	Documents merge-request group and notes branch merge deprecation (since vNEXT).
src/keboola_agent_cli/commands/branch.py	Marks branch merge deprecated and adds a structured deprecation field (plus human warning).
src/keboola_agent_cli/cli.py	Wires merge-request group and hidden alias mr; registers MergeRequestService in CLI context.
plugins/kbagent/skills/kbagent/SKILL.md	Updates skill triggers/decision table to include merge requests and deprecates branch merge entry.
plugins/kbagent/skills/kbagent/references/merge-request-workflow.md	New workflow reference for merge-request lifecycle, conflicts, auto-merge safety, and --json targeting rule.
plugins/kbagent/skills/kbagent/references/gotchas.md	Adds merge-request gotchas (auto-merge is destructive; --json merge requires explicit target; other sharp edges).
plugins/kbagent/skills/kbagent/references/commands-reference.md	Adds merge-request section + documents branch merge deprecation.
plugins/kbagent/skills/kbagent/references/branch-workflow.md	Updates branch workflow to point to merge-request for CLI-based merges.
plugins/kbagent/agents/keboola-expert.md	Updates agent matrix with merge-request guidance and safety notes.
docs/web-server-endpoints.md	Regenerates endpoint inventory and adds merge-requests router section.
docs/merge-requests-layer3.md	Adds Layer 3 “as-built” documentation for merge-requests client surface and wire contract.
docs/merge-requests-layer2-followups.md	Adds/updates Layer 2 follow-up tracking document inherited by Layer 1.
CLAUDE.md	Updates command inventory and merge-request operational notes (since vNEXT).

Review details

• Files reviewed: 34/34 changed files
• Comments generated: 0
• Review effort level: Lite

---

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

[Copilot]

🟡 Changes recommended

Diff-envelope handling, Rich output safety, file encoding, and E2E coverage contain unresolved correctness gaps.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 34/34 changed files
• Comments generated: 8
• Review effort level: Balanced

[martinsifra]

Confirmed and fixed in 2042c3b -- setup now asserts merge-request list succeeds and skips only on feature_enabled: false; a crash or auth regression is a failure, as the class docstring promised.

[martinsifra]

Confirmed and fixed in 2042c3b -- _emit_warnings escapes each warning before it reaches Rich. Pinned by test_warning_text_with_markup_does_not_crash (a [/x] in a post-merge warning).

[martinsifra]

Confirmed and fixed in 2042c3b -- the path is escaped before the hint-next line; same test covers a [x] in the filename.

[martinsifra]

Confirmed and fixed in 2042c3b -- a side missing any required content key (name/rows/configuration/isDisabled) is now unclassifiable on EITHER side, and _diff_warnings names the holes for both. One shared _envelope_holes criterion feeds the classifier, the resolution candidate and the warnings so they cannot drift. Pinned by test_holed_theirs_envelope_yields_no_rows_and_a_warning (before: a fabricated 'theirs removed configuration.limit' row).

[martinsifra]

Confirmed and fixed in 2042c3b -- the E2E now manufactures a real conflict (config in production, branch inherits it, both sides change it) and walks all eleven commands: create, update, list, detail, conflicts (via --branch), diff + --output, resolve --take ours, request-review, request-changes, approve, the bare --json merge exit 2, merge, and the production content check. Still feature-gated -- the E2E project lacks branches-merge-requests today; the skip reason carries the one-time enable command.

[martinsifra]

Confirmed and fixed in 2042c3b -- asserts error.code == API_ERROR with 422 in the message (the 422 rides the generic http_base path, so that is the precise shape).

[martinsifra]

Confirmed and fixed -- the rationale now says both 409 shapes carry a machine code but the codes are endpoint-specific, so the mapping lives in the service (http_base maps by status alone). The L2 RFC lives on ms/merge-requests-rfcs (76a2adb); this branch's first commit is rebuilt from it.

[zajca]

Review: merge-request Layer 1 (DMD-1900)

Read the whole diff, walked every convention-#17 surface, and ran the full gate locally on
ms/dmd-1900/cli-layer-1. Everything is green: ruff, ruff format, ty (1 pre-existing
downgraded unresolved-import warning), skill-check, version-check, version-gate-check
(27 vNEXT awaiting the release PR — correct for a feature PR), command-sync (279 commands),
endpoints-check, error-codes, sentinel-guards, loc-check (all four new command modules inside
budget), 6697 passed / 184 skipped. No pyproject.toml bump and no changelog.py entry —
also correct for a feature PR.

The design work here is strong, and the three decisions you asked reviewers to push on hold up
under attack. Two things I would fix before merge, both small; the rest is non-blocking.

---

1. _deleted_side_message branch ordering misreports the state — and the --take it recommends silently deletes

commands/_merge_request_render.py:361-383

The None checks sit after the is True and not <other> pair, so they are unreachable for the
combinations that matter. Reproduced against the branch:

ours=None   theirs=True   -> "Production deleted this configuration; your branch changed it."
                             "Resolve with `--take delete` (drop it) or `--take ours` (keep your version)."
ours=True   theirs=None   -> "Your branch deleted this configuration; production changed it."
                             "... or `--take theirs` (keep production's version)."

ours_deleted is None means the config is not in the branch at all (merge_request_service.py:963:
bool(ours.get("isDeleted")) if ours is not None else None) — so "your branch changed it" is false, and
there is no "your version" to keep. It gets worse one layer down: resolve_conflict
(merge_request_service.py:1192-1197) collapses --take ours on a None side into
resolution = "delete". So both options the user is offered as opposites execute the same
destructive resolution, while the sentence presents one of them as the way to preserve their work.
Symmetric for the ours=True, theirs=None case.

The module docstring already states the intent — a None flag should "render defensively, no
recommendation" — the ordering is what defeats it. Moving the two is None branches above the pair
fixes it. Nothing currently covers None/True: the only ours_deleted fixture in
test_merge_request_cli.py:676 is True.

Given --take on this path is destructive and irreversible from the CLI's point of view, this is the
one item I would not merge without.

2. _REASON_MAX_LENGTH = 1000 is hand-synced across two surfaces, with no service-side guard

commands/_merge_request_writes.py:55 and server/routers/merge_requests.py:42

This is precisely the drift hazard the PR argues against for the auto-merge vocabulary — and solves
correctly there, by putting validate_auto_merge_flags / arms_auto_merge in the service so "the CLI
and the serve router cannot drift". The reason cap got the opposite treatment:
MergeRequestService.request_changes (merge_request_service.py:668-685) only documents the cap in
its docstring and validates nothing, so the two hand-copied literals are the only guards. The router
test name says it out loud — test_merge_request_request_changes_caps_reason_like_the_cli.

It is also the only hardcoded *_MAX_LENGTH anywhere under commands/, server/routers/ or
services/ in the repo, against CONTRIBUTING's "no magic numbers — all configuration values go in
constants.py".

One constant in constants.py, validated once in request_changes, both surfaces mapping the
resulting error, removes the class of problem rather than the instance.

Same class, worth deciding together: --external-id help (_merge_request_writes.py:85) advertises
"max 255 chars" and nothing validates it — no client, service, CLI or router check. Either validate
both consistently or describe both as server-enforced; documenting one cap and enforcing the other in
two places is the confusing middle.

---

Non-blocking

3. Test classes are named after the review that found the bug, not the behaviour they protect.
test_merge_request_cli.py:1422 TestOpusReviewFollowUps, :1606 TestLayer2Followups, :1647
TestCopilotBalancedFollowUps, with inline # M1: / # F3: round references. The repo's comment
convention is "describe state or reason, never the change", and these scatter behaviour by provenance:
the Rich-markup escaping test and the UTF-8 --output test live under "CopilotBalancedFollowUps"
rather than beside the renderer / TestDiffOutputErrors tests. Six months from now, nobody looking for
"does the merge renderer key on cleanup_skipped?" will look in TestLayer2Followups. The
behaviour-named classes to fold them into already exist.

4. The E2E suite is good and has never run. TestE2EMergeRequestLifecycle is a genuinely thorough
16-step scenario (manufactured conflict, both resolution paths, the 422, the bare---json merge exit 2,
explicit teardown) — but it skips on feature_enabled: false and the E2E project has no
branches-merge-requests, so all eleven commands ship with zero real-API verification. The PR is
honest about that, so this is not a hidden gap. My concern is the unexercised assertions rather than the
policy: step 4 asserts created["branch_from_id"] == branch_id, and create never passes through
_stamp_target (_merge_request_writes.py:190-191 sets only merge_request_id /
resolved_from_branch), so that key must come from the service payload. Since enabling the flag is the
one-time super-admin command the PR already documents, running the suite once before merge converts
"written" into "verified" cheaply — and is the only way the wire shapes get checked at all.

5. Route-level escalation coverage is partial. test_server_router_calls.py proves the armed-MR
escalation for request-review and the arming escalation for create — approve, resolve and
update --auto-merge-strategy ride the shared helpers with no route-level 403 test. Normally I would not
mention it, but this PR's own argument is that without the HTTP checks the classification is decorative,
which makes coverage here load-bearing rather than decorative itself.

6. Two small consistency notes.

• The hoisted parse_json_arg (commands/_helpers.py:79) is now shared, but the two call sites map its
  ValueError differently: INVALID_ARGUMENT / exit 2 in the merge-request group,
  INPUT_ERROR at transformation.py:352. One helper, two codes for the same malformed @file.
• The generated SKILL.md table gained 22 rows because the hidden mr alias is enumerated beside
  merge-request. Pre-existing generator behaviour (sl already contributes 33 rows), so not this PR's
  bug — but a hidden=True filter in generate_skill_table.py would be a cheap follow-up for an
  agent-facing table.

---

Verified sound (so it does not get re-litigated)

• The disarm does not escalate, on both surfaces, with tests pinning it
  (test_merge_request_cli.py:818 test_disarmed_none_is_NOT_destructive, test_server_router_calls.py:2839). This was the easiest thing in
  the design to get backwards and it is right.
• FLAG_ESCALATIONS really is enforced, not just declared: permissions.py:_matches_pattern consults
  it before OPERATION_REGISTRY, so cli:destructive and --deny-destructive do cover the five escalated
  strings. Escalation tests run against the real PermissionEngine (exit 6 / HTTP 403), not a mocked check
  — which is the difference between testing the rule and testing the assertion about the rule.
• The mr alias cannot bypass policy: the group callback hardcodes
  check_cli_permission(ctx, "merge-request") (merge_request.py:80), so kbagent mr merge evaluates
  merge-request.merge. Worth stating because registering the same Typer app twice is exactly where an
  alias-shaped hole would live.
• --state is case-insensitive end to end (command pre-check, service filter, router) — the obvious
  "validated lowered, filtered raw" bug is not there.
• Every convention-#17 surface is updated, and the gotchas / workflow / AGENT_CONTEXT prose is
  unusually good — the "WHAT MAY HAPPEN WITHOUT A HUMAN SAYING SO" block in CLAUDE.md is the right
  amount of alarm in the right place.

One question, not a change request

merge blocks up to MERGE_JOB_MAX_WAIT (600 s) with no --timeout, and both STORAGE_JOB_TIMEOUT and
MR_NOT_READY_TO_MERGE carry retryable=True. For a non-idempotent destructive operation that tells a
timed-out caller to retry into a merge still in flight, which then hits the project merge lock. The
timeout message does say the job continues server-side, so it is survivable, and both flags are
pre-existing Layer 2/3 code rather than this PR's — but workspace load (#687) grew a --timeout for
exactly this shape. Is the answer "yes, accepted, and the lock makes the retry harmless"? If so it
belongs in the gotchas entry beside the existing "no --wait/--timeout" line.

---

Fix 1 and 2 and this is ready from my side. Nice work — the auto-merge analysis in particular is the kind
of thing that only gets found by someone actually reading the backend, and the group is safer for it.

[zajca]

Addendum to my review — three more findings, one of them must-fix

A second pass (parallel security / silent-failure / design passes) surfaced three things my first
review missed. I re-verified each against the branch before writing it up — the first one I
reproduced by running the code.

---

3. (must-fix) _print_row_success interpolates derived_state into Rich markup unescaped

commands/_merge_request_common.py:391-392

state = str(d.get("derived_state") or d.get("state") or "").replace("_", " ")
c.print(f"[bold green]Success:[/bold green] {headline} -- state: {state}")

state is the one interpolation in this module that does not go through escape(), and it is
wire-controlled: derive_state() (services/merge_request_service.py:181-183) returns the
backend's mr.get("derivedState") verbatim when present — the DMD-1988 server field, no
vocabulary check, no escaping — and that lands in result["derived_state"], which is exactly what
this renderer reads.

Reproduced:

$ derived_state = "in_review [/x]"
>>> rich.errors.MarkupError: closing tag '[/x]' at position 80 doesn't match any open tag

_print_row_success is the human-mode success renderer for create / update / request-review /
approve / request-changes / resolve — every write except merge, which has its own
(correctly-escaped) renderer. So the crash lands after the mutating call already succeeded,
including on approve / resolve against an already-armed MR: the auto-merge countdown has
started, and the operator sees a traceback instead of the success line and the
_warn_armed warning that was supposed to tell them so. --json is unaffected.

One-line fix:

state = escape(str(d.get("derived_state") or d.get("state") or "").replace("_", " "))

Worth calling out because the PR description says this class of bug was "fixed everywhere,
including the ad-hoc prints outside the renderer module" — the sweep just missed the shared helper
that all six write commands funnel through, which is the highest-traffic instance of it. Same
root cause as the [/x]-in-a-config-id case Phase 5 caught; the module docstring (:9-15) states
the invariant this violates.

4. get_merge_request_row skips the feature pre-flight — and rule #2 pushes --json callers straight onto that path

services/merge_request_service.py:473-479

It is the only method in MergeRequestService without a _require_merge_requests_feature(client)
call — every sibling has one (create_merge_request :576, update_merge_request :626,
request_review :651, approve :662, request_changes :681, merge :742, resolve_conflict
:1168, and find_merge_request_for_branch's no-match path :445; 8 call sites total). That is a
deliberate, tested tradeoff at its own call site
(test_row_by_id_is_one_get_and_nothing_else) — the gap is that it was not carried through to
the callers that made the row fetch unconditional:

• conflicts --merge-request-id N (merge_request.py:206, need_row=True)
• resolve --merge-request-id N (_merge_request_writes.py:557, need_row=True always)
• request-review / approve --merge-request-id N (_merge_request_writes.py:313,
  escalate_when_armed=True forces it)
• merge --merge-request-id N interactive, without --yes (_merge_request_writes.py:451)
• and over serve, _escalate_if_armed (routers/merge_requests.py:76) for the same three
  transitions

On a feature-less or SOX project each of these 403s in the row fetch first, so the operator gets
the bare role-denial-shaped error the class docstring (:325-327, :366-368) explicitly says only
a client-side check can word properly — and goes hunting for a token/RBAC problem instead of
enabling branches-merge-requests. The correctly-guarded write method they were about to call
never runs.

The interaction is what makes this worth fixing rather than documenting: decision #2 requires
--json callers to pass an explicit --merge-request-id for destructive ops, and the explicit-id
path is precisely the one that loses the friendly error. resolve --json --merge-request-id N —
the recommended scripting form — gets the raw 403. (merge --json --merge-request-id N escapes it
only because need_row=False there, so the shape is inconsistent between two commands in the same
group.)

Cheapest fix that keeps the "one GET" property: check the feature lazily — only when the GET
itself 403s — mirroring what find_merge_request_for_branch already does on its no-match path.

5. register() uses a private Typer attribute, and the precedent cited for it is not real

commands/_merge_request_writes.py:601-603

app.registered_commands.extend(writes_app.registered_commands)

registered_commands is Typer internals, and this is the only use of it anywhere in src/ or
tests/ (1 grep hit, repo-wide). The docstring at :7-8 justifies it as "precedent:
_storage_describe.register" — but _storage_describe.register (_storage_describe.py:32-36)
declares its commands with the public @app.command() decorator inside register(app):

def register(app: typer.Typer) -> None:
    """Mount the describe commands onto ``app`` (the ``storage`` Typer group)."""

    @app.command("describe-bucket", rich_help_panel=_DESCRIBE)
    def storage_describe_bucket(...):

So both halves are off: the mechanism is not the same, and the file cited as precedent is an
example of the safer alternative. The docstring is the part I'd insist on — it is what will tell
the next person this pattern is established here when it isn't, and a private-list mutation is
what breaks silently on a Typer bump. Switching to the decorator form gets the same flat mount
with the public API.

---

Non-blocking design push-backs (decisions, not defects)

On decision #2 — exit 2 is carrying a state-derived precondition. Your own test names it:
test_armed_implicit_target_under_json_exits_2_after_resolution
(tests/test_merge_request_cli.py:1055). The identical invocation succeeds while the MR is
unarmed and exits 2 once somebody else arms it — nothing about the command line changed. Per
CONTRIBUTING, 2 is a usage error, and a script currently cannot tell "fix your flags" from "your
target became dangerous behind your back" except by parsing the message text. The rule itself I
think is right; I'd keep exit 2 if you prefer the precedent and just give the state-derived case
its own ErrorCode so callers can branch on it.

On decision #1 — resolve's escalation is the over-broad one. The part you flagged as
possibly over-broad is actually fine: update's non-arming fields genuinely cannot move an MR
toward approved, because the required-approvals count is decoupled project metadata
(docs/merge-requests-notes.md:107-112), not derived from the reviewer list — so gating update
purely on the value it is about to write is correct. resolve is where it bites: it escalates on
any armed MR regardless of distance from mergeable, so a fresh MR with five conflicts and two
required approvers needs destructive rights to fix conflict #1 the moment anyone arms it. A
"walk the conflicts, never merge" agent policy stops being expressible under
--deny-destructive — which pushes teams to drop --deny-destructive for MR work entirely and
lose it for the ops it is well-suited to. Since FLAG_ESCALATIONS keys are already just
operation strings, a narrower trigger (escalate only on the last unresolved conflict) or a
distinct key for resolve-on-armed both fit the existing engine.

Third copy of the escalation rule. routers/merge_requests.py:66-79 hand-reimplements
_escalate_if_armed with the same intent as the CLI's version, different signature, no shared
code. Byte-for-byte correct today (I checked the key strings) — but it is the same class as my
finding #2 about _REASON_MAX_LENGTH: safety-critical logic maintained by hand in two files, and
the next narrowing of the policy (per the point above) has to land in both or the surfaces
diverge silently. Worth extracting alongside whatever you do about #2.

Scope, one specific ask. docs/merge-requests-layer2.md and -layer2-followups.md (422 lines)
do not exist on origin/main — this branch is writing the Layer 2 design doc retroactively, for a
service that shipped separately in #703, while also patching that same service by +206/-26. I'm
not asking for a split. I am asking for a line in the PR description saying which parts of
layer2.md describe what #703 already shipped versus what this PR changes, because nobody
reviewing "the CLI layer" is positioned to catch drift between that doc and the merged service.

One sub-threshold note, since it is cheap. diff --output PATH's OSError branch
(merge_request.py:301-306) exits before formatter.output / _emit_warnings, so a diff that
also carried an envelope-hole warning loses it when the file write fails. Narrow double-failure
window, mentioned only because it is a one-line reorder.

---

Additionally cleared (so it does not get re-checked)

The parallel passes independently confirmed, beyond what my first review covered:

• No destructive-classification bypass. All 12 route permission keys resolve to real,
  correctly-classified OPERATION_REGISTRY entries — none fall through to the fail-closed
  write default. Every escalation check provably precedes its service.* call (create 141-153,
  update 235-244, _transition 315-327, resolve 561-569). --auto-merge-at without a strategy is
  a hard exit 2, not a silent accept. request-changes not escalating is structurally right — it
  moves the MR away from approved and deletes approvals.
• _stamp_target never stamps a stale fact: setdefault cannot overwrite a service-computed
  key, and the branch_from_id fallback chain matches the shape each result actually carries
  (nested branches.branchFromId for row-shaped results, flat branch_id for the diff).
• _emit_warnings really is on every path, detail's feature_enabled is False early return
  included (it returns after the emit at :169).
• No swallowed total failure: the 409 remap and STORAGE_JOB_FAILED both propagate; only the
  genuinely best-effort post-merge cleanup degrades to warnings[] + cleanup_skipped, keeping
  exit 0 — which is the correct call.
• No token or secret handling in any new file (zero grep hits).

Revised ask: findings 1, 2 and 3 before merge; 4 and 5 I'd take here too since both are a few
lines, but I won't hold the PR on them if you'd rather issue them. Everything under "design
push-backs" is a decision for you — an answer in the thread is enough, no code required.

[keboola-pr-reviewer-bot]

Verdict: needs_human (risk 4/5) · profile keboola-mcp-server

PR exceeds size cap (4214 source LOC across 30 source files; 0 test/fixture LOC across 0 files excluded from the cap).

[martinsifra]

Thanks — all five must-fixes are in (2e8dc0e), and the two design push-backs ended up settling something bigger than either of them.

What changed since your review — one model, not two:

You pointed at the same hazard from two sides: a permission that depends on a GET (finding 4 / "third copy"), and an exit 2 that depends on state nobody typed (push-back on decision #2). Both came from the escalations being conditional — on a flag's value, or on whether the MR happened to be armed. We dropped the conditions entirely. Destructive is now a static property of the command: request-review, approve, resolve, merge and the new auto-merge are destructive always (each moves an MR toward or into production — on the 0-approval default request-review lands directly in approved); create/update/request-changes are writes. FLAG_ESCALATIONS is back to its single original entry, the transitions do no GET before writing, the router's check is the route dependency alone, and the --json explicit-target rule runs before any network call for every destructive command.

Auto-merge is its own command (merge-request auto-merge --strategy immediately|scheduled|none), destructive, prompting when it arms; create/update no longer take a flag for it. The disarm rides the same class on purpose — a none special case would be a value-dependent class again, and a caller who could not arm never needs to disarm.

Per item:

1. _deleted_side_message — None checked first, no recommendation on a missing side; two pins.
2. MERGE_REQUEST_REASON_MAX_LENGTH / MERGE_REQUEST_EXTERNAL_ID_MAX_LENGTH in constants.py, validated once in the service (INVALID_ARGUMENT → exit 2 / HTTP 400), no per-surface literals left.
3. derived_state escaped in _print_row_success; pinned with a server-supplied [/x].
4. get_merge_request_row runs the feature pre-flight lazily on a 403; happy path stays one GET; three pins. (It is now only read where something is rendered from the row — the merge prompt and conflicts' branch_from_id — never to decide permissions.)
5. register() uses app.command(name)(fn); docstring corrected.

Design push-backs: A (state-derived exit 2) and B (resolve over-broad) — resolved by the model change rather than a new error code or a narrower trigger; resolve is destructive by command, and the over-breadth on a 2-approval project is stated in the RFC with the reason (the required count is unreadable with a Storage token, DMD-1969; a class flipping on a number the CLI cannot read would be the state condition again). Third copy — gone with the escalation. Scope — the PR description now has a section separating what in layer2.md describes #703 from what this PR changes. --output OSError — warnings emitted before the exit. Test naming — regrouped by behaviour. Route-level coverage — every destructive route incl. the disarm has a 403 test. --timeout — yes, accepted: the job continues server-side and the project merge lock makes a premature retry answer MR_NOT_READY_TO_MERGE, never a second merge; that is now in gotchas beside the no---timeout line.

E2E still cannot run (the E2E project lacks branches-merge-requests); the one-time enable is documented and on Martin's list before merge.

[zajca]

Actionable findings from the automated review.

[zajca]

Reviewed by Opus.

_envelope_holes treats a required content key as present whenever it is a key of the envelope (key not in envelope), but the replace guard in resolve_conflict rejects a body where the key is present and null (key not in body or body[key] is None), and additionally rejects an empty/whitespace name.

So for a side whose envelope carries an explicit "name": null (the diff envelope declares name nullable — the guard's own comment says so), _envelope_holes returns []. Consequently:

• _resolution_candidate composes a candidate instead of returning None,
• _diff_warnings emits no warning,
• merge_request_diff --output FILE happily writes the file and the hint line tells the user to resolve --resolved @FILE,
• resolve_conflict then fails that body with ConfigError("A resolved body must spell out the full replaced content (rebase REPLACES): missing name.") — blaming the caller for a file kbagent itself wrote.

This is exactly the drift _resolution_candidate's docstring says the shared _DIFF_CONTENT_KEYS constant prevents ("a prefill built anywhere else that dropped description when null … would produce a file kbagent itself then refuses").

Failure scenario. Backend returns a diff whose ours.diff carries "name": null with all other required keys present. kbagent merge-request diff --component-id X --config-id Y --output cand.json exits 0, emits no warning, writes the candidate and prints Next: edit cand.json, then `merge-request resolve ... --resolved @cand.json` . Running that resolve exits 5 with "missing name".

Verified against the head tree:

ours = {"isDeleted": False, "diff": {"name": None, "rows": [], "configuration": {},
                                     "isDisabled": False, "description": "x"}}
_envelope_holes(ours)               -> []
_resolution_candidate(ours)         -> {'name': None, 'description': 'x', 'configuration': {},
                                        'isDisabled': False, 'rows': []}
resolve_conflict guard on that body -> missing ['name']  (ConfigError, exit 5)

The same presence-only criterion also feeds _classify_three_way.classifiable(), so such a side is classified against a None value rather than being reported as unclassifiable.

Fix: make _envelope_holes mirror the guard — treat a present-but-None required key (and a blank name) as a hole — so the candidate is suppressed and the reason lands in warnings[] instead of in a file the user is told to edit.

[zajca]

Reviewed by Opus.

--reason gets an explicit CLI pre-check in merge_request_request_changes (line 355) whose comment states the intent: "this pre-check exists only so the flag error carries exit 2 like every other bad flag". --external-id has no such pre-check on create/update, so an over-cap value reaches MergeRequestService._too_long, which raises KeboolaApiError(error_code=INVALID_ARGUMENT). _handle_error routes that through map_error_to_exit_code, and INVALID_ARGUMENT is not in any of its branches — it falls through to the generic return 1.

Verified on the head tree:

map_error_to_exit_code(KeboolaApiError(error_code=ErrorCode.INVALID_ARGUMENT)) -> 1

Over serve the same code maps correctly (_CALLER_REFUSAL_CODES -> HTTP 400), so only the CLI surface is inconsistent — which undercuts the PR's own stated resolution of review item 2 ("both surfaces map INVALID_ARGUMENT"). A --json script cannot tell "you passed a bad flag" (2) from "the operation failed" (1), and the two sibling caps in the same group disagree with each other.

Failure scenario. kbagent --json merge-request create --branch 123 --title t --external-id <256 chars> exits 1 with INVALID_ARGUMENT, while kbagent --json merge-request request-changes --id 7 --reason <1001 chars> exits 2 with the same error class. A wrapper branching on exit 2 for usage errors misclassifies the first as a transient failure and retries it.

Fix: add the same len(external_id) > MERGE_REQUEST_EXTERNAL_ID_MAX_LENGTH -> _usage_error(...) pre-check to merge_request_create and merge_request_update (or map INVALID_ARGUMENT to exit 2 in map_error_to_exit_code, which would also cover _require_in_conflict_set and the absent-branchFromId case this PR re-coded to INVALID_ARGUMENT).