docs(rfc): rewrite the Layer 1 RFC after review [DMD-1900]

[martinsifra]

RFC for the kbagent merge-request command group (Layer 1, DMD-1900). Docs only — one file,
one commit, no code.

⚠️ Merge order — do not just click Merge

The intended order is L2 → main first, then L1 → main. This PR is based on
ms/dmd-1899/cli-layer-2 only so the diff shows L1's changes alone — merging it as-is would
fold Layer 1 into the Layer 2 branch, which is not the plan.

1. merge #703 (Layer 2) into main
2. retarget this PR to main
3. merge this

Why a second RFC PR

#708 landed the first draft. Two independent review
passes (Opus + Sonnet) and a design session then changed several decisions materially — enough
that a patch-on-top would have been unreadable, so the document is rewritten. The base already
contains v1, so this diff is the delta.

The command surface did not change. What changed is what may happen without a human saying
so, plus a batch of factual corrections.

The decisions that need sign-off

1. Auto-merge is a destructive act, and classifying only merge as destructive was theatre.

autoMergeStrategy is not metadata. A background scheduler
(AutoMergeScheduleProvider.php:24-26) selects every approved MR armed with it and runs it
through the same MergeProcessor the merge endpoint uses, under a system token
(AutoMergeCandidateRepository.php:44-47, AutoMergeTickHandler.php:86). It polls — the approve
path has no hook — and a blocked tick retries indefinitely. Order does not matter: arming
before approval fires when the state arrives. So create --auto-merge-strategy immediately
followed by request-review ends in a production merge with merge never called, and nothing
kbagent returns reports it.

Consequently create / update escalate to destructive when they arm it, and
request-review / approve / resolve escalate on an already-armed MR. The dividing line:

Confirmation sits where a human chooses an irreversible outcome. Escalation sits wherever
one is caused.

--auto-merge-strategy none never escalates — it is the disarm, and escalating it would let
--deny-destructive lock the hazard in place.

2. One rule replaces a per-command safety table.

When an invocation resolves to destructive, --json requires an explicit target.

Derived from a house rule that holds without exception across the repo: implicit branch
resolution selects the scope; the target is always named on the command line.
storage delete-table takes the active branch but requires --table-id; branch delete
requires --branch because there the branch is the target. A bare
kbagent --json merge-request merge --project acme would satisfy neither of the repo's two
destructive shapes — it neither prompts nor names anything, and would be the first kbagent
command where nothing on the command line identifies what gets destroyed.

Humans keep the full active-branch fallback and get a prompt; scripts, which received the id in
the previous call's JSON, name it. Inverting --yes under --json was considered and
rejected — zero precedent across the 48 commands carrying that flag, and a familiar flag
with reversed semantics is worse than a rule that names the target.

3. --merge-request-id (short alias --id), not --mr-id.

Measured convention: a bare noun is the context you work in (--project 206×, --branch 94×,
--model 23×); --<noun>-id is the object you act on (20 flags, ~170 uses). An MR is an
object, not a context. --mr-id abbreviated the noun, which nothing else in the CLI does.

4. Every serve route declares require_permission.

Today only auth.py does; the other ~30 routers ignore the engine. Without it the entire
classification above is decorative for serve callers.

What the reviews caught

• The module split was justified with the wrong metric — output.py is at 679 code lines
  of a 1000 ceiling, not the 1013 raw lines cited. CONTRIBUTING measures code lines and warns
  about exactly this mistake. The split still stands, on qualitative grounds.
• --reviewer-id names a real foot-gun but pointed at the wrong mechanism: on this stack
  typer.Option(None, …) yields None correctly; the danger is the typer.Option([], …) style
  live at commands/agent.py:862, which yields [] and silently clears the reviewer set.
• Rich markup escaping was missing entirely — an MR titled Fix [bold] parsing mangles the
  table, an unbalanced [/] raises MarkupError. Ten command modules already import escape.
• The mandatory CLI-layer test file was absent from the plan. Since E2E is unresolved, it is
  the only automated coverage this work will have.
• Plus: --state/--take should pre-validate to exit 2 (every peer flag does) rather than
  reaching the service at exit 5; plain ConfigError had no exit-code story;
  resolve --take delete --change-description silently drops the text
  (rebase_config_delete has no such parameter); update with no fields is a server-side
  no-op; serve wiring steps and the by-branch route-order hazard; and branch merge also
  unconditionally resets active_branch_id, so calling it a harmless URL builder misleads.

Not found by either review: the wire field list was wrong three ways — createdAt exists,
mergerName is nested in merge{}, and autoMergeStrategy/autoMergeAt are response fields
too. That surfaced only by reading MergeRequestResponseProvider.php:86-117 directly. The RFC
now deletes its field list rather than fixing it: merge-requests-notes.md is the authority
and this document must not carry a second copy.

Cross-layer

Seven findings surfaced while writing this went to Layer 2 and were applied in 07daa50, which
is in this PR's base. Two claims in the RFC were aspirational when written and are now simply
true: get_config_diff derives the branch from the merge request, and _enrich_row carries
allowed_actions on every return.

Open

E2E is deliberately unsettled and the RFC says so in place. No project carries
branches-merge-requests, kbagent cannot provision one (ManageClient has no project create;
Connection's suite makes its own), and the happy path necessarily merges into production. The
proposed path — enable the feature on the existing E2E project via kbagent feature project-add, gate with a FEATURE_NOT_ENABLED skip meanwhile — is recorded, not decided.

Verification

Every file:line citation in the document was re-checked against the working tree and against
the local keboola/connection checkout. No code changed, so there is nothing to run — but note
that an RFC is a plan: nothing here is enforced by CI until the commands exist.

Worth pushing back on

The three judgement calls, in order of how much they would cost to reverse later:

1. the --json explicit-target rule for destructive invocations (an ergonomic tax on scripts)
2. escalating request-review / approve / resolve on an armed MR — deliberately
   conservative, because the required-approvals count is unreadable with a Storage token
   (DMD-1969), so it escalates even where the
   operation would not yet merge
3. shipping the serve router in the same PR as the commands rather than as a follow-up

🤖 Generated with Claude Code

[linear-code]

DMD-1900